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

feat(sender): do not bump gas price for nonce-gapped transactions #1458

Merged

Conversation

colinlyguo
Copy link
Member

@colinlyguo colinlyguo commented Jul 19, 2024

Purpose or design rationale of this PR

One of the mitigations of commit stuck due to blob fee spike issue:

Consider: Only bump fees for the next executable transaction. But we should carefully consider this: 
* If fees spike, and we only bumped the next tx, once it’s included, we’ll need to bump the next one too, increasing the commit latency. 
* So “bump fees only for next executable transaction” might need to be paired with a more aggressive fee bump logic.

The above considerations can be done by:

  1. Compare confirmed nonce with current tx, if tx.Nonce > confirmed nonce + 1, then skip bumping the gas price of this transaction.
  2. When resubmitting type 2 and 3 transactions, it will compare current basefee and blobbasefee with L1's at that time, leaving gas tip bumping "less aggressive" than before.

Why query a full node instead of using db's confirmed status of a tx?

  • The latency to get "if the last tx is confirmed" could be smaller, using the "latest" tag is reliable and sufficient in most cases.
  • Reduce db read ops.

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:

  • feat: A new feature

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

@Thegaram Thegaram self-requested a review July 19, 2024 10:25
@codecov-commenter
Copy link

codecov-commenter commented Jul 19, 2024

Codecov Report

Attention: Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.

Project coverage is 52.63%. Comparing base (04215f3) to head (9d4a4fe).

Files Patch % Lines
rollup/internal/controller/sender/sender.go 63.63% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1458   +/-   ##
========================================
  Coverage    52.63%   52.63%           
========================================
  Files          156      156           
  Lines        12426    12435    +9     
========================================
+ Hits          6540     6545    +5     
- Misses        5325     5328    +3     
- Partials       561      562    +1     
Flag Coverage Δ
rollup 57.16% <63.63%> (-0.01%) ⬇️

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.

@colinlyguo colinlyguo added the bump-version Bump the version tag for deployment label Jul 24, 2024
@0xmountaintop 0xmountaintop merged commit 85e2e7a into develop Aug 2, 2024
5 checks passed
@0xmountaintop 0xmountaintop deleted the fix-do-not-bump-gas-price-for-nonce-gapped-transactions branch August 2, 2024 05:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump-version Bump the version tag for deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants