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

Bump gas price based on elapsed blocks for cancelling #3302

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

3shv
Copy link

@3shv 3shv commented Mar 3, 2025

Related to issue #3241

@3shv 3shv requested a review from a team as a code owner March 3, 2025 10:01
Copy link

github-actions bot commented Mar 3, 2025

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@3shv 3shv force-pushed the bumpGasFeeOnResubmit branch from 1c8e5b9 to 1613373 Compare March 3, 2025 10:31
@3shv
Copy link
Author

3shv commented Mar 3, 2025

I have read the CLA Document and I hereby sign the CLA

github-actions bot added a commit that referenced this pull request Mar 3, 2025
Copy link
Contributor

@MartinquaXD MartinquaXD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.
There are 2 bigger remarks here:

  1. submission logic can be quite finicky and complex so I'd like to only add the complexity where it's necessary. This PR focuses on the refunder component which doesn't really struggle with cancellations so I'd prefer to not overcomplicate things unnecessarily.
  2. A similar reasoning applies to the component the issue was about (driver) so the change would be ideally less involved than this. It seems in the respective driver code this should be easier, since it can be solved with the introduction of one new local variable.

@3shv 3shv changed the title Bump price based on elapsed blocks for cancelling Bump gas price based on elapsed blocks for cancelling Mar 4, 2025
@3shv
Copy link
Author

3shv commented Mar 4, 2025

Thanks for your review. I updated only the mempool.rs code now. Not sure where the tests should go..

) -> Result<TxId, Error> {
let gas_price_bump_factor = GAS_PRICE_BUMP.powf(blocks_elapsed.max(1) as f64);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the issue #3241 description I think we need to just multiply the GAS_PRICE_BUMP by blocks_elapsed.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, yeah 😅

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.. Fixed it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@3shv your original version was actually correct (with the exception of using .powf() instead of powi()). Perhaps the issue was worded unfortunately.
The reason is that according to EIP-1559 the base fee can increase at most 12.5% from block n to n+1. So if we want to get the highest possible gas price after 3 blocks we would compute:
originalGasPrice * 1.125 * 1.125 * 1.125 or originalGasPrice * 1.125.powi(3).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, that's interesting.
Thanks for the clarification. Learning new stuff everyday 😇

Copy link
Contributor

@squadgazzz squadgazzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

From the original issue:

I am not sure this is also valid for BASE, so the issue needs double checking though.

Was it checked?

@3shv
Copy link
Author

3shv commented Mar 7, 2025

I am not sure this is also valid for BASE, so the issue needs double checking though.
Was it checked?

@squadgazzz I haven't checked carefully. But when I took a quick look, I couldn't find anything different in BASE chain. Please correct me if I'm mistaken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants