Bifolding IVC: Add Poseidon gadget for hashing commitments #2437
Conversation
There was a problem hiding this comment.
Blockers:
- Capability design/next row access is hacky and not future-proof, I'm happy to discuss it if you think this design is better
- Cosmetics (easy to fix tho)
Essential but won't block if it's delivered next
- No test (how can I check if this code works? why do you think it does?)
ivc/src/poseidon/interpreter.rs
Outdated
| state | ||
| } | ||
|
|
||
| ///constrains a round |
There was a problem hiding this comment.
The overwhelming standard is space after // or ///, and then capitalise the first word when it's a doc, as you'd do in a regular sentence.
| ///constrains a round | |
| /// Constrains a round |
There was a problem hiding this comment.
(this comment applies to a lot of places in this file, and to other PRs generally too)
| @@ -112,6 +112,8 @@ | |||
| //! executed, `0` otherwise. | |||
|
|
|||
| pub mod ivc; | |||
| /// poseidon implementation that hashes commitments to the columns containing values | |||
| pub mod poseidon; | |||
There was a problem hiding this comment.
This needs a more distinctive name. poseidon_hashed_com?..
| @@ -36,6 +36,10 @@ pub trait ColAccessCap<F: PrimeField, CIx: ColumnIndexer> { | |||
| /// Turns a constant value into a variable. | |||
| fn constant(value: F) -> Self::Variable; | |||
| } | |||
| /// Environment capability for accessing the next row, constraint only. | |||
| pub trait NextCap<F: PrimeField, CIx: ColumnIndexer>: ColAccessCap<F, CIx> { | |||
| fn read_next(&self, col: CIx) -> Self::Variable; | |||
There was a problem hiding this comment.
I'd strongly suggest to put this into ColAccessCap as read_column_next: it's literally the same semantics (reading a column). Any reasons why you didn't go for this design?
| @@ -36,6 +36,10 @@ pub trait ColAccessCap<F: PrimeField, CIx: ColumnIndexer> { | |||
| /// Turns a constant value into a variable. | |||
| fn constant(value: F) -> Self::Variable; | |||
| } | |||
| /// Environment capability for accessing the next row, constraint only. | |||
| pub trait NextCap<F: PrimeField, CIx: ColumnIndexer>: ColAccessCap<F, CIx> { | |||
There was a problem hiding this comment.
Another question on this framework: don't you also need write_column_next?
| @@ -56,6 +55,16 @@ impl<F: PrimeField, CIx: ColumnIndexer, LT: LookupTableID> ColAccessCap<F, CIx> | |||
| Expr::Atom(ExprInner::Constant(cst_expr_inner)) | |||
| } | |||
| } | |||
| impl<F: PrimeField, CIx: ColumnIndexer, LT: LookupTableID> NextCap<F, CIx> | |||
There was a problem hiding this comment.
You have to implement lenses for your new capability too (missing in this PR), otherwise it won't compose well in the future.
ivc/src/poseidon/interpreter.rs
Outdated
| let check_out = env.read_column(PoseidonColumn::Check); | ||
| let res = round(env, state, absorb, check_out); | ||
| for i in 0..=2 { | ||
| let next = env.read_next(PoseidonColumn::State(i)); |
There was a problem hiding this comment.
I don't understand the semantics of this. The idea behind constrain_X is that it can be used as a real assertion in the witness-building code.
- When you execute
constraint_roundin theConstraintBuildermode, you'll get the constraint, alright. - But when you execute it in the
WitnessBuildermode, this will fail, because ... currentWitnessBuilderdoes not allow you to write into the future cells.
I suggest this mechanic is added within this PR. You'll just need to:
- Store
cur_rowexplicitly in theWitnessBuilder, so thatcur_row = witness.len() - 2(e.g. witness is two rows, but current row is 0, and row 1 is "next"). - Extend capabilities of read/write column to read/write next column.
- Add
write_column_nexttocompute_round.
volhovm
changed the title
No description provided.