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.

Sign up for GitHub

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

Jump to bottom

fix: small fixes to avm witgen #10749

Merged
merged 2 commits into from
Dec 18, 2024
Merged

fix: small fixes to avm witgen #10749

merged 2 commits into from
Dec 18, 2024

Conversation

IlyasRidhuan
Copy link
Contributor

@IlyasRidhuan IlyasRidhuan commented Dec 15, 2024 •
edited
Loading

  • Add tree snapshots to nested call contexts
  • Handle DA limits properly in witgen
  • Respect staticcall in tree operations

@IlyasRidhuan Graphite App
Copy link
Contributor Author

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

@IlyasRidhuan IlyasRidhuan changed the title fix: add tree snapshots to ctx fix: small fixes to avm witgen Dec 16, 2024
@IlyasRidhuan IlyasRidhuan marked this pull request as ready for review December 16, 2024 15:01
dbanks12
dbanks12 approved these changes Dec 16, 2024
View reviewed changes
barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp
Comment on lines +2646 to 2845
uint32_t unique_public_data_slot_writes = get_public_data_writes_count();
AvmError error = current_ext_call_ctx.is_static_call ? AvmError::STATIC_CALL_ALTERATION
: unique_public_data_slot_writes >= MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX
? AvmError::SIDE_EFFECT_LIMIT_REACHED
: AvmError::NO_ERROR;

if (storage_write_counter >= MAX_PUBLIC_DATA_UPDATE_REQUESTS_PER_TX) {
if (!is_ok(error)) {
// NOTE: the circuit constraint for this limit should only be applied
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be able to remove error = ... SIDE..LIMIT.. below

@IlyasRidhuan IlyasRidhuan force-pushed the ir/12-15-fix_add_tree_snapshots_to_ctx branch 2 times, most recently from fe89dcc to c114f31 Compare December 17, 2024 11:54
@IlyasRidhuan IlyasRidhuan force-pushed the ir/12-15-fix_add_tree_snapshots_to_ctx branch from c114f31 to e81f2ad Compare December 18, 2024 10:16
@github-actions GitHub Actions
Copy link
Contributor

Changes to circuit sizes

Generated at commit: e11e6e480b00f8adf186d2046daac1898d7aa26f, compared to commit: c90bb16a5880c42752809f383f517181e6f8a53a

🧾 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%
private_kernel_reset +4 ❌ +0.00% +4 ❌ +0.00%
rollup_base_public 0 ➖ 0.00% -37,017 ✅ -0.29%
rollup_base_private +172 ❌ +0.00% -36,564 ✅ -0.32%
rollup_block_root 0 ➖ 0.00% -111,087 ✅ -2.52%
rollup_block_merge 0 ➖ 0.00% -74,057 ✅ -3.75%
rollup_root 0 ➖ 0.00% -74,057 ✅ -3.75%
rollup_merge 0 ➖ 0.00% -74,025 ✅ -4.07%
parity_root 0 ➖ 0.00% -148,083 ✅ -4.07%
private_kernel_empty +2 ❌ +0.21% -36,995 ✅ -4.10%
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%
private_kernel_reset 97,779 (+4) +0.00% 634,165 (+4) +0.00%
rollup_base_public 3,851,252 (0) 0.00% 12,617,717 (-37,017) -0.29%
rollup_base_private 3,619,773 (+172) +0.00% 11,224,939 (-36,564) -0.32%
rollup_block_root 682,326 (0) 0.00% 4,293,399 (-111,087) -2.52%
rollup_block_merge 32,406 (0) 0.00% 1,900,801 (-74,057) -3.75%
rollup_root 32,390 (0) 0.00% 1,900,787 (-74,057) -3.75%
rollup_merge 1,791 (0) 0.00% 1,744,952 (-74,025) -4.07%
parity_root 4,290 (0) 0.00% 3,487,933 (-148,083) -4.07%
private_kernel_empty 967 (+2) +0.21% 865,005 (-36,995) -4.10%

@IlyasRidhuan IlyasRidhuan merged commit 96887b6 into master Dec 18, 2024
76 of 78 checks passed
@IlyasRidhuan IlyasRidhuan deleted the ir/12-15-fix_add_tree_snapshots_to_ctx branch December 18, 2024 12:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbanks12 dbanks12 dbanks12 approved these changes

@jeanmon jeanmon Awaiting requested review from jeanmon jeanmon is a code owner

@fcarreiro fcarreiro Awaiting requested review from fcarreiro fcarreiro is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@IlyasRidhuan @dbanks12