fix(GPO): make default gas tip configurable #1096
Conversation
WalkthroughThe changes in this pull request primarily focus on the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
eth/gasprice/gasprice.go (1)
198-205: LGTM! Well-documented change with clear rationale.The comments effectively explain why increasing the minimum tip to 100 wei is necessary:
- To avoid txpool transaction replacement issues
- To handle integer arithmetic rounding
- While maintaining negligible cost impact
Consider adding a code comment with an example of the rounding issue, e.g.:
// - txpool commonly prevents replacing transactions with the same tip cap. // - A 1 wei tip may fail to increase (e.g., 1.1x rounds down to 1 wei) due to integer arithmetic. +// Example: 1 wei * 1.1 = 1.1 wei, which rounds down to 1 wei // 2. Negligible cost:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
eth/gasprice/gasprice.go(1 hunks)eth/gasprice/gasprice_test.go(1 hunks)params/version.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- params/version.go
🔇 Additional comments (2)
eth/gasprice/gasprice_test.go (2)
216-216: LGTM! Test updated to match new minimum tip cap.
The test cases comprehensively cover both pre-Curie and post-Curie scenarios with various congestion levels, ensuring the new minimum tip cap of 100 wei is correctly applied.
Line range hint 198-205: Verify impact on dependent code.
Let's check if any other parts of the codebase make assumptions about minimum tip values.
✅ Verification successful
The initial results show that SuggestTipCap is used in multiple files, and we found some references to tip values. Let's examine the actual implementations and usages to ensure there are no assumptions being violated by this change.
Based on the search results, I can now generate the final response:
No impact on dependent code from minimum tip change
The codebase analysis shows that the minimum tip value of 100 wei is safely implemented:
- The minimum tip is only applied in non-congested conditions after Curie (EIP-1559)
- Other parts of the codebase either:
- Use their own tip logic (e.g., SimulatedBackend uses fixed 1 wei tip)
- Rely on the oracle's output without making assumptions about minimum values
- The change is well-documented with clear rationale about txpool protections and negligible cost impact
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential hardcoded minimum tip values or tip-related tests
rg -A 2 -B 2 "tip.*wei|wei.*tip" --type go
# Search for any other gas price oracle tests that might need updates
fd -e go -x grep -l "SuggestTipCap"
Length of output: 3315
Script:
#!/bin/bash
# Let's examine the implementation and usage of SuggestTipCap across files
ast-grep --pattern 'SuggestTipCap($$$)'
# Let's also check for any direct references to tip values
rg -A 3 -B 3 '(minimum|min).*tip' --type go
Length of output: 40739
colinlyguo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
eth/gasprice/gasprice.go (4)
43-48: Enhance code comments for clarity and consistencyThe added comments explaining the reasons for setting
DefaultGasTipto 100 wei are informative. However, consider reformatting them for better readability and consistency with Go's commenting style.Apply this diff to improve the comments:
// DefaultGasTip is set to 100 wei instead of 1 wei for the following reasons: -// 1. It prevents issues with very low tip values (e.g., 1 wei): -// - Transaction pools commonly prevent replacing transactions with the same tip cap. -// - Extremely low tips like 1 wei may fail to increase due to rounding in integer arithmetic of SDK implementations (e.g. 1 wei * 1.5 = 1 wei). -// 2. The cost of gas tip 100 wei is negligible compared to base fee normally. +// +// 1. Prevents issues with very low tip values (e.g., 1 wei): +// - Transaction pools may prevent replacing transactions with the same tip cap. +// - Extremely low tips like 1 wei may not increase due to rounding errors in SDK implementations (e.g., 1 wei * 1.5 = 1 wei). +// 2. The cost of a gas tip of 100 wei is negligible compared to the base fee. //
61-61: Add documentation for the newDefaultGasTipfieldThe
DefaultGasTipfield in theConfigstruct lacks a comment explaining its purpose. Adding a comment will improve code readability and maintainability.Add a comment above the
DefaultGasTipfield:+ // DefaultGasTip is the default minimum tip cap to use after the Curie upgrade (EIP-1559). DefaultGasTip *big.Int `toml:",omitempty"` // Default minimum tip cap to use after Curie (EIP 1559).
92-92: InitializedefaultGasTipin theOraclestruct properlyThe
Oraclestruct now includes thedefaultGasTipfield, but it lacks a comment explaining its purpose. Also, ensure consistency with other field comments.Add a comment for the
defaultGasTipfield:congestedThreshold int // Number of pending transactions to consider the network congested and suggest a minimum tip cap. defaultBasePrice *big.Int // Base price to set when CongestedThreshold is reached before Curie (EIP-1559). + defaultGasTip *big.Int // Default minimum tip cap to use after Curie (EIP-1559). historyCache *lru.Cache
211-214: Clarify the logic inSuggestTipCapmethodThe comments and logic in the
SuggestTipCapmethod could be improved for clarity. Specifically, mention that the default gas tip is used after the Curie upgrade, and the default base price is used before Curie.Modify the comments for better understanding:
if pendingTxCount < oracle.congestedThreshold { // Determine the suggested gas price based on the network configuration: - // - Before Curie (EIP-1559): total gas price = base fee + tip cap. - // - After Curie (EIP-1559): use the default tip cap. + // - Before Curie (EIP-1559): use the default base price. + // - After Curie (EIP-1559): use the default gas tip. price := oracle.defaultGasTipAdditionally, ensure that the comments accurately reflect the code logic.
cmd/utils/flags.go (1)
744-747: Verify consistency of flag naming and usage descriptionsThe new flag
GpoDefaultGasTipFlaguses the namegpo.defaultgastip. For consistency with other flags, consider using hyphens instead of camel case in the flag name. Also, clarify the usage description.Modify the flag definition:
GpoDefaultGasTipFlag = cli.Int64Flag{ - Name: "gpo.defaultgastip", + Name: "gpo.default-gas-tip", - Usage: "Default minimum tip cap (in wei) to be used after Curie fork (EIP-1559)", + Usage: "Default minimum tip cap (in wei) to use after the Curie upgrade (EIP-1559)", }This change ensures consistency with other flags and improves readability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
eth/gasprice/gasprice.go (2)
92-92: Consider enhancing the field commentWhile the field addition is correct, the comment could be more specific about when this tip is used.
- defaultGasTip *big.Int // Default gas tip to suggest when the network is not congested. + defaultGasTip *big.Int // Default gas tip (in wei) to suggest when the network is not congested (pendingTxCount < congestedThreshold).
211-213: Consider clarifying the pre/post-Curie behavior in commentsWhile the logic is correct, the comment could be clearer about the behavioral differences.
- // Before Curie (EIP-1559), we need to return the total suggested gas price. After Curie we return defaultGasTip wei as the tip cap, - // as the base fee is set separately or added manually for legacy transactions. + // Gas price behavior differs based on Curie (EIP-1559) activation: + // - Pre-Curie: Returns defaultBasePrice as the total gas price + // - Post-Curie: Returns defaultGasTip (100 wei) as the tip cap only, since base fee is handled separately
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
eth/gasprice/gasprice.go(5 hunks)
🔇 Additional comments (2)
eth/gasprice/gasprice.go (2)
43-48: LGTM! Well-documented constant update
The updated DefaultGasTip value and its comprehensive documentation clearly explain the rationale behind increasing from 1 wei to 100 wei. The change effectively addresses the transaction pool rejection and rounding issues while maintaining negligible cost impact.
61-61: LGTM! Well-structured config field addition
The DefaultGasTip field is appropriately added with the omitempty tag and clear documentation about its post-Curie usage.
colinlyguo
force-pushed
the
fix-GPO-increase-min-suggested-tip
branch
from
3fed4b8 to
81cb6cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
cmd/utils/flags.go(2 hunks)eth/gasprice/gasprice.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eth/gasprice/gasprice.go
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
eth/gasprice/gasprice.go (1)
144-148: Minor: Improve consistency in log messageThe validation logic is correct, but the log message could be more consistent by using the local variable instead of the constant.
- log.Warn("Sanitizing invalid gasprice oracle default gas tip cap", "provided", params.DefaultGasTipCap, "updated", DefaultGasTipCap) + log.Warn("Sanitizing invalid gasprice oracle default gas tip cap", "provided", params.DefaultGasTipCap, "updated", defaultGasTipCap)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
cmd/utils/flags.go(2 hunks)eth/gasprice/gasprice.go(6 hunks)params/version.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- params/version.go
- cmd/utils/flags.go
🔇 Additional comments (5)
eth/gasprice/gasprice.go (5)
43-48: LGTM! Well-documented constant change
The updated DefaultGasTipCap value and its comprehensive documentation clearly explain the rationale behind increasing from 1 wei to 100 wei. The comments effectively address both technical considerations (tx pool behavior, rounding issues) and practical implications (negligible cost impact).
61-61: LGTM! Config field properly defined
The DefaultGasTipCap field is well-integrated into the Config struct with appropriate TOML tag and clear documentation.
92-92: LGTM! Oracle field properly defined
The defaultGasTipCap field is well-documented and appropriately placed in the Oracle struct.
212-214: LGTM! Clear distinction between pre and post-Curie behavior
The updated logic correctly implements the configurable gas tip behavior while maintaining backward compatibility with pre-Curie behavior. The comments clearly explain the different scenarios.
Line range hint 211-224: Verify test coverage for the updated gas tip behavior
The changes to gas tip calculation logic should be covered by tests, especially the different behaviors before and after Curie.
✅ Verification successful
Let me check the test implementations to verify the coverage of different behaviors.
Let me check the test cases to verify coverage of both pre and post Curie scenarios.
Test coverage is comprehensive for gas tip behavior
The test file includes thorough coverage with TestSuggestTipCapCongestedThreshold that verifies:
- Pre-Curie behavior with default base price (2000)
- Post-Curie behavior with default gas tip cap (100)
- Different congestion scenarios (0, 49, 50, 100 pending transactions)
- Fork transitions (no fork, genesis fork, first block fork)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for test coverage of the new gas tip behavior
# Look for related test files
fd -e go -e _test.go gasprice
# Check for test cases covering the new defaultGasTipCap
rg -A 5 "TestSuggest.*TipCap|TestNewOracle.*GasTip"
Length of output: 848
Script:
#!/bin/bash
# Check the implementation of the test cases to verify coverage
rg -B 5 -A 15 "TestSuggestTipCapCongestedThreshold" eth/gasprice/gasprice_test.go
Length of output: 719
Script:
#!/bin/bash
# Get the full test case implementations
rg -B 10 -A 30 "TestSuggestTipCapCongestedThreshold" eth/gasprice/gasprice_test.go
Length of output: 1973
1. Purpose or design rationale of this PR
This PR makes the default gas tip configurable. The default value has been updated to 100 wei (from previous 2 wei) for the following reasons:
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
Chores