743abbcbde refactor: inline constant return value of `BlockTreeDB::WriteBatchSync` and `BlockManager::WriteBlockIndexDB` and `BlockTreeDB::WriteFlag` (Lőrinc)
e030240e90 refactor: inline constant return value of `CDBWrapper::Erase` and `BlockTreeDB::WriteReindexing` (Lőrinc)
cdab9480e9 refactor: inline constant return value of `CDBWrapper::Write` (Lőrinc)
d1847cf5b5 refactor: inline constant return value of `TxIndex::DB::WriteTxs` (Lőrinc)
50b63a5698 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 743abbcbde
janb84:
ACK 743abbcbde
TheCharlatan:
ACK 743abbcbde
sipa:
ACK 743abbcbde
Tree-SHA512: b2a550bff066216f1958d2dd9a7ef6a9949de518cc636f8ab9c670e0b7a330c1eb8c838e458a8629acb8ac980cea6616955cd84436a7b8ab9096f6d648073b1e
060bb55508 rpc: add decoded tx details to gettransaction with extra wallet fields (Matthew Zipkin)
ad1c3bdba5 [move only] move DecodeTxDoc() to a common util file for sharing (Matthew Zipkin)
d633db5416 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 060bb55508🌛
achow101:
ACK 060bb55508
rkrux:
lgtm ACK 060bb55508
Tree-SHA512: aae4854d2bb4e9a7bc1152691ea90e594e8da8a63c9c7fda72a504fb6a7e54ae274ed5fa98d35d270e0829cc8f8d2fd35a5fc9735c252a10aa42cc22828930e7
01cc20f330 test: improve coverage for a resolved stalling situation (Martin Zumsande)
9af6daf07e test: remove magic number when checking for blocks that have arrived (Martin Zumsande)
3069d66dca 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 01cc20f330
stringintech:
re-ACK 01cc20f
achow101:
ACK 01cc20f330
sipa:
utACK 01cc20f330
Tree-SHA512: a97f7a7ef5ded538ee35576e04b3fbcdd46a6d0189c7ba3abacc6e0d81e800aac5b0c2d2565d0462ef6fd4acc751989f577fd6adfd450171a7d6ab26f437df32
1fc7a81f1f 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 1fc7a81f1f
stickies-v:
ACK 1fc7a81f1f
achow101:
ACK 1fc7a81f1f
vasild:
ACK 1fc7a81f1f
hodlinator:
Concept ACK 1fc7a81f1f
Tree-SHA512: 44ed1da8336de5a3d937e11a13e6f1789064e23eb70640a1c406fbb0074255344268f6eb6b06f036ca8d22bfeb4bdea319c3085a2139d848f6d36a4f8352b76a
79b4c276e7 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 79b4c276e7🏑
theStack:
Tested ACK 79b4c276e7
Tree-SHA512: 2a723d9bc5d1d50a8321a4f8a8cac3da3125d373ea71e6cc9d03de07307008f58970e361490d4c34530a6a976cb078b62d0ef09b7fb321ca1cfb9249a70d99a5
4543a3bde2 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 c235aa468b
Tree-SHA512: 856fb8b7dc2e743c9c67164023bf53faf8766079aeccc82a30c8b90c85920b31977b6a8b26e51e5485b20e445a3ca6ff806e701a53e95f70181ea30055e3528c
fad6efd3be refactor: Use STR_INTERNAL_BUG macro where possible (MarcoFalke)
fada379589 doc: Remove unused bugprone-lambda-function-name suppression (MarcoFalke)
fae1d99651 refactor: Use const reference to std::source_location (MarcoFalke)
fa5fbcd615 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 fad6efd3be
stickies-v:
ACK fad6efd3be
hodlinator:
re-ACK fad6efd3be
Tree-SHA512: e78a2d812d5ae22e45c93db1661dafbcd22ef209b3d8d8d5f2ac514e92fd19a17c3f0a5db2ef5e7748aa2083b10c0465326eb36812e6a80e238972facd2c7e98
0698c6b494 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 0698c6b494
Tree-SHA512: 840fc1621d6fa9ad43501a3691a31cffd66c1ac8d34167f7ab0fe33e1a395198c241b3c31f3d0ebc314e28c0edb6055cc2ca3deba6408dcbd14390fd679a4803
dee7eec643 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 dee7eec643
dergoegge:
ACK dee7eec643
Tree-SHA512: 2fe5ffb6c3d06f75694646473c29b4cc9fe571f4659631ec174d444a14716771308eedeb7acab3bef7f62e9bfa8ed0462da0163b214cccdc6a9ad63bbf66d2a0
fa6db67369 ci: [refactor] Extract build_dir constant in ci-test-each-commit-exec.py (MarcoFalke)
fa95e6cdc1 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 fa6db67369, I have reviewed the code and it looks OK.
Tree-SHA512: 61a78de7bcbf42bd266cb035f354862f5d1e1235acd2a81041e3a68a4d3ab4703fa2cfc993f28e4dacaa74e3cccc9ef568d5d4526605ce5a00bcd7c347b97121
dcb56fd4cb 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 dcb56fd4cb
ryanofsky:
Code review ACK dcb56fd4cb, 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 dcb56fd4cb
TheCharlatan:
ACK dcb56fd4cb
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-
fa1e8d8bad 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 fa1e8d8bad
hebasto:
ACK fa1e8d8bad.
yuvicc:
ACK fa1e8d8bad
Tree-SHA512: c0127678db5913402c92b7602d159faae26539dc33f6159abd909b33746dd4626b8cbb6a86d8ccd3c9c83e06956fe55fb721a034480498d0cd87349aceea51f9
24bcad3d4d 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 24bcad3d4d🐏
darosior:
Neat. utACK 24bcad3d4d.
stickies-v:
ACK 24bcad3d4d
Tree-SHA512: 92c87e431f06a15d8eeb02e20e9154b272c4586ddacf77c8d83783091485fb82c24ecbd711db7043a92cf6169746db24ad46a5904d694aea9d3c3aa96da725f0
ec8516ceb7 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 c847dee148). 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 f193ea889d) instead, without involving the node.
ACKs for top commit:
rkrux:
crACK ec8516ceb7
brunoerg:
code review ACK ec8516ceb7
Tree-SHA512: cab3701f1a8fbcff0eecea4cfdc632ffac226afd2eefe3c9274a84ee1bb71fb231a57cd0876025c714be257a249157b048b67e309b3734442c425d85cf481cf6
2bd155e6ee 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 2bd155e6ee🍨
l0rinc:
ACK 2bd155e6ee
brunoerg:
ACK 2bd155e6ee
w0xlt:
Code Review ACK 2bd155e6ee
pablomartin4btc:
cr ACK 2bd155e6ee
rkrux:
crACK 2bd155e6ee
Tree-SHA512: 566bd204ebf8b7e1b06513fd86fd5a3bb8097c5e875e50758f886abdc405c02717554334b10eb77e72400d0361924824b655e558b1f06e3064d1c837252e04af
9577daa3b8 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 9577daa3b8
hebasto:
ACK 9577daa3b8.
pablomartin4btc:
ACK 9577daa3b8
Tree-SHA512: a9c08944aff962a61d0111317b79cbb44368e85d0255daf8d3c229c6532cf8c11046fe6bba81025ef994d92b4324247ec75f5bdaf4a89b3ef403c5be37e63bd8
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.
ed5720509f 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 ed5720509f as per the mentioned review comment of the previous PR.
stickies-v:
ACK ed5720509f
Tree-SHA512: f365d86c76b88b7730c4182192f8fbacc536121de367d03f27450087b39d13bb0cc21ca5ede9428077ccf5be90e959e892d7d383c8a2900b7bfd2864dde37466
038849e2e0 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 038849e2e0
l0rinc:
I wanted to ask the same on the original PR but forgot - ACK 038849e2e0
Tree-SHA512: c0b24235425e80baeac3158c7169122364f31140367bc289430d34f01cd38f9f6a3931319f6fe4e1dc86bc4d87e21a5b4b8a2263c199e8083593f89ce592a177
5c41fa2918 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 5c41fa2918
hebasto:
ACK 5c41fa2918.
willcl-ark:
utACK 5c41fa2918
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.
6c7a34f3b0 kernel: Add Purpose section to header documentation (TheCharlatan)
7e9f00bcc1 kernel: Allowing reducing exports (TheCharlatan)
7990463b10 kernel: Add pure kernel bitcoin-chainstate (TheCharlatan)
36ec9a3ea2 Kernel: Add functions for working with outpoints (TheCharlatan)
5eec7fa96a kernel: Add block hash type and block tree utility functions to C header (TheCharlatan)
f5d5d1213c kernel: Add function to read block undo data from disk to C header (TheCharlatan)
09d0f62638 kernel: Add functions to read block from disk to C header (TheCharlatan)
a263a4caf2 kernel: Add function for copying block data to C header (TheCharlatan)
b30e15f432 kernel: Add functions for the block validation state to C header (TheCharlatan)
aa262da7bc kernel: Add validation interface to C header (TheCharlatan)
d27e27758d kernel: Add interrupt function to C header (TheCharlatan)
1976b13be9 kernel: Add import blocks function to C header (TheCharlatan)
a747ca1f51 kernel: Add chainstate load options for in-memory dbs in C header (TheCharlatan)
070e77732c kernel: Add options for reindexing in C header (TheCharlatan)
ad80abc73d kernel: Add block validation to C header (TheCharlatan)
cb1590b05e kernel: Add chainstate loading when instantiating a ChainstateManager (TheCharlatan)
e2c1bd3d71 kernel: Add chainstate manager option for setting worker threads (TheCharlatan)
65571c36a2 kernel: Add chainstate manager object to C header (TheCharlatan)
c62f657ba3 kernel: Add notifications context option to C header (TheCharlatan)
9e1bac4585 kernel: Add chain params context option to C header (TheCharlatan)
337ea860df kernel: Add kernel library context object (TheCharlatan)
28d679bad9 kernel: Add logging to kernel library C header (TheCharlatan)
2cf136dec4 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 6c7a34f3b0
ismaelsadeeq:
reACK 6c7a34f3b0👾
fanquake:
ACK 6c7a34f3b0 - soon we'll be running bitcoin (kernel)
Tree-SHA512: ffe7d4581facb7017d06da8b685b81f4b5e4840576e878bb6845595021730eab808d8f9780ed0eb0d2b57f2647c85dcb36b6325180caaac469eaf339f7258030
79d6f458e2 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 79d6f458e2
hebasto:
re-ACK 79d6f458e2.
Tree-SHA512: dcaa15f0939d65a804107ceb110037f44d0ff70759f4d42fcc497a9c173ac28b1287b867f01732224788d1c1f9c883565bafc3abed3ccf28f1b67f23997ce3cf
52b1595850 depends: disable builtin variables (fanquake)
8b5a28fa78 depends: disable builtin rules and suffixes. (Dmitry Goncharov)
Pull request description:
This picks up #22126. Previously, this was more complicated to do, as depends packages (upnp, natpmp) used the rules being disabled. Those packages have since been removed.
When there is no rule to build a target in the makefile, make looks for a builtin rule. When `-r` is specified make no longer performs this lookup.
E.g. the following in an excerpt from `make -d` output. Here, make looks for a rule to build `all`.
```bash
Considering target file 'all'.
File 'all' does not exist.
Looking for an implicit rule for 'all'.
Trying pattern rule with stem 'all'.
Trying implicit prerequisite 'all.o'.
Trying pattern rule with stem 'all'.
Trying implicit prerequisite 'all.c'.
Trying pattern rule with stem 'all'.
Trying implicit prerequisite 'all.cc'.
Trying pattern rule with stem 'all'.
Trying implicit prerequisite 'all.C'.
Trying pattern rule with stem 'all'.
Trying implicit prerequisite 'all.cpp'.
Trying pattern rule with stem 'all'.
Trying implicit prerequisite 'all.p'.
Trying pattern rule with stem 'all'.
Trying implicit prerequisite 'all.f'.
...
```
Many more lines like this are omitted.
Because this build system does not use make builtin rules or suffixes, there is no benefit in having builtin rules enabled.
There are 2 benefits in having builtin rules disabled.
1. Improves performance by eliminating redundant lookups.
2. Simplifies troubleshooting by reducing the output of make `-d` or make `-p`.
Also see: https://www.gnu.org/software/make/manual/make.html#index-_002d_002dno_002dbuiltin_002drules.
ACKs for top commit:
hebasto:
re-ACK 52b1595850.
Tree-SHA512: 8b5fab5f8b01096942801d34edc47511e794d7eaf628ea78f8d2e020b83c91ed430f397dfb2b735d5fe957cdf5f10b0942fde6396f031f3ed9a773e0e6a6fabc
1e6e32fa8a ci: run native fuzz with MSAN job (fanquake)
3784d15bcd ci: use LLVM libcxx 21.1.5 (fanquake)
Pull request description:
I think this job should exist in this repo (not just qa-assets), if the alternative is double-handling changes to the interpreter. #32998 made changes which were then re-changed in #33600, to work around a false positive.
The unchached runtime of this job with `-lg` is `~32m`, with `-md` it's `~43m`.
Timeout is set to 150m, as the slow GHA runners were close to hitting a 120m limit.
ACKs for top commit:
maflcko:
lgtm ACK 1e6e32fa8a
dergoegge:
utACK 1e6e32fa8a
Tree-SHA512: afd4cb0039f4f49ddc23f5553a5bf6d5ceffbc12d91acd6890d5cc40c30b7421b23d04f305983d94c862daa6fc07535b1331d7fa2a8ebfe9f19c20d83d95c692
7632e0ba31 ci: fix configure docker action inputs (will)
0b3b8a3be1 ci: fix lint docker caching (will)
Pull request description:
Fixes: #33735
Correct runner type selection for the lint job.
This was erroneously left-out during refactor of the runner selection mechanism in #33302 causing the lint job to run on GH hosts (and therefore not be able to acces local cirrus caches).
ACKs for top commit:
maflcko:
re-ACK 7632e0ba31📞
hebasto:
ACK 7632e0ba31.
Tree-SHA512: b228a79d13ed80c75fc5e51c4fb93c7fad1cb33c00a659afe65033ce09d95e6ac84e01627f2e58e640ff483d798ac1b9e23f14d31a9c045fd99367059ceef5b4
b4d0288c46 doc: update Guix INSTALL.md (fanquake)
Pull request description:
It's somewhat annoying that Guix is falling out of being packaged by distros. For some more context, see https://lwn.net/Articles/1035491/.
> However, it is likely that the [Guix](https://guix.gnu.org/en/) package manager will soon be removed from the repositories for Debian 13 and Debian 12 ("bookworm", also called oldstable).
This seems to be happening. You can't `apt install guix` using the current release of Debian. https://packages.debian.org/search?keywords=guix. Guix is not going to be included in next release of Ubuntu (`25.10`): https://packages.ubuntu.com/search?keywords=guix.
Looking at https://aur.archlinux.org/packages/guix, comments over the last few months seem to indicate that the build is broken.
A 1.5.0 release is planned for sometime in January 2026: https://codeberg.org/guix/release-planning/wiki/release-1.5.0-project/. So hopefully the situation is going to improve in future.
ACKs for top commit:
willcl-ark:
ACK b4d0288c46
hebasto:
ACK b4d0288c46.
Tree-SHA512: 545f3529af82c18556ddfe104c01f77e28da31018a44047812450565a9b3bad3afa60f714b375c06c3a10aed722d54500846aa70a8069c5fe2d96b26d426b6c1
5d784bebaf clang-tidy: Disable `ArrayBound` check in src/ipc and src/test (Hennadii Stepanov)
5efdb0ef30 ci: Update Clang in "tidy" job (Hennadii Stepanov)
Pull request description:
This PR:
1. Updates to [IWYU 0.25](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.25), which is compatible with Clang 21.
2. Fixes new "modernize-use-default-member-init" warnings. The warning in `interpreter.cpp` is a [false positive](https://github.com/llvm/llvm-project/issues/160394), so it has been suppressed.
ACKs for top commit:
maflcko:
review ACK 5d784bebaf🎒
ryanofsky:
Code review ACK 5d784bebaf, just adding clang version comment since last review.
Tree-SHA512: a1d853675ec064170ee0f1cd16be6a900676588d4a1e7b5def8733933b140ba1a9520ec6f6a42bf7638b2ff7cf2fe4d5866d407f68b677b49d2bd68ff345f735
81e5c8385b test: cover invalid codesep positions for signature in taproot (Greg Sanders)
Pull request description:
There is some basic coverage, but I felt like adding some boundary conditions where the only issue is the codesep value would be nice.
ACKs for top commit:
ajtowns:
ACK 81e5c8385b
TheCharlatan:
ACK 81e5c8385b
Tree-SHA512: de74895c3bb49854987654720ebcefea2f47c4a55ba6ab4a52878f6a9a0bd8b3085afa3485101610327fa8d35c3d074542f58540e126460bd4bea918cb0054ee
