sponge icon indicating copy to clipboard operation
sponge copied to clipboard

Split permutation from sponge construction

Open hdevalence opened this issue 4 years ago • 5 comments

Work towards arkworks-rs/crypto-primitives#93; this doesn't touch the constraint system implementation yet, in order to be able to get design feedback on the software part.

  • The poseidon::PoseidonParameters struct is renamed to poseidon::Parameters but otherwise remains unchanged.

  • The poseidon::PoseidonSpongeState struct is renamed to poseidon::State and redefined to hold just the state itself, as well as the parameters needed to run the permutation. It exposes a permute(&mut self) method, rate() and capacity() accessors, as well as Index, IndexMut, AsRef, and AsMut impls that allow access to the state.

  • The poseidon::PoseidonSponge struct is renamed to poseidon::Sponge and holds a State and a DuplexSpongeMode. In other words, it consists of the state, together with the extra data tracking how that state is being used to implement a higher-level duplex construction.

  • The CryptographicSponge trait is changed so that new() takes an owned, Self::Parameters, not a borrowed one. This allows the caller to decide where to copy data, instead of forcing the sponge implementation to clone internally. Or, a CryptographicSponge implementation could declare the associated Parameters type to be some shared type (like an Arc wrapper) that avoids the need to copy at all.

  • The SpongeExt trait that allows converting back and forth between a state and a sponge is deleted; it's not safe to pass between abstraction layers that way.


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.

  • [x] Targeted PR against correct branch (master)
  • [x] Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • ~~[ ] Wrote unit tests~~
  • [x] Updated relevant documentation in the code
  • [ ] Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • [ ] Re-reviewed Files changed in the Github PR explorer

hdevalence avatar Oct 19 '21 23:10 hdevalence

(A rendered copy of the docs is available here: https://rustdoc.penumbra.zone/main/ark_sponge/poseidon/index.html)

hdevalence avatar Oct 20 '21 03:10 hdevalence

The changes look great so far, though I'm not the most qualified to review this stuff; @ValarDragon and @weikengchen are more familiar with this code. Also, is there something specific that you'd like to get feedback on?

Pratyush avatar Oct 21 '21 18:10 Pratyush

Also, is there something specific that you'd like to get feedback on?

Yeah, the main thing is whether this general approach seems good, before doing the work of also updating the constraint implementations.

hdevalence avatar Oct 21 '21 18:10 hdevalence

Hey, just bumping this -- if this approach seems good I can also apply it to the constraint implementations.

hdevalence avatar Nov 11 '21 23:11 hdevalence

Feel free to apply to the constraints implementations.

weikengchen avatar Nov 13 '21 09:11 weikengchen