fix(rollup): sender use pending block base fee - #1556
Conversation
Sorry, something went wrong.
WalkthroughThe changes involve a modification to the getBlockNumberAndBaseFeeAndBlobFee method in the Sender struct located in rollup/internal/controller/sender/sender.go. The method now retrieves the block header for the pending block number instead of the latest confirmed block. This alteration affects the calculation of base fees, as it takes into account the state of the pending block. Additionally, the version number in common/version/version.go has been incremented from "v4.4.70" to "v4.4.71". Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review details Configuration used: CodeRabbit UI Files that changed from the base of the PR and between e2405f4 and 6401710. 📒 Files selected for processing (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? ❤️ Share 🪧 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 (.coderabbit.yaml)
Documentation and Community
|
Sorry, something went wrong.
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)📜 Review detailsLine 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} }
Configuration used: CodeRabbit UI
Review profile: CHILL
Files that changed from the base of the PR and between bc8f9db and 21ecaa6.
📒 Files selected for processing (1)- rollup/internal/controller/sender/sender.go (1 hunks)
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 successfulLGTM! The change to use pending block's base fee is safe and isolated.
The modification to use rpc.PendingBlockNumber instead of the implicit latest block is a targeted fix that only affects the base fee estimation in the getBlockNumberAndBaseFeeAndBlobFee method. The search results confirm that:
🏁 Scripts executed
- This is the only instance where HeaderByNumber is used for base fee estimation
- Other usages of HeaderByNumber in 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
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
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ 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.
|
Sorry, something went wrong.
…l-tech/scroll into fix-rollup-sender-base-fee
|
✅ Actions performed
Review triggered.
|
Sorry, something went wrong.
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 fee
Check 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 tag in common/version.go been updated or have you added bump-version label to this PR?
Breaking change label
Does this PR have the breaking-change label?
Summary by CodeRabbit
New Features
Bug Fixes
Version Update