fix(sender): make sure gas price is above baseFee - #1531
Conversation
Sorry, something went wrong.
colinlyguo
left a comment
There was a problem hiding this comment.
nice catch.
Sorry, something went wrong.
Codecov Report❌ Patch coverage is 25.00000% with 3 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## develop #1531 +/- ##
===========================================
- Coverage 52.12% 52.11% -0.01%
===========================================
Files 157 157
Lines 12429 12433 +4
===========================================
+ Hits 6479 6480 +1
- Misses 5393 5395 +2
- Partials 557 558 +1
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.
The question is where does this 0.01GWei come from? Is it returned from the L2 node when querying the gas price or is it s.config.MinGasTip? See here:
I can only speculate but maybe something like this happened: the first transaction was not going through (send via SendTransaction here) for example because the L2 base fee went up (due to L1 base fee changes). So this tx (and nonce) was "stuck". Now in the resubmitTransaction we didn't account for the base fee for legacy transactions which is why it couldn't recover. |
Sorry, something went wrong.
agree. the probability of this speculation is higher when the network's L2 txns are mainly formed by gas oracle price update (txns updating L1 base fee and L1 blob base fee). |
Sorry, something went wrong.
WalkthroughThe changes in this pull request include an update to the version number in common/version/version.go, changing it from "v4.4.77" to "v4.4.78". Additionally, modifications were made to the createReplacingTransaction method in rollup/internal/controller/sender/sender.go to adjust the gas price for legacy transactions, ensuring it meets the base fee requirement. Minor adjustments to logging statements were also included to enhance clarity regarding gas price adjustments. Changes
Possibly related PRs
Suggested reviewers
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 details360-363: Consider optimizing for rapid base fee increases.
In scenarios where the base fee increases rapidly, the current implementation might require multiple resubmissions before a transaction is included. Consider adding a small multiplier (e.g., 1.1x) to the base fee to provide a buffer against near-term base fee increases.
baseFeeInt := new(big.Int).SetUint64(baseFee) +// Add 10% buffer to handle near-term base fee increases +baseFeeInt = new(big.Int).Mul(baseFeeInt, big.NewInt(11)) +baseFeeInt = new(big.Int).Div(baseFeeInt, big.NewInt(10)) if gasPrice.Cmp(baseFeeInt) < 0 { gasPrice = baseFeeInt }
Configuration used: CodeRabbit UI
Review profile: CHILL
Reviewing files that changed from the base of the PR and between 3ab5752 and 7fd3a17.
📒 Files selected for processing (2)- common/version/version.go (1 hunks)
- rollup/internal/controller/sender/sender.go (1 hunks)
- common/version/version.go
360-363: LGTM! Implementation correctly ensures gas price meets base fee requirement.
The added check properly ensures that resubmitted legacy transactions have a gas price at least equal to the current base fee, which directly addresses the issue where transactions were failing due to insufficient gas price.
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 484-486: Consider using configurable escalation factors for blob transactions.
Currently, blob transactions use a hardcoded 2x multiplier (big.NewInt(2)), while legacy and dynamic fee transactions use the configurable EscalateMultipleNum/Den. Consider using the same configurable factors for consistency and flexibility.
- // bumping at least 100% - gasTipCap := new(big.Int).Mul(originalGasTipCap, big.NewInt(2)) - gasFeeCap := new(big.Int).Mul(originalGasFeeCap, big.NewInt(2)) - blobGasFeeCap := new(big.Int).Mul(originalBlobGasFeeCap, big.NewInt(2)) + gasTipCap := new(big.Int).Mul(originalGasTipCap, escalateMultipleNum) + gasTipCap = new(big.Int).Div(gasTipCap, escalateMultipleDen) + gasFeeCap := new(big.Int).Mul(originalGasFeeCap, escalateMultipleNum) + gasFeeCap = new(big.Int).Div(gasFeeCap, escalateMultipleDen) + blobGasFeeCap := new(big.Int).Mul(originalBlobGasFeeCap, escalateMultipleNum) + blobGasFeeCap = new(big.Int).Div(blobGasFeeCap, escalateMultipleDen)
Configuration used: CodeRabbit UI
Review profile: CHILL
Reviewing files that changed from the base of the PR and between 57bf5c3 and d5d6390.
📒 Files selected for processing (2)- common/version/version.go (1 hunks)
- rollup/internal/controller/sender/sender.go (1 hunks)
- common/version/version.go
365-368: LGTM: Gas price floor implementation looks correct.
The implementation properly ensures that the gas price never falls below the base fee for legacy transactions, which directly addresses the reported issue with the gas oracle sender. This is a good temporary fix while the underlying gas oracle issue is being investigated.
365-368: Verify impact on gas price calculations.
Let's verify that no other parts of the codebase assume gas prices can be below base fee.
✅ Verification successfulBased on the search results, I can now generate the final response since I have enough context about gas price calculations and comparisons in the codebase:
Gas price enforcement is correctly implemented
The check for enforcing gas price to be at least base fee is localized to the sender package and doesn't require changes elsewhere because:
- The gas price oracle components (L1Relayer and L2Relayer) handle gas price updates independently through dedicated contracts
- The sender package is the central place that handles transaction sending, and all components use it through the Sender interface
- No other direct gas price comparisons or calculations were found in the codebase that would be affected by this change
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for gas price calculations and comparisons
rg -A 5 'gasPrice.*baseFee|baseFee.*gasPrice'
# Search for potential gas oracle configurations
rg -A 5 'gasPriceOracle|gas.*oracle|oracle.*gas'
Length of output: 30500
Sorry, something went wrong.
Purpose or design rationale of this PR
During the sdk test, we found that suggested gas price might be insufficent if the txpool have a few txs (cc @jonastheis any clue if you look into gas oracle logic?). and our txpool will reject a legacy tx if its gas price is below basefee.
Feedback from altlayer:
After running for a while, the suggested gas price will drop to 0.01GWei, and gas oracle sender couldn't send any tx successfully.
Then they have to manually send a tx with gas price = baseFee. After that, the gas oracle sender can resume working.
While I think the real solution is to fix the gas oracle, we can quick hack it in this way to make our sdk testnet working.
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