Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(sender): gas price bump corner case when resubmitting tx #1058

Merged
merged 7 commits into from
Dec 28, 2023

Conversation

colinlyguo
Copy link
Member

@colinlyguo colinlyguo commented Dec 26, 2023

Purpose or design rationale of this PR

Issue: node returns “replacement transaction underpriced” when commit-batch-sender resubmits tx with a “bumped” gas tip and gas cap of 120% (a configured value), causing a commitBatch tx delay.

Reason:

  1. if gas cap or gas tip of replacement EIP-1559 tx (gas price of pre EIP-1559 tx) is the same as the old one in the tx_pool, the l1-el node will reject this tx, code ref (the version of l1-el node receiving the tx is v1.13.2): https://github.com/ethereum/go-ethereum/blob/v1.13.2/core/txpool/legacypool/list.go#L305-L307
  2. the tx’s gas tip is only 3wei, thus floor(3wei * 1.2) = 3wei, thus rejected by the tx_pool. tx: https://sepolia.etherscan.io/tx/0xcd41a23a90b1f22e106b1c1f224575ca6afa68e1e140e3b03d72db16574d2dbd

Mainnet Impact: I think this error would rarely happen in mainnet, because the gas tip is very unlikely to be <= 4 wei to trigger the same issue.

Fixes:

  1. Add a fix when gas tip and gas cap are the same before and after the bump.
  2. Add more logs in resubmitting tx (hard to know the context when submitting currently).

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:

  • fix: A bug fix

Deployment tag versioning

Has tag in common/version.go been updated or have you added bump-version label to this PR?

  • Yes

Breaking change label

Does this PR have the breaking-change label?

  • No, this PR is not a breaking change

@colinlyguo colinlyguo added bug Something isn't working bump-version Bump the version tag for deployment labels Dec 26, 2023
@colinlyguo colinlyguo self-assigned this Dec 26, 2023
Copy link

codecov bot commented Dec 26, 2023

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (b0609fa) 48.90% compared to head (ce00cf6) 48.97%.

Files Patch % Lines
rollup/internal/controller/sender/sender.go 81.08% 6 Missing and 1 partial ⚠️
common/cmd/cmd.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1058      +/-   ##
===========================================
+ Coverage    48.90%   48.97%   +0.06%     
===========================================
  Files          158      158              
  Lines        12710    12726      +16     
===========================================
+ Hits          6216     6232      +16     
+ Misses        6064     6063       -1     
- Partials       430      431       +1     
Flag Coverage Δ
common 55.02% <0.00%> (-0.04%) ⬇️
coordinator 18.91% <ø> (ø)
database 42.85% <ø> (ø)
rollup 66.18% <81.08%> (+0.18%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

georgehao
georgehao previously approved these changes Dec 26, 2023
@georgehao georgehao merged commit 4909dc8 into develop Dec 28, 2023
21 checks passed
@georgehao georgehao deleted the fix-gas-price-bump branch December 28, 2023 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working bump-version Bump the version tag for deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants