Add GroupCanonicalEncoding#68
Open
kayabaNerve wants to merge 2 commits intozkcrypto:release-0.14.0from
Open
Conversation
Author
|
Alternatively to |
`PrimeField::from_repr` required the representation of an element be canonical. `GroupEncoding::from_bytes` (unfortunately) did not, and implementations have not always required points be canonical. Notably, `curve25519-dalek` does not (dalek-cryptography/curve25519-dalek#380). Instead of modifying `GroupEncoding` to state representations must be canonical, which may be a debated change and would be easily overlooked when updating, a new trait is added. This shouldn't be invasive at all, yet allows callers to require a canonical encoding and implementations to declare they offer one. I'm unaware of any library whose `to_bytes` isn't canonical, yet I added a `to_canonical_bytes` for parity with `from_canonical_bytes`. `from_bytes_unchecked` is left without a counterpart as the entire point of this trait is to perform a canonicity check. Descending from `GroupEncoding` (necessary to access `Repr` via) allows the original `from_bytes_unchecked` to still be used. This is necessary as currently, within a generic setting, a bespoke marker trait must be defined and manually implemented, or callers must assume `to_bytes` will be canonical and after decoding, re-encode to check for equivalency. This generally incurs a field inversion which is quite expensive.
Author
|
Updated to target the 0.14 branch. The first commit is not a marker trait yet a bespoke trait, as described in the PR. The second commit changes it to a marker trait which allows implementation in a single line (albeit with an inefficient |
kayabaNerve
added a commit
to serai-dex/serai
that referenced
this pull request
Sep 3, 2025
This helps identify where the various functionalities are used, or rather, not used. The `Ciphersuite` trait present in `patches/ciphersuite`, facilitating the entire FCMP++ tree, only requires the markers _and_ canonical point decoding. I've opened a PR to upstream such a trait into `group` (zkcrypto/group#68). `WrappedGroup` is still justified for as long as `Group::generator` exists. Moving `::generator()` to its own trait, on an independent structure (upstream) would be massively appreciated. @tarcieri also wanted to update from `fn generator()` to `const GENERATOR`, which would encourage further discussion on zkcrypto/group#32 and zkcrypto/group#45, which have been stagnant. The `Id` trait is occasionally used yet really should be first off the chopping block. Finally, `WithPreferredHash` is only actually used around a third of the time, which more than justifies it being a separate trait. --- Updates `dalek_ff_group::Scalar` to directly re-export `curve25519_dalek::Scalar`, as without issue. `dalek_ff_group::RistrettoPoint` also could be replaced with an export of `curve25519_dalek::RistrettoPoint`, yet the coordinator relies on how we implemented `Hash` on it for the hell of it so it isn't worth it at this time. `dalek_ff_group::EdwardsPoint` can't be replaced for an re-export of `curve25519_dalek::SubgroupPoint` as it doesn't implement `zeroize`, `subtle` traits within a released, non-yanked version. Relevance to #201 and dalek-cryptography/curve25519-dalek#811 (comment). Also updates the `Ristretto` ciphersuite to prefer `Blake2b-512` over `SHA2-512`. In order to maintain compliance with FROST's IETF standard, `modular-frost` defines its own ciphersuite for Ristretto which still uses `SHA2-512`.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PrimeField::from_reprrequired the representation of an element be canonical.GroupEncoding::from_bytes(unfortunately) did not, and implementations have not always required points be canonical. Notably,curve25519-dalekdoes not (dalek-cryptography/curve25519-dalek#380).Instead of modifying
GroupEncodingto state representations must be canonical, which may be a debated change and would be easily overlooked when updating, a new trait is added. This shouldn't be invasive at all, yet allows callers to require a canonical encoding and implementations to declare they offer one.I'm unaware of any library whose
to_bytesisn't canonical, yet I added ato_canonical_bytesfor parity withfrom_canonical_bytes.from_bytes_uncheckedis left without a counterpart as the entire point of this trait is to perform a canonicity check. Descending fromGroupEncoding(necessary to accessReprvia) allows the originalfrom_bytes_uncheckedto still be used.This is necessary as currently, within a generic setting, a bespoke marker trait must be defined and manually implemented, or callers must assume
to_byteswill be canonical and after decoding, re-encode to check for equivalency. This generally incurs a field inversion which is quite expensive.I'm aware an RFC process is defined, but it isn't required for anything which isn't invasive. I believe this is sufficiently not invasive to scrape by. Please correct me if I'm wrong.