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: add priority fees to gas settings #10763

Merged
merged 8 commits into from
Dec 18, 2024
Merged

feat: add priority fees to gas settings #10763

merged 8 commits into from
Dec 18, 2024

Conversation

LeilaWang
Copy link
Collaborator

Please read contributing guidelines and remove this line.

@LeilaWang LeilaWang linked an issue Dec 16, 2024 that may be closed by this pull request
Copy link
Contributor

github-actions bot commented Dec 16, 2024

Changes to circuit sizes

Generated at commit: 2850d563baf604c47437f0aa5a583a2a59df9358, compared to commit: a3fba8442fdd62f429054c3367984fd4206bbbeb

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
private_kernel_init +9 ❌ +0.03% +9 ❌ +0.02%
private_kernel_tail +4 ❌ +0.07% +2 ❌ +0.01%
private_kernel_inner +6 ❌ +0.01% +6 ❌ +0.01%
private_kernel_tail_to_public +4 ❌ +0.03% +2 ❌ +0.01%
private_kernel_reset_4_4_4_4_4_4_4_4_4 +4 ❌ +0.01% +6 ❌ +0.01%
rollup_base_private +172 ❌ +0.00% +466 ❌ +0.00%
private_kernel_reset +4 ❌ +0.00% +4 ❌ +0.00%
private_kernel_empty +2 ❌ +0.21% +2 ❌ +0.00%
rollup_base_public 0 ➖ 0.00% +12 ❌ +0.00%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
private_kernel_init 27,953 (+9) +0.03% 40,483 (+9) +0.02%
private_kernel_tail 5,342 (+4) +0.07% 15,189 (+2) +0.01%
private_kernel_inner 46,260 (+6) +0.01% 64,366 (+6) +0.01%
private_kernel_tail_to_public 15,896 (+4) +0.03% 30,041 (+2) +0.01%
private_kernel_reset_4_4_4_4_4_4_4_4_4 36,615 (+4) +0.01% 92,096 (+6) +0.01%
rollup_base_private 3,619,773 (+172) +0.00% 11,261,969 (+466) +0.00%
private_kernel_reset 97,779 (+4) +0.00% 634,165 (+4) +0.00%
private_kernel_empty 967 (+2) +0.21% 902,002 (+2) +0.00%
rollup_base_public 3,851,252 (0) 0.00% 12,654,746 (+12) +0.00%

@fcarreiro fcarreiro removed their request for review December 17, 2024 10:09
Copy link
Collaborator

@sirasistant sirasistant left a comment

Choose a reason for hiding this comment

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

Looks good! Just a doubt and one missing thing 🚀

@@ -20,13 +21,20 @@ export async function cancelTx(
return;
}

const maxFeesPerGas = new GasFees(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to also increase the max fee per gas? I'd expect only increasing priority fees for cancellation 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I included max fees because the gasSettings passed to this function is fetched from the previous tx. So the max fees might be tailored to only fit the previous tx's fees. But now you pointed this out, I guess a better approach is for the user to also be able to increase the max fees for this new tx. Will change it now! Thanks 😃

@@ -46,6 +47,7 @@ inline void read(uint8_t const*& it, GasSettings& gas_settings)
read(it, gas_settings.gas_limits);
read(it, gas_settings.teardown_gas_limits);
read(it, gas_settings.max_fees_per_gas);
read(it, gas_settings.max_priority_fees_per_gas);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we also need to update fee calculation in AvmTraceBuilder::pay_fee

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh right! Didn't realise fee computation is already implemented there.

@LeilaWang LeilaWang merged commit 263eaad into master Dec 18, 2024
73 of 77 checks passed
@LeilaWang LeilaWang deleted the lw/priority_fees branch December 18, 2024 09:38
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.

feat: add max_priority_fee_per_gas to GasSettings
2 participants