-
Notifications
You must be signed in to change notification settings - Fork 279
secp256k1 ECDSA verification example. Uses new secp256k1 inlines which requires a refactor to allow for custom trace generation. #1178
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
Conversation
…Shrinks binaries with no costs and avoids linker errors that come up with crazy inlining
|
Sorry for the delay, will review today. |
0xAndoroid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this task on, and sorry for the delay in review process.
Mostly some nits, just one big change is to change the custom trace function with a custom advice function or something; and to not spoil the proof on signature verification.
There's also a pending question about how this advice going to interact with GPU.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
0xAndoroid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional Review: Incorrect hcf() Usage
The hcf() function should only be used when advice is invalid (prover is cheating). For invalid user input, use regular panic!() so the proof remains valid but execution terminates.
Incorrect usages that should use panic!() instead of hcf():
sdk.rs:74-Secp256k1Fq::from_u64_arr- Invalid field element from user inputsdk.rs:236-Secp256k1Fr::from_u64_arr- Invalid field element from user inputsdk.rs:383-Secp256k1Point::new- Point not on curve from user input
Remove ecdsa_verify_hard_fail entirely (sdk.rs:793-807)
Per the design requirements:
- Valid signature → successful proof, no panic
- Invalid signature → successful proof with panic or
Err()return - Invalid advice → spoil proof via
hcf()
ecdsa_verify_hard_fail using hcf() for invalid signatures is incorrect - it spoils the proof when it should just panic normally. Since ecdsa_verify_soft_fail already handles the correct behavior (returning Result), this hard fail variant should be removed to avoid misuse.
Correct usages (keep as-is):
sdk.rs:193-Fq::divadvice verification (c*b==a) ✓sdk.rs:330-Fr::divadvice verification (c*b==a) ✓sdk.rs:347-Fr::divcanonical form check ✓sdk.rs:617-decompose_scalaradvice verification ✓
0xAndoroid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review: Incorrect hcf() usage
|
The conclusion on the |
…hcf via unwrap or spoil proof
Signed-off-by: Andrew Tretyakov <[email protected]>
Signed-off-by: Andrew Tretyakov <[email protected]>
0xAndoroid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pushing this through @mathmasterzach
Great work!
InlineTraceFunctionto allow for custom trace generation for user defined inlines (e.g. supplying a/b as advice to a user defined inline).VirtualAssertEQto allow for "halt-and-catch-fire" style instructions outside of inlines (to spoil proofs when conditions aren't met)..eh_framerelocations.