The piece of `p2p_private_broadcast.py` which tests the correctness of
`abortprivatebroadcast` issues a new `sendrawtransaction` call. That
call schedules up to 3 new connections: peer=13, peer=14 and possibly
peer=15 before it gets aborted.
These up to 3 in-the-process-of-opening private broadcast connections
have `CNode::m_connected` set early - when the `CNode` object is
created. Later in the test the mock time is advanced by 20 minutes and
those "old" connections pick a transaction for rebroadcast but that
triggers `PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME` immediately:
```
2026-02-21T13:28:14.209766Z [privbcast] [net.cpp:4006] [CNode] [net] Added connection peer=20
2026-02-21T13:28:14.309792Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net.cpp:4074] [PushMessage] [net] sending inv (37 bytes) peer=20
2026-02-21T13:28:14.309801Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net_processing.cpp:5745] [SendMessages] [privatebroadcast] Disconnecting: did not complete the transaction send within 180 seconds, peer=20
```
This prematurely stops the private broadcast connection and results in
a failure like:
```
AssertionError: ... not({} == {'ping': 1, 'tx': 1})
```
4b53cbd69220c1c786bb23a72c0b26a6f78a38f7 test: Test for musig() in various miniscript expressions (Ava Chow)
ec0f47b15cb3269015523e6fab8ae9241f4181a1 miniscript: Using Func and Expr when parsing keys, hashes, and locktimes (Ava Chow)
6fd780d4fbc497b657025afe48d0dfbf103ee120 descriptors: Increment key_exp_index in ParsePubkey(Inner) (Ava Chow)
b12281bd86e2298ba6cdd79d55c9d6e23e5136a5 miniscript: Use a reference to key_exp_index in KeyParser (Ava Chow)
ce4c66eb7c5e99e3df1c20d5c0ae8278a714b9f8 test: Test that key expression indexes match key count (Ava Chow)
Pull request description:
The miniscript parser currently only looks for the next `)` when parsing key, hash, and locktime expressions. This fails to parse when the expressions contain a nested expression. Currently, this is only possible with `musig()` inside of key expressions. However, this pattern can be generalized to handling hashes and locktimes, so I implemented those too.
Fixes#34076
ACKs for top commit:
rkrux:
ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7
sipa:
ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7
darosior:
Other than that, Approach ACK 4b53cbd69220c1c786bb23a72c0b26a6f78a38f7. That makes sense to me but i have not closely reviewed the code.
Tree-SHA512: 01040c7b07a59d8e3725ff11ab9543b256aea22535fb94059f490a5bb45319e859666af04c2f0a4edcb8cf1e6dfc7bd8a8271b21ad81143bafccd4d0a39cae9c
fd06157d1465d93b960e8be6e8e419295abde9a1 test: Add coverage for restarted node without any block sync (Fabian Jahr)
3d7ab7ecb7dfcdfb8aaa45869388887b948841c8 rpc, test: Address feedback from #29668 (Fabian Jahr)
312919c9dd5dba7da20317604e1638bdc5010f14 test: Indices can not start based on block data without undo data (Fabian Jahr)
a9a3b29dd687b4c355e131fefc145e8e48b48b17 index: Check availability of undo data for indices (Fabian Jahr)
881ab4fc82fe3cf36b227cf1ba704448df160745 support multiple block status checks in CheckBlockDataAvailability (furszy)
Pull request description:
Currently, we check that `BLOCK_HAVE_DATA` is available for all blocks an index needs to sync during startup. However, for `coinstatsindex` and `blockfilterindex` we also need the undo data for these blocks. If that data is missing in the blocks, we are currently still starting to sync each of these indices and then crash later when we encounter the missing data.
This PR adds explicit knowledge of which block data is needed for each index and then checks its availability during startup before initializing the sync process on them.
This also addresses a few open comments from #29668 in the last commit.
ACKs for top commit:
achow101:
ACK fd06157d1465d93b960e8be6e8e419295abde9a1
furszy:
utACK fd06157d1465d93b960e8be6e8e419295abde9a1
sedited:
Re-ACK fd06157d1465d93b960e8be6e8e419295abde9a1
Tree-SHA512: e2ed81c93372b02daa8ddf2819df4164f96d92de05b1d48855410ecac78d5fcd9612d7f0e63a9d57d7e75a0b46e1bea278e43ea87f2693af0220d1f9c600e416
231dd04b8dcb85ed1bc9191b839fb6cd64acea86 build: define CMAKE_COMPILE_WARNING_AS_ERROR as a cache option (will)
Pull request description:
`CMAKE_COMPILE_WARNING_AS_ERROR` is not a cache variable by default in CMake, so it has no value in the configure summary when not set, and even when set cannot be toggled in `ccmake`. Define it as an `option()` to make it a cache BOOL with a default of OFF.
From the original MR to cmake, this was deliberately not set as a cache variable: https://gitlab.kitware.com/cmake/cmake/-/merge_requests/7187 (see Brad King's reply to the collapsed comments from Marc Chevrier).
Most `CMAKE_*` variables which are expected to be toggled by users are (as far as I can research) cache variables by default. Those that are considered likely to be set by the project (e.g. `CMAKE_CXX_STANDARD` or `CMAKE_POSITION_INDEPENDENT_CODE`) are not, along with read-only variables, script/internal variables, platform sppecific variables, template variables. `CMAKE_COMPILE_WARNING_AS_ERROR` may be a slight outlier here.
I count ~ 600 documented CMAKE_* variables, of which ~ 60 are default cache variables.
I could only see a few of these like:
- CMAKE_COMPILE_WARNING_AS_ERROR
- CMAKE_CXX_STANDARD,
- CMAKE_POSITION_INDEPENDENT_CODE
- CMAKE_INTERPROCEDURAL_OPTIMIZATION
...that we (or any project) _might_ want to expose as user-togglable, and would have to add as an `option()` in CMakeLists.txt.
ACKs for top commit:
ajtowns:
Yeah, seems to work for me. ACK 231dd04b8dcb85ed1bc9191b839fb6cd64acea86
fanquake:
ACK 231dd04b8dcb85ed1bc9191b839fb6cd64acea86
hebasto:
ACK 231dd04b8dcb85ed1bc9191b839fb6cd64acea86, tested on Fedora 43.
Tree-SHA512: e380d79188859c97a9f7e45215a789daff55b8bff945a3d61fe4de3daae54bb7196aca0f6a6f1165979e787238dc26e94d217cff35f9642031ff72f4b39655e1
79c934b51cdca75971aecca8d89586522a5a9733 cmake: Fix NetBSD-specific workaround for Boost (Hennadii Stepanov)
Pull request description:
The recently merged https://github.com/bitcoin/bitcoin/pull/34143 broke builds with depends on NetBSD due to a workaround introduced in 5a5ddbd78922236402df378c8588a7b0b3f83a13.
The upstream [bug](https://gnats.netbsd.org/59856) has been fixed, and the entire workaround can be removed once the fixed Boost package becomes generally available.
However, it seems prudent to amend the workaround now to have it workable in the 31.0 release.
Here are CI runs:
- broken: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/21933683654
- fixed: https://github.com/hebasto/bitcoin-core-nightly/actions/runs/21933683654
ACKs for top commit:
fanquake:
ACK 79c934b51cdca75971aecca8d89586522a5a9733
Tree-SHA512: 360e4943fbedc7692fa2eba48f3375fc927d3aaef178dec6c0fafd3addfad0c89db9d1fe88e214313ebd285584bf8dc7d7b0a3a41d905c7f3d28293aa743405a
f700609e8ada3b48fd45ec19979cd72d943d47a6 doc: Release notes for mining IPC interface bump (Ryan Ofsky)
9453c153612ae9b30308362048099bc53afcde6f ipc mining: break compatibility with existing clients (version bump) (Sjors Provoost)
70de5cc2d205672743379f2e1a94290ee8b4b84b ipc mining: pass missing context to BlockTemplate methods (incompatible schema change) (Sjors Provoost)
2278f017afad4d2c570527b15df776ee64fc1ee2 ipc mining: remove deprecated methods (incompatible schema change) (Ryan Ofsky)
c6638fa7c5e97f9fd7a5ea8feb29f8caeac788bd ipc mining: provide default option values (incompatible schema change) (Ryan Ofsky)
a4603ac77412790b6498ab1750017e31353740bf ipc mining: declare constants for default field values (Ryan Ofsky)
ff995b50cf9e1ea521f3cf546339f05d10b79a4d ipc test: add workaround to block_reserved_weight exception test (Ryan Ofsky)
b970cdf20fce43fb58dde1cbf713e97ff21d7a2e test framework: expand expected_stderr, expected_ret_code options (Ryan Ofsky)
df53a3e5ec8781833c29682ff9e459fca489fa7b rpc refactor: stop using deprecated getCoinbaseCommitment method (Ryan Ofsky)
Pull request description:
This PR increments the field number of the `Init.makeMining` method and makes the old `makeMining` method return an error, so IPC mining clients not using the latest schema file will get an error and not be able to access the Mining interface.
Normally, there shouldn't be a need to break compatibility this way, but the mining interface has evolved a lot since it was first introduced, with old clients using the original methods less stable and performant than newer clients. So now is a good time to introduce a cutoff, drop deprecated methods, and stop supporting old clients which can't function as well.
Bumping the field number is also an opportunity to make other improvements that would be awkward to implement compatibly:
- Making Cap'n Proto default parameter and field values match default values of corresponding C++ methods and structs.
- Adding missing Context parameters to Mining.createNewBlock and checkBlock methods so these methods will be executed on separate execution threads and not block the Cap'n Proto event loop thread.
More details about these changes are in the commit messages.
ACKs for top commit:
Sjors:
ACK f700609e8ada3b48fd45ec19979cd72d943d47a6
enirox001:
ACK f700609e8ada3b48fd45ec19979cd72d943d47a6
ismaelsadeeq:
ACK f700609e8ada3b48fd45ec19979cd72d943d47a6
sedited:
ACK f700609e8ada3b48fd45ec19979cd72d943d47a6
Tree-SHA512: 0901886af00214c138643b33cec21647de5671dfff2021afe06d78dfd970664a844cde9a1e28f685bb27edccaf6e0c3f2d1e6bb4164bde6b84f42955946e366d
0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a Minimize mempool lock, sync txo spender index only when and if needed (sstone)
3d82ec5bdd019cf1c048c41fe44faa855fcb8b53 Add a "tx output spender" index (sstone)
Pull request description:
This PR adds a new "tx output spender" index, which allows users to query which tx spent a given outpoint with the `gettxspendingprevout` RPC call that was added by https://github.com/bitcoin/bitcoin/pull/24408.
Such an index would be extremely useful for Lightning, and probably for most layer-2 protocols that rely on chains of unpublished transactions.
UPDATE: this PR is ready for review and issues have been addressed:
- using a watch-only wallet instead would not work if there is a significant number of outpoints to watch (see https://github.com/bitcoin/bitcoin/pull/24539#issuecomment-1276595646)
- this PR does not require `-txindex` anymore
We use a composite key with 2 parts (suggested by romanz): hash(spent outpoint) and tx position, with an empty value. Average composite key size is 15 bytes.
The spending tx can optionally be returned by `gettxspendingprevout` (even it `-txindex is not set`).
ACKs for top commit:
hodlinator:
re-ACK 0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a
sedited:
Re-ACK 0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a
fjahr:
ACK 0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a
w0xlt:
reACK 0b96b9c600e0dd946fd4d0e827e7f7cbef7a571a
Tree-SHA512: 95c2c313ef4086e7d5bf1cf1a3c7b91cfe2bb1a0dcb4c9d3aa8a6e5bfde66aaca48d85a1f1251a780523c3e4356ec8a97fe6f5c7145bc6ccb6f820b26716ae01
* The bitcoin.conf related checks do not need any timeout, because the
logging happens in the main thread, before the node is fully started.
* The net thread related checks do need a timeout, because the threads
may be late to start after the node is fully started.
This is required to actually erase the orphan transaction when the
BlockConnected event is handled in the background validation interface
queue thread.
cae6d895f8a8cf5f57e05519536fda5d62b10841 fuzz: add target for CoinsViewOverlay (Andrew Toth)
86eda88c8e486eb1db724e60948f71349d050e1d fuzz: move backend mutating block to end of coins_view (Andrew Toth)
89824fb27b228a12d5c2f63106c2a4d793e73107 fuzz: pass coins_view_cache to TestCoinsView in coins_view (Andrew Toth)
73e99a59665551243d6dbe03a0e9baa9cab046b9 coins: don't mutate main cache when connecting block (Andrew Toth)
67c0d1798e6147f48d4bafc2c9e5ff30f2a62340 coins: introduce CoinsViewOverlay (Andrew Toth)
69b01af0eb9017a6ae7ca3134c9dcf89e74dbfa8 coins: add PeekCoin() (Andrew Toth)
Pull request description:
This is a slightly modified version of the first few commits of #31132, which can be merged as an independent change. It has a small benefit on its own, but will help in moving the parent PR forward.
When accessing coins via the `CCoinsViewCache`, methods like `GetCoin` can call `FetchCoin` which actually mutate `cacheCoins` internally to cache entries when they are pulled from the backing db. This is generally a performance improvement for single threaded access patterns, but it precludes us from accessing entries in a `CCoinsViewCache` from multiple threads without a lock.
Another aspect is that when we use the resettable `CCoinsViewCache` view backed by the main cache for use in `ConnectBlock()`, we will insert entries into the main cache even if the block is determined to be invalid. This is not the biggest concern, since an invalid block requires proof-of-work. But, an attacker could craft multiple invalid blocks to fill the main cache. This would make us `Flush` the cache more often than necessary. Obviously this would be very expensive to do on mainnet.
Introduce `CoinsViewOverlay`, a `CCoinsViewCache` subclass that reads coins without mutating the underlying cache via `FetchCoin()`.
Add `PeekCoin()` to look up a Coin through a stack of `CCoinsViewCache` layers without populating parent caches. This prevents the main cache from caching inputs pulled from disk for a block that has not yet been fully validated. Once `Flush()` is called on the view, these inputs will be added as spent to `coinsCache` in the main cache via `BatchWrite()`.
This is the foundation for async input fetching, where worker threads must not mutate shared state.
ACKs for top commit:
l0rinc:
ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
sipa:
reACK cae6d895f8a8cf5f57e05519536fda5d62b10841
sedited:
Re-ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
willcl-ark:
ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
vasild:
Cursory ACK cae6d895f8a8cf5f57e05519536fda5d62b10841
ryanofsky:
Code review ACK cae6d895f8a8cf5f57e05519536fda5d62b10841. PR is basically back to the form I had acked the first time, implementing `PeekCoin()` by calling `GetCoin()`. This is not ideal because `PeekCoin()` is not supposed to modify caches and `GetCoin()` does that, but it at least avoids problems of the subsequent approach tried where `GetCoin()` calls `PeekCoin` and would result in bugs when subclasses implement `GetCoin` forgetting to override `PeekCoin`. Hopefully #34124 can clean all of this by making relevant methods pure virtual.
Tree-SHA512: a81a98e60ca9e47454933ad879840cc226cb3b841bc36a4b746c34b350e07c546cdb5ddc55ec1ff66cf65d1ec503d22201d3dc12d4e82a8f4d386ccc52ba6441
afea2af1391314be0e5caaa0125c884da2405316 net: reduce log level for PCP/NAT-PMP NOT_AUTHORIZED failures (ANAVHEOBA)
Pull request description:
Cherry-picks (and tweaks) a commit from #34117 which the ANAVHEOBA
did not follow up with when changes were requested.
The tweak here is to log once at `LogWarning`, so that users have a chance
to spot misconfiguration.
----
Users running on home networks with routers that don't support PCP (Port
Control Protocol) or NAT-PMP port mapping receive frequent warning-level
log messages every few minutes:
"pcp: Mapping failed with result NOT_AUTHORIZED (code 2)"
This is expected behavior for many consumer routers that have PCP
disabled by default, not an actionable error.
Add explicit constants for the NOT_AUTHORIZED result code (value 2)
for both NAT-PMP and PCP protocols. Log the first NOT_AUTHORIZED
failure at warning level for visibility, then downgrade subsequent
occurrences to LogDebug to avoid log noise. Other failure types
continue to warn unconditionally.
Fixes#34114
ACKs for top commit:
achow101:
ACK afea2af1391314be0e5caaa0125c884da2405316
sedited:
ACK afea2af1391314be0e5caaa0125c884da2405316
Tree-SHA512: 43dad9f3cca0ef9b82446045a3ccd90418cd5595c9965e938d9d539bbba863dde6b4a358adbee56f8d97d6efbf947eb5ddbbaf426faefcf3b1e36e4c8edb0d94
fa90d44a22838591e47eec206c23f87d69ad842a test: Fix intermittent issues in feature_assumevalid.py (MarcoFalke)
Pull request description:
The test has many issues:
* The `send_blocks_until_disconnected` seems to imply to wait until a disconnect happens. However, in reality it will blindly send all blocks and then return early, when done or when a disconnect happens. This will cause test failures when python is running faster than Bitcoin Core, for example when using sanitizers.
* The `assert_debug_log` scopes are bloated, which makes finding the above issue harder. This is, because a test failure will basically print the 1000+ line debug log excerpt twice, with the second instance stripped of useful python test logs. Also, the checks are less precise, if they happen over a larger scope/snippet.
Fix all issues, by:
* Removing `send_blocks_until_disconnected` and just sending the blocks that are needed to get the disconnect and then explicitly waiting for the disconnect.
* Reduce the scopes of the debug log checks. This can be reviewed with the git options `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`.
ACKs for top commit:
l0rinc:
ACK fa90d44a22838591e47eec206c23f87d69ad842a
achow101:
ACK fa90d44a22838591e47eec206c23f87d69ad842a
Tree-SHA512: 8a18fa0009fb40a47317976e20b230625182874391babe0da3c3dfd55d2cc8e2da7008ce8f38691d01cd37ae02cac7c9e88b8193c2ed66d3621ee3f792649f71
2a1d0db7994eb2aa8527944f62161b6b8af682da doc: Mention private broadcast RPCs in release notes (Andrew Toth)
c3378be10b0a90e81b46e53eb85c41eb8caabac5 test: Cover abortprivatebroadcast in p2p_private_broadcast (Andrew Toth)
557260ca14ac5fb4732f4ce0692a2bf364bb5238 rpc: Add abortprivatebroadcast (Andrew Toth)
15dff452eb61ae9e2fd7b48c957e795c4c397443 test: Cover getprivatebroadcastinfo in p2p_private_broadcast (Andrew Toth)
996f20c18af02281034c51af4b2766d8f4d37a2c rpc: Add getprivatebroadcastinfo (Andrew Toth)
5e64982541f301773156a87988c60ca7797a3f06 net: Add PrivateBroadcast::GetBroadcastInfo (Andrew Toth)
573bb542be80b63b1713a0b76bedaa5e37c3783f net: Store recipient node address in private broadcast (Andrew Toth)
Pull request description:
Follow up from #29415
Sending a transaction via private broadcast does not have any way for a user to track the status of the transaction before it gets returned by another peer. The default logs have been removed as well in #34267. Nor is there any way to abort a transaction once it has been added to the private broadcast queue.
This adds two new RPCs:
- `getprivatebroadastinfo` returns information about what transactions are in the private broadcast queue, including all the peers' addresses we have chosen and timestamps.
- `abortprivatebroadcast` stops broadcasting a transaction in the private broadcast queue.
ACKs for top commit:
nervana21:
tACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
achow101:
ACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
l0rinc:
ACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
danielabrozzoni:
tACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
sedited:
ACK 2a1d0db7994eb2aa8527944f62161b6b8af682da
Tree-SHA512: cc8682d0be68a57b42bea6e3d091da2b80995d9e6d3b98644cb120a05c2b48a97c2e211173289b758c4f4e23f1d1a1f9be528a9b8c6644f71d1dd0ae5f673326
afb1bc120ecce2bf663093e15c93f5592c0d4a98 validation: Use dirty entry count in flush warnings and disk space checks (Pieter Wuille)
b413491a1cdd9a51f2aa10b775650f54f6785e3e coins: Keep track of number of dirty entries in `CCoinsViewCache` (Pieter Wuille)
7e52b1b945c4137e0fb05715090635ce82ed04b3 fuzz: call `EmplaceCoinInternalDANGER` as well in `SimulationTest` (Lőrinc)
Pull request description:
### Problem
Now that non-wiping flushes are possible (#28280, #28233), the cache may be mostly clean at flush time.
But the flush warning, disk-space check, and benchmark logging still used total cache size, so a node with a 10 GiB cache that only needs to write a small fraction of dirty entries could still trigger a scary warning via the disk-space checks.
The previous `DynamicMemoryUsage` metric was also fundamentally wrong for estimating disk writes, even before non-wiping flushes. In-memory coin size differs from on-disk write size due to LevelDB overhead, log doubling, and compaction.
The warning also only fired in `FlushStateToDisk`, so `AssumeUTXO` snapshot loads never warned at all.
### Fix
This PR tracks the actual number of dirty entries via `m_dirty_count` in `CCoinsViewCache`, maintained alongside the existing dirty-flag linked list, `SanityCheck` cross-validating both counts.
The warning and benchmark log move from `FlushStateToDisk` down to `CCoinsViewDB::BatchWrite`, where the actual I/O happens. This is the single place all flush paths converge (regular flushes, syncs, and snapshot loads), so the warning now fires correctly for `AssumeUTXO` too.
The threshold changes from 1 GiB of memory to 10 million dirty entries, which is roughly equivalent but avoids the in-memory vs on-disk size confusion.
The disk-space safety check now uses `GetDirtyCount()` with the existing conservative 48-byte-per-entry estimate, preventing unnecessary shutdowns when the cache is large but mostly clean.
---
Note: the first commit adds fuzz coverage for `EmplaceCoinInternalDANGER` in `SimulationTest` to exercise the accounting paths before modifying them.
Note: this is a revival of #31703 with all outstanding review feedback addressed.
ACKs for top commit:
Eunovo:
Concept ACK afb1bc120e
andrewtoth:
re-ACK afb1bc120ecce2bf663093e15c93f5592c0d4a98
sipa:
Code review ACK afb1bc120ecce2bf663093e15c93f5592c0d4a98
sedited:
ACK afb1bc120ecce2bf663093e15c93f5592c0d4a98
Tree-SHA512: 4133c6669fd20836ae2fb62ed804cdf6ebaa61076927b54fc412e42455a2f0d4cadfab0844064f9c32431eacb1f5e47b78de8e5cde1b26ba7239a7becf92f369
726b3663cc8e2164d4e9452f12f5866f5e8f6f1a http: properly respond to HTTP request during shutdown (furszy)
59d24bd5dd2a4549888cf7c557461e6b4959f82f threadpool: make Submit return Expected instead of throwing (furszy)
Pull request description:
Fixes#34573.
As mentioned in https://github.com/bitcoin/bitcoin/issues/34573#issuecomment-3891596958, the ThreadPool PR (#33689) revealed an existing issue.
Before that PR, we were returning an incorrect error "Request rejected because http work queue depth exceeded" during shutdown for unhandled requests (we were not differentiating between "queue depth exceeded" and "server interrupted" errors). Now, with the ThreadPool inclusion, we return the proper error but we don't handle it properly.
This PR improves exactly that. Handling the missing error and properly returning it to the user.
The race can be reproduced as follows:
1) The server receives an http request.
2) Processing of the request is delayed, and shutdown is triggered in the meantime.
3) During shutdown, the libevent callback is unregistered and the threadpool interrupted.
4) The delayed request (step 2) resumes and tries to submit a task to the now-interrupted server.
Reproduction test can be found https://github.com/bitcoin/bitcoin/pull/34577#issuecomment-3902672521.
Also, to prevent this kind of issue from happening again, this PR changes task submission
to return the error as part of the function's return value using `util::Expected` instead of
throwing the exception. Unlike exceptions, which require extra try-catch blocks and can be
ignored, returning `Expected` forces callers to explicitly handle failures, and attributes
like `[[nodiscard]]` allow us catch unhandled ones at compile time.
ACKs for top commit:
achow101:
ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
sedited:
ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
pinheadmz:
re-ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
andrewtoth:
ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
hodlinator:
re-ACK 726b3663cc8e2164d4e9452f12f5866f5e8f6f1a
Tree-SHA512: ef026e299adde1148c9fc575e7d937e957bf0ddedfc1cf081941b568736417c2eefcd8bc8c8aea795d7347040ed05da4371bddcdbda7d385e04bf4dc8d875780
We sync txospenderindex after we've checked the mempool for spending transaction, and only if search is not limited to the mempool and no
spending transactions have been found for some of the provided outpoints.
This should minimize the chance of having a block containing a spending transaction that is no longer in the mempool but has not been indexed yet.
8966352df3fc56fd2c00a45eecd292a240a34546 doc: add release notes (ismaelsadeeq)
704a09fe7187d5e4c949dea05baba7fe13bdb676 test: ensure fee estimator provide fee rate estimate < 1 s/vb (ismaelsadeeq)
243e48cf493378acd3a4bde638765544ade9f7b2 fees: delete unused dummy field (ismaelsadeeq)
fc4fbda42af1e84f74acbd8b6a0f384d2711c85b fees: bump fees file version (ismaelsadeeq)
b54dedcc8563286861b2ccda68bc246ad61338c0 fees: reduce `MIN_BUCKET_FEERATE` to 100 (ismaelsadeeq)
Pull request description:
This is a simple PR that updates the block policy estimator’s `MIN_BUCKET_FEERATE` constant to be 100, which is identical to the policy `DEFAULT_MIN_RELAY_TX_FEE`.
This change enables the block policy fee rate estimator to return sub-1 sat/vB fee rate estimates.
The change is correct because the estimator creates buckets of fee rates from
`MIN_BUCKET_FEERATE`,
`MIN_BUCKET_FEERATE` + `FEE_SPACING`,
`MIN_BUCKET_FEERATE` + `2 * FEE_SPACING`,
… up to `MAX_BUCKET_FEERATE`.
This means it will record sub-1 sat/vB fee rates in the buckets and may return them as a fee rate estimate when that bucket is the lowest one with sufficient transactions for a given target.
---
While touching this part of the fee estimator code, this PR got rid of the dummy value persisted in the file
ACKs for top commit:
achow101:
ACK 8966352df3fc56fd2c00a45eecd292a240a34546
musaHaruna:
ACK [8966352](8966352df3)
polespinasa:
ACK 8966352df3fc56fd2c00a45eecd292a240a34546
Tree-SHA512: 9424e0820fcbe124adf5e5d9101a8a2983ba802887101875b2d1856d700c8b563d7a6f6ca3e3db29cb939f786719603aadbf5480d59d55d0951ed1c0caa49868
e0463b4e8c25f8a5fe10999f2821e7b221d2e40a rpc: add coinbase_tx field to getblock (Sjors Provoost)
Pull request description:
This adds a `coinbase_tx` field to the `getblock` RPC result, starting at verbosity level 1. It contains only fields guaranteed to be small, i.e. not the outputs.
Initial motivation for this was to more efficiently scan for BIP54 compliance. Without this change, it requires verbosity level 2 to get the coinbase, which makes such scan very slow. See https://github.com/bitcoin-inquisition/bitcoin/pull/99#issuecomment-3852370506.
Adding these fields should be useful in general though and hardly makes the verbosity 1 result longer.
```
bitcoin rpc help getblock
getblock "blockhash" ( verbosity )
If verbosity is 0, returns a string that is serialized, hex-encoded data for block 'hash'.
If verbosity is 1, returns an Object with information about block <hash>.
If verbosity is 2, returns an Object with information about block <hash> and information about each transaction.
...
Result (for verbosity = 1):
{ (json object)
"hash" : "hex", (string) the block hash (same as provided)
"confirmations" : n, (numeric) The number of confirmations, or -1 if the block is not on the main chain
"size" : n, (numeric) The block size
"strippedsize" : n, (numeric) The block size excluding witness data
"weight" : n, (numeric) The block weight as defined in BIP 141
"coinbase_tx" : { (json object) Coinbase transaction metadata
"version" : n, (numeric) The coinbase transaction version
"locktime" : n, (numeric) The coinbase transaction's locktime (nLockTime)
"sequence" : n, (numeric) The coinbase input's sequence number (nSequence)
"coinbase" : "hex", (string) The coinbase input's script
"witness" : "hex" (string, optional) The coinbase input's first (and only) witness stack element, if present
},
"height" : n, (numeric) The block height or index
"version" : n, (numeric) The block version
...
```
```
bitcoin rpc getblock 000000000000000000013c986f9aebe800a78454c835ccd07ecae2650bfad3f6 1
```
```json
{
"hash": "000000000000000000013c986f9aebe800a78454c835ccd07ecae2650bfad3f6",
"confirmations": 2,
"height": 935113,
"version": 561807360,
"...": "...",
"weight": 3993624,
"coinbase_tx": {
"version": 2,
"locktime": 0,
"sequence": 4294967295,
"coinbase": "03c9440e04307c84692f466f756e6472792055534120506f6f6c202364726f70676f6c642ffabe6d6d9a8624235259d3680c972b0dd42fa3fe1c45c5e5ae5a96fe10c182bda17080e70100000000000000184b17d3f138020000000000",
"witness": "0000000000000000000000000000000000000000000000000000000000000000"
},
"tx": [
"70eb053340c7978c5aa1b34d75e1ba9f9d1879c09896317f306f30c243536b62",
"5bcf8ed2900cb70721e808b8977898e47f2c9001fcee83c3ccd29e51c7775dcd",
"3f1991771aef846d7bb379d2931cccc04e8421a630ec9f52d22449d028d2e7f4",
"..."
]
}
```
ACKs for top commit:
sedited:
Re-ACK e0463b4e8c25f8a5fe10999f2821e7b221d2e40a
darosior:
re-utACK e0463b4e8c25f8a5fe10999f2821e7b221d2e40a
Tree-SHA512: 1b3e7111e6a0edffde8619c49b3e9bca833c8e90e416defc66811bd56dd00d45b69a84c8fd9715508f4e6515f77ac4fb5c59868ab997ae111017c78c05b74ba3
1b36bf0c5d71ea2e7e3595eb10611ea1a8c3a0d1 subprocess: Fix `-Wunused-private-field` for `Child` class on Windows (Hennadii Stepanov)
9f2b338bc01832e335f652e23f1318ee44cdd63c subprocess: Fix `-Wunused-private-field` for `Popen` class on Windows (Hennadii Stepanov)
Pull request description:
This PR is a prerequisite for https://github.com/bitcoin/bitcoin/pull/31507.
It resolves `-Wunused-private-field` warnings triggered in `src/util/subprocess.h` when compiling with clang-cl on Windows:
```
D:\a\bitcoin\bitcoin\src\util/subprocess.h(759,10): warning : private field 'parent_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(760,7): warning : private field 'err_wr_pipe_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
D:\a\bitcoin\bitcoin\src\util/subprocess.h(1038,7): warning : private field 'child_pid_' is not used [-Wunused-private-field] [D:\a\bitcoin\bitcoin\build\src\util\bitcoin_util.vcxproj]
```
Only the second commit has been [submitted](https://github.com/arun11299/cpp-subprocess/pull/127) upstream. The first commit is specific to this repository and not applicable upstream.
ACKs for top commit:
maflcko:
review ACK 1b36bf0c5d71ea2e7e3595eb10611ea1a8c3a0d1 👋
purpleKarrot:
ACK 1b36bf0c5d71ea2e7e3595eb10611ea1a8c3a0d1
sedited:
ACK 1b36bf0c5d71ea2e7e3595eb10611ea1a8c3a0d1
Tree-SHA512: 1bc0544d769264fa74d2f39150595ee6339af4bca7b7051ecaecbe234c17b643b715e00cfb9302a16ffc4856957f4fa47c216aebf03fec0cd95c387f51bd29a6
fa5672dcafa154dff7409eaaf762febe1d76aad7 refactor: [gui] Use SettingTo<int64_t> over deprecated SettingToInt (MarcoFalke)
fac3ecaf69d6f2d655e71644c98364206f7e2ddc rpc: Properly parse -rpcworkqueue/-rpcthreads (MarcoFalke)
faee36f63b5fde886458d0415778719ea2233d14 util: Add SettingTo<Int>() and GetArg<Int>() (MarcoFalke)
Pull request description:
The integral arg parsing has many issues:
* There is no way to parse an unsigned integral type at all
* There is no way to parse an integral type of less width than int64_t
* As a result, calling code splatters confusing c-style casts just to let the code compile. However, usually there are no range checks and proper range handling.
For example, when someone (maybe for testing) wants to set the rpc work queue to the maximum possible number, there is no easy way to do so without reading the source code and manually crafting the exact integer value. Using the "9999 hack" will silently set it to `-1` (!)
To test:
`/bld-cmake/bin/bitcoin-qt -datadir=/tmp -regtest -rpcworkqueue=99999999999999999999999999 -printtoconsole=1 -server=1 -debug=http | grep 'set work queue of depth'`
Before:
```
[http] set work queue of depth -1
```
After:
```
[http] set work queue of depth 2147483647
ACKs for top commit:
stickies-v:
ACK fa5672dcafa154dff7409eaaf762febe1d76aad7
pinheadmz:
ACK fa5672dcafa154dff7409eaaf762febe1d76aad7
sedited:
ACK fa5672dcafa154dff7409eaaf762febe1d76aad7
Tree-SHA512: e5060453a0aa1c4e27080e928b0ae2d1015fe487246e4059866eef415f301bc7712ce306d95076ce5b66a5e57c620715b33998192c0ff06b0384085a0390c714
This adds a "coinbase_tx" field to the getblock RPC result, starting
at verbosity level 1. It contains only fields guaranteed to be small,
i.e. not the outputs.
a099655f2e1be313a6b51bba9799ea512c6eda3c scripted-diff: Update `DeriveType` enum values to mention ranged derivations (rkrux)
Pull request description:
While reviewing the MuSig2 descriptors PR #31244, I realized that the enum
`DeriveType` here logically refers to the derive type for ranged descriptors.
This became evident to me while going through the implementations of `IsRange`
& `IsHardened` functions of `BIP32PubkeyProvider`, and the `ParsePubkeyInner`
function. Initially I got confused by reading `IsRange` translating to
`!= DeriveType::NO`, but later realised it specifically referred to the presence
of ranged derivations. I propose explicitly mentioning "ranged" in the values
of the `DeriveType` enum would make it easier to parse the descriptors code.
This enum is used in one file only - `script/descriptors.cpp`. That's why I
explicitly passed it as the argument in the `sed` command in the script.
ACKs for top commit:
hodlinator:
re-ACK a099655f2e1be313a6b51bba9799ea512c6eda3c
pablomartin4btc:
ACK a099655f2e1be313a6b51bba9799ea512c6eda3c
PeterWrighten:
ACK a099655
Tree-SHA512: 03f11e5a37edd4f92b7113c13cdeabb11c62cc5d836874f9a4eee107362d64a1745e6a65079033dc260a58d8693bccc9dce9c18e9433a05258e8a6b34242514c
Adds an outpoint -> txid index, which can be used to find which transactions spent a given output.
We use a composite key with 2 parts (suggested by @romanz): hash(spent outpoint) and tx position, with an empty value.
To find the spending tx for a given outpoint, we do a prefix search (prefix being the hash of the provided outpoint), and for all keys that match this prefix
we load the tx at the position specified in the key and return it, along with the block hash, if does spend the provided outpoint.
To handle reorgs we just erase the keys computed from the removed block.
This index is extremely useful for Lightning and more generally for layer-2 protocols that rely on chains of unpublished transactions.
If enabled, this index will be used by `gettxspendingprevout` when it does not find a spending transaction in the mempool.
62e378584e77cb8382facc6a7c18133d1cf9ffb3 guix: don't export TZ twice (fanquake)
badcf1c68dbf9bdb796e7b79459343c22ae20965 guix: fix typo in guix-codesign (fanquake)
Pull request description:
Remove a double export of `TZ` and fix a typo.
ACKs for top commit:
janb84:
ACK 62e378584e77cb8382facc6a7c18133d1cf9ffb3
hebasto:
ACK 62e378584e77cb8382facc6a7c18133d1cf9ffb3, I have reviewed the code and it looks OK.
Tree-SHA512: 6e0b66d20db2b0535bea7d29a28782d0ddd423eaace5f2aa708a227985d465e33def10e9f8ac5ea5482fe640ea0df4ca9df8e9f417bf5d040867564fc60aebd0
24699fec8422a4d9219f8c5272370351e7adea7f doc: Add initial asmap data documentation (Fabian Jahr)
bab085d282b1ad1790861d710fd570f8531c9364 ci: Use without embedded asmap build option in one ci job (Fabian Jahr)
e53934422a29bdcb022d32f8eb6e171218cd3a26 doc: Expand documentation on asmap feature and tooling (Fabian Jahr)
6244212a5532a8a625e344fdbc8144f4befdd385 init, net: Implement usage of binary-embedded asmap data (Fabian Jahr)
6202b50fb9003a4feadd879ae189ee6f730e8155 build: Generate ip_asn.dat.h during build process (Fabian Jahr)
634cd60dc8f646b25701c45ac35a1175ce4c4da9 build: Add embedded asmap data (Fabian Jahr)
Pull request description:
This is the final in a series of PRs that implement the necessary changes for embedding of asmap data into the binary. This last part add the initial asmap data, implements the build changes and adds further documentation.
Currently an asmap file needs to be acquired by there user from some location or the user needs to generate one themselves. Then they need to move the file to the right place in datadir or pass the path to the file as `-asmap=PATH` in order to use the asmap feature. The change here allows for builds to embed asmap data into the bitcoind binary which makes it possible to use the feature without handling of the asmap file by the user. If the user starts bitcoind with `-asmap` the embedded data will be used for bucketing of nodes.
The data lives in the repository at `src/node/data/ip_asn.dat` and can be replaced with a new version at any time. The idea is that the data should be updated with every release. By default the data at that location is embedded into the binary but there is also a build option to prevent this (`-DWITH_EMBEDDED_ASMAP=OFF`). In this case the original behavior of the `-asmap` option is maintained.
ACKs for top commit:
achow101:
ACK 24699fec8422a4d9219f8c5272370351e7adea7f
sipa:
ACK 24699fec8422a4d9219f8c5272370351e7adea7f
hodlinator:
ACK 24699fec8422a4d9219f8c5272370351e7adea7f
Tree-SHA512: c2e33dbeea387efdfd3d415432bf8fa64de80f272c1207015ea53b85bb77f5c29f1dae5644513a23c844a98fb0a4bb257bf765f38b15bfc4c41984f0315b4c6a
24f93c9af7f6627cd7d09a1a5f10667846b048eb release note (Pol Espinasa)
331a5279d2775fb701a0bf4607436ec05e476df3 wallet, rpc:remove settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR removes the settxfee RPC and paytxfee setting (Bitcoin Core 31.0).
These two features were deprecated in https://github.com/bitcoin/bitcoin/pull/31278.
ACKs for top commit:
achow101:
ACK 24f93c9af7f6627cd7d09a1a5f10667846b048eb
w0xlt:
reACK 24f93c9af7f6627cd7d09a1a5f10667846b048eb
Tree-SHA512: e090f1a72ba2cbeba7c982dd51dfdcf6baf0a164827337cf56fd85f733e143b8d6116b8cd53c59c812cacef193dfa0b101a830fc455e32bf225e8505e7b2a554
Makes sure we respond to the client as the HTTP request attempts to submit a task to
the thread pool during server shutdown.
Roughly what happens:
1) The server receives an HTTP request and starts calling http_request_cb().
2) Meanwhile on another thread, shutdown is triggered which calls InterruptHTTPServer()
and unregisters libevent http_request_cb() callback and interrupts the thread pool.
3) The request (step 1) resumes and tries to submit a task to the now-interrupted server.
This fix detects failed submissions immediately, and the server responds with
HTTP_SERVICE_UNAVAILABLE.
6df4a045f79767a7f459e5232c722882396f15c6 qt: Replace three dots with ellipsis (Hennadii Stepanov)
Pull request description:
From the translator's [comment](https://app.transifex.com/bitcoin/bitcoin/translate/#pl/qt-translation-031x/611215465/) on Transifex:
> Source text has 3 dots, instead of 3-dot character. Most commonly used form is the 3-dot character: …
You could consider updating it.
Top commit has no ACKs.
Tree-SHA512: 73ca2d574798b682abca352346fd3664293f14b66c16b01d1a10128e840072fae883b65db14b86e8a4dffdd849563f4c9412d99baeb9735c919d3629ad9799f7
fa4cb96bdec21319cc54c6dd96a14acf1719e3c6 test: Set assert_debug_log timeout to 0 (MarcoFalke)
Pull request description:
The `assert_debug_log` helper is meant to be a context manager. However, it has a default timeout of 2 seconds, and can act as a silent polling/wait/sync function. This is confusing and brittle, and leads to intermittent bugs such as https://github.com/bitcoin/bitcoin/pull/34571.
Fix all issues, by setting the default timeout to zero. Then adjust all call sites to either use this correctly like a context manager, or adjust them explicitly to specify a timeout, to document that this is a polling sync function.
ACKs for top commit:
hodlinator:
re-ACK fa4cb96bdec21319cc54c6dd96a14acf1719e3c6
brunoerg:
ACK fa4cb96bdec21319cc54c6dd96a14acf1719e3c6
Tree-SHA512: 111a45c84327c3afea98fd9f8de13dd7d11969b2309471b4ad21694b290a3734f6e1eda75a41b39613b0995c5b7075298085cea2ca06aa07d4385eb8d72fe95b
fb3e1bf9c9772631571ca46d29c50330ebf54dfd test: check LoadBlockIndex correctly recomputes invalidity flags (stratospher)
29740c06ac53f55f71acf2a1b42b193aac39f579 validation: remove BLOCK_FAILED_MASK (stratospher)
b5b2956bda32b7b4ebc25c83b4d792ecd01f02b4 validation: reset BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID when loading from disk (stratospher)
37bc207852788340dc2a1b33a73748f43226978a validation: stop using BLOCK_FAILED_CHILD (stratospher)
120c631e16893821ea4c73ff70ac60e4fec0429f refactor: use clearer variables in InvalidateBlock() (stratospher)
18f11695c755c379ca67ca0bce8d17492ad9af18 validation: don't update BLOCK_FAILED_VALID to BLOCK_FAILED_CHILD in InvalidateBlock (stratospher)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/32173
even though we have a distinction between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` in the codebase,
we don't use it for anything. Whenever we check for BlockStatus, we use `BLOCK_FAILED_MASK` which encompasses both of them.
Since there is no functional difference between `BLOCK_FAILED_VALID` and `BLOCK_FAILED_CHILD` and it's added
code complexity to correctly categorise them (ex: https://github.com/bitcoin/bitcoin/pull/31405#discussion_r1914366243, https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-565506585), we could just remove it.
Looking for conceptual feedback on whether it's better to improve handling of `BLOCK_FAILED_CHILD` in the codebase or remove `BLOCK_FAILED_CHILD`.
Of less relevance, but it would also fix a `reconsiderblock` crash that could happen in the situation mentioned in https://github.com/bitcoin/bitcoin/issues/32173#issuecomment-2767030982
Similar attempt in the past in https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-568073859
ACKs for top commit:
stickies-v:
re-ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd
alexanderwiederin:
ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd
mzumsande:
re-ACK fb3e1bf9c9772631571ca46d29c50330ebf54dfd
Tree-SHA512: e97b739885c40a8c021966438e9767cc02bc183056236d6a8c64f6819347ae70c0fbcd71cc2528917560d9f4fd56aed45faf1b6c75d98de7b08b621693a97fbc
fa48d421636c256069010bc03c121c36ed9c0a0c test: Stricter unit test (MarcoFalke)
fa626bd143419a7141311e84490aacd8a6691c33 util: Remove brittle and confusing sp::Popen(std::string) (MarcoFalke)
Pull request description:
The subprocess Popen call that accepts a full `std::string` has many issues:
* It promotes brittle and broken code, where spaces are not properly quoted. Example: https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065
* The internally used `util::split` function does incorrectly split on spaces, instead of using `shlex.split`.
* It is redundant and not needed, because a vector interface already exists.
Fix all issues by removing it and just using the vector interface.
This pull request should not change any behavior: Note that the command taken from `gArgs.GetArg("-signer", "")` is still passed through the `sp::util::split` helper, just like before. Fixing that is left for a follow-up, so that this change here is basically just a refactor.
This also fixes a unit test bug as a side-effect: Fixes https://github.com/bitcoin/bitcoin/issues/32574.
ACKs for top commit:
janb84:
cr ACK fa48d421636c256069010bc03c121c36ed9c0a0c
fjahr:
Code review ACK fa48d421636c256069010bc03c121c36ed9c0a0c
hebasto:
re-ACK fa48d421636c256069010bc03c121c36ed9c0a0c.
Tree-SHA512: 3d29226977c9392502f9361e2bd42b471ad03761bbf6a94ef6e545cbe4492ad5858da1ac9cc64b2791aacb9b6e6f3c3f63dbcc3a2bf45f6a13b5bc33eddf8c2b
c2fcf250697325636218225d578c3844ab9ca633 clusterlin: inline GetReachable into Deactivate (optimization) (Pieter Wuille)
d90f98ab4aaaa6d524d8a2ab9fb3a8ba162ebb00 clusterlin: inline UpdateChunk into (De)Activate (optimization) (Pieter Wuille)
b684f954bbfcd9a18c05110b1124276554ba072e clusterlin: unidirectional MakeTopological initially (optimization) (Pieter Wuille)
1daa600c1ca82d16dd1661292949e56b4c4c6d94 clusterlin: track suboptimal chunks (optimization) (Pieter Wuille)
63b06d5523f1da37f76dc1f8b659959a127975f3 clusterlin: keep track of active children (optimization) (Pieter Wuille)
ae16485aa94dc745556615d569914b75ea855f02 clusterlin: special-case self-merges (optimization) (Pieter Wuille)
3221f1a074e7b313e97f969d11ff8a13a1dc5aa6 clusterlin: make MergeSequence take SetIdx (simplification) (Pieter Wuille)
7194de3f7c5324a6e719ed25ef292d162b4f5d88 clusterlin: precompute reachable sets (optimization) (Pieter Wuille)
6f898dbb8bfad68b3c2168471ced211222d6f7f3 clusterlin: simplify PickMergeCandidate (optimization) (Pieter Wuille)
dcf458ffb99c57c31c5c53e001c98fc1a8baadec clusterlin: split up OptimizeStep (refactor) (Pieter Wuille)
cbd684a4713dbe1adc77986e5924867f0e22fb6a clusterlin: abstract out functions from MergeStep (refactor) (Pieter Wuille)
b75574a6531ebe1f4077482705ad9e2db0ed3438 clusterlin: improve TxData::dep_top_idx type (optimization) (Pieter Wuille)
73cbd15d457221e2e896549a32df7f5a59a9df5c clusterlin: get rid of DepData (optimization) (Pieter Wuille)
7c6f63a8a9dc7e3d168dcb023f55936cd47ccf0b clusterlin: pool SetInfos (preparation) (Pieter Wuille)
20e2f3e96df31ffd32f0752e28d90de30942f5ed scripted-diff: rename _rep -> _idx in SFL (Pieter Wuille)
268fcb6a53ec034ec12a9c2cbaceca677664962b clusterlin: add more Assumes and sanity checks (tests) (Pieter Wuille)
d69c9f56ea96e333dbf2f5b4b4e63685147fbc72 clusterlin: count chunk deps without loop (optimization) (Pieter Wuille)
f66fa69ce008e512af8d7cc6b22f0b3a0a080b2c clusterlin: split tx/chunk dep counting (preparation) (Pieter Wuille)
900e45977889f9a189036f7b0e562f8e404c7190 clusterlin: avoid depgraph argument in SanityCheck (cleanup) (Pieter Wuille)
666b37970f1566b7b369c7c5f840099fa6eda800 clusterlin: fix type to count dependencies (Pieter Wuille)
Pull request description:
Follow-up to #32545, part of #30289.
This contains many of the optimizations that were originally part of #32545 itself.
Here is a list of commits and the geometric average of the benchmark timings. Note that these aren't worst cases, but because of the nature of the optimizations here, I do expect them to apply roughly equally to all kinds of clusters. In other words, the relative improvement shown by these numbers should be representative:
| commit title | ns per optimal linearization |
|:--|--:|
| clusterlin: split tx/chunk dep counting (preparation) | 24,760.30 |
| clusterlin: count chunk deps without loop (optimization) | 24,677.64 |
| scripted-diff: rename _rep -> _idx in SFL | 24,640.08 |
| clusterlin: get rid of DepData, reuse sets (optimization) | 24,389.01 |
| clusterlin: improve TxData::dep_top_idx type (optimization) | 22,578.58 |
| clusterlin: abstract out functions from MergeStep (refactor) | 22,577.15 |
| clusterlin: split up OptimizeStep (refactor) | 22,981.11 |
| clusterlin: simplify PickMergeCandidate (optimization) | 22,018.63 |
| clusterlin: precompute reachable sets (optimization) | 21,194.91 |
| clusterlin: make MergeSequence take SetIdx (simplification) | 21,135.60 |
| clusterlin: special-case self-merges (optimization) | 20,588.13 |
| clusterlin: keep track of active children (optimization) | 13,911.22 |
| clusterlin: track suboptimal chunks (optimization) | 13,629.42 |
| clusterlin: unidirectional MakeTopological initially (optimization) | 12,796.57 |
| clusterlin: inline UpdateChunk into (De)Activate (optimization) | 12,706.35 |
| clusterlin: inline GetReachable into Deactivate (optimization) | 12,525.66 |
And to show that they apply to all clusters roughly similarly:
<img width="1239" height="640" alt="output(1)" src="https://github.com/user-attachments/assets/edd73937-3f87-4582-b2b9-eaed7e6ff352" />
ACKs for top commit:
instagibbs:
reACK c2fcf250697325636218225d578c3844ab9ca633
ajtowns:
reACK c2fcf250697325636218225d578c3844ab9ca633
Tree-SHA512: e8920f7952a6681b4c1d70c864bd0e9784127ae4fd7c740be3e24a473f72e83544d2293066ed9b3e685b755febd6bbbc6c7da0c9b6ef3699b05eaa8d5bc073a0
Unlike exceptions, which can be ignored as they require extra try-catch
blocks, returning expected errors forces callers to always handle
submission failures.
Not throwing an exception also fixes an unclean shutdown bug
#34573 since we no longer throw when attempting to Submit()
from the libevent callback http_request_cb().
Add a test for block index transitioning from legacy
BLOCK_FAILED_CHILD to BLOCK_FAILED_VALID behavior.
In the scenario where a valid block has a BLOCK_FAILED_CHILD
parent and a BLOCK_FAILED_VALID grandparent, ensure that all
three blocks are correctly marked as BLOCK_FAILED_VALID
after reloading the block index.
- there maybe existing block indexes stored in disk with
BLOCK_FAILED_CHILD
- since they don't exist anymore, clean up block index entries with
BLOCK_FAILED_CHILD and reset it to BLOCK_FAILED_VALID.
even though we have a distinction between BLOCK_FAILED_VALID
and BLOCK_FAILED_CHILD in the codebase, we don't use it for
anything. since there's no functional difference between them
and it's unnecessary code complexity to categorise them correctly,
just mark as BLOCK_FAILED_VALID instead.
Improve upon the variable name for `invalid_walk_tip` to make the
InvalidateBlock logic easier to read. Block tip before disconnection
is now tracked directly via `disconnected_tip`, and `new_tip`
is the tip after the disconnect.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Avoid two full iterations over all of a chunks' transactions to
recompute the reachable sets, by inlining them into the
dependency-updating loops.
Note that there is no need to do the same for Activate, because the
reachable sets after merging can be computed directly from the input
chunks' reachable sets. Deactivate needs to recompute them, however.
The two calls to UpdateChunk, in Activate and Deactive each, are subtly
different: the top one needs to update the chunk_idx of iterated
transactions, while the bottom one leaves it unchanged. To exploit this
difference, inline the four function calls, getting rid of UpdateChunks.
This is also a preparation for a future improvement that inlines the
recomputation of reachable sets in the same loop in Deactivate.
It suffices to initially only attempt one direction of merges in
MakeTopological(), and only try both directions on chunks that are the
result of other merges.
This means we can iterate over all active dependencies in a
cluster/chunk in O(ntx) time rather than O(ndeps) (*), as the number of
active dependencies in a set of transactions of size is at most ntx-1.
(*) Asymptotically, this is not actually true, as for large transaction
counts, iterating over a BitSet still scales with ntx. In practice
however, where BitSets are represented by a constant number of integers,
it holds.
