Poseidon hash #114

GhostOfGauss · 2022-02-04T18:15:37Z

This PR adds our first hash function--the optimized Poseidon hash, complete with parameter generation written in Rust. Credit goes to @tsunrise and @BoyuanFeng for writing this. The contents of this PR is adapted from their implementation here. (See also this spec) The biggest change is the addition of a HashFunction trait and making hashing stateless.

Some work remains, but I'm opening this draft to start collecting feedback. What remains is documentation and testing.

Feedback is needed on the overall interface with hash functions. The HashFunction trait is a starting point for discussion. This topic was opened in Issue #109 and discussion should continue there.

This will close #104

LukePearson1 · 2022-02-04T20:21:07Z

Is this PR ready for review @GhostOfGauss? If so, please convert from draft.

GhostOfGauss · 2022-02-04T21:21:30Z

No, not ready yet

bhgomes · 2022-02-04T21:58:43Z

plonk-hashing/src/lib.rs

+#[macro_use]
+pub extern crate ark_std;


We should not depend on ark_std and use the core and alloc libraries instead.

Okay, took care of this. There were two spots (here and here ) where Manta's implementation uses ark_ff::vec and I was unsure whether to remove these. It compiles fine without these imports--can you confirm that it's safe to remove them?

The ark_ff::vec should be removed in favor of alloc::vec. It's just a re-export.

bhgomes · 2022-02-04T22:03:00Z

plonk-hashing/src/poseidon/poseidon.rs

+    }
+}
+
+trait HashFunction<const WIDTH: usize, COM = ()> {


This should be arity, not width since the notion of "width" is a poseidon-only thing.

Agreed, but see comment below regarding const generic expressions

bhgomes · 2022-02-04T22:09:34Z

plonk-hashing/src/poseidon/poseidon.rs

+    // The input ought to have size ARITY, but const generic expressions aren't
+    // allowed yet
+    fn hash(&self, input: &[Self::Input], compiler: &mut COM) -> Self::Output;


We can use const-generics. See this example: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=cc008c861ad5c5455d1ca26622284a4b.

But can we use const-generic expressions? The issue I was referring to is that as you said above, HashFunction trait should have the generic ARITY but within PoseidonSpec we need to arrays whose size is ARITY + 1. That produces "error: generic parameters may not be used in const operations" with the suggestion to "use #![feature(generic_const_exprs)] to allow generic const expressions"

The notion of width is internal to Poseidon, so the way it's represented in memory is an implementation detail, not something that is exposed to the interface. Using arrays would be nice but since the feature is unstable, the other options are to use a Vec for the state vector (of length ARITY + 1) or to separate the state into two parts like this:

type State = (F, [F; ARITY]);

In either case, the input to the hash function is of the form [F; ARITY], the rest of the state is not part of the interface.

NB: In my implementation in manta-rs I use Vec for simplicity but for the production circuit I'm going to try and use State as above.

Just a side note, this poseidon implementation use WIDTH (ARITY + 1) instead of ARITY. We can change it to ARITY once generic_const_exprs is stable.
@bhgomes I guess we have to use slice instead of const generics? because

as @GhostOfGauss said, cannot evaluate WIDTH - 1 at compile time

compiler will probably optimize the overhead out?

@tsunrise To make it equivalent to the array case in terms of efficiency, we only need to keep all the data on the stack, so we can use something like (F, [F; ARITY]) as the state vector. This also preserves the input memory format since the state enters the algorithm in these two parts anyway, so copy elision will remove some overhead here that you would have if you had to copy everything into a [F; ARITY + 1] which is more nontrivial to optimize. Maybe the Rust optimizer is very good but I wouldn't rely on optimizations that are not explicitly based on the language semantics like ownership and moving.

markulf · 2022-02-06T20:41:19Z

plonk-hashing/src/poseidon/poseidon_ref.rs

+        type NepArity = generic_array::typenum::U2;
+
+        let (nep_consts, ark_consts) =
+            collect_neptune_constants::<NepArity>(Strength::Standard);


collect_neptune_constants is missing, as well as neptune dev_dependencies? Nice to have these tests.

This code is from plonk_prototype repo and we temporarily used neptune library as a reference to check the correctness of our code.

Anyway, we should not even import neptune library here. We need to hardcoded tests.

You mean hardcoded test vectors. Sounds good.

Those hardcoded test vectors are now here: https://github.com/Manta-Network/Plonk-Prototype/blob/d2e25aae7ad4526791f4543cebf1e4df5fbf14ac/src/poseidon/poseidon_ref.rs#L380-L415. Those test vectors come from my modified variants of the original sage script. @GhostOfGauss You can add this test here remove any dependency on Neptune

markulf · 2022-02-06T20:44:35Z

plonk-hashing/src/poseidon/poseidon_ref.rs

+        });
+        let plonk_hash = poseidon_circuit.output_hash(&mut c);
+
+        c.check_circuit_satisfied();


check_circuit_satisfied requires #[cfg(feature = "trace")]

markulf · 2022-02-06T22:41:21Z

plonk-hashing/src/poseidon/poseidon_ref.rs

+        constants: PoseidonConstants<S::ParameterField>,
+    ) -> Self {
+        let mut elements = S::zeros(c);
+        elements[0] = S::alloc(c, constants.domain_tag);


This loads constants.domain_tag into a witness variable using c.add_input, but how do we constrain the witness to correspond to the actual constants?

Good point... a naive way is that for the input phase we can just use an zero_var for element[0] and keep track of domain tag, and then we can handle this in the first round of add_round_constants and s-box.

tsunrise

Thanks

tsunrise · 2022-02-10T07:51:31Z

plonk-hashing/src/poseidon/constraints.rs

@@ -0,0 +1,473 @@
+//! Library independent specification for field trait.


I don't think this file is needed here

tsunrise · 2022-02-10T07:58:45Z

plonk-hashing/src/poseidon/poseidon.rs

+    // The input ought to have size ARITY, but const generic expressions aren't
+    // allowed yet
+    fn hash(&self, input: &[Self::Input], compiler: &mut COM) -> Self::Output;


Just a side note, this poseidon implementation use WIDTH (ARITY + 1) instead of ARITY. We can change it to ARITY once generic_const_exprs is stable.
@bhgomes I guess we have to use slice instead of const generics? because

as @GhostOfGauss said, cannot evaluate WIDTH - 1 at compile time

compiler will probably optimize the overhead out?

tsunrise · 2022-02-10T08:04:46Z

plonk-hashing/src/poseidon/poseidon_ref.rs

+        constants: PoseidonConstants<S::ParameterField>,
+    ) -> Self {
+        let mut elements = S::zeros(c);
+        elements[0] = S::alloc(c, constants.domain_tag);


Good point... a naive way is that for the input phase we can just use an zero_var for element[0] and keep track of domain tag, and then we can handle this in the first round of add_round_constants and s-box.

tsunrise · 2022-02-10T08:06:16Z

plonk-hashing/src/poseidon/poseidon_ref.rs

+        )
+    }
+
+    #[test]


we do not need to test on r1cs.

(Indeed, we should remove everything related r1cs here. It's not relevant. )

tsunrise · 2022-02-10T08:06:53Z

plonk-hashing/src/poseidon/poseidon_ref.rs

+        conversion::cast_field,
+        neptune_hyper_parameter::collect_neptune_constants,
+    };
+    use neptune::{


We should use some hardcoded test instead of relying on neptune library.

tsunrise · 2022-02-10T08:08:09Z

plonk-hashing/src/poseidon/poseidon_ref.rs

+        type NepArity = generic_array::typenum::U2;
+
+        let (nep_consts, ark_consts) =
+            collect_neptune_constants::<NepArity>(Strength::Standard);


This code is from plonk_prototype repo and we temporarily used neptune library as a reference to check the correctness of our code.

Anyway, we should not even import neptune library here. We need to hardcoded tests.

tsunrise · 2022-02-10T08:10:03Z

plonk-hashing/src/poseidon/round_constant.rs

+    match field {
+        1 => {
+            for _ in 0..num_constants {
+                while {


use F::from_le_bytes_mod_order instead of while loop here. See https://github.com/arkworks-rs/sponge/blob/51d6fc9aac1fa69f44a04839202b5de828584ed8/src/poseidon/grain_lfsr.rs#L135

tsunrise · 2022-02-12T08:25:07Z

@GhostOfGauss The round constant generation code in Manta's plonk prototype repo was incorrect, and I fixed that so it now passes the hardcoded tests. When you have some time can you sync those changes here?

iquerejeta

Some comments from a first pass.

iquerejeta · 2022-02-11T14:04:06Z

plonk-hashing/src/poseidon/constants.rs

+impl<F: PrimeField> PoseidonConstants<F> {
+    /// Generate all constants needed for poseidon hash of specified
+    /// width.  Note that WIDTH = ARITY + 1
+    pub fn generate<const WIDTH: usize>() -> Self {


Would it make sense to declare WIDTH as a constant to the structure rather than the function? This way we ensure that the poseidon trait and the constants used are initialised for the same width.

iquerejeta · 2022-02-14T11:56:56Z

plonk-hashing/src/poseidon/poseidon.rs

+        if !last_round {
+            let needed = *const_offset + to_take;
+            assert!(
+                needed <= constants.compressed_round_constants.len(),


This overflow could happen in any round, not only the last, right? Might be missing some check performed on the number of constants though. But maybe bounding the constants with the Width (as suggested in a different comment) might help in avoiding these type of overflows.

iquerejeta · 2022-02-14T17:15:25Z

plonk-hashing/src/poseidon/mds.rs

+/// A `SparseMatrix` is specifically one of the form of M''.
+/// This means its first row and column are each dense, and the interior matrix
+/// (minor to the element in both the row and column) is the identity.
+/// We will pluralize this compact structure `sparse_matrixes` to distinguish


Naming could be confusing. What about sparse_factorized instead of sparse_matrixes?

iquerejeta · 2022-02-14T17:47:19Z

plonk-hashing/src/poseidon/poseidon.rs

+            //
+            // // First row is dense.
+            let tmp = Self::muli(c, &state[0], &matrix.v_rest[j - 1]);
+            *val = Self::add(c, val, &tmp);


might be missing important points of how constraints are built, but would it be possible to merge both additions into one? First one is after all an addition of zero

*val = Self::add(c, state[j], &tmp);

Yes, you are right. I've updated the code in the prototype repo here. I believe those changes will soon be synced to this PR.

iquerejeta · 2022-02-14T17:49:24Z

plonk-hashing/src/poseidon/mds.rs

+    /// `w_hat` is the first column of the M'' matrix. It will be directly
+    /// multiplied (scalar product) with a row of state elements.
+    pub w_hat: Vec<F>,
+    /// `v_rest` contains all but the first (already included in `w_hat`).


Suggested change

/// `v_rest` contains all but the first (already included in `w_hat`).

/// `v_rest` contains the first row except for the first value (already included in `w_hat`).

iquerejeta · 2022-02-15T12:30:02Z

plonk-hashing/src/poseidon/mds.rs

+    pub m_hat: Matrix<F>,
+    pub m_hat_inv: Matrix<F>,
+    pub m_prime: Matrix<F>,
+    pub m_double_prime: Matrix<F>,


can't this one be defined as sparse?

drewstone · 2022-02-16T16:10:03Z

plonk-hashing/src/poseidon/poseidon.rs

+    fn output_hash(
+        c: &mut COM,
+        elements: &[Self::Field; WIDTH],
+        constants: &PoseidonConstants<Self::ParameterField>,


Could we consider adding an index for the output element here? Or maybe some understanding on why the 2nd element is selected and not the 1st (0-indexed element).

Todd Norton added 3 commits February 4, 2022 11:30

adds poseidon hash, does not compile no std

07eb0ee

adds poseidon hash, does not compile no std

7e4dcba

compiles with std, docs and tests missing

d667021

GhostOfGauss added T-feature Type: new features T-gadget Implments a new Plonk gadget labels Feb 4, 2022

GhostOfGauss requested review from bhgomes, davidnevadoc, tsunrise and LukePearson1 February 4, 2022 18:15

GhostOfGauss self-assigned this Feb 4, 2022

bhgomes reviewed Feb 4, 2022

View reviewed changes

remove ark-std dependencies

097353b

markulf reviewed Feb 6, 2022

View reviewed changes

LukePearson1 requested review from lopeetall and removed request for lopeetall February 8, 2022 01:03

tsunrise reviewed Feb 10, 2022

View reviewed changes

dutterbutter mentioned this pull request Feb 11, 2022

[TASK] Research and Compare Optimizing Poseidon Hash tangle-network/zero-knowledge-gadgets#167

Open

4 tasks

iquerejeta reviewed Feb 15, 2022

View reviewed changes

drewstone reviewed Feb 16, 2022

View reviewed changes

simonmasson mentioned this pull request May 30, 2022

Hash function choices for Taiga anoma/taiga#25

Closed

XuyangSong mentioned this pull request May 31, 2022

Poseidon hash anoma/taiga#26

Merged

7 tasks

		@@ -0,0 +1,473 @@
		//! Library independent specification for field trait.

+                      )
+                  }
+                  #[test]

	/// `v_rest` contains all but the first (already included in `w_hat`).
	/// `v_rest` contains the first row except for the first value (already included in `w_hat`).

Poseidon hash #114

Are you sure you want to change the base?

Poseidon hash #114

Conversation

GhostOfGauss commented Feb 4, 2022

LukePearson1 commented Feb 4, 2022

GhostOfGauss commented Feb 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhgomes Feb 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsunrise left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsunrise commented Feb 12, 2022 • edited Loading

iquerejeta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsunrise Feb 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bhgomes Feb 4, 2022 •

edited

Loading

tsunrise commented Feb 12, 2022 •

edited

Loading

tsunrise Feb 18, 2022 •

edited

Loading