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
c30b889f17e0e75b63cdfa2fe021b0d0b59c4526 Clarify that the ABI-incompatible versions are earlier (Pieter Wuille)
881fc33d0c1fcaac8a2c37a72a1db431329de636 Consistency in naming of modules (Pieter Wuille)
9ecf8149a199e12bb8d6c804878931e8b2e403b9 Reduce font size in changelog (Pieter Wuille)
2dc133a67ff74e088a75993fe2ed3d5c6c369915 Add more changelog entries (Pieter Wuille)
ac233e181a5cd96cb3918d669b035221baa18062 Add links to diffs to changelog (Pieter Wuille)
cee8223ef6d6b2d60b0b81762f1aeb0fd40613e0 Mention semantic versioning in changelog (Pieter Wuille)
Pull request description:
ACKs for top commit:
real-or-random:
ACK c30b889f17e0e75b63cdfa2fe021b0d0b59c4526
jonasnick:
ACK c30b889f17e0e75b63cdfa2fe021b0d0b59c4526
Tree-SHA512: 0f753eae0ea4d65035bfbcd81b90169111ea030cf7196dd072fb1ccc8aac1437768031f3fcef431584028da68b66873204e16e03bcde4a6ae96b08ab7f97a480
9c5a4d21bbe820173e361211f42d5fee4765944c Do not define unused `HAVE_VALGRIND` macro (Hennadii Stepanov)
ad8647f548cba9c367f837fcdb76de280fa93c19 Drop no longer relevant files from `.gitignore` (Hennadii Stepanov)
b627ba7050b608e869515a8ef622d71bf8c13b54 Remove dependency on `src/libsecp256k1-config.h` (Hennadii Stepanov)
Pull request description:
Cherry-picked the first commit from #1142 and addressed a [comment](https://github.com/bitcoin-core/secp256k1/pull/1142#issuecomment-1295099597).
ACKs for top commit:
sipa:
utACK 9c5a4d21bbe820173e361211f42d5fee4765944c
real-or-random:
utACK 9c5a4d21bbe820173e361211f42d5fee4765944c
Tree-SHA512: c6f268261fc5edee855a7e69fdf9f6c5f4b859eb1e078e3c44c3ee4c9c445738af3de9fc2fbcca90db9b9e38681da8217faaeb0735201052b16ea397a7817db9
7a74688201318cbbe30b0d1601aae16dc14ee17a ci: add missing CFLAGS & CPPFLAGS variable to print_environment (Jonas Nick)
c2e0fdadebd2f9bf06fd73b9e89dae03133d71f9 ci: set -u in cirrus.sh to treat unset variables as an error (Jonas Nick)
Pull request description:
This PR is supposed to prevent accidental misuse of cirrus.sh. Maybe there is a way to check if `CC`, `AR` and `NM` are set within the loop that deals with the other variables, but so far I did not come up with one (that's POSIX shell compliant).
ACKs for top commit:
real-or-random:
ACK 7a74688201318cbbe30b0d1601aae16dc14ee17a
hebasto:
re-ACK 7a74688201318cbbe30b0d1601aae16dc14ee17a
Tree-SHA512: 91e42b3f1192fbf86e6fb43942713e78b2bee977ddd95256ea7448f84324369399d31ec4eedd47af595bf994bbc9396e26bb5c93bdb7f58c4310b5d3d5d66731
