fix(stm): Fix the implementation difference for schnorr signature between cpu and circuit#2994
Conversation
hjeljeli32
left a comment
There was a problem hiding this comment.
@damrobi
Thanks a lot for the quick turnaround on this PR 🙏
This looks really good to me and is fully aligned with the decisions we discussed yesterday.
I left a couple of small comments regarding doc comments that are now slightly outdated, but nothing blocking from my side.
Great work 👍
There was a problem hiding this comment.
Pull request overview
This PR aligns the CPU implementation of the unique Schnorr signature with the circuit version by switching the hashing-to-curve and signature APIs to operate on Jubjub base field elements rather than raw bytes, removing the CPU-side SHA-256 step.
Changes:
- Update
hash_to_projective_point,sign, andverifyto take&[BaseFieldElement]instead of&[u8]. - Re-export
BaseFieldElementfor use in benchmarks and update benches/tests to pass field elements. - Remove the
sha2dependency and refresh golden vectors impacted by the hashing change.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| mithril-stm/src/signature_scheme/unique_schnorr_signature/signing_key.rs | sign now takes base field elements; call path updated accordingly. |
| mithril-stm/src/signature_scheme/unique_schnorr_signature/signature.rs | verify now takes base field elements; tests + golden vectors updated. |
| mithril-stm/src/signature_scheme/unique_schnorr_signature/mod.rs | Re-export BaseFieldElement; update proptests to use new API. |
| mithril-stm/src/signature_scheme/unique_schnorr_signature/jubjub/field_elements.rs | Add test/bench-only conversion from byte slices to BaseFieldElement. |
| mithril-stm/src/signature_scheme/unique_schnorr_signature/jubjub/curve_points.rs | hash_to_projective_point now hashes base field elements and drops SHA-256. |
| mithril-stm/src/lib.rs | Expose BaseFieldElement under benchmark-internals for benches. |
| mithril-stm/benches/schnorr_sig.rs | Update bench to pass &[BaseFieldElement] to sign/verify. |
| mithril-stm/Cargo.toml | Remove sha2 dependency now that SHA-256 is no longer used here. |
| Cargo.lock | Lockfile updated to reflect dependency removal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
50ebfe3 to
eb394ae
Compare
…chnorr and updated the tests
…ment by adding a sha hash of the input bytes
eb394ae to
a7b0897
Compare
Content
This PR includes a fix to the implementation of the unique schnorr signature that differs between cpu and circuit. This fix mainly consists in modifying the input to the
signandverifyfunctions of theunique_schnorr_signaturemodule to takeBaseFieldElements instead of arbitrary bytes. The bytes that were previously handled within the sign function will be treated outside of the signature in a later PR.Warning: This PR does not include the proper handling of the message bytes and merkle root before giving them to the signature function!
Content of the PR:
hash_to_projective_pointto haveBaseFieldElementas inputssignandverifyfunctions to take&[BaseFieldElement]instead of&[u8]BaseFieldElementto the benchesTryFrom<&[u8]>implementation toBaseFieldElementusing SHA256 to compress arbitrary bytes slicesPre-submit checklist
Comments
Issue(s)
Relates to #2993