feat: suggest gas price based on latest block's transactions #1220
Conversation
WalkthroughThe changes remove the congestion threshold logic from the gas price oracle, eliminating related configuration fields, CLI flags, and code paths. A new Scroll-specific priority fee suggestion mechanism is introduced, relying on block capacity heuristics instead of pending transaction counts. Versioning is incremented, and a chain config helper for Scroll is added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GasPriceOracle
participant ChainConfig
participant BlockHeader
participant ScrollLogic
User->>GasPriceOracle: SuggestTipCap(ctx, header)
GasPriceOracle->>ChainConfig: IsScroll()
alt Scroll chain
GasPriceOracle->>ScrollLogic: SuggestScrollPriorityFee(ctx, header)
ScrollLogic->>BlockHeader: Analyze block capacity, receipts, txs
ScrollLogic-->>GasPriceOracle: Return suggested priority fee
else Non-Scroll chain
GasPriceOracle->>BlockHeader: Sample recent blocks
GasPriceOracle-->>User: Return percentile tip cap
end
GasPriceOracle-->>User: Return suggested tip cap
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (3)
eth/gasprice/scroll_gasprice.go (3)
33-34: Fix comment formatting.The comment is unnecessarily split across lines and has a trailing space.
- // find the maximum transaction size by any of the transactions in the block to use as the block - // size limit capacity margin + // find the maximum transaction size by any of the transactions in the block to use as the block + // size limit capacity margin
51-51: Use the += operator for clarity.- totalTxSizeUsed = totalTxSizeUsed + su + totalTxSizeUsed += su
93-93: Consider making the fee markup percentage configurable.The 10% markup over the median is hardcoded. Consider making this configurable to allow for easier tuning without code changes.
You could add a field to the Oracle struct:
// In Oracle struct definition feeMarkupPercent int // Percentage markup over median when at capacity (default: 10)Then use it here:
- newSuggestion := new(big.Int).Add(median, new(big.Int).Div(median, big.NewInt(10))) + markup := new(big.Int).Mul(median, big.NewInt(int64(oracle.feeMarkupPercent))) + markup = new(big.Int).Div(markup, big.NewInt(100)) + newSuggestion := new(big.Int).Add(median, markup)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eth/gasprice/feehistory.go(3 hunks)eth/gasprice/gasprice.go(1 hunks)eth/gasprice/scroll_gasprice.go(1 hunks)params/config.go(9 hunks)rollup/rollup_sync_service/rollup_sync_service_test.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.
eth/gasprice/feehistory.go (1)
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.
🧬 Code Graph Analysis (2)
rollup/rollup_sync_service/rollup_sync_service_test.go (1)
params/config.go (1)
ScrollConfig(699-720)
eth/gasprice/scroll_gasprice.go (3)
eth/gasprice/gasprice.go (1)
Oracle(79-94)rpc/types.go (1)
BlockNumber(58-58)params/config.go (1)
ChainConfig(641-685)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
rollup/rollup_sync_service/rollup_sync_service_test.go (1)
29-29: LGTM: Test initialization correctly updated for pointer typeThe changes correctly update the test initialization to use pointer syntax for the
Scrollfield, which is consistent with the type change inparams/config.go.Also applies to: 78-78
eth/gasprice/gasprice.go (1)
207-209: SuggestScrollPriorityFee implementation verified
- The method
func (oracle *Oracle) SuggestScrollPriorityFee(ctx context.Context, header *types.Header) *big.Intis defined ineth/gasprice/scroll_gasprice.go(lines 132–141).- No missing‐method issues detected.
params/config.go (3)
684-684: LGTM: Consistent type change from value to pointerThe change from
Scroll ScrollConfigtoScroll *ScrollConfigis applied consistently across all chain configurations and enables proper nil checking.
297-297: LGTM: All chain configurations updated consistentlyAll chain configurations have been correctly updated to use pointer syntax (
&ScrollConfig{) instead of value syntax, which is consistent with the type change.Also applies to: 345-345, 398-398, 448-448, 492-492, 534-534, 574-574
1022-1025: LGTM: Well-implemented IsScroll() methodThe new
IsScroll()method correctly checks for nil pointer and provides a clean way to determine if a chain is configured as a Scroll chain.eth/gasprice/scroll_gasprice.go (1)
108-141: Well-designed implementation with excellent documentation!The function properly handles caching with appropriate locking and returns a defensive copy. The extensive comment clearly explains the rationale for Scroll's specific approach to priority fee calculation.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/gasprice/scroll_gasprice.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.
🧬 Code Graph Analysis (1)
eth/gasprice/scroll_gasprice.go (3)
eth/gasprice/gasprice.go (1)
Oracle(79-94)params/config.go (1)
ChainConfig(641-685)rpc/types.go (1)
BlockNumber(58-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test
- GitHub Check: build-mock-ccc-geth
- GitHub Check: check
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
eth/gasprice/scroll_gasprice.go (9)
1-12: Package structure and imports look good.The imports are appropriate for the functionality implemented. The package declaration follows Go conventions.
14-18: Correct fork-dependent suggestion initialization.The logic properly determines the initial suggestion based on whether the chain is at or past the Curie fork, using the appropriate default values.
22-26: Proper error handling for receipt retrieval.The code correctly handles both nil receipts and errors, falling back to the default suggestion when receipts cannot be retrieved.
42-46: Proper error handling for block retrieval.The code correctly handles both nil blocks and errors, falling back to the default suggestion when the block cannot be retrieved.
70-84: Excellent documentation of heuristic approach.The detailed comment clearly explains the rationale behind the capacity detection heuristic and acknowledges its limitations. This is valuable for maintainability.
90-96: Correct median calculation and tip adjustment.The code properly calculates the median effective gas tip and applies a 10% markup. The sorting and median selection logic is correct.
103-106: Proper price capping implementation.The code correctly caps the suggestion by the maximum price and safely modifies the suggestion in-place.
111-134: Comprehensive and well-structured documentation.The method documentation thoroughly explains the algorithm, its rationale, and the specific considerations for Scroll's architecture. This is excellent for maintainability and understanding.
135-144: Proper caching and thread safety.The method correctly wraps the calculation, updates the cache under lock protection, and returns a copy of the result to prevent external modifications.
Co-authored-by: Jonas Theis <4181434+jonastheis@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
eth/gasprice/scroll_gasprice_test.go (2)
80-81: Update comment to reflect Scroll context.The comment mentions "optimism priority fee suggestions" but this is for Scroll's gas price oracle.
- // only the most recent block is considered for optimism priority fee suggestions, so this is + // only the most recent block is considered for scroll priority fee suggestions, so this is // where we add the test transactions
39-42: Improve struct field naming for clarity.The
gasLimitfield intestTxDatais actually used for gas consumed (receipt gas used), not transaction gas limit. This is confusing.type testTxData struct { priorityFee int64 - gasLimit uint64 + gasUsed uint64 }And update the usage accordingly:
- r.GasUsed = tx.gasLimit + r.GasUsed = tx.gasUsed
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/geth/main.go(0 hunks)cmd/geth/usage.go(0 hunks)eth/gasprice/scroll_gasprice_test.go(1 hunks)
💤 Files with no reviewable changes (2)
- cmd/geth/main.go
- cmd/geth/usage.go
🧰 Additional context used
🧠 Learnings (1)
eth/gasprice/scroll_gasprice_test.go (1)
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
eth/gasprice/scroll_gasprice.go (2)
65-65: Add nil check for Scroll configuration to prevent potential runtime panic.The code accesses
oracle.backend.ChainConfig().Scrollwithout checking ifScrollis nil, which could cause a panic if the Scroll configuration is not set.Apply this fix:
- if !oracle.backend.ChainConfig().Scroll.IsValidBlockSize(maxTxSizeUsed) { + scrollConfig := oracle.backend.ChainConfig().Scroll + if scrollConfig == nil || !scrollConfig.IsValidBlockSize(maxTxSizeUsed) { log.Error("found tx consuming more size than the block size limit", "size", maxTxSizeUsed) return suggestion, isCongested }
70-71: Add nil check for Scroll configuration in capacity calculation.Similar to the previous issue, the code accesses
oracle.backend.ChainConfig().Scrollmethods without nil checks, risking a panic.Apply this fix:
+ scrollConfig := oracle.backend.ChainConfig().Scroll + if scrollConfig == nil { + log.Error("Scroll configuration is nil") + return suggestion, isCongested + } if header.GasUsed+maxTxGasUsed > header.GasLimit || - !oracle.backend.ChainConfig().Scroll.IsValidBlockSizeForMining(totalTxSizeUsed+maxTxSizeUsed) { + !scrollConfig.IsValidBlockSizeForMining(totalTxSizeUsed+maxTxSizeUsed) {eth/gasprice/scroll_gasprice_test.go (1)
115-123: Fix gas limit inconsistency in test transaction creation.The transaction gas limit is set to
params.TxGas(21000) but the receipt'sGasUsedis set totx.gasLimitfrom test data. This creates invalid scenarios where gas used can exceed the transaction's gas limit.Apply this fix to ensure consistency:
txdata := &types.DynamicFeeTx{ ChainID: params.TestChainConfig.ChainID, Nonce: nonce, To: &common.Address{}, - Gas: params.TxGas, + Gas: tx.gasLimit, GasFeeCap: big.NewInt(100 * params.GWei), GasTipCap: big.NewInt(tx.priorityFee), Data: GenerateRandomBytes(int(tx.payloadSize)), }
🧹 Nitpick comments (2)
eth/gasprice/scroll_gasprice.go (1)
97-108: Verify median calculation for even number of transactions.The median calculation uses
len(tips)/2which works for odd numbers but may not give the true median for even numbers of transactions.For mathematical accuracy with even-length arrays, consider using the average of the two middle values:
- median := tips[len(tips)/2] + var median *big.Int + if len(tips)%2 == 0 { + mid1 := tips[len(tips)/2-1] + mid2 := tips[len(tips)/2] + median = new(big.Int).Add(mid1, mid2) + median.Div(median, big.NewInt(2)) + } else { + median = tips[len(tips)/2] + }eth/gasprice/scroll_gasprice_test.go (1)
90-96: Consider using crypto/rand for test data generation.The current implementation uses
math/randfor generating random bytes, which produces predictable sequences. For more robust testing, consider using cryptographically secure random generation.+import "crypto/rand" -func GenerateRandomBytes(length int) []byte { - b := make([]byte, length) - for i := range b { - b[i] = byte(rand.Intn(256)) - } - return b -} +func GenerateRandomBytes(length int) []byte { + b := make([]byte, length) + rand.Read(b) + return b +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eth/gasprice/feehistory.go(1 hunks)eth/gasprice/gasprice.go(3 hunks)eth/gasprice/gasprice_test.go(0 hunks)eth/gasprice/scroll_gasprice.go(1 hunks)eth/gasprice/scroll_gasprice_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- eth/gasprice/gasprice_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- eth/gasprice/feehistory.go
- eth/gasprice/gasprice.go
🧰 Additional context used
🧠 Learnings (2)
eth/gasprice/scroll_gasprice_test.go (1)
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.
eth/gasprice/scroll_gasprice.go (1)
Learnt from: jonastheis
PR: scroll-tech/go-ethereum#1115
File: rollup/da_syncer/da_syncer.go:35-41
Timestamp: 2025-02-14T04:10:06.754Z
Learning: The `override` parameter in `DASyncer.SyncOneBlock()` is intentionally designed to allow rewriting existing chain data during recovery mode, enabling reconstruction of a valid canonical L2 chain from permissionless committed batches.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
eth/gasprice/scroll_gasprice.go (2)
14-21: LGTM: Clear fork-based suggestion logic.The implementation correctly determines the base suggestion based on the Curie fork status, with helpful comments explaining the pre/post-Curie behavior.
70-91: LGTM: Well-documented capacity heuristic.The capacity calculation logic is well-documented with clear explanation of the heuristic approach and its trade-offs. The comments effectively explain why this approach is chosen over transaction pool queries.
eth/gasprice/scroll_gasprice_test.go (3)
69-74: LGTM: Correct mock backend chain configuration.The
ChainConfig()method correctly returns the test configuration with proper Scroll-specific parameters and Curie block settings.
133-254: LGTM: Comprehensive test coverage.The test function correctly covers multiple scenarios including pre/post-Curie conditions, various capacity states, and both gas limit and payload size constraints. The test cases are well-structured and verify expected behavior.
156-160: LGTM: Accurate capacity calculation test.The test correctly demonstrates the capacity heuristic: with two transactions having gas limits 21000 and 21001, the calculation
header.GasUsed (42001) + maxTxGasUsed (21001) = 63002exceeds the block gas limit of 63000, properly triggering the "at capacity" condition.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
eth/gasprice/scroll_gasprice_test.go (2)
159-159: Clarify the capacity heuristic in test comments.The "at capacity" condition is triggered when
header.GasUsed + maxTxGasUsed > blockGasLimit, not simply when total gas used exceeds the limit. With two transactions having gas limits 21000 and 21001, the calculation is: 42001 (GasUsed) + 21001 (maxTxGasUsed) = 63002 > 63000 (blockGasLimit).- // 2 txs w same priority fee (1 gwei), but second tx puts it right over gas limit capacity + // 2 txs w same priority fee (1 gwei); since header.GasUsed (21000+21001) + maxTxGasUsed (21001) = 63002 > blockGasLimit (63000), capacity is triggeredAlso applies to: 211-211
114-126: Fix gas limit inconsistency between transaction and receipt.The transaction gas limit is hardcoded to
params.TxGas(21000) but the receipt's gas used is set totx.gasLimitfrom test data. This creates invalid scenarios where gas used exceeds the transaction's gas limit.txdata := &types.DynamicFeeTx{ ChainID: params.TestChainConfig.ChainID, Nonce: nonce, To: &common.Address{}, - Gas: params.TxGas, + Gas: tx.gasLimit, GasFeeCap: big.NewInt(100 * params.GWei), GasTipCap: big.NewInt(tx.priorityFee), Data: GenerateRandomBytes(int(tx.payloadSize)), }
🧹 Nitpick comments (2)
eth/gasprice/scroll_gasprice_test.go (2)
106-107: Update comment to reference Scroll instead of Optimism.The comment references "optimism priority fee suggestions" but this is Scroll-specific code.
- // only the most recent block is considered for optimism priority fee suggestions, so this is + // only the most recent block is considered for scroll priority fee suggestions, so this is
185-185: Clarify the payload capacity heuristic in test comments.Similar to gas capacity, the payload capacity check likely uses a heuristic involving the maximum transaction payload size.
- // 2 txs w same priority fee (1 gwei), but second tx puts it right over block payload capacity + // 2 txs w same priority fee (1 gwei); second tx triggers payload capacity heuristicAlso applies to: 237-237
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/gasprice/scroll_gasprice_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
eth/gasprice/scroll_gasprice_test.go (1)
Learnt from: Thegaram
PR: scroll-tech/go-ethereum#1204
File: miner/scroll_worker.go:836-845
Timestamp: 2025-06-11T15:54:05.820Z
Learning: When `processTxn` rejects a transaction because its calculated L1 data fee is ≥ `fees.MaxL1DataFee()`, this indicates a mis-configured system parameter, not a bad transaction, so the tx must remain in the TxPool rather than being purged.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
eth/gasprice/scroll_gasprice.go (4)
72-72: Consider performance impact of transaction size calculation.The
txs[i].Size()method involves RLP encoding operations which can be expensive when processing blocks with many transactions. This was noted in previous reviews as a potential performance bottleneck.Consider adding benchmark tests to measure the performance impact and potentially optimize if needed for high-throughput scenarios.
45-48: Add block number to error log for better debugging.The error logging could be more informative by including the specific block number that failed, as suggested in previous reviews.
- if receipts == nil || err != nil { - log.Error("failed to get block receipts", "block number", header.Number, "err", err) - return suggestion, isCongested - } + if receipts == nil || err != nil { + log.Error("failed to get block receipts", "block", header.Number, "hash", header.Hash(), "err", err) + return suggestion, isCongested + }
84-86: Add nil check for Scroll configuration to prevent potential panic.The code accesses
oracle.backend.ChainConfig().Scrollwithout checking ifScrollis nil, which could cause a runtime panic. This was flagged in previous reviews.- if !oracle.backend.ChainConfig().Scroll.IsValidBlockSize(maxTxSizeUsed) { + scrollConfig := oracle.backend.ChainConfig().Scroll + if scrollConfig == nil { + log.Error("Scroll configuration is nil") + return suggestion, isCongested + } + if !scrollConfig.IsValidBlockSize(maxTxSizeUsed) { log.Error("found tx consuming more size than the block size limit", "size", maxTxSizeUsed) return suggestion, isCongested }
89-90: Add nil check for Scroll configuration in capacity validation.Similar to the previous issue, the code accesses
oracle.backend.ChainConfig().Scrollmethods without nil checks, risking a panic during capacity validation.+ scrollConfig := oracle.backend.ChainConfig().Scroll + if scrollConfig == nil { + log.Error("Scroll configuration is nil") + return suggestion, isCongested + } if header.GasUsed+maxTxGasUsed > header.GasLimit || - !oracle.backend.ChainConfig().Scroll.IsValidBlockSizeForMining(totalTxSizeUsed+maxTxSizeUsed) { + !scrollConfig.IsValidBlockSizeForMining(totalTxSizeUsed+maxTxSizeUsed) {
🧹 Nitpick comments (1)
eth/gasprice/scroll_gasprice.go (1)
116-127: Validate the priority fee calculation heuristic.The algorithm calculates a new priority fee suggestion as 10% above the median effective gas tip when blocks are at capacity. This approach is reasonable for Scroll's single block builder model, but consider the security implications mentioned in previous reviews about potential manipulation through large transactions.
While the 10% increment over median is a sound heuristic, monitor real-world usage to ensure it provides adequate protection against fee manipulation while maintaining reasonable user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/gasprice/scroll_gasprice.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
eth/gasprice/scroll_gasprice.go (1)
Learnt from: jonastheis
PR: scroll-tech/go-ethereum#1115
File: rollup/da_syncer/da_syncer.go:35-41
Timestamp: 2025-02-14T04:10:06.754Z
Learning: The `override` parameter in `DASyncer.SyncOneBlock()` is intentionally designed to allow rewriting existing chain data during recovery mode, enabling reconstruction of a valid canonical L2 chain from permissionless committed batches.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
eth/gasprice/scroll_gasprice.go (2)
135-143: Cache update logic is well-implemented.The cache update mechanism properly checks if the current header is the latest before updating, preventing stale data from being cached. The locking strategy is also appropriate.
148-176: Excellent documentation and public interface design.The extensive documentation clearly explains the rationale behind the Scroll-specific algorithm, making it easy for other developers to understand the design decisions. The public interface is clean and follows Go conventions.
yiweichi
dismissed stale reviews from colinlyguo and coderabbitai[bot]
via
8226689
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
eth/gasprice/scroll_gasprice.go (3)
70-77: Potential performance impact from transaction size calculations.The loop calls
txs[i].Size()which involves RLP encoding and was flagged in past reviews as potentially expensive for blocks with many transactions.Additionally, there's a potential integer overflow risk at line 76 where
totalTxSizeUsedis accumulated without bounds checking, though this would only occur with extremely large blocks.
84-87: Critical: Potential nil pointer dereference risk.The code accesses
oracle.backend.ChainConfig().Scrollwithout checking ifScrollis nil, which could cause a panic.Add nil checks before accessing Scroll methods:
- if !oracle.backend.ChainConfig().Scroll.IsValidBlockSize(maxTxSizeUsed) { + scrollConfig := oracle.backend.ChainConfig().Scroll + if scrollConfig == nil { + log.Error("Scroll configuration is nil") + return suggestion, isCongested + } + if !scrollConfig.IsValidBlockSize(maxTxSizeUsed) { log.Error("found tx consuming more size than the block size limit", "size", maxTxSizeUsed) return suggestion, isCongested }
89-90: Critical: Potential nil pointer dereference risk in capacity check.Similar to the previous issue, the code accesses
oracle.backend.ChainConfig().Scrollmethods without nil checks.Add nil checks for the Scroll configuration:
- if header.GasUsed+maxTxGasUsed > header.GasLimit || - !oracle.backend.ChainConfig().Scroll.IsValidBlockSizeForMining(totalTxSizeUsed+maxTxSizeUsed) { + scrollConfig := oracle.backend.ChainConfig().Scroll + if scrollConfig == nil { + log.Error("Scroll configuration is nil") + return suggestion, isCongested + } + if header.GasUsed+maxTxGasUsed > header.GasLimit || + !scrollConfig.IsValidBlockSizeForMining(totalTxSizeUsed+maxTxSizeUsed) {
🧹 Nitpick comments (1)
eth/gasprice/scroll_gasprice.go (1)
135-143: Minor: Ignored error from HeaderByNumber.The error from
oracle.backend.HeaderByNumberis ignored, which could potentially lead to cache updates with stale data if the call fails.Consider handling the error:
- latestHeader, _ := oracle.backend.HeaderByNumber(ctx, rpc.LatestBlockNumber) - if header.Hash() == latestHeader.Hash() { + latestHeader, err := oracle.backend.HeaderByNumber(ctx, rpc.LatestBlockNumber) + if err != nil { + log.Error("failed to get latest header for cache update", "err", err) + return suggestion, isCongested + } + if header.Hash() == latestHeader.Hash() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eth/gasprice/scroll_gasprice.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
eth/gasprice/scroll_gasprice.go (1)
Learnt from: jonastheis
PR: scroll-tech/go-ethereum#1115
File: rollup/da_syncer/da_syncer.go:35-41
Timestamp: 2025-02-14T04:10:06.754Z
Learning: The `override` parameter in `DASyncer.SyncOneBlock()` is intentionally designed to allow rewriting existing chain data during recovery mode, enabling reconstruction of a valid canonical L2 chain from permissionless committed batches.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: test
- GitHub Check: check
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
eth/gasprice/scroll_gasprice.go (6)
14-32: LGTM: Proper double-checked locking pattern implemented.The caching logic correctly implements the double-checked locking pattern with proper use of RWMutex to avoid race conditions while minimizing lock contention.
34-40: LGTM: Fork-aware gas price initialization.The logic correctly handles the Curie fork transition by using different baseline suggestions (tip cap vs. base price) based on EIP-1559 activation.
42-56: LGTM: Proper error handling for receipt fetching.The error handling for receipt fetching is appropriate, logging errors and returning early with the default suggestion when receipts are unavailable.
111-128: LGTM: Sound capacity-based fee calculation logic.The median-based fee calculation with 10% increment provides a reasonable market-responsive mechanism for congested blocks. The logic correctly handles edge cases like empty transaction lists and properly sorts tips before median calculation.
130-133: LGTM: Proper price capping mechanism.The suggestion is correctly capped by
oracle.maxPriceto prevent excessive gas price recommendations.
148-176: LGTM: Well-documented public API method.The method provides excellent documentation explaining the rationale behind the Scroll-specific approach and correctly delegates to the internal calculation method while returning a copy to prevent external modification.
1. Purpose or design rationale of this PR
This pr focus on implementing a simplified version of the GPO without needing to look at the tx pool, however look at the status of latest block. Suggest gas price base on if the latest block is at capacity. this will help to:
- reduce node configuration complexity by removing congestion threshold parameter
- reduce RPC node overhead by removing expensive query of tx pool size
- enable private mempool while any RPC node is still able to provide reasonable gas price estimate
reference doc
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