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

fix: Poseidon2 verify_batch start_top_level is not constrained only at start #1403

Merged
merged 4 commits into from
Mar 10, 2025

Conversation

jonathanpwang
Copy link
Contributor

@jonathanpwang jonathanpwang commented Mar 10, 2025

Finding Link: https://cantina.xyz/code/c486d600-bed0-4fc6-aed1-de759fd29fa2/findings/167

Description of Fix

While the variable was called start_top_level, we only constrained that the top level has start_top_level = 1 but never that it must be equal to 0 on non-top levels. We change the existing constraint so that it enforces start_top_level = 1 iff its the start of a top level (meaning previous row is end and current row is incorporate_row).

Closes INT-3567

This comment has been minimized.

Copy link
Contributor

@TlatoaniHJ TlatoaniHJ 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 to me. I should note that when the row is not top level at all we are not constraining start_top_level. We don't need to because the value of start_top_level is not relevant for those rows (setting it to 1 on those rows neither allows an exploit nor causes a constraint to fail), but this maybe should be documented (or we could just constrain it to be 0 on those rows).

@jonathanpwang
Copy link
Contributor Author

Looks good to me. I should note that when the row is not top level at all we are not constraining start_top_level. We don't need to because the value of start_top_level is not relevant for those rows (setting it to 1 on those rows neither allows an exploit nor causes a constraint to fail), but this maybe should be documented (or we could just constrain it to be 0 on those rows).

I'll constrain it's zero just for clarity.

@zlangley
Copy link
Contributor

Can't we just do

    builder.assert_eq(end.clone() * next.incorporate_row, next.start_top_level);

instead of

    builder
        .when(end.clone())
        .when(next.incorporate_row)
        .assert_one(next.start_top_level);

@jonathanpwang
Copy link
Contributor Author

Can't we just do

    builder.assert_eq(end.clone() * next.incorporate_row, next.start_top_level);

instead of

    builder
        .when(end.clone())
        .when(next.incorporate_row)
        .assert_one(next.start_top_level);

I think so, will add it.

Copy link

group app.proof_time_ms app.cycles app.cells_used leaf.proof_time_ms leaf.cycles leaf.cells_used
verify_fibair (+13 [+1.0%]) 1,381 147,009 8,194,532 - - -
fibonacci_program (-24 [-0.5%]) 4,974 1,500,096 51,485,167 - - -
regex_program (+112 [+0.8%]) 14,036 4,140,164 167,389,450 - - -
ecrecover_program (-23 [-0.9%]) 2,516 295,181 15,586,346 - - -

Commit: 2c23fa1

Benchmark Workflow

Copy link
Contributor

@TlatoaniHJ TlatoaniHJ left a comment

Choose a reason for hiding this comment

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

Amazing

@jonathanpwang jonathanpwang merged commit d9f525b into main Mar 10, 2025
18 of 19 checks passed
@jonathanpwang jonathanpwang deleted the fix/start_top_level branch March 10, 2025 21:40
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.

3 participants