Remove unnecessary sign variable from wnaf_const #741
Conversation
jonasnick
force-pushed
the
ecmult_const_playground
branch
2 times, most recently
from
0f0030e to
f95fb31
Compare
|
ACK modulo this: It took me some more effort to think about the invariant in the first loop iteration, which depends on all that negation/flipping stuff before the loop. This made me add two tests tests, which for example catch a bug if we'd change |
jonasnick
force-pushed
the
ecmult_const_playground
branch
from
f95fb31 to
0a80990
Compare
|
Thanks for the additional edge case tests. Cherry-picked and squashed. |
jonasnick
force-pushed
the
ecmult_const_playground
branch
from
0a80990 to
37dba32
Compare
Before, test_constant_wnaf used scalar_cadd_bit to correct for the skew. But this function does not correctly deal with overflows which is why num = -1 couldn't be tested. This commit also adds tests for 0, 1/2 and 1/2-1 as they are corner cases in constant_wnaf.
|
ACK 37dba32 I verified the correctness of the change and claimed invariant by manual inspection. I tested the code, both with 32bit and 64bit scalars. |
|
ACK. I reviewed the code and agree that it cannot be negative. I also added a test to check vastly more values, tested with endomorphism on-and-off, and introduced several faults and confirmed that the tests work. |
Summary: * Fix test_constant_wnaf for -1 and add a test for it. Before, test_constant_wnaf used scalar_cadd_bit to correct for the skew. But this function does not correctly deal with overflows which is why num = -1 couldn't be tested. This commit also adds tests for 0, 1/2 and 1/2-1 as they are corner cases in constant_wnaf. * Remove unnecessary sign variable from wnaf_const This is a backport of libsecp256k1 [[bitcoin-core/secp256k1#741 | PR741]] Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D7595
Summary: * Fix test_constant_wnaf for -1 and add a test for it. Before, test_constant_wnaf used scalar_cadd_bit to correct for the skew. But this function does not correctly deal with overflows which is why num = -1 couldn't be tested. This commit also adds tests for 0, 1/2 and 1/2-1 as they are corner cases in constant_wnaf. * Remove unnecessary sign variable from wnaf_const This is a backport of libsecp256k1 [[bitcoin-core/secp256k1#741 | PR741]] Test Plan: ninja check-secp256k1 Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D7595
3 participants
There currently is a single branch in the
ecmul_constfunction that is not being exercised by the tests. This branch is unreachable and therefore I'm suggesting to remove it.For your convenience the paper the wnaf algorithm can be found here (The Width-w NAF Method Provides Small Memory and Fast Elliptic Scalar Multiplications Secure against Side Channel Attacks). Similarly, unless I'm missing something important, I don't see how their algorithm needs to consider
sign(u[i-1])unlessdcan be negative - which doesn't make much sense to me either.