Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use modified divsteps with initial delta=1/2 for constant-time #906

Merged
merged 4 commits into from
Apr 22, 2021

Conversation

sipa
Copy link
Contributor

@sipa sipa commented Mar 19, 2021

This updates the divsteps-based modular inverse code to use the modified version which starts with delta=1/2. For variable time, the delta=1 variant is still used as it appears to be faster.

See https://github.com/sipa/safegcd-bounds/tree/master/coq and https://medium.com/blockstream/a-formal-proof-of-safegcd-bounds-695e1735a348 for a proof of correctness of this variant.

TODO:

  • Update unit tests to include edge cases specific to this variant

I'm still running the Coq proof verification for the 590 bound in non-native mode. It's unclear how long this will take.

@real-or-random
Copy link
Contributor

Concept ACK

There's still this comment and the corresponding code:
/* Bounds on eta that follow from the bounds on iteration count (max 12*62 divsteps). */

The docs and the code might be more readable without eta having two different meanings. Maybe try another name for the new eta?

@sipa sipa force-pushed the 202012_safegcd_prime branch 2 times, most recently from e9352ef to fb8112f Compare March 19, 2021 18:53
@sipa
Copy link
Contributor Author

sipa commented Mar 19, 2021

There's still this comment and the corresponding code:
/* Bounds on eta that follow from the bounds on iteration count (max 12*62 divsteps). */

Fixed. There was an equivalent in modinv32 as well.

The docs and the code might be more readable without eta having two different meanings. Maybe try another name for the new eta?

Renamed to zeta.

@roconnor-blockstream
Copy link
Contributor

roconnor-blockstream commented Mar 24, 2021

FWIW I don't believe that it is necessary to wait for the Coq proof checked in "paranoid mode" to complete. OTOH, I estimate the check will probably be completed before this gets merged anyways, so it doesn't really matter.

Edit: (23 days later) I no longer believe the check will be completed before this gets merged.

@real-or-random
Copy link
Contributor

@sipa Is this ready for review?

@sipa
Copy link
Contributor Author

sipa commented Mar 26, 2021

@real-or-random I plan to add some more unit tests, but otherwise yes.

@sipa sipa force-pushed the 202012_safegcd_prime branch 2 times, most recently from 968985c to 4abc135 Compare March 29, 2021 04:38
@sipa
Copy link
Contributor Author

sipa commented Mar 29, 2021

This PR is ready, I think.

coqc is still running:

  PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND                                                                                 
25511 pw        20   0   10.6g  10.4g    204 R 100.0  88.6  59500:51 coqc

@sipa sipa force-pushed the 202012_safegcd_prime branch 2 times, most recently from aa77c81 to 0ca692f Compare April 1, 2021 20:28
@sipa
Copy link
Contributor Author

sipa commented Apr 1, 2021

Added a selection of randomly generated inputs (with generic modulus, and for field/scalar) as tests that trigger the largest/smallest d and e values at various points during the execution over a few billion runs.

@sipa
Copy link
Contributor Author

sipa commented Apr 12, 2021

Update: coqc has been running for 8 weeks.

@real-or-random
Copy link
Contributor

Hm, I think I'm in favor of not waiting any longer for Coq... We have the bounds analysis plus the Coq proof in native mode. This should be more than enough to be confident in the algorithm.

I'll try to review this soon.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK mod nit

doc/safegcd_implementation.md Outdated Show resolved Hide resolved
Instead of using eta=-delta, use zeta=-(delta+1/2) to represent
delta. This variant only needs at most 590 iterations for 256-bit
inputs rather than 724 (by convex hull bounds analysis).
Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK be0609f careful code review and some testing

@roconnor-blockstream
Copy link
Contributor

roconnor-blockstream commented Apr 15, 2021

I'm not really planning to review this PR myself at this time. But I do ACK the 590 constant. I have actually very carefully reviewed those three digits.

@gmaxwell
Copy link
Contributor

ACK be0609f

@sanket1729
Copy link

crACK be0609f

@real-or-random real-or-random merged commit efad350 into bitcoin-core:master Apr 22, 2021
@sipa
Copy link
Contributor Author

sipa commented Apr 23, 2021

Posthumous update: the paranoid-mode Coq proof finished succesfully 3 hours before this got merged.

@roconnor-blockstream
Copy link
Contributor

roconnor-blockstream commented Apr 23, 2021

For the record, the Coq runtime in paranoid-mode ended up being approximately 66 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants