Similar to speeding up serialization; in our parsing logic we did a
bunch of expensive stuff then expensively inverted it. Drop everything
except the essential checks and then memcpy.
`secp256k1_pedersen_commit_serialize` would call `_load` (which does a
sqrt to fully decompress the key, then a conditional negation based on
the flag), then check the Jacobian symbol of the resulting y-coordinate,
then re-serialize based on this.
Instead, don't do any of this stuff. Copy the flag directly out of the
internal representation and copy the x-coordinate directly out of the
internal representation.
Checked that none of the other _serialize methods in the modules do
this.
Fixes#293
The expression `G.decode('hex')` fails with the following error message
on Sage 9.5:
AttributeError: 'str' object has no attribute 'decode'
Fix that by converting the hex-string to bytes using `bytes.fromhex`.
The previous implementation returns an off-curve point for the input t=0.
This rewrite addresses that issue by implicity returning the on-curve point
(d, sqrt(1 + b)), which is the point that the paper Indifferentiable Hashing
to Barreto–Naehrig Curves suggests returning in this case.
Note: At the moment it is cryptographically impossible for the input t to be 0.
By providing an uppercase variant of these verification functions, it is
better visible that it is test code and surrounding `#ifdef VERIFY`
blocks can be removed (if there is no other code around that could
remain in production mode), as they don't serve their purpose any more.
At some places intentional blank lines are inserted for grouping and
better readadbility.
Now that the `VERIFY_CHECK` compiles to empty in non-VERIFY mode, blocks
that only consist of these macros don't need surrounding `#ifdef VERIFY`
conditions anymore.
At some places intentional blank lines are inserted for grouping and
better readadbility.
As suggested in issue #1381, this will make things simpler and
improve code readability, as we don't need to force omitting of
evaluations on a case-by-case basis anymore and hence can remove
lots of `#ifdef VERIFY`/`#endif` lines (see next commit). Plus,
VERIFY_CHECK behaves now identical in both non-VERIFY and coverage mode,
making the latter not special anymore and hopefully decreasing
maintenance burden. The idea of "side-effect safety" is given up.
Note that at two places in the ellswift module void-casts of return
values have to be inserted for non-VERIFY builds, in order to avoid
"variable ... set but not used [-Wunused-but-set-variable]"
warnings.
This commit also explicitly initializes shortpubkey. For some reason, removing
surrounding, unrelated lines results in gcc warnings when configured with
--enable-ctime-tests=no --with-valgrind=no.
d23da6d55714271c720fee58fbff5e5ef2fe193f use secp256k1_scalar_verify checks (stratospher)
c7d0454932b42a9728b55033c94e000b1dbbb6f2 add verification for scalars (stratospher)
ad152151b06a40aaf6cd90561356ff451996455d update max scalar in scalar_cmov_test and fix schnorrsig_verify exhaustive test (stratospher)
Pull request description:
From #1360. This PR:
1. adds `secp256k1_scalar_verify` to make sure scalars are reduced mod the group order in VERIFY mode
2. uses `secp256k1_scalar_verify` in all the scalar functions except `secp256k1_scalar_clear`, `secp256k1_scalar_reduce_512`, `secp256k1_scalar_mul_512` and `secp256k1_scalar_*_var` functions in `scalar_low_impl.h`
ACKs for top commit:
real-or-random:
utACK d23da6d55714271c720fee58fbff5e5ef2fe193f
theStack:
Code-review ACK d23da6d55714271c720fee58fbff5e5ef2fe193f
Tree-SHA512: a371b319d948198c4038d35c9ea58f4b94de4dc312215e2b78a323c2acd4ae1355d97935c558b388774832d6d0058b97ff8ca50c3aab40b9ede5307760d0a505
The first byte provided to secp256k1_bppp_parse_one_of_points is allowed to be
0, 1, 2, or 3 since it encodes the Y coordinate of two points. In a previous fix
we wrongly assumed it can only be 2 or 3.
The test is supposed to create an invalid sign byte. Before this PR,
the generated sign byte could in fact be valid due to an overflow.
Co-authored-by: Jonas Nick <jonasd.nick@gmail.com>
- `secp256k1_scalar_set_int` in scalar_low uses input mod EXHAUSTIVE_TEST_ORDER
- directly store s in sig64 without reducing it mod the group order for testing
When configured with `--disable-module-ecdh --enable-module-recovery`, then
`./tests 64 81af32fd7ab8c9cbc2e62a689f642106` fails with
```
src/modules/ellswift/tests_impl.h:396: test condition failed: secp256k1_memcmp_var(share32_bad, share32a, 32) != 0
```
This tests verifies that changing the `party` bit of the
`secp256k1_ellswift_xdh` function results in a different share. However, that's
not the case when the secret keys of both parties are the same and this is
actually what happens in the observed test failure. The keys can be equal in
this test case because they are created by the `random_scalar_order_test`
function whose output is not uniformly random (it's biased towards 0).
This commit restores the assummption that the secret keys differ.
