Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved issues were identified, and the supplied assessments support approval.
Pull request overview
Adds Bandersnatch support to EdDSA key and signature decoding, with renewed integration-test coverage.
Changes:
- Decode Bandersnatch compressed points using the correct package.
- Support Bandersnatch signature scalar offsets.
- Re-enable Bandersnatch EdDSA tests.
File summaries
| File | Changes |
|---|---|
std/signature/eddsa/eddsa.go |
Adds Bandersnatch point and signature parsing. |
std/signature/eddsa/eddsa_test.go |
Re-enables Bandersnatch EdDSA tests. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Thanks for the PR! gnark-crypto now has the fix, can you also |
…ch EdDSA test Depends on Consensys-Incorporated/gnark-crypto#882 Signed-off-by: Reno <renaud.dubois.fr@gmail.com>
…Incorporated/gnark-crypto#882) Signed-off-by: Reno <renaud.dubois.fr@gmail.com>
12fb2c5 to
a0d0387
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Done: rebased on master and bumped gnark-crypto to master (go get github.com/consensys/gnark-crypto@master, the pseudo-version carrying #882). TestEddsa passes locally including the re-enabled Bandersnatch configuration; the CodeQL job should be green too now. |
ivokub
left a comment
There was a problem hiding this comment.
Yep, looks good -- the tests pass locally for me as well. The failing CI test is due to code generation update which updates the included testing tinyfield package. I'll address it in a separate PR.
Depends on Consensys-Incorporated/gnark-crypto#882
Description
Solve an incorrect import that drag Jujub instead of Bandersnatch. See also issue 881
Note
Medium Risk
Touches EdDSA witness decoding for a new curve; incorrect parsing would break or mis-verify circuits, but the change is narrow and covered by re-enabled tests.
Overview
Adds Bandersnatch (BLS12-381) support for decompressing public keys and signatures in the std EdDSA
Assignpath, and bumpsgnark-cryptoto pick up the matching curve definitions (Consensys-Incorporated/gnark-crypto#882).parseSignatureandparsePointnow handletedwards.BLS12_381_BANDERSNATCHusingecc/bls12-381/bandersnatchpoint types instead of the wrong Jubjub/twisted-Edwards import (issue #881). The integration test for Bandersnatch + MiMC is turned back on, with forge-test offset handling aligned to the other 32-byte curves.Reviewed by Cursor Bugbot for commit a0d0387. Bugbot is set up for automated code reviews on this repo. Configure here.