Refactor hash-to-curve around digest::XofReader trait #643
+424
−379
Conversation
This file contains 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
burdges
requested review from
Pratyush,
mmagician and
weikengchen
and removed request for
a team
This was referenced Apr 25, 2023
|
Do you guys want this PR to be from a non-organizational repo so that you can change it? I could move somewhere else or @mmagician could move it to be internal or something else? |
MapToCurve::new seemingly originates from a more runtime oriented elliptic curve crate: https://github.com/armfazh/redox-ecc/blob/master/src/ellipticcurve.rs#L36 Arguably test_parameters should be inherent methods, invoked by whoever defined the parameters, but maybe not? I left the trait method for now.
Co-authored-by: Pratyush Mishra <[email protected]>
Co-authored-by: Pratyush Mishra <[email protected]>
Co-authored-by: Pratyush Mishra <[email protected]>
Co-authored-by: Pratyush Mishra <[email protected]>
rustfmt is wrong that f()? should be preferred over f() ?, as errors paths should often be highlighted, but nobody merged that fix yet, so whatever. rust-lang/rustfmt#5595
added 21 commits
June 11, 2023 17:11
Interface remains shit, but it becomes fixable now.
I'd prefer free fns here myself, but likely everyone else prefers some trait, and it does make the interface more consistent.
work quite right yet
6 tasks
|
@burdges any progress on splitting up the hashing PRs as we discussed offline, for reviews to be more "digestible"? |
|
I've been too distracted lately but I'll try to get to it next week. It's really not that hard. |
This was referenced Aug 29, 2023
Closed
|
Odd, why does 15fc1a5 not appear above now? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Addresses several concerns:
Hash-to-field and hash-to-curve are logically consumers of XoFs. The hash-to-curve spec is written this way. Also, ristretto exploits this, likely so do other elligator deployments, including some ed25519 protocols. It follows polymorphic implementations should expose hashing directly from XoFs, but still push users towards compliant domain separation. In fact, arkworks' hash-to-curve cannot currently use shake cipher suites, or other XoFs, only sha2 ones, which excludes some elligator curves, and all curves with security levels above 128 bits.
At the same time, the hash-to-curve spec creates messy dependencies across its internal XoF boundary. In particular, a security parameter plays several roles but depends upon the curve, or even the specific hash-to-curve. We hide this annoying security parameter in the MapToCurve trait. It's plausible but uncertain if
digest::core_api::BlockSizeUsermight permit computing this security parameter from the hash selected.Arkworks'
Field as UniformRandcannot be made constant-time, so this exposes hashing-to-fields directly. At present, this hashing-to-fields is not constant-time because arkworks' field arithmetic is not constant-time, but at least one could deploy a protocol built with hash-to-field and later do constant-time field code.We do not expose hash-to-field in exactly the style of the hash-to-curve spec. A hash-to-curve for bls12-381 has 128 bits of security, but one could build a curve with higher security with the same base field or whose group order matched the base field, and then require shake256.
Arkworks needs considerable other boilerplate for hash-to-curve. We make a true default-per-curve hash-to-curve possible.
We preserve roughly the old MapToCurve interface, which already looked suitable for some snark friendly hash-to-curve schemes, although perhaps not within its security assumptions. It's likely the new XoF interface supports some other snark friendly hash-to-curve schemes.
XofVecis an ugly hack, but frankly so is this whole sha2 XoF. This PR removes a lot of needless allocations elsewhere though.We should discuss this new interface further of course, so no curves PR for now, but doing one looks pretty trivial.
supersedes: #627
closes: #629 & #630
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
Pendingsection inCHANGELOG.mdFiles changedin the GitHub PR explorer