Problem with mont form inversion #606

dvdplm · 2024-06-11T07:25:33Z

Test to illustrate a potential problem with the Bernstein&Yang code to
invert numbers in Montgomery form. The specific parameters shown here
come from intermittent test failures of homomorphic_mul in the
Synedrion signining library (i.e. parameters are generated with an
unseeded CSPRNG).

Run the test with cargo t inversion_v06_vs_v055.

For convenience, after the test follows a commented out version of the
same that passes with crypto-bigint v0.5.5.

To compare with v0.5.5 do the following:

Checkout the old code: git checkout v0.5.5
Run cargo update -p proc-macro2 to work around a recent compiler
incompatibility
Paste the commented out v0.5 test in the test module of
runtime_mod.rs
Run the test with cargo t inversion_v06v_v055
Notice it passes

tarcieri · 2024-06-12T14:15:10Z

@dvdplm it looks like this test is making use of internals / private fields. Can you write it entirely in terms of the public API? Otherwise I can't tell if you're miscomputing constants.

It would also be good to generate reference values with e.g. sage, and show your work for that.

dvdplm · 2024-06-12T19:31:57Z

Can you write it entirely in terms of the public API?

You mean to instantiate the MontyForm values? Sure, I can try to do that.

I think having known good values would be very good, and I had a look at the Sage docs but couldn't find much info on how to do modular maths with it. I'll try again though.

dvdplm · 2024-06-13T06:25:34Z

Can you write it entirely in terms of the public API?

Done.

tarcieri · 2024-06-13T14:35:25Z

@dvdplm as an alternative to sage, you could also compare results with num-modular, which is already one of the dev-dependencies

dvdplm · 2024-06-14T11:20:09Z

also compare results with num-modular

@tarcieri I added a test using num-modular. It agrees with crypto-bigint v0.5.5.

Add tests for random uints and their inverses for both v06 and v05

dvdplm · 2024-06-20T09:29:42Z

@tarcieri @fjarri I think I have found another clue that might help us narrow down what is going wrong here. The problem seems to lie in the modulus, in the size of the modulus. In the latest commit (e0da58b) I have added more tests to illustrate how moduli of sizes U2048 and U4096 seem to break on v0.6. Weirdly enough U1024 moduli seem fine. I did not try U8192 yet.

Not sure what the next steps should be but I wanted to leave as much information as possible here.

dvdplm · 2024-06-20T09:31:47Z

src/modular/monty_form/inv.rs

+    fn v06_inversion_random_uints_u2048_modulus() {
+        let mut rng = ChaChaRng::from_seed(RANDOM_SEED);
+        let modulus = U2048::from_be_hex(concat!(
+            "1fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",


Changing this to 1effffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff makes the test pass. For this issue it doesn't seem to matter what any other bytes are.

dvdplm · 2024-06-20T09:32:27Z

src/modular/monty_form/inv.rs

+        let mut rng = ChaChaRng::from_seed(RANDOM_SEED);
+        let modulus = U4096::from_be_hex(concat!(
+            // 0x07… fails (bin 00000111)
+            "06ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",


For U4096 the "failure value" is different.

dvdplm · 2024-06-20T09:33:00Z

src/modular/monty_form/inv.rs

+    fn v06_inversion_random_uints_u1024_modulus() {
+        let mut rng = ChaChaRng::from_seed(RANDOM_SEED);
+        let modulus = U1024::from_be_hex(concat!(
+            "1fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",


U1024 seems to be able to cope with any modulus.

tarcieri · 2024-06-20T16:01:34Z

@dvdplm that's a good data point to know re: specific sized integers being impacted

My best guess would be it's a bug in the bernstein_yang_nlimbs! macro which computes the number of 62-bit limbs needed to represent a given-sized value:

crypto-bigint/src/macros.rs

Lines 12 to 19 in 2b81eae

    
           /// Calculate the number of 62-bit unsaturated limbs required to represent the given number of bits when performing 
        
           /// Bernstein-Yang inversions. 
        
           // TODO(tarcieri): replace with `generic_const_exprs` (rust-lang/rust#76560) when stable 
        
           macro_rules! bernstein_yang_nlimbs { 
        
               ($bits:expr) => { 
        
                   (($bits / 64) + (($bits / 64) * 2).div_ceil(64) + 1) 
        
               }; 
        
           }

That's used here when writing impls of the PrecomputeInverter trait:

crypto-bigint/src/uint/macros.rs

Line 11 in 2b81eae

{ bernstein_yang_nlimbs!($bits as usize) },

tarcieri · 2024-06-20T16:14:20Z

I was able to produce a test failure using the existing test suite by modifying the proptests for modular inversion in tests/monty_form.rs to use U2048 instead of U256.

I then made those same tests pass by modifying bernstein_yang_nlimbs to be the following:

 macro_rules! bernstein_yang_nlimbs { 
     ($bits:expr) => { 
         (($bits / 64) + (($bits / 64) * 2).div_ceil(64) + 2) 
     }; 
 }

That probably isn't the best way to fix this specific problem, but I believe it does demonstrate that there are currently too few 62-bit limbs for a given sized integer, leading to truncation and thus the miscomputed results.

@dvdplm I haven't tested your specific vectors, but perhaps you could try that locally and see if it corrects the issue?

tarcieri · 2024-06-20T16:15:39Z

Cargo.toml

@@ -38,6 +38,7 @@ num-modular = { version = "0.6", features = ["num-bigint", "num-integer", "num-t
 proptest = "1"
 rand_core = { version = "0.6", features = ["std"] }
 rand_chacha = "0.3"
+crypto_bigint_05 = {package = "crypto-bigint", version = "0.5.5"}


Would you mind removing this dependency and the tests that depend on it, leaving only tests against the current codebase with fixed vectors?

I mean, this PR is just a throw-away thing to tinker with a tricky bug and this was convenient for that purpose. If there's anything we salvage here long term it's perhaps the test vectors, but perhaps we can put those in your other PR?

tarcieri · 2024-06-20T16:18:15Z

I'm also a little confused why the current calculation for the number of limbs is insufficient, but I'll have to go back through the paper and find where it's defined.

FWIW, it currently computes 34 limbs for a 2048-bit integer, where 34 * 62 = 2108 bits

tarcieri · 2024-06-20T16:29:14Z

Aha, so the issue is for a given UNSAT_LIMBS, we can calculate the maximum allowed size of any modulus or input value in bits as:

(UNSAT_LIMBS * 62) - 64

We currently compute 34 limbs for a 2048-bit integer:

(34 * 62) - 64 = 2044

That's 4-bits too small.

The previous calculation of the number of unsaturated 62-bit limbs needed to represent an integer of a given size was incorrect, leading to miscomputed results as seen in #606. This commit switches to a much simpler calculation based on `div_ceiling(62)`, and also adds a const assertion that the computed number of limbs is sufficient to hold a `$bits`-sized integer.

tarcieri · 2024-06-20T18:18:28Z

Draft PR with a fix here: #610

@dvdplm I'd appreciate it if you could rebase on that and see if it fixed your issues

dvdplm · 2024-06-20T18:19:55Z

That's 4-bits too small.

Awesome sleuthing! I was thiiiiiis close, but you got there first! :)

dvdplm · 2024-06-20T18:20:19Z

I'd appreciate it if you could rebase on that and see if it fixed your issues

Yep, will do first thing in the morning.

dvdplm · 2024-06-20T18:31:40Z

@dvdplm I haven't tested your specific vectors, but perhaps you could try that locally and see if it corrects the issue?

Changing the + 1 to + 2 fixes the failing tests, so that's a good first indication that your fix is good. I'll test the PR properly as well.

The previous calculation of the number of unsaturated 62-bit limbs needed to represent an integer of a given size was incorrect, leading to miscomputed results as seen in #606. This commit switches to a much simpler calculation based on `div_ceiling(62)`, and also adds a const assertion that the computed number of limbs is sufficient to hold a `$bits`-sized integer.

dvdplm · 2024-06-21T16:32:47Z

I'd appreciate it if you could rebase on that and see if it fixed your issues

I have checked that #610 indeed fixes all test failures on this branch. 🎉

To check upstream it would be really good to have a new pre-release of crypto-bigint, is that possible?

tarcieri · 2024-06-22T01:24:06Z

Yes, I can cut another release soon

tarcieri · 2024-06-23T23:40:32Z

Released in v0.6.0-rc.0

The previous calculation of the number of unsaturated 62-bit limbs needed to represent an integer of a given size was incorrect, leading to miscomputed results as seen in RustCrypto#606. This commit switches to a much simpler calculation based on `div_ceiling(62)`, and also adds a const assertion that the computed number of limbs is sufficient to hold a `$bits`-sized integer.

tarcieri · 2024-07-31T20:17:23Z

Going to close this out, but I'd still be curious in getting some of these tests in if you'd like to resubmit just those

dvdplm added 2 commits June 11, 2024 09:14

Test to illustrate a mont form inversion problem

cce105a

Update descr

723aa4c

Rewrite tests using public API

2f050b7

Add a test using num-modular

5063f50

dvdplm added 3 commits June 18, 2024 12:28

Add v0.5.5 as dev-dep

6c803e6

Add tests for random uints and their inverses for both v06 and v05

Lockfile

3f84e2d

More tests, better description, cleanup

e0da58b

dvdplm commented Jun 20, 2024

View reviewed changes

tarcieri reviewed Jun 20, 2024

View reviewed changes

tarcieri mentioned this pull request Jun 20, 2024

Fix bernstein_yang_nlimbs! calculation #610

Merged

Merge branch 'master' into dp-BY-inversion

d3b2d7c

tarcieri closed this Jul 31, 2024

dvdplm mentioned this pull request Aug 5, 2024

Inversion tests #639

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem with mont form inversion #606

Problem with mont form inversion #606

dvdplm commented Jun 11, 2024 •

edited

Loading

tarcieri commented Jun 12, 2024

dvdplm commented Jun 12, 2024

dvdplm commented Jun 13, 2024

tarcieri commented Jun 13, 2024

dvdplm commented Jun 14, 2024

dvdplm commented Jun 20, 2024

dvdplm Jun 20, 2024

dvdplm Jun 20, 2024

dvdplm Jun 20, 2024

tarcieri commented Jun 20, 2024

tarcieri commented Jun 20, 2024

tarcieri Jun 20, 2024

dvdplm Jun 20, 2024

tarcieri commented Jun 20, 2024

tarcieri commented Jun 20, 2024

tarcieri commented Jun 20, 2024

dvdplm commented Jun 20, 2024

dvdplm commented Jun 20, 2024

dvdplm commented Jun 20, 2024

dvdplm commented Jun 21, 2024

tarcieri commented Jun 22, 2024

tarcieri commented Jun 23, 2024

tarcieri commented Jul 31, 2024

Problem with mont form inversion #606

Problem with mont form inversion #606

Conversation

dvdplm commented Jun 11, 2024 • edited Loading

tarcieri commented Jun 12, 2024

dvdplm commented Jun 12, 2024

dvdplm commented Jun 13, 2024

tarcieri commented Jun 13, 2024

dvdplm commented Jun 14, 2024

dvdplm commented Jun 20, 2024

dvdplm Jun 20, 2024

Choose a reason for hiding this comment

dvdplm Jun 20, 2024

Choose a reason for hiding this comment

dvdplm Jun 20, 2024

Choose a reason for hiding this comment

tarcieri commented Jun 20, 2024

tarcieri commented Jun 20, 2024

tarcieri Jun 20, 2024

Choose a reason for hiding this comment

dvdplm Jun 20, 2024

Choose a reason for hiding this comment

tarcieri commented Jun 20, 2024

tarcieri commented Jun 20, 2024

tarcieri commented Jun 20, 2024

dvdplm commented Jun 20, 2024

dvdplm commented Jun 20, 2024

dvdplm commented Jun 20, 2024

dvdplm commented Jun 21, 2024

tarcieri commented Jun 22, 2024

tarcieri commented Jun 23, 2024

tarcieri commented Jul 31, 2024

dvdplm commented Jun 11, 2024 •

edited

Loading