Re-apply "Faster wrapping multiplication"#954
Conversation
…thods into a separate module Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
Signed-off-by: Andrew Whitehead <cywolf@gmail.com>
| while k < lhs.len() { | ||
| let j = k - i; | ||
| if k == rhs.len() { | ||
| if j == rhs.len() { |
There was a problem hiding this comment.
Was this the only issue? I saw some other test failures besides the out-of-bounds panic
There was a problem hiding this comment.
Yep, seems to fix all the tests. Besides the panic it could cause incorrect results, some of the new tests capture that:
thread 'uint::mul::tests::wrapping_mul_mixed' panicked at src/uint/mul.rs:357:9:
assertion `left == right` failed
left: Uint(0x0000000000000000751EA4E5BF0EB289)
right: Uint(0x7527C959D947A4F1751EA4E5BF0EB289)
baloo
left a comment
baloo
left a comment
There was a problem hiding this comment.
Ran CI on RSA with this change: RustCrypto/RSA#581
|
I generally wish we had better testing coverage testing which is something I intend to pursue after a release. I feel like we've had a few deficiencies which weren't caught by manual code review or tests. It's generally a bit scary to have defects that aren't caught until we actually integration test against a full cryptographic algorithm implementation. And the real worries are in all those untested branches. I keep poking at things like hax which would let us possibly have small snippets of code which are proven to be equivalent to some formally verified reference implementation (for all inputs, using e.g. symbolic execution). |
3 participants
This reapplies the changes in #930 with additional tests for
wrapping_mulandwrapping_square, and a small fix for an error occurring in thersacrate.