scalar_split_lambda requires that the input pointer is different to both output
pointers. Without this fix, the internal benchmarks crash when compiled with
-DVERIFY.
This was introduced in commit 362bb25608dbcd724a07dd5170c4ebe081c3dd84 (which
requires configuring with --enable-endomorphism to exhibit the crash).
e39d954f118a29db2c33e9a9a507053fff5573ed tests: Add CHECK_ILLEGAL(_VOID) macros and use in static ctx tests (Tim Ruffing)
61841fc9ee5aa1ffde3ccd512660207034125ebd contexts: Forbid randomizing secp256k1_context_static (Tim Ruffing)
4b6df5e33e197a50fd7d9bc4c14b8ba8526013b9 contexts: Forbid cloning/destroying secp256k1_context_static (Tim Ruffing)
Pull request description:
As discussed in #1126.
For randomization, this has a history. Initially, this threw the illegal callback but then we changed it to be a no-op on non-signing contexts: 6198375218 But this was with (non-static) none/verification contexts in mind, not with the static context. If we anyway forbid cloning the static context, you should never a way to randomize a copy of the static context. (You need a copy because the static context itself is not writable. But you cannot obtain a copy except when using memcpy etc.)
ACKs for top commit:
sipa:
utACK e39d954f118a29db2c33e9a9a507053fff5573ed
apoelstra:
ACK e39d954f118a29db2c33e9a9a507053fff5573ed
Tree-SHA512: dc804b15652d536b5d67db7297ac0e65eab3a64cbb35a9856329cb87e7ea0fe8ea733108104b3bba580077fe03d6ad6b161c797cf866a74722bab7849f0bb60c
8f51229e0348a1524fed541f334cd4f1726d2685 ctime_tests: improve output when CHECKMEM_RUNNING is not defined (Jonas Nick)
Pull request description:
When seeing the output
```
Unless compiled under msan, this test can only usefully be run inside valgrind.
```
I thought that I would have to go back to the `configure` output to manually check if it was compiled under memsan to determine whether this test can be usefully run outside valgrind. But when we go into this branch then it was definitely not compiled under msan, which means that we can make the output clearer.
ACKs for top commit:
sipa:
utACK 8f51229e0348a1524fed541f334cd4f1726d2685
real-or-random:
utACK 8f51229e03
Tree-SHA512: a4953a158b1375d8fc3a2ee29e7014c5399becf5f75ffd3765c0141861e092fbc120003e00dfd25ec54b92a466e133377b96d5a9f4017c100aaf64fb9a045df1
2cd4e3c0a976c78c1619fc7456fcc4eaa92897a9 Drop no longer used `SECP_{LIBS,INCLUDE}` variables (Hennadii Stepanov)
613626f94c77a484f6acf22a72ab6cd8ddda00cd Drop no longer used `SECP_TEST_{LIBS,INCLUDE}` variables (Hennadii Stepanov)
Pull request description:
`SECP_INCLUDES`, `SECP_LIBS`, `SECP_TEST_LIBS` and `SECP_TEST_INCLUDES` were introduced in 78cd96b15153e209cf4829a511f9efdfdcf7e4d0.
The last usage of the `SECP_TEST_{LIBS,INCLUDE}` variables was removed in https://github.com/bitcoin-core/secp256k1/pull/983.
The last usage of the `SECP_LIBS` variable was removed in https://github.com/bitcoin-core/secp256k1/pull/831.
The last usage of the `SECP_INCLUDE` variable was removed in https://github.com/bitcoin-core/secp256k1/pull/1169.
ACKs for top commit:
sipa:
utACK 2cd4e3c0a976c78c1619fc7456fcc4eaa92897a9
real-or-random:
utACK 2cd4e3c0a976c78c1619fc7456fcc4eaa92897a9
Tree-SHA512: ceee39dfb74aaeaa9a1e52fba819f32cee8e08922872bca2bfd6db8575c9b4695da476a4b8e8579abb92d6484fbf461e691369b160ecbc792261dbb454349efb
d6ff738d5bbdf965590fc07efca23b13c0ea3082 Ensure safety of ctz_debruijn implementation. (Russell O'Connor)
Pull request description:
Adding `U` to the magic constants ensures that we are not mixing unsigned and signed value during multiplication, and ensures that the multiplication will not be subject to integer promotion.
The `(uint32_t)`/`(uint64_t)` casts ensure the values are properly truncated no matter the size of an int.
Prior to this commit, if `secp256k1_ctz32_var_debruijn` were some how managed to be built on a platform with 64-bit ints, (though this function is specifically only intended to be used on 32-bit platforms) it would perform an out-of-bounds array access.
ACKs for top commit:
real-or-random:
utACK d6ff738d5bbdf965590fc07efca23b13c0ea3082
apoelstra:
ACK d6ff738d5bbdf965590fc07efca23b13c0ea3082
Tree-SHA512: f2292fa6e03deff4598514f9070b1357ce307ce1d2b34c15da120198c2f9171dfae9e0aaddb99f2c577ec368a903337eb68281518e93e43c381c9875aa84144e
Adding U to the magic constants ensures that we are not mixing unsigned and signed value during multiplication, and ensures that the multiplication will not be subject to integer promotion.
The (uint32_t)/(uint64_t) casts ensure the values are properly truncated no matter the size of an int.
Prior to this commit, if secp256k1_ctz32_var_debruijn were some how managed to be built on a platform with 64-bit ints, (though this function is specifically only intended to be used on 32-bit platforms) it would perform an out-of-bounds array access.
ce60785b2654e60b43577dd75996b7020afbfec8 Introduce SECP256K1_B macro for curve b coefficient (Pieter Wuille)
4934aa79958b506a6e9cfcfe30a8f685db3f5f5f Switch to exhaustive groups with small B coefficient (Pieter Wuille)
Pull request description:
This has the advantage that in the future, multiplication with B can be done using `secp256k1_fe_mul_int` rather than the slower `secp256k1_fe_mul`.
ACKs for top commit:
real-or-random:
ACK ce60785b2654e60b43577dd75996b7020afbfec8 also ran the exhaustive tests with the group of size 7
apoelstra:
ACK ce60785b2654e60b43577dd75996b7020afbfec8
Tree-SHA512: 006041189d18319ddb9c0ed54e479f393b83ab2a368d198bd24860d1d2574c0c1a311aea24fbef2e74bb7859a687dfc803b9e963e6dc5c61cb707e20f52b5a70
0f088ec11263261497661215c110a4c395acc0ac Rename CTIMETEST -> CTIMETESTS (Pieter Wuille)
74b026f05d52216fa4c83cbfada416a30ddfc9b9 Add runtime checking for DECLASSIFY flag (Pieter Wuille)
5e2e6fcfc0ebcdaad96fda9db9b8946d8bcdc8e5 Run ctime test in Linux MSan CI job (Pieter Wuille)
18974061a3ffef514cc393768401b2f104fe6cef Make ctime tests building configurable (Pieter Wuille)
5048be17e93a21ab2e33b939b40339ed4861a692 Rename valgrind_ctime_test -> ctime_tests (Pieter Wuille)
6eed6c18ded7bd89d82fe1ebb13b488a2cf5e567 Update error messages to suggest msan as well (Pieter Wuille)
8e11f89a685063221fa4c2df0ee750d997aee386 Add support for msan integration to checkmem.h (Pieter Wuille)
8dc64079eb1db5abafbc18e335bcf179ae851ae8 Add compile-time error to valgrind_ctime_test (Pieter Wuille)
0db05a770ebd41999b88358ee6ab4bdd6a7d57ee Abstract interactions with valgrind behind new checkmem.h (Pieter Wuille)
4f1a54e41d84a81e4506668bfabed1f3c632973b Move valgrind CPPFLAGS into SECP_CONFIG_DEFINES (Pieter Wuille)
Pull request description:
This introduces an abstraction layer `src/checkmem.h`, which defines macros for interacting with memory checking tools. Depending on the environment, they're mapped to MemorySanitizer builtins, Valgrind integration macros, or nothing at all.
This means that msan builds immediately benefit from existing undefined memory checks in the tests. It also means those builds result in a `ctime_tests` (new name for `valgrind_ctime_test`) binary that can usefully test constant-timeness (not inside Valgrind, and with the downside that it's not running against a production library build, but it's faster and available on more platforms).
Such an msan-ctime test is added to the Linux x86_64 msan CI job, as an example. More CI cases could be added (e.g. for MacOs or ARM Linux) later.
ACKs for top commit:
real-or-random:
ACK 0f088ec11263261497661215c110a4c395acc0ac
hebasto:
ACK 0f088ec11263261497661215c110a4c395acc0ac, I have reviewed the code and it looks OK. Able to build `ctime_tests` using MSan.
Tree-SHA512: f4ffcc0c2ea794894662d9797b3a349770a4b361996f967f33d7d14b332171de5d525f50bcebaeaf7d0624957083380962079c75e490d1b7d71f8f9eb6211590
d4a6b58df7490ff9c656e158f246cf396b4cfa72 Add `noverify_tests` to `.gitignore` (Hennadii Stepanov)
Pull request description:
This is a follow up of #1188.
ACKs for top commit:
sipa:
ACK d4a6b58df7490ff9c656e158f246cf396b4cfa72
real-or-random:
utACK d4a6b58df7490ff9c656e158f246cf396b4cfa72
Tree-SHA512: a249c949d4b1432c6a5ff05a49f51a1f605f026ce6faa01bebee12a49d1ad2e38a344c35d2a21b827ceb40190448306262af7ca9a4385ebd96115d18ace42856
e862c4af0c5a7300129700d38eff499a836a108d Makefile: add -I$(top_srcdir)/src to CPPFLAGS for precomputed (Matt Whitlock)
Pull request description:
When performing an out-of-source-tree build, regenerating the source files for the precomputed ecmult tables places them outside the source tree. Then, when they are to be compiled, they cannot find the headers they need because the source tree is absent from their include search path. This appears to have been an oversight, as the relevant `-I` options are present in `libsecp256k1_la_CPPFLAGS` but were missing from `libsecp256k1_precomputed_la_CPPFLAGS`. This PR adds them.
ACKs for top commit:
sipa:
utACK e862c4af0c5a7300129700d38eff499a836a108d
real-or-random:
ACK e862c4af0c5a7300129700d38eff499a836a108d
Tree-SHA512: f58b8670b2798f2ca4bd6e9fd83218afcd14cf1b796cd18fb40e7b8a148dcdfabe5f0beae81bc6b82727c97a507431e6a7c72d756587e047daf1ea81242cccf9
9a93f48f502da7aaa893b90a575434892b23fc9e refactor: Rename STTC to STATIC_CTX in tests (Tim Ruffing)
3385a2648d7e9dd03094bb65065f30f385101fef refactor: Rename global variables to uppercase in tests (Tim Ruffing)
Pull request description:
On top of #1186 .
I feel that this is an improvement, but it touches a lot of lines and so it deserves a separate discussion.
ACKs for top commit:
sipa:
ACK 9a93f48f502da7aaa893b90a575434892b23fc9e
Tree-SHA512: b6dad2ffff2267034bf8cefdd3ef7ea11e9bcb8142d64b460ca61e0d3ab8de22fb3ee994dea0fb32feee3864d07395c070abffab318690d09d104294895300c4
203760023c60d250cb5937e27bcf29e7a829096c tests: Add noverify_tests which is like tests but without VERIFY (Tim Ruffing)
Pull request description:
mentioned in https://github.com/bitcoin-core/secp256k1/issues/1037#issuecomment-1371870423
Let's see how this affects CI time
ACKs for top commit:
sipa:
ACK 203760023c60d250cb5937e27bcf29e7a829096c
apoelstra:
ACK 203760023c60d250cb5937e27bcf29e7a829096c
Tree-SHA512: fab1ce1499d418671d3d0ecfddf15d75b7c2bbfbfb4be958a95730491244185a906c7133aba4d0bec56ee6c721cb525750eef4cafc12f386484af931e34b0e8e
When performing an out-of-source-tree build, regenerating the source
files for the precomputed ecmult tables places them outside the source
tree. Then, when they are to be compiled, they cannot find the headers
they need because the source tree is absent from their include search
path. This appears to have been an oversight, as the relevant -I options
are present in libsecp256k1_la_CPPFLAGS but were missing from
libsecp256k1_precomputed_la_CPPFLAGS. This commit adds them.
39e8f0e3d7ba7924e9cc5f9e0c56747e942f1eab refactor: Separate run_context_tests into static vs proper contexts (Tim Ruffing)
a4a09379b1a6f65d5a1801cffae0992b49660d82 tests: Clean up and improve run_context_tests() further (Tim Ruffing)
fc90bb569564d552ec0b5706fde6e94bb5313f4e refactor: Tidy up main() (Tim Ruffing)
f32a36f620e979b13040ffd2cd55cfc6fac5bad0 tests: Don't use global context for context tests (Tim Ruffing)
ce4f936c4fa077d0473985479c61bd6544172aae tests: Tidy run_context_tests() by extracting functions (Tim Ruffing)
18e0db30cb4a89989f040a5f212d54b306ffd96e tests: Don't recreate global context in scratch space test (Tim Ruffing)
b19806122e9065c6f434fc6160cd0c57fa3fea8c tests: Use global copy of secp256k1_context_static instead of clone (Tim Ruffing)
Pull request description:
This is an improved version of some of the tidying/refactoring in #1170.
I think it's enough to deserve a separate PR. Once this is merged, I'll get back to the actual goal of #1170 (namely, forbidding cloning and randomizing static contexts.)
This PR is a general clean up of the context tests. A notable change is that this avoids a code smell where `run_context_tests()` would use the global `ctx` variable like a local one (i.e., create a context in it and destroy it afterwards). After this PR, the global `ctx` is properly initialized for all the other tests, and they can decide whether they want to use it or not. Same for a global `sttc`, which is a memcpy of the static context (we need a writable copy in order to be able to set callbacks).
Note that this touches code which is also affected by #1167 but I refrained from trying to solve this issue. The goal of this PR is simply not to worsen the situation w.r.t. #1167. We should really introduce a macro to solve #1167 but that's another PR.
ACKs for top commit:
sipa:
utACK 39e8f0e3d7ba7924e9cc5f9e0c56747e942f1eab
apoelstra:
ACK 39e8f0e3d7ba7924e9cc5f9e0c56747e942f1eab
Tree-SHA512: a22471758111061a062b126a52a0de24a1a311d1a0332a4ef006882379a4f3f2b00e53089e3c374bf47c4051bb10bbc6a9fdbcf6d0cd4eca15b5703590395fba
2f9ca284e2af0c738e701b3dc3e348b87f26a7a2 Drop `SECP_CONFIG_DEFINES` from examples (Hennadii Stepanov)
Pull request description:
User applications shouldn't need or rely on `SECP_CONFIG_DEFINES`.
See https://github.com/bitcoin-core/secp256k1/pull/1178#discussion_r1059457252.
ACKs for top commit:
sipa:
utACK 2f9ca284e2af0c738e701b3dc3e348b87f26a7a2
real-or-random:
utACK 2f9ca284e2af0c738e701b3dc3e348b87f26a7a2
Tree-SHA512: c8e81e6842b31e7f4ebcbb18d5962f7d7308f024025d6225330a7ec099739278bb43ad98243698c5802bcc49bf7e247ab7cae7f40008fbba87f0d0e46cbe1e85
d21647520532957a78027be1ab606b814a2ec720 test secp256k1_i128_to_i64 (Russell O'Connor)
4bc429019dc4bff6af0f9824ad6ab6745f09f8ba Add a secp256k1_i128_to_u64 function. (Russell O'Connor)
Pull request description:
I wanted to experiment with what would be required to split up `secp256k1_i128_to_i64` between those cases when a signed 64 bit value is being demoted, versus an unsigned 64 bit value is being extracted from the lower bits, and this is the result.
I'm not sure this is a useful PR, so feel free to close it. However, since it is already written, I figured it is worth at least discussing.
ACKs for top commit:
sipa:
utACK d21647520532957a78027be1ab606b814a2ec720
real-or-random:
ACK d21647520532957a78027be1ab606b814a2ec720
Tree-SHA512: 41dbb1d33b3078bee8e71a838cfad6f1859c0bba602ae061259add8e9e8ea5aa482daa41de79dbd7433ddbef4a0bc52757f3c45d63acc9c0eb05aa3ca891b922
