fix(rollup): sender use pending block base fee #1556
Conversation
WalkthroughThe changes involve a modification to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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)
rollup/internal/controller/sender/sender.go (1)
Line range hint
629-642: Consider enhancing error handling with specific error types.The current error handling uses a generic error message. Consider creating specific error types for different failure scenarios (header retrieval, base fee calculation, blob fee calculation) to help with error handling in the caller functions.
Example implementation:
+type HeaderError struct { + Op string + Err error +} + +func (e *HeaderError) Error() string { + return fmt.Sprintf("header %s failed: %v", e.Op, e.Err) +} func (s *Sender) getBlockNumberAndBaseFeeAndBlobFee(ctx context.Context) (uint64, uint64, uint64, error) { header, err := s.client.HeaderByNumber(ctx, big.NewInt(rpc.PendingBlockNumber.Int64())) if err != nil { - return 0, 0, 0, fmt.Errorf("failed to get header by number, err: %w", err) + return 0, 0, 0, &HeaderError{"retrieval", err} }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- rollup/internal/controller/sender/sender.go (1 hunks)
🔇 Additional comments (1)
rollup/internal/controller/sender/sender.go (1)
629-629: LGTM! Important fix for base fee estimation.The change to use the pending block's base fee instead of the latest block's base fee is a crucial fix. This helps prevent "max fee per gas less than block base fee" errors by providing a more accurate and up-to-date base fee estimation, especially during periods of L2 gas price volatility.
Let's verify if there are any other places in the codebase that might need similar updates:
✅ Verification successful
LGTM! The change to use pending block's base fee is safe and isolated.
The modification to use
rpc.PendingBlockNumberinstead of the implicit latest block is a targeted fix that only affects the base fee estimation in thegetBlockNumberAndBaseFeeAndBlobFeemethod. The search results confirm that:
- This is the only instance where
HeaderByNumberis used for base fee estimation- Other usages of
HeaderByNumberin the codebase are for different purposes (block syncing, confirmation tracking, etc.)- The change is contained within the transaction sending logic and doesn't affect other components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other instances where HeaderByNumber is called with nil rg -A 2 'HeaderByNumber.*nil\)' # Search for other base fee related functions ast-grep --pattern 'func $$$BaseFee$$$'Length of output: 75
Script:
#!/bin/bash # Let's try a broader search for HeaderByNumber usage rg -A 2 'HeaderByNumber' # Also search for base fee related code rg -A 2 'baseFee|BaseFee' # And check for any other potential block number usage rg -A 2 'BlockNumber|blockNumber'Length of output: 96621
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1556 +/- ##
===========================================
- Coverage 52.84% 52.84% -0.01%
===========================================
Files 157 157
Lines 12641 12640 -1
===========================================
- Hits 6680 6679 -1
Misses 5382 5382
Partials 579 579
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…l-tech/scroll into fix-rollup-sender-base-fee
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Purpose or design rationale of this PR
The baseFee sender get from latest block of L2 is not accurate, since the baseFee on L2 could spike, and sometime make it not high enough even we baseFee*2.
So we can use baseFee of pending block, it's more accurate. and will pervent issue:
err: max fee per gas less than block base feeCheck more details here: https://www.notion.so/scrollzkp/l2geth-returns-wrong-gas-price-issue-1297792d22af8045bbb1e57ff974e6c7
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:
Deployment tag versioning
Has
tagincommon/version.gobeen updated or have you addedbump-versionlabel to this PR?Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
New Features
Bug Fixes
Version Update