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

hotfix: make aggregator fee multiplier customizable #1690

Open
wants to merge 4 commits into
base: testnet
Choose a base branch
from

Conversation

Oppen
Copy link
Collaborator

@Oppen Oppen commented Dec 30, 2024

  1. Change some constants:
    • AGGREGATOR_GAS_COST: 400_000 => 330_000;
    • ADDITIONAL_SUBMISSION_GAS_COST_PER_PROOF: 13_000 => 2_000;
    • DEFAULT_AGGREGATOR_FEE_PERCENTAGE_MULTIPLIER: 150 => 125;
  2. Take the aggregator fee percentage multiplier from batcher config: batcher.aggregator_fee_percentage_multiplier, using DEFAULT_AGGREGATOR_FEE_PERCENTAGE_MULTIPLIER as a default if not set;
  3. Compute the constant gas cost on runtime from batcher.aggregator_fee_percentage_multiplier config.

Testing

Basic testing involves just the regular flow, start a devnet, send some proofs, check that they verify and batches make it to L1. The expected behavior is cheaper costs, in the telemetry it should show changes vs testnet in fee per proof.

To test more in depth, try:

  • Not setting anything under batcher.aggregator_fee_percentage_multiplier, it should work and charge the same (the defaults in the example config files and the constant match);
  • Setting much lower values;
  • Setting much higher values,
    Each of these changes should require only a batcher restart, not recompilation and not restarting of other components.

Type of change

  • Refactor

Checklist

  • “Hotfix” to testnet, everything else to staging
  • Linked to Github Issue
  • This change depends on code or research by an external entity
    • Acknowledgements were updated to give credit
  • Unit tests added
  • This change requires new documentation.
    • Documentation has been added/updated.
  • This change is an Optimization
    • Benchmarks added/run
  • Has a known issue
  • If your PR changes the Operator compatibility (Ex: Upgrade prover versions)
    • This PR adds compatibility for operator for both versions and do not change batcher/docs/examples
    • This PR updates batcher and docs/examples to the newer version. This requires the operator are already updated to be compatible

@Oppen Oppen marked this pull request as ready for review December 30, 2024 23:03
Copy link
Collaborator

@MarcosNicolau MarcosNicolau left a comment

Choose a reason for hiding this comment

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

Works for me.

Copy link
Contributor

@PatStiles PatStiles left a comment

Choose a reason for hiding this comment

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

Worked locally LGTM

pub const BATCHER_SUBMISSION_BASE_GAS_COST: u128 = 125_000;
pub const ADDITIONAL_SUBMISSION_GAS_COST_PER_PROOF: u128 = 13_000;
pub const ADDITIONAL_SUBMISSION_GAS_COST_PER_PROOF: u128 = 2_000;
Copy link
Contributor

Choose a reason for hiding this comment

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

verify this number is OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes it is ok

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