f1652528be5a287a3c33a4fae1e5763693333c2b Normalize ge produced from secp256k1_pubkey_load (stratospher)
Pull request description:
The output `ge` in secp256k1_pubkey_load is normalized when `sizeof(secp256k1_ge_storage) = 64` but not when it's not 64. ARG_CHECK at the end of the function assumes normalization. So normalize ge in the other code path too.
context: [#1129(comment)](https://github.com/bitcoin-core/secp256k1/pull/1129/files#r1196167066)
ACKs for top commit:
sipa:
utACK f1652528be5a287a3c33a4fae1e5763693333c2b
real-or-random:
ACK f1652528be5a287a3c33a4fae1e5763693333c2b tested by changing the two `== 64` checks to `== 65`
Tree-SHA512: 0de1caad85ccdb42053f8e09576135257c88fda88455ef25e7640049c05a1e03d1e9bae1cd132d2e6fc327fd79929257a8b21fe1cc41c82374b6cd88e6744aa3
7067ee54b4206c26b382980f3c20b5fa0262a23a tests: add tests for `secp256k1_{read,write}_be64` (Sebastian Falbesoner)
740528caad8c37e335cba2bcd02790d94c22e767 scalar: use newly introduced `secp256k1_{read,write}_be64` helpers (4x64 impl.) (Sebastian Falbesoner)
Pull request description:
This is a simple follow-up to #1339, as suggested in comment https://github.com/bitcoin-core/secp256k1/pull/1339#issuecomment-1587508040.
ACKs for top commit:
stratospher:
ACK 7067ee5.
real-or-random:
utACK 7067ee54b4206c26b382980f3c20b5fa0262a23a
Tree-SHA512: f9bc2ab610099948ffac1e6bb3c822bd90b81a7110ab74cec03175e2c92ed27694a15f9cdaa7c4f1b460fe459f61c3d1d102c99592169f127fdd7539a1a0c154
The output ge is normalized when sizeof(secp256k1_ge_storage) = 64
but not when it's not 64. ARG_CHECK at the end of the function
assumes normalization. So normalize ge in the other code path too.
After calculating the right-hand side of the elliptic curve equation
(x^3 + 7), the field element `x3` has a magnitude of 2 (1 as result of
`secp256k1_fe_mul`, then increased by 1 due to `secp256k1_fe_add_int`).
This is fine for `secp256k1_fe_equal_var`, as the second parameter only
requires the magnitude to not exceed 31, and the normalize_weak call can
hence be dropped.
887183e7de4d1941792d78471a3adb004fd4ec8e scalar: use `secp256k1_{read,write}_be32` helpers (4x64 impl.) (Sebastian Falbesoner)
52b84238de27889f2517c845757bf5c2c65ed1d6 scalar: use `secp256k1_{read,write}_be32` helpers (8x32 impl.) (Sebastian Falbesoner)
Pull request description:
This refactoring PR takes use of the `secp256k1_{read,write}_be32` helpers (introduced in PR #1093, commit 8d89b9e6e562000cdb91a70a85fae5e4817cec8a) in the scalar <-> byte array conversion functions, for both the 8x32 and 4x64 implementations. (An alternative for the latter would be to introduce special helpers for reading/writing uint64_t in big endian `secp256k1_{read,write}_be64`).
Verified via `objdump -D libsecp256k1.a` that `secp256k1_scalar_set_b32` for 4x64 compiles to the same code on master and the PR (`secp256k1_scalar_get_b32` is apparently always inlined) on amd64 with clang 13.0.0.
ACKs for top commit:
sipa:
utACK 887183e7de4d1941792d78471a3adb004fd4ec8e
Tree-SHA512: 915cb4624c6da0530dce4ec3ac48e88dd735386302cd2e15759e3c30102d81186f382ffe71493ddd0538069f1b558db543d9bb900dfdb69acb60effedc33f705
f3644287b10cd7339fbb0e35e953235351f5efb6 docs: correct `pubkey` param descriptions for `secp256k1_keypair_{xonly_,}pub` (Sebastian Falbesoner)
Pull request description:
ACKs for top commit:
real-or-random:
ACK f3644287b1 because it's consistent with the other docs
jonasnick:
ACK f3644287b10cd7339fbb0e35e953235351f5efb6
Tree-SHA512: cc4db4637301335ea9d23ac43bb3a78de54af79a5262dba2013945f87d80670c7ae1e106101a59c04225eb077e9a9e0ecc9d9d3bfe2d11cdc90f098ebd479f49
From an API perspective, the functions `secp256k1_keypair_pub` and
`secp256k1_keypair_xonly_pub` always succeed (i.e. return the value 1),
so the other cases in the `pubkey` parameter descriptions never happen
and can hence be removed.
Note that the "1 always" return value description was previously done in
commit b8f8b99f0fb3a5cd4c6fb1c9c8dfed881839e19e (PR #1089), which also
explains why invalid inputs for the affected functions are in practice
only possible in violation of the type system.
db29bf220c73bc71b9d48c262ba2e88c1e741f92 ci: Remove quirk that runs dummy command after wineserver (Tim Ruffing)
c7db4942b34acd2a34e6249112f6c1db6cf5681d ci: Fix error D8037 in `cl.exe` (Hennadii Stepanov)
7dae11586135b59bd59fd9889c56aa9b6509021e Revert "ci: Move wine prefix to /tmp to avoid error D8037 in cl.exe" (Hennadii Stepanov)
Pull request description:
Since the 2146cbfaf0, the `msvc-wine` effectively initializes the WINE prefix when running the `install.sh` script. See [`install.sh`#L143](2146cbfaf0/install.sh (L143)):
```sh
WINEDEBUG=-all wine64 wineboot &>/dev/null
```
Our following `wine64 wineboot --init` just messes up with the prefix.
This PR fixes this issue.
Also https://github.com/bitcoin-core/secp256k1/pull/1327 has been reverted as apparently it does not work. And https://github.com/bitcoin-core/secp256k1/pull/1320 has been combined into this one.
ACKs for top commit:
real-or-random:
ACK db29bf220c73bc71b9d48c262ba2e88c1e741f92
Tree-SHA512: 59e61bde0060f67501f93da8b4e193f2bfcda85d849c16bb017e38af7aa9e3b569fe2fd4aa5cdb658c3b2345cc42fad98323e329b519389b2e881ecfd403d147
605e07e3657e4ff8d11893ee680a0446febf73a1 fix input range comment for `secp256k1_fe_add_int` (Sebastian Falbesoner)
Pull request description:
This seems to be a typo that was introduced with commit 4371f98346b0a50c0a77e93948fe5e21d9346d06 (PR #1066).
ACKs for top commit:
sipa:
ACK 605e07e3657e4ff8d11893ee680a0446febf73a1
real-or-random:
ACK 605e07e3657e4ff8d11893ee680a0446febf73a1
Tree-SHA512: 7ee99cf7140c698d1146072734ba986de7328f78b2c076ee445067ef64a6a335c8669f1e733e10f5e14f98b566c799cc4c51b3eb0f036cd178b3c93476c6df2e
ade5b367018a624ff7ca1ecbb4a64889d47b0142 tests: add checks for scalar constants `secp256k1_scalar_{zero,one}` (Sebastian Falbesoner)
654246c63585422a184121a26d42dcae792e87c6 refactor: take use of `secp256k1_scalar_{zero,one}` constants (Sebastian Falbesoner)
Pull request description:
Rather than allocating a (non-constant) scalar variable on the stack with the sole purpose of setting it to a constant value, the global constants `secp256k1_scalar_{zero,one}` (apparently introduced in 34a67c773b0871e5797c7ab506d004e80911f120, PR #710) can be directly used instead for the values 0 or 1. There is very likely not even a difference in run-time, but it leads to simpler and less code which might be nice.
ACKs for top commit:
sipa:
utACK ade5b367018a624ff7ca1ecbb4a64889d47b0142
real-or-random:
utACK ade5b367018a624ff7ca1ecbb4a64889d47b0142
Tree-SHA512: 0ff05a449c153f7117a4a56efef04b2087c2330f4692f3390a0b1d95573785ac7ae3fe689ed0ec2ecc64b575d2489d6e341d32567e75a1a4b4d458c3ecd406a1
e83801f5db2cb63e6343eff0ecfa40b7cd4a9090 test: Warn if both `VERIFY` and `COVERAGE` are defined (Hennadii Stepanov)
Pull request description:
Solves one item in https://github.com/bitcoin-core/secp256k1/issues/1235.
Also see: https://github.com/bitcoin-core/secp256k1/pull/1113#discussion_r1127856040.
ACKs for top commit:
sipa:
utACK e83801f5db2cb63e6343eff0ecfa40b7cd4a9090
real-or-random:
ACK e83801f5db2cb63e6343eff0ecfa40b7cd4a9090
Tree-SHA512: 25e10a09ba2c3585148becd06f2a03d85306208bda333827c9ba73eb7fd94ad15536f10daf1b335703e5cb0539584f001501ce9c578f478ff1ebc1051aefde7d
27504d5c941df89bc828067248270179c9dcb04b ci: Move wine prefix to /tmp to avoid error D8037 in cl.exe (Tim Ruffing)
Pull request description:
Don't ask me why this makes a difference. It may be some permission problem even though everything in Cirrus CI runs as root anyway. In any case, I'll probably get mad if I investigate this further.
Fixes#1326.
ACKs for top commit:
hebasto:
ACK 27504d5c941df89bc828067248270179c9dcb04b, tested in my personal Cirrus account.
Tree-SHA512: 08bb1734827579b59c705a44ee8fad6d504031eb5659c2743649be95fb048794b95ac0869a994bfa732f7f0714b4d12674c325637fe079b2266f18a3c14bbec0
Don't ask me why this makes a difference. It may be some permission
problem even though everything in Cirrus CI runs as root anyway. In
any case, I'll probably get mad if I investigate this further.
Fixes#1326.
6433175ffe2435bcee7333e21480e4194083caae Do not invoke fe_is_zero on failed set_b32_limit (Pieter Wuille)
Pull request description:
Noticed in the CI output of #1313 (https://cirrus-ci.com/task/5117786435878912)
The code violates the field element contract that states that a field element that comes out of a failed `secp256k1_fe_set_b32_limit` call cannot be used before overwriting it. This is not an issue in practice, as such failure can only occur with negligible probability, but the experimental compiler in that CI setting is technically correct in detecting this possibility.
Fix it by setting it to 1 based on a `secp256k1_fe_normalizes_to_zero` test rather than a `secp256k1_fe_is_zero` one (which does not require normalization).
ACKs for top commit:
stratospher:
ACK 6433175
real-or-random:
utACK 6433175ffe2435bcee7333e21480e4194083caae
Tree-SHA512: 49da4535181c4607c1f4d23d1fd7cd65e7751c7cfa68643f1da77f3ec7961754fc8553bb415137fd61d86c805fe69f5adf97c05b9dc4d3bf357ae7c6409cc51a
5768b502291ed4214cf84b7d80ff540ed57d45c4 build: Enable -DVERIFY for precomputation binaries (Tim Ruffing)
Pull request description:
because... why not?!
I realized that this can't hurt when working on #1313.
ACKs for top commit:
sipa:
ACK 5768b502291ed4214cf84b7d80ff540ed57d45c4
Tree-SHA512: 2412cb93097f5c7904cfded6816bc5cdc69d958b4023ddaffd6e7575615ac5bfcd3a7cfc9ce2c0b0e6526a6f000dd84ecd32909d9d207a3644aadb5d34905911
31b4bbee1e115865a8a3aff6ccf04f6108371c5d Make fe_cmov take max of magnitudes (Pieter Wuille)
Pull request description:
This addresses part of #1001.
The magnitude and normalization of the output of `secp256k1_fe_cmov` should not depend on the runtime value of `flag`.
ACKs for top commit:
real-or-random:
utACK 31b4bbee1e115865a8a3aff6ccf04f6108371c5d
stratospher:
ACK 31b4bbe.
Tree-SHA512: 08bef9f63797cb8a1f3ea63c716c09aaa267dfee285b74ef5fbb47d614569d2787ec73d21bce080214872dfe70246f73cea42ad3c24e6baccecabe3312f71433
3ad1027a4034da674aeee2a92dfba69b347bbe91 Revert "Remove unused scratch space from API" (Jonas Nick)
Pull request description:
This reverts commit 712e7f8722eba5dec2bc6b37d75aadeb6f6e633b.
Removing the scratch space from the API may break bindings to the library.
ACKs for top commit:
sipa:
ACK 3ad1027a4034da674aeee2a92dfba69b347bbe91
real-or-random:
ACK 3ad1027a4034da674aeee2a92dfba69b347bbe91
Tree-SHA512: ad394c0a2f83fe3a5f400c0e8f2b9bf40037ce4141d4414e6345918f5e6003c61da02a538425a49bdeb5700f5ecb713bd58f5752c0715fb1fcc4950099fdc0e6
