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

chore: bumping constants #9431

Merged
merged 1 commit into from
Oct 25, 2024
Merged

chore: bumping constants #9431

merged 1 commit into from
Oct 25, 2024

Conversation

benesjan
Copy link
Contributor

@benesjan benesjan commented Oct 25, 2024

I needed to modify some constants due to changes in partial notes and I decided to do this in a separate PR from the one up the stack to make it all easier to review.

Copy link
Contributor Author

benesjan commented Oct 25, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @benesjan and the rest of your teammates on Graphite Graphite

@dbanks12
Copy link
Contributor

I'd hesitate to actually merge a bump to 12 million gas until we confirm

  1. where this cost blowup is coming from
  2. whether we are anywhere near overflowing the AVM trace
  3. and whether we suspect that upcoming optimizations (like to sha256) will bring this cost down

otherwise we might be screwing ourselves

Copy link
Contributor

Changes to circuit sizes

Generated at commit: c0e9a595ccade91e7a1251ce77a00a610c69d161, compared to commit: 7766c8e714185d6e8b9fa392d7f371fb30da8f1a

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
public_kernel_inner +78,400 ❌ +29.64% +160,584 ❌ +31.55%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
public_kernel_inner 342,892 (+78,400) +29.64% 669,595 (+160,584) +31.55%

@benesjan benesjan enabled auto-merge (squash) October 25, 2024 18:13
Copy link
Contributor

@dbanks12 dbanks12 left a comment

Choose a reason for hiding this comment

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

Approving since there are also external users/devs running into the gas limit. We can revisit lowering this once

  1. the sha256 blowup is reduced
  2. we have a better way to profile AVM execution
  3. we have a better idea of what gas limit will safely prevent AVM trace overflow (or we choose some other way to prevent AVM trace overflow)

@benesjan benesjan merged commit 91c50dd into master Oct 25, 2024
63 checks passed
@benesjan benesjan deleted the 10-25-chore_bumping_constants branch October 25, 2024 18:15
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.

2 participants