Binary XGCD

[erik-3milabs]

Continuation of the work started in #755

[kayabaNerve]

The new commit, "Fix bug", which I have not personally reviewed, passes my personal code! Thank you!

[kayabaNerve]

crypto-bigint/src/modular/bingcd/xgcd.rs:309:13:
b is never negative

This is from the "Fix bug" commit I prior commented worked for my prior noted issues. I'm actually using it in the same context, solely running a different test suite, so I'm unsure why this current test suite is managing to trip this low-level bug when my prior one didn't. I'll try to find a reproducible test case, but at least having noted this ideally lets some review be started on.

EDIT: binxgcd on the following Uints, which are of size of the size of their serializations. 64-bit host.

Uint(0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD03641424EB38E6AC0E34DE2F34BFAF22DE683E1F4B92847B6871C780488D797042229E1)

Uint(0x0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFD755DB9CD5E9140777FA4BD19A06C82839D671CD581C69BC5E697F5E45BCD07C52EC373A8BDC598B4493F50A1380E1281)

[erik-3milabs]

@kayabaNerve, thank you for being a persistent tester! I hope to find some time later this week to debug the issue you presented. I appreciate your patience!

(I'm delighted I added in those .excepts: I now know exactly where to look for the issue 🙈 )

[kayabaNerve]

Also just hit a is never negative. Unfortunately, that seems to be much more infrequent for the numbers I'm generating, so I may not be able to provide a vector.

[erik-3milabs]

crypto-bigint/src/modular/bingcd/xgcd.rs:309:13:
b is never negative

This is from the "Fix bug" commit I prior commented worked for my prior noted issues. I'm actually using it in the same context, solely running a different test suite, so I'm unsure why this current test suite is managing to trip this low-level bug when my prior one didn't. I'll try to find a reproducible test case, but at least having noted this ideally lets some review be started on.

EDIT: binxgcd on the following Uints, which are of size of the size of their serializations. 64-bit host.

Uint(0x00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000007FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD03641424EB38E6AC0E34DE2F34BFAF22DE683E1F4B92847B6871C780488D797042229E1)

Uint(0x0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFD755DB9CD5E9140777FA4BD19A06C82839D671CD581C69BC5E697F5E45BCD07C52EC373A8BDC598B4493F50A1380E1281)

@kayabaNerve I had a quick look today. Your counterexample demonstrates there are more annoying ways in which the compact_a/compact_b variables misrepresent the actual a and b. I will have to think a bit about how to resolve this 🤔

[kayabaNerve]

I did try poking at this myself to see if I could fix it, but it really wasn't immediately familiar/obvious to me. I ended up just making a fork which never uses the optimized algorithm as to not block my work.

Pros: Works.
Cons: My runtime on GCDs went from 2s to ~20s when I have a deadline for an academic paper in just a few days 😬

I may try to work on this again if I myself have free time before the deadline, but any further help from you would truly be appreciated! This truly is a massive improvement for crypto-bigint on this topic and it's fundamentally what makes my current research topic go from 'initial impl for reference which isn't discussable' to 'initial impl which yields feasible results for practical deployment'.

EDIT: The Bezout coefficients are correct or their additive inverse modulo the other value divided by their GCD, for this specific test case. From these incorrect values, it's trivial to calculate the proper values with a post-processing run.

I tried to do so on every result, to restore functioning despite not having a proper fix, but that yields other errors so this isn't a universal fact.

[erik-3milabs]

I've worked on this here-and-there for the past couple days, without much luck.

The problem

The failing input presented by @kayabaNerve illustrates that the trick does not always apply.

Solutions

I've thought of a couple solutions:

1) Using Int's in BinXgcdMatrix.

Not an option, as integer overflows can happen when the input values have their most significant bit set.

2) Using ExtendedInt's in BinXgcdMatrix.

While possible on paper, this makes the multiplication between matrix and update_matrix and absolute nightmare.

3) Adding an second pattern attribute to BinXgcdMatrix.

In particular, the idea would be to have a top_pattern and bottom_pattern attribute. The first would indicate whether the signs in the first row of the matrix are either [ - + ] or [+ - ] and the bottom_pattern would do the same for the bottom row.

This also does not seem to work, as there are exceptional cases where the signs in a row are BOTH positive. Specifically, this seems to happen for inputs a, b such that for some small x and y it holds that x * a + y * b is divisible by 2^k >> x, y.

4) Solution 1, but now with an extra limb.

That more-or-less requires us to keep UNSAT_LIMBS around which is exactly what we're trying to avoid.

Now what?

I don't know yet. If you're working with Uints that do not have their top bit set, you could opt for the first solution. But that is a temporary solution at best.

I'll be away for the next couple weeks; I hope to return to this issue afterwards.

[tarcieri]

Solution 1, but now with an extra limb.

That more-or-less requires us to keep UNSAT_LIMBS around which is exactly what we're trying to avoid.

@erik-3milabs if it's just a single extra limb, with some effort you could always make a struct that adds that extra limb which avoids type-level size calculations

[tarcieri]

@erik-3milabs looks like this one needs a rebase

[erik-3milabs]

Alright, it took some time, but I think I managed to fix the bug.

@erik-3milabs if it's just a single extra limb, with some effort you could always make a struct that adds that extra limb which avoids type-level size calculations

@tarcieri this was indeed the solution. I thought this couldn't work, because you'd have to multiply two of these ExtraLimbInts, but it turns out that is not necessary!

@erik-3milabs looks like this one needs a rebase
Because we merged bingcd, this now needs a lot more than just a rebase 😅

To ease the review process, I propose to split this PR into (at least) two parts:

1. classic_binxgcd + all the boilerplate code to provide it to Uint, NonZeroUint, OddUint, Int, NonZeroInt and OddInt; this one won't be all that complicated, since all the parts are already in place for this.
2. optimized_binxgcd, which is a rather complicated algorithm that needs many eyes to go over it. Let's not bog the reviewer down with boiler plate code for that PR.

As such, this PR will remain a draft for a bit longer.

[erik-3milabs]

The content of this PR is moved to #854 and #856

[erik-3milabs]

Closing this PR; it is superseded by #854 and #856