fix(sender): make sure gas price is above baseFee #1531
Conversation
| @@ -357,6 +366,10 @@ func (s *Sender) resubmitTransaction(tx *gethTypes.Transaction, baseFee, blobBas | |||
| originalGasPrice := tx.GasPrice() | |||
| gasPrice := new(big.Int).Mul(originalGasPrice, escalateMultipleNum) | |||
| gasPrice = new(big.Int).Div(gasPrice, escalateMultipleDen) | |||
| baseFeeInt := new(big.Int).SetUint64(baseFee) | |||
| if gasPrice.Cmp(baseFeeInt) < 0 { | |||
| gasPrice = baseFeeInt | |||
There was a problem hiding this comment.
may as well add a multiplier here? the drawback would be over-charging by the multiplier.
There was a problem hiding this comment.
gasPrice is already multiplied. if after being multiplied it's still below baseFee we should bump it. I prefer not to multiplying again here.
There was a problem hiding this comment.
if the txn is submitted, and before it's included in a block, the block's base fee is updated by gas-oracle price update, the gas price will be < base fee again. same suspicion in another comment discussion.
0xmountaintop
dismissed stale reviews from georgehao and colinlyguo
via
30a7b47
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1531 +/- ##
===========================================
+ Coverage 52.78% 52.92% +0.14%
===========================================
Files 157 157
Lines 12652 12624 -28
===========================================
+ Hits 6678 6681 +3
+ Misses 5392 5362 -30
+ Partials 582 581 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The question is where does this
I can only speculate but maybe something like this happened: the first transaction was not going through (send via |
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). |
WalkthroughThe changes in this pull request include an update to the version number in 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? 🪧 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)
360-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 }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
common/version/version.go(1 hunks)rollup/internal/controller/sender/sender.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- common/version/version.go
🔇 Additional comments (1)
rollup/internal/controller/sender/sender.go (1)
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.
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
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