Skip to content

Conversation

@Oppen
Copy link
Contributor

@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. Renamed AGGREGATOR_GAS_COST and CONSTANT_GAS_COST to DEFAULT_AGGREGATOR_GAS_COST and DEFAULT_CONSTANT_GAS_COST respectively;
  4. Compute the constant gas cost on runtime from batcher.aggregator_fee_percentage_multiplier config and batcher.aggregator_gas_cost;
  5. Both these config variables are new.

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.
    Same for batcher.aggregator_gas_cost, which is 330k by default.

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
Member

@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

@Oppen Oppen force-pushed the hotfix/agg_fee_multiplier_cfg branch from 0a4860f to 263f5bd Compare January 7, 2025 17:08
Copy link
Contributor

@avilagaston9 avilagaston9 left a comment

Choose a reason for hiding this comment

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

We are debugging an issue when trying to submit only one proof

Copy link
Contributor

@avilagaston9 avilagaston9 left a comment

Choose a reason for hiding this comment

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

We understood the problem. It wasn't an issue!

@avilagaston9
Copy link
Contributor

I think we should also update infra/ansible/playbooks/templates/config-files/config-batcher.yaml.j2.

@JuArce JuArce merged commit cd0c331 into testnet Jan 10, 2025
3 checks passed
@JuArce JuArce deleted the hotfix/agg_fee_multiplier_cfg branch January 10, 2025 14:32
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.

8 participants