Conversation
akoidefi
force-pushed
the
feat/poseidon-hash
branch
from
1212e6b to
c897f74
Compare
markosg04
left a comment
markosg04
left a comment
There was a problem hiding this comment.
Excellent job! Just have one question about the usage of Fr.
| fn hasher() -> Poseidon<Fr> { | ||
| Poseidon::<Fr>::new_circom(POSEIDON_WIDTH).expect("Failed to initialize Poseidon") | ||
| } |
There was a problem hiding this comment.
light-poseidon seems to rely on trait PrimeField from arkworks.
I think it is fine to couple to arkworks for now, especially if this is the best poseidon lib. Ideal world is that this would be generic over F: JoltField, which I don't think we can achieve. Maybe if it was over generic like F: PrimeField then it's easier to swap to Fq or BLS fields later.
Do you foresee any problems with fixing Fr in a snark composition context? In snark composition, sum-checks are over Fq (BN254 base field), and in this implementation we interpret all bytes as arkworks Fr. My hunch is that it's fine, but I wonder if there could be some weird attacks since Fq modulus is larger than Fr (a tiny number of collisions). Probably this fact is not really exploitable, but maybe worth a bit of thought.
if we make generic over F: PrimeField we could probably avoid this by just using Fq when the sum-check is working with Fq.
There was a problem hiding this comment.
This is a good catch. I'll make it generic on F: PrimeField.
akoidefi
force-pushed
the
feat/poseidon-hash
branch
2 times, most recently
from
b46cfbb to
34e0e30
Compare
0xParti
force-pushed
the
feat/poseidon-hash
branch
from
854b65f to
6338e06
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| self.n_rounds as usize <= expected_state_history.len(), | ||
| "Fiat-Shamir transcript mismatch: n_rounds {} exceeds expected history length {}", | ||
| self.n_rounds, | ||
| expected_state_history.len() | ||
| ); | ||
| assert!( | ||
| new_state == expected_state_history[self.n_rounds as usize], |
There was a problem hiding this comment.
Off-by-one error causing array index out of bounds panic in test mode. After incrementing n_rounds at line 190, the code checks self.n_rounds as usize <= expected_state_history.len() at line 195, then accesses expected_state_history[self.n_rounds as usize] at line 201. When n_rounds equals len(), this accesses an invalid index (arrays are 0-indexed, so valid indices are 0 to len-1).
// Fix: Change <= to <
assert!(
self.n_rounds as usize < expected_state_history.len(),
"Fiat-Shamir transcript mismatch: n_rounds {} exceeds expected history length {}",
self.n_rounds,
expected_state_history.len()
);
assert!(
new_state == expected_state_history[self.n_rounds as usize - 1],
"Fiat-Shamir transcript mismatch at round {}",
self.n_rounds
);Or alternatively access at [self.n_rounds as usize - 1] since n_rounds was just incremented.
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
Adds PoseidonTranscript using light_poseidon over BN254. Uses width-3 Poseidon to include n_rounds in every hash call for domain separation, same as Blake2b/Keccak. Chunks large inputs into 32-byte pieces since Poseidon has fixed-width inputs. Gated behind transcript-poseidon feature flag.
Co-authored-by: Markos <[email protected]>
- Add PoseidonParams<F> trait for type-level parameter configuration - Implement FrParams (uses new_circom) and FqParams (generated params) - Add poseidon_fq_params.rs with BN254 Fq parameters (128-bit security) - Create type aliases PoseidonTranscriptFr and PoseidonTranscriptFq - Fq params generated with poseidon-paramgen v0.4.0 (audited by NCC Group) This enables SNARK composition where sumchecks operate over Fq.
Integrates the poseidon-paramgen library (arkworks 0.5 compatible fork) to enable verification that hardcoded BN254 Fq Poseidon parameters match the audited output from poseidon-paramgen v0.4.0 (NCC Group, Summer 2022). This adds transparency and verifiability to the Fq parameters needed for SNARK recursion, where the verifier operates in the base field rather than the scalar field. Changes: - Add workspace deps for defi-wonderland/poseidon377 fork (arkworks-0.5 branch) - Add poseidon-paramgen and poseidon-parameters as optional deps in jolt-core - Create poseidon_param_gen.rs with generate_fq_params() and verification tests - Update poseidon_fq_params.rs header with verification instructions Tests verify: - Round counts (8 full, 56 partial) - Alpha/S-box exponent (5) - MDS matrix (4x4, 16 elements) - Round constants (256 values) - Integration with light-poseidon hasher - Transcript determinism
akoidefi
force-pushed
the
feat/poseidon-hash
branch
from
6338e06 to
ce4f920
Compare
3 participants
Adds PoseidonTranscript using light_poseidon over BN254.
Uses width-3 Poseidon to include n_rounds in every hash call for domain separation, same as Blake2b/Keccak. Chunks large inputs into 32-byte pieces since Poseidon has fixed-width inputs.
Gated behind transcript-poseidon feature flag.