chore: Switch to ruint, cleanup conversion functions and add tests
#33
Conversation
src/witness/memory.rs
Outdated
| @@ -186,7 +186,7 @@ impl SafeMemory { | |||
| /// * `ptr` - The memory address where the field element will be written. | |||
| /// * `fr` - The [`U256`] field element to write. | |||
| fn write_short(&mut self, store: &impl AsStoreRef, ptr: usize, fr: U256) -> Result<()> { | |||
| let num = fr.to_words()[0] as u32; | |||
| let num = fr.as_limbs()[0] as u32; // wtf is happening | |||
There was a problem hiding this comment.
I thought there might be a bug here, but after the tests passed and the constraints were satisfied, the behavior is correct
src/util.rs
Outdated
| pub fn ff_as_limbs<F: PrimeField>(f: &F) -> &[u32; 8] { | ||
| let binding = f.to_repr(); | ||
| let repr: &[u8; 32] = binding.as_ref().try_into().unwrap(); | ||
| // this doesn't work if the platform we're on is not little endian :scream: |
There was a problem hiding this comment.
It's even a bit worse than that: it assumes that the field implementation is using LE for to_repr(), see the tests introduced in lurk-lang/arecibo@fd3ae7a
There was a problem hiding this comment.
Yes, should I be more explicit in mentioning this issue? There's also no way to directly test within circom-scotia, since we can't guarantee downstream will use any particular field impl... soooo
There was a problem hiding this comment.
I guess we have two ways of addressing this:
- Keep this as a limitation for now, document it more clearly and open an issue
- Change the trait we deal with to
PrimeFieldBitsinstead ofPrimeFieldto allow forto_le_bitsmethod call, ensuring correctness.
@winston-h-zhang, feel free to chose any.
winston-h-zhang
force-pushed
the
use-ruint
branch
4 times, most recently
from
1db7188 to
12f3bf8
Compare
winston-h-zhang
force-pushed
the
use-ruint
branch
from
12f3bf8 to
d1e7b1b
Compare
src/util.rs
Outdated
| /// Assumes little endian | ||
| pub fn ff_as_limbs<F: PrimeField>(f: F) -> [u32; 8] { | ||
| let binding = f.to_repr(); | ||
| let repr: [u8; 32] = binding.as_ref().try_into().unwrap(); |
There was a problem hiding this comment.
Better error definition and propagation could be defined here. Or at least an expect giving us some context :)
src/util.rs
Outdated
| *digit = limbs[i]; | ||
| } | ||
|
|
||
| F::from_repr(repr).unwrap() |
There was a problem hiding this comment.
As above, would be great to have Rust error handling or at least an expect
|
@tchataigner Thanks for the review! I rewrote the functions to use |
This PR swtiches the bignum crate from
crypto-biginttoruint. The motivation for this particular switch is because https://github.com/philsippl/circom-witness-rs/tree/master usesruint. After considering whether to switch toruintor stay withcrypto-bigintand port the code fromcircom-witness-rs, I decidedruintis a bit easier to work with.After making the switch, I cleaned up the
from_vec_u32, etc, conversion functions into a utils module with tests for safety.