e1dc4afeeb6b3da4b7d9b0ca31ab21dda0a84478 test: Rename wallet name in restore attempt in wallet_assumeutxo (Fabian Jahr)
Pull request description:
I hope this fixes#34354
Based on this error from the logs `filesystem error: cannot remove: The process cannot access the file because it is being used by another process` it looks like there still exists a wallet file by the same name from the previous test case hasn't been cleaned up yet by it's process fully. This should be fixed by giving the failing `restorewallet` case a different wallet name and this shouldn't have any further effects on the rest of the test because is expected to fail anyway. The following (successful) call already uses a different wallet name.
ACKs for top commit:
achow101:
ACK e1dc4afeeb6b3da4b7d9b0ca31ab21dda0a84478
w0xlt:
ACK e1dc4afeeb6b3da4b7d9b0ca31ab21dda0a84478
rkrux:
ACK e1dc4afeeb6b3da4b7d9b0ca31ab21dda0a84478
Tree-SHA512: b5c53252a3b71fde150b29cc90cfd80a8678e3d7a39bcd6038e6722f2ac50d0a0db480e0a8ad43e39d4738971c39280415822e4d64c02895cbb6bd05ff3fc02e
fa61fadad1c3df0274c3ddd351b8d68a0c4fe644 doc: Fix wrong code in WITH_LOCK doxygen comment (MarcoFalke)
Pull request description:
The typo is harmless, but a bit confusing every time i read it
ACKs for top commit:
hebasto:
re-ACK fa61fadad1c3df0274c3ddd351b8d68a0c4fe644.
l0rinc:
ACK fa61fadad1c3df0274c3ddd351b8d68a0c4fe644
Tree-SHA512: 302a284198178954512267e8c0a5708738d77aac1cf609d8cbb386bee78d705f7e0df42a7bd8300afc18d42fa271c7f4cda932b1cbea33385622b3760bb95fad
6a8dbf9b9352db48580cd81c8dac027f3138fedf p2p: add validation check for initial self-announcement (frankomosh)
Pull request description:
This is a follow up to #34146 . Adds validation check to the initial self-announcement code path. `IsAddrCompatible()` check can prevent sending non-routable addresses to peers that don't support addrv2.
ACKs for top commit:
fjahr:
utACK 6a8dbf9b9352db48580cd81c8dac027f3138fedf
Crypt-iQ:
crACK 6a8dbf9b9352db48580cd81c8dac027f3138fedf
stratospher:
ACK 6a8dbf9. preserves the existing behaviour. also learnt that Addr-fetch ADDR processing logic allows receiving a self-announcement with 1 address [without disconnecting](b6c5d1e450) and won't be affected.
sedited:
ACK 6a8dbf9b9352db48580cd81c8dac027f3138fedf
Tree-SHA512: 988110d72fd698634111eb68c0204f42457b9b9b3d7b6ca3e11815cc702f6921266ae8f27f27aa31c3672efdb99478870fc4d1e8f5fa63aceae6f81521b31d8b
This prevents potential intermittend failures on windows when the wallet by the same name from the previous test case hasn't been cleaned up yet by it's process.
0aba464ce76522f1be3bb9e471b45438738de492 test: switch order of error code and message check (rkrux)
Pull request description:
I feel it'd be easier to debug intermittent test failures if the error message is present in the logs instead of error code. So, switching order of error code and message in the `try_rpc` function to aid error debugging.
Should help in debugging #34354 IMO. It's an intermittent failure on Windows that I can't reproduce and it's more difficult to figure out what could have gone wrong only by seeing the error code like below in the CI logs. Given that the functional tests pass, I don't see a harm in checking for error message first and throwing it in case of a mismatch.
```python
AssertionError: Unexpected JSONRPC error code -1
```
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the change.
* Bug fixes are most welcome when they come with steps to reproduce or an
explanation of the potential issue as well as reasoning for the way the bug
was fixed.
* Features are welcome, but might be rejected due to design or scope issues.
If a feature is based on a lot of dependencies, contributors should first
consider building the system outside of Bitcoin Core, if possible.
* Refactoring changes are only accepted if they are required for a feature or
bug fix or otherwise improve developer experience significantly. For example,
most "code style" refactoring changes require a thorough explanation why they
are useful, what downsides they have and why they *significantly* improve
developer experience or avoid serious programming bugs. Note that code style
is often a subjective matter. Unless they are explicitly mentioned to be
preferred in the [developer notes](/doc/developer-notes.md), stylistic code
changes are usually rejected.
-->
<!--
Bitcoin Core has a thorough review process and even the most trivial change
needs to pass a lot of eyes and requires non-zero or even substantial time
effort to review. There is a huge lack of active reviewers on the project, so
patches often sit for a long time.
-->
ACKs for top commit:
maflcko:
lgtm ACK 0aba464ce76522f1be3bb9e471b45438738de492
polespinasa:
lgtm ACK 0aba464ce76522f1be3bb9e471b45438738de492
fjahr:
utACK 0aba464ce76522f1be3bb9e471b45438738de492
brunoerg:
code review ACK 0aba464ce76522f1be3bb9e471b45438738de492
sedited:
ACK 0aba464ce76522f1be3bb9e471b45438738de492
Tree-SHA512: b09ba4b5d13a2c93a4a28a5c1b06af44a91295974236bb8326b74a988878c431e9ce0e19ec14bb98ac2b002da877abaa7da6a9851424453bcb494c0317b57227
75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb wallettool: Disallow creating new unnamed wallets (Ava Chow)
5875a9c502632eb5c74df07e41af38582da6e884 wallet: disallow unnamed wallets in createwallet and restorewallet (Ava Chow)
d30ad4a9129da04e249e3938f643dc47bf060e7e wallet, rpc: Use HandleWalletError in createwallet (Ava Chow)
Pull request description:
We've been moving in the direction that all wallets must have a name. Therefore, we shouldn't allow creating new unnamed wallets. `createwallet`, `restorewallet`, and the wallet tool's `create` and `createfromdump` all now require the user to provide a non-empty wallet name when creating/restoring a wallet.
The GUI is already enforcing this, but we were not enforcing it for RPCs or in the underlying `CreateWallet` and `RestoreWallet` functions.
Wallet migration does still need to be able to restore unnamed wallets, so there is a new argument to `RestoreWallet` to explicitly allow that behavior for migration only.
ACKs for top commit:
rkrux:
lgtm ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb
polespinasa:
re ACK 75b704df9d5ccb262d19ffe95b7ca6d1a934f3fb
Tree-SHA512: 8bde76d0b091e9276788c69412934af3426da2a7a69a00f94072d36c1a075cd41744ecdd5fef2b72870c1351b76aae061f124f716bb23f4839be20c464fc5ebd
fab055c907f1ab9ecac49e3d72909289a3b08c2d test: Scale NetworkThread close timeout with timeout_factor (MarcoFalke)
Pull request description:
Not sure if this fixes https://github.com/bitcoin/bitcoin/issues/34248, but scaling here probably makes sense, considering some CI setups run in nested VMs with a different arch system-qemu.
ACKs for top commit:
hebasto:
ACK fab055c907f1ab9ecac49e3d72909289a3b08c2d, the diff looks reasonable.
Tree-SHA512: 98f9b0bdc3b02b692a14129f88c05f2df0d1e11e4167ff5d0cc6a3a6efd8994a743e969e83c71cb534537f134e07ba9a5cba3eb2010a6b6cf69bec959faf2c43
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
I feel it'd be easier to debug intermittent test failures if the
error message is present in the logs instead of error code. So,
switching order of error code and message in the `try_rpc` function
to aid error debugging.
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>
Migration still needs to be able to restore unnamed wallets, so
allow_unnamed is added to RestoreWallet to explicitly allow that
behavior for migration only.
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
