Skip to content
This repository has been archived by the owner on Feb 15, 2023. It is now read-only.

Split permutation from sponge construction #30

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hdevalence
Copy link
Contributor

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.

  • Targeted PR against correct branch (master)
  • Linked to Github issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • [ ] Wrote unit tests
  • 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

Work towards #29; 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.
hdevalence added a commit to penumbra-zone/poseidon377 that referenced this pull request Oct 20, 2021
This uses the API changes from:
- https://github.com/arkworks-rs/sponge/issues/29
- arkworks-rs/sponge#30
to avoid working through the Arkworks sponge interface, and do hashing using
the permutation directly.
hdevalence added a commit to penumbra-zone/poseidon377 that referenced this pull request Oct 20, 2021
This uses the API changes from:
- https://github.com/arkworks-rs/sponge/issues/29
- arkworks-rs/sponge#30
to avoid working through the Arkworks sponge interface, and do hashing using
the permutation directly.
hdevalence added a commit to penumbra-zone/poseidon377 that referenced this pull request Oct 20, 2021
This uses the API changes from:
- https://github.com/arkworks-rs/sponge/issues/29
- arkworks-rs/sponge#30
to avoid working through the Arkworks sponge interface, and do hashing using
the permutation directly.
hdevalence added a commit to penumbra-zone/poseidon377 that referenced this pull request Oct 20, 2021
This uses the API changes from:
- https://github.com/arkworks-rs/sponge/issues/29
- arkworks-rs/sponge#30
to avoid working through the Arkworks sponge interface, and do hashing using
the permutation directly.
@hdevalence
Copy link
Contributor Author

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

@Pratyush Pratyush added the breaking-change Breaking change label Oct 21, 2021
@Pratyush
Copy link
Member

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?


/// Parameters describing a Poseidon instance.
#[derive(Clone, Debug)]
pub struct Parameters<F: PrimeField> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So to clarify, these are Parameters for the underlying permutation, right? They don't have anything to do with the sponge, beyond the rate and capacity, right? If you could add a comment clarifying that, it would be great. Thanks!

src/poseidon/sponge.rs Outdated Show resolved Hide resolved
src/poseidon/state.rs Outdated Show resolved Hide resolved
@hdevalence
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@weikengchen
Copy link
Member

Feel free to apply to the constraints implementations.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
breaking-change Breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants