Conversation
Switch the Goldilocks field (p = 2^64 - 2^32 + 1) from Fp64<MontBackend<FConfig64, 1>> to SmallFp<FConfig64> using #[derive(SmallFpConfig)]. This leverages native u64 Montgomery arithmetic for 15-22% faster sumcheck and 4-12% faster NTT operations. Key changes: - fields.rs: SmallFp field definition with const fn goldilocks_mont() for compile-time Montgomery constant computation in Fp2/Fp3 extension configs - Cargo.toml: patch ark-ff/ark-std/ark-serialize to git master for SmallFp; update spongefish to patched version fixing BigInt<2> encoding/deserialization - cooley_tukey.rs: fix test using BigInt<2> (16 bytes) instead of BigInt<1> (8 bytes) Spongefish patches (in /tmp/spongefish-patched): - Fix impl_encoding! macro: drain leading zeros from BigInt<2>.to_bytes_be() - Fix impl_deserialize! macro: use BigInt::from_bits_be + from_bigint instead of from_be_bytes_mod_order (broken for SmallFp due to from_raw Montgomery bug)
… fix - spongefish: z-tech/spongefish rev 2613967 (encoding + NargDeserialize fixes for SmallFp extension fields) - ark-ff: algebra a2d4d660 (includes #1082: fix SmallFp from_random_bytes Montgomery confusion)
Merging this PR will not alter performance
Comparing 
Footnotes
|
Cargo.toml
Outdated
| [patch.crates-io] | ||
| ark-ff = { git = "https://github.com/arkworks-rs/algebra" } | ||
| ark-std = { git = "https://github.com/arkworks-rs/std" } | ||
| ark-serialize = { git = "https://github.com/arkworks-rs/algebra" } |
There was a problem hiding this comment.
Let's wait until this is in a published ark-ff crate before merging?
I prefer not depending on main branches directly (even though we already do it for spongefish, but at least we pin a rev).
Depending on an unpegged main branch is pretty fragile. For example, everything would break if ark-std decides to finally upgrade their rand version.
There was a problem hiding this comment.
We'd be happy to get a list of items you think could be cleaned up if you want to chat btw.
src/algebra/fields.rs
Outdated
| /// Since p ≈ 2^64, we have k = 64 and R = 2^64. | ||
| /// | ||
| /// Montgomery form of `v` is `v · R mod p`, computed in u128 to avoid overflow. | ||
| const fn goldilocks_mont(v: u64) -> u64 { |
There was a problem hiding this comment.
Why is this not part of SmallFp? I would expect an
impl SmallFp {
pub const fn from_u64(n: u64) -> Self;
}
or similar being generated by the macro that does exactly this.
It's also leaks the abstraction in that it assumes SmallFp uses Montogomery representation, which is not obvious for small field (i.e. M31 works better without IIRC).
There was a problem hiding this comment.
Fair point wrote the little helper here: arkworks-rs/algebra#1084
Also, we're happy to further optimize any Mersenne primes when/ if you have a specific need: https://andrewzitek.xyz/smallfp-site/
I'm confident this would be a reasonable change in the framework we've created.
2 participants
What does this PR do?
Results of running cargo bench
SmallFpvsFp64<MontBackend>for the Goldilocks field (--release.Interleaved RS Encode (median)
Fp64SmallFpSumcheck First Round (median)
Fp64SmallFp