e637dc2c01 refactor: Replace uint256 type with Wtxid in PackageMempoolAcceptResult struct (marcofleon)
a3baead7cb validation: use wtxid instead of txid in CheckEphemeralSpends (marcofleon)
Pull request description:
This PR addresses a small bug in [`AcceptMultipleTransactions`](https://github.com/bitcoin/bitcoin/blob/45719390a1434ad7377a5ed05dcd73028130cf2d/src/validation.cpp#L1598) where a txid was being inserted into a map that should only hold wtxids. `CheckEphemeralSpends` has an out parameter on failure that records that the child transaction did not spend the parent's dust. Instead of using the txid of this child, use its wtxid.
The second commit in this PR is a refactor of the `PackageMempoolAcceptResult` struct to use the `Wtxid` type instead of `uint256`. This helps to prevent errors like this in the future.
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/32025/commits/e637dc2c01c3b566e6c51c911c5881a8d206c924
glozow:
ACK e637dc2c01, hooray for type safety
dergoegge:
Code review ACK e637dc2c01
Tree-SHA512: 17039efbb241b7741e2610be5a6d6f88f4c1cbe22d476931ec99e43f993d259a1a5e9334e1042651aff49edbdf7b9e1c1cd070a28dcba5724be6db842e4ad1e0
568fcdddae scripted-diff: Adjust documentation per top-level target output location (Hennadii Stepanov)
026bb226e9 cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
This PR sets the target output locations to the `bin` and `lib` subdirectories within the build tree, creating a directory structure that mirrors that of the installed targets.
This approach is widely adopted by the large projects, such as [LLVM](https://github.com/llvm/llvm-project/blob/e146c1867e8decfd423034f63a3a863733e03f04/lldb/cmake/modules/LLDBStandalone.cmake#L128-L130):
```cmake
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
```
The `libsecp256k1` project has also recently [adopted](https://github.com/bitcoin-core/secp256k1/pull/1553) this approach.
With this PR, all binaries are conveniently located. For example, run:
```
$ ./build/bin/fuzz
```
instead of:
```
$ ./build/src/test/fuzz/fuzz
```
On Windows, all required DLLs are now located in the same directory as the executables, allowing to run `bitcoin-chainstate.exe` (which loads `bitcoinkernel.dll`) without the need to copy DLLs or modify the `PATH` variable.
The idea was briefly discussed among the build team during the recent CoreDev meeting.
---
**Warning**: This PR changes build locations of newly built executables like `bitcoind` and `test_bitcoin` from `src/` to `bin/` without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.
ACKs for top commit:
theStack:
Light re-ACK 568fcdddae
ryanofsky:
Code review ACK 568fcdddae. Only change since last review was rebasing. I'm ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to \*silently\* break an everyday developer workflow like `git pull; make bitcoind`. I wouldn't have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
TheCharlatan:
Re-ACK 568fcdddae
theuni:
ACK 568fcdddae
Tree-SHA512: 1aa5ecd3cd49bd82f1dcc96c8e171d2d19c58aec8dade4bc329df89311f9e50cbf6cf021d004c58a0e1016c375b0fa348ccd52761bcdd179c2d1e61c105e3b9f
fac1dd9dff test: Fix authproxy named args debug logging (MarcoFalke)
Pull request description:
In Python the meaning of `args or argsn` is that `argsn` is fully ignored when `args` is a list with at least one element. However, the RPC server accepts mixed positional and named args in the same RPC.
Fix the debug log by always printing both. Also, add a new `_json_dumps` helper to avoid bloated code.
Can be tested via `--tracerpc` on a call that uses named args mixed with positional args.
ACKs for top commit:
i-am-yuvi:
Tested ACK fac1dd9dff
rkrux:
tACK fac1dd9dff
musaHaruna:
Tested ACK [fac1dd9](https://github.com/bitcoin/bitcoin/commit/fac1dd9dffba1033245c283bc0468e801c14e910)
ryanofsky:
Code review ACK fac1dd9dff. Thanks for logging fix. This change should have been included in #19762
Tree-SHA512: ff63fbc2564b2c7589e9294baacf4c7a79f10d593776813392510702ca726e3893a29db3ba261f3aee1789a59bb215d7cb10fc85ca1a02632631d3722ddcdfc5
The Windows binaries are particularly big when they contain debug
info, closing in on 500mb. Add a note to the Windows build instructions
about using `--strip`.
d76647eb8f ci: use LLVM 20.1.0 for MSAN (fanquake)
Pull request description:
Now that the release is out, stop using rc1.
ACKs for top commit:
l0rinc:
utACK d76647eb8f
Sjors:
ACK d76647eb8f
Tree-SHA512: 2eefad118dd617c3e9554caee1549965fc4acc437dbef1252c1912faea57ccbfcb39f879911f27f4274931c1d8723b9c7f981aafa6958e70d4371f16c368c627
e181bda061 guix: Apply all codesignatures to Windows binaries (Ava Chow)
aafbd23fd9 guix: Apply codesignatures to all MacOS binaries (Ava Chow)
3656b828dc contrib: Sign all Windows binaries too (Ava Chow)
31d325464d contrib: Sign and notarize all MacOS binaries (Ava Chow)
710d5b5149 guix: Update signapple (Ava Chow)
e8b3c44da6 build: Include all Windows binaries for codesigning (Ava Chow)
dd4ec840ee build: Include all MacOS binaries for codesigning (Ava Chow)
4e5c9ceb9d guix: Rename Windows unsigned binaries to unsigned.zip (Ava Chow)
d9d49cd533 guix: Rename MacOS binaries to unsigned.tar.gz (Ava Chow)
c214e5268f guix: Rename unsigned.tar.gz to codesigning.tar.gz (Ava Chow)
Pull request description:
I have updated signapple to notarize MacOS app bundles without adding any additional dependencies. Further, it can also sign and apply detached signatures to standalone binaries.
As such, we can use signapple to perform the notarization and stapling steps so that MacOS will run the app bundle after it is installed. `detached-sig-create.sh` is updated to have a notarization step and to download the ticket which will be included in the detached signatures. The workflow is largely unchanged for the MacOS codesigners except for the additional requirement of having an App Store Connect API key and Team UUID, instructions for which can be found at https://github.com/achow101/signapple/blob/master/docs/notarization.md. For guix builders, the workflow is unchanged.
Additionally, the standalone binaries packaged in the MacOS `.tar.gz` and Windows `.zip` will now be codesigned. `detached-sig-create.sh` was updated to handle these, so the workflow for both MacOS and Windows codesigners remains unchanged. For guix builders, the workflow is also unchanged.
Because those binaries will how have codesigned and unsigned versions, the build command is modified to output `-unsigned.{tar.gz,zip}` archives containing the binaries. Since this happens to conflict with the tarball used for codesigning, the codesigning tarball was renamed to `-codesigning.tar.gz`. Both MacOS and Windows codesigners will need to adjust their workflows to account for the new name.
Fixes#15774 and #29749
ACKs for top commit:
Sjors:
Tested ACK e181bda061
davidgumberg:
Tested ACK https://github.com/bitcoin/bitcoin/commit/e181bda061ca63021511be6e286fdf6a5818df49.
pinheadmz:
tested ACK e181bda061
Tree-SHA512: ce0e2bf38e1748cdaa0d13be6f61c3289cd09cfb7d071a68b0b13d2802b3936c9112eda6e4c7b29c535c0995d56b14871442589cdcea2e7707e35c1b278b9263
d1e6bb8bbf Merge bitcoin-core/minisketch#92: doc: rename from sipa to bitcoin-core
b7b7dd0ec8 doc: rename from sipa to bitcoin-core
git-subtree-dir: src/minisketch
git-subtree-split: d1e6bb8bbf8ef104b9dd002cab14a71b91061177
972b604dc4 doc: update location of minisketch repository (fanquake)
Pull request description:
This repository is now at https://github.com/bitcoin-core/minisketch.
ACKs for top commit:
hebasto:
ACK 972b604dc4.
theStack:
ACK 972b604dc4
Tree-SHA512: a164f97700e73b284429993f9639d1d4eab23cc09ded3104be392d5d259297c2906906a565ffa8848a495e8f35cbbe18ba4155fe1d16cda0406ac3c75f9d9a62
c718bffc36 build: don't use ccache with MSVC (fanquake)
Pull request description:
Set `WITH_CCACHE` to `OFF` for MSVC, so it doesn't show as `ON` in the configure summary.
Fixes#31771.
ACKs for top commit:
hebasto:
ACK c718bffc36, I have reviewed the code and it looks OK.
Tree-SHA512: cf20b36a496a446051d0586c55d05a9133dce26a958e6b39b510094c56014275b866806c88be40ffa918b4280970c082b981b4e52e4bfdb8cec1556f666898fc
ecf54a32ed cmake: Add support for builtin `codegen` target (Hennadii Stepanov)
a8c78a0574 cmake: Revamp handling of data files (Hennadii Stepanov)
Pull request description:
This PR leverages the approach from the https://github.com/chaincodelabs/libmultiprocess project and introduces a new functions `target_json_data_sources()` and `target_raw_data_sources()`, which minimize the amount of code required to assign to assign a `*.json` or `*.raw` data file to the `test_bitcoin`, `bench_bitcoin` or `unitester` targets.
As requested in https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2654622689, the `codegen` build target is now supported, if available:
```
$ cmake --version
cmake version 3.31.5
CMake suite maintained and supported by Kitware (kitware.com/cmake).
$ cmake -G "Ninja" -B build
$ cmake --build build --target codegen
```
ACKs for top commit:
fjahr:
re-ACK ecf54a32ed
Sjors:
re-tACK ecf54a32ed
theuni:
ACK ecf54a32ed
Tree-SHA512: bab92df6b81c47d9d97ba8db37470a6d7aa435d5578afe40df7154885eda55afc59f0bf20dc9db3b2fd88ceb9a0319b9678f9e9af01e7afd4851ec3a79f3f402
75d5d235a6 doc: Update translation generation instructions (pablomartin4btc)
Pull request description:
This is a follow-up of #31731.
Technically this change [fixes](https://github.com/bitcoin/bitcoin/pull/31731#discussion_r1928888001) the preset configuration execution failure as it needs multiprocess to be enabled, so we disable it using `-DWITH_MULTIPROCESS=OFF`.
This code will need to be updated by removing `-DWITH_MULTIPROCESS=OFF` in https://github.com/bitcoin/bitcoin/pull/31741.
ACKs for top commit:
ryanofsky:
Code review ACK 75d5d235a6. Looks good as a temporary fix and I think after #31741 we should be able to drop the extra argument.
Tree-SHA512: 793e095aa277ab0ea864b49542cc6fe458f9a16123e244f92a77de84fe141837dc7bcc06f543539ad23aaa207c0ea49a47f3afb4493f13fcefb3af87ad7c7c4f
e747ed989e contrib: fix read metadata related comment (rkrux)
d3095ac35a contrib: update `dumptxoutset` command in utxo_to_sqlite doc (rkrux)
Pull request description:
I noticed couple discrepancies in the documentation and comments of `utxo_to_sqlite` tool while using it, this PR fixes them. More details in the commit messages.
ACKs for top commit:
theStack:
lgtm ACK e747ed989e
Tree-SHA512: c5dab25d97c50a203ba008acfc5c93baf20cf24261a5a5127bdacc41eeeeae1c9d7b129710c2997e9fd1536ec000186e7f6395efc0188e1111b47868b7d9745a
d82dc10415 ci: Fix filtering out Qt generated files from `compile_commands.json` (Hennadii Stepanov)
Pull request description:
This PR:
1. Adjusts the regex for Qt-generated files to match the CMake build directory structure.
2. Moves the filtering command to run before `clang-tidy`, ensuring that Qt-generated files are not needlessly processed.
Fixes https://github.com/bitcoin/bitcoin/issues/31801.
ACKs for top commit:
maflcko:
ACK d82dc10415🚂
Tree-SHA512: 12ff40ffb134c56d848976e73aef7a8f895b856992b37b09ce27a1dcf6adef2cf148ba7e7d32b52ce1866c9bbeb165f9b6e6106c6c23b774d00d61bf379e13c9
c73b59d47f fuzz: implement targets for PCP and NAT-PMP port mapping requests (Antoine Poinsot)
1695c8ab5b fuzz: in FuzzedSock::GetSockName(), return a random-length name (Antoine Poinsot)
0d472c1953 fuzz: never return an uninitialized sockaddr in FuzzedSock::GetSockName (Antoine Poinsot)
39b7e2b590 fuzz: add steady clock mocking to FuzzedSock (Antoine Poinsot)
6fe1c35c05 pcp: make NAT-PMP error codes uint16_t (Antoine Poinsot)
01906ce912 pcp: make the ToString method const (Antoine Poinsot)
Pull request description:
Based on https://github.com/bitcoin/bitcoin/pull/31022, this introduces a fuzz target for `PCPRequestPortMap` and `NATPMPRequestPortMap`.
Like in #31022 we set `CreateSock` to return a `Sock` which mocks the responses from the server and uses a mocked steady clock for the `Wait`s. Except here we simply respond with fuzzer-provided data until the client stop sending requests. We also sometimes inject errors and connection failures based on fuzzer-provided data.
We reuse the existing `FuzzedSock`, so a preparatory commit is included that adds steady clock mocking to it. This may be useful for other harnesses as well.
ACKs for top commit:
laanwj:
re-ACK c73b59d47f
marcofleon:
ACK c73b59d47f
dergoegge:
utACK c73b59d47f
Tree-SHA512: 24cd4d958a0999946a0c3d164a242fc3f0a0b66770630252b881423ad0065d29fdaab765014d193b705d3eff397f201d51a88a3ca80c63fd3867745e6f21bb2b
d871d77825 test: Remove non-portable IPv6 test (Hennadii Stepanov)
Pull request description:
On Illumos-based systems, such as OpenIndiana and OmniOS, the assumption that "the default zone ID of 0 can be omitted for the default scope" is incorrect. As a result, `getaddrinfo("fe80::1%0", ...)` returns the `EAI_NONAME` error instead of resolving to "fe80::1".
See: https://www.illumos.org/man/3SOCKET/getaddrinfo.
This PR removes the problematic code introduced in https://github.com/bitcoin/bitcoin/pull/19951.
ACKs for top commit:
fanquake:
ACK d871d77825
Tree-SHA512: 2ef5c22f826d16661deb1d6d005cbda64179e8b83be43d3d6ac51caff02187cf224355c9da144ff110a6ae2cb68f0338ea9b62af8e0f9f1014a518cf9dad6ab5
2c4b229c90 cmake: Introduce `FUZZ_LIBS` (Hennadii Stepanov)
ea929c0848 scripted-diff: Rename CMake helper module (Hennadii Stepanov)
8d238c1dfd cmake: Delete `check_cxx_source_links*` macros (Hennadii Stepanov)
71bf8294a9 cmake: Convert `check_cxx_source_compiles_with_flags` to a function (Hennadii Stepanov)
88ee6800c9 cmake: Delete `check_cxx_source_links_with_flags` macro (Hennadii Stepanov)
09e8fd25b1 build: Don't override CMake's default try_compile target (Hennadii Stepanov)
Pull request description:
This was requested in https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2515287092.
From https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2511246212:
> (Almost?) every CMake check internally uses the [`try_compile()`](https://cmake.org/cmake/help/latest/command/try_compile.html) command, whose behaviour, in turn, depends on the [`CMAKE_TRY_COMPILE_TARGET_TYPE`](https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_TARGET_TYPE.html) variable:
>
> 1. The default value, `EXECUTABLE`, enables both compiler and linker checks.
>
> 2. The `STATIC_LIBRARY` value enables only compiler checks.
>
>
> To mimic Autotools' behaviour, we [disabled](https://github.com/bitcoin/bitcoin/blob/d3f42fa08fc385752881afa5740f40287cfb4d8b/cmake/module/CheckSourceCompilesAndLinks.cmake#L9-L10) linker checks by setting `CMAKE_TRY_COMPILE_TARGET_TYPE` to `STATIC_LIBRARY` globally (perhaps not the best design). This effectively separates the entire CMake script into regions where `CMAKE_TRY_COMPILE_TARGET_TYPE` is:
>
> * unset
>
> * set to `STATIC_LIBRARY`
>
> * set to `EXECUTABLE`
From https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2515287092:
> > This seems very fragile and unintuitive, and the fact that this could silently break at any point is not documented in any way. I don't think other bad design decisions should lead to us having to write even more boilerplate code to fix things that should "just work" (minus the upstream bugs).
>
> Agreed. I forgot that we set `CMAKE_TRY_COMPILE_TARGET_TYPE` globally. And even worse, it's buried in a module. If that upsets CMake internal tests, I think we should undo that.
This PR ensures that `CMAKE_TRY_COMPILE_TARGET_TYPE` is modified only within local scopes.
Additionally, the `FUZZ_LIBS` variable has been introduced to handle additional libraries required for linking, rather than link options, in certain build environment, such as OSS-Fuzz.
ACKs for top commit:
TheCharlatan:
Re-ACK 2c4b229c90
theuni:
utACK 2c4b229c90
Tree-SHA512: f72ffa8f50f216fc1a2f8027ba8ddfd4acd42b94ff6c1cb2138f2da51eb8f945660e97d3c247d7f3f7ec8dfebbccec3ab84347d6ae2e3f8a40f3d7aa8b14cde9
9e4a4b4832 cmake: Check `-Wno-*` compiler options for `leveldb` target (Hennadii Stepanov)
Pull request description:
Otherwise, https://cirrus-ci.com/task/4830737755537408:
```
At global scope:
cc1plus: note: unrecognized command-line option ‘-Wno-conditional-uninitialized’ may have been intended to silence earlier diagnostics
```
ACKs for top commit:
TheCharlatan:
ACK 9e4a4b4832
Tree-SHA512: 05553c80399180e01d45c3f02074ca0ce620011b29b03bef5433b87c9d88fd281fb6bf0203fc6fff590f3780c182a3fab8307002536b6350e03748420c346602
c4c5cf1748 cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree (Hennadii Stepanov)
eb540a2629 cmake: Remove `core_sanitizer_{cxx,linker}_flags` helper variables (Hennadii Stepanov)
Pull request description:
On the master branch @ 70e20ea024, the `APPEND_CPPFLAGS`, `APPEND_CFLAGS` and `APPEND_LDFLAGS` are not correctly applied when building C code in the `secp256k1` subtree, as intended.
This behaviour occurs due to two issues:
1. The command here: https://github.com/bitcoin/bitcoin/blob/70e20ea024ce4f39abc4022e1ba19d5a6db2a207/src/CMakeLists.txt#L77
does not affect the code in `add_subdirectory(secp256k1)` above it.
2. `APPEND_LDFLAGS` is not passed to the subtree's build system at all.
This PR fixes both issues.
Additionally, the helper variables `core_sanitizer_cxx_flags` and `core_sanitizer_linker_flags` have been removed.
ACKs for top commit:
theuni:
utACK c4c5cf1748.
TheCharlatan:
ACK c4c5cf1748
Tree-SHA512: 707acfa623f0436e34e9e6ba8ce2979e0fde5e196e2242fd1cde4c50f433938549781193d8a06419a0866bbe6d69d76f8383d973afbd87d944407963b318c5c9
ff4ddd3d2e Revert "cmake: Ensure generated sources are up to date for `translate` target" (Hennadii Stepanov)
03b3166aac cmake: Exclude generated sources from translation (Hennadii Stepanov)
Pull request description:
This PR fixes an error encountered when building the `translate` target:
```
$ gmake -j $(nproc) -C depends MULTIPROCESS=1
$ cmake -G "Unix Makefiles" --preset dev-mode --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DWITH_USDT=OFF
$ cmake --build build_dev_mode -t translate
gmake[3]: *** No rule to make target 'src/test/ipc_test.capnp.c++', needed by 'src/qt/CMakeFiles/translate'. Stop.
gmake[2]: *** [CMakeFiles/Makefile2:1646: src/qt/CMakeFiles/translate.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1653: src/qt/CMakeFiles/translate.dir/rule] Error 2
gmake: *** [Makefile:699: translate] Error 2
```
The previous [attempt](https://github.com/bitcoin/bitcoin/commit/864386a7444fb5cf16613956ce8bf335f51b67d5) to address this issue worked only with Ninja generators and has been reverted.
Essentially, this PR modifies the `translate` target so that it ignores generated sources rather than attempting to update them.
At present, multiprocess-specific sources do not contain any translatable strings. Nonetheless, it is prudent to maintain a general approach.
ACKs for top commit:
TheCharlatan:
ACK ff4ddd3d2e
pablomartin4btc:
tACK ff4ddd3d2e
Tree-SHA512: 6471498a33b145e073f76bd007591b0449e5d520f141c3e3afeca02a09c160fd0f572ec7172dd84703cdc2a1175ad8f3c91e8b0bf705d671338d760786765f56
113a7a363f build: remove ENABLE_HARDENING cond from check-security (fanquake)
Pull request description:
This check is only used in release builds, where hardening should always be enabled. I can't think of a reason we'd want to silently skip these checks if hardening was inadvertently disabled.
ACKs for top commit:
maflcko:
lgtm ACK 113a7a363f
TheCharlatan:
ACK 113a7a363f
hebasto:
ACK 113a7a363f.
Tree-SHA512: 46217e8ab238e23374d758b12e5b6bdc22353d8bf272aa0d2260cdea023b5b80aba972dccaa0a4fb8da21c8c665991848f7fd79966d20ac2489d499c68d95639
9cf746d663 cmake: add optional source files to crc32c directly (Daniel Pfeifer)
9c7823c5b5 cmake: add optional source files to bitcoin_crypto directly (Daniel Pfeifer)
Pull request description:
Avoid having many static libraries by adding the optional sources to the target `bitcoin_crypto` directly.
Set the necessary compile options at the source file level, rather than the target level.
fixes: #31697
ACKs for top commit:
s373nZ:
ACK 9cf746d663
hebasto:
re-ACK 9cf746d663.
TheCharlatan:
ACK 9cf746d663
Tree-SHA512: 04b468ccbd284d63fc83b382177bb8183b325369835c3b92e555e159955c73d71712a63a2e556f8da68a1232ac07d3845e11f1057c50666843db91db98fca979
fa3e409c9a contrib: Add deterministic-fuzz-coverage (MarcoFalke)
Pull request description:
The goal of this script is to detect and debug the remaining fuzz determinism and stability issues (https://github.com/bitcoin/bitcoin/issues/29018).
ACKs for top commit:
marcofleon:
Tested ACK fa3e409c9a
brunoerg:
tested ACK fa3e409c9a
Tree-SHA512: f336537d64188d6bc3c53880f4552a09cc498841c539cb7b4f14e622c9542531b970c1a6910981f7506e7bf659d2ce83471d58f5f51b0a411868f4c11eaf6b2a
This check is only used in release builds, where hardening should always
be enabled. I can't think of a reason we'd want to silently skip these
checks if hardening was inadvertently disabled.
81c174e318 cmake: Refer to the configure log instead of printing PIE test error (Hennadii Stepanov)
65a0920ca6 cmake: Add `CheckLinkerSupportsPIE` module (Hennadii Stepanov)
Pull request description:
This new module is a wrapper around CMake's `CheckPIESupported` module that fixes an upstream bug.
See: https://gitlab.kitware.com/cmake/cmake/-/issues/26463.
Fixes https://github.com/bitcoin/bitcoin/issues/30771.
ACKs for top commit:
theuni:
utACK 81c174e318.
vasild:
ACK 81c174e318
Tree-SHA512: 77d7022238551a4e69c59d1fe6b78975bb552cbbed5339459853d7ebf0086813036081f464fed230be330b3bd7d6cf8590b536b064028d2f786d6ae40f342f95
9b033bebb1 cmake: rename Kernel component to bitcoinkernel for consistency (Cory Fields)
2e0c92558e cmake: add and use install_binary_component (Cory Fields)
0264c5d86c cmake: use per-target components for bitcoin-qt and bitcoin-gui (Cory Fields)
fb0546b1c5 ci: don't try to install for a fuzz build (Cory Fields)
Pull request description:
This makes it possible to build/install only the desired binaries regardless of the configuration.
For consistency, the component names match the binary names. `Kernel` and `GUI` have been renamed.
Additionally it fixes#31762 by installing only the manpages for the configured targets (and includes them in the component installs for each).
Also fixes#31745.
Alternative to #31765 which is (imo) more correct/thorough.
Can be tested using (for ex):
```bash
$ cmake -B build
$ cmake --build build -t bitcoind -t bitcoin-cli
$ cmake --install build --component bitcoind
$ cmake --install build --component bitcoin-cli
```
ACKs for top commit:
hebasto:
ACK 9b033bebb1.
TheCharlatan:
Re-ACK 9b033bebb1
stickies-v:
re-ACK 9b033bebb1
Tree-SHA512: fd4818e76f190dbeafbf0c246b466f829771902c9d6d7111ed917093b811c8a5536a4a45e20708f73e7f581d6cb77c8e61cfa69e065788dcf0886792f553a355
dead908654 cmake: Improve compatibility with Python version managers (Hennadii Stepanov)
Pull request description:
This PR resolves the issue [highlighted](https://github.com/bitcoin/bitcoin/pull/31411#issuecomment-2516745547) in https://github.com/bitcoin/bitcoin/pull/31411:
> Here's another case where CMake just picks some other Python...
The fix leverages two [hints](https://cmake.org/cmake/help/latest/module/FindPython3.html#hints) for the CMake `FindPython3` module:
1. `Python3_FIND_FRAMEWORK` is set to `LAST`. This ensures that Unix-style package components are preferred over frameworks on macOS. As a side effect, the `FindPython3` module reports a shim or symlink (e.g., from `pyenv`) rather than the underlying framework's binary. The module's output aligns with the result of the `which` command.
2. `Python3_FIND_UNVERSIONED_NAMES` is set to `FIRST`. This supports scenarios where tools like `pyenv`—which use shims—have multiple Python versions installed.
Here are examples of output on my macOS 15.1.1 (Intel) with installed Homebrew's [Python 3.13.0](https://formulae.brew.sh/formula/python@3.13):
- without any Python version manager:
```
% which -a python3
/usr/local/bin/python3
/usr/bin/python3
% cmake -B build
<snip>
-- Found Python3: /usr/local/bin/python3 (found suitable version "3.13.0", minimum required is "3.10") found components: Interpreter
<snip>
```
- using `pyenv`:
```
% pyenv versions
system
* 3.10.14 (set by /Users/hebasto/dev/bitcoin/.python-version)
3.12.8
3.13.1
% which -a python3
/Users/hebasto/.pyenv/shims/python3
/usr/local/bin/python3
/usr/bin/python3
% cmake -B build
<snip>
-- Found Python3: /Users/hebasto/.pyenv/shims/python3 (found suitable version "3.10.14", minimum required is "3.10") found components: Interpreter
<snip>
```
Both variables, `Python3_FIND_FRAMEWORK` and `Python3_FIND_UNVERSIONED_NAMES`, can still be overridden by the user via the command line if needed.
ACKs for top commit:
theuni:
No opinion on the python selection changes themselves, but code-review ACK dead908654
willcl-ark:
ACK dead908654
Tree-SHA512: 69f8541223e5b6c35c892b4ba2a2dcfc24b41a10cf20accc75d3008b16434db8a9240c99c886c3a4566ba24269c5b0e0d856357891811f0a77b39f4afbee3634
70398ae05b mapport: make ProcessPCP void (Antoine Poinsot)
9e6cba2988 mapport: remove unnecessary 'g_mapport_enabled' (Antoine Poinsot)
8fb45fcda0 mapport: remove unnecessary 'g_mapport_current' variable (Antoine Poinsot)
1b223cb19b mapport: merge DispatchMapPort into StartMapPort (Antoine Poinsot)
9bd936fa34 mapport: drop unnecessary function (Antoine Poinsot)
2a6536ceda mapport: rename 'use_pcp' to 'enable' (Antoine Poinsot)
c4e82b854c mapport: make 'enabled' and 'current' bool (Antoine Poinsot)
Pull request description:
Followup to #31130, this does a couple cleanups to `src/mapport.*` to clarify the logic now that there is a single protocol option for port mapping.
ACKs for top commit:
laanwj:
Code review ACK 70398ae05b
TheCharlatan:
ACK 70398ae05b
Tree-SHA512: d9a3ab4fcd59a7cf4872415c40cc7ac3a98dfc5aa25e195d4df880bb588bac286c30c3471e9d9499de379a75f45dcd0a82019eba3cb9f342004ae1482d0ba075
2434aeab62 depends: avoid an unset CMAKE_OBJDUMP (fanquake)
Pull request description:
Similar to #31840, currently our Linux toolchain file contains:
```bash
set(CMAKE_AR "aarch64-linux-gnu-ar")
set(CMAKE_RANLIB "aarch64-linux-gnu-ranlib")
set(CMAKE_STRIP "aarch64-linux-gnu-strip")
set(CMAKE_OBJCOPY "aarch64-linux-gnu-objcopy")
set(CMAKE_OBJDUMP "")
```
`objdump` is currently only used for the macOS cross build, where it's `llvm-objdump`, but we should be consistent in producing a toolchain file that points to actual tools, rather than leaving variables unset.
ACKs for top commit:
hebasto:
ACK 2434aeab62.
theuni:
utACK 2434aeab62
Tree-SHA512: 65f6b7b9cae79e9c0784c108709139125e52d8f2818afbea5f719bc1b6dc338b870abbdfcb174ae541c0027a7ac07cb56012735b7a37b58b9a6e55a48c0257cf
0a02e7fdea test: deduplicates p2p_tx_download constants (Sergi Delgado Segura)
Pull request description:
Some of the networking constants defined in p2p_tx_download.py are more generally defined in p2p.py
ACKs for top commit:
i-am-yuvi:
re-ACK 0a02e7fdea
maflcko:
review ACK 0a02e7fdea🔖
danielabrozzoni:
re-ACK 0a02e7fdea
tdb3:
re ACK 0a02e7fdea
Tree-SHA512: 05fc114a32b6b42a7c57563a38f1a8921e0bb224c4b124ae9d395c3a1105ae6e9cdfc62f603f4f2dee55cef5f6a6ed400d328740ad84fbd3093c5e0f3fb2982a
Similar to #31840, currently our Linux toolchain file contains:
```bash
set(CMAKE_AR "aarch64-linux-gnu-ar")
set(CMAKE_RANLIB "aarch64-linux-gnu-ranlib")
set(CMAKE_STRIP "aarch64-linux-gnu-strip")
set(CMAKE_OBJCOPY "aarch64-linux-gnu-objcopy")
set(CMAKE_OBJDUMP "")
```
`objdump` is currently only used for the macOS cross build, where it's
`llvm-objdump`, but we should be consistent in producing a toolchain
file that points to actual tools, rather than leaving variables unset.
e3c0152769 cmake: Copy `cov_tool_wrapper.sh.in` to the build tree (Hennadii Stepanov)
Pull request description:
This PR ensures that `cov_tool_wrapper.sh.in` is available when invoking the `Coverage.cmake` script from any directory.
Here is an example of usage on Ubuntu 24.10 with the default GCC 14.2.0:
```
$ cmake -B build -DCMAKE_BUILD_TYPE=Coverage -DCMAKE_CXX_FLAGS="-fprofile-update=atomic" -DCMAKE_C_FLAGS="-fprofile-update=atomic"
$ cmake --build build -j $(nproc)
$ cd ..
$ cmake -DJOBS=$(nproc) -DLCOV_OPTS="--ignore-errors inconsistent,inconsistent" -P bitcoin/build/Coverage.cmake
```
Fixes https://github.com/bitcoin/bitcoin/issues/31638.
ACKs for top commit:
theuni:
utACK e3c0152769
Tree-SHA512: ccfc8e893567f199d9b05ea3916cac06fca89c5892cc7492d5251c331c35408222fd918ed08017515e2dfca10970ae3f633b3917bfb7037db539559e71d7f711
3edaf0b428 depends: add missing Darwin objcopy (fanquake)
Pull request description:
Our CMake toolchain for a Darwin cross build currently contains:
```bash
set(CMAKE_AR "/usr/bin/llvm-ar")
set(CMAKE_RANLIB "/usr/bin/llvm-ranlib")
set(CMAKE_STRIP "/usr/bin/llvm-strip")
set(CMAKE_OBJCOPY "arm64-apple-darwin-objcopy")
set(CMAKE_OBJDUMP "/usr/bin/llvm-objdump")
```
`objcopy` isn't currently used for the Darwin build (only for Linux and splitting the debug symbols), but we shouldn't be producing a toolchain file that refers to nonexistent tools.
ACKs for top commit:
laanwj:
Code review ACK 3edaf0b428
theuni:
utACK 3edaf0b428
Tree-SHA512: b74deb9f3f053c37d03505e698419d4a14131131f12a042dab698a81f2ad76b71fd55c1d1afd5f5822cc50fdaad5acdab15a8b20626c56f705179add1165449f
f89f16846e depends: Fix compiling `libevent` package on NetBSD (Hennadii Stepanov)
Pull request description:
Libevent [introduced](https://github.com/libevent/libevent/pull/909) the [`typeof`](https://gcc.gnu.org/onlinedocs/gcc/Typeof.html) C language extension in the NetBSD-specific code, which was pulled into our depends in https://github.com/bitcoin/bitcoin/pull/21991.
However, GCC [states](https://gcc.gnu.org/onlinedocs/gcc/Alternate-Keywords.html):
> the various `-std` options disable certain keywords.
Due to our use of https://github.com/bitcoin/bitcoin/blob/b042c4f0538c6f9cdf8efbcef552796851e38a85/depends/hosts/netbsd.mk#L1
the `typeof` keyword is disabled, resulting in a compilation error:
```
$ gmake -C depends libevent CC=/usr/pkg/gcc14/bin/gcc CXX=/usr/pkg/gcc14/bin/g++
<snip>
[ 37%] Building C object CMakeFiles/event_core_static.dir/kqueue.c.o
/home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/libevent/2.1.12-stable-ca6b96ec97c/kqueue.c: In function 'kq_setup_kevent':
/home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/libevent/2.1.12-stable-ca6b96ec97c/kqueue.c:56:27: error: implicit declaration of function 'typeof' [-Wimplicit-function-declaration]
56 | #define INT_TO_UDATA(x) ((typeof(((struct kevent *)0)->udata))(intptr_t)(x))
| ^~~~~~
/home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/libevent/2.1.12-stable-ca6b96ec97c/kqueue.c:190:30: note: in expansion of macro 'INT_TO_UDATA'
190 | out->udata = INT_TO_UDATA(ADD_UDATA);
| ^~~~~~~~~~~~
/home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/libevent/2.1.12-stable-ca6b96ec97c/kqueue.c:56:64: error: expected expression before 'intptr_t'
56 | #define INT_TO_UDATA(x) ((typeof(((struct kevent *)0)->udata))(intptr_t)(x))
| ^~~~~~~~
/home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/libevent/2.1.12-stable-ca6b96ec97c/kqueue.c:190:30: note: in expansion of macro 'INT_TO_UDATA'
190 | out->udata = INT_TO_UDATA(ADD_UDATA);
| ^~~~~~~~~~~~
/home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/libevent/2.1.12-stable-ca6b96ec97c/kqueue.c:56:27: error: called object is not a function or function pointer
56 | #define INT_TO_UDATA(x) ((typeof(((struct kevent *)0)->udata))(intptr_t)(x))
| ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/hebasto/dev/bitcoin/depends/work/build/x86_64-unknown-netbsd10.0/libevent/2.1.12-stable-ca6b96ec97c/kqueue.c:190:30: note: in expansion of macro 'INT_TO_UDATA'
190 | out->udata = INT_TO_UDATA(ADD_UDATA);
| ^~~~~~~~~~~~
gmake[3]: *** [CMakeFiles/event_core_static.dir/build.make:328: CMakeFiles/event_core_static.dir/kqueue.c.o] Error 1
<snip>
```
This PR resolves this issue by following GCC's [recommendation](https://gcc.gnu.org/onlinedocs/gcc/Typeof.html):
> write `__typeof__` instead of `typeof`.
ACKs for top commit:
fanquake:
ACK f89f16846e
Tree-SHA512: c0d2e535408db120535781f8518c616b0f5a39b1c6babb2a74e8e0565348aaf00b0f5a93cac0af7cf6d6bf028d5d58763fe71b3969ed9c7059fa7c3dca9d084c
12fa9511b5 build: simplify dependency graph (Cory Fields)
c4e498300c build: avoid unnecessary dependencies on generated headers (Cory Fields)
Pull request description:
These changes speed up my build (default config/options/targets) by roughly 10%. I suspect the difference may be more significant in other build configs.
Before:
> $ time cmake --build build -j24
> real3m26.932s
After:
> $ time cmake --build build -j24
> real3m7.556s
Generally they allow for jobservers (either `make -jX` or `ninja`) to be better utilized. This can be verified using `top` while building and looking at the number of compiles running at any given time before/after these changes. Before, it's easy to observe periods of stalling when only one or two compiles are happening. After these changes, the compiler process count should mostly match the number of jobs given (`-jX`) until it falls off at the end.
---
The first commit sets [DEPENDS_EXPLICIT_ONLY](https://cmake.org/cmake/help/latest/command/add_custom_command.html#command:add_custom_command) for commands which generate our test header files. Without this option, `test_bitcoin`'s generated headers won't be built until all of its other dependencies have been built. This introduces a significant stall in the build, though currently only Ninja benefits from this being set, and only CMake >= 3.27 understands it.
Example from a generated `build.ninja`:
Before:
> \# Custom command for src/test/data/base58_encode_decode.json.h
>
> build src/test/data/base58_encode_decode.json.h | ${cmake_ninja_workdir}src/test/data/base58_encode_decode.json.h: CUSTOM_COMMAND /home/cory/dev/bitcoin/src/test/data/base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake || libcrc32c.a libcrc32c_sse42.a libleveldb.a libminisketch.a minisketch_clmul src/bitcoin_clientversion src/crypto/libbitcoin_crypto.a src/crypto/libbitcoin_crypto_avx2.a src/crypto/libbitcoin_crypto_sse41.a src/crypto/libbitcoin_crypto_x86_shani.a src/generate_build_info src/libbitcoin_cli.a src/libbitcoin_common.a src/libbitcoin_consensus.a src/libbitcoin_node.a src/secp256k1/src/libsecp256k1.a src/secp256k1/src/secp256k1_precomputed src/test/util/libtest_util.a src/univalue/libunivalue.a src/util/libbitcoin_util.a src/wallet/libbitcoin_wallet.a src/zmq/libbitcoin_zmq.a
After:
> \# Custom command for src/test/data/base58_encode_decode.json.h
>
> build src/test/data/base58_encode_decode.json.h | ${cmake_ninja_workdir}src/test/data/base58_encode_decode.json.h: CUSTOM_COMMAND /home/cory/dev/bitcoin/src/test/data/base58_encode_decode.json /home/cory/dev/bitcoin/cmake/script/GenerateHeaderFromJson.cmake
---
The second commit is more significant. It sets [CMAKE_OPTIMIZE_DEPENDENCIES](https://cmake.org/cmake/help/latest/prop_tgt/OPTIMIZE_DEPENDENCIES.html) globally, which allows the objects of static libs to be built in parallel when one lib depends on the other. This can be set as a per-lib property, ~but I don't see any need for that as we don't currently have any edge-cases where this wouldn't be ok. If those should arise, we could always disable on a per-lib basis~.
Edit: turns out this triggers an [upstream bug](https://gitlab.kitware.com/cmake/cmake/-/issues/24058), which I guess can be considered an edge-case until fixed in CMake. I've added 2 per-lib opt-outs as a result.
Example:
Before:
> \# Link the static library src/libbitcoin_cli.a
>
> build src/libbitcoin_cli.a: CXX_STATIC_LIBRARY_LINKER__bitcoin_cli_RelWithDebInfo src/CMakeFiles/bitcoin_cli.dir/compat/stdin.cpp.o src/CMakeFiles/bitcoin_cli.dir/rpc/client.cpp.o || src/univalue/libunivalue.a
After:
> \# Link the static library src/libbitcoin_cli.a
>
> build src/libbitcoin_cli.a: CXX_STATIC_LIBRARY_LINKER__bitcoin_cli_RelWithDebInfo src/CMakeFiles/bitcoin_cli.dir/compat/stdin.cpp.o src/CMakeFiles/bitcoin_cli.dir/rpc/client.cpp.o
>
ACKs for top commit:
l0rinc:
utACK 12fa9511b5
hebasto:
ACK 12fa9511b5.
Tree-SHA512: f85f507e70cdc06acd07542161d9f9b8edf9ba866f08c8ef17aaaed770fa11530a27521c4413456d863463a6e77d4d6983fa623a64e17bbd602c2bc70aacc112
fa952acdb6 ci: Skip read-write of default env vars (MarcoFalke)
Pull request description:
If they remain unset, they use the default anyway. Except for `USER`, but this seems unused anyway.
Can be checked via:
```
sh-5.2# touch /tmp/empty_env
sh-5.2# podman run --rm --env-file /tmp/empty_env 'ubuntu:24.04' env
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
container=podman
HOME=/root
HOSTNAME=19ece5c9e052
ACKs for top commit:
0xB10C:
ACK fa952acdb6
Prabhat1308:
utACK [fa952ac](https://github.com/bitcoin/bitcoin/pull/31678/commits/fa952acdb6ef1b07b7927202d1373fa7479fd5e4)
Tree-SHA512: fe0c173b23cfda3025306303a44ffe32ecc57c2e0e1a2376594696f9887ed22f5105da84e898e790041bf15a4aa42a365fba016710ad269d439dda691977be90
fa3a4eafa1 test: Remove stale gettime test (MarcoFalke)
Pull request description:
The `gettime` test is stale:
* It was added to sanity check the `time` implementation in the mingw toolchain to catch a 32-bit vs 64-bit mismatch in commit eaafa23cbd. However, since commit 0000a63689, `std::chrono::system_clock` is used.
* Even though `system_clock` may also return incorrect values, such an error should affect *all* `GetTime<>` calls (not only the second-precision ones). (I expect such an error to lead to a signed integer overflow in the normal nanosecond precision, so it should be caught by ubsan or by the `assert(ret > 0s)`. If not, the error should be apparent on startup in the debug log.)
So remove it for now. An alternative would be to extend the test to cover `time` again, and adjust the comment to say that the test should be fixed along with the block header timestamp. Since that timestamp can't grow beyond 2106 anyway, see the `_test_y2106` functional test.
ACKs for top commit:
l0rinc:
ACK fa3a4eafa1
laanwj:
ACK fa3a4eafa1
Tree-SHA512: fd485e74962b659ee23ba2952d284fa9d6cfb9d9844a5e70013c8ead495ed77f5b784d5ca3ba0b30c492a5d27b2e81f9e1e0dbc530af7da1494789ac5e055b99
b28917be36 depends: Make default `host` and `build` comparable (Hennadii Stepanov)
Pull request description:
To detect cross-compiling, the host and build platforms are compared. The `build` variable is always an output of `config.sub`, but the `host` is not. This can lead to false results. For example, on OpenBSD:
- host=amd64-unknown-openbsd7.5
- build=x86_64-unknown-openbsd7.5
This PR sets the default value of the `host` variable to the value of `build`, ensuring cross-compiling won't be triggered when the `HOST` variable is not set.
This PR fixes needless triggering of cross-compiling for CMake-built packages in depends on OpenBSD due to this code:https://github.com/bitcoin/bitcoin/blob/eb85cacd2969caaea682104f498f6b2e6cfb80f8/depends/funcs.mk#L193-L197
No changes in Guix build.
ACKs for top commit:
laanwj:
Concept and code review ACK b28917be36
theuni:
utACK b28917be36.
Tree-SHA512: 8c5835cb8b739355b71f7cb161b350ad8b038a00e6b1def36354ba228cea3dcb9883df3c9a8e79d7d0143241f6f054129fe90772b1b2579702db51237f9d66ff
56a9b847bb build: set build type and per-build-type flags as early as possible (Cory Fields)
f605f7a9c2 build: refactor: set debug definitions in main CMakeLists (Cory Fields)
Pull request description:
This ensures that most compiler tests are not run with the wrong build type's flags. The initial c++ checks are an exception to that because many internal CMake variables are unset until a language is selected, so it's problematic to change our build type before that.
The difference can be seen in `build/CMakeFiles/CMakeConfigureLog.yaml`. Before, `Debug` was used for many of the earlly checks. After this PR, it's only the first 2 checks.
ACKs for top commit:
hebasto:
ACK 56a9b847bb.
Tree-SHA512: 87947352d6d4fd08554515822cb13634ed3be33fcda2af817c22ef943b1d0856ceb39311ddc01b8221397528fdc09f630dc7e74fc92f5a4a073f09c4ae493596
76c090145e guix: remove test-security/symbol-check scripts (fanquake)
Pull request description:
These scripts are becoming more of nuisance, than a value-add; particularly since we've been building releases using Guix. Adding new (release bin) tests can be harder, because it requires constructing a failing test, which is becoming less easy, e.g trying to disable a feature or protection that has been built into the compiler/toolchain by default.
In the pre-Guix days, these were valuable to sanity-check the environment, because we were pulling that pre-built from Ubuntu, with little control. At this point, it's less clear what these scripts are (sanity) checking.
Note that these also weren't completely ported to CMake (#31698), see also #31715 which contains other fixes that would be needed for these test-tests, to accomodate future changes.
ACKs for top commit:
hebasto:
ACK 76c090145e.
theuni:
utACK 76c090145e
Tree-SHA512: 99b5e7c0645c6966a45b17f411b5bee61df23c64d8258cce0ad9cdea4c3af4d4db32ca5fd80d0df2a3a30ba873eb772cc0d5901c345ff7f0eea13fcb971443b4
Our CMake toolchain for a Darwin cross build currently contains:
```bash
set(CMAKE_AR "/usr/bin/llvm-ar")
set(CMAKE_RANLIB "/usr/bin/llvm-ranlib")
set(CMAKE_STRIP "/usr/bin/llvm-strip")
set(CMAKE_OBJCOPY "arm64-apple-darwin-objcopy")
set(CMAKE_OBJDUMP "/usr/bin/llvm-objdump")
```
`objcopy` isn't currently used for the Darwin build (only for Linux and
splitting the debug symbols), but we shouldn't be producing a toolchain
file that refers to nonexistent tools.
8fe552fe6e test: add missing sync to p2p_tx_download.py (Martin Zumsande)
Pull request description:
If the node hasn't processed the inv from the outbound peer before the mocktime bump, the peer won't be preferred after the other inv timeouts, failing the test . Therefore, add a sync, just like there is one after the `send_message` calls in the previous lines.
Fixes#31833
ACKs for top commit:
maflcko:
lgtm ACK 8fe552fe6e
instagibbs:
ACK 8fe552fe6e
Tree-SHA512: fda935d8a4081b5ecae96f5a73c04f4bb91feaeb09b5c159ffd45cf16668c4345ff268c57f383ba7c7ff544ee07b21f97aa28f257ade809c18b9310837795e7a
ea687d2029 doc: swap CPPFLAGS for APPEND_CPPFLAGS (fanquake)
Pull request description:
`APPEND_CPPFLAGS` will be understood by our CMake, whereas `CPPFLAGS` will not. Attempting what is currently documented will just give:
```bash
CMake Warning:
Ignoring extra path from command line:
"CPPFLAGS=-DDEBUG_LOCKCONTENTION"
```
ACKs for top commit:
fjahr:
ACK ea687d2029
hebasto:
ACK ea687d2029.
Tree-SHA512: b8d3359b77a813535a4fa715619b815cd88e5440950f7d4cd045514e6b45d3f1c7f62061315c8581d0a99c0aec38340d34008be05657d198d868b241d19b7828
fa5a02bcfa ci: Use clang-20 for sanitizer tasks (MarcoFalke)
Pull request description:
A new clang version generally comes with bugfixes, new (sanitizer) features, and deprecations.
Upgrade the sanitizer tasks to use the new version.
This was also suggested in https://github.com/bitcoin/bitcoin/pull/31691#issuecomment-2602517116
ACKs for top commit:
fanquake:
ACK fa5a02bcfa - tested 20 in some other infra, we just needed to fix the same deprecation warnings we'd seen, in cryptofuzz: https://github.com/fanquake/cryptofuzz/commit/09ca550c3ef7d6645869cb1ac014a4413d5388ba.
Tree-SHA512: 6114d790b5d7145eb5f019e02da6c2c833342707ad67fb9f9c09506001afbef0c9b9beee7e51321f17f12ea692509d6428e6072ad105dba51e4d54cd057621cd
These scripts are becoming more of nuisance, than a value-add;
particularly since we've been building releases using Guix. Adding new
(release bin) tests can be harder, because it requires constructing a
failing test, which is becoming less easy e.g trying to disable a
feature or protection that has been built into the compiler/toolchain by
default.
In the pre-Guix days, these were valuable to sanity-check the environment,
because we were pulling that pre-built from Ubuntu, with little control.
At this point, it's less clear what these scripts are (sanity) checking.
Note that these also weren't completely ported to CMake (#31698), see
also #31715 which contains other fixes that would be needed for these
test-tests, to accomodate future changes.
e107bf78f9 [fuzz] TxOrphanage::SanityCheck accounting (glozow)
22dccea553 [fuzz] txorphan byte accounting (glozow)
982ce10178 add orphanage byte accounting to TxDownloadManagerImpl::CheckIsEmpty() (glozow)
c289217c01 [txorphanage] track the total number of announcements (glozow)
e5ea7daee0 [txorphanage] add per-peer weight accounting (glozow)
672c69c688 [refactor] change per-peer workset to info map within orphanage (glozow)
59cd0f0e09 [txorphanage] account for weight of orphans (glozow)
Pull request description:
Part of orphan resolution project, see #27463.
Definitions:
- **Announcement** is a unique pair (wtxid, nodeid). We can have multiple announcers for the same orphan since #31397.
- **Size** is the weight of an orphan. I'm calling it "size" and "bytes" because I think we can refine it in the future to be memusage or be otherwise more representative of the orphan's actual cost on our memory. However, I am open to naming changes.
This is part 1/2 of a project to also add limits on orphan size and count. However, this PR **does not change behavior**, just adds internal counters/tracking and a fuzzer. I will also open a second PR that adds behavior changes, which requires updating a lot of our tests and careful thinking about DoS.
ACKs for top commit:
instagibbs:
reACK https://github.com/bitcoin/bitcoin/pull/31810/commits/e107bf78f9d722fcdeb5c1fba5a784dd7747e12f
marcofleon:
reACK e107bf78f9
sipa:
utACK e107bf78f9
Tree-SHA512: 855d725d5eb521d131e36dacc51990725e3ca7881beb13364d5ba72ab2202bbfd14ab83864b13b1b945a4ec5e17890458d0112270b891a41b1e27324a8545d72
faca7ac132 ci: Bump fuzz task timeout (MarcoFalke)
Pull request description:
The fuzz task seems to be the most CPU intense task (going through GB of data through all fuzz inputs for all fuzz targets).
Normally, the task takes 44 minutes (example https://cirrus-ci.com/task/5077976091459584), but under higher load, it may take longer (https://cirrus-ci.com/task/5966231095738368).
I tried to move it to GHA to see how it compares, but it will be even slower there: https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/13182526514/job/36796629409.
The CI machines were recently updated to increase the CI performance, so in theory they could be updated again, but this can take some time and seems like the wrong fix anyway, because it will just hide the problem:
Ideally fuzzing is fast and when evaluating a fuzz input takes more than 10 seconds, it feels more like a slow unit test loop. So ideally fuzz timeouts should be fixed (https://github.com/bitcoin/bitcoin/issues/31066, https://github.com/bitcoin/bitcoin/issues/30498, ...). However, this can also take time.
So temporarily bump the fuzz timeout for now.
ACKs for top commit:
dergoegge:
ACK faca7ac132
Tree-SHA512: cfb06d14712d94be6b28a17eee821dcc762453e8efbd9376200f8a0e784a55c2140e45ac48bee9b71ef6e85ae7345155dddc1239cbf0cd4bc02583848fe46308
e3622a9692 tracing: document that peer addrs can be >68 chars (0xb10c)
b19b526758 tracing: log_p2p_connections.bt example (0xb10c)
caa5486574 tracing: connection closed tracepoint (0xb10c)
b2ad6ede95 tracing: add misbehaving conn tracepoint (0xb10c)
68c1ef4f19 tracing: add inbound connection eviction tracepoint (0xb10c)
4d61d52f43 tracing: add outbound connection tracepoint (0xb10c)
85b2603eec tracing: add inbound connection tracepoint (0xb10c)
Pull request description:
This adds five new tracepoints with documentation and tests for network connections:
- established connections with `net:inbound_connection` and `net:outbound_connection`
- closed connections (both closed by us or by the peer) with `net:closed_connnection`
- inbound connections that we choose to evict with `net:evicted_inbound_connection`
- connections that are misbehaving and punished with `net:misbehaving_connection`
I've been using these tracepoints for a few months now to monitor connection lifetimes, re-connection frequency by IP and netgroup, misbehavior, peer discouragement, and eviction and more. Together with the two existing P2P message tracepoints they allow for a good overview of local P2P network activity. Also sort-of addresses https://github.com/bitcoin/bitcoin/pull/22006#discussion_r636775863.
I've been back and forth on which arguments to include. For example, `net:evicted_connection` could also include some of the eviction metrics like e.g. `last_block_time`, `min_ping_time`, ... but I've left them out for now. If wanted, this can be added here or in a follow-up. I've tried to minimize a potential performance impact by measuring executed instructions with `gdb` where possible (method described [here](https://github.com/bitcoin/bitcoin/pull/23724#issuecomment-996919963)). I don't think a few hundred extra instructions are too crucial, as connection opens/closes aren't too frequent (compared to e.g. P2P messages). Note: e.g. `CreateNodeFromAcceptedSocket()` usually executes between 80k and 90k instructions for each new inbound connection.
| tracepoint | instructions |
|----------------------------|--------------------------------------------------------|
| net:inbound_connection | 390 ins |
| net:outbound_connection | between 700 and 1000 ins |
| net:closed_connnection | 473 ins |
| net:evicted_inbound_connection | not measured; likely similar to net:closed_connnection |
| net:misbehaving_connection | not measured |
Also added a bpftrace (tested with v0.14.1) `log_p2p_connections.bt` example script that produces output similar to:
```
Attaching 6 probes...
Logging opened, closed, misbehaving, and evicted P2P connections
OUTBOUND conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, total_out=1
INBOUND conn from 127.0.0.1:45324: id=1, type=inbound, network=0, total_in=1
MISBEHAVING conn id=1, message='getdata message size = 50001'
CLOSED conn to 127.0.0.1:15287: id=0, type=block-relay-only, network=0, established=1231006505
EVICTED conn to 127.0.0.1:45324: id=1, type=inbound, network=0, established=1612312312
```
ACKs for top commit:
laanwj:
re-ACK e3622a9692
vasild:
ACK e3622a9692
sipa:
utACK e3622a9692
Tree-SHA512: 1032dcac6fe0ced981715606f82c2db47016407d3accb8f216c978f010da9bc20453e24a167dcc95287f4783b48562ffb90f645bf230990e3df1b9b9a6d4e5d0
723440c5b8 test framework, wallet: rename get_scriptPubKey method to get_output_script (Alfonso Roman Zubeldia)
fa0232a3e0 test: add validation for gettxout RPC response (Alfonso Roman Zubeldia)
Pull request description:
Added a new test in `test/functional/rpc_blockchain.py` to validate the gettxout RPC response. This new test ensures all response elements are verified, including `bestblock`, `confirmations`, `value`, `coinbase`, and `scriptPubKey` details.
Also renamed the method `get_scriptPubKey` from `test/functional/test_framework/wallet.py` to the modern name `get_output_script` as suggested by maflcko (https://github.com/bitcoin/bitcoin/pull/30226#discussion_r1925491846)
ACKs for top commit:
fjahr:
reACK 723440c5b8
maflcko:
lgtm ACK 723440c5b8
brunoerg:
code review ACK 723440c5b8
Tree-SHA512: 3384578909d2e7548cef302c5b8a9fed5b82dfc942892503ad4a05e73f5cceafad1eab3af9a27e54aef3db7631f8935298d6b882c70d2f02a9a75b8e3c209b6c
846a138728 func test: Expand tx download preference tests (Greg Sanders)
Pull request description:
1. Check that outbound nodes are treated the same as whitelisted connections for
the purposes of `getdata` delays
2. Add test case that demonstrates download retries are preferentially
given to outbound (preferred) connections
even when multiple announcements are
considered ready.
`NUM_INBOUND` is a magic number large enough that it should fail over 90% of the time
if the underlying outbound->preferred->PriorityComputer logic was broken. Bumping this
to 100 peers cost another 14 seconds locally for the sub-test, so I made it pretty small.
ACKs for top commit:
i-am-yuvi:
tACK 846a138728 good catch
maflcko:
ACK 846a138728🍕
marcofleon:
lgtm ACK 846a138728
Tree-SHA512: 337aa4dc33b5c0abeb4fe7e4cd5e389f7f53ae25dd991ba26615c16999872542391993020122fd255af4c7163f76c1d1feb2f2d6114f12a364c0360d4d52b8c3
d9c8aacce3 depends, refactor: Avoid hardcoding `host_prefix` in toolchain file (Hennadii Stepanov)
Pull request description:
This PR allows the entire `depends/<host_prefix>` directory to be relocatable.
Only `libevent` package configuration files are non-relocatable for the version `2.1.12-stable` we use now. However, this issue has been fixed upstream in https://github.com/libevent/libevent/commit/1f1593ff27bdf51c3e1c45b92cfb0ac931960298 and friends.
Fixes https://github.com/bitcoin/bitcoin/issues/31050.
ACKs for top commit:
theuni:
Neat. utACK d9c8aacce3
ryanofsky:
Code review ACK d9c8aacce3
Tree-SHA512: c4c340722e63fc1da36fba2b15f025a6e1d477da1991194d678f546f2f3ea9454e2f0ff054aae6ae6c332a0781a597c3ce63b9018b46b8c258033f0d19efbef3
faf8fc5487 lint: Call lint_commit_msg from test_runner (MarcoFalke)
fa99728b0c lint: Move commit range printing to test_runner (MarcoFalke)
fa673cf344 lint: Call lint_scripted_diff from test_runner (MarcoFalke)
Pull request description:
The lint `commit-script-check.sh` can not be called from the test_runner at all and must be called manually. Also, some checks require `COMMIT_RANGE` to be set.
Fix all issues by moving two lint checks into the test_runner. Also, the proper commit range is passed to the checks by the test_runner, so that the user no longer has to do it.
ACKs for top commit:
kevkevinpal:
reACK [faf8fc5](https://github.com/bitcoin/bitcoin/pull/31653/commits/faf8fc5487d409eeff7b7b260eabb6929a7b7a5f)
willcl-ark:
tACK faf8fc5487
Tree-SHA512: 78018adc618d997508c226c9eee0a4fada3899cdfd91587132ab1c0389aea69127bafc3a900e90e30aca2c6bae9dcd6e6188ef287e91413bc63ee66fb078b1af
fa8ade300f refactor: Avoid GCC false positive error (MarcoFalke)
fa40807fa8 ci: Enable DEBUG=1 for one GCC-12+ build to catch 117966 regressions (MarcoFalke)
Pull request description:
It is possible that someone accidentally removes the workaround in fa9e0489f5, or more likely that someone accidentally adds new code without the workaround.
Avoid this by adding a temporary CI check.
This can be tested by reverting the workaround and observing a failure.
ACKs for top commit:
hebasto:
ACK fa8ade300f, I've tested locally on Ubuntu 24.04.
Tree-SHA512: 7ee1538fd5304a5ab91ac8c7619a573548d7e0345592a1e9d38b3b73729e09e7c77a9ee703d64cf02a8218de3148376d7836e294abb939aa7533034ba36dfb6c
faf2f2c654 test: Avoid redundant stop and error spam on shutdown (MarcoFalke)
fae3bf6b87 test: Avoid redundant stop and error spam on startup failure (MarcoFalke)
fa0dc09b90 test: Remove --noshutdown flag (MarcoFalke)
fad441fba0 test: Treat leftover process as error (MarcoFalke)
Pull request description:
The `--noshutdown` flag is brittle, confusing, and redundant:
* Someone wanting to inspect the state after a test failure will likely also want to debug the state on the python side, so the option is redundant with `--pdbonfailure`. If there was a use case to replicate `--pdbonfailure` without starting pdb, a dedicated flag could be added for that use case.
* It is brittle to use the flag for a passing test, because it will disable checks in the test. For example, on shutdown LSan will perform a leak check, and the test framework will check that the node did not crash, and it will check that the node did not print errors to stderr.
Fix all issues by removing it.
Also, tidy up startup error messages to be less confusing as a result.
ACKs for top commit:
hodlinator:
re-ACK faf2f2c654
pablomartin4btc:
re tACK faf2f2c654
Tree-SHA512: 46d7ae59c7be88b93f1f9e0b6be21af0fc101e646512e2c5e725682cb18bfec8aa010e0ebe89ce9ffe239e5caac0da5f81cc97b79e738d26ca5fa31930e8e4e3
8996fef8ae test: p2p: check that INV messages not matching wtxidrelay are ignored (Sebastian Falbesoner)
e0b3336822 test: p2p: fix sending of manual INVs in tx download test (Sebastian Falbesoner)
Pull request description:
The `test_inv_block` sub-test in p2p_tx_download.py has a subtle bug: the manual msg_inv announcements from peers currently have no effect, since they don't match the wtxidrelay setting (=true by default for `P2PInterface` instances) and are hence ignored by the nodes (since 2d282e0c / PR #18044):
https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/src/net_processing.cpp#L3904-L3911
Though the test still passes on master, it does so without the intended scenario of asking an additional peer (triggering the GETDATA_TX_INTERVAL delay). Fix this by sending the INV message with MSG_WTX instead of MSG_TX. This increases the test run time by about one minute intentionally.
It might be good to avoid issues like this in the future, happy to add test framework improvements if someone has a concrete idea.
(Got into the topic of tx/wtx announcements via the discussion https://github.com/bitcoin/bitcoin/pull/31397#discussion_r1904121487)
ACKs for top commit:
maflcko:
ACK 8996fef8ae😸
danielabrozzoni:
ACK 8996fef8ae
mzumsande:
Code Review ACK 8996fef8ae
Tree-SHA512: 3da26f9539c89d64c3b0d0579d9af2a6a4577615eed192506e1fb4318421b235f99a6672a497dea3050fba85dad32678f37fd2cda9ecb70cbf52982db37982e8
fad83e759a doc: Fix incorrect send RPC docs (MarcoFalke)
Pull request description:
It would be good to have accurate RPC docs, so that humans and machines can read them and rely on them.
This fixes one issue.
ACKs for top commit:
fjahr:
utACK fad83e759a
rkrux:
tACK fad83e759a
luke-jr:
tACK fad83e759a
Tree-SHA512: 65d0cc18a62ef44833621464d74b743d24ffe2b853596dce2c4f423df0495142d50387c02ba1b54f5ca77d4ddb083d55116a8ac92698aa6558762d841664911e
5c3e4d8b29 doc: add a section about using MSan (Antoine Poinsot)
Pull request description:
Just a couple lines in a subsection of the sanitizers section mentioning that using the memory sanitizer is a bit more involve than other sanitizers, describing the steps and pointing to an example.
ACKs for top commit:
fanquake:
ACK 5c3e4d8b29
dergoegge:
ACK 5c3e4d8b29
Tree-SHA512: 4ff73c2dd0f25cb96148e54bd867b8d340bd0fbc9b9a736a705125039352eb1d40bd724f9f262a44d3dbd1bea8f03166cf30e571d882fec02ceb1dd399ef7422
4da7bfdcc9 test: add coverage for unknown address type for `createwalletdescriptor` (brunoerg)
Pull request description:
Calling `createwalletdescriptor` RPC with an unknown address type throws an error. This PR adds test coverage for it as done for other RPCs (`getnewaddress `, `getrawchangeaddress`, etc).
ACKs for top commit:
maflcko:
lgtm ACK 4da7bfdcc9
rkrux:
tACK 4da7bfdcc9
Tree-SHA512: 490bc3ffeb70b0f26db0a44d3950d7410fef35d4056487f2e82c081fb14ca277a18943c487235e0163a29f90fc741a262c29835beb9f41936affa4e73ddad25f
fa9aced800 test: Check that reindex with prune wipes blk files (MarcoFalke)
fa9593efc2 test: Use high-level python types (MarcoFalke)
Pull request description:
This adds missing test coverage for `CleanupBlockRevFiles`.
ACKs for top commit:
TheCharlatan:
Re-ACK fa9aced800
l0rinc:
ACK fa9aced800
tdb3:
re ACK fa9aced800
Tree-SHA512: b31ff8a896ce344437715e7fb7efdb8cd7e11470e8465d8972fddfdb58ffd78257786c4060e8596cc53b6278f8ac6a9b6eb05a06e9df58b8b240bdaa719a8e5b
d38ade7bc4 qa: Use `sys.executable` when invoking other Python scripts (Hennadii Stepanov)
Pull request description:
This PR fixes the `rpc_signer.py` and `wallet_signer.py` functional tests on systems where `python3` is not available in the `PATH`, causing the shebang `#!/usr/bin/env python3` to fail.
Here are logs on NetBSD 10.0:
- without this PR:
```
$ python3.12 ./build/test/functional/test_runner.py rpc_signer.py wallet_signer.py
Temporary test directory at /tmp/test_runner_₿_🏃_20241219_160538
Remaining jobs: [rpc_signer.py, wallet_signer.py --descriptors]
1/2 - rpc_signer.py failed, Duration: 1 s
stdout:
2024-12-19T16:05:40.012000Z TestFramework (INFO): PRNG seed is: 1833166631173850775
2024-12-19T16:05:40.012000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1
2024-12-19T16:05:40.754000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 160, in try_rpc
fun(*args, **kwds)
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: RunCommandParseJSON error: process(/home/hebasto/dev/bitcoin/test/functional/mocks/signer.py enumerate) returned 127: env: python3: No such file or directory
(-1)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
self.run_test()
File "/home/hebasto/dev/bitcoin/build/test/functional/rpc_signer.py", line 72, in run_test
assert_raises_rpc_error(-1, 'fingerprint not found',
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 151, in assert_raises_rpc_error
assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/util.py", line 166, in try_rpc
raise AssertionError(
AssertionError: Expected substring not found in error message:
substring: 'fingerprint not found'
error message: 'RunCommandParseJSON error: process(/home/hebasto/dev/bitcoin/test/functional/mocks/signer.py enumerate) returned 127: env: python3: No such file or directory
'.
2024-12-19T16:05:40.756000Z TestFramework (INFO): Stopping nodes
2024-12-19T16:05:40.873000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1
2024-12-19T16:05:40.873000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1/test_framework.log
2024-12-19T16:05:40.873000Z TestFramework (ERROR):
2024-12-19T16:05:40.873000Z TestFramework (ERROR): Hint: Call /home/hebasto/dev/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20241219_160538/rpc_signer_1' to consolidate all logs
2024-12-19T16:05:40.873000Z TestFramework (ERROR):
2024-12-19T16:05:40.873000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-12-19T16:05:40.873000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-12-19T16:05:40.873000Z TestFramework (ERROR):
stderr:
Remaining jobs: [wallet_signer.py --descriptors]
2/2 - wallet_signer.py --descriptors failed, Duration: 1 s
stdout:
2024-12-19T16:05:40.014000Z TestFramework (INFO): PRNG seed is: 7530764367977090686
2024-12-19T16:05:40.014000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0
2024-12-19T16:05:40.526000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
self.run_test()
File "/home/hebasto/dev/bitcoin/build/test/functional/wallet_signer.py", line 66, in run_test
self.test_valid_signer()
File "/home/hebasto/dev/bitcoin/build/test/functional/wallet_signer.py", line 83, in test_valid_signer
self.nodes[1].createwallet(wallet_name='hww', disable_private_keys=True, descriptors=True, external_signer=True)
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/test_node.py", line 935, in createwallet
return self.__getattr__('createwallet')(wallet_name, disable_private_keys, blank, passphrase, avoid_reuse, descriptors, load_on_startup, external_signer)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/hebasto/dev/bitcoin/test/functional/test_framework/authproxy.py", line 146, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: RunCommandParseJSON error: process(/home/hebasto/dev/bitcoin/test/functional/mocks/signer.py enumerate) returned 127: env: python3: No such file or directory
(-1)
2024-12-19T16:05:40.528000Z TestFramework (INFO): Stopping nodes
2024-12-19T16:05:40.645000Z TestFramework (WARNING): Not cleaning up dir /tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0
2024-12-19T16:05:40.646000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0/test_framework.log
2024-12-19T16:05:40.646000Z TestFramework (ERROR):
2024-12-19T16:05:40.646000Z TestFramework (ERROR): Hint: Call /home/hebasto/dev/bitcoin/test/functional/combine_logs.py '/tmp/test_runner_₿_🏃_20241219_160538/wallet_signer_0' to consolidate all logs
2024-12-19T16:05:40.646000Z TestFramework (ERROR):
2024-12-19T16:05:40.646000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2024-12-19T16:05:40.646000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2024-12-19T16:05:40.646000Z TestFramework (ERROR):
stderr:
TEST | STATUS | DURATION
rpc_signer.py | ✖ Failed | 1 s
wallet_signer.py --descriptors | ✖ Failed | 1 s
ALL | ✖ Failed | 2 s (accumulated)
Runtime: 1 s
```
- with this PR:
```
$ python3.12 ./build/test/functional/test_runner.py rpc_signer.py wallet_signer.py
Temporary test directory at /tmp/test_runner_₿_🏃_20241219_160011
Remaining jobs: [rpc_signer.py, wallet_signer.py --descriptors]
1/2 - rpc_signer.py passed, Duration: 2 s
Remaining jobs: [wallet_signer.py --descriptors]
2/2 - wallet_signer.py --descriptors passed, Duration: 3 s
TEST | STATUS | DURATION
rpc_signer.py | ✓ Passed | 2 s
wallet_signer.py --descriptors | ✓ Passed | 3 s
ALL | ✓ Passed | 5 s (accumulated)
Runtime: 3 s
```
ACKs for top commit:
maflcko:
lgtm ACK d38ade7bc4
stickies-v:
ACK d38ade7bc4 . I have a minor concern about `sys.executable` not being guaranteed to return a valid Python path, but this patch seems good enough as is so no blocker.
Tree-SHA512: 91fe0abc0b7e2b599c5562f8b225ba60f94c5bd6baa77d8df532155ef4d3ef6c6a862cee7f4a7f565ed4bb3251adcda813b4a4f79be1aa6a4ffdfda8b4e53415
733fa0b0a1 miner: never create a template which exploits the timewarp bug (Antoine Poinsot)
Pull request description:
This check was introduced in #30681 but only enabled for testnet4. To avoid potentially creating an invalid block template if a soft fork to fix the timewarp attack were to activate in the future, we should have this check on all networks. It also seems wise for our miner to not support it whether or not a soft fork activates to fix it at the consensus level.
ACKs for top commit:
Sjors:
ACK 733fa0b0a1
fjahr:
utACK 733fa0b0a1
TheCharlatan:
ACK 733fa0b0a1
Tree-SHA512: 9b3bc8b26a57f93425b17dda80bcfac4ecb750a3d26bc3eb8df619135634e369ac15982fac0c9770b1df207bd2e418ffe02a98f37968f024e55262d97715a4f5
c31166ac77 cmake: Fail if `Libmultiprocess` is missing when `WITH_MULTIPROCESS=ON` (Hennadii Stepanov)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/31708:
```
$ cmake -B build -DWITH_MULTIPROCESS=ON
-- The CXX compiler identification is GNU 13.3.0
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found SQLite3: /usr/include (found suitable version "3.45.1", minimum required is "3.7.17")
CMake Error at CMakeLists.txt:146 (find_package):
By not providing "FindLibmultiprocess.cmake" in CMAKE_MODULE_PATH this
project has asked CMake to find a package configuration file provided by
"Libmultiprocess", but CMake did not find one.
Could not find a package configuration file provided by "Libmultiprocess"
with any of the following names:
LibmultiprocessConfig.cmake
libmultiprocess-config.cmake
Add the installation prefix of "Libmultiprocess" to CMAKE_PREFIX_PATH or
set "Libmultiprocess_DIR" to a directory containing one of the above files.
If "Libmultiprocess" provides a separate development package or SDK, be
sure it has been installed.
-- Configuring incomplete, errors occurred!
```
ACKs for top commit:
vasild:
ACK c31166ac77
ryanofsky:
Code review ACK c31166ac77
TheCharlatan:
ACK c31166ac77
Tree-SHA512: 503e6d7ff253c9ae95b13ff0649af7db97c74a97c04ca6fe88130defae251b94bfe9f4466300d3fab16397c7c8346b392a80a7b80a2d6517464a4eabe3aa40db
e94c9d1712 [doc] Amend notes on benchmarking (dergoegge)
Pull request description:
This gives some more context on the motivation and larger picture of benchmarks.
ACKs for top commit:
l0rinc:
ACK e94c9d1712
instagibbs:
reACK e94c9d1712
darosior:
reACK e94c9d1712
brunoerg:
reACK e94c9d1712
Tree-SHA512: 2cbf51f283f2efc0938e7021ae48db51fe89caf9ef9780821e99fa745dff839e2d202ca956ce6cc48b8319db304069728e77883feefe486264eb1783a0610c93
66d21d0eb6 qa: check parsed multipath descriptors dont share references (Antoine Poinsot)
09a1875ad8 miniscript: Make NodeRef a unique_ptr (Ava Chow)
9ccb46f91a miniscript: Ensure there is no NodeRef copy constructor or assignment operator (Ava Chow)
6d11c9c60b descriptor: Add proper Clone function to miniscript::Node (Ava Chow)
Pull request description:
Multipath descriptors requires performing a deep copy, so a Clone function that does that is added to miniscript::Node instead of the current shallow copy.
Fixes#30864
ACKs for top commit:
darosior:
re-ACK 66d21d0eb6
hodlinator:
re-ACK 66d21d0eb6🚀
brunoerg:
reACK 66d21d0eb6
Tree-SHA512: bea017497ed3cc0b2da2df7e3ccae1fa4a324769b7da1065963da131235bd8bfdcdfe337a3fabbb3ab4d3822611211fca6a9772e18e2ee1cb3d853e831ff6f88
d44626a9c2 depends: Override default build type for `libevent` (Hennadii Stepanov)
Pull request description:
This PR fixes a regression for the `libevent` package introduced in https://github.com/bitcoin/bitcoin/pull/29835.
The `libevent` package defaults to the "Release" build type, which overrides our per-build-type optimization flags with `-O3`.
To prevent this behavior, set `CMAKE_BUILD_TYPE` to "None", consistent with how other packages are handled.
ACKs for top commit:
fanquake:
ACK d44626a9c2
Tree-SHA512: 77abd2e28ad8dda86eb0548d8e49ecf23bac08a2e07dc35c71db62539aa659d471c863d361534c3cf693f9945c1b4f12de7e04eef05d11f8cc5e86d6eff5242d
merge-script