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](fa952acdb6)
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:eb85cacd29/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
0f716f2889 qa: cover PROTOCOL_ERROR variant in PCP unit tests (Antoine Poinsot)
fc700bb47f test: Add tests for PCP and NATPMP implementations (laanwj)
caf9521033 net: Use mockable steady clock in PCP implementation (laanwj)
03648321ec util: Add mockable steady_clock (laanwj)
ab1d3ece02 net: Add optional length checking to CService::SetSockAddr (laanwj)
Pull request description:
Add a NodeSteadyClock, a steady_clock that can be mocked with millisecond precision. Use this in the PCP implementation.
Then add a mock for a simple scriptable UDP server,, which is used to test various code paths (including successful mappings, timeouts and errors) in the PCP and NATPMP implementations.
Includes "net: Add optional length checking to CService::SetSockAddr" from #31014 as a prerequisite.
ACKs for top commit:
darosior:
re-ACK 0f716f2889
i-am-yuvi:
Concept ACK 0f716f2889
achow101:
ACK 0f716f2889
Tree-SHA512: 6f91b24e6fe46a3fded7a13972efd77c98e6ef235f8898e4ae44068c5df32d1cdabb22cb66c351b338dc98cb2073b624e43607a28107f4999302bfbe7a138229
2ffea09820 build: disable bitcoin-node if daemon is not built (Sjors Provoost)
Pull request description:
When building for fuzzing with multiprocess enabled, we were still trying to build `bitcoin-node`. This PR fixes that, by applying a similar check as for `bitcoin-gui`.
Before:
```
cmake -B build -DBUILD_FOR_FUZZING=ON -DWITH_MULTIPROCESS=ON
...
Configure summary
=================
Executables:
bitcoind ............................ OFF
bitcoin-node (multiprocess) ......... ON
bitcoin-qt (GUI) .................... OFF
bitcoin-gui (GUI, multiprocess) ..... OFF
...
cmake --build build
...
[ 84%] Built target bitcoin-node
```
After:
```
bitcoin-node (multiprocess) ......... OFF
```
And no `bitcoin-node` target gets built (not to be confused with `bitcoin_node`).
ACKs for top commit:
hebasto:
ACK 2ffea09820.
ryanofsky:
Code review ACK 2ffea09820
laanwj:
Code review ACK 2ffea09820
Tree-SHA512: bdb0b62049f77929d5c084bf98a076e9933de91eb30853ed89edd23cc81b3d4aec4cd57c9a9e21cf1d6930885f8c408dda830db6884b4e326c7fb348f1fbab4c
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
9b7023d31a Fuzz HRP of bech32 as well (Lőrinc)
c1a5d5c100 Split out bech32 separator char to header (Lőrinc)
Pull request description:
Instead of the static "bc" human-readable part, it's now randomly generated based on https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki and the extra restrictions in the code:
> The human-readable part, which is intended to convey the type of data, or anything else that is relevant to the reader. This part MUST contain 1 to 83 US-ASCII characters, with each character having a value in the range [33-126]. HRP validity may be further restricted by specific applications.
Since `bech32::Encode` rejects uppercase letters, we're actually generating values in the `[33-126] - ['A'-'Z']` range.
Split out of https://github.com/bitcoin/bitcoin/pull/30596/files#r1706957219
ACKs for top commit:
sipa:
ACK 9b7023d31a
achow101:
ACK 9b7023d31a
marcofleon:
Code review ACK 9b7023d31a. The separation into two targets and the new `GenerateRandomHRP` seem fine to me.
brunoerg:
code review ACK 9b7023d31a
Tree-SHA512: 22a261b8e7b5516e98f4e7990811954454595438a49a10191ed4ca42b5c71c5054fcc73f2d94e23b498ea833c7f1d5adb225f537ef1a24d15b428259450cdf98
b2e9fdc00f test: expect that files may disappear from /proc/PID/fd/ (Vasil Dimov)
Pull request description:
`get_socket_inodes()` calls `os.listdir()` and then iterates on the results using `os.readlink()`. However a file may disappear from the directory after `os.listdir()` and before `os.readlink()` resulting in a `FileNotFoundError` exception.
It is expected that this may happen for `bitcoind` which is running and could open or close files or sockets at any time. Thus ignore the `FileNotFoundError` exception.
ACKs for top commit:
arejula27:
ACK [`b2e9fdc`](b2e9fdc00f)
sipa:
utACK b2e9fdc00f
achow101:
ACK b2e9fdc00f
theuni:
utACK b2e9fdc00f
hodlinator:
ACK b2e9fdc00f
Tree-SHA512: 8eb05393e4de4307a70af446c3fc7e8f7dc3f08bf9d68d74d02b0e4e900cfd4865249f297be31f1fd7b05ffea45eb855c5cfcd75704167950c1deb4f17109f33
With the exception of the first c++ checks, this ensures that compiler tests
are never run with the wrong build type's flags.
Co-Authored-By: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
This prevents intermittent failures - 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.
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
b448b01494 test: add a mocked Sock that allows inspecting what has been Send() to it (Vasil Dimov)
f1864148c4 test: put the generic parts from StaticContentsSock into a separate class (Vasil Dimov)
4b58d55878 test: move the implementation of StaticContentsSock to .cpp (Vasil Dimov)
Pull request description:
Put the generic parts from `StaticContentsSock` into a separate class `ZeroSock` so that they can be reused in other mocked `Sock` implementations.
Add a new `DynSock` whose `Recv()` and `Send()` methods can be controlled after the object is created. To achieve that, the caller/creator of `DynSock` provides to its constructor two pipes (FIFOs) - recv-pipe and send-pipe. Whatever data is written to recv-pipe is later received by `DynSock::Recv()` method and whatever data is written to the socket using `DynSock::Send()` can later be found in the send-pipe. For convenience there are also two methods to send and receive `CNetMessage`s.
---
This is used in https://github.com/bitcoin/bitcoin/pull/26812 (first two commits from that PR).
Extracting as a separate PR suggested here: https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1619152037.
ACKs for top commit:
Sjors:
re-ACK b448b01494
jonatack:
re-ACK b448b01494
pinheadmz:
ACK b448b01494
Tree-SHA512: 4a36f038192ec4ef63366cbe1a38ae70e7e015630c9f7c44926b756b20ab8c08138acae41801f23b30f6629c7059c1f81e001806e86584ff1bf1fa5b44d9caec
386eecff5f doc: add release notes (ismaelsadeeq)
3eaa0a3b66 miner: init: add `-blockreservedweight` startup option (ismaelsadeeq)
777434a2cd doc: rpc: improve `getmininginfo` help text (ismaelsadeeq)
c8acd4032d init: fail to start when `-blockmaxweight` exceeds `MAX_BLOCK_WEIGHT` (ismaelsadeeq)
5bb31633cc test: add `-blockmaxweight` startup option functional test (ismaelsadeeq)
2c7d90a6d6 miner: bugfix: fix duplicate weight reservation in block assembler (ismaelsadeeq)
Pull request description:
* This PR attempts to fix the duplicate coinbase weight reservation issue we currently have.
* Fixes #21950
We reserve 4000 weight units for coinbase transaction in `DEFAULT_BLOCK_MAX_WEIGHT`
7590e93bc7/src/policy/policy.h (L23)
And also reserve additional `4000` weight units in the default `BlockCreationOptions` struct.
7590e93bc7/src/node/types.h (L36-L40)
**Motivation**
- This issue was first noticed during a review here https://github.com/bitcoin/bitcoin/pull/11100#discussion_r136157411)
- It was later reported in issue #21950.
- I also came across the bug while writing a test for building the block template. I could not create a block template above `3,992,000` in the block assembler, and this was not documented anywhere. It took me a while to realize that we were reserving space for the coinbase transaction weight twice.
---
This PR fixes this by consolidating the reservation to be in a single location in the codebase.
This PR then adds a new startup option `-blockreservedweight` whose default is `8000` that can be used to lower or increase the block reserved weight for block header, txs count, coinbase tx.
ACKs for top commit:
Sjors:
ACK 386eecff5f
fjahr:
Code review ACK 386eecff5f
glozow:
utACK 386eecff5f, nonblocking nits. I do think the release notes should be clarified more
pinheadmz:
ACK 386eecff5f
Tree-SHA512: f27efa1da57947b7f4d42b9322b83d13afe73dd749dd9cac49360002824dd41c99a876a610554ac2d67bad7485020b9dcc423a8e6748fc79d6a10de6d4357d4c
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: 09ca550c3e.
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 e107bf78f9
marcofleon:
reACK e107bf78f9
sipa:
utACK e107bf78f9
Tree-SHA512: 855d725d5eb521d131e36dacc51990725e3ca7881beb13364d5ba72ab2202bbfd14ab83864b13b1b945a4ec5e17890458d0112270b891a41b1e27324a8545d72
f5b9a2f68c build: use CLIENT_NAME in libbitcoinkernel.pc.in (fanquake)
Pull request description:
Follows up from when the `pc.in` was added.
ACKs for top commit:
davidgumberg:
utACK f5b9a2f68c
stickies-v:
ACK f5b9a2f68c
theuni:
utACK f5b9a2f68c
hebasto:
ACK f5b9a2f68c.
Tree-SHA512: 7c32db919aa226f9894ed21baa3f794d1181d43d36ae56ba2d187e1a9bafd89feadc6209ab5b5a1b90d8a3de54fb910736397b1061ef48b232b59792ee250d55
2706c5b7c8 test: test_inv_block, use mocktime instead of waiting (Greg Sanders)
Pull request description:
Performance issue reported in https://github.com/bitcoin/bitcoin/pull/31437#issuecomment-2640221382
It seems that code as-is waits for wall-clock time to pass to synchronize mempools. Locally, sometimes the subtest takes a couple seconds, sometimes it takes an additional minute.
Just use mocktime?
ACKs for top commit:
sr-gi:
tACK [2706c5b](2706c5b7c8)
rishkwal:
tACK 2706c5b
Prabhat1308:
tACK [2706c5b](2706c5b7c8)
Tree-SHA512: 561fe3d67282c67e1ed7dd5eeb137964c083d498534ea5f749f3d782e73a3f47d23faee6cca39866eaba770fda7b7d60a9f740f16bdb451d6a5e9105417cb158
407062f2ac depends: Avoid using the `-ffile-prefix-map` compiler option (Hennadii Stepanov)
Pull request description:
This PR is similar to https://github.com/bitcoin/bitcoin/pull/31337 and applies analogous changes to all dependency packages.
The issue was [recently noticed](https://github.com/bitcoin/bitcoin/pull/31661#discussion_r1923896475) when `-ffile-prefix-map` was added to the `libevent` package, which is built in OSS-Fuzz.
This PR replaces `-ffile-prefix-map` in all packages for consistency.
Fixes https://github.com/bitcoin/bitcoin/issues/31770.
ACKs for top commit:
davidgumberg:
Tested ACK 407062f2ac.
theuni:
utACK 407062f2ac
Tree-SHA512: c501519c2397b7f11cdab13c0cd4b98a73b305817dba6ff61efc1c80c3cb44134bbd7f55eaecc1dab97f817ce44b28b6c81ccef74ea2d62c93ac43130be4efaf
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
No change for now, moving from map of NodeId->workset to
NodeId->PeerOrphanInfo struct that holds the workset.
In future commits, we will start tracking more things per-peer in the
orphanage.
2f27c91086 qt: Update the `src/qt/locale/bitcoin_en.xlf` translation source file (Hennadii Stepanov)
864386a744 cmake: Ensure generated sources are up to date for `translate` target (Hennadii Stepanov)
2b51dd384b Update Transifex slug for 29.x (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](864386a744/doc/release-process.md).
It is required to open Transifex translations for v29.0, as scheduled in https://github.com/bitcoin/bitcoin/issues/31029.
The previous similar PR: https://github.com/bitcoin/bitcoin/pull/30548.
**Notes for reviewers:**
1. This is the first release process conducted after migrating the build system to CMake. This revealed a bug, which is fixed in the second commit
2. To reproduce the diff in the third commit, follow these steps:
```
gmake -C depends -j $(nproc) MULTIPROCESS=1
cmake --preset dev-mode --toolchain depends/$(./depends/config.guess)/toolchain.cmake
cmake --build build_dev_mode --target translate
```
ACKs for top commit:
stickies-v:
ACK 2f27c91086
Tree-SHA512: 325ce2418f218b82cc3b0a6c727473963455680cdf6383a85768613ed9e485974b2e52bd5b2e7a7472ad8ebe40bccb2884764d7f9e83dc10a587cd7892e0028b
Some sources might be generated, and while they likely do not contain
any translatable strings, this change generalizes the approach to
include generated sources in the translation process as well.
1973a9e4f1 test: fixes p2p_ibd_txrelay wait time (Sergi Delgado Segura)
Pull request description:
`p2p_ibd_txrelay` expects no GETDATA to have been received by a peer after announcing a transaction. The reason is that the node is doing IBD, so transaction requests are not replied to. However, the way this is checked is wrong, and the check will pass even if the node **was not** in IBD.
This is due to the mocktime not being properly initialized, so the check is always performed earlier than it should, making it impossible for the request to be there.
This can be checked by modifying the test so the peer **is not doing IBD**, and checking how the test succeeds on that assert (even though it fails later on, given the nature of the test):
```diff
index 882f5b5c13..3a69ae5860 100755
--- a/test/functional/p2p_ibd_txrelay.py
+++ b/test/functional/p2p_ibd_txrelay.py
@@ -34,7 +34,7 @@ NORMAL_FEE_FILTER = Decimal(100) / COIN
class P2PIBDTxRelayTest(BitcoinTestFramework):
def set_test_params(self):
- self.setup_clean_chain = True
+ # self.setup_clean_chain = True
self.num_nodes = 2
self.extra_args = [
["-minrelaytxfee={}".format(NORMAL_FEE_FILTER)],
@@ -43,9 +43,11 @@ class P2PIBDTxRelayTest(BitcoinTestFramework):
def run_test(self):
self.log.info("Check that nodes set minfilter to MAX_MONEY while still in IBD")
- for node in self.nodes:
- assert node.getblockchaininfo()['initialblockdownload']
- self.wait_until(lambda: all(peer['minfeefilter'] == MAX_FEE_FILTER for peer in node.getpeerinfo()))
+ # for node in self.nodes:
+ # assert node.getblockchaininfo()['initialblockdownload']
+ # self.wait_until(lambda: all(peer['minfeefilter'] == MAX_FEE_FILTER for peer in node.getpeerinfo()))
```
ACKs for top commit:
i-am-yuvi:
ACK 1973a9e4f1
glozow:
ACK 1973a9e4f1
Tree-SHA512: c4b3afe9927c5480671ebf5c1f6ee5fc7e3aeefeb13c210fa83587a6c126e1a8e40ad8e46587537d0f4bf06a36bbf2310ca065d685d4d9286e5a446b8d5b2235
3f4b104b1b test: make sure we are on sync with a peer before checking if they have sent a message (Sergi Delgado Segura)
Pull request description:
p2p_orphan_handling checks whether a message has not been requested slightly too soon, making the check always succeed. This passes unnoticed since the expected result is for the message to not have been received, but it will make the test not catch a relevant change that should make it fail.
An easy way to check this is the case is to modify one of the test cases to force a request within the expected time, and check how the request is not seen. After the change, the test would crash as expected:
```diff
index 963d92485c..30ab5f2035 100755
--- a/test/functional/p2p_orphan_handling.py
+++ b/test/functional/p2p_orphan_handling.py
@@ -186,9 +185,12 @@ class OrphanHandlingTest(BitcoinTestFramework):
parent_inv = CInv(t=MSG_WTX, h=int(tx_parent_arrives["tx"].getwtxid(), 16))
assert_equal(len(peer_spy.get_invs()), 0)
peer_spy.assert_no_immediate_response(msg_getdata([parent_inv]))
+ txid = 0xdeadbeef
+ peer_spy.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=txid)]))
# Request would be scheduled with this delay because it is not a preferred relay peer.
self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY)
+ peer_spy.assert_never_requested(int(txid))
peer_spy.assert_never_requested(int(tx_parent_arrives["txid"], 16))
peer_spy.assert_never_requested(int(tx_parent_doesnt_arrive["txid"], 16))
# Request would be scheduled with this delay because it is by txid.
```
It is worth noting that this is not seen in the cases where the message is expected to be received, because in such cases `assert_never_requested` is always after a `wait_....` method, which is already waiting for the node to sync on their end.
ACKs for top commit:
i-am-yuvi:
ACK 3f4b104b1b
instagibbs:
ACK 3f4b104b1b
glozow:
ACK 3f4b104b1b
Tree-SHA512: 321a6605d630bed2217b6374e999dbb84da14138263dd8adf65fe3a6cd7981a50c873beced9cf05cb6d747a912e91017c58e7d4323d25449c87d83095ff4cba9
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
The `-ffile-prefix-map` compiler option implicitly enables
`-fprofile-prefix-map` in GCC or `-fcoverage-prefix-map` in Clang, which
can cause issues with coverage builds.
This change ensures that only the options necessary for build
reproducibility are applied.
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
7e0db87d4f test: added additional coverage to waitforblock and waitforblockheight rpc's (kevkevinpal)
Pull request description:
Similar to https://github.com/bitcoin/bitcoin/pull/31746
This adds test coverage to the `waitforblock` and `waitforblockheight` rpc's by adding a test to assert we get an rpc error if we include a negative timeout
ACKs for top commit:
Sjors:
utACK 7e0db87d4f
Prabhat1308:
ACK [7e0db87](7e0db87d4f)
brunoerg:
code review ACK 7e0db87d4f
BrandonOdiwuor:
Code Review ACK 7e0db87d4f
Tree-SHA512: c3b1b3a525e91e0092b783d73d2401126e3b8792a394be00374258f20cf3d619498e6625d3aad5e5ced29509c5eac828ee03c4ee66ba405b91e89be7e8b07311
