feat: update Poseidon2 to match upstream P3#905
feat: update Poseidon2 to match upstream P3#905Nashtare wants to merge 3 commits into0xMiden:nextfrom
Conversation
adr1anh
left a comment
adr1anh
left a comment
There was a problem hiding this comment.
Thanks for this!
I didn't realize our constants were different, which might affect a lot of our downstream repos. We've already done it with the Rpo transition, but we should check how complicated it would be to do again. Moreover, given the current Poseidon2 discussion, we might want to hold off for a few weeks until to make sure we don't need to switch again.
In the case of benchmarks of the VM, we could try just importing Poseidon2 from p3 directly. This would give us an idea of the performance improvement when SIMD is available.
| pub const DIGEST_RANGE: Range<usize> = DIGEST_RANGE; | ||
|
|
||
| /// Matrix used for computing the linear layers of internal rounds. | ||
| pub const MAT_DIAG: [Felt; STATE_WIDTH] = MAT_DIAG; |
There was a problem hiding this comment.
I wonder if we should keep these exports for the VM constrains.
There was a problem hiding this comment.
I think we probably do need to re-export the constants and other relevant data as it would be used in the constraint definitions in Miden VM.
| mod test; | ||
|
|
||
| #[cfg(feature = "std")] | ||
| static P3_POSEIDON2: std::sync::LazyLock<p3_goldilocks::Poseidon2Goldilocks<12>> = |
There was a problem hiding this comment.
I wonder if there's a way we could change the way Plonky's treats its constants to avoid the Vec storage entirely. I wonder what situations need custom constants.
I had to do a couple tweaks on the
Yeah this definitely makes sense |
huitseeker
left a comment
huitseeker
left a comment
There was a problem hiding this comment.
OFC, this moves quite a bit off the public API of miden-crypto, but I assume that's the point. A bit worried by the no-std adjustments, see inline.
| /// Applies Poseidon2 permutation to the provided state. | ||
| /// | ||
| /// This delegates to the Poseidon2 implementation. | ||
| #[inline(always)] | ||
| pub fn apply_permutation(state: &mut [Felt; STATE_WIDTH]) { |
There was a problem hiding this comment.
nit: is there a reason to delete this comment?
4 participants
Describe your changes
Update Poseidon2 implementation to match upstream Plonky3's for Goldilocks and support SIMD acceleration.
Most of the changes are related to updated hardcoded data (commit a6211e5)
closes #904
Checklist before requesting a review
nextaccording to naming convention.