Switched EcdsaPod and MDLPod to work with 29-bit BigInt Chunks#10
Switched EcdsaPod and MDLPod to work with 29-bit BigInt Chunks#10dgulotta merged 6 commits into0xPARC:mainfrom
Conversation
|
Looks good overall! I'll add some comments on individual items. |
|
Our CI automatically checks the code with |
dgulotta
left a comment
dgulotta
left a comment
There was a problem hiding this comment.
A few things from plonky2-ecdsa:
https://github.com/Weobe/plonky2-ecdsa/blob/c2f20afcd5722235199dd8fbb88985c8b22c2d9e/src/gadgets/curve_windowed_mul.rs#L156
Extra println
https://github.com/Weobe/plonky2-ecdsa/blob/c2f20afcd5722235199dd8fbb88985c8b22c2d9e/src/gates/mul_nonnative.rs#L276-L329
It looks like this is a test and not something that should be in run_once.
https://github.com/Weobe/plonky2-ecdsa/blob/c2f20afcd5722235199dd8fbb88985c8b22c2d9e/src/gadgets/biguint.rs#L35-L37
The line let i = i.clone(); can be removed if you replace i with *i on the next line. Or you can eliminate the need to write *i by replacing for i in x with for &i in x or for i in x.iter().copied()
It would be good to add a comment about how mul_nonnative works and the assumptions being made, as well as some comments in the curve functions explaining why it's okay to do additions that are not range checked.
It would also be good to run cargo clippy on your plonky2-ecdsa and plonky2_ux repos.
.gitignore
Outdated
| @@ -1,2 +1,3 @@ | |||
| /target | |||
| Cargo.lock | |||
| src/main.rs | |||
There was a problem hiding this comment.
If you have a file that's only your system and you don't want to commit it, you can add the path to .git/info/exclude. The .gitignore is for files that many users will have (usually because they are auto generated).
Cargo.toml
Outdated
| x509-parser = "0.16" | ||
| p256 = "0.13.2" | ||
| log = "0.4.14" | ||
| env_logger = "0.10.0" |
There was a problem hiding this comment.
Does the library use these dependencies now, or were they just for your testing?
| use super::*; | ||
|
|
||
| #[ignore] | ||
| fn get_test_rsa_pod() -> Result<(Box<dyn RecursivePod>, VDSet, Vec<u8>)> { |
There was a problem hiding this comment.
This function isn't a test, so I don't think the #[ignore] does anything.
src/mdlpod.rs
Outdated
| // let recovered_pod = MdlPod::deserialize_data(params, data, vd_set, pod.id())?; | ||
| // assert!(pod.equals(recovered_pod.as_ref())); | ||
| // Ok(()) | ||
| // } |
There was a problem hiding this comment.
This test can be ignored by default since it takes a long time to run, but it shouldn't be commented out.
|
|
||
| #[test] | ||
| #[ignore] | ||
| fn test_rsa_pod_with_mainpod_verify() -> Result<()> { |
There was a problem hiding this comment.
There should be at least one RSA test that isn't ignored.
src/mdlpod.rs
Outdated
| impl MdlVerifyTarget { | ||
| fn add_targets(builder: &mut CircuitBuilder<F, D>) -> Self { | ||
| let sig = P256VerifyTarget::add_targets(builder, MSO_MAX_BITS_PADDED); | ||
| builder.print_gate_counts(0); |
There was a problem hiding this comment.
It looks like this is left over from testing.
|
0xPARC recently forked plonky2, and it looks like your Cargo.toml files need to be updated to use the fork. Currently pod2 is using: |
Hmm, I guess introduction-pods is using an older version of pod2 that isn't using the plonky2 fork yet. So I'm not sure exactly why there is a conflict. |
|
It looks the problem is that ax0/plonky2-u32 was updated to use the plonky2 fork. So I guess introduction-pods and your forks should be updated to use the plonky2 fork, and introduction-pods should also be updated to use the latest pod2 commit. Sorry you ran into this! |
|
If updating causes too many problems, you could instead revert to using revision a18e790 of ax0/plonky2-u32. |
2 participants
Making this change allows us to write a more optimized Custom Gate for nonnative multiplication by getting rid of unnecessary carry operations