3dd815f048c80c9a35f01972e0537eb42531aec7 validation: pre-reserve leaves to prevent reallocs with odd vtx count (Lőrinc)
7fd47e0e56087cd3b5fd76a532bdc3ac331e832e bench: make `MerkleRoot` benchmark more representative (Lőrinc)
f0a21831087410687c4ca31ac00e44f380b859be test: adjust `ComputeMerkleRoot` tests (Lőrinc)
Pull request description:
#### Summary
`ComputeMerkleRoot` [duplicates the last hash](39b6c139bd/src/consensus/merkle.cpp (L54-L56)) when the input size is odd. If the caller provides a `std::vector` whose capacity equals its size, that extra `push_back` forces a reallocation, doubling its capacity (causing peak memory usage of 3x the necessary size).
This affects roughly half of the created blocks (those with odd transaction counts), causing unnecessary memory fragmentation during every block validation.
#### Fix
* Pre-reserves vector capacity to account for the odd-count duplication using `(size + 1) & ~1ULL`.
* This syntax produces [optimal assembly](https://github.com/bitcoin/bitcoin/pull/32497#discussion_r2553107836) across x86/ARM and 32/64-bit platforms for GCC & Clang.
* Eliminates default construction of `uint256` objects that are immediately overwritten by switching from `resize` to `reserve` + `push_back`.
#### Memory Impact
[Memory profiling](https://github.com/bitcoin/bitcoin/pull/32497#issuecomment-3563724551) shows **50% reduction in peak allocation** (576KB → 288KB) and elimination of reallocation overhead.
#### Validation
The benchmark was updated to use an odd leaf count to demonstrate the real-world scenario where the reallocation occurs.
A full `-reindex-chainstate` up to block **896 408** ran without triggering the asserts.
<details>
<summary>Validation asserts</summary>
Temporary asserts (not included in this PR) confirm that `push_back` never reallocates and that the coinbase witness hash remains null:
```cpp
if (hashes.size() & 1) {
assert(hashes.size() < hashes.capacity()); // TODO remove
hashes.push_back(hashes.back());
}
leaves.reserve((block.vtx.size() + 1) & ~1ULL); // capacity rounded up to even
leaves.emplace_back();
assert(leaves.back().IsNull()); // TODO remove
```
</details>
#### Benchmark Performance
While the main purpose is to improve predictability, the reduced memory operations also improve hashing throughput slightly.
ACKs for top commit:
achow101:
ACK 3dd815f048c80c9a35f01972e0537eb42531aec7
optout21:
reACK 3dd815f048c80c9a35f01972e0537eb42531aec7
hodlinator:
re-ACK 3dd815f048c80c9a35f01972e0537eb42531aec7
vasild:
ACK 3dd815f048c80c9a35f01972e0537eb42531aec7
w0xlt:
ACK 3dd815f048c80c9a35f01972e0537eb42531aec7 with minor nits.
danielabrozzoni:
Code review ACK 3dd815f048
Tree-SHA512: e7b578f9deadc0de7d61c062c7f65c5e1d347548ead4a4bb74b056396ad7df3f1c564327edc219670e6e2b2cb51f4e1ccfd4f58dd414aeadf2008d427065c11f
969c840db52da796c319f84c9a9a20b1de902ccf log,blocks: avoid `ComputeTotalSize` and `GetHash` work when logging is disabled (Lőrinc)
babfda332b6aa41143eb694193358ef2c76ebefe log,net: avoid `ComputeTotalSize` when logging is disabled (Lőrinc)
1658b8f82b99f9ba3b42a50ba1f72b19a38c8e75 refactor: rename `CTransaction::GetTotalSize` to signal that it's not cached (Lőrinc)
Pull request description:
### Context
The new accounting options introduced in https://github.com/bitcoin/bitcoin/pull/32582 can be quite heavy, and are not needed when debug logging is disabled.
### Problem
`PartiallyDownloadedBlock::FillBlock()` and `PeerManagerImpl::SendBlockTransactions()` accumulate transaction sizes for debug logging by calling `ComputeTotalSize()` in loops, which invokes expensive `GetSerializeSize()` serializations.
The block header hash is also only computed for the debug log.
### Fixes
Guard the size and hash calculations with `LogAcceptCategory()` checks so the serialization and hashing work only occurs when compact block debug logging is enabled.
Also modernized the surrounding code a bit since the change is quite trivial.
### Reproducer
You can test the change by starting an up-to-date `bitcoind` node with `-debug=cmpctblock` and observing compact block log lines such as:
> [cmpctblock] Successfully reconstructed block 00000000000000000001061eaa6c0fe79258e7f79606e67ac495765cb121a520 with 1 txn prefilled, 3122 txn from mempool (incl at least 3 from extra pool) and 641 txn (352126 bytes) requested
<details>
<summary>Test patch</summary>
```patch
diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp
index 58620c93cc..f16eb38fa5 100644
--- a/src/blockencodings.cpp
+++ b/src/blockencodings.cpp
@@ -186,6 +186,7 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const
ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active)
{
+ LogInfo("PartiallyDownloadedBlock::FillBlock called");
if (header.IsNull()) return READ_STATUS_INVALID;
block = header;
@@ -218,6 +219,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
}
if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
+ LogInfo("debug log enabled");
const uint256 hash{block.GetHash()}; // avoid cleared header
uint32_t tx_missing_size{0};
for (const auto& tx : vtx_missing) tx_missing_size += tx->ComputeTotalSize(); // avoid cleared txn_available
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 5600c8d389..c081825f77 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -2470,6 +2470,7 @@ uint32_t PeerManagerImpl::GetFetchFlags(const Peer& peer) const
void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlock& block, const BlockTransactionsRequest& req)
{
+ LogInfo("PeerManagerImpl::SendBlockTransactions called");
BlockTransactions resp(req);
for (size_t i = 0; i < req.indexes.size(); i++) {
if (req.indexes[i] >= block.vtx.size()) {
@@ -2480,6 +2481,7 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
}
if (LogAcceptCategory(BCLog::CMPCTBLOCK, BCLog::Level::Debug)) {
+ LogInfo("debug log enabled");
uint32_t tx_requested_size{0};
for (const auto i : req.indexes) tx_requested_size += block.vtx[i]->ComputeTotalSize();
LogDebug(BCLog::CMPCTBLOCK, "Peer %d sent us a GETBLOCKTXN for block %s, sending a BLOCKTXN with %u txns. (%u bytes)\n", pfrom.GetId(), block.GetHash().ToString(), resp.txn.size(), tx_requested_size);
```
</details>
ACKs for top commit:
davidgumberg:
reACK 969c840db5
achow101:
ACK 969c840db52da796c319f84c9a9a20b1de902ccf
hodlinator:
re-ACK 969c840db52da796c319f84c9a9a20b1de902ccf
sedited:
Re-ACK 969c840db52da796c319f84c9a9a20b1de902ccf
danielabrozzoni:
reACK 969c840db52da796c319f84c9a9a20b1de902ccf
Tree-SHA512: 9780102d29778165144e3602d934ed4cb96660fd7b9ff2581b223c619e419139b8348e60f226af448702ae527736a1806d169b44342c5a82795590f664e16efe
a3c71c720158fd024d072ad32e78a3b353de0723 [test] Add BIP 328 test vectors for Musig2 (w0xlt)
Pull request description:
Built on https://github.com/bitcoin/bitcoin/pull/31244
This PR adds explicit tests for Bitcoin Core's MuSig2 interface.
Any issues in musig2.{cpp,h} will likely also be caught by the descriptor tests, but having more detailed tests for the MuSig2 class itself improves test reporting/coverage.
It uses BIP 328 test vectors.
ACKs for top commit:
achow101:
ACK a3c71c720158fd024d072ad32e78a3b353de0723
rkrux:
lgtm ACK a3c71c7
Tree-SHA512: fc13beb5445c292cd7c75a47810fb1c4032ee2e3c1800dc44089b95959ccce8330291084bf788457e1d55c02d706ef04be7044badfee134149e004c44b19ec32
9c7e4771b13d4729fd20ea08b7e2e3209b134fff test: Test listdescs with priv works even with missing priv keys (Novo)
ed945a685473712c1a822379effa42fd49223515 walletrpc: reject listdes with priv key on w-only wallets (Novo)
9e5e9824f11b1b0f9e2a4e28124edbb1616af519 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo)
5c4db25b61d417a567f152169f4ab21a491afb95 descriptor: refactor ToPrivateString for providers (Novo)
2dc74e3f4e5e6f01c8810359b91041bc6865f1c7 wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo)
e842eb90bb6db39076a43b010c0c7898d50b8d92 descriptors: add HavePrivateKeys() (Novo)
Pull request description:
_TLDR:
Currently, `listdescriptors [private=true]` will fail for a non-watch-only wallet if any descriptor has a missing private key(e.g `tr()`, `multi()`, etc.). This PR changes that while making sure `listdescriptors [private=true]` still fails if there no private keys. Closes #32078_
In non-watch-only wallets, it's possible to import descriptors as long as at least one private key is included. It's important that users can still view these descriptors when they need to create a backup—even if some private keys are missing ([#32078 (comment)](https://github.com/bitcoin/bitcoin/issues/32078#issuecomment-2781428475)). This change makes it possible to do so.
This change also helps prevent `listdescriptors true` from failing completely, because one descriptor is missing some private keys.
### Notes
- The new behaviour is applied to all descriptors including miniscript descriptors
- `listdescriptors true` still fails for watch-only wallets to preserve existing behaviour https://github.com/bitcoin/bitcoin/pull/24361#discussion_r920801352
- Wallet migration logic previously used `Descriptor::ToPrivateString()` to determine which descriptor was watchonly. This means that modifying the `ToPrivateString()` behaviour caused descriptors that were previously recognized as "watchonly" to be "non-watchonly". **In order to keep the scope of this PR limited to the RPC behaviour, this PR uses a different method to determine `watchonly` descriptors for the purpose of wallet migration.** A follow-up PR can be opened to update migration logic to exclude descriptors with some private keys from the `watchonly` migration wallet.
### Relevant PRs
https://github.com/bitcoin/bitcoin/pull/24361https://github.com/bitcoin/bitcoin/pull/32186
### Testing
Functional tests were added to test the new behaviour
EDIT
**`listdescriptors [private=true]` will still fail when there are no private keys because non-watchonly wallets must have private keys and calling `listdescriptors [private=true]` for watchonly wallet returns an error**
ACKs for top commit:
Sjors:
ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
achow101:
ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
w0xlt:
reACK 9c7e4771b1 with minor nits
rkrux:
re-ACK 9c7e4771b13d4729fd20ea08b7e2e3209b134fff
Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60
fabf8d1c5bdb6a944762792cdc762caa6c1a760b fuzz: Restore SendMessages coverage in process_message(s) fuzz targets (MarcoFalke)
fac7fed397f0fa3924600c1806a46d235a62ee5f refactor: Use std::reference_wrapper<AddrMan> in Connman (MarcoFalke)
Pull request description:
*Found and reported by Crypt-iQ (thanks!)*
Currently the process_message(s) fuzz targets do not have any meaningful `SendMessages` code coverage. This is not ideal.
Fix the problem by adding back the coverage, and by hardening the code here, so that the problem hopefully does not happen again in the future.
### Historic context for this regression
The regression was introduced in commit fa11eea4059a608f591db4469c07a341fd33a031, which built a new deterministic peerman object. However, the patch was incomplete, because it was missing one hunk to replace `g_setup->m_node.peerman->SendMessages(&p2p_node);` with `peerman->SendMessages(&p2p_node);`.
This means the stale and empty peerman from the node context and not the freshly created and deterministic peerman was used.
A simple fix would be to just submit the missing patch hunk. However, this still leaves the risk that the issue is re-introduced at any time in the future. So instead, I think the stale and empty peerman should be de-constructed, so that any call to it will lead to a hard sanitizer error and fuzz failure.
Doing that also uncovered another issue: The connman was holding on to a reference to a stale and empty addrman.
So fix all issues by:
* Allowing the addrman reference in connman to be re-seatable
* Clearing all stale objects, before creating new objects, and then using references to the new objects in all code
ACKs for top commit:
Crypt-iQ:
crACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b
frankomosh:
ACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b
marcofleon:
code review ACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b
sedited:
ACK fabf8d1c5bdb6a944762792cdc762caa6c1a760b
Tree-SHA512: 2e478102b3e928dc7505f00c08d4b9e4f8368407b100bc88f3eb3b82aa6fea5a45bae736c211f5af1551ca0de1a5ffd4a5d196d9473d4c3b87cfed57c9a0b69d
de509c6df97910cc11c5025d5f80fb05b9a88e80 iwyu: Add missed line to IWYU patch (Hennadii Stepanov)
Pull request description:
This PR makes IWYU suggest `<cassert>` over `<assert.h>`.
Fixes https://github.com/bitcoin/bitcoin/issues/34237.
ACKs for top commit:
maflcko:
lgtm ACK de509c6df97910cc11c5025d5f80fb05b9a88e80
Tree-SHA512: edba91eaf36992f684be2920f5da8c13a25ba6d79b879b92193e2af106cd454a64d7c4cf9dabc25675490df9edbccff1fd54c9f393e984a3a7a628b1c65f6c53
9482f00df0b05e8ef710a7f0fac3262855ce335f chore: Update outdated GitHub Actions versions (Padraic Slattery)
Pull request description:
This PR updates outdated GitHub Action versions to ensure compatibility and improve functionality. The following changes are made to the GitHub Actions:
- `actions/upload-artifact` updated from v4 to v6
- `actions/cache` updated from v4 to v5
- `actions/download-artifact` updated from v5 to v7
The updates are necessary to support newer environments and features, and ensure consistent behavior across different workflows. The changes will be tested in the CI pipeline of the pull request.
ACKs for top commit:
fanquake:
ACK 9482f00df0b05e8ef710a7f0fac3262855ce335f
Tree-SHA512: 248e79162c5b2748e1a367d87a360d62eb961c24b4f8060bb932ef99a79ef10cab3e65175c092226c90140f31686fb9424911e6609729cb186b304b598a9af44
d938947b3a89a61784a72c533df623f9eb2fec49 doc: Add "Using IWYU" to Developer Notes (Hennadii Stepanov)
e1a90bcecc823a4abaa2a57f393cad675a2ccbc0 iwyu: Do not export `crypto/hex_base.h` header (Hennadii Stepanov)
19a2edde50c38412712306bf527faad6cbf81c2c iwyu: Do not export C++ headers in most cases (Hennadii Stepanov)
Pull request description:
First two commits address comments from discussion in https://github.com/bitcoin/bitcoin/pull/33779:
- https://github.com/bitcoin/bitcoin/pull/33779#discussion_r2697579248
- https://github.com/bitcoin/bitcoin/pull/33779#discussion_r2697707343
The last commit adds a new section to the Developer Notes to document IWYU usage.
ACKs for top commit:
maflcko:
re-ACK d938947b3a89a61784a72c533df623f9eb2fec49 🚀
Tree-SHA512: 8d6c63e9d2fd190815211d80e654cb7379d16b6611b8851444f49bbbaa0fbc93557675fbcc558afd9a1cdf643570fba5eff9c1aecb5530f978797387b10b9a11
03f363d37884fe68d2f84a3def3fd6fe7bf4a506 doc: Document IWYU workaround (Hennadii Stepanov)
Pull request description:
This PR addresses the following comments:
- https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2640003086:
> it would be good to reduce and report this bug upstream. Otherwise, wide-spread use of iwyu in this code-base seems risky.
- https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2640035350:
> Would have been good if it was documented, rather than adding undocumented workarounds for buggy tools.
ACKs for top commit:
maflcko:
lgtm ACK 03f363d37884fe68d2f84a3def3fd6fe7bf4a506
sedited:
ACK 03f363d37884fe68d2f84a3def3fd6fe7bf4a506
Tree-SHA512: 160a963c07f853995c8b4741a6ccca1d8431a576c760fca082116cebde4d133f7c8ec51f09e8f85f54428f86bad2635e1bd708177eecf71feb0bf1489f1e2b3e
0dafc0d83c3e4c48c0ab2efad4b160d1b8c6d252 clang-format: use AngleBracket for main includes (stickies-v)
Pull request description:
This project uses angle brackets instead of quotes for project-specific headers. Setting [`MainIncludeChar`](https://clang.llvm.org/docs/ClangFormatStyleOptions.html#mainincludechar) enables `clang-format` to automatically detect the main header, so it can be kept as the top group of includes.
For example, without this change, `clang-format` would demote `<signet.h>` from being the main header in `src/signet.cpp`. With this change, the order is preserved.
On 5e49f5d63c74512c8f46fe7de7deb0341f13244a:
```
% clang-format src/signet.cpp | head -n 15
// Copyright (c) 2019-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <consensus/merkle.h>
#include <consensus/params.h>
#include <consensus/validation.h>
#include <logging.h>
#include <primitives/block.h>
#include <primitives/transaction.h>
#include <script/interpreter.h>
#include <script/script.h>
#include <signet.h>
#include <streams.h>
#include <uint256.h>
```
With this PR:
```
% clang-format src/signet.cpp | head -n 10
// Copyright (c) 2019-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <signet.h>
#include <consensus/merkle.h>
#include <consensus/params.h>
#include <consensus/validation.h>
#include <logging.h>
```
Note: `AngleBracket` `requires clang-format 19`, and will cause older versions (including our current minimum llvm version `17`) to fail
ACKs for top commit:
maflcko:
review ACK 0dafc0d83c3e4c48c0ab2efad4b160d1b8c6d252
sedited:
Nice, ACK 0dafc0d83c3e4c48c0ab2efad4b160d1b8c6d252
hebasto:
ACK 0dafc0d83c3e4c48c0ab2efad4b160d1b8c6d252, tested on Ubuntu 25.10.
Tree-SHA512: c0876f505ec188f76e435af0731c411c66266b83e4c08528d0637263abcd84b3968ee6fbfa72630192f1a0cd2728af873d3d6c32f93ab8b228222fad16f232be
a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43 Fix 11-year-old mis-categorized error code in OP_IF evaluation (Calin Culianu)
Pull request description:
This was introduced by commit ab9edbd6b6eb3efbca11f16fa467c3c0ef905708.
It appears the original author may have gotten tired and pasted the wrong error code into this 1 place. Every other situation where the value stack lacks the required number of arguments for the op-code, SCRIPT_ERR_INVALID_STACK_OPERATION is reported. Not so here.
This commit fixes the situation.
EDIT: Note this turns out to be a dupe of the abandoned #30359 .
ACKs for top commit:
billymcbip:
tACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
achow101:
ACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
darosior:
utACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
sedited:
ACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
Tree-SHA512: e8c01a3e2448b5d49b76a0cab3f38a2d0249b71beeb7d9d05d5ecc3812bd91d0bd1d0f78b809b6f4ccb73186fa119cb1ed3779a73284b83a67ae219ef378fa6c
`PartiallyDownloadedBlock::FillBlock()` computed the block header hash and summed missing transaction sizes for debug logging unconditionally, including when cmpctblock debug logging is disabled.
Guard the debug-only hash and size computations with `LogAcceptCategory`.
Since `txn_available` is invalidated after the first loop (needed for efficient moving), we compute `tx_missing_size` by iterating `vtx_missing` directly. This is safe because the later `tx_missing_offset` check guarantees `vtx_missing` was fully consumed during reconstruction.
Use `block.GetHash()` instead of `header.GetHash()`, since header is cleared before logging.
No behavior change when debug logging is enabled: the reported counts, hashes, and byte totals remain the same.
`PeerManagerImpl::SendBlockTransactions()` computed the total byte size of requested transactions for a debug log line by calling `ComputeTotalSize()` in a tight loop, triggering serialization even when debug logging is off.
Guard the size accumulation with `LogAcceptCategory` so the serialization work only happens when the log line can be emitted.
No behavior change when debug logging is enabled: the reported block hash, transaction count, and byte totals are the same.
The bounds checks still run unconditionally; the debug-only loop iterates the already-validated response contents.
Separating debug-only work from the critical path reduces risk and favors the performance-critical non-debug case.
This also narrows the racy scope of when logging is toggled from another thread.
Transaction hashes are cached, it may not be intuitive that their sizes are actually recalculated every time.
This is done before the other refactors to clarify why we want to avoid calling this method;
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
This project uses angle brackets instead of quotes for project-specific
headers. Setting MainIncludeChar enables clang-format to automatically
detect the main header, so it can be kept as the top group of includes.
For example, without this change, the below command would demote
<signet.h> from being the main header. With this change, the order is
preserved.
`clang-format -i src/signet.cpp`
`IWYU pragma: export` enforces the transitive inclusion of the headers,
which undermines the purpose of IWYU.
The remained cases seem useful and could be considered separately:
- `<cassert>` in `util/check.h`
- `<filesystem>` in `util/fs.h`
- `<chrono>` in `util/time.h`
faf07bd1ab26621e7c76fe78d2bf3c16ad3792b7 doc: Fix typo found by LLM (MarcoFalke)
faf66673ac60bb0407b3468a0ea64ce78303f1ed refactor: [move-only] Merge core_io module (MarcoFalke)
fa6947f4915f4ecef45b3bd849cc41653da97b64 kernel: Remove unused core_read.cpp from kernel (MarcoFalke)
Pull request description:
Currently the core_io module is split across two translation units. This will confuse code readers and tooling about the real state of the module.
Fix that by merging the module and removing the mapping workarounds.
Also, remove the module from the kernel lib, because it is not used there: The kernel does not use any json or string parsing or formatting.
ACKs for top commit:
hebasto:
re-ACK faf07bd1ab26621e7c76fe78d2bf3c16ad3792b7, only rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/34296#pullrequestreview-3675359502).
sedited:
Re-ACK faf07bd1ab26621e7c76fe78d2bf3c16ad3792b7
stickies-v:
ACK faf07bd1ab26621e7c76fe78d2bf3c16ad3792b7
Tree-SHA512: 3f5d91f1a4cb86dfe329b28ff31e93d65f2f0659a6f6f2de22ca6fb65056fb818ae369ef0ad773d4f5b92f63891a7a9450246377d8e14c34bc43f3deee0554cb
a5a8c4139c811e697b3c0b4d87737e04b60c53c8 ci, iwyu: Fix warnings in `src/kernel` and treat them as errors (Hennadii Stepanov)
Pull request description:
Now seems like a good time to update the includes in `src/kernel`.
ACKs for top commit:
maflcko:
review ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8 🍱
purpleKarrot:
ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8
sedited:
ACK a5a8c4139c811e697b3c0b4d87737e04b60c53c8
Tree-SHA512: ba401b27b03dee66d52d0b348972268e162506c4bafa40f408349173b68c40a11f20ca24f46c98945515e1d5c84f740d6e6784f7e4c799df46ab816cf5d11483
fa64d8424b8de49e219bffb842a33d484fb03212 refactor: Enforce readability-avoid-const-params-in-decls (MarcoFalke)
faf0c2d942c8de7868a3fd3afc7fc9ea700c91d4 refactor: Avoid copies by using const references or by move-construction (MarcoFalke)
Pull request description:
Top level `const` in declarations is problematic for many reasons:
* It is often a typo, where one wanted to denote a const reference. For example `bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, ...` is missing the `&`. This will create a redundant copy of the value.
* In constructors it prevents move construction.
* It can incorrectly imply some data is const, like in an imaginary example `std::span<int> Shuffle(const std::span<int>);`, where the `int`s are *not* const.
* The compiler ignores the `const` from the declaration in the implementation.
* It isn't used consistently anyway, not even on the same line.
Fix some issues by:
* Using a const reference to avoid a copy, where read-only of the value is intended. This is only done for values that may be expensive to copy.
* Using move-construction to avoid a copy
* Applying `readability-avoid-const-params-in-decls` via clang-tidy
ACKs for top commit:
l0rinc:
diff reACK fa64d8424b8de49e219bffb842a33d484fb03212
hebasto:
ACK fa64d8424b8de49e219bffb842a33d484fb03212, I have reviewed the code and it looks OK.
sedited:
ACK fa64d8424b8de49e219bffb842a33d484fb03212
Tree-SHA512: 293c000b4ebf8fdcc75259eb0283a2e4e7892c73facfb5c3182464d6cb6a868b7f4a6682d664426bf2edecd665cf839d790bef0bae43a8c3bf1ddfdd3d068d38
dd904298c13b14ef518e24fa63c6d0962f4a2de0 gui: Show an error message if the restored wallet name is empty (Ava Chow)
Pull request description:
The Restore Wallet dialog rejects wallet names that are empty, but was doing so silently. This is confusing, we should be presenting an error message to the user.
ACKs for top commit:
hebasto:
ACK dd904298c13b14ef518e24fa63c6d0962f4a2de0. Tested on Fedora 43.
Tree-SHA512: f4b60f32d1c2550dbce8613f25d29a92588b1ecfc8e8e5dac691a6bdb21a77508288a904539b68333d96bde5ebb993912253f4a293e4c583891f553d95762e77
fa38ffac6ff560bf76a2bfa48a300a79d31ba466 contrib: [refactor] Use shorter read_text from pathlib (MarcoFalke)
fab8bc03082d4e10b5c8008b96d2951991746064 contrib: Revert "verify-commits sha1 exceptions" (MarcoFalke)
Pull request description:
This reverts commit 8ac134be5e57680eb1c6ef596e5de085825e83ee, because it is no longer needed.
See https://github.com/bitcoin/bitcoin/pull/34245#issuecomment-3759448369
Also, use the shorter pathlib `read_text`, which is available since Python 3.5
ACKs for top commit:
dergoegge:
utACK fa38ffac6ff560bf76a2bfa48a300a79d31ba466
sedited:
ACK fa38ffac6ff560bf76a2bfa48a300a79d31ba466
hebasto:
ACK fa38ffac6ff560bf76a2bfa48a300a79d31ba466.
Tree-SHA512: 83049349d4a5c74ad700c2912d727584b88944a75d572c10661a76b69b08093ef7ebf786b359455e36d7467a708de46a77da41a54512e057d7eed8206984c8fd
979d41bfab248990d7d520873d17fe52daa8d402 qa: Fix Windows logging bug (Hennadii Stepanov)
Pull request description:
The regex `(.*)` was capturing `\r` from subprocess output on Windows, causing the closing parenthesis in logs to wrap to the next line.
For [example](https://github.com/hebasto/bitcoin/actions/runs/20993438084/job/60350204808):
```
208/454 - feature_bip68_sequence.py passed, Duration: 10 s
209/454 - rpc_bind.py --ipv4 skipped (not on a Linux system
)
210/454 - rpc_bind.py --ipv6 skipped (not on a Linux system
)
211/454 - rpc_packages.py passed, Duration: 8 s
212/454 - rpc_bind.py --nonloopback skipped (not on a Linux system
)
213/454 - p2p_feefilter.py passed, Duration: 4 s
```
Stripping whitespace from the regex match fixes the formatting. [See](https://github.com/hebasto/bitcoin/actions/runs/20993564177/job/60350024373):
```
208/454 - feature_bip68_sequence.py passed, Duration: 9 s
209/454 - rpc_bind.py --ipv4 skipped (not on a Linux system)
210/454 - rpc_bind.py --ipv6 skipped (not on a Linux system)
211/454 - rpc_bind.py --nonloopback skipped (not on a Linux system)
212/454 - rpc_packages.py passed, Duration: 7 s
```
ACKs for top commit:
maflcko:
lgtm ACK 979d41bfab248990d7d520873d17fe52daa8d402
l0rinc:
lightly tested ACK 979d41bfab248990d7d520873d17fe52daa8d402
Tree-SHA512: bafe1937a519e45e4cab395bae622acf65220f313c773a0729ba7dccc3a0a048602f1c04b3e8cdd80d2cf68ae36cef802a819530485d5a745db8abcadf141f68
3e340672ecadb2a32b19574a99a5162806af645e test: use ephemeral ports in p2p_private_broadcast.py (w0xlt)
Pull request description:
The test `p2p_private_broadcast.py` gets some Python P2P nodes to listen and instructs the SOCKS5 proxy to redirect connections to them instead of to the requested addresses. This way the `bitcoind` which uses the proxy is tricked to think it has connected to real routable internet IP addresses or `.onion` addresses.
Picking the ports where to Python P2P nodes to listen however is tricky to be done in a non-conflicting way, given that other tests may run in parallel. https://github.com/bitcoin/bitcoin/pull/34186 made it possible to let the OS select a free port, so use that in
`p2p_private_broadcast.py`.
---
_Suggested in https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2654849875_
ACKs for top commit:
l0rinc:
code review ACK 3e340672ecadb2a32b19574a99a5162806af645e
polespinasa:
tACK 3e340672ecadb2a32b19574a99a5162806af645e
mzumsande:
utACK 3e340672ecadb2a32b19574a99a5162806af645e
Tree-SHA512: e94efd33a1845e1767aaada55f91c60bc5fc1166c281ef578a391e95e2791a922d84aa6ed1ce06e7d6ca1a65f84da52fd79d9b2f40705c1944a53c67b7392e4d
d09a19fd41cb71a5d1c10297763e72bc551a7d3a test: add coverage for issue 34206 (Greg Sanders)
4c7cfd37ad9517334a0848e6778243ddef1843f4 wallet: remove erroneous-on-reorg Assume() (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/34206
I'm not certain the test is worth keeping, but included it for now to show minimal example that crashes without fix. Can be removed.
ACKs for top commit:
bensig:
ACK d09a19fd41cb71a5d1c10297763e72bc551a7d3a
dergoegge:
utACK d09a19fd41cb71a5d1c10297763e72bc551a7d3a
Tree-SHA512: 7eac19e97be6db8e38af396c406066fdcec532332e685a38bb33f0a988701c7bd5a0967f51426737fd56972847b761a3d873495928ff66efa8512fb267a9622b
The addrman field is already a reference. However, some tests would
benefit from the reference being re-seatable, so that they do not have
to create a full Connman each time.
fac70ea8b5bb33d05a47c36f2c5f1d79f119315c fuzz: Exclude too expensive inputs in miniscript_string target (MarcoFalke)
fa907864786056258302a611bf4df0319138a71b iwyu: Fix includes for test/fuzz/util/descriptor module (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/30498
Accepting "expensive" fuzz inputs which have no real use-case is problematic, because it prevents the fuzz engine from spending time on the next useful fuzz input.
For example this one will take several seconds (the flamegraph shows the time is spent in minscipt `NoDupCheck`):
```
curl -fLO '41bae50cff'
FUZZ=miniscript_string /usr/bin/time ./bld-cmake/bin/fuzz ./41bae50cffd1741150a1b330d02ab09f46ff8cd1
```
Inspecting the inputs shows that it has many sub frags, so rejecting based on `HasTooManySubFrag` should be sufficient.
ACKs for top commit:
darosior:
ACK fac70ea8b5bb33d05a47c36f2c5f1d79f119315c
brunoerg:
code review ACK fac70ea8b5bb33d05a47c36f2c5f1d79f119315c
dergoegge:
utACK fac70ea8b5bb33d05a47c36f2c5f1d79f119315c
Tree-SHA512: 7f1e0d9ce24d67ec63e5b7c2dd194efa51f38beb013564690afe0f920e5ff1980c85ce344828c0dc3f34b6851db7fe72a76b1a775c6d51c94fb91431834f453b
da56ef239b12786e3a177afda14352dda4a70bc6 clusterlin: minimize chunks (feature) (Pieter Wuille)
Pull request description:
Part of #30289.
This was split off from #34023, because it's not really an optimization but a feature. The feature existed pre-SFL, so this brings SFL to parity in terms of functionality with the old code.
The idea is that while optimality - as achieved by SFL before this PR - guarantees a linearization whose feerate diagram is optimal, it may be possible to split chunks into smaller equal-feerate parts. This is desirable because even though it doesn't change the diagram, it provides more flexibility for optimization (binpacking is easier when the pieces are smaller).
Thus, this PR introduces the stronger notion of "minimality": optimal chunks, which are also split into their smallest possible pieces. To accomplish that, an additional step in the SFL algorithm is added which aims to split chunks into minimal equal-feerate parts where possible, without introducing circular dependencies between them. It works based on the observation that if an (already otherwise optimal) chunk has a way of being split into two equal-feerate parts, and T is a given transaction in the chunk, then we can find the split in two steps:
* One time, pretend T has $\epsilon$ higher feerate than it really has. If a split exists with T in the top part, this will find it.
* The other time, pretend T has $\epsilon$ lower feerate than it really has. If a split exists with T in the bottom part, this will find it.
So we try both on each found optimal chunk. If neither works, the chunk is minimal. If one works, recurse into the split chunks to split them further.
ACKs for top commit:
instagibbs:
reACK da56ef239b
marcofleon:
crACK da56ef239b12786e3a177afda14352dda4a70bc6
Tree-SHA512: 2e94d6b78725f5f9470a939dedef46450b85c4e5e6f30cba0b038622ec2b417380747e8df923d1f303706602ab6d834350716df9678de144f857e3a8d163f6c2
The test `p2p_private_broadcast.py` gets some Python P2P nodes to listen
and instructs the SOCKS5 proxy to redirect connections to them instead
of to the requested addresses. This way the `bitcoind` which uses the
proxy is tricked to think it has connected to real routable internet
IP addresses or `.onion` addresses.
Picking the ports where to Python P2P nodes to listen however is tricky
to be done in a non-conflicting way, given that other tests may run in
parallel. https://github.com/bitcoin/bitcoin/pull/34186 made it possible
to let the OS select a free port, so use that in
`p2p_private_broadcast.py`.
fa3df5271232ee342c225da183be95dc47bde77f bench: Require semicolon after BENCHMARK(foo) (MarcoFalke)
fa8938f08c9a9da81a482bccb6bfe86f37a5a841 bench: Remove incorrect __LINE__ in BENCHMARK macro (MarcoFalke)
fa51a28a948dbab7109f660bf11dbfd389c839ed scripted-diff: Remove priority_level from BENCHMARK macro (MarcoFalke)
fa790c3eeaae1bb600ae59d013b170087ea5fb0e bench: Remove -priority-level= option (MarcoFalke)
Pull request description:
The option was added in https://github.com/bitcoin/bitcoin/pull/26158, when the project was using an autotools-based build system. However, in the meantime this option is unused:
* First, commit 27f11217ca63e0f8f78f14db139150052dcd9962 removed the option from one CI task
* Then https://github.com/bitcoin/bitcoin/pull/32310 removed the option from CMakeList.txt, because:
* they only run as a sanity check (fastest version)
* no one otherwise runs them, not even CI
* issues have been missed due to this
Finally, after commit 0ad4376a49fae6f705128b326ba92317cb8e0639, I don't see a single reason to keep this option, so remove it.
Also, there is a commit to turn a silent ignore of duplicate bench names into an error.
ACKs for top commit:
achow101:
ACK fa3df5271232ee342c225da183be95dc47bde77f
l0rinc:
ACK fa3df5271232ee342c225da183be95dc47bde77f
hebasto:
re-ACK fa3df5271232ee342c225da183be95dc47bde77f, only suggested changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/34210#pullrequestreview-3652414135).
Tree-SHA512: 68a314bff551fa878196d5a615d41d71e1c8c504135e6fc555659aa9f0c8786957d49ba038448e933554a8bc54caea2ddd7d628042c5627bf3bf37628210f8fb
ce63d37ebee8d062310a778c5efa6475814188e9 test: use dynamic port allocation to avoid test conflicts (woltx)
Pull request description:
Use `port=0` for dynamic port allocation in test framework components to avoid intermittent "address already in use" errors when running tests concurrently or when ports are stuck in TIME_WAIT state. Example: https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2634509304
Changes:
- Update `socks5.py` and `p2p.py` to support dynamic port allocation
- Convert `feature_proxy.py` and `feature_anchors.py` to use `port=0`
ACKs for top commit:
achow101:
ACK ce63d37ebee8d062310a778c5efa6475814188e9
vasild:
ACK ce63d37ebee8d062310a778c5efa6475814188e9
mzumsande:
re-ACK ce63d37ebee8d062310a778c5efa6475814188e9
Tree-SHA512: 4efcedca3bde209fbd1bdc2a4ae04b7d53515587d86e421ce61064f78c675c71b45d9782b514c5e7cfc0e92842c947d49f7a3fddb03fe619fcdec9b565f0ecbd
7b5d256af4a0f954a919604ed4346db3a814fb6d test: Add bitcoin-chainstate test for assumeutxo functionality (stringintech)
2bc32656498517fe58bd41dcbd0afd306d51d4b0 Fix `ChainstateManager::AddChainstate()` assertion crash (stringintech)
5f3d6bdb6659dba16941e6d6a05fd883d3f49a9d Add regtest support to bitcoin-chainstate tool (stringintech)
Pull request description:
This PR adds functional test coverage for the bitcoin-chainstate tool loading a datadir initialized with an assumeutxo snapshot.
The PR also includes:
- Fix for assertion crash in `ChainstateManager::AddChainstate()` when `prev_chainstate` has no initialized mempool (required for the test to pass)
- `-regtest` flag support for bitcoin-chainstate to enable the testing
This work started while experimenting with the bitcoin-chainstate tool and how the kernel API (#30595) behaved when loading a datadir containing assumeutxo data, during the time that PR was still under review. sedited suggested opening a PR to add this test coverage.
ACKs for top commit:
achow101:
ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
theStack:
Concept and code-review ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
sedited:
Re-ACK 7b5d256af4a0f954a919604ed4346db3a814fb6d
Tree-SHA512: 5d3b0050cf2d53144b5f65451c991d5e212117b4541ae1368ecf58fde5f3cca4f018aad6ae32257b9ebb1c28b926424fbcff496ba5487cdc4eb456cea6db8b24
792e2edf57ab31ae5c6f98acf33af8f67506630f p2p: first addr self-announcement in separate msg (0xb10c)
Pull request description:
This makes sure the initial address self-announcement a node sends to a peer happends in a separate P2P message. This has benefits for both inbound and outbound connections:
For inbound connections from a peer to us, previously, we might send the self-announcement along with our response to a GETADDR request. However, the self-announcement might replace an address from the GETADDR response. This isn't clean.
For outbound connections from us to a peer, previously, it could have happend that we send the self-announcement along with other addresses. Since shortly after connection open, the peer might only have one rate-limiting token for us, and the addresses are shuffeld on arrival, it's possible that the self-announcement gets rate-limited. However, note that these rate-limitings seem to be rare in practice.
This is inspired by and based on https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3462287763. The discussion there should be helpful for reviewers.
ACKs for top commit:
bensig:
ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
achow101:
ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
fjahr:
Code review ACK 792e2edf57ab31ae5c6f98acf33af8f67506630f
frankomosh:
Code Review ACK [792e2ed](792e2edf57)
Tree-SHA512: e3d39b1e3ae6208b54df4b36c624a32d70a442e01681f49e0c8a65076a818b5bf203c2e51011dc32edbbe3637b3c0b5f18de26e3461c288aa3806646a209a260
8fb5e5f41ddf550a78b1253184d79a107097815a test: check wallet rescan properly in feature_pruning (brunoerg)
9b57c8d2bd15a414e08a9e43367d8d3d82c25fe4 test: fix feature_pruning when built without wallet (brunoerg)
Pull request description:
Fixes#34175
In `feature_pruning`, the`wallet_test` doesn't require any specific wallet functionality and this test is important for one of next ones (`test_scanblocks_pruned`). The reason is that it synchronizes the node 5 and, without this sync, `test_scanblocks_pruned` will fail since we expect `scanblocks` to fail due to `Block not available (pruned data)` and it doesn't happen without this sync.
ACKs for top commit:
achow101:
ACK 8fb5e5f41ddf550a78b1253184d79a107097815a
furszy:
utACK 8fb5e5f41ddf550a78b1253184d79a107097815a
musaHaruna:
Tested ACK [8fb5e5f](8fb5e5f41d)
w0xlt:
ACK 8fb5e5f41d
Tree-SHA512: 812afbf4343a7493e2169eb6735fce25692d5cb19972abafc772b8c05a64b9c7027f6675cd084f345977e916e62a722d671f90831bbdc51683e0cd253fa482f0
