This commit creates new test utilities for future Taproot script
tests within script_tests.json. The key features of this commit are the
addition of three new tags: `#SCRIPT#`, `#CONTROLBLOCK#`, and
`#TAPROOTOUTPUT#`. These tags streamline the test creation process by
eliminating the need to manually generate these components outside the
test suite.
* `#SCRIPT#`: Parses Tapscript and outputs a byte string of opcodes.
* `#CONTROLBLOCK#`: Automatically generates the control block for a given
Taproot output.
* `#TAPROOTOUTPUT#`: Generates the final Taproot scriptPubKey.
589ed1a8eafe1daed379ebb383602c8f220aef19 wallet: migration, avoid loading wallet after failure when it wasn't loaded before (furszy)
Pull request description:
Fixes#31447.
During migration failure, only load wallet back into memory when the wallet was
loaded prior to migration. This fixes the case where BDB is not supported, which
implies that no legacy wallet can be loaded into memory due to the lack of db
writing functionality.
Link to error description https://github.com/bitcoin/bitcoin/issues/31447#issuecomment-2528757140.
This PR also improves migration backup related comments to better document the
current workflow.
ACKs for top commit:
achow101:
ACK 589ed1a8eafe1daed379ebb383602c8f220aef19
rkrux:
ACK 589ed1a8eafe1daed379ebb383602c8f220aef19
pablomartin4btc:
tACK 589ed1a8eafe1daed379ebb383602c8f220aef19
Tree-SHA512: c7a489d2b253c574ee0287b691ebe29fe8d026f659f68a3f6108eca8b4e1e420c67ca7803c6bd70c1e1440791833fabca3afbcf8fe8524c6c9fc08de95b618d0
1ea7e45a1f445d32a2b690d52befb2e63418653b test: raise explicit error if any of the needed release binaries is missing (Sebastian Falbesoner)
Pull request description:
If the `releases` directory exists, but still only a subset of the necessary previous release binaries are available, the test fails by throwing an exception (sometimes leading to follow-up exceptions like `AssertionError: [node 0] Error: no RPC connection`) and printing out a stack trace, which can be confusing and at a first glance suggests that the node crashed or some alike.
Improve this by checking and printing out *all* of the missing release binaries and failing with an explicit error in this case. Also add an info on how to download previous releases binaries. Noticed while testing #30328.
Can be tested by e.g.
```
$ rm -rf ./releases
$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py
```
master:
<details>
<summary>Long test fail output</summary>
```
...
2024-12-10T18:48:54.067000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 590, in start_nodes
node.start(extra_args[i], *args, **kwargs)
File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 257, in start
self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
File "/usr/lib/python3.10/subprocess.py", line 971, in __init__
self._execute_child(args, executable, preexec_fn, close_fds,
File "/usr/lib/python3.10/subprocess.py", line 1863, in _execute_child
raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '/home/thestack/bitcoin/releases/v28.0/bin/bitcoind'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
self.setup()
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
self.setup_network()
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
self.setup_nodes()
File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 54, in setup_nodes
self.start_nodes()
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 595, in start_nodes
self.stop_nodes()
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 610, in stop_nodes
node.stop_node(wait=wait, wait_until_stopped=False)
File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 396, in stop_node
self.stop(wait=wait)
File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 215, in __getattr__
assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 0] Error: no RPC connection
2024-12-10T18:48:54.118000Z TestFramework (INFO): Stopping nodes
Traceback (most recent call last):
File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 1097, in <module>
WalletMigrationTest(__file__).main()
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 159, in main
exit_code = self.shutdown()
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 331, in shutdown
self.stop_nodes()
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 610, in stop_nodes
node.stop_node(wait=wait, wait_until_stopped=False)
File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 396, in stop_node
self.stop(wait=wait)
File "/home/thestack/bitcoin/test/functional/test_framework/test_node.py", line 215, in __getattr__
assert self.rpc_connected and self.rpc is not None, self._node_msg("Error: no RPC connection")
AssertionError: [node 0] Error: no RPC connection
[node 0] Cleaning up leftover process
...
```
</details>
PR:
```
...
2025-01-01T20:26:27.999000Z TestFramework (INFO): PRNG seed is: 4570383538468804512
2025-01-01T20:26:28.000000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_lz66_zcu
2025-01-01T20:26:28.003000Z TestFramework (ERROR): Binary not found: /home/thestack/bitcoin/releases/v28.0/bin/bitcoind
2025-01-01T20:26:28.003000Z TestFramework (ERROR): Binary not found: /home/thestack/bitcoin/releases/v28.0/bin/bitcoin-cli
2025-01-01T20:26:28.003000Z TestFramework (INFO): Previous releases binaries can be downloaded via `test/get_previous_releases.py -b`.
2025-01-01T20:26:28.003000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 131, in main
self.setup()
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 315, in setup
self.setup_network()
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 409, in setup_network
self.setup_nodes()
File "/home/thestack/bitcoin/./build/test/functional/wallet_migration.py", line 50, in setup_nodes
self.add_nodes(self.num_nodes, versions=[
File "/home/thestack/bitcoin/test/functional/test_framework/test_framework.py", line 537, in add_nodes
raise AssertionError("At least one release binary is missing")
AssertionError: At least one release binary is missing
2025-01-01T20:26:28.061000Z TestFramework (INFO): Stopping nodes
...
```
ACKs for top commit:
fjahr:
re-ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b
kevkevinpal:
ACK [1ea7e45](1ea7e45a1f)
maflcko:
lgtm ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b
achow101:
ACK 1ea7e45a1f445d32a2b690d52befb2e63418653b
pablomartin4btc:
tACK 1ea7e45a1f445d32a2b690d52befb2e63418653b
Tree-SHA512: b621c3ce044ca8fc8715a4f4b1f96a8592a470c319a64444cced9ba692d315cfd4885a066679bf377b19136fa3530d9cff6f18894a45aa9c716d39b12719baa0
7c123c08ddce87ae55abfa83bea4a691bc0bd5e7 miner: add package feerate vector to CBlockTemplate (ismaelsadeeq)
Pull request description:
This PR enables `BlockAssembler` to add all selected packages' fee and virtual size to a vector, and then return the vector as a member of `CBlockTemplate` struct.
This PR is the first step in the https://github.com/bitcoin/bitcoin/issues/30392 project.
The packages' vsize and fee are used in #30157 to select a percentile fee rate of the top block in the mempool.
ACKs for top commit:
rkrux:
tACK 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7
ryanofsky:
Code review ACK 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7. Changes since last review are rebasing due to a test conflict, giving the new field a better name and description, resolving the test conflict, and renaming a lot of test variables. The actual code change is still one-line change.
glozow:
reACK 7c123c08ddce87ae55abfa83bea4a691bc0bd5e7
Tree-SHA512: 767b0b3d4273cf1589fd2068d729a66c7414c0f9574b15989fbe293f8c85cd6c641dd783cde55bfabab32cd047d7d8a071d6897b06ed4295c0d071e588de0861
- The package feerates are ordered by the sequence in which
packages are selected for inclusion in the block template.
- The commit also tests this new behaviour.
Co-authored-by: willcl-ark <will@256k1.dev>
If the `releases` directory exists, but still only a subset of the
necessary previous release binaries are available, the test fails by
throwing an exception (sometimes leading to follow-up exceptions like
"AssertionError: [node 0] Error: no RPC connection") and printing out
a stack trace, which can be confusing and at a first glance suggests
that the node crashed or some alike.
Improve this by checking and printing out *all* of the missing release
binaries and failing with an explicit error in this case. Also add an
info on how to download previous releases binaries.
Noticed while testing #30328.
Can be tested by e.g.
$ ./test/get_previous_releases.py -b
$ rm -rf ./releases/v28.0/
$ ./build/test/functional/wallet_migration.py
3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40 doc: Clarify comments about endianness after #30526 (Ryan Ofsky)
Pull request description:
This is a documentation-only change following up on suggestions made in the #30526 review.
Motivation for this change is that I was recently reviewing #31583, which reminded me how confusing the arithmetic blob code was and made me want to write better comments.
ACKs for top commit:
achow101:
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
TheCharlatan:
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
Sjors:
ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
BrandonOdiwuor:
LGTM ACK 3e0a992a3f0f2b15b7be5049dc4f3134b4b0bc40
Tree-SHA512: 90d5582a25a51fc406d83ca6b8c4e5e4d3aee828a0497f4b226b2024ff89e29b9b50d0ae8ddeac6abf2757fe78548d58cf3dd54df4b6d623f634a2106048091d
04249682e381f976de6ba56bb4fb2996dfa194ab test: use Mining interface in miner_tests (Sjors Provoost)
Pull request description:
Needed for both #31283 and #31564.
By using the Mining interface in `miner_tests.cpp` we increase its coverage in unit tests.
ACKs for top commit:
achow101:
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab
ryanofsky:
Code review ACK 04249682e381f976de6ba56bb4fb2996dfa194ab, just minor suggested changes (renames, comments, BOOST_REQUIREs) since last review and some more extra clarifications and checks added to the CreateNewBlock_validity test. The CreateNewBlock_validity changes seem clear and easy to understand now.
vasild:
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab
tdb3:
ACK 04249682e381f976de6ba56bb4fb2996dfa194ab
Tree-SHA512: 2761cb7555d759670e40d8f37b96a079f8e12a588ac43313b9e63c69afd478321515873a8896ea56784f0100dac4476b0c0e0ef8b5418f8aea24d9965cace4d4
fa397177acfa1006ea6feee0b215c53e51f284de util: Add missing types in make_secure_unique (MarcoFalke)
Pull request description:
The return type of `std::forward` depends on the template type, and can not be recovered from the args. Attempting to do so will result in a compile failure. For example, `make_secure_unique<std::string>(std::string{});` does not compile on current master, but does with this pull.
Another example would be `make_secure_unique<std::pair<std::string, std::unique_ptr<int>>>(std::string{}, std::make_unique<int>(21));`
ACKs for top commit:
hodlinator:
ACK fa397177acfa1006ea6feee0b215c53e51f284de
hebasto:
ACK fa397177acfa1006ea6feee0b215c53e51f284de.
TheCharlatan:
ACK fa397177acfa1006ea6feee0b215c53e51f284de
Tree-SHA512: cc902c1111c929a79a6f806b5097136a465e8c727474176bad30a5777ebbb30bedb0bd35273b43bf839d2c00492500ddec724bd17349250451f6b329cb71e6f2
e8f0e6efaf555a7d17bdb118464bd572bd5f3933 lint: output-only - Avoid repeated arrows, trim (Hodlinator)
fa9aacf614f6066ff3b5c02f0daaaeb0ebb93f33 lint: Move assertion linter into lint runner (MarcoFalke)
Pull request description:
On failure, this makes the output more consistent with the other linters. Each failure will be marked with an '⚠️ ' emoji and explanation, making it easier to spot.
Also, add --line-number to the filesystem linter.
Also, add newlines after each failing check, to visually separate different failures from each other.
Can be reviewed with:
`--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`
ACKs for top commit:
davidgumberg:
crACK e8f0e6efaf
hodlinator:
re-ACK e8f0e6efaf555a7d17bdb118464bd572bd5f3933
TheCharlatan:
ACK e8f0e6efaf555a7d17bdb118464bd572bd5f3933
Tree-SHA512: 9896ff882af9d673ec3e6d2718f877b2fdc8514faba50942fcebacb9de95b1f5b4a5db595e1338fa7f505d06df2df304897350cc55c558c7a85232800e5fd804
0a76c292ac8fa29166a7ec6efda1fafce86af0d3 doc: Install `net/py-pyzmq` port on FreeBSD for `interface_zmq.py` (Hennadii Stepanov)
Pull request description:
On FreeBSD, Python's `zmq` module is provided as a separate port.
This PR updates the FreeBSD Build Guide to include this port, enabling the `interface_zmq.py` functional test.
ACKs for top commit:
maflcko:
lgtm ACK 0a76c292ac8fa29166a7ec6efda1fafce86af0d3
vasild:
ACK 0a76c292ac8fa29166a7ec6efda1fafce86af0d3
Tree-SHA512: c13eada3e870149f47348145d6a29f41125ac75efd88eabe6dd2d0429e0377ed280e76a764cfaf627498c1d07b9135a995cc644146fa666bc3bfa0eb2c86e88b
2bdaf52ed1259fd3bec22b680e12563fcee0a8b3 doc: Update NetBSD Build Guide (Hennadii Stepanov)
Pull request description:
This PR:
1. Updates the documented NetBSD version.
2. Adds the optional ZeroMQ package to align the guide with other *BSD systems.
3. Updates the Python version to meet the minimum requirement specified in https://github.com/bitcoin/bitcoin/pull/30527.
4. Suggests to Install [`net/py-zmq`](https://ftp.netbsd.org/pub/pkgsrc/current/pkgsrc/net/py-zmq/index.html) package to enable the `interface_zmq.py` functional test.
5. Fix a formatting issue.
See also the recent NetBSD nightly build at https://github.com/hebasto/bitcoin-core-nightly/actions/runs/12554769828/job/35003929261.
ACKs for top commit:
tdb3:
ACK 2bdaf52ed1259fd3bec22b680e12563fcee0a8b3
Tree-SHA512: 16562e7325dd92e44fa641c8d4e64df69ee76175fc8eba61da06775d4a751307825a6ffe1743fb614a591ba1d33d197ea6b7f9111f5a5b335f6b257bb4868bf6
fa0411ee305fe04800c54d141fbbeac342eaa764 ci: Run functional tests in msan task (MarcoFalke)
Pull request description:
Now that the CI machines have a bit more CPU, it seems good to run the functional tests as well under msan. (Also, bump the llvm minor version)
ACKs for top commit:
TheCharlatan:
ACK fa0411ee305fe04800c54d141fbbeac342eaa764
Tree-SHA512: 0dbb2b934485ed54b8caafb5bcd96ddef87088b148dab72a584f721c398bb7fda4095fb720b9ad602dc71f8f40a1e0f29e1b08b2879b78b90b29d46604df36c3
29bca9713d21916b315c2ca0e9183bf567f39351 test: fix typo in mempool_ephemeral_dust (epysqyli)
Pull request description:
The `test_node_restart` test in `test/functional/mempool_ephemeral_dust.py` has a repetition in the comment.
ACKs for top commit:
maflcko:
lgtm ACK 29bca9713d21916b315c2ca0e9183bf567f39351
Tree-SHA512: 9828d23ca27e24d64031cd103ce9f9bd9e997ef9b63e6122ad6573073fb3c956964a72cd23dfa5773e52e195eee668762ab470bb540e686a4abd3d7561b40c59
faf7eac364fb7f421a649b483286ac8681d92b31 test: clang-format -i src/univalue/test/unitester.cpp (MarcoFalke)
fafa9cc7a5994003a7dccb67f83ee409e4b8f544 test: Embed univalue json tests in binary (MarcoFalke)
fa044857caf70db5d9608e168e59f3169a66977a test: Re-enable univalue test fail18.json (MarcoFalke)
63b6b638aa5e1de1858294d0d14dce38f45a9cbb build: Use character literals for generated headers to avoid narrowing (Lőrinc)
Pull request description:
All other benchmarks and tests have their data embedded, except for the univalue json tests.
This is not only confusing, but also problematic, when the test binary is moved to a different system for testing, because one has to put the test files in the source dir that was used at compile-time.
Fix all issues by embedding them. Also, re-enable a disabled test. Also, fix an issue in the GenerateHeaderFromJson.cmake.
Requested in https://github.com/bitcoin/bitcoin/pull/31434/files#r1876000910
ACKs for top commit:
l0rinc:
ACK faf7eac364fb7f421a649b483286ac8681d92b31
fjahr:
tACK faf7eac364fb7f421a649b483286ac8681d92b31
achow101:
ACK faf7eac364fb7f421a649b483286ac8681d92b31
TheCharlatan:
Re-ACK faf7eac364fb7f421a649b483286ac8681d92b31
hebasto:
Re-ACK faf7eac364fb7f421a649b483286ac8681d92b31. The commit, which modifies CMake scripts, has been replaced with the one from https://github.com/bitcoin/bitcoin/pull/31547, and a formatting commit has been added since my recent [review](https://github.com/bitcoin/bitcoin/pull/31542#pullrequestreview-2517189261).
Tree-SHA512: 72ad202125746f32ccf07411ad3efd2771f27a40525c204cba3c9c83b3ca46d05dd18f6fa5985720c6684bdcbb4c4853fc609ced095ddd1a124832318dd8a55d
fa83bec78ef3e86445e522afa396c97b58eb1902 refactor: Allow std::byte in Read(LE/BE) (MarcoFalke)
Pull request description:
Starting with C++17, `std::byte` is often (not always) a better choice over `uint8_t` for new code.
However, the existing codebase discourages the use of `std::byte`, when helpers such as `ReadLE32` are used. This is because calling code will be cluttered with byte-casts.
Fix it by allowing `std::byte` pointers in `ReadLE32` (and friends).
ACKs for top commit:
sipa:
utACK fa83bec78ef3e86445e522afa396c97b58eb1902
fjahr:
Code review ACK fa83bec78ef3e86445e522afa396c97b58eb1902
theuni:
utACK fa83bec78ef3e86445e522afa396c97b58eb1902
l0rinc:
ACK fa83bec78ef3e86445e522afa396c97b58eb1902
Tree-SHA512: 83604dc9df9ad447ad1b6f81f1e1844554c2c5331fcb78bdba1300e050d9dcbe9cf7a1b2dd250772bb23a8bf02a4ec26441012fe2f4bcc670ef31c15151adb15
This is a documentation-only change following up on suggestions made in the
#30526 review.
Motivation for this change is that I was recently reviewing #31583, which
reminded me how confusing the arithmetic blob code was and made me want to
write better comments.
b6f0593f43304f4ff31e8b68558ceeb1b588403c doc: add release note about testmempoolaccept debug-message (Matthew Zipkin)
f9cac635237142090271022164fa5d58e014493d test: cover testmempoolaccept debug-message in RBF test (Matthew Zipkin)
f9650e18ea6edb41c0136cc2ec3c7e0aba1bf83a rbf: remove unecessary newline at end of error string (Matthew Zipkin)
221c789e91696569fa34dbd162d26e98cf9cab64 rpc: include verbose reject-details field in testmempoolaccept response (Matthew Zipkin)
Pull request description:
Adds a new field `reject-details` in `testmempoolaccept` responses to include `m_debug_message` from `ValidationState`. This string is the complete error message thrown by the mempool in response to `sendrawtransaction`.
The extra verbosity is helpful to consumers of `testmempoolaccept`, which is sort of a debug tool anyway.
example:
>
> {
> "txid": "07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8",
> "wtxid": "5dc243b1b92ee2f5a43134eb3e23449be03d1abb3d7f3c03c836ed0f13c50185",
> "allowed": false,
> "reject-reason": "insufficient fee",
> "reject-details": "insufficient fee, rejecting replacement 07d7a59a7bdad4c3a5070659ea04147c9b755ad9e173c52b6a38e017abf0f5b8; new feerate 0.00300000 BTC/kvB <= old feerate 0.00300000 BTC/kvB"
> }
ACKs for top commit:
rkrux:
re-ACK b6f0593f43304f4ff31e8b68558ceeb1b588403c
glozow:
ACK b6f0593f43304f4ff31e8b68558ceeb1b588403c
Tree-SHA512: 340b8023d59cefa84598879c4efdb7c399a3f62da126e87c595523f302e53d33098fc69da9c5f8c92b7580dc75466c66cea372051f935b197265648fe15c43a3
1. Update the documented NetBSD version.
2. Add the optional ZeroMQ package to align the guide with other *BSD
systems.
3. Update the Python version to meet the minimum requirement specified
in https://github.com/bitcoin/bitcoin/pull/30527.
4. Install `net/py-zmq` package to enable the `interface_zmq.py`
functional test.
5. Fix a formatting issue.
366ae00b779acd59a61719422f0597acb17fb3e0 descriptor: Assume `ParseScript` is not being called with a P2WPKH context (brunoerg)
e36640859089baabc46f68217843f96a3ebdc20c descriptor: remove unreachable verification for `pkh` (brunoerg)
Pull request description:
This PR removes an unreachable verification in the `ParseScript` function. It returns an error if `pkh` is not being used at top level, sh, wsh or tr. However, any usage of `pkh` without these contexts will not reach this verification but other ones like "invalid keys" (e.g. `wpkh(pkh(L4gM1FBdyHNpkzsFh9ipnofLhpZRp2mwobpeULy1a6dBTvw8Ywtd))`).
ACKs for top commit:
davidgumberg:
crACK 366ae00b77
achow101:
ACK 366ae00b779acd59a61719422f0597acb17fb3e0
tdb3:
cr ACK 366ae00b779acd59a61719422f0597acb17fb3e0
sipa:
crACK 366ae00b779acd59a61719422f0597acb17fb3e0
Tree-SHA512: b954221a77eed623aeed5eb54f14e82c49540a151d3388831924caa7a784e48a2a975e418af1e13d491e4f8cded3b1797aa39e0e4e39e302a991105df09cdec0
b29d68f942e333d2cfdd6be8d49fe484e0d15e11 test: descriptor: fix test for `MaxSatisfactionWeight` (brunoerg)
Pull request description:
To get the maximum size of a satisfaction for a descriptor with no max sig, the parameter `use_max_sig` should be false.
ACKs for top commit:
fjahr:
utACK b29d68f942e333d2cfdd6be8d49fe484e0d15e11
achow101:
ACK b29d68f942e333d2cfdd6be8d49fe484e0d15e11
tdb3:
re ACK b29d68f942e333d2cfdd6be8d49fe484e0d15e11
furszy:
utACK b29d68f942e
Tree-SHA512: 8559718d126e60ce21a34183f74d227546108b43e3897e49622d6677ed9e7707caa962fd811d8787bd4dafc48a0e779ef11050d5990293faa2f91ded4aaa4f4b
fa63b8232f38e78d3c6413fa7d51809f376de75c test: generateblocks called by multiple threads (MarcoFalke)
fa62c8b1f04a5386ffa171aeff713d55bd874cbe rpc: Extend scope of validation mutex in generateblock (MarcoFalke)
Pull request description:
The mutex (required by TestBlockValidity) must be held after creating the block, until TestBlockValidity is called. Otherwise, it is possible that the chain advances in the meantime and leads to a crash in TestBlockValidity: `Assertion failed: pindexPrev && pindexPrev == chainstate.m_chain.Tip() (validation.cpp: TestBlockValidity: 4338)`
Fixes#31562
ACKs for top commit:
davidgumberg:
reACK fa63b8232f
achow101:
ACK fa63b8232f38e78d3c6413fa7d51809f376de75c
ismaelsadeeq:
re-ACK fa63b8232f38e78d3c6413fa7d51809f376de75c
mzumsande:
utACK fa63b8232f38e78d3c6413fa7d51809f376de75c
Tree-SHA512: 3dfda1192af52546ab11fbffe44af8713073763863f4a63fbcdbdf95b1c6cbeb003dc4b8b29e7ec67362238ad15e07d8f6855832a0c68dc5370254f8cbf9445c
bc43ecaf6dc0830a27296d3a29428814fed07bb1 test: add functional test for balance after snapshot completion (Martin Zumsande)
226d03dd610dd65938554bcf0abfe79f7ca7fb4d validation: Send correct notification during snapshot completion (Martin Zumsande)
Pull request description:
After AssumeUtxo background sync is completed in a `ActivateBestChain()` call, the `GetRole()` function called with `BlockConnected()` returns `ChainstateRole::NORMAL` instead of `ChainstateRole::BACKGROUND` for this chainstate.
This would make the wallet (which ignores `BlockConnected` notifications for the background chainstate) process it, change `m_last_block_processed_height` to the (ancient) snapshot height, and display an incorrect balance.
Fix this by caching the chainstate role before calling `ActivateBestChainStep()`.
Also contains a test for this situation that fails on master.
Fixes#31546
ACKs for top commit:
fjahr:
re-ACK bc43ecaf6dc0830a27296d3a29428814fed07bb1
achow101:
ACK bc43ecaf6dc0830a27296d3a29428814fed07bb1
furszy:
Code review ACK bc43ecaf6dc
TheCharlatan:
lgtm ACK bc43ecaf6dc0830a27296d3a29428814fed07bb1
Tree-SHA512: c5db677cf3fbab3a33ec127ec6c27c8812299e8368fd3c986bc34d0e515c4eb256f6104479f27829eefc098197de3af75d64ddca636b6b612900a0e21243e4f2
d9d5bc2e7466033d989432f53a112325fa3d6d4a qa: Limit `-maxconnections` in tests (Hennadii Stepanov)
Pull request description:
On systems such as NetBSD, the `bitcoind` typically prints the following warning:
```
Warning: Reducing -maxconnections from 125 to 96, because of system limitations.
```
This breaks the functional test framework (see https://github.com/bitcoin/bitcoin/issues/23968).
This PR limits the `-maxconnections` to mitigate the issue and enable functional tests on NetBSD.
Fixes https://github.com/bitcoin/bitcoin/issues/23968.
Here is CI log from the [`bitcoin-core-nightly`](https://github.com/hebasto/bitcoin-core-nightly) repository: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/12415868523/job/34663207030
ACKs for top commit:
maflcko:
re-ACK d9d5bc2e7466033d989432f53a112325fa3d6d4a
achow101:
ACK d9d5bc2e7466033d989432f53a112325fa3d6d4a
mzumsande:
Code Review ACK d9d5bc2e7466033d989432f53a112325fa3d6d4a
tdb3:
tested ACK d9d5bc2e7466033d989432f53a112325fa3d6d4a
Tree-SHA512: ad02adce98ce609176c9688289a0ca347932da5b6f259c6ab4e686914352f3d61f819adc150be80220f969200ee6a0c1c8737426525816fa0df58d688c51410c
fa0998f0a028161fe7264d0e81af36ffdcb43384 test: Avoid intermittent error in assert_equal(pruneheight_new, 248) (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/31446
The test uses the P2P network to sync blocks, which has no inherent guarantee that the blocks are sent and received in the right order, assuming the headers are received first.
This can mean that the first block file is flushed with block at height 249 and block at height 248 is added to the second file. In the log it looks like: `Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...249, time=2011-02-02...2024-12-03) (onto 1) (height 248)`. The test assumes that the height of the last pruned block in the first file is 248, expecting it to look like: `Leaving block file 0: CBlockFileInfo(blocks=249, size=65319, heights=0...248, time=2011-02-02...2024-12-09) (onto 1) (height 249) `.
Fix the issue by using a linear dumb sync.
ACKs for top commit:
achow101:
ACK fa0998f0a028161fe7264d0e81af36ffdcb43384
mzumsande:
Code Review ACK fa0998f0a028161fe7264d0e81af36ffdcb43384
i-am-yuvi:
Code Review ACK fa0998f0a028161fe7264d0e81af36ffdcb43384
fjahr:
Code review ACK fa0998f0a028161fe7264d0e81af36ffdcb43384
Tree-SHA512: 59cb4317be6cf9012c9bf7a3e9f5ba96b8b114b30bd2ac42af4fe742cd26a634d685b075f04a84bd782b2a43a342d75bb20a042bd82ad2831dbf844d39517ca2
fa6e599cf9fbacb393ac17e0a1b3572f217817f0 test: Call generate through test framework only (MarcoFalke)
Pull request description:
The generate RPCs are special in that they should only be called by the test framework itself. This way, they will call the sync function on the nodes, which can avoid intermittent test issues. Also, when the sync is disabled, it will happen explicitly by setting the `sync_fun`.
Apply this rule here, so that all generate calls are written consistently.
ACKs for top commit:
achow101:
ACK fa6e599cf9fbacb393ac17e0a1b3572f217817f0
rkrux:
tACK fa6e599cf9fbacb393ac17e0a1b3572f217817f0
hodlinator:
ACK fa6e599cf9fbacb393ac17e0a1b3572f217817f0
i-am-yuvi:
Tested ACK fa6e599cf9fbacb393ac17e0a1b3572f217817f0
Tree-SHA512: 31079997f1e17031ecd577904457e0560388aa53cadb1bbda281865271e8e4cf244bc6bf315838a717bf9d6620c201093e30039aa0007bec3629f7ca56abfba3
a2c45ae5480a2ee643665d6ecaee9714a287a70e test: report failure during utf8 response decoding (furszy)
Pull request description:
Useful for debugging issues such https://github.com/bitcoin/bitcoin/pull/31241#issuecomment-2462816933.
Prints the entire response content instead of printing only the position of the byte it can't be decoded.
The diff between the error messages can be seen by running the `wallet_migration.py` functional test with the following patch applied:
```
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
--- a/src/wallet/rpc/wallet.cpp(revision d65918c5da52c7d5035b4151dee9ffb2e94d4761)
+++ b/src/wallet/rpc/wallet.cpp(date 1731005254673)
@@ -801,7 +801,7 @@
}
UniValue r{UniValue::VOBJ};
- r.pushKV("wallet_name", res->wallet_name);
+ r.pushKV("wallet_name", "\xc3\x28");
if (res->watchonly_wallet) {
r.pushKV("watchonly_name", res->watchonly_wallet->GetName());
}
```
ACKs for top commit:
achow101:
ACK a2c45ae5480a2ee643665d6ecaee9714a287a70e
theStack:
re-ACK a2c45ae5480a2ee643665d6ecaee9714a287a70e
rkrux:
tACK a2c45ae5480a2ee643665d6ecaee9714a287a70e
ismaelsadeeq:
utACK a2c45ae5480a2ee643665d6ecaee9714a287a70e
Tree-SHA512: 6abb524b5a215c51ec881eea91ebe8174140a88ff3874c8c88676157edae7818801356586a904dbb21b45053183315a6d71dbf917d753611d8e413776b57c484
fa494a1d53f3f030fafe7b533d72b2200428a0fd refactor: Specify const in std::span constructor, where needed (MarcoFalke)
faaf4800aa752dde63b8987b1eb0de4e54acf717 Allow std::span in stream serialization (MarcoFalke)
faa5391f77037601875cf4ed154bc42840d34b12 refactor: test: Return std::span from StringBytes (MarcoFalke)
fa86223475353cc994cc2563ba5aecc406d00815 refactor: Avoid passing span iterators when data pointers are expected (MarcoFalke)
faae6fa5f614425f6d58af6f224d4f5aae3e1bed refactor: Simplify SpanPopBack (MarcoFalke)
facc4f120b067af6f94f3125cecc9dafff3e5d57 refactor: Replace fwd-decl with proper include (MarcoFalke)
fac3a782eaf3fa5f12cd908ef6dbc874d4b0e2ba refactor: Avoid needless, unsafe c-style cast (MarcoFalke)
Pull request description:
The `std::span` type is already used in some parts of the codebase, and in most contexts can implicitly convert to and from `Span`. However, the two types are not identical in behavior and trying to use one over the other can result in compile failures in some contexts.
Fix all those issues by allowing either `Span` or `std::span` in any part of the codebase.
All of the changes are also required for the scripted-diff to replace `Span` with `std::span` in https://github.com/bitcoin/bitcoin/pull/31519
ACKs for top commit:
sipa:
utACK fa494a1d53f3f030fafe7b533d72b2200428a0fd
fjahr:
Code review ACK fa494a1d53f3f030fafe7b533d72b2200428a0fd
achow101:
ACK fa494a1d53f3f030fafe7b533d72b2200428a0fd
theuni:
utACK fa494a1d53f3f030fafe7b533d72b2200428a0fd.
adamandrews1:
utACK fa494a1d53
Tree-SHA512: 9440941823e884ff5d7ac161f58b9a0704d8e803b4c91c400bdb5f58f898e4637d63ae627cfc7330e98a721fc38285a04641175aa18d991bd35f8b69ed1d74c4
e56fc7ce6a92eae7e80657d9f57a148cc002358d rpc: increase the defaults for -rpcthreads and -rpcworkqueue (Vasil Dimov)
Pull request description:
`rpcthreads` was introduced with a default of 4 in 2013 in 21eb5adadbe3110a8708f2570185566e1f137a49
`rpcworkqueue` was introduced with a default of 16 in 2015 in 40b556d3742a1f65d67e2d4c760d0b13fe8be5b7
Resolves: https://github.com/bitcoin/bitcoin/issues/29386
---
Just bump the ancient default values. There is no perfect default that would fit everybody. This could lead to https://bikeshed.com/
ACKs for top commit:
achow101:
ACK e56fc7ce6a92eae7e80657d9f57a148cc002358d
andrewtoth:
ACK e56fc7ce6a92eae7e80657d9f57a148cc002358d
storopoli:
ACK e56fc7ce6a92eae7e80657d9f57a148cc002358d
tdb3:
ACK e56fc7ce6a92eae7e80657d9f57a148cc002358d
Tree-SHA512: ba3ea7392fda57950daa6b4c4d38ecdef9eebe5e786824d25f8b5cea03e760ffff7f77f3acd8eb6c6178b1e92b282e02cabb940ed7222eec7f73efdb819eef06
ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e rpc: add signet_challenge field to getblockchaininfo and getmininginfo (Ash Manning)
Pull request description:
Signet challenges are currently only available via `getblocktemplate` RPC.
`getblockchaininfo` and `getmininginfo` both provide inadequate information to distinguish signets. Since these are the RPCs used to determine the current network, they should also provide the signet challenge for signets.
Test coverage is included in `test/functional/feature_signet.py`.
ACKs for top commit:
sipa:
utACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e
achow101:
ACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e
i-am-yuvi:
Concept ACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e
Sjors:
ACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e
zaidmstrr:
Tested ACK [ecaa786](ecaa786cc1)
Tree-SHA512: 9ccf4ae634ee74353a2a895efb881fdc62ae703a134ccd219da2cd6080c7d38319e689054584722457a7cc79004bd6022292a3b0b90eaab9f7003564665e1ea4
b9766c9977e58a9ebc358d9879576376e76a72b1 Remove unused variable assignment (yancy)
Pull request description:
The variable is conditionally assigned toward the end of the loop and not used after. It's then set back to its default value at the beginning of the loop.
ACKs for top commit:
theuni:
utACK b9766c9977e58a9ebc358d9879576376e76a72b1
achow101:
ACK b9766c9977e58a9ebc358d9879576376e76a72b1
hodlinator:
crACK b9766c9977e58a9ebc358d9879576376e76a72b1
danielabrozzoni:
code review ACK b9766c9977e58a9ebc358d9879576376e76a72b1
murchandamus:
ACK b9766c9977e58a9ebc358d9879576376e76a72b1
Tree-SHA512: 45e62b0dd561a473f5ae21bfa91db494940b752886669c85b63a83b68d2a157a301e9450082635e921f3dc812e6307f4ad1674806b74b3e7e0f9f4db543ad93d
5709718b830161b7c2ba0db545ef0cfa98423597 coins: warn on shutdown for big UTXO set flushes (Lőrinc)
Pull request description:
Split out of https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2549027130
Setting a large `-dbcache` size postpones the index writes until the coins cache size exceeds the specified limit. This causes the final flush after manual termination to seemingly hang forever (e.g. tens of minutes for 20 GiB); Now that the `dbcache` upper cap has been lifted, this will become even more apparent, so a warning will be shown when large UTXO sets are flushed (currently >1 GiB), such as:
> 2024-12-18T18:25:03Z Flushed fee estimates to fee_estimates.dat.
> 2024-12-18T18:25:03Z [warning] Flushing large (1 GiB) UTXO set to disk, it may take several minutes
> 2024-12-18T18:25:09Z Shutdown: done
---
You can reproduce it by starting `bitcoind` with a large `-dbcache`:
> mkdir demo && cmake -B build -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/src/bitcoind -datadir=demo **-dbcache=10000**
Waiting until the used memory is over 1 GiB
> 2024-12-18T18:25:02Z UpdateTip: [...] progress=0.069009 cache=**1181.1MiB**(8827981txo)
And cancelling the process from the terminal:
> ^C2024-12-18T18:25:03Z tor: Thread interrupt
> [...]
> 2024-12-18T18:25:03Z **[warning] Flushing large (1 GiB) UTXO set to disk, it may take several minutes*
ACKs for top commit:
sipa:
utACK 5709718b830161b7c2ba0db545ef0cfa98423597
tdb3:
re ACK 5709718b830161b7c2ba0db545ef0cfa98423597
1440000bytes:
ACK 5709718b83
danielabrozzoni:
tACK 5709718b830161b7c2ba0db545ef0cfa98423597
Tree-SHA512: 608cf797de788501ccb2986508c155f5660c5f6f7a414524bfcc2820cfa9ebe3da558d13f2317d1f121a82d49ffe1e711a1152c743c22dab9f9807363f4ed8d5
06443b8f28bcec4315cec262eb03343dee5465a6 net: clarify if we ever sent or received from peer (Sjors Provoost)
1d01ad4d73e0eaae36c31153884e3d394ebc66b7 net: add LogIP() helper, use in net_processing (Sjors Provoost)
937ef9eb408e377cde4cab4dfcd27120afdedf1b net_processing: use CNode::DisconnectMsg helper (Sjors Provoost)
ad224429f823a66b431401d44bae21ed254a97e1 net: additional disconnection logging (Sjors Provoost)
Pull request description:
While debugging unexpected disconnections, possibly related to #28331, I found some additional [net] logging to be useful.
All cases where we disconnect now come with a log message that has the word `disconnecting`:
* all calls to `CloseSocketDisconnect()` log `disconnecting peer=…`
* wherever we set `pnode->fDisconnect = true;`
* for all `InactivityCheck` cases (which in turn sets `fDisconnect`)
* replaces "dropping" with "disconnecting" in `Network not active, dropping peer=…`
A few exceptions are listed here: https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1890824361
I changed `CloseSocketDisconnect()` to no longer log `disconnecting`, and instead have all the call sites do so.
This PR introduces two helper functions on `CNode`: `DisconnectMsg` and `LogIP`. The second and third commit use these helpers in `net_processing.cpp` so these disconnect messages are more consistent now (e.g. some didn't log the IP). No new messages are added there though.
The `LogIP()` helper is rarely used outside of a disconnect event, but it's available for future use.
Any `LogPrint` this PR touches is replaced with `LogDebug` (superseded by #30750), and every `LogPrintf ` with `LogInfo`.
ACKs for top commit:
davidgumberg:
reACK 06443b8f28
vasild:
ACK 06443b8f28bcec4315cec262eb03343dee5465a6
danielabrozzoni:
ACK 06443b8f28bcec4315cec262eb03343dee5465a6
hodlinator:
ACK 06443b8f28bcec4315cec262eb03343dee5465a6
Tree-SHA512: 525f4c11568616e1d48455a3fcab9e923da7432377fe9230468c15403d2e9b7ce712112df8fbd547cfec01dce0d1f26107cfc1b90f78cfc1fe13e08d57b08464
If AssumeUtxo background sync is completed in this
ActivateBestChain() call, the GetRole() function
returns "normal" instead of "background" for this chainstate.
This would make the wallet (which ignores BlockConnected
notifcation for the background chainstate) process it, change
m_last_block_processed_height, and display an incorrect
balance.
The mutex (required by TestBlockValidity) must be held after creating
the block, until TestBlockValidity is called. Otherwise, it is possible
that the chain advances in the meantime and leads to a crash in
TestBlockValidity:
Assertion failed: pindexPrev && pindexPrev == chainstate.m_chain.Tip() (validation.cpp: TestBlockValidity: 4338)
The diff can be reviewed with the git options
--ignore-all-space --function-context
Setting a large `-dbcache` size postpones the index writes until the coins cache size exceeds the specified limit.
This causes the final flush after manual termination to seemingly hang forever (e.g. tens of minutes for 20 GiB);
Now that the `dbcache` upper cap has been lifted, this will become even more apparent, so a warning will be shown when large UTXO sets are flushed (currently >1 GiB), such as:
> 2024-12-18T18:25:03Z Flushed fee estimates to fee_estimates.dat.
> 2024-12-18T18:25:03Z [warning] Flushing large (1 GiB) UTXO set to disk, it may take several minutes
> 2024-12-18T18:25:09Z Shutdown: done
Note that the related BCLog::BENCH units were also converted to `KiB` from `kB` to unify the bases.
Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
b8710201fbd01bf2cba3c3b1fd312316b1ae22af guix: disable timezone tools & profiling in glibc (fanquake)
23b8a424fb06d2e669d147b4f58e640b6eab646a guix: bump glibc 2.31 to 7b27c450c34563a28e634cccb399cd415e71ebfe (fanquake)
Pull request description:
An additional commit has been backported to the 2.31 branch:
https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/release/2.31/master.
Pass `--disable-timezone-tools`: removes `var/profiles/x86_64-linux-gnu/sbin/zdump`.
Pass `--disable-profile`: profiling is disabled by default, but make that explicit.
ACKs for top commit:
theuni:
utACK b8710201fbd01bf2cba3c3b1fd312316b1ae22af
hebasto:
ACK b8710201fbd01bf2cba3c3b1fd312316b1ae22af.
Tree-SHA512: 0d9a0e7451cc42384bbdd0b46c740c7aa964dc12e3f0376de586bf90e57799ebb04675892861cb38a53b5ca0e265061fa7111596cf1c94171303d0d048785ab4
be1a2e5dfbdf4b3799f07e48c41b09b54b72f9b6 doc: Install `py3-zmq` port on OpenBSD for `interface_zmq.py` (Hennadii Stepanov)
Pull request description:
On OpenBSD, Python's `zmq` module is provided as a separate [port](https://www.ports.to/path/net/py-zmq,python3.html).
This PR updates the OpenBSD Build Guide to include this port, enabling the `interface_zmq.py` functional test.
Also updates the documented OpenBSD version.
ACKs for top commit:
theStack:
Tested ACK be1a2e5dfbdf4b3799f07e48c41b09b54b72f9b6
Tree-SHA512: 4d560385b94e8c7491aa19d2157d8a799617e08136601dc565a909d4c74e12582a1d273bc97ad7c2d0e57c5cf7377560ba02ef58c12f8991652322553740d2ba
