add non congested gas fee to fee history #1163
Conversation
WalkthroughThis PR updates the fee history processing in the Oracle module. It modifies the method signatures and internal logic of both the Changes
Sequence Diagram(s)sequenceDiagram
participant FH as FeeHistory
participant O as Oracle
participant BE as Backend
FH->>O: Call resolveBlockRange(ctx, lastBlock, blocks)
O->>BE: Retrieve latest/pending block header
BE-->>O: Return block, receipts, headHeader, etc.
O-->>FH: Return block, receipts, headHeader, and additional values
FH->>O: Call processBlock(bf, percentiles, nonCongestedPrice)
O-->>FH: Process block with updated reward logic
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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
🧹 Nitpick comments (2)
eth/gasprice/feehistory.go (2)
125-125: Missing error handling for StatsWithMinBaseFee.The code ignores the second return value from
oracle.backend.StatsWithMinBaseFee(headHeader.BaseFee)which could potentially contain error information.Consider handling the error value returned by
StatsWithMinBaseFeeto ensure more robust error reporting:-pendingTxCount, _ := oracle.backend.StatsWithMinBaseFee(headHeader.BaseFee) +pendingTxCount, err := oracle.backend.StatsWithMinBaseFee(headHeader.BaseFee) +if err != nil { + log.Warn("Failed to get pending transaction count", "err", err) + // Use a fallback or default behavior +}
124-137: Add null check for headHeader.There's a potential risk of null pointer dereference if
headHeaderis nil when accessingheadHeader.BaseFeeandheadHeader.Number.Add a null check for
headHeaderto prevent potential null pointer exceptions:var reward *big.Int +if headHeader == nil { + reward, _ = tx.EffectiveGasTip(bf.block.BaseFee()) +} else { pendingTxCount, _ := oracle.backend.StatsWithMinBaseFee(headHeader.BaseFee) if pendingTxCount < oracle.congestedThreshold { // Before Curie (EIP-1559), we need to return the total suggested gas price. After Curie we return defaultGasTipCap wei as the tip cap, // as the base fee is set separately or added manually for legacy transactions. price := oracle.defaultGasTipCap if !oracle.backend.ChainConfig().IsCurie(headHeader.Number) { price = oracle.defaultBasePrice } reward = price } else { reward, _ = tx.EffectiveGasTip(bf.block.BaseFee()) } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/gasprice/feehistory.go(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
eth/gasprice/feehistory.go (2)
eth/gasprice/gasprice.go (1)
Oracle(79-94)params/config.go (1)
ChainConfig(632-675)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
eth/gasprice/feehistory.go (7)
87-87: Method signature updated with a new parameter.The
processBlockmethod now accepts a newheadHeaderparameter. This change is aligned with the PR objective to add non-congested gas fee information to the fee history.
159-159: Method signature updated with a new return value.The
resolveBlockRangemethod now returns an additional parameter to provide the head header, which is needed for the non-congested gas fee calculation inprocessBlock.
164-164: Added head header tracking.The code now properly tracks the head header throughout the block resolution process, whether it comes from a pending block or the latest block.
Also applies to: 171-171, 185-185
177-177: Updated return statements to include the head header.All return statements in
resolveBlockRangeare updated to include the head header value. This is consistent with the method signature change.Also applies to: 187-187, 193-193, 199-199, 206-206
248-248: Updated method call to receive the new head header return value.The call to
resolveBlockRangeis properly updated to retrieve the head header value.
275-275: Updated method calls to pass the head header parameter.Both calls to
processBlocknow correctly pass the head header parameter, ensuring consistent behavior throughout the fee history calculation.Also applies to: 297-297
124-137:Details
✅ Verification successful
Added logic to differentiate between congested and non-congested network conditions.
The code now checks the pending transaction count against a congestion threshold to determine the appropriate reward calculation:
- For non-congested conditions: Uses a default price based on whether we're before or after the Curie upgrade
- For congested conditions: Uses the transaction's effective gas tip as before
This implementation addresses the PR objective by providing different gas fee recommendations based on network congestion status.
🏁 Script executed:
#!/bin/bash # Verify if StatsWithMinBaseFee is correctly implemented to return pending tx count rg -A 3 "StatsWithMinBaseFee" --type goLength of output: 3802
Approval: Gas Fee Reward Logic Correctly Differentiates Network Conditions
The updated code correctly leverages
StatsWithMinBaseFeeto acquire the pending transaction count from the pool. It then uses this count to determine the reward:
- Non-Congested: If the pending count is below
oracle.congestedThreshold, it usesoracle.defaultGasTipCap(ororacle.defaultBasePricewhen pre-Curie) as the reward.- Congested: Otherwise, it falls back to using the transaction's effective gas tip.
The shell script output confirms that the
StatsWithMinBaseFeeimplementation is functioning as expected. This logic fulfills the PR objective by adapting gas fee recommendations based on current network congestion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eth/gasprice/feehistory.go (1)
251-252: Consider caching the pending transaction count.This call to
StatsWithMinBaseFeecould potentially be expensive. While you've already moved it outside the loop as suggested in previous reviews, consider caching this value with an appropriate TTL if it's called frequently.-pendingTxCount, _ := oracle.backend.StatsWithMinBaseFee(headHeader.BaseFee) +// Get pending transaction count, using cached value if available +pendingTxCount := oracle.getPendingTxCountWithCache(headHeader.BaseFee)Where you would implement a helper method with appropriate caching:
func (oracle *Oracle) getPendingTxCountWithCache(baseFee *big.Int) int { oracle.cacheLock.RLock() lastCheck := oracle.lastPendingTxCheck oracle.cacheLock.RUnlock() // If cache is recent enough (e.g., less than 5 seconds old), use cached value if time.Since(lastCheck) < 5*time.Second { oracle.cacheLock.RLock() count := oracle.cachedPendingTxCount oracle.cacheLock.RUnlock() return count } // Otherwise, fetch new value and update cache count, _ := oracle.backend.StatsWithMinBaseFee(baseFee) oracle.cacheLock.Lock() oracle.cachedPendingTxCount = count oracle.lastPendingTxCheck = time.Now() oracle.cacheLock.Unlock() return count }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/gasprice/feehistory.go(7 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
eth/gasprice/feehistory.go (2)
eth/gasprice/gasprice.go (1)
Oracle(79-94)params/config.go (1)
ChainConfig(632-675)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
eth/gasprice/feehistory.go (7)
87-87: Updated function signature to support non-congested gas pricing.The function signature has been modified to accept a new parameter
nonCongestedPrice *big.Intwhich is consistent with the PR's objective of adding non-congested gas fee information to the fee history.
124-129: Conditional reward calculation based on network congestion state.The code now intelligently selects between the non-congested price (when provided) and the effective gas tip calculated from the transaction. This is a good approach as it allows for dynamic fee suggestions based on network conditions.
152-152: Updated function signature to return head header information.The function signature now includes the head header as a return value, which is necessary for determining non-congested pricing in the calling code.
157-158: Updated return statements to include head header information.All return statements have been properly updated to include the new return value (headHeader). The initialization of headHeader is correctly handled in both pending block and latest header scenarios.
Also applies to: 164-165, 170-171, 178-181, 186-187, 192-193, 199-200
247-259: Added non-congested gas price determination logic.This implementation addresses the core objective of the PR by determining when the network is not congested (based on pending transaction count) and suggesting an appropriate minimal gas price. The logic correctly handles both pre-Curie and post-Curie scenarios.
The code follows the recommendation from the previous review to calculate this value once per fee history request rather than inside the processing loop.
241-242: Properly captures the head header from resolveBlockRange.The code correctly captures the additional return value from
resolveBlockRangeand handles error cases appropriately.
283-284: Updated processBlock calls with the non-congested price parameter.Both calls to
processBlockhave been properly updated to pass the newnonCongestedPriceparameter, ensuring consistent behavior throughout the codebase.Also applies to: 305-306
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
eth/gasprice/feehistory.go (1)
251-251: Error handling for StatsWithMinBaseFee.The error returned by
oracle.backend.StatsWithMinBaseFee()is ignored. Consider logging the error or handling it appropriately.-pendingTxCount, _ := oracle.backend.StatsWithMinBaseFee(headHeader.BaseFee) +pendingTxCount, err := oracle.backend.StatsWithMinBaseFee(headHeader.BaseFee) +if err != nil { + log.Debug("Failed to get stats with min base fee", "err", err) + // Continue with pendingTxCount = 0 or handle as appropriate +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/gasprice/feehistory.go(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
eth/gasprice/feehistory.go (5)
87-87: Function signature updated to include the non-congested price parameter.The
processBlockfunction now accepts a new parameternonCongestedPrice *big.Int, which is used to determine the reward for transactions when the network is not congested. The change makes sense in the context of the feature being added.
124-129: Conditional reward determination based on congestion state.The code now selects between two different reward calculation strategies:
- Using the provided
nonCongestedPricewhen network is not congested- Falling back to transaction's effective gas tip when congested
This implementation aligns with the PR's objective to add non-congested gas fee support.
152-152: Updated function signature to include head header.The
resolveBlockRangefunction now also returns the head header, which is necessary for determining the non-congested gas price based on the current network state. This change supports the new feature properly.
283-283: Updated processBlock call with non-congested price.The
processBlockcall has been updated to include the newnonCongestedPriceparameter, ensuring consistent application of the feature throughout the codebase.
305-305: Updated processBlock call with non-congested price.This second call to
processBlockhas been correctly updated to include thenonCongestedPriceparameter, ensuring the non-congested gas fee logic is applied consistently.
1. Purpose or design rationale of this PR
...
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