- CTxMemPool::removeUnchecked description comment is stale and incorrect
after cluster mempool.
This commit fixes the issue by deleting the stale comment and describing
only the implicit behaviour triggered by the method.
faa18dceba1dd69703a252f751c233d227164689 refactor: Use std::bind_front over std::bind (MarcoFalke)
Pull request description:
`std::bind` has many issues:
* It is verbosely listing all placeholders, but in a meaningless way, because it doesn't name the args or their types.
* It silently ignores args passed to it, when one arg is overridden. For example [1] compiles fine on current master.
* Accidentally duplicated placeholders compile fine as well.
* Usually the placeholders aren't even needed.
* This makes it hard to review, understand, and maintain.
Fix all issues by using `std::bind_front` from C++20, which allows to drop the brittle `_1, _2, ...` placeholders. The replacement should be correct, if the trailing placeholders are ordered.
Introducing the same silent bug on top of this pull request [2] will now lead to a compile failure.
----
[1]
```diff
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 694fb535b5..7661dd361e 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
m_handler_status_changed = m_wallet->handleStatusChanged(std::bind(&NotifyKeyStoreStatusChanged, this));
- m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
+ m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind(NotifyAddressBookChanged, this, CTxDestination{}, std::placeholders::_2, std::placeholders::_3, std::placeholders::_4, std::placeholders::_5));
m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind(NotifyTransactionChanged, this, std::placeholders::_1, std::placeholders::_2));
```
[2]
```diff
diff --git a/src/qt/walletmodel.cpp b/src/qt/walletmodel.cpp
index 578713c0ab..84cced741c 100644
--- a/src/qt/walletmodel.cpp
+++ b/src/qt/walletmodel.cpp
@@ -412,3 +412,3 @@ void WalletModel::subscribeToCoreSignals()
m_handler_status_changed = m_wallet->handleStatusChanged(std::bind_front(&NotifyKeyStoreStatusChanged, this));
- m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this));
+ m_handler_address_book_changed = m_wallet->handleAddressBookChanged(std::bind_front(NotifyAddressBookChanged, this, CTxDestination{}));
m_handler_transaction_changed = m_wallet->handleTransactionChanged(std::bind_front(NotifyTransactionChanged, this));
ACKs for top commit:
janb84:
cr ACK faa18dceba1dd69703a252f751c233d227164689
fjahr:
Code review ACK faa18dceba1dd69703a252f751c233d227164689
hebasto:
ACK faa18dceba1dd69703a252f751c233d227164689, I have reviewed the code and it looks OK.
Tree-SHA512: 9dd13f49527e143a2beafbaae80b1358981f07a2ce20d25cffb1853089a32ff71639e6d718d1d193754522f9ac04e3e168ba017d5fc67a11a5918e79a92b3461
faa59b367985648df901bdd7b5bba69ef898ea08 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3dba7c03f9242440cb55eb37b493a7a util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430aa83ddb266aca029e270aec81c021d util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac48009598611d28b6583559af513c337166aeb util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2d27d173162888226df669fb8aeea47 util: Make Expected::value() throw (MarcoFalke)
fa1de1103fe5d97ddddc9e45286e32751151f859 util: Add Unexpected::error() (MarcoFalke)
faa109f8be7fca125c55ca84e6c0baf414c59ae6 test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b8d3a3aa09eca4f47e1741912328785 Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)
Pull request description:
Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.
They are currently unused, but bring the port closer to the original `std::expected` implementation:
* Make `Expected::value()` throw when no value exists
* Add `Unexpected::error()` methods
* Add `Expected<void, E>` specialization
* Add `Expected::value()&&` and `Expected::error()&&` methods
* Add `Expected::swap()`
Also, include a tiny tidy fixup:
* tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check
ACKs for top commit:
stickies-v:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
ryanofsky:
Code review ACK faa59b367985648df901bdd7b5bba69ef898ea08. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
hodlinator:
re-ACK faa59b367985648df901bdd7b5bba69ef898ea08
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
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
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`
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
This is not expected to be needed in this codebase, but brings the
implementation closer to std::expected::value().
Also, add noexcept, where std::expected has them. This will make
operator-> and operator* terminate, when has_value() is false.
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
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
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
de4242f47476769d0a7f3e79e8297ed2dd60d9a4 refactor: Use reference for chain_start in HeadersSyncState (Daniela Brozzoni)
e37555e5401f9fca39ada0bd153e46b2c7ebd095 refactor: Use initializer list in CompressedHeader (Daniela Brozzoni)
0488bdfefe92b2c9a924be9244c91fe472462aab refactor: Remove unused parameter in ReportHeadersPresync (Daniela Brozzoni)
256246a9fa5b05141c93aeeb359394b9c7a80e49 refactor: Remove redundant parameter from CheckHeadersPoW (Daniela Brozzoni)
ca0243e3a6d77d2b218749f1ba113b81444e3f4a refactor: Remove useless CBlock::GetBlockHeader (Pieter Wuille)
45686522224598bed9923e60daad109094d7bc29 refactor: Use std::span in HasValidProofOfWork (Daniela Brozzoni)
4066bfe561a45f61a3c9bf24bec7f600ddcc7467 refactor: Compute work from headers without CBlockIndex (Daniela Brozzoni)
0bf6139e194f355d121bb2aea74715d1c4099598 p2p: Avoid an IsAncestorOfBestHeaderOrTip call (Pieter Wuille)
Pull request description:
This is a partial* revival of #25968
It contains a list of most-unrelated simplifications and optimizations to the code merged in #25717:
- Avoid an IsAncestorOfBestHeaderOrTip call: Just don't call this function when it won't have any effect.
- Compute work from headers without CBlockIndex: Avoid the need to construct a CBlockIndex object just to compute work for a header, when its nBits value suffices for that. Also use some Spans where possible.
- Remove useless CBlock::GetBlockHeader: There is no need for a function to convert a CBlock to a CBlockHeader, as it's a child class of it.
It also contains the following code cleanups, which were suggested by reviewers in #25968:
- Remove redundant parameter from CheckHeadersPoW: No need to pass consensusParams, as CheckHeadersPow already has access to m_chainparams.GetConsensus()
- Remove unused parameter in ReportHeadersPresync
- Use initializer list in CompressedHeader, also make GetFullHeader const
- Use reference for chain_start in HeadersSyncState: chain_start can never be null, so it's better to pass it as a reference rather than a raw pointer
*I decided to leave out three commits that were in #25968 (4e7ac7b94d04e056e9994ed1c8273c52b7b23931, ab52fb4e95aa2732d1a1391331ea01362e035984, 7f1cf440ca1a9c86085716745ca64d3ac26957c0), since they're a bit more involved, and I'm a new contributor. If this PR gets merged, I'll comment under #25968 to note that these three commits are still up for grabs :)
ACKs for top commit:
l0rinc:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
polespinasa:
re-ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
sipa:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
achow101:
ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
hodlinator:
re-ACK de4242f47476769d0a7f3e79e8297ed2dd60d9a4
Tree-SHA512: 1de4f3ce0854a196712505f2b52ccb985856f5133769552bf37375225ea8664a3a7a6a9578c4fd461e935cd94a7cbbb08f15751a1da7651f8962c866146d9d4b
7fc465ece88284c79728cacbc1d4c2fe63c60e1a doc: fix incorrect description of `PackageMempoolChecks` (ismaelsadeeq)
1412b779ad0a7d98396e45676ba75bd8e90446e0 refactor: execute `PackageMempoolChecks` during package rbf only (ismaelsadeeq)
Pull request description:
This is a simple PR that fixes the incorrect description of what is done in `PackageMempoolChecks`
> // Enforce package mempool ancestor/descendant limits (distinct from individual
> // ancestor/descendant limits done in PreChecks) and run Package RBF checks.
After cluster mempool, we no longer enforce ancestor/descendant limits in both `PreChecks` and `PackageMempoolChecks`; instead, cluster limit is enforced in `PackageMempoolChecks`.
This PR fixes the incorrect comment by;
- Making it clear why it is necessary to have two calls of `CheckMempoolPolicyLimts` in both `PackageMempoolChecks` and after in `AcceptMultipleTransactionsInternal` by executing `PackageMempoolChecks` only during package RBF only. No need to jump into the next subroutine when there is no conflict.
- Renames `PackageMempoolChecks` to `PackageRBFChecks`; the method name is self-explanatory now, hence no need for a description comment.
ACKs for top commit:
yashbhutwala:
ACK 7fc465ece88284c79728cacbc1d4c2fe63c60e1a
instagibbs:
ACK 7fc465ece88284c79728cacbc1d4c2fe63c60e1a
glozow:
utACK 7fc465ece88284c79728cacbc1d4c2fe63c60e1a
Tree-SHA512: 38655f9d05be54cadd224fad376da9871a85efc7801306b58d4f51aee658036cdce2ab406143a3439d7211fc9bb0fc86bd330852e8926d79660944872b8fae8d
fa1d17d56c83d6ad89c1f688824ec0dc1c294012 refactor: Use uint64_t over size_t for serialize corruption check in fees.dat (MarcoFalke)
Pull request description:
Serialization should not behave differently on different architectures. See also the related commit 3789215f73466606eb111714f596a2a5e9bb1933.
However, on fees.dat file corruption, 32-bit builds may run into an unsigned integer overflow and report the wrong corruption reason, or may even silently continue after the corruption.
This is a bit hard to reproduce, because 32-bit platforms are rare and most of them don't support running the unsigned integer overflow sanitizer. So the possible options to reproduce are:
* Run on armhf and manually annotate the code to detect the overflow
* Run on i386 with the integer sanitizer (possibly via `podman run -it --rm --platform linux/i386 'debian:trixie'`)
* Run the integer sanitizer on any 64-bit platform and manually replace type in the affected line by `uint32_t`
Afterwards, the steps to reproduce are:
```
export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config python3-zmq libzmq3-dev libevent-dev libboost-dev libsqlite3-dev systemtap-sdt-dev libcapnp-dev capnproto libqrencode-dev qt6-tools-dev qt6-l10n-tools qt6-base-dev clang llvm libc++-dev libc++abi-dev -y
cmake -B ./bld-cmake -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero --preset=dev-mode
cmake --build ./bld-cmake --parallel $(nproc)
curl -fLO '6074731370'
UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" FUZZ=policy_estimator_io ./bld-cmake/bin/fuzz ./607473137013139e3676e30ec4b29639e673fa9b
```
The output will be something like:
```
/b-c/src/policy/fees/block_policy_estimator.cpp:448:25: runtime error: unsigned integer overflow: 346685954 * 219 cannot be represented in type 'unsigned int'
#0 0x5b0b1bbe in TxConfirmStats::Read(AutoFile&, unsigned int) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:448:25
#1 0x5b0b7d3f in CBlockPolicyEstimator::Read(AutoFile&) /b-c/bld-cmake/src/./policy/fees/block_policy_estimator.cpp:1037:29
#2 0x592a9783 in policy_estimator_io_fuzz_target(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/./test/fuzz/policy_estimator_io.cpp:32:32
#3 0x5896ba8e in void std::__invoke_impl<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(std::__invoke_other, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:61:14
#4 0x5896b8eb in std::enable_if<is_invocable_r_v<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>, void>::type std::__invoke_r<void, void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>>(void (*&)(std::span<unsigned char const, 4294967295u>), std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/invoke.h:111:2
#5 0x5896b44b in std::_Function_handler<void (std::span<unsigned char const, 4294967295u>), void (*)(std::span<unsigned char const, 4294967295u>)>::_M_invoke(std::_Any_data const&, std::span<unsigned char const, 4294967295u>&&) /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:290:9
#6 0x59845c95 in std::function<void (std::span<unsigned char const, 4294967295u>)>::operator()(std::span<unsigned char const, 4294967295u>) const /usr/lib/gcc/i686-linux-gnu/14/../../../../include/c++/14/bits/std_function.h:591:9
#7 0x5983a0da in test_one_input(std::span<unsigned char const, 4294967295u>) /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:88:5
#8 0x5983cb80 in main /b-c/bld-cmake/src/test/fuzz/util/./test/fuzz/fuzz.cpp:271:13
#9 0xf75aecc2 (/lib/i386-linux-gnu/libc.so.6+0x24cc2) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75)
#10 0xf75aed87 in __libc_start_main (/lib/i386-linux-gnu/libc.so.6+0x24d87) (BuildId: 2dc5f2945fad35c1b07d1a5a32520b3c41afaa75)
#11 0x58932db6 in _start (/b-c/bld-cmake/bin/fuzz+0x235ddb6) (BuildId: 7d8d83a77923f14e99c0de64acbc5f5bfc2cce9b)
SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /b-c/src/policy/fees/block_policy_estimator.cpp:448:25
```
Note: This is marked a "refactor", because the code change does not affect 64-bit builds, and on the still remaining rare 32-bit builds today it is extremely unlikely to happen in production.
ACKs for top commit:
bensig:
ACK fa1d17d56c83d6ad89c1f688824ec0dc1c294012
ismaelsadeeq:
utACK fa1d17d56c83d6ad89c1f688824ec0dc1c294012
luke-jr:
Also, utACK fa1d17d56c83d6ad89c1f688824ec0dc1c294012 as an improvement.
Tree-SHA512: 696bf8e0dbe4777c84cb90e313c7f8f9ee90d4b3e64de1222f8472b2d9d0f3a0f6f027fda743dd6ca8c6aab94f404db7a65bb562a76000d9c33a8a39de28d8d4
2f5b1c5f80590ffa6b5a5bcfb21fddb1dc22e852 psbt: Fix `PSBTInputSignedAndVerified` bounds `assert` (Lőrinc)
Pull request description:
This PR fixes an off-by-one in a debug assertion in `PSBTInputSignedAndVerified`.
The function indexes `psbt.inputs[input_index]`, so the assertion must not allow indexing at `psbt.inputs.size()`.
Found during review: https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2685892867
ACKs for top commit:
optout21:
utACK 2f5b1c5f80590ffa6b5a5bcfb21fddb1dc22e852
maflcko:
lgtm ACK 2f5b1c5f80590ffa6b5a5bcfb21fddb1dc22e852
achow101:
ACK 2f5b1c5f80590ffa6b5a5bcfb21fddb1dc22e852
Tree-SHA512: cec613a9a38358d5caa243197d746baa129aebfd7fe697689f28e652f94c4683873c4676d5eb2eb909ea19de5e5f6e54ecc5f3162384a48f6f38a59273667689
