Base the task on --preset=dev-mode to ensure maximal coverage and add
the following:
bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
Also, shorten the name, for a less cluttered web view.
The test case no longer detects this specific issue for GCC versions
12.1+, as explained in the
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90348 thread and in this
compiler-explorer playground:
https://godbolt.org/z/Y48osrjM8
So remove the test case and update the -fstack-reuse=none cmake
docstring with the underlying affected GCC versions, and the bug URL.
The test was added in commit ddb75c2e87a60ed24065bdf0c3bfabf4e058cef1.
After the create_directories wrapper removal, the test is redundant with
the unit test in the upstream stdlib. Also, there is a Bitcoin Core
functional test that covers this behavior in
test/functional/feature_dirsymlinks.py
So remove this unit test.
Finally, I could not find a real system that still ships a buggy stdlib
(v11.2) in their package manager. A stand-alone test is also available
in compiler-explorer under https://godbolt.org/z/aeMKraYrT.
743abbcbde9e5a2db489bca461c98df461eff7d0 refactor: inline constant return value of `BlockTreeDB::WriteBatchSync` and `BlockManager::WriteBlockIndexDB` and `BlockTreeDB::WriteFlag` (Lőrinc)
e030240e909493549e24aa8bcd5b382cab6e2c79 refactor: inline constant return value of `CDBWrapper::Erase` and `BlockTreeDB::WriteReindexing` (Lőrinc)
cdab9480e9e35656f490878f92dab5427b36f21d refactor: inline constant return value of `CDBWrapper::Write` (Lőrinc)
d1847cf5b5af232ad180f5d302361b72334952b2 refactor: inline constant return value of `TxIndex::DB::WriteTxs` (Lőrinc)
50b63a5698e533376ef7a20bc0c440d3d6bf7a9f refactor: inline constant return value of `CDBWrapper::WriteBatch` (Lőrinc)
Pull request description:
Related to https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223587480
### Summary
`WriteBatch` always returns `true` - the errors are handled by throwing `dbwrapper_error` instead.
### Context
This boolean return value of the `Write` methods is confusing because it's inconsistent with `CDBWrapper::Read`, which catches exceptions and returns a boolean to indicate success/failure. It's bad that `Read` returns and `Write` throws - but it's a lot worse that `Write` advertises a return value when it actually communicates errors through exceptions.
### Solution
This PR removes the constant return values from write methods and inlines `true` at their call sites. Many upstream methods had boolean return values only because they were propagating these constants - those have been cleaned up as well.
Methods that returned a constant `true` value that now return `void`:
- `CDBWrapper::WriteBatch`, `CDBWrapper::Write`, `CDBWrapper::Erase`
- `TxIndex::DB::WriteTxs`
- `BlockTreeDB::WriteReindexing`, `BlockTreeDB::WriteBatchSync`, `BlockTreeDB::WriteFlag`
- `BlockManager::WriteBlockIndexDB`
### Note
`CCoinsView::BatchWrite` (and transitively `CCoinsViewCache::Flush` & `CCoinsViewCache::Sync`) were intentionally not changed here. While all implementations return `true`, the base `CCoinsView::BatchWrite` returns `false`. Changing this would cause `coins_view` tests to fail with:
> terminating due to uncaught exception of type std::logic_error: Not all unspent flagged entries were cleared
We can fix that in a follow-up PR.
ACKs for top commit:
achow101:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
janb84:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
TheCharlatan:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
sipa:
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
Tree-SHA512: b2a550bff066216f1958d2dd9a7ef6a9949de518cc636f8ab9c670e0b7a330c1eb8c838e458a8629acb8ac980cea6616955cd84436a7b8ab9096f6d648073b1e
060bb55508245776bb6a39c8b7849769ee588d69 rpc: add decoded tx details to gettransaction with extra wallet fields (Matthew Zipkin)
ad1c3bdba547685ca4163316017ab78e965c7ad1 [move only] move DecodeTxDoc() to a common util file for sharing (Matthew Zipkin)
d633db54166497685b80a12c51db6772982e01fe rpc: add "ischange: true" in wallet gettransaction decoded tx output (Matthew Zipkin)
Pull request description:
This change is motivated by external RBF clients like https://github.com/CardCoins/additive-rbf-batcher/. It saves the user a redundant re-looping of tx outputs, calling `getaddressinfo` on each one, looking for the change output in order to adjust the fee.
The field `"ischange"` only appears when `gettransaction` is called on a wallet, and is either `true` or not present at all. I chose not to include `ischange: false` because it is confusing to see that on *received* transactions.
Example of the new field:
```
"vout": [
{
"value": 1.00000000,
"n": 0,
"scriptPubKey": {
"asm": "0 5483235e05c76273b3b50af62519738781aff021",
"desc": "addr(bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu)#d42g84j6",
"hex": "00145483235e05c76273b3b50af62519738781aff021",
"address": "bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu",
"type": "witness_v0_keyhash"
}
},
{
"value": 198.99859000,
"n": 1,
"scriptPubKey": {
"asm": "0 870ab1ab58632b05a417d5295f4038500e407592",
"desc": "addr(bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju)#tgapemkv",
"hex": "0014870ab1ab58632b05a417d5295f4038500e407592",
"address": "bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju",
"type": "witness_v0_keyhash"
},
"ischange": true
}
]
```
ACKs for top commit:
furszy:
ACK [060bb55](060bb55508)
maflcko:
review ACK 060bb55508245776bb6a39c8b7849769ee588d69 🌛
achow101:
ACK 060bb55508245776bb6a39c8b7849769ee588d69
rkrux:
lgtm ACK 060bb55508245776bb6a39c8b7849769ee588d69
Tree-SHA512: aae4854d2bb4e9a7bc1152691ea90e594e8da8a63c9c7fda72a504fb6a7e54ae274ed5fa98d35d270e0829cc8f8d2fd35a5fc9735c252a10aa42cc22828930e7
01cc20f3307c532f402cdf5dc17f2f14920b725b test: improve coverage for a resolved stalling situation (Martin Zumsande)
9af6daf07ed0a82386c1930e67683af5f2090e8b test: remove magic number when checking for blocks that have arrived (Martin Zumsande)
3069d66dcac0e1eeca2142a2d72d3d010335346f p2p: During block download, adjust pindexLastCommonBlock better (Martin Zumsande)
Pull request description:
As described in #32179, `pindexLastCommonBlock` is updated later than necessary
in master.
In case of a linear chain with no forks, it can be moved forward at the beginning of
`FindNextBlocksToDownload`, so that the updated value can be used to better estimate `nWindowEnd`.
This helps the node to request all blocks from peers within the correct 1024-block-window and avoids peers being incorrectly marked as stallers.
I also changed `p2p_ibd_stalling.py` to cover the situation after a resolved situation, making sure that no additional peers are marked for stalling.
Fixes#32179
ACKs for top commit:
Crypt-iQ:
crACK 01cc20f3307c532f402cdf5dc17f2f14920b725b
stringintech:
re-ACK 01cc20f
achow101:
ACK 01cc20f3307c532f402cdf5dc17f2f14920b725b
sipa:
utACK 01cc20f3307c532f402cdf5dc17f2f14920b725b
Tree-SHA512: a97f7a7ef5ded538ee35576e04b3fbcdd46a6d0189c7ba3abacc6e0d81e800aac5b0c2d2565d0462ef6fd4acc751989f577fd6adfd450171a7d6ab26f437df32
1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596 log: reduce excessive messages during block replay (Lőrinc)
Pull request description:
### Summary
After an incomplete reindex the blocks will need to be replayed.
This results in excessive `Rolling back` and `Rolling forward` messages which quickly triggers the recently introduced log rate limiter.
Change the logging strategy to:
- Add single `LogInfo` messages showing the full range being replayed for both rollback and roll forward;
- Log progress at `LogInfo` level only every 10,000 blocks to track the long operations.
### Reproducer:
* Start a normal ibd, stop after some progress
* Do a reindex, stop before it finishes
* Restart the node normally without specifying the reindex parameter
It should start rolling the blocks forward.
Before this change the excessive logging would show:
```
[*] Rolling forward 000000002f4f55aecfccc911076dc3f73ac0288c83dc1d79db0a026441031d40 (46245)
[*] Rolling forward 0000000017ffcf34c8eac010c529670ba6745ea59cf1edf7b820928e3b40acf6 (46246)
```
After the change it shows:
```
Replaying blocks
Rolling forward to 00000000000000001034012d7e4facaf16ca747ea94b8ea66743086cfe298ef8 (326223 to 340991)
Rolling forward 00000000000000000faabab19f17c0178c754dbed023e6c871dcaf74159c5f02 (330000)
Rolling forward 00000000000000000d9b2508615d569e18f00c034d71474fc44a43af8d4a5003 (340000)
...
Rolled forward to 00000000000000001034012d7e4facaf16ca747ea94b8ea66743086cfe298ef8
```
(similarly to rolling back)
ACKs for top commit:
Crypt-iQ:
crACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
stickies-v:
ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
achow101:
ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
vasild:
ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
hodlinator:
Concept ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596
Tree-SHA512: 44ed1da8336de5a3d937e11a13e6f1789064e23eb70640a1c406fbb0074255344268f6eb6b06f036ca8d22bfeb4bdea319c3085a2139d848f6d36a4f8352b76a
79b4c276e7b9b526fa8f563b1e09b2b970baece6 Bugfix: QA: rpc_bind: Skip nonloopback test if no such address is found (Luke Dashjr)
Pull request description:
Without this, I get:
```
2025-09-19T03:14:05.157000Z TestFramework (INFO): PRNG seed is: 3218602557639511064
2025-09-19T03:14:05.158000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin-test/a
2025-09-19T03:14:05.158000Z TestFramework (INFO): Check for ipv6
2025-09-19T03:14:05.158000Z TestFramework (INFO): Check for non-loopback interface
2025-09-19T03:14:05.158000Z TestFramework (INFO): Bind test for []
2025-09-19T03:14:05.516000Z TestFramework (INFO): Bind test for []
2025-09-19T03:14:05.871000Z TestFramework (INFO): Bind test for ['[::1]']
2025-09-19T03:14:06.227000Z TestFramework (INFO): Bind test for ['127.0.0.1', '[::1]']
2025-09-19T03:14:06.583000Z TestFramework (INFO): Using interface None for testing
2025-09-19T03:14:06.583000Z TestFramework (INFO): Bind test for [None]
2025-09-19T03:14:06.583000Z TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "/Bitcoin/bitcoin/workingtree/test/functional/test_framework/test_framework.py", line 135, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/Bitcoin/bitcoin/workingtree/test/functional/rpc_bind.py", line 126, in run_test
self._run_nonloopback_tests()
~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "/Bitcoin/bitcoin/workingtree/test/functional/rpc_bind.py", line 157, in _run_nonloopback_tests
self.run_bind_test([self.non_loopback_ip], self.non_loopback_ip, [self.non_loopback_ip],
~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[(self.non_loopback_ip, self.defaultport)])
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/Bitcoin/bitcoin/workingtree/test/functional/rpc_bind.py", line 38, in run_bind_test
expected = [(addr_to_hex(addr), port) for (addr, port) in expected]
~~~~~~~~~~~^^^^^^
File "/Bitcoin/bitcoin/workingtree/test/functional/test_framework/netutil.py", line 132, in addr_to_hex
if '.' in addr: # IPv4
^^^^^^^^^^^
TypeError: argument of type 'NoneType' is not iterable
```
ACKs for top commit:
maflcko:
review ACK 79b4c276e7b9b526fa8f563b1e09b2b970baece6 🏑
theStack:
Tested ACK 79b4c276e7b9b526fa8f563b1e09b2b970baece6
Tree-SHA512: 2a723d9bc5d1d50a8321a4f8a8cac3da3125d373ea71e6cc9d03de07307008f58970e361490d4c34530a6a976cb078b62d0ef09b7fb321ca1cfb9249a70d99a5
4543a3bde26ff2440c16b06cc1dcf1994dc85720 Squashed 'src/minisketch/' changes from ea8f66b1ea..d1bd01e189 (Hennadii Stepanov)
Pull request description:
This PR updates the `minisketch` subtree to latest upstream, which includes:
- https://github.com/bitcoin-core/minisketch/pull/75
- https://github.com/bitcoin-core/minisketch/pull/98
ACKs for top commit:
fanquake:
ACK c235aa468b0dcc67b49340dbe9b675c513cec7bf
Tree-SHA512: 856fb8b7dc2e743c9c67164023bf53faf8766079aeccc82a30c8b90c85920b31977b6a8b26e51e5485b20e445a3ca6ff806e701a53e95f70181ea30055e3528c
fad6efd3bef1d123f806d492f019e29530b03a5e refactor: Use STR_INTERNAL_BUG macro where possible (MarcoFalke)
fada379589a17e86396aa7c2ce458ff2ff602b84 doc: Remove unused bugprone-lambda-function-name suppression (MarcoFalke)
fae1d99651e29341e486a10e6340335c71a2144e refactor: Use const reference to std::source_location (MarcoFalke)
fa5fbcd61563942122623fe2840a677853081990 util: Allow Assert() in contexts without __func__ (MarcoFalke)
Pull request description:
Without this, compile warnings could be hit about `__func__` being only valid inside functions.
```
warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function] note: expanded from macro Assert
115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
| ^
```
Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486258473
This also introduces a slight behaviour change, because `std::source_location::function_name` usually includes the entire function signature instead of just the name.
ACKs for top commit:
l0rinc:
Code review ACK fad6efd3bef1d123f806d492f019e29530b03a5e
stickies-v:
ACK fad6efd3bef1d123f806d492f019e29530b03a5e
hodlinator:
re-ACK fad6efd3bef1d123f806d492f019e29530b03a5e
Tree-SHA512: e78a2d812d5ae22e45c93db1661dafbcd22ef209b3d8d8d5f2ac514e92fd19a17c3f0a5db2ef5e7748aa2083b10c0465326eb36812e6a80e238972facd2c7e98
0698c6b494de0e28c9b909585905aab5b187286e doc: Correct `pkgin` command usage on NetBSD (Hennadii Stepanov)
Pull request description:
When using `pkgin` on NetBSD, the `install` command must be specified.
ACKs for top commit:
fanquake:
ACK 0698c6b494de0e28c9b909585905aab5b187286e
Tree-SHA512: 840fc1621d6fa9ad43501a3691a31cffd66c1ac8d34167f7ab0fe33e1a395198c241b3c31f3d0ebc314e28c0edb6055cc2ca3deba6408dcbd14390fd679a4803
dee7eec64389aa48daff6f7f3ecbd931af72050a doc: mention coverage build in quickstart section (frankomosh)
Pull request description:
Adds a single comment in the libFuzzer quick-start that links to the Developer Notes coverage section. No build flags are changed or shown.
ACKs for top commit:
janb84:
ACK dee7eec64389aa48daff6f7f3ecbd931af72050a
dergoegge:
ACK dee7eec64389aa48daff6f7f3ecbd931af72050a
Tree-SHA512: 2fe5ffb6c3d06f75694646473c29b4cc9fe571f4659631ec174d444a14716771308eedeb7acab3bef7f62e9bfa8ed0462da0163b214cccdc6a9ad63bbf66d2a0
fa6db67369fbf9b9f0ec839b898edd7ba8bfe31a ci: [refactor] Extract build_dir constant in ci-test-each-commit-exec.py (MarcoFalke)
fa95e6cdc1c5f85b57b295e19f248f3db4187cfd ci: Use cmake --preset=dev-mode in test-each-commit task (MarcoFalke)
Pull request description:
Using the preset should reduce the bloat and need to maintain several places to list the same cmake cache variables.
The only difference should be that `bitcoin-chainstate (experimental)` will be enabled, which seems fast and in line with the goal of the CI task.
* Before: https://github.com/bitcoin/bitcoin/actions/runs/19174075826/job/54814118651#step:8:315
* After: (this pull) https://github.com/bitcoin/bitcoin/actions/runs/19190748069/job/54864837086#step:7:324
```diff
bitcoin-tx .......................... ON
bitcoin-util ........................ ON
bitcoin-wallet ...................... ON
- bitcoin-chainstate (experimental) ... OFF
+ bitcoin-chainstate (experimental) ... ON
libbitcoinkernel (experimental) ..... ON
kernel-test (experimental) .......... ON
Optional features:
ACKs for top commit:
hebasto:
ACK fa6db67369fbf9b9f0ec839b898edd7ba8bfe31a, I have reviewed the code and it looks OK.
Tree-SHA512: 61a78de7bcbf42bd266cb035f354862f5d1e1235acd2a81041e3a68a4d3ab4703fa2cfc993f28e4dacaa74e3cccc9ef568d5d4526605ce5a00bcd7c347b97121
dcb56fd4cb59e6857c110dd87019459989dc1ec3 interfaces: add interruptWait method (ismaelsadeeq)
Pull request description:
This is an attempt to fix#33575 see the issue for background and the usefulness of this feature.
This PR uses one of the suggested approaches: adding a new `interruptWaitNext()` method to the mining interface.
It introduces a new boolean variable, `m_interrupt_wait`, which is set to `false` when the thread starts waiting. The `interruptWaitNext()` method wakes the thread and sets `m_interrupt_wait` to `true`.
Whenever the thread wakes up, it checks whether the wait was aborted; if so, it simply set ` m_interrupt_wait ` to false and return`nullptr`.
This PR also adds a functional test for the new method. The test uses `asyncio` to spawn two tasks and attempts to ensure that the wait is executed before the interrupt by using an event monitor. It adds a 0.1-second buffer to ensure the wait has started executing.
If that buffer elapses without `waitNext` executing, the test will fail because a transaction is created after the buffer.
ACKs for top commit:
furszy:
Code ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
ryanofsky:
Code review ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3, just tweaking semantics slightly since last review so if an `interruptWait` call is made shortly after a `waitNext` call it will reliably cause the `waitNext` call to return right away without blocking, even if the `waitNext` call had not begun to execute or wait yet.
Sjors:
tACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
TheCharlatan:
ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3
Tree-SHA512: a03f049e1f303b174a9e5d125733b6583dfd8effa12e7b6c37bd9b2cff9541100f5f4514e80f89005c44a57d7e47804afe87aa5fdb6831f3b0cd9b01d83e42be
The removed comment become obsolete after bitcoin/bitcoin#32697 and
bitcoin/bitcoin#32881.
-BEGIN VERIFY SCRIPT-
sed -i "s/ Some tests are disabled if Python 3 is not available.//g" \
$( git grep -l " Some tests are disabled if Python 3 is not available." ./doc/ )
-END VERIFY SCRIPT-
fa1e8d8bad92f5fba2b086d78581df4c8123b098 refactor: Add missing include in bitcoinkernel_wrapper.h (MarcoFalke)
Pull request description:
Otherwise, the compilation may fail with:
```
/home/admin/actions-runner/_work/_temp/src/kernel/bitcoinkernel_wrapper.h:271:14: error: no type named 'exception_ptr' in namespace 'std'; did you mean 'exception'?
271 | std::exception_ptr exception;
| ~~~~~^~~~~~~~~~~~~
| exception
/cxx_build/include/c++/v1/__exception/exception.h:72:33: note: 'exception' declared here
72 | class _LIBCPP_EXPORTED_FROM_ABI exception {
| ^
In file included from /home/admin/actions-runner/_work/_temp/src/bitcoin-chainstate.cpp:1:
/home/admin/actions-runner/_work/_temp/src/kernel/bitcoinkernel_wrapper.h:284:35: error: no member named 'current_exception' in namespace 'std'
284 | data.exception = std::current_exception();
| ^~~~~~~~~~~~~~~~~
/home/admin/actions-runner/_work/_temp/src/kernel/bitcoinkernel_wrapper.h:290:14: error: no member named 'rethrow_exception' in namespace 'std'
290 | std::rethrow_exception(user_data.exception);
| ^~~~~~~~~~~~~~~~~
/home/admin/actions-runner/_work/_temp/src/kernel/bitcoinkernel_wrapper.h:273:65: error: no viable conversion from 'std::nullptr_t' to 'std::exception'
273 | UserData user_data = UserData{.bytes = &bytes, .exception = nullptr};
| ^~~~~~~
/home/admin/actions-runner/_work/_temp/src/kernel/bitcoinkernel_wrapper.h:733:16: note: in instantiation of function template specialization 'btck::write_bytes<btck_Block>' requested here
733 | return write_bytes(get(), btck_block_to_bytes);
| ^
/cxx_build/include/c++/v1/__exception/exception.h:75:25: note: candidate constructor not viable: no known conversion from 'std::nullptr_t' to 'const exception &' for 1st argument
75 | _LIBCPP_HIDE_FROM_ABI exception(const exception&) _NOEXCEPT = default;
| ^ ~~~~~~~~~~~~~~~~
4 errors generated.
ACKs for top commit:
TheCharlatan:
ACK fa1e8d8bad92f5fba2b086d78581df4c8123b098
hebasto:
ACK fa1e8d8bad92f5fba2b086d78581df4c8123b098.
yuvicc:
ACK fa1e8d8bad92f5fba2b086d78581df4c8123b098
Tree-SHA512: c0127678db5913402c92b7602d159faae26539dc33f6159abd909b33746dd4626b8cbb6a86d8ccd3c9c83e06956fe55fb721a034480498d0cd87349aceea51f9
24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7 refactor: remove dead code in `CountWitnessSigOps` (Lőrinc)
Pull request description:
Found while reviewing #32840
The `nullptr` witness path was dead in normal code paths: replacing it with reference enables us deleting unreachable logic.
Code coverage proof:
https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html#L2135
ACKs for top commit:
kevkevinpal:
ACK [24bcad3](24bcad3d4d)
maflcko:
review ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7 🐏
darosior:
Neat. utACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7.
stickies-v:
ACK 24bcad3d4df59690f30c9df8ebb62f0bddd0f1c7
Tree-SHA512: 92c87e431f06a15d8eeb02e20e9154b272c4586ddacf77c8d83783091485fb82c24ecbd711db7043a92cf6169746db24ad46a5904d694aea9d3c3aa96da725f0
ec8516ceb7568d7b09836b830023978bd37f8462 test: remove obsolete `get_{key,multisig}` helpers from wallet_util.py (Sebastian Falbesoner)
Pull request description:
This small cleanup PR is a late follow-up to #31250 (commit c847dee1488a294c9a9632a00ba1134b21e41947). These helpers are unused and wouldn't work anymore, as they call a legacy wallet RPC (`dumpprivkey`). They were only ever used for testing the `importmulti` RPC, which also doesn't exist anymore. Functional tests that need to create key pairs and derive various output script types from them can use `get_generate_key` (introduced in #16528, commit f193ea889ddb53d9a5c47647966681d525e38368) instead, without involving the node.
ACKs for top commit:
rkrux:
crACK ec8516ceb7568d7b09836b830023978bd37f8462
brunoerg:
code review ACK ec8516ceb7568d7b09836b830023978bd37f8462
Tree-SHA512: cab3701f1a8fbcff0eecea4cfdc632ffac226afd2eefe3c9274a84ee1bb71fb231a57cd0876025c714be257a249157b048b67e309b3734442c425d85cf481cf6
2bd155e6ee7e3cabd76083ac921b34bb45d98769 test: move create_malleated_version() to messages.py for reuse (Vasil Dimov)
Pull request description:
Move `create_malleated_version()` from `p2p_orphan_handling.py` to `test_framework/messages.py` so that it can be reused by other tests.
---
This is part of [#29415 Broadcast own transactions only via short-lived Tor or I2P connections](https://github.com/bitcoin/bitcoin/pull/29415). Putting it in its own PR to reduce the size of #29415 and because it does not depend on the other commits from there.
ACKs for top commit:
maflcko:
review ACK 2bd155e6ee7e3cabd76083ac921b34bb45d98769 🍨
l0rinc:
ACK 2bd155e6ee7e3cabd76083ac921b34bb45d98769
brunoerg:
ACK 2bd155e6ee7e3cabd76083ac921b34bb45d98769
w0xlt:
Code Review ACK 2bd155e6ee
pablomartin4btc:
cr ACK 2bd155e6ee7e3cabd76083ac921b34bb45d98769
rkrux:
crACK 2bd155e6ee7e3cabd76083ac921b34bb45d98769
Tree-SHA512: 566bd204ebf8b7e1b06513fd86fd5a3bb8097c5e875e50758f886abdc405c02717554334b10eb77e72400d0361924824b655e558b1f06e3064d1c837252e04af
9577daa3b88a8fbe8c96d2848adcc88c55c55a29 doc: Add cmake help option in Windows build instructions (frankomosh)
Pull request description:
Follow-up to #33088.
Adds `cmake -B build -LH` documentation to Windows build guides, similar to Unix build documentation.
Based on the suggestion and example provided by stickies-v in #33088, with minor adjustment to match existing indented code block format in `build-windows.md`.
Tested for:
- WSL Ubuntu with mingw-w64 cross-compilation
- Windows 11 with Visual Studio 2022 (MSVC)
ACKs for top commit:
waketraindev:
ACK 9577daa
stickies-v:
ACK 9577daa3b88a8fbe8c96d2848adcc88c55c55a29
hebasto:
ACK 9577daa3b88a8fbe8c96d2848adcc88c55c55a29.
pablomartin4btc:
ACK 9577daa3b88a8fbe8c96d2848adcc88c55c55a29
Tree-SHA512: a9c08944aff962a61d0111317b79cbb44368e85d0255daf8d3c229c6532cf8c11046fe6bba81025ef994d92b4324247ec75f5bdaf4a89b3ef403c5be37e63bd8
2d23820ee11678d567c75f94c40011ed9f0e274f refactor: remove dead branches in `SingletonClusterImpl` (Lőrinc)
Pull request description:
Found during review: [cluster mempool: control/optimize TxGraph memory usage](https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2423058928)
### Fixes
`SplitAll()` always calls `ApplyRemovals()` first, for a singleton, it empties the cluster, therefore any `SingletonClusterImpl` passed to `Split()` must be empty.
`TxGraphImpl::ApplyDependencies()` first merges each dependency group and asserts the group has at least one dependency. Since `parent` != `child`, `TxGraphImpl::Merge()` upgrades the merge target to `GenericClusterImpl`, therefore the `ApplyDependencies()` is never dispatched to `SingletonClusterImpl`.
### Coverage proof:
* https://maflcko.github.io/b-c-cov/fuzz.coverage/src/txgraph.cpp.gcov.html#L1446
* https://storage.googleapis.com/oss-fuzz-coverage/bitcoin-core/reports/20251103/linux/src/bitcoin-core/src/txgraph.cpp.html#L1446
ACKs for top commit:
instagibbs:
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
sipa:
ACK 2d23820ee11678d567c75f94c40011ed9f0e274f
Tree-SHA512: 5135913206c800d5344df61c6654f00917cb85567bc5b821576c7891805cf7689bf47968434a06517d09183dadfefc257d24c42b55a7b99486a4c9b11fc523af
Now that the __func__ is no longer used, the
NOLINTBEGIN(bugprone-lambda-function-name) can be removed.
Also, re-format the NONFATAL_UNREACHABLE macro, while touching the
adjacent line.
ed5720509f03ddd7f9158b9adf0d8fd7f56c8578 kernel: Use enumeration type for flags argument (TheCharlatan)
Pull request description:
Just a small followup from https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3485634089.
ACKs for top commit:
alexanderwiederin:
ACK ed5720509f
rkrux:
lgtm ACK ed5720509f03ddd7f9158b9adf0d8fd7f56c8578 as per the mentioned review comment of the previous PR.
stickies-v:
ACK ed5720509f03ddd7f9158b9adf0d8fd7f56c8578
Tree-SHA512: f365d86c76b88b7730c4182192f8fbacc536121de367d03f27450087b39d13bb0cc21ca5ede9428077ccf5be90e959e892d7d383c8a2900b7bfd2864dde37466
038849e2e09bb9f4ce1fb5a1f291745506c6a52d clang-tidy: Remove no longer needed NOLINT (Hennadii Stepanov)
Pull request description:
From https://github.com/bitcoin/bitcoin/pull/33714/files#r2491476516:
> Actually, the `NOLINT` was fixed and can be removed? You've confirmed that it is undeclared on the listed platforms, so it can't be hit by `readability-redundant-declaration`
ACKs for top commit:
maflcko:
lgtm ACK 038849e2e09bb9f4ce1fb5a1f291745506c6a52d
l0rinc:
I wanted to ask the same on the original PR but forgot - ACK 038849e2e09bb9f4ce1fb5a1f291745506c6a52d
Tree-SHA512: c0b24235425e80baeac3158c7169122364f31140367bc289430d34f01cd38f9f6a3931319f6fe4e1dc86bc4d87e21a5b4b8a2263c199e8083593f89ce592a177
5c41fa2918c8fee36d0e0375e753249f1efa7c07 guix: disable libsanitizer in Linux GCC build (fanquake)
Pull request description:
This causes issues when building against newer glibcs (i.e 2.42), and isn't needed in any case.
```bash
../../../../gcc-14.3.0/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp:483:31: error: invalid application of ‘sizeof’ to incomplete type ‘__sanitizer::termio’
483 | unsigned struct_termio_sz = sizeof(struct termio);
| ^~~~~~~~~~~~~~~~~~~~~
```
Extracted from #25573.
ACKs for top commit:
maflcko:
lgtm ACK 5c41fa2918c8fee36d0e0375e753249f1efa7c07
hebasto:
ACK 5c41fa2918c8fee36d0e0375e753249f1efa7c07.
willcl-ark:
utACK 5c41fa2918c8fee36d0e0375e753249f1efa7c07
Tree-SHA512: a2a7b1a72155d47a1f2a1f3270d7a8255fad432c2d5d77f139e489634a3abb0ce1152c3c19fd7df629fa17c53ddb80eb1807dd195b7b7455d12d49a09c7b92dc
Used to display available configuration options, for consistency with recent changes in Unix build docs
Co-authored-by: stickies-v <stickies-v@users.noreply.github.com>
Performance likely does not matter here, but from a perspective of
code-readablilty, a const reference should be preferred for read-only
access.
So use it here.
This requires to set -Wno-error=dangling-reference for GCC 13.1
compilations, but this false-positive is fixed in later GCC versions.
See also https://godbolt.org/z/fjc6be65M
Without this, compile warnings could be hit about __func__ being only
valid inside functions.
warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function]
note: expanded from macro Assert
115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
| ^
Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486258473
These helpers use a legacy wallet RPC (`dumpprivkey`) and thus don't
work anymore. They were only ever used for testing the `importmulti`
RPC, which also doesn't exist anymore.
This causes issues when building against newer glibcs (i.e 2.42), and isn't needed
in any case.
```bash
../../../../gcc-14.3.0/libsanitizer/sanitizer_common/sanitizer_platform_limits_posix.cpp:483:31: error: invalid application of ‘sizeof’ to incomplete type ‘__sanitizer::termio’
483 | unsigned struct_termio_sz = sizeof(struct termio);
| ^~~~~~~~~~~~~~~~~~~~~
```
Extracted from #25573.
6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 kernel: Add Purpose section to header documentation (TheCharlatan)
7e9f00bcc1742932e40426dddd906851b46c24d3 kernel: Allowing reducing exports (TheCharlatan)
7990463b1059ba5fc4ebe37fd1105a9e168ae20d kernel: Add pure kernel bitcoin-chainstate (TheCharlatan)
36ec9a3ea2322adf8d73e711fb17cf2a64f5bcaa Kernel: Add functions for working with outpoints (TheCharlatan)
5eec7fa96aa3042025181c4c4b57263beb869244 kernel: Add block hash type and block tree utility functions to C header (TheCharlatan)
f5d5d1213cc4f4ef8bfe335736c665ed7bc3137d kernel: Add function to read block undo data from disk to C header (TheCharlatan)
09d0f626388a10eed1f264386014665fcae4fa22 kernel: Add functions to read block from disk to C header (TheCharlatan)
a263a4caf2311bc31dc2ef1c04dab9517ee0d28f kernel: Add function for copying block data to C header (TheCharlatan)
b30e15f4329ab0ee6bb5c4c1d1f6067be364c59e kernel: Add functions for the block validation state to C header (TheCharlatan)
aa262da7bcfa9bf3d0105e6f689eae7c6e95a0e5 kernel: Add validation interface to C header (TheCharlatan)
d27e27758d51bc2aa125dc967691aacc4f3811d3 kernel: Add interrupt function to C header (TheCharlatan)
1976b13be9c87baa1229b1573bdc8a1da562db0d kernel: Add import blocks function to C header (TheCharlatan)
a747ca1f516e7ec73758c6017e2eca5635ab2b74 kernel: Add chainstate load options for in-memory dbs in C header (TheCharlatan)
070e77732cdb927cc27ddd39c52dec22c5d717a0 kernel: Add options for reindexing in C header (TheCharlatan)
ad80abc73df38f94d887a905773c4500ca0c2961 kernel: Add block validation to C header (TheCharlatan)
cb1590b05efd090bc2e4be49b5a649f8d248afa0 kernel: Add chainstate loading when instantiating a ChainstateManager (TheCharlatan)
e2c1bd3d713ffe0b8eede711e84f64e0fe4ae836 kernel: Add chainstate manager option for setting worker threads (TheCharlatan)
65571c36a265ec340343b555d1537c58ab335538 kernel: Add chainstate manager object to C header (TheCharlatan)
c62f657ba330572969ab5e86c739712e800bcbcb kernel: Add notifications context option to C header (TheCharlatan)
9e1bac45852d177cf387314a54053a3f7ec8ce99 kernel: Add chain params context option to C header (TheCharlatan)
337ea860dfda12dac084209027a54fba857e7a89 kernel: Add kernel library context object (TheCharlatan)
28d679bad9fda3f180ab0f7d34353e1fa9294d68 kernel: Add logging to kernel library C header (TheCharlatan)
2cf136dec4ce16c8a7c47b35c7c9244dfc3b6da8 kernel: Introduce initial kernel C header API (TheCharlatan)
Pull request description:
This is a first attempt at introducing a C header for the libbitcoinkernel library that may be used by external applications for interfacing with Bitcoin Core's validation logic. It currently is limited to operations on blocks. This is a conscious choice, since it already offers a lot of powerful functionality, but sits just on the cusp of still being reviewable scope-wise while giving some pointers on how the rest of the API could look like.
The current design was informed by the development of some tools using the C header:
* A re-implementation (part of this pull request) of [bitcoin-chainstate](https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-chainstate.cpp).
* A re-implementation of the python [block linearize](https://github.com/bitcoin/bitcoin/tree/master/contrib/linearize) scripts: https://github.com/TheCharlatan/bitcoin/tree/kernelLinearize
* A silent payment scanner: https://github.com/josibake/silent-payments-scanner
* An electrs index builder: https://github.com/josibake/electrs/commits/electrs-kernel-integration
* A rust bitcoin node: https://github.com/TheCharlatan/kernel-node
* A reindexer: https://github.com/TheCharlatan/bitcoin/tree/kernelApi_Reindexer
The library has also been used by other developers already:
* A historical block analysis tool: https://github.com/ismaelsadeeq/mining-analysis
* A swiftsync hints generator: https://github.com/theStack/swiftsync-hints-gen
* Fast script validation in floresta: https://github.com/vinteumorg/Floresta/pull/456
* A swiftsync node implementation: https://github.com/2140-dev/swiftsync/tree/master/node
Next to the C++ header also made available in this pull request, bindings for other languages are available here:
* Rust: https://github.com/TheCharlatan/rust-bitcoinkernel
* Python: https://github.com/stickies-v/py-bitcoinkernel
* Go: https://github.com/stringintech/go-bitcoinkernel
* Java: https://github.com/yuvicc/java-bitcoinkernel
The rust bindings include unit and fuzz tests for the API.
The header currently exposes logic for enabling the following functionality:
* Feature-parity with the now deprecated libbitcoin-consensus
* Optimized sha256 implementations that were not available to previous users of libbitcoin-consensus thanks to a static kernel context
* Full support for logging as well as control over categories and severity
* Feature parity with the existing experimental bitcoin-chainstate
* Traversing the block index as well as using block index entries for reading block and undo data.
* Running the chainstate in memory
* Reindexing (both full and chainstate-only)
* Interrupting long-running functions
The pull request introduces a new kernel-only test binary that purely relies on the kernel C header and the C++ standard library. This is intentionally done to show its capabilities without relying on other code inside the project. This may be relaxed to include some of the existing utilities, or even be merged into the existing test suite.
The complete docs for the API as well as some usage examples are hosted on [thecharlatan.ch/kernel-docs](https://thecharlatan.ch/kernel-docs/index.html). The docs are generated from the following repository (which also holds the examples): [github.com/TheCharlatan/kernel-docs](https://github.com/TheCharlatan/kernel-docs).
#### How can I review this PR?
Scrutinize the commit messages, run the tests, write your own little applications using the library, let your favorite code sanitizer loose on it, hook it up to your fuzzing infrastructure, profile the difference between the existing bitcoin-chainstate and the bitcoin-chainstate introduced here, be nitty on the documentation, police the C interface, opine on your own API design philosophy.
To get a feeling for the API, read through the tests, or one of the examples.
To configure this PR for making the shared library and the bitcoin-chainstate and test_kernel utilities available:
```
cmake -B build -DBUILD_KERNEL_LIB=ON -DBUILD_UTIL_CHAINSTATE=ON
```
Once compiled the library is part of the build artifacts that can be installed with:
```
cmake --install build
```
#### Why a C header (and not a C++ header)
* Shipping a shared library with a C++ header is hard, because of name mangling and an unstable ABI.
* Mature and well-supported tooling for integrating C exists for nearly every popular language.
* C offers a reasonably stable ABI
Also see https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2285719575.
#### What about versioning?
The header and library are still experimental and I would expect this to remain so for some time, so best not to worry about versioning yet.
#### Potential future additions
In future, the C header could be expanded to support (some of these have been roughly implemented):
* Handling transactions, block headers, coins cache, utxo set, meta data, and the mempool
* Adapters for an abstract coins store
* Adapters for an abstract block store
* Adapters for an abstract block tree store
* Allocators and buffers for more efficient memory usage
* An "[io-less](https://sans-io.readthedocs.io/how-to-sans-io.html)" interface
* Hooks for an external mempool, or external policy rules
#### Current drawbacks
* For external applications to read the block index of an existing Bitcoin Core node, Bitcoin Core needs to shut down first, since leveldb does not support reading across multiple processes. Other than migrating away from leveldb, there does not seem to be a solution for this problem. Such a migration is implemented in #32427.
* The fatal error handling through the notifications is awkward. This is partly improved through #29642.
* Handling shared pointers in the interfaces is unfortunate. They make ownership and freeing of the resources fuzzy and poison the interfaces with additional types and complexity. However, they seem to be an artifact of the current code that interfaces with the validation engine. The validation engine itself does not seem to make extensive use of these shared pointers.
* If multiple instances of the same type of objects are used, there is no mechanism for distinguishing the log messages produced by each of them. A potential solution is #30342.
* The background leveldb compaction thread may not finish in time leading to a non-clean exit. There seems to be nothing we can do about this, outside of patching leveldb.
ACKs for top commit:
alexanderwiederin:
re-ACK 6c7a34f3b0
stringintech:
re-ACK 6c7a34f
laanwj:
Code review ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969
ismaelsadeeq:
reACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 👾
fanquake:
ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969 - soon we'll be running bitcoin (kernel)
Tree-SHA512: ffe7d4581facb7017d06da8b685b81f4b5e4840576e878bb6845595021730eab808d8f9780ed0eb0d2b57f2647c85dcb36b6325180caaac469eaf339f7258030
79d6f458e2300e1f47b94467cda233e1c761f8be random: scope environ extern to macOS, BSDs and Illumos (fanquake)
Pull request description:
These platforms fail to compile with it removed.
macOS: #33675
BSDs / Illumos: https://github.com/hebasto/bitcoin-core-nightly/pull/79.
ACKs for top commit:
l0rinc:
ACK 79d6f458e2300e1f47b94467cda233e1c761f8be
hebasto:
re-ACK 79d6f458e2300e1f47b94467cda233e1c761f8be.
Tree-SHA512: dcaa15f0939d65a804107ceb110037f44d0ff70759f4d42fcc497a9c173ac28b1287b867f01732224788d1c1f9c883565bafc3abed3ccf28f1b67f23997ce3cf
