d76ec4de14 fuzz: make sure PSBT serialization roundtrips (Antoine Poinsot)
Pull request description:
~~Invalid public keys were accepted in Musig2 partial signatures. Because we serialize invalid keys as the empty byte string, this would lead us to creating an invalid PSBT serializations.~~
~~This can be checked by reverting the first commit with the fix and simply running the target against the existing qa-assets corpus for the `psbt` harness.~~
This patch found the issue fixed in #34219 with a single run against the existing qa-assets corpus. It is useful to make sure there are no similar bugs, and we don't introduce roundtrip regressions outside of the specifc instance of accepting invalid public keys in Musig2 fields.
*(Edited on March 4 to only contain the fuzz harness patch)*
ACKs for top commit:
davidgumberg:
crACK https://github.com/bitcoin/bitcoin/commit/d76ec4de148724e6b384efb0a4180722ed30db10
achow101:
ACK d76ec4de14
dergoegge:
utACK d76ec4de14
brunoerg:
code review ACK d76ec4de14
Tree-SHA512: ab5f8d4e6a1781ecdef825e1a0e2793a6b553f36c923a4a35cb1af4070eead9d9780f6cc9a76235aa03462e52a129d15e61f631490b43651dc4395f3f1c005f3
faa68ed4bd test: Fix intermittent issue in wallet_assumeutxo.py (MarcoFalke)
Pull request description:
The test has many issues:
* It fails intermittently, due to the use of `-stopatheight` (https://github.com/bitcoin/bitcoin/issues/34710)
* Using `-stopatheight` is expensive, because it requires two restarts, making the test slow
* The overwritten `def setup_network` does not store the extra args via the `add_nodes` call, so if code is added in the future to restart a node, it may not pick up its global extra args.
Fix all issues by:
* Adding and using a fast `dumb_sync_blocks` util helper to achieve what `-stopatheight` doesn't achieve
* Calling `self.add_nodes(self.num_nodes, self.extra_args)` in the overridden `setup_network`
Can be tested via this diff:
```diff
diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp
index ab0e5ccb69..49384c10b8 100644
--- a/src/node/kernel_notifications.cpp
+++ b/src/node/kernel_notifications.cpp
@@ -61,2 +61,3 @@ kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state
if (m_stop_at_height && index.nHeight >= m_stop_at_height) {
+ LogInfo("Send shutdown signal after reaching stop height");
if (!m_shutdown_request()) {
diff --git a/src/util/tokenpipe.cpp b/src/util/tokenpipe.cpp
index c982fa6fc4..a5565ebd36 100644
--- a/src/util/tokenpipe.cpp
+++ b/src/util/tokenpipe.cpp
@@ -4,2 +4,3 @@
#include <util/tokenpipe.h>
+#include <util/time.h>
@@ -60,2 +61,3 @@ int TokenPipeEnd::TokenRead()
ssize_t result = read(m_fd, &token, 1);
+ UninterruptibleSleep(500ms);
if (result < 0) {
```
On master: The test fails
On this pull: The test passes
Fixes https://github.com/bitcoin/bitcoin/issues/34710
ACKs for top commit:
kevkevinpal:
ACK [faa68ed](https://github.com/bitcoin/bitcoin/pull/34728/commits/faa68ed4bdce6540f99d23c9b200ca575794a1b2)
achow101:
ACK faa68ed4bd
w0xlt:
ACK faa68ed4bd
Tree-SHA512: 6fcd52b6f6a5fa5a5e41e7b3cf5c733af62af9c60271e7d22c266aca90f2af67f91ffe80a3ed8b8d1a91d001700870f493207998bac988c4e3671a3a0dba7ba7
da7f70a532 test: use port 0 for I2P addresses in p2p_private_broadcast.py (Vasil Dimov)
a8ebcfd34c test: let connections happen in any order in p2p_private_broadcast.py (Vasil Dimov)
67696b207f net: extend log message to include attempted connection type (Vasil Dimov)
Pull request description:
If the following two events happen:
* (likely) the automatic 10 initial connections are not made to all
networks
* (unlikely) the network-specific logic kicks in almost immediately.
It is using exponential distribution with a mean of 5 minutes
(`rng.rand_exp_duration(EXTRA_NETWORK_PEER_INTERVAL)`).
So if both happen, then the 11th connection may not be the expected
private broadcast, but a network-specific connection.
Fix this by retrieving the connection type from
`destinations_factory()`. This is more flexible because it allows
connections to happen in any order and does not break if e.g. the 11th
connection is not the expected first private broadcast.
This also makes the test run faster:
before: 19-44 sec
now: 10-25 sec
because for example there is no need to wait for the initial 10
automatic outbound connections to be made in order to proceed.
Fixes: https://github.com/bitcoin/bitcoin/issues/34387
ACKs for top commit:
achow101:
ACK da7f70a532
andrewtoth:
ACK da7f70a532
mzumsande:
Code Review ACK da7f70a532
Tree-SHA512: 7c293e59c15c148a438e0119343b05eb278205640658c99336d4caf4848c5bae92b48e15f325fa616cbc9d5f394649abfa02406a76e802cffbd3d312a22a6885
fa48f8c865 test: Add missing timeout_factor to zmq socket (MarcoFalke)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/34189
Otherwise, the test may intermittently fail on slow CI systems that have `--timeout-factor=` properly set.
It can be tested by running `./bld-cmake/test/functional/interface_zmq.py --timeout-factor=10` with this diff:
```diff
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index c7be6abc3a..b14cf2aee6 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -166,2 +166,3 @@ void ValidationSignals::SyncWithValidationInterfaceQueue()
LOG_EVENT(fmt, local_name, __VA_ARGS__); \
+ UninterruptibleSleep(45ms); \
event(); \
diff --git a/test/functional/interface_zmq.py b/test/functional/interface_zmq.py
index 6717007626..eee377daea 100755
--- a/test/functional/interface_zmq.py
+++ b/test/functional/interface_zmq.py
@@ -176,3 +176,3 @@ class ZMQTest (BitcoinTestFramework):
for sub in subscribers:
- sub.socket.set(zmq.RCVTIMEO, recv_timeout*1000)
+ sub.socket.set(zmq.RCVTIMEO, int(recv_timeout * 1000))
@@ -271,3 +271,3 @@ class ZMQTest (BitcoinTestFramework):
[(topic, address) for topic in ["hashblock", "hashtx"]],
- recv_timeout=2) # 2 second timeout to check end of notifications
+ recv_timeout=0.2) # 2 second timeout to check end of notifications
self.disconnect_nodes(0, 1)
```
Before this pull: Test fails with `zmq.error.Again: Resource temporarily unavailable`
After this pull: Test passes
ACKs for top commit:
l0rinc:
code review ACK fa48f8c865
achow101:
ACK fa48f8c865
Tree-SHA512: 5ff8bdd807ff4b644daa634bb7b469da3da3915f58afba63a90e662df99cbebc86636e34e2b1b313c8629773aef2a239fb3025226a84d2ec22f6ecd4cea666c4
bbc8f1e0a7 ipc mining: Prevent ``Assertion `m_node.chainman' failed`` errors on early startup (Ryan Ofsky)
a7cabf92e4 init refactor: Only initialize node.notifications one time (Ryan Ofsky)
c8e332cb33 init refactor: Remove node.init accesss in AppInitInterfaces (Ryan Ofsky)
Pull request description:
This fixes ``Assertion `m_node.chainman' failed`` errors first reported https://github.com/bitcoin/bitcoin/issues/33994#issuecomment-3602551596 when IPC mining methods are called before ChainstateManager is loaded.
The fix works by making the `Init.makeMining` method wait until chainstate data is loaded. It's probably the simplest possible fix but other alternatives like moving the wait to `Mining.createNewBlock` were discussed in the thread https://github.com/bitcoin/bitcoin/pull/34661#discussion_r2848176298 and could be implemented later without changes to clients.
ACKs for top commit:
Sjors:
utACK bbc8f1e0a7
ismaelsadeeq:
ACK bbc8f1e0a7
achow101:
ACK bbc8f1e0a7
Tree-SHA512: 3e2e4e28ccff364b2303efd06ce337a229c28609076638500acb29559f716a15ad99409c6970ce9ad91776d53e3f9d959f1bbbd144ea9a4a2fb578ddbf2da267
When unloading a wallet, there may be unexecuted callbacks in the
validation interface queue that can still execute after we have
completed all of the other wallet shutdown tasks. Instead of letting
these run in the background, once the notifications are disconnected,
wait for the queue to drain before continuing with wallet shutdown.
408d5b12e8 test: include response body in non-JSON HTTP error msg (Matthew Zipkin)
9dc653b3b4 test: threadpool, add coverage for all Submit() errors (furszy)
ce2a984ee3 test: cleanup, use HasReason in threadpool_tests.cpp (l0rinc)
d9c6769d03 test: refactor, decouple HasReason from test framework machinery (furszy)
dbbb780af0 test: move and simplify BOOST_CHECK ostream helpers (Hodlinator)
3b7cbcafcb test: ensure Stop() thread helps drain the queue (seduless)
ca101a2315 test: coverage for queued tasks completion after interrupt (furszy)
bf2c607aaa threadpool: active-wait during shutdown (furszy)
e88d274430 test: add threadpool Start-Stop race coverage (furszy)
8cd4a4363f threadpool: guard against Start-Stop race (furszy)
9ff1e82e7d test: cleanup, block threads via semaphore instead of shared_future (l0rinc)
Pull request description:
A few follow-ups to #33689, includes:
1) `ThreadPool` active-wait during shutdown:
Instead of just waiting for workers to finish processing tasks, `Stop()` now helps them actively.
This speeds up the JSON-RPC and REST server shutdown, resulting in a faster node shutdown when many requests remain unhandled. This wasn't included in the original PR due to the behavior change this introduces.
2) Decouple `HasReason` from the unit test framework machinery
This avoids providing the entire unit test framework dependency to low-level tests that only require access to the `HasReason` utility class. Examples are: `reverselock_tests.cpp`, `sync_tests.cpp`, `util_check_tests.cpp`, `util_string_tests.cpp`, `script_parse_tests.cpp` and `threadpool_tests.cpp`. These tests no longer gain access to unnecessary components like the chainstate, node context, caches, etc. It includes l0rinc's `threadpool_tests.cpp` `HasReason` changes.
3) Include response body in non-JSON HTTP error messages
Straight from pinheadmz [comment](https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2783817192), it makes debugging CI issues easier.
ACKs for top commit:
maflcko:
review ACK 408d5b12e8🕗
achow101:
ACK 408d5b12e8
hodlinator:
re-ACK 408d5b12e8
Tree-SHA512: 57aa0ef96886f32bf95a0bd7f87c878d31c9df9e34cb96de615eee703ce0824b5cfdf8f5c9cd19a3594559994295b5810c38c94f5efd6291cbbd83a95473357a
3281824ecf fuzz: prevent invalid `FRESH` entries and surface `BatchWrite` errors (Lőrinc)
780f460635 fuzz: avoid invalid `AddCoin` overwrites (Lőrinc)
d7e0d510f2 fuzz: make `AddCoins` query view for overwrites (Lőrinc)
b8fa6f0f70 util: introduce `TrySub` to prevent unsigned underflow (Lőrinc)
Pull request description:
### Problem
This is an alternative approach to #34647, fixes#34645.
### Fix
First, add `CheckedSub` and use it for decrements of `m_dirty_count` and `cachedCoinsUsage`, so unsigned underflows turn into immediate failures instead of silently wrapping and only failing later.
<details><summary>Assertion `j <= i' failed.</summary>
```bash
util/overflow.h:44 T CheckedSub(const T, const U) [T = unsigned long, U = bool]: Assertion `j <= i' failed.
==72817== ERROR: libFuzzer: deadly signal
#0 0x556e9225eab5 in __sanitizer_print_stack_trace (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x191dab5) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#1 0x556e921acafc in fuzzer::PrintStackTrace() (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x186bafc) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#2 0x556e92191bb7 in fuzzer::Fuzzer::CrashCallback() (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1850bb7) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#3 0x7164cfc458cf (/lib/x86_64-linux-gnu/libc.so.6+0x458cf) (BuildId: ae7440bbdce614e0e79280c3b2e45b1df44e639c)
#4 0x7164cfca49bb in __pthread_kill_implementation nptl/pthread_kill.c:43:17
#5 0x7164cfca49bb in __pthread_kill_internal nptl/pthread_kill.c:89:10
#6 0x7164cfca49bb in pthread_kill nptl/pthread_kill.c:100:10
#7 0x7164cfc4579d in raise signal/../sysdeps/posix/raise.c:26:13
#8 0x7164cfc288cc in abort stdlib/abort.c:73:3
#9 0x556e92f9d591 in assertion_fail(std::source_location const&, std::basic_string_view<char, std::char_traits<char>>) /mnt/my_storage/bitcoin/src/util/check.cpp:41:5
#10 0x556e9250daf0 in bool&& inline_assertion_check<false, bool>(bool&&, std::source_location const&, std::basic_string_view<char, std::char_traits<char>>) /mnt/my_storage/bitcoin/src/util/check.h:90:13
#11 0x556e9250daf0 in unsigned long CheckedSub<unsigned long, bool>(unsigned long, bool) /mnt/my_storage/bitcoin/src/util/overflow.h:44:5
#12 0x556e9250daf0 in CoinsViewCacheCursor::NextAndMaybeErase(std::pair<COutPoint const, CCoinsCacheEntry>&) /mnt/my_storage/bitcoin/src/coins.h:282:25
#13 0x556e92507eb2 in (anonymous namespace)::MutationGuardCoinsViewCache::BatchWrite(CoinsViewCacheCursor&, uint256 const&) /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:90:75
#14 0x556e92c17a2b in CCoinsViewCache::Flush(bool) /mnt/my_storage/bitcoin/src/coins.cpp:282:11
#15 0x556e924fb732 in TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_1::operator()() const /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:135:34
#16 0x556e924fb732 in unsigned long CallOneOf<TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_10, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_11>(FuzzedDataProvider&, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_0, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_1, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_2, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_3, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_4, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_5, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_6, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_7, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_8, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_9, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_10, TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool)::$_11) /mnt/my_storage/bitcoin/src/test/fuzz/util.h:42:27
#17 0x556e924fb732 in TestCoinsView(FuzzedDataProvider&, CCoinsViewCache&, CCoinsView&, bool) /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:114:9
#18 0x556e92503b0c in coins_view_overlay_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) /mnt/my_storage/bitcoin/src/test/fuzz/coins_view.cpp:404:5
#19 0x556e92bcb7a5 in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/lib/gcc/x86_64-linux-gnu/15/../../../../include/c++/15/bits/std_function.h:593:9
#20 0x556e92bcb7a5 in test_one_input(std::span<unsigned char const, 18446744073709551615ul>) /mnt/my_storage/bitcoin/src/test/fuzz/fuzz.cpp:88:5
#21 0x556e92bcb7a5 in LLVMFuzzerTestOneInput /mnt/my_storage/bitcoin/src/test/fuzz/fuzz.cpp:216:5
#22 0x556e9219318f in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x185218f) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#23 0x556e92192799 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1851799) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#24 0x556e92194139 in fuzzer::Fuzzer::MutateAndTestOne() (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1853139) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#25 0x556e92194c95 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1853c95) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#26 0x556e92181255 in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x1840255) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#27 0x556e921ad696 in main (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x186c696) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
#28 0x7164cfc2a577 in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#29 0x7164cfc2a63a in __libc_start_main csu/../csu/libc-start.c:360:3
#30 0x556e921757e4 in _start (/mnt/my_storage/bitcoin/build_fuzz/bin/fuzz+0x18347e4) (BuildId: d77c4d5f9dfd38ea06fab463f49341735205e109)
NOTE: libFuzzer has rudimentary signal handlers.
Combine libFuzzer with AddressSanitizer or similar for better crash reports.
SUMMARY: libFuzzer: deadly signal
MS: 2 PersAutoDict-CopyPart- DE: "\005\000"-; base unit: ecb626aff8724f0fdde38a0a6965718f2096d474
artifact_prefix='/tmp/fuzz_artifacts/'; Test unit written to /tmp/fuzz_artifacts/crash-1d19026c1a23f08bfe693fd684a56ce51187c6e5
./build_fuzz/bin/fuzz /tmp/fuzz_corpus/coins_view_overlay -max_total_time=3600 -rss_limit_mb=2560 -artifact_prefix=/tmp/fuzz_artifacts/ >fuzz-16.log 2>&1
```
</details>
The coins view fuzz targets can call `AddCoin`/`AddCoins` and construct `BatchWrite` cursors in ways that violate `CCoinsViewCache` caller contracts. These invalid states can trigger `BatchWrite` `std::logic_error` and can desync dirty-entry accounting (caught by `Assume(m_dirty_count == 0)` currently).
Make the fuzzer avoid generating invalid states instead of catching and resetting:
* Derive `AddCoin`’s `possible_overwrite` from `PeekCoin`, so `possible_overwrite=false` is only used when the outpoint is absent - similarly to https://github.com/bitcoin/bitcoin/blob/67c0d1798e6147f48d4bafc2c9e5ff30f2a62340/src/test/fuzz/coinscache_sim.cpp#L312-L317
- Only use `AddCoins(check=false)` when we have confirmed the txid has no unspent outputs; otherwise fall back to `check=true` so `AddCoins` determines overwrites via the view.
- When constructing a `CoinsViewCacheCursor`, avoid setting `FRESH` when the parent already has an unspent coin, and ensure `FRESH` implies `DIRTY`.
### Fuzzing
The original error could be reproduced in ~10 minutes using `coins_view_overlay`. I ran the `coins_view`, `coins_view_db`, `coins_view_overlay`, and `coinscache_sim` fuzzers for this PR overnight and they didn't fail anymore.
ACKs for top commit:
achow101:
ACK 3281824ecf
sipa:
ACK 3281824ecf. Ran the 4 relevant fuzz tests for ~1 CPU day each. Will run more overnight.
andrewtoth:
ACK 3281824ecf
Tree-SHA512: b8155e8d21740eb7800e373c27a8a1457eb84468c24af879bac5a1ed251ade2aec99c34a350a31f2ebb74e41bb7380bf20214d38d14fe23310a43282d2434fb7
5cd57943b8 test: verify node state after restart in assumeutxo (Yash Bhutwala)
Pull request description:
## Summary
This PR replaces the TODO comment in `wallet_assumeutxo.py` (line 242) with actual test assertions that verify node and wallet behavior after a restart during assumeutxo background sync.
## Changes
The new tests verify:
- Two chainstates exist (background validation not complete)
- Background chainstate is still at `START_HEIGHT`
- Snapshot chainstate has synced to at least `PAUSE_HEIGHT`
- Wallets cannot be loaded after restart (expected behavior)
- Wallet backup from before snapshot height cannot be restored
## Motivation
During implementation, I discovered that **wallets cannot be loaded after a node restart during assumeutxo background sync**. This is expected behavior because:
- The wallet loading code checks if required blocks are available for rescanning
- During assumeutxo background sync, blocks before the snapshot are not available
- This applies to all wallets, including watch-only wallets created at the snapshot height
This is a valuable test addition because it documents this expected behavior and ensures it doesn't regress. Users should be aware that if they restart their node during assumeutxo background sync, they won't be able to load their wallets until the background sync completes.
## Related
refs #28648
Addresses the TODO comment that was originally added as part of the assumeutxo wallet test implementation.
ACKs for top commit:
Bicaru20:
Code review ACK 5cd57943b8
achow101:
ACK 5cd57943b8
fjahr:
ACK 5cd57943b8
sedited:
ACK 5cd57943b8
polespinasa:
code lgtm ACK 5cd57943b8
Tree-SHA512: 4a125c5247168da2bbf4d855b4150ca453bb5e4cce1a62e633ce5e43acdc2c58883a6a94dcc46b38f8b4c44206fe42cec4db151a76aded53d8ea433ea5eb2562
50cf6838e6 wallet: rpc: manpage: fix example missing `fee_rate` argument (SomberNight)
Pull request description:
The function signature for the `send` RPC is:
```
send [{"address":amount,...},{"data":"hex"},...] ( conf_target "estimate_mode" fee_rate options version )
```
The last example in the manpage is missing the `fee_rate` arg, but is trying to specify the `options` arg, by index. The parser confuses the intended `options` arg as the missing `fee_rate` arg.
See:
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
error code: -8
error message:
Cannot specify both conf_target and fee_rate. Please provide either a confirmation target in blocks for automatic fee estimation, or an explicit fee rate.
```
vs
```
$ bitcoin-cli -rpcuser=doggman -rpcpassword=donkey -rpcport=18554 -regtest send '{"bcrt1qusm48zmlzwr32csxdw4ar7atw260h22c9ten9l": 0.1}' 1 economical null '{"add_to_wallet": false, "inputs": [{"txid":"0b7e1a471dc948b7a6187936b16e6d7d9833629b2f9dd8a392eb89928f63aaad", "vout":0}]}'
{
"psbt": "cHNidP8BAHECAAAAAa2qY4+SieuSo9idL5tiM5h9bW6xNnkYprdIyR1HGn4LAAAAAAD9////AkR2DwQAAAAAFgAUpLDwJu+wFRHLQAgKAb0psk7UVd2AlpgAAAAAABYAFOQ3U4t/E4cVYgZrq9H7q3K0+6lYAAAAAAABAIUCAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA/////wQC4wMA/////wLIF6gEAAAAABYAFLMY1zihXrefAA0DA5nld4MCPjkrAAAAAAAAAAAmaiSqIant4vYcP3HR3v0/qZnfo2lTdVxpBol5mWK0i+vYNpdOjPkAAAAAAQEfyBeoBAAAAAAWABSzGNc4oV63nwANAwOZ5XeDAj45KwEIawJHMEQCIElTV4pbUrsPR9qHWcioowVv3QVWHizxwevfD0u/I8YyAiBCY3OzF81PSLM00h4ueQkehYuxDFZu7Jk51iejphKnnwEhA0VKdYVSyBpWoxBwTDOupB58Fi3mEBs+u+OOqEYVd2sZACICA98YLWyH7dBCfXVxe7woiLSTgV1mJN8Zc8KgZ77pVSg+GNBMeT5UAACAAQAAgAAAAIABAAAAbAAAAAAA",
"txid": "625b71b314a6ac4f738634e29dc007cd5edc0427c1ae96ab706d06a62910cea2",
"hex": "02000000000101adaa638f9289eb92a3d89d2f9b6233987d6d6eb1367918a6b748c91d471a7e0b0000000000fdffffff0244760f0400000000160014a4b0f026efb01511cb40080a01bd29b24ed455dd8096980000000000160014e437538b7f13871562066babd1fbab72b4fba9580247304402204953578a5b52bb0f47da8759c8a8a3056fdd05561e2cf1c1ebdf0f4bbf23c6320220426373b317cd4f48b334d21e2e79091e858bb10c566eec9939d627a3a612a79f012103454a758552c81a56a310704c33aea41e7c162de6101b3ebbe38ea84615776b1900000000",
"complete": true
}
```
ACKs for top commit:
svanstaa:
tACK https://github.com/bitcoin/bitcoin/commit/50cf6838e6aa51e0d712cbc1e13d686253bc8fe0
kannapoix:
Tested ACK 50cf6838e6
achow101:
ACK 50cf6838e6
rkrux:
tACK 50cf6838e6
theStack:
Tested ACK 50cf6838e6
Tree-SHA512: 499701729038cd863b612698098a73ce995589fc5ab08a2962f8edf1ff3cb3de6f7090e04722ca13ba7707a566fa3750ae549b6ad55750a3d01127eb6b94a79f
e8f8b74a46 test: index, improve txospenderindex_initial_sync() test code (furszy)
ac3bea07cd test: improve rpc_gettxspendingprevout.py code (furszy)
Pull request description:
Fixes#34637.
Was reviewing #34637 and, while reading the new txospender index
test code for the first time, found it could use some cleanups. Finding
stuff in there is harder than it should be due to the amount of dup code.
The first commit cleans up `rpc_gettxspendingprevout.py` by introducing
helper functions to avoid repeating the same dicts everywhere, using
for-loops instead of duplicating the same checks for each node, and
renaming variables to better reflect what they actually represent.
The second commit reorganizes `txospenderindex_initial_sync()`
moving index initialization after the test setup phase, since the index
doesn't participate in it anyway. It adds a post-sync check to catch
cases where `Sync()` aborted prematurely.
Note:
This is just a pre-work for deeper index changes I'm cooking.
ACKs for top commit:
achow101:
ACK e8f8b74a46
sedited:
Re-ACK e8f8b74a46
w0xlt:
reACK e8f8b74a46
Tree-SHA512: 3f7026712ab20a43f376afa28c683dcd5daec8ed1bbf1c36d7ec6bbf231f468d4de74efae4aa8295ff3afb83986286ccaf31c03b34e45fc9971652f064791ed0
c462e54f9d test: don't always assert NUM_PRIVATE_BROADCAST_PER_TX broadcasts (Vasil Dimov)
3710566305 test: move abortprivatebroadcast test at the end (Vasil Dimov)
Pull request description:
_test: move abortprivatebroadcast test at the end_
The piece of `p2p_private_broadcast.py` which tests the correctness of
`abortprivatebroadcast` issues a new `sendrawtransaction` call. That
call schedules up to 3 new connections: peer=13, peer=14 and possibly
peer=15 before it gets aborted.
These up to 3 in-the-process-of-opening private broadcast connections
have `CNode::m_connected` set early - when the `CNode` object is
created. Later in the test the mock time is advanced by 20 minutes and
those "old" connections pick a transaction for rebroadcast but that
triggers `PRIVATE_BROADCAST_MAX_CONNECTION_LIFETIME` immediately:
```
2026-02-21T13:28:14.209766Z [privbcast] [net.cpp:4006] [CNode] [net] Added connection peer=20
2026-02-21T13:28:14.309792Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net.cpp:4074] [PushMessage] [net] sending inv (37 bytes) peer=20
2026-02-21T13:28:14.309801Z (mocktime: 2026-02-21T13:48:14Z) [msghand] [net_processing.cpp:5745] [SendMessages] [privatebroadcast] Disconnecting: did not complete the transaction send within 180 seconds, peer=20
```
This prematurely stops the private broadcast connection and results in
a failure like:
```
AssertionError: ... not({} == {'ping': 1, 'tx': 1})
```
---
_test: don't always assert NUM_PRIVATE_BROADCAST_PER_TX broadcasts_
In `p2p_private_broadcast.py` in the function `check_broadcasts()` we
should assert that the broadcast was done to `broadcasts_to_expect`
peers, not to `NUM_PRIVATE_BROADCAST_PER_TX`. This is because in the
"Basic" test we check the first broadcast manually because it is done to
`nodes[1]` and then check the other two by
`check_broadcasts(..., NUM_PRIVATE_BROADCAST_PER_TX - 1, ...)`.
The first broadcast might not have fully concluded by the time we call
`check_broadcasts()` to check the remaining 2.
Demanding always `NUM_PRIVATE_BROADCAST_PER_TX` can lead to:
```
Traceback (most recent call last):
File "/home/vd/gh/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 142, in main
self.run_test()
~~~~~~~~~~~~~^^
File "/tmp/build/clang22/test/functional/p2p_private_broadcast.py", line 347, in run_test
self.check_broadcasts("Basic", txs[0], NUM_PRIVATE_BROADCAST_PER_TX - 1, NUM_INITIAL_CONNECTIONS + 1)
~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/tmp/build/clang22/test/functional/p2p_private_broadcast.py", line 313, in check_broadcasts
assert_greater_than_or_equal(sum(1 for p in peers if "received" in p), NUM_PRIVATE_BROADCAST_PER_TX)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/vd/gh/bitcoin/bitcoin/test/functional/test_framework/util.py", line 94, in assert_greater_than_or_equal
raise AssertionError("%s < %s" % (str(thing1), str(thing2)))
AssertionError: 2 < 3
```
ACKs for top commit:
l0rinc:
ACK c462e54f9d
achow101:
ACK c462e54f9d
andrewtoth:
ACK c462e54f9d
Tree-SHA512: 0de8d0eae079eeedc3bfad39df8129a8fa0d7734bdc03b4fb3e520a2f13a187d68118ffc210556af125d634f0ff51a1b081b34a023ac68a1c6a0caf541cecb91
fd06157d14 test: Add coverage for restarted node without any block sync (Fabian Jahr)
3d7ab7ecb7 rpc, test: Address feedback from #29668 (Fabian Jahr)
312919c9dd test: Indices can not start based on block data without undo data (Fabian Jahr)
a9a3b29dd6 index: Check availability of undo data for indices (Fabian Jahr)
881ab4fc82 support multiple block status checks in CheckBlockDataAvailability (furszy)
Pull request description:
Currently, we check that `BLOCK_HAVE_DATA` is available for all blocks an index needs to sync during startup. However, for `coinstatsindex` and `blockfilterindex` we also need the undo data for these blocks. If that data is missing in the blocks, we are currently still starting to sync each of these indices and then crash later when we encounter the missing data.
This PR adds explicit knowledge of which block data is needed for each index and then checks its availability during startup before initializing the sync process on them.
This also addresses a few open comments from #29668 in the last commit.
ACKs for top commit:
achow101:
ACK fd06157d14
furszy:
utACK fd06157d14
sedited:
Re-ACK fd06157d14
Tree-SHA512: e2ed81c93372b02daa8ddf2819df4164f96d92de05b1d48855410ecac78d5fcd9612d7f0e63a9d57d7e75a0b46e1bea278e43ea87f2693af0220d1f9c600e416
afea2af139 net: reduce log level for PCP/NAT-PMP NOT_AUTHORIZED failures (ANAVHEOBA)
Pull request description:
Cherry-picks (and tweaks) a commit from #34117 which the ANAVHEOBA
did not follow up with when changes were requested.
The tweak here is to log once at `LogWarning`, so that users have a chance
to spot misconfiguration.
----
Users running on home networks with routers that don't support PCP (Port
Control Protocol) or NAT-PMP port mapping receive frequent warning-level
log messages every few minutes:
"pcp: Mapping failed with result NOT_AUTHORIZED (code 2)"
This is expected behavior for many consumer routers that have PCP
disabled by default, not an actionable error.
Add explicit constants for the NOT_AUTHORIZED result code (value 2)
for both NAT-PMP and PCP protocols. Log the first NOT_AUTHORIZED
failure at warning level for visibility, then downgrade subsequent
occurrences to LogDebug to avoid log noise. Other failure types
continue to warn unconditionally.
Fixes#34114
ACKs for top commit:
achow101:
ACK afea2af139
sedited:
ACK afea2af139
Tree-SHA512: 43dad9f3cca0ef9b82446045a3ccd90418cd5595c9965e938d9d539bbba863dde6b4a358adbee56f8d97d6efbf947eb5ddbbaf426faefcf3b1e36e4c8edb0d94
fa90d44a22 test: Fix intermittent issues in feature_assumevalid.py (MarcoFalke)
Pull request description:
The test has many issues:
* The `send_blocks_until_disconnected` seems to imply to wait until a disconnect happens. However, in reality it will blindly send all blocks and then return early, when done or when a disconnect happens. This will cause test failures when python is running faster than Bitcoin Core, for example when using sanitizers.
* The `assert_debug_log` scopes are bloated, which makes finding the above issue harder. This is, because a test failure will basically print the 1000+ line debug log excerpt twice, with the second instance stripped of useful python test logs. Also, the checks are less precise, if they happen over a larger scope/snippet.
Fix all issues, by:
* Removing `send_blocks_until_disconnected` and just sending the blocks that are needed to get the disconnect and then explicitly waiting for the disconnect.
* Reduce the scopes of the debug log checks. This can be reviewed with the git options `--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space`.
ACKs for top commit:
l0rinc:
ACK fa90d44a22
achow101:
ACK fa90d44a22
Tree-SHA512: 8a18fa0009fb40a47317976e20b230625182874391babe0da3c3dfd55d2cc8e2da7008ce8f38691d01cd37ae02cac7c9e88b8193c2ed66d3621ee3f792649f71
2a1d0db799 doc: Mention private broadcast RPCs in release notes (Andrew Toth)
c3378be10b test: Cover abortprivatebroadcast in p2p_private_broadcast (Andrew Toth)
557260ca14 rpc: Add abortprivatebroadcast (Andrew Toth)
15dff452eb test: Cover getprivatebroadcastinfo in p2p_private_broadcast (Andrew Toth)
996f20c18a rpc: Add getprivatebroadcastinfo (Andrew Toth)
5e64982541 net: Add PrivateBroadcast::GetBroadcastInfo (Andrew Toth)
573bb542be net: Store recipient node address in private broadcast (Andrew Toth)
Pull request description:
Follow up from #29415
Sending a transaction via private broadcast does not have any way for a user to track the status of the transaction before it gets returned by another peer. The default logs have been removed as well in #34267. Nor is there any way to abort a transaction once it has been added to the private broadcast queue.
This adds two new RPCs:
- `getprivatebroadastinfo` returns information about what transactions are in the private broadcast queue, including all the peers' addresses we have chosen and timestamps.
- `abortprivatebroadcast` stops broadcasting a transaction in the private broadcast queue.
ACKs for top commit:
nervana21:
tACK 2a1d0db799
achow101:
ACK 2a1d0db799
l0rinc:
ACK 2a1d0db799
danielabrozzoni:
tACK 2a1d0db799
sedited:
ACK 2a1d0db799
Tree-SHA512: cc8682d0be68a57b42bea6e3d091da2b80995d9e6d3b98644cb120a05c2b48a97c2e211173289b758c4f4e23f1d1a1f9be528a9b8c6644f71d1dd0ae5f673326
726b3663cc http: properly respond to HTTP request during shutdown (furszy)
59d24bd5dd threadpool: make Submit return Expected instead of throwing (furszy)
Pull request description:
Fixes#34573.
As mentioned in https://github.com/bitcoin/bitcoin/issues/34573#issuecomment-3891596958, the ThreadPool PR (#33689) revealed an existing issue.
Before that PR, we were returning an incorrect error "Request rejected because http work queue depth exceeded" during shutdown for unhandled requests (we were not differentiating between "queue depth exceeded" and "server interrupted" errors). Now, with the ThreadPool inclusion, we return the proper error but we don't handle it properly.
This PR improves exactly that. Handling the missing error and properly returning it to the user.
The race can be reproduced as follows:
1) The server receives an http request.
2) Processing of the request is delayed, and shutdown is triggered in the meantime.
3) During shutdown, the libevent callback is unregistered and the threadpool interrupted.
4) The delayed request (step 2) resumes and tries to submit a task to the now-interrupted server.
Reproduction test can be found https://github.com/bitcoin/bitcoin/pull/34577#issuecomment-3902672521.
Also, to prevent this kind of issue from happening again, this PR changes task submission
to return the error as part of the function's return value using `util::Expected` instead of
throwing the exception. Unlike exceptions, which require extra try-catch blocks and can be
ignored, returning `Expected` forces callers to explicitly handle failures, and attributes
like `[[nodiscard]]` allow us catch unhandled ones at compile time.
ACKs for top commit:
achow101:
ACK 726b3663cc
sedited:
ACK 726b3663cc
pinheadmz:
re-ACK 726b3663cc
andrewtoth:
ACK 726b3663cc
hodlinator:
re-ACK 726b3663cc
Tree-SHA512: ef026e299adde1148c9fc575e7d937e957bf0ddedfc1cf081941b568736417c2eefcd8bc8c8aea795d7347040ed05da4371bddcdbda7d385e04bf4dc8d875780
8966352df3 doc: add release notes (ismaelsadeeq)
704a09fe71 test: ensure fee estimator provide fee rate estimate < 1 s/vb (ismaelsadeeq)
243e48cf49 fees: delete unused dummy field (ismaelsadeeq)
fc4fbda42a fees: bump fees file version (ismaelsadeeq)
b54dedcc85 fees: reduce `MIN_BUCKET_FEERATE` to 100 (ismaelsadeeq)
Pull request description:
This is a simple PR that updates the block policy estimator’s `MIN_BUCKET_FEERATE` constant to be 100, which is identical to the policy `DEFAULT_MIN_RELAY_TX_FEE`.
This change enables the block policy fee rate estimator to return sub-1 sat/vB fee rate estimates.
The change is correct because the estimator creates buckets of fee rates from
`MIN_BUCKET_FEERATE`,
`MIN_BUCKET_FEERATE` + `FEE_SPACING`,
`MIN_BUCKET_FEERATE` + `2 * FEE_SPACING`,
… up to `MAX_BUCKET_FEERATE`.
This means it will record sub-1 sat/vB fee rates in the buckets and may return them as a fee rate estimate when that bucket is the lowest one with sufficient transactions for a given target.
---
While touching this part of the fee estimator code, this PR got rid of the dummy value persisted in the file
ACKs for top commit:
achow101:
ACK 8966352df3
musaHaruna:
ACK [8966352](https://github.com/bitcoin/bitcoin/commit/8966352df3fc56fd2c00a45eecd292a240a34546)
polespinasa:
ACK 8966352df3
Tree-SHA512: 9424e0820fcbe124adf5e5d9101a8a2983ba802887101875b2d1856d700c8b563d7a6f6ca3e3db29cb939f786719603aadbf5480d59d55d0951ed1c0caa49868
24699fec84 doc: Add initial asmap data documentation (Fabian Jahr)
bab085d282 ci: Use without embedded asmap build option in one ci job (Fabian Jahr)
e53934422a doc: Expand documentation on asmap feature and tooling (Fabian Jahr)
6244212a55 init, net: Implement usage of binary-embedded asmap data (Fabian Jahr)
6202b50fb9 build: Generate ip_asn.dat.h during build process (Fabian Jahr)
634cd60dc8 build: Add embedded asmap data (Fabian Jahr)
Pull request description:
This is the final in a series of PRs that implement the necessary changes for embedding of asmap data into the binary. This last part add the initial asmap data, implements the build changes and adds further documentation.
Currently an asmap file needs to be acquired by there user from some location or the user needs to generate one themselves. Then they need to move the file to the right place in datadir or pass the path to the file as `-asmap=PATH` in order to use the asmap feature. The change here allows for builds to embed asmap data into the bitcoind binary which makes it possible to use the feature without handling of the asmap file by the user. If the user starts bitcoind with `-asmap` the embedded data will be used for bucketing of nodes.
The data lives in the repository at `src/node/data/ip_asn.dat` and can be replaced with a new version at any time. The idea is that the data should be updated with every release. By default the data at that location is embedded into the binary but there is also a build option to prevent this (`-DWITH_EMBEDDED_ASMAP=OFF`). In this case the original behavior of the `-asmap` option is maintained.
ACKs for top commit:
achow101:
ACK 24699fec84
sipa:
ACK 24699fec84
hodlinator:
ACK 24699fec84
Tree-SHA512: c2e33dbeea387efdfd3d415432bf8fa64de80f272c1207015ea53b85bb77f5c29f1dae5644513a23c844a98fb0a4bb257bf765f38b15bfc4c41984f0315b4c6a
24f93c9af7 release note (Pol Espinasa)
331a5279d2 wallet, rpc:remove settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR removes the settxfee RPC and paytxfee setting (Bitcoin Core 31.0).
These two features were deprecated in https://github.com/bitcoin/bitcoin/pull/31278.
ACKs for top commit:
achow101:
ACK 24f93c9af7
w0xlt:
reACK 24f93c9af7
Tree-SHA512: e090f1a72ba2cbeba7c982dd51dfdcf6baf0a164827337cf56fd85f733e143b8d6116b8cd53c59c812cacef193dfa0b101a830fc455e32bf225e8505e7b2a554
fa0677d131 refactor: Use SpanReader over DataStream (MarcoFalke)
fad3eb3956 refactor: Use SpanReader over DataStream (MarcoFalke)
fa06e26764 refactor: [qt] Use SpanReader to avoid two vector copies (MarcoFalke)
fabd4d2e2e refactor: Avoid UB in SpanReader::ignore (MarcoFalke)
fa20bc2ec2 refactor: Use empty() over eof() in the streams interface (MarcoFalke)
fa879db735 test: Read debug log for self-checking comment (MarcoFalke)
Pull request description:
This changes all places, where possible, to use SpanReader over DataStream. This makes the code easier to read and reason about, because `SpanReader` can never write data. Also, the code should be minimally faster, because it avoids a full redundant copy of the whole vector of bytes.
ACKs for top commit:
stickies-v:
re-ACK fa0677d131
achow101:
ACK fa0677d131
janb84:
re ACK fa0677d131
sipa:
crACK fa0677d131
Tree-SHA512: 1d9f43fc6e71d481cf7b8f8457f479745ee331734649e9e2c2ab00ce5d317112796c77afc328612ed004e65ac5c16fc92279d760cfb012cfddce9098c4af810f
fa43897c1d doc: Fix LLM nits in net_processing.cpp (MarcoFalke)
bbbba0fd4b scripted-diff: Use references when nullptr is not possible (MarcoFalke)
fac5415466 refactor: Separate peer/maybe_peer in ProcessMessages and SendMessages (MarcoFalke)
fac529188e refactor: Pass Peer& to ProcessMessage (MarcoFalke)
fa376095a0 refactor: Pass CNode& to ProcessMessages and SendMessages (MarcoFalke)
fada838014 refactor: Make ProcessMessage private again (MarcoFalke)
fa80cd3cee test: [refactor] Avoid calling private ProcessMessage() function (MarcoFalke)
Pull request description:
There is a single unit test, which calls the internal `ProcessMessage` function. This is problematic, because it makes future changes harder, since they will need to carry over this public internal interface each time.
Also, there is a mixed use of pointers and references in p2p code, where just based on context, a pointer may sometimes assumed to be null, or non-null. This is confusing when reading the code, or making or reading future changes.
Fix both issues in a series of commits, to:
* refactor the single unit test to call higher-level functions
* Make `ProcessMessage` private again
* Use references instead of implicit non-null pointers, mostly in a scripted-diff
ACKs for top commit:
optout21:
reACK fa43897c1d
ajtowns:
ACK fa43897c1d
Crypt-iQ:
crACK fa43897c1d
achow101:
ACK fa43897c1d
Tree-SHA512: d03d8ea35490a995f121be3d2f3e4a22d1aadfeab30bc42c4f8383dab0e6e27046260e792d9e5a94faa6777490ba036e39c71c50611a38f70b90e3a01f002c9e
48161f6a05 wallet: introduce "tx amount exceeds balance when fees are included" error (stratospher)
b7fa609ed1 wallet: remove PreSelectedInputs (stratospher)
7819da2c16 walllet: use CoinsResult instead of PreSelectedInputs (stratospher)
e5474079f1 wallet: introduce GetAppropriateTotal() in CoinsResult (stratospher)
d8ea921d01 wallet: correctly reserve in CoinsResult::All() (stratospher)
7072d825e3 wallet: ensure COutput added in set are unique (stratospher)
fefa3be782 wallet: fix, make 'total_effective_amount' optional actually optional (stratospher)
Pull request description:
picks up https://github.com/bitcoin/bitcoin/pull/25269.
This PR re-implements the code path so that an error message is thrown when a transaction's total amount (including fees) exceeds the available balance. It also refactors the wallet's coin selection code.
1. the first 3 commits are unrelated to the code but few small bug fixes which are nice to fix. but also kind of impacts the remaining logic. (could PR separately if reviewers wish)
1. c467325aaf187d7f056bb1ea1cec6b7c4250af2e: make `total_effective_amount` optional actually optional
2. 2202ab597596c84fc49f8784e823372b7a9efcbe: ensure `set<shared_ptr<COutput>>` has unique COutput
3. a5ffbbf122d66fc4ad9b2e7c6d7d1dfa1816388e: Correctly reserve size when flattening `CoinsResult.coins` map to vector
3. the next 3 commits from 4745d5480ca5c3809edd51140e4d2c0433582844 replace the `PreSelectedInputs` struct with `CoinsResult` and removes `PreSelectedInputs`.
4. the last commit (e664484a6d34c1795ebb0925ab31faea5d64ab00) deals with the error message - `AmountWithFeeExceedsBalance` error inside `WalletModel::prepareTransaction` is never thrown and remains an unused code path. This is because `createTransaction` does not retrieve the fee when the process fails. The fee return arg is set only at the end of the process, when the transaction is successfully created. Therefore, if the transaction creation fails, the fee is not available inside `WalletModel::prepareTransaction` to trigger the `AmountWithFeeExceedsBalance` error.
This PR re-implements the feature inside `CreateTransactionInternal` and adds test coverage for it.
| on master | on PR |
|-----------|-------|
| <img src="https://github.com/user-attachments/assets/a903e687-2466-42c7-b898-5dec24bfe515" width="750" alt="Insufficient funds" /> | <img src="https://github.com/user-attachments/assets/74bb3c83-6132-4c09-91f0-0a446618b3c8" width="750" alt="AmountWithFeeExceedsBalance" /> |
the unreachable code path is removed in https://github.com/bitcoin-core/gui/pull/807 which requires this PR.
ACKs for top commit:
achow101:
ACK 48161f6a05
furszy:
utACK 48161f6
Tree-SHA512: a963fac8d6714f76571df8cf9aff70601536dc6faa4326fbb5892c3f080dc393f0d7c6e2d21879c7a2c898bf0092adb154376d9b0a8929b31575ce9d1d47dec2
In feature_assumevalid.py, we check that a modified block 102 is invalid
by asserting a message in the debug.log. However, this can
intermittently fail as exiting the assert_debug_log can occur before the
block has actually been validated, thus causing the test to fail as the
validation error message is not present in the chunk of the debug.log
being examined.
We can wait for the block to make an invalid chain tip to ensure that the log
line will be present.
e67a676df9 fix: uptime RPC returns 0 on first call (Lőrinc)
Pull request description:
### Problem
#34328 switched uptime to use monotonic time, but `g_startup_time` was a function-local static in `GetUptime()`, meaning it was initialized on first call rather than at program start.
This caused the first uptime RPC to always return 0.
### Fix
Move `g_startup_time` to namespace scope so it initializes at program start, ensuring the first `uptime()` call returns actual elapsed time.
### Reproducer
Revert the fix and run the test or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
./build/bin/bitcoind -regtest -daemon
sleep 10
./build/bin/bitcoin-cli -regtest uptime
./build/bin/bitcoin-cli -regtest stop
```
<details>
<summary>Before (uptime is initialized on first call)</summary>
```bash
Bitcoin Core starting
0
Bitcoin Core stopping
```
</details>
<details>
<summary>After (first uptime call is in-line with sleep)</summary>
```bash
Bitcoin Core starting
10
Bitcoin Core stopping
```
</details>
----
Fixes#34423, added reporter as coauthor.
ACKs for top commit:
maflcko:
lgtm ACK e67a676df9
optout21:
crACK e67a676df9
carloantinarella:
Tested ACK e67a676df9
achow101:
ACK e67a676df9
musaHaruna:
Tested ACK [e67a676](https://github.com/bitcoin/bitcoin/pull/34437/changes/e67a676df9af5ece5307438ae1b4ddb0730e3482)
Tree-SHA512: b156f7ed15c3fbb50e8a15f6c9a0d4e2ffb956d0c6ef949e0f8a661a564a20c0d3ed2149fae75ce7e2baa9326788d5037e726e7a7ac2c6ef4e70e4862cd5763f
54d0393058 FUZZ: Test that BnB finds best solution (Murch)
Pull request description:
BnB’s solution is the input set with the lowest waste score, excluding any supersets of other solution candidates.
This fuzz test compares a brute force search with the BnB result to ensure that BnB succeeds.
ACKs for top commit:
achow101:
ACK 54d0393058
brunoerg:
ACK 54d0393058
Tree-SHA512: 96b6a822f53311d9a76abe8c217794e0a2dd5bd713db0a15dc70e065099b8245c430e1174e24133e0a802218ff0f2943dfcc3d638c3716485d5607c452854e7d
f2b8acc0ed remove glozow from trusted keys (glozow)
Pull request description:
As planned!
Also bump trusted-git-root to 88a7294356, which is the last merge signed with this key.
ACKs for top commit:
achow101:
ACK f2b8acc0ed
stickies-v:
ACK f2b8acc0ed
Tree-SHA512: e68025c519ab003ad464783325502c1a6a3f560c0708a9383332cb0971be2039f462fc95f85373bb075191c165ca8e3904312a66e41b52d1b99dd2a8a837be9a
db2effaca4 scripted-diff: refactor: CWallet::Create() -> CreateNew() (David Gumberg)
27e021ebc0 wallet: Correctly log stats for encrypted messages. (David Gumberg)
d8bec61be2 wallet: remove loading logic from CWallet::Create (David Gumberg)
f35acc893f refactor: wallet: Factor out `WriteVersion()` from `PopulateWalletFromDB()` (David Gumberg)
e12ff8aca0 test: wallet: Split create and load (David Gumberg)
70dbc79b09 wallet: Use CWallet::LoadExisting() for loading existing wallets. (David Gumberg)
ae66e01164 wallet: Create separate function for wallet load (David Gumberg)
bc69070416 refactor: Wallet stats logging in its own function (David Gumberg)
a9d64cd49c wallet: Remove redundant birth time update (David Gumberg)
b4a49cc727 wallet: Move argument parsing to before DB load (David Gumberg)
b15a94a618 refactor: Split out wallet argument loading (David Gumberg)
a02c4a82d8 refactor: Move -walletbroadcast setting init (David Gumberg)
411caf7281 wallet: refactor: PopulateWalletFromDB use switch statement. (David Gumberg)
a48e23f566 refactor: wallet: move error handling to PopulateWalletFromDB() (David Gumberg)
0972785fd7 wallet: Delete unnecessary PopulateWalletFromDB() calls (David Gumberg)
f0a046094e scripted-diff: refactor: CWallet::LoadWallet->PopulateWalletFromDB (David Gumberg)
Pull request description:
This PR is mostly a refactor which splits out logic used for creating wallets and for loading wallets, both of which are presently contained in `CWallet::Create()` into `CWallet::CreateNew()` and `CWallet::LoadExisting()`
The real win of this PR is that `CWallet::Create()` uses a very bad heuristic for trying to guess whether or not it is supposed to be creating a new wallet or loading an existing wallet:
https://github.com/bitcoin/bitcoin/blob/370c59261269fd9043674e0f4fd782a89e724473/src/wallet/wallet.cpp#L2882-L2885
This heuristic assumes that wallets with no `ScriptPubKeyMans` are being created, which sounds reasonable, but as demonstrated in #32112 and #32111, this can happen when the user tries to load a wallet file that is corrupted, both issues are fixed by this PR and any other misbehavior for wallet files which succeeded the broken heuristic's sniff test for new wallets.
It was already the case that every caller of `CWallet::Create()` knows whether it is creating a wallet or loading one, so we can avoid replacing this bad heuristic with another one, and just shift the burden to the caller.
ACKs for top commit:
achow101:
ACK db2effaca4
polespinasa:
approach ACK db2effaca4
w0xlt:
reACK db2effaca4
murchandamus:
ACK db2effaca4
rkrux:
ACK db2effaca4
Tree-SHA512: c28d60e0a3001058da3fd2bdbe0726c7ebe742a4b900a1dee2e5132eccc22e49619cb747a99b4032b000eafd4aa2fdd4ec244c32be2012aba809fdc94b5f6ecd
02b5f6078d fees: make flushes log debug only (ismaelsadeeq)
Pull request description:
This is a simple PR that updates the flushing log to use debug-level logging under the estimatefee category. It also ensures the log consistently includes only the full file path.
The motivation behind this is that the "Flushed fee estimates to fee_estimates.dat." logs can become noisy; it's done after one hour, so hiding it in the debug estimatefee category seems reasonable.
---
I left the logs when the file is not found as info because that should only occur when you start a fresh node, change datadir, or explicitly delete the file
ACKs for top commit:
achow101:
ACK 02b5f6078d
furszy:
utACK 02b5f6078d
l0rinc:
Lightly tested ACK 02b5f6078d
sipa:
utACK 02b5f6078d
Tree-SHA512: 3e4b822caa23db9b30f1bb8990b36f35dcfcd82dbfb27c319463447da1df988eded84c47d5319ad637c822bdd04f0a727a176da3632c40d786332b6a5aaa6d89
d511adb664 [miner] omit dummy extraNonce via IPC (Sjors Provoost)
bf3b5d6d06 test: clarify getCoinbaseRawTx() comparison (Sjors Provoost)
78df9003d6 [doc] Update comments on dummy extraNonces in tests (Anthony Towns)
Pull request description:
This PR changes the Mining IPC interface to stop including a dummy `extraNonce` in the coinbase `scriptSig` by default, exposing only the consensus-required BIP34 height. This simplifies downstream mining software (including Stratum v2), avoids forcing clients to strip or ignore data we generate, and reduces the risk of incompatibilities if future soft forks add required commitments to the `scriptSig`.
Existing behavior is preserved for RPCs, tests, regtest, and internal mining by explicitly opting in to the dummy `extraNonce` where needed (e.g. to satisfy `bad-cb-length` at low heights), so consensus rules and test coverage are unchanged. The remainder of the PR consists of small comment fixes, naming clarifications, and test cleanups to make the intent and behavior clearer.
ACKs for top commit:
achow101:
ACK d511adb664
ryanofsky:
Code review ACK d511adb664. Just rebased since last review and make suggested tweaks. I'd really like to see this PR merged for the cleanups and sanity it brings to this code. Needs another reviewer though.
sedited:
ACK d511adb664
Tree-SHA512: d41fa813eb6b5626f4f475d8abc506b29090f4a2d218f2d6824db58b5ebe2ed7c584a903b44de18ccec142bb79c257b0aba6d6da073f56175aec88df96aaaaba
Since pk(), pk_k(), pkh(), pk_h(), sha256(), ripemd160(), hash256(),
hash160(), after(), and older() all are single argument expressions that
are parsed immediately, we can use the Expr and Func parsing functions
to determine what the arguments of these expressions are, rather than
searching for the next closing parentheses.
This fixes an issue when pk(), pk_k(), pkh(), and pk_h() include a
musig() expression as Expr properly handles nested expressions.
b189a34557 test: add case where `TOTAL_TRIES` is exceeded yet solution remains (yancy)
Pull request description:
Show that `CoinGrider` halts searching when the number of attempts exceeds `TOTAL_TRIES`. To do so, show that a solution is found, then add one more entry to the same set of inputs. Since the search orders by `effective_value`, the solution is constructed such that only values with the lowest `effective_value` have the least weight. Only the lowest weight values will not exceed the `max_selection_weight`. Therefore, `CoinGrinder` will not evaluate all lowest weight solutions together before exceeding `TOTAL_TRIES` since they are last found.
This test case was inspired by a similar test for `BnB` currently named `bnb_test`.
ACKs for top commit:
frankomosh:
Code review ACK b189a34
achow101:
ACK b189a34557
murchandamus:
ACK b189a34557
Tree-SHA512: 1df0b6e29ae219edbeed14cfa97f0ad4688d6bf97ed946719ba3c3b69e004f3dee82991578eb5aceb554914b70c5b68feff9e321283c1fc8bc0fedf08df2cb4c
552bc82b17 doc: Use multipath descriptors in descriptors.md and linked test (Anurag chavan)
Pull request description:
Updates documentation and `wallet_miniscript_decaying_multisig_descriptor_psbt.py` to use single multipath descriptors with `<0;1>` syntax instead of separate external/internal descriptors.
## Changes
- **doc/descriptors.md**: Update examples (lines 70-71) to use `/<0;1>/*` multipath syntax
- **doc/descriptors.md**: Update Basic Multisig Example instructions (line 179) to use single multipath descriptor
- **test/functional/wallet_miniscript_decaying_multisig_descriptor_psbt.py**: Refactor to use single multipath descriptor pattern matching `wallet_multisig_descriptor_psbt.py`
## Implementation
- `_get_xpub()` now extracts external descriptor and converts to multipath format
- `create_multisig()` imports single descriptor that expands to receive and change addresses
- Removed fake checksums from documentation examples
- Added clear comments documenting multipath convention
Fixes#34086
ACKs for top commit:
yashbhutwala:
ACK 552bc82b17
achow101:
ACK 552bc82b17
rkrux:
lgtm tACK 552bc82
Tree-SHA512: cc99271a3955daa475242d9f4ef8f09f4c94c64e48ec4647ecfd95dceb38bb0cdd91b78ec2d5f033b449d175eaecbdda49d6c766c8a1e1a01fed93be4eb0cfc0
6f7b4323cb test: remove UNKNOWN_ERROR from script_tests (Bruno Garcia)
bd31a92d67 script: use SCRIPT_ERR_SCRIPTNUM for CScriptNum errors (Bruno Garcia)
0ca4dcd786 script: add SCRIPT_ERR_SCRIPTNUM error (Bruno Garcia)
Pull request description:
When evaluating a script, the current code is bad for analyzing some errors because it returns `SCRIPT_ERR_UNKNOWN_ERROR` for errors that are clearly known.
`CScriptNum` has two well defined errors: number overflow and non-minimally encoded number. However, for both errors we return as unknown. This PR changes it by adding a new ScriptError that is used for any `CScriptNum` error.
ACKs for top commit:
achow101:
ACK 6f7b4323cb
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/34381/commits/6f7b4323cb46687ba9df7073a9cde427985d2dfc
darosior:
ACK 6f7b4323cb
Tree-SHA512: e656d9992251fbc95d33966fa18ce64bf714179d51ba6a7f429e5a55bc58e7fc08827e4ab71ace0dd385dac7e1feaea621b49503387793a30eae7a7e44aa6b0f
964c44cdcd test(miniscript): Prove avoidance of stack overflow (Hodlinator)
198bbaee49 refactor(miniscript): Destroy nodes one full subs-vector at a time (Hodlinator)
50cab8570e refactor(miniscript): Remove NodeRef & MakeNodeRef() (Hodlinator)
15fb34de41 refactor(miniscript): Remove superfluous unique_ptr-indirection (Hodlinator)
e55b23c170 refactor(miniscript): Remove Node::subs mutability (Hodlinator)
c6f798b222 refactor(miniscript): Make fields non-const & private (Hodlinator)
22e4115312 doc(miniscript): Remove mention of shared pointers (Hodlinator)
Pull request description:
Removes one level of unnecessary indirection, which was a change that originally [aided in finding one issue](https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2434704657) in #30866. Simplifies the code one step further than 09a1875ad8 belonging to aforementioned PR.
Also adds test which verifies resistance to stack overflow when it comes to `~Node()` and `Node::Clone()`.
No observed difference when running benchmarks: ExpandDescriptor/WalletIsMineDescriptors/WalletIsMineMigratedDescriptors/WalletLoadingDescriptors.
Followup to #30866.
ACKs for top commit:
achow101:
ACK 964c44cdcd
darosior:
Code review ACK 964c44cdcd
l0rinc:
ACK 964c44cdcd
Tree-SHA512: 32927e8f0f916fb70372ffd110f7ec7207d9e7a099c21c0a7482a12e96593b673c339719f4ab166ad7c086dc43767315fc1742c5b236a3facc45c4cfeb5872e9
516be10bb5 wallet: Rename `RecordType::DELETE` to `RecordType::DELETE_FLAG` (Hennadii Stepanov)
Pull request description:
On Windows, the `winnt.h` header defines `DELETE` as a macro for a "Standard Access Right" bitmask (0x00010000L).
This introduces a fragile dependency on header inclusion order: if Windows headers happen to be included before the `RecordType` enum definition, the preprocessor expands `DELETE` into a numeric literal, causing syntax errors.
Rename the enumerator to `DELETE_FLAG` to remove this fragility and avoid the collision entirely.
Split from https://github.com/bitcoin/bitcoin/pull/34448.
ACKs for top commit:
maflcko:
re-lgtm ACK 516be10bb5
achow101:
ACK 516be10bb5
Tree-SHA512: eba054b395e18c07efb2901b28f542b042b62d85e1a798eeff35f8431530cb667fa791c47c4125cecdb689213b458ba396715495415e9b83bb322509a9376222
7d9e1a8102 test: Verify peer usage after assumeutxo validation completes (stringintech)
0067abe153 p2p: Allow block downloads from peers without snapshot block after assumeutxo validation (stringintech)
Pull request description:
Currently, after assumeutxo background validation finishes, the node continues to skip peers that don't have the snapshot block in their best chain until restart. This unnecessarily excludes peers from block downloads even though the background sync has completed and undo data is available.
The restriction persists because `m_chainman.CurrentChainstate().SnapshotBase()` continues to return the snapshot base block until restart, even after validation completes. Added `m_chainman.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED` check to only apply the peer restriction while background validation is ongoing.
Also added test coverage in `feature_assumeutxo.py` that verifies peers without the snapshot block can be used for block downloads after background validation completes. The test fails without this fix.
ACKs for top commit:
fjahr:
Re-ACK 7d9e1a8102
achow101:
ACK 7d9e1a8102
sedited:
Re-ACK 7d9e1a8102
Tree-SHA512: 5515971da7bf7efc55eecdf03686f44c20c9e52dd168e7cfa119032d6a8ebccee69df7143075e4e9d0a01426cd9ae7202dce5c00919a82478ebf49a15dc0fe19
3e0fd0e4dd refactor: rename will_reuse_cache to reallocate_cache (Andrew Toth)
44b4ee194d validation: reuse same CCoinsViewCache for every ConnectBlock call (Andrew Toth)
8fb6043231 coins: introduce CCoinsViewCache::ResetGuard (Andrew Toth)
041758f5ed coins: use hashBlock setter internally for CCoinsViewCache methods (Andrew Toth)
8dd9200fc9 coins: add Reset on CCoinsViewCache (Andrew Toth)
Pull request description:
This is the first commit of #31132, which can be merged as an independent change. It has a small benefit on its own, but will help in moving the parent PR forward.
Add a `Reset()` method to `CCoinsViewCache` that clears `cacheCoins`, `cachedCoinsUsage`, and `hashBlock` without flushing to the `base` view. This allows efficiently reusing a cache instance across multiple blocks.
Add `CCoinsViewCache::CreateResetGuard` method to return a `CCoinsViewCache::ResetGuard`. The `ResetGuard` automatically calls `Reset()` on destruction. This RAII pattern ensures the cache is always properly reset between blocks.
Add `m_connect_block_view` as a persistent `CCoinsViewCache` for `ConnectBlock`, avoiding repeated memory allocations.
ACKs for top commit:
l0rinc:
ACK 3e0fd0e4dd
achow101:
ACK 3e0fd0e4dd
sedited:
ACK 3e0fd0e4dd
Tree-SHA512: a95feaa062a9eb7cf7514425a7e7adffd347cd1f7b32b4c1fefcde30002141757c184174702b3104a029dcd33194f8bd734159deebb2e668716089305b42cb00
c6ca2b85a3 validation: do not wipe utxo cache for stats/scans/snapshots (Pieter Wuille)
7099e93d0a refactor: rename `FlushStateMode::ALWAYS` to `FORCE_FLUSH` (Lőrinc)
Pull request description:
Revival of https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-3432564955 with the remaining comments applied on top
> Since #28280, the cost of a non-wiping sync of the UTXO cache is only proportional to the number of dirty entries, rather than proportional to the size of the entire cache. Because of that, there is no reason to perform a wiping flush in case the contents of the cache is still useful.
>
> Split the `FlushStateMode::ALWAYS` mode into a FORCE_SYNC (non-wiping) and a FORCE_FLUSH (wiping), and then use the former in `scantxoutset`, `gettxoutsetinfo`, snapshot creation.
(slightly updated after #30214)
ACKs for top commit:
optout21:
reACK c6ca2b85a3
cedwies:
reACK c6ca2b8 (trivial)
achow101:
ACK c6ca2b85a3
sedited:
ACK c6ca2b85a3
Tree-SHA512: f3525a85dc512db4a0a9c749ad47c0d3fa44085a121aa54cd77646260a719c71f754ec6570ae77779c0ed68a24799116f79c686e7a17ce57a26f6a598f7bf926
e770392084 test: addrman: test self-announcement time penalty handling (Bruno Garcia)
Pull request description:
This PR adds a test case for addrman that verifies that addresses announcing themselves (addr == source) are exempt from time penalties, while addresses announced by others receive the expected penalty.
It fixes the following mutant (https://corecheck.dev/mutation/src/addrman.cpp#L561):
```diff
diff --git a/src/addrman.cpp b/src/addrman.cpp
index 206b54118e..c6a045fd8d 100644
--- a/src/addrman.cpp
+++ b/src/addrman.cpp
@@ -558,7 +558,7 @@ bool AddrManImpl::AddSingle(const CAddress& addr, const CNetAddr& source, std::c
AddrInfo* pinfo = Find(addr, &nId);
// Do not set a penalty for a source's self-announcement
- if (addr == source) {
+ if (addr != source) {
time_penalty = 0s;
}
```
ACKs for top commit:
maflcko:
review ACK e770392084🐤
achow101:
ACK e770392084
fjahr:
Code review ACK e770392084
naiyoma:
tACK e770392084
Tree-SHA512: ec029d1e1e979f91840af944984cad530a1ce9a0eceb123230817f0ef3b9ad47253eebc4c953d350de2d904b59496fcd4757123c8bd63cf0e09c3581da48fff8
2ee7f9b259 coins: assume `GetCoin` only returns unspent coins (Andrew Toth)
eec551aaf1 fuzz: keep `coinscache_sim` backend free of spent coins (Andrew Toth)
3e4155fcef test: do not return spent coins from `CCoinsViewTest::GetCoin` (Andrew Toth)
ee1e40f580 txdb: assert `CCoinsViewDB::GetCoin` only returns unspent coins (Lőrinc)
Pull request description:
This PR is split out from #33018 to keep that PR focused on removing the `FRESH-but-not-DIRTY` cache state.
### Problem
`::GetCoin()` is an interface for querying the UTXO set, so production implementations should only ever return unspent coins. Tests should mimic this to provide useful feedback.
### Fix:
* Add a fail-fast assertion that `CCoinsViewDB::GetCoin()` never returns a spent coin.
* Align unit tests and fuzz simulations with the production `GetCoin()` contract by never returning spent coins.
* Replace the unreachable “spent coin returned by parent” handling in `CCoinsViewCache::FetchCoin()` with `Assert(!coin.IsSpent())`, drop outdated `spent+FRESH` docs, and tighten `SanityCheck()` invariants.
Behavior is unchanged, it just aligns our tests to exercise valid states.
ACKs for top commit:
andrewtoth:
re-ACK 2ee7f9b259
optout21:
crACK 2ee7f9b259
achow101:
ACK 2ee7f9b259
w0xlt:
reACK 2ee7f9b259
Tree-SHA512: be21cc09690410fc04ca25e1ba47aae6186bc037e413b3bb1e6e9a04e6364cbfac5a2fcdc49b638fec848cd29243fab0cc0581b9923f34fafe8366828f690ed4
1f60ca360e wallet: fix removeprunedfunds bug with conflicting transactions (Martin Zumsande)
Pull request description:
`removeprunedfunds` removes all entries from `mapTxSpends` for the inputs of the pruned tx. However, this is incorrect, because there could be multiple entries from conflicting transactions (that shouldn't be removed as well). This could lead to the wallet creating invalid transactions, trying to double spend utxos.
The bug persists when the conflicting tx was mined, because the wallet trusts its internal accounting instead of calling `AddToSpends` again.
The added test should fail on master.
ACKs for top commit:
achow101:
ACK 1f60ca360e
fjahr:
tACK 1f60ca360e
furszy:
utACK 1f60ca360e
vasild:
ACK 1f60ca360e
Tree-SHA512: 3cc9ed547530fd53e25721177b76ab2e1eae16ce2c0e63fc01b20fdbf8bd02655dae51167ad56f9dec748d34c61ce65d38f993370820601f8257c73b876a3347
8b9d30e3fa bench/test: clarify merkle bench and witness test intent (Lőrinc)
Pull request description:
Follow-up to #32497.
Clarify why the witness merkle test uses an odd leaf count (it exercises leaf duplication in `ComputeMerkleRoot()`), and make the coinbase witness hash initialization explicit.
Also simplify the leaf-copy loop in the `MerkleRoot` benchmark for readability.
No production code is changed in this follow-up, for simplicity and safety.
ACKs for top commit:
optout21:
ACK 8b9d30e3fa
maflcko:
lgtm ACK 8b9d30e3fa
achow101:
ACK 8b9d30e3fa
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/34376/commits/8b9d30e3facff0cd132dc3faf6282d75b1f9b532
danielabrozzoni:
tACK 8b9d30e3fa
Tree-SHA512: 6efca7c19ebf96bb8d0def4217ed30d3b74b58a7be15566967e98aba9b03aaddd0e0ebb3b8f43130b5f397a7d9eed0470a48a55438f440e0bceefb87edd16b27
3dd815f048 validation: pre-reserve leaves to prevent reallocs with odd vtx count (Lőrinc)
7fd47e0e56 bench: make `MerkleRoot` benchmark more representative (Lőrinc)
f0a2183108 test: adjust `ComputeMerkleRoot` tests (Lőrinc)
Pull request description:
#### Summary
`ComputeMerkleRoot` [duplicates the last hash](https://github.com/bitcoin/bitcoin/blob/39b6c139bd6be33699af781f1d71f6fed303d468/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 3dd815f048
optout21:
reACK 3dd815f048
hodlinator:
re-ACK 3dd815f048
vasild:
ACK 3dd815f048
w0xlt:
ACK 3dd815f048 with minor nits.
danielabrozzoni:
Code review ACK 3dd815f048
Tree-SHA512: e7b578f9deadc0de7d61c062c7f65c5e1d347548ead4a4bb74b056396ad7df3f1c564327edc219670e6e2b2cb51f4e1ccfd4f58dd414aeadf2008d427065c11f
969c840db5 log,blocks: avoid `ComputeTotalSize` and `GetHash` work when logging is disabled (Lőrinc)
babfda332b log,net: avoid `ComputeTotalSize` when logging is disabled (Lőrinc)
1658b8f82b 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 https://github.com/bitcoin/bitcoin/commit/969c840db52da796c319f84c9a9a20b1de902ccf
achow101:
ACK 969c840db5
hodlinator:
re-ACK 969c840db5
sedited:
Re-ACK 969c840db5
danielabrozzoni:
reACK 969c840db5
Tree-SHA512: 9780102d29778165144e3602d934ed4cb96660fd7b9ff2581b223c619e419139b8348e60f226af448702ae527736a1806d169b44342c5a82795590f664e16efe
a3c71c7201 [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 a3c71c7201
rkrux:
lgtm ACK a3c71c7
Tree-SHA512: fc13beb5445c292cd7c75a47810fb1c4032ee2e3c1800dc44089b95959ccce8330291084bf788457e1d55c02d706ef04be7044badfee134149e004c44b19ec32
9c7e4771b1 test: Test listdescs with priv works even with missing priv keys (Novo)
ed945a6854 walletrpc: reject listdes with priv key on w-only wallets (Novo)
9e5e9824f1 descriptor: ToPrivateString() pass if at least 1 priv key exists (Novo)
5c4db25b61 descriptor: refactor ToPrivateString for providers (Novo)
2dc74e3f4e wallet/migration: use HavePrivateKeys in place of ToPrivateString (Novo)
e842eb90bb 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 9c7e4771b1
achow101:
ACK 9c7e4771b1
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/32471/commits/9c7e4771b13d4729fd20ea08b7e2e3209b134fff with minor nits
rkrux:
re-ACK 9c7e4771b1
Tree-SHA512: f9b3b2c3e5425a26e158882e39e82e15b7cb13ffbfb6a5fa2868c79526e9b178fcc3cd88d3e2e286f64819d041f687353780bbcf5a355c63a136fb8179698b60
a7b581423e Fix 11-year-old mis-categorized error code in OP_IF evaluation (Calin Culianu)
Pull request description:
This was introduced by commit ab9edbd6b6.
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 a7b581423e
achow101:
ACK a7b581423e
darosior:
utACK a7b581423e
sedited:
ACK a7b581423e
Tree-SHA512: e8c01a3e2448b5d49b76a0cab3f38a2d0249b71beeb7d9d05d5ecc3812bd91d0bd1d0f78b809b6f4ccb73186fa119cb1ed3779a73284b83a67ae219ef378fa6c
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.
fa3df52712 bench: Require semicolon after BENCHMARK(foo) (MarcoFalke)
fa8938f08c bench: Remove incorrect __LINE__ in BENCHMARK macro (MarcoFalke)
fa51a28a94 scripted-diff: Remove priority_level from BENCHMARK macro (MarcoFalke)
fa790c3eea 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 27f11217ca 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 0ad4376a49, 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 fa3df52712
l0rinc:
ACK fa3df52712
hebasto:
re-ACK fa3df52712, only suggested changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/34210#pullrequestreview-3652414135).
Tree-SHA512: 68a314bff551fa878196d5a615d41d71e1c8c504135e6fc555659aa9f0c8786957d49ba038448e933554a8bc54caea2ddd7d628042c5627bf3bf37628210f8fb
ce63d37ebe 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 ce63d37ebe
vasild:
ACK ce63d37ebe
mzumsande:
re-ACK ce63d37ebe
Tree-SHA512: 4efcedca3bde209fbd1bdc2a4ae04b7d53515587d86e421ce61064f78c675c71b45d9782b514c5e7cfc0e92842c947d49f7a3fddb03fe619fcdec9b565f0ecbd
7b5d256af4 test: Add bitcoin-chainstate test for assumeutxo functionality (stringintech)
2bc3265649 Fix `ChainstateManager::AddChainstate()` assertion crash (stringintech)
5f3d6bdb66 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 7b5d256af4
theStack:
Concept and code-review ACK 7b5d256af4
sedited:
Re-ACK 7b5d256af4
Tree-SHA512: 5d3b0050cf2d53144b5f65451c991d5e212117b4541ae1368ecf58fde5f3cca4f018aad6ae32257b9ebb1c28b926424fbcff496ba5487cdc4eb456cea6db8b24
792e2edf57 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 792e2edf57
achow101:
ACK 792e2edf57
fjahr:
Code review ACK 792e2edf57
frankomosh:
Code Review ACK [792e2ed](https://github.com/bitcoin/bitcoin/pull/34146/commits/792e2edf57ab31ae5c6f98acf33af8f67506630f)
Tree-SHA512: e3d39b1e3ae6208b54df4b36c624a32d70a442e01681f49e0c8a65076a818b5bf203c2e51011dc32edbbe3637b3c0b5f18de26e3461c288aa3806646a209a260
8fb5e5f41d test: check wallet rescan properly in feature_pruning (brunoerg)
9b57c8d2bd 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 8fb5e5f41d
furszy:
utACK 8fb5e5f41d
musaHaruna:
Tested ACK [8fb5e5f](https://github.com/bitcoin/bitcoin/commit/8fb5e5f41ddf550a78b1253184d79a107097815a)
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/34185/commits/8fb5e5f41ddf550a78b1253184d79a107097815a
Tree-SHA512: 812afbf4343a7493e2169eb6735fce25692d5cb19972abafc772b8c05a64b9c7027f6675cd084f345977e916e62a722d671f90831bbdc51683e0cd253fa482f0
de4242f474 refactor: Use reference for chain_start in HeadersSyncState (Daniela Brozzoni)
e37555e540 refactor: Use initializer list in CompressedHeader (Daniela Brozzoni)
0488bdfefe refactor: Remove unused parameter in ReportHeadersPresync (Daniela Brozzoni)
256246a9fa refactor: Remove redundant parameter from CheckHeadersPoW (Daniela Brozzoni)
ca0243e3a6 refactor: Remove useless CBlock::GetBlockHeader (Pieter Wuille)
4568652222 refactor: Use std::span in HasValidProofOfWork (Daniela Brozzoni)
4066bfe561 refactor: Compute work from headers without CBlockIndex (Daniela Brozzoni)
0bf6139e19 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 de4242f474
polespinasa:
re-ACK de4242f474
sipa:
ACK de4242f474
achow101:
ACK de4242f474
hodlinator:
re-ACK de4242f474
Tree-SHA512: 1de4f3ce0854a196712505f2b52ccb985856f5133769552bf37375225ea8664a3a7a6a9578c4fd461e935cd94a7cbbb08f15751a1da7651f8962c866146d9d4b
2f5b1c5f80 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 2f5b1c5f80
maflcko:
lgtm ACK 2f5b1c5f80
achow101:
ACK 2f5b1c5f80
Tree-SHA512: cec613a9a38358d5caa243197d746baa129aebfd7fe697689f28e652f94c4683873c4676d5eb2eb909ea19de5e5f6e54ecc5f3162384a48f6f38a59273667689
c5825d4b7f qa: Require `--exclude` for each excluded test (Hennadii Stepanov)
Pull request description:
This PR allows a long `--exclude ...` argument in the `test/functional/test_runner.py` invocation to be split across multiple lines, with optional per-line explanatory comments. I found this useful for the CI scripts in https://github.com/hebasto/bitcoin-core-nightly.
ACKs for top commit:
l0rinc:
tested ACK c5825d4b7f
maflcko:
review ACK c5825d4b7f🛄
achow101:
ACK c5825d4b7f
rkrux:
ACK c5825d4b7f
Tree-SHA512: bcf42848516197978b65df8a8bc68e036a62c9afc6158274eac74a325dc01991eb063a042f940c53ea15a7feb18d4bdfc45d8c71f0ef20c76140b12e07ba3ac5
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.
fafbc70d48 rpc: [wallet] Use unsigned type for tx version in sendall (MarcoFalke)
Pull request description:
It is confusing to parse the unsigned tx version as a signed type. Also, it makes it harder to use the integer sanitizer.
Can be tested via:
* Build with the flags `-DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DSANITIZERS=undefined,integer,float-divide-by-zero`
* Set the existing suppressions: `export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=0:report_error_type=1"`
* Start the RPC server, e.g. `./bld-cmake/bin/bitcoin-qt -datadir=/tmp -regtest -server`
* Call the sendall RPC, e.g. `./bld-cmake/bin/bitcoin-cli -datadir=/tmp -regtest -named sendall '["bcrt1qlrt3xps4wxpfcjmljrayr2ualczmnfvd4vzdq3"]' fee_rate=1.234 version=-1`
Before:
```
src/wallet/rpc/spend.cpp:1470:42: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4294967295 (32-bit, unsigned)
Invalid parameter, version out of range(1~3)
```
After:
```
JSON integer out of range
ACKs for top commit:
bensig:
ACK fafbc70d48
achow101:
ACK fafbc70d48
rkrux:
utACK fafbc70d48
theStack:
ACK fafbc70d48
Tree-SHA512: bb7cf54e9691ad2591646b138ffdfac95bf77c5234d489f4e4f2c60b41bdc14cdc18a030fecb0a6ac64e55e4c69b37835afd334f87d8a44b8df6cda053e8fefb
fa65bc0e79 test: Run bench sanity checks in parallel with functional tests (MarcoFalke)
fa9fdbce79 test: Pass bench exe into test framework utils (MarcoFalke)
Pull request description:
The ctest target `bench_sanity_check` has many issues:
* With sanitizers enabled, it is one of the slowest targets, often taking several minutes. See https://github.com/bitcoin/bitcoin/issues/32770#issuecomment-2984264066.
* There is no insight from ctest into how long each individual sanity check takes.
* On a timeout, or OOM issue, there is no insight into which sub-bench failed. The failure will generally just look like `75/153 Test #9: bench_sanity_check ...................***Failed 770.84 sec out of memory`
* Places that can't use ctest (like the Windows-cross CI task) have to explicitly run it, or risk forgetting to run it.
* All benchmarks are run sequentially, when they could run in parallel instead.
Both issues can lead to CI timeouts and leave CPU unused during testing.
Fix all issues by running it as part of the functional tests instead. This is similar to the rpcauth tests (https://github.com/bitcoin/bitcoin/pull/32881) and util tests [bitcoin-tx, and bitcoin-util] (https://github.com/bitcoin/bitcoin/pull/32697).
ACKs for top commit:
achow101:
ACK fa65bc0e79
l0rinc:
Tested ACK fa65bc0e79
janb84:
tACK fa65bc0e79
willcl-ark:
ACK fa65bc0e79
Tree-SHA512: d27e363b7896a7543a4ee8df41a56e58b74f07d4f296e2e5ee293fc91817d0be310e26905755fb94d44417d94fa29ad4cc5d4aa19e78d25d41bc2d9e0948c034
4ce3f4a265 rpc, net: deprecate `startingheight` field of `getpeerinfo` RPC (Sebastian Falbesoner)
Pull request description:
This PR deprecates the "startingheight" result field of the `getpeerinfo` RPC, following the discussion in #33990.
Rationale: the reported starting height of a peer in the VERSION message is untrusted, and it doesn't seem to be useful anymore (after #20624), so deprecating the corresponding field seems reasonable. After that, it can be removed, along with the `m_starting_height` field of the Peer / CNodeStats structs, as it is sufficient to show the reported height only once at connection in the debug log.
ACKs for top commit:
optout21:
crACK 4ce3f4a265
achow101:
ACK 4ce3f4a265
fjahr:
utACK 4ce3f4a265
rkrux:
crACK 4ce3f4a265
janb84:
cr ACK 4ce3f4a265
Tree-SHA512: b296a28d30084fd35c67a2162e85576e3365e5d6fffe5b1add500034c1850604ee8c37b61afe812bfab8a7cc20f6a9e22db445e3c371311a5f82a777e5700ebf
5805a8b540 psbt: detect invalid MuSig2 pubkeys in deserialization (rkrux)
Pull request description:
Throw error while deserializing PSBT if invalid pubkeys are passed
as a MuSig2 aggregate or participant.
Should fix#33999 & #34201 by throwing error at the very start while decoding
an invalid PSBT that should subsequently not allow the MuSig2
signing operation to take place, thereby avoiding the crash.
ACKs for top commit:
fjahr:
utACK 5805a8b540
achow101:
ACK 5805a8b540
Tree-SHA512: 4741db96b278e9f3d532e1873af9530a70bbc7a8d3625b9e1c07001acc472fc10cbb79995c16bc4d06cc568ef98fe8d2b8e8d87b617dc05d7554085ffb92dfef
6da6f503a6 refactor: Let CCoinsViewCache::BatchWrite return void (TheCharlatan)
Pull request description:
CCoinsViewCache::BatchWrite always returns true if called from a backed cache, so just return void instead. Also return void from ::Sync and ::Flush.
This allows for dropping a FatalError condition and simplifying some dead error handling code a bit.
Since we now no longer exercise the "error path" when returning from `CCoinsView::BatchWrite`, make the method clear the cache instead. This should only be exercised by tests and not change production behaviour. This might slightly improve the coins_view fuzz test's ability to generate better coverage.
ACKs for top commit:
l0rinc:
ACK 6da6f503a6
andrewtoth:
re-ACK 6da6f503a6
achow101:
ACK 6da6f503a6
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/33866/commits/6da6f503a6dd8de85780ca402f5202095aa8b6ea
Tree-SHA512: dfaa325b0cf8108910aebf1b27434aaddb639d10d860e96797c77ea42eca9035a54a7dc1d6a5d4eae2b75fcc9356206d3d5672243d2c906e80d19024c8b95408
76c092ff80 wallet: warn against accidental unsafe older() import (Sjors Provoost)
592157b759 test: move SEQUENCE_LOCKTIME flags to script (Sjors Provoost)
Pull request description:
[BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/exploring-extended-relative-timelocks/1818/23), but is unsafe when used unintentionally: `older(65536)` is equivalent to `older(1)`.
This PR emits a warning when `importdescriptors` contains such a descriptor.
The first commit makes `SEQUENCE_LOCKTIME` flags reusable by other tests.
The main commit adds the `ForEachNode` helper to `miniscript.h` which is then used in the `MiniscriptDescriptor` constructor to check for `Fragment::OLDER` with unsafe values. These are stored in `m_warnings`, which the RPC code then collects via `Warnings()`.
It adds both a unit and functional test.
---
A previous version of this PR prevented the import, unless the user opted in with an `unsafe` flag. It also used string parsing in the RPC code.
---
Based on:
- [x] https://github.com/bitcoin/bitcoin/pull/33914
ACKs for top commit:
pythcoiner:
reACK 76c092ff80
achow101:
ACK 76c092ff80
rkrux:
lgtm re-ACK 76c092ff80
brunoerg:
reACK 76c092ff80
Tree-SHA512: 8e944e499bd4a43cc27eeb889f262b499b9b07aa07610f4a415ccb4e34a9110f9946646f446a54ac5bf17494d8d96a89e4a1fa278385db9b950468f27283e17a
217dbbbb5e test: Add musig failure scenarios (Fabian Jahr)
c9519c260b musig: Check session id reuse (Fabian Jahr)
e755614be5 sign: Remove duplicate sigversion check (Fabian Jahr)
0f7f0692ca musig: Move MUSIG_CHAINCODE to musig.cpp (Fabian Jahr)
Pull request description:
This is a follow-up to #29675 and primarily adds test coverage for some of the most prominent failure cases in the last commit.
The following commits address a few left-over nit comments that didn't make it in before merge.
ACKs for top commit:
achow101:
ACK 217dbbbb5e
rkrux:
lgtm ACK 217dbbb
Tree-SHA512: d73807bc31791ef1825c42f127c7ddfbc70b2b7cf782bc11341666e32e86b787ffc7aed64caea992909cef3a85fc6629282d8209c173aadec77f72fd0da96c45
1ed8e76165 rpc, doc: clarify the response of listtransactions RPC (rkrux)
Pull request description:
I noticed this behaviour while perf testing PR #27286 and it was not something that I expected, updating the doc to make it present in the RPCHelp command.
ACKs for top commit:
achow101:
ACK 1ed8e76165
furszy:
ACK 1ed8e76165
musaHaruna:
ACK [1ed8e76](https://github.com/bitcoin/bitcoin/pull/32737/changes/1ed8e7616527c69dbaa9904cda59e3b73c29fa5d) since my last review. New changes looks good, it's much easier to understand as well, looking at it from a user's perspective.
Tree-SHA512: 893a8e259201ac2140f46f827d81e681d2ec478c9571cceb10864aaa1b941991ce2263357d7c2b0024c04a9f8fbc372a020104b26e022c96289d271675947033
1841bf9cb6 test: address self-announcement (0xb10c)
Pull request description:
Test that a node sends a self-announcement with its external IP to in- and outbound peers after connection open and again sometime later.
Since the code for the test is mostly the same for addr and addrv2 messages, I opted to add a new test file instead of having duplicate code in `p2p_addr_relay.py` and `p2p_addrv2_relay.py`.
ACKs for top commit:
Bicaru20:
ACK https://github.com/bitcoin/bitcoin/commit/1841bf9cb67bca99273b3efa38633113422f5882
achow101:
ACK 1841bf9cb6
rkrux:
ACK 1841bf9
fjahr:
Code review ACK 1841bf9cb6
Tree-SHA512: 692a01e9f10eb55ee870de623e85182a10a75225766e0f0251ad5d9e369537ec27ca6e06905374190f3afe00ba6f71ae72f262228baaa535238a87160e1ce4f1
Some users will have backups of a legacy wallet which cannot be restored
due to being a legacy wallet, and therefore cannot be migrated from the
GUI. This menu item allows such users to restore and migrate their
wallets in a single action.
restore_and_migrate first restores a wallet file to the wallets
directory in the expected layout, then it performs legacy to descriptor
wallet migration on the restored wallet.
c1e554d3e5 refactor: consolidate 3 separate locks into one block (Andrew Toth)
41479ed1d2 test: add test for periodic flush inside ActivateBestChain (Andrew Toth)
84820561dc validation: periodically flush dbcache during reindex-chainstate (Andrew Toth)
Pull request description:
After #30611 we periodically do a non-erasing flush of the dbcache to disk roughly every hour during IBD.
The intention was to also do this periodic flush during reindex-chainstate, so we would not risk losing progress during a system failure when reindexing with a high dbcache value.
It was discovered that reindex-chainstate does not perform a PERIODIC flush until it has already reached the tip. Since reindexing to tip usually happens within 24 hours, this behaviour was unnoticed with the previous periodic flush interval. Note that reindex-chainstate still does IF_NEEDED flushes during `ConnectBlock`, so this also would not be noticed when running with a lower dbcache value.
This patch moves the PERIODIC flush from after the outer loop in `ActivateBestChain` to inside the outer loop after we release `cs_main`. This will periodically flush during IBD, reindex-chainstate, and steady state.
ACKs for top commit:
l0rinc:
ACK c1e554d3e5
achow101:
ACK c1e554d3e5
sipa:
utACK c1e554d3e5
Tree-SHA512: c447ad03e16c9978b8ed2c285b38e1b4c56e7778ab93b6f64435116f47b8931017f5f56ab53eb61656693146aaced776f666af573a41ab28e8f2b6d8657fa756
0ac969cddf validation: don't reallocate cache for short-lived CCoinsViewCache (Lőrinc)
c8f5e446dc coins: reduce lookups in dbcache layer propagation (Lőrinc)
Pull request description:
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)
### Summary
Previously, when the parent coins cache had no entry and the child did, `BatchWrite` performed a find followed by `try_emplace`, which resulted in multiple `SipHash` computations and bucket traversals on the common insert path.
On a different path, these caches were recreated needlessly for every block connection.
### Fix for double fetch
This change uses a single leading `try_emplace` and branches on the returned `inserted` flag. In the `FRESH && SPENT` case (not used in production, only exercised by tests), we erase the just-inserted placeholder (which is constant time with no rehash anyway). Semantics are unchanged for all valid parent/child state combinations.
This change is a minimal version of [bitcoin/bitcoin@`723c49b` (#32128)](https://github.com/bitcoin/bitcoin/pull/32128/commits/723c49b63bb10da843fbb6efc6928dca415cc47f) and draws simplification ideas [bitcoin/bitcoin@`ae76ec7` (#30673)](https://github.com/bitcoin/bitcoin/pull/30673/commits/ae76ec7bcff0a08a61f294882a71e46d177b009f) and https://github.com/bitcoin/bitcoin/pull/30326.
### Fix for temporary cache recreation
Related to parent cache propagation, the second commit makes it possible to avoid destructuring-recreating-destructuring of these short-live parent caches created for each new block.
A few temporary `CCoinsViewCache`'s are destructed right after the `Flush()`, therefore it is not necessary to call `ReallocateCache` to recreate them right before they're killed anyway.
This change was based on a subset of https://github.com/bitcoin/bitcoin/pull/28945, the original authors and relevant commenters were added as coauthors to this version.
-----
Reindex-chainstate indicates ~1% speedup.
<details>
<summary>Details</summary>
```python
COMMITS="647cdb4f7e8041affed887e2325ee03a91078bb1 0b0c3293ffd75afb27dadc0b28426b40132a8c6b"; \
STOP=909090; DBCACHE=4500; \
CC=gcc; CXX=g++; \
BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
(echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \
hyperfine \
--sort command \
--runs 2 \
--export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
--parameter-list COMMIT ${COMMITS// /,} \
--prepare "killall bitcoind 2>/dev/null; rm -f $DATA_DIR/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard && \
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DENABLE_IPC=OFF && ninja -C build bitcoind && \
./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 20" \
--cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
"COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0"
647cdb4f7e Merge bitcoin/bitcoin#33311: net: Quiet down logging when router doesn't support natpmp/pcp
0b0c3293ff validation: don't reallocate cache for short-lived CCoinsViewCache
Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e)
Time (mean ± σ): 16233.508 s ± 9.501 s [User: 19064.578 s, System: 951.672 s]
Range (min … max): 16226.790 s … 16240.226 s 2 runs
Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b)
Time (mean ± σ): 16039.626 s ± 17.284 s [User: 18870.130 s, System: 950.722 s]
Range (min … max): 16027.405 s … 16051.848 s 2 runs
Relative speed comparison
1.01 ± 0.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 647cdb4f7e)
1.00 COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=909090 -dbcache=4500 -reindex-chainstate -blocksonly -connect=0 -printtoconsole=0 (COMMIT = 0b0c3293ffd75afb27dadc0b28426b40132a8c6b)
```
</details>
ACKs for top commit:
optout21:
utACK 0ac969cddf
achow101:
ACK 0ac969cddf
andrewtoth:
utACK 0ac969cddf
sedited:
ACK 0ac969cddf
Tree-SHA512: 9fcc3f1a8314368576a4fba96ca72665527eaa3a97964ab5b39491757f3527147d134f79a5c3456f76c1330c7ef862989d23f764236c5e2563be89a81c1cee47
288b8c30be doc: Drop (default: none) from -i2psam description (Ryan Ofsky)
f6ec3519a3 init: Require explicit -asmap filename (Ryan Ofsky)
Pull request description:
Currently, if `-asmap` is specified without a filename bitcoind tries to load `ip_asn.map` data file.
This change now requires `-asmap=ip_asn.map` or another filename to be specified explicitly.
The change is intended to make behavior of the option explicit and avoid confusion reported https://github.com/bitcoin/bitcoin/issues/33386 where documentation specifies a default file which is not actually loaded by default. It was originally implemented in
https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3410302383 and various alternatives are discussed there.
ACKs for top commit:
brunoerg:
reACK 288b8c30be
fjahr:
re-ACK 288b8c30be
vostrnad:
utACK 288b8c30be
achow101:
ACK 288b8c30be
Tree-SHA512: 11a38a03892a58d6ccc1505cfbf915f58a86df9891761d89dc54b92d40593ee3cbb2d7c7bdbb922b871b3529072ef7f34cc98393aff6e8f0633b56352315b27c
de7c3587cd doc: Update add checksum instructions in tutorial (Ben Westgate)
2a46e94a16 doc: Update multisig-tutorial.md to use multipath descriptors (Ben Westgate)
Pull request description:
### Summary
Update `doc/multisig-tutorial.md` to use multipath descriptor format
instead of separate external/internal descriptors. The tutorial now:
- extracts a single `xpub_n` per participant
- constructs a multipath `wsh(sortedmulti(...))` descriptor with `<0;1>`
change index semantics
- uses `getdescriptorinfo` to compute descriptor checksum
- explains that `importdescriptors` expands the multipath descriptor
into internal and external descriptors
- update `/test/functional/wallet_multisig_descriptor_psbt.py` functional test / documentation to use multi-path descriptors
---
### Motivation
A single multipath descriptor is the most convenient pattern for multisig; our documentation should use it.
---
### What changed
- replaced extraction of `external_xpub_n` and `internal_xpub_n` with
extraction of a single `xpub_n`
- removed instructions to create and import separate external/internal
descriptors
- added instructions to build a multipath `wsh(sortedmulti(...))`
descriptor and derive checksum with `getdescriptorinfo`
- checksum field is parsed and appended as the multipath descriptor is not the canonical "desc" output
- clarified that `importdescriptors` automatically expands multipath
descriptors into internal and external forms
- similar changes to the functional test: wallet_multisig_descriptor_psbt.
---
### Testing
I have run the updated shell snippets and confirmed the multipath descriptor produces the same `listdescriptors` output after importing as the two descriptor method in bitcoin:master.
---
### Related issues / PRs
This tutorial change references the multipath descriptor
consolidation (see commit / PR referenced in the change). The commit
message points to bitcoin#22838 as the upstream change that enables
this behavior.
---
### Release note (for changelog)
Documentation: update multisig tutorial and multisig functional test to use multipath descriptors
ACKs for top commit:
Sjors:
utACK de7c3587cd
kannapoix:
Core review ACK: de7c3587cd
achow101:
ACK de7c3587cd
rkrux:
crACK de7c3587cd
Tree-SHA512: ca7275d8ad04922b3fa8d2a3084ff96aa2104265f1fc2f749814dd16776351ab692d67e7e643d08052f7492e3eaa1a9a4dfe3470163e57939a49f782d3df511a
fa1bf6818f clang-format: Set InsertNewlineAtEOF: true (MarcoFalke)
Pull request description:
Now that the minimum supported clang version is 17, the `InsertNewlineAtEOF` setting can be set to `true` in the clang-format file. (https://releases.llvm.org/16.0.0/tools/clang/docs/ClangFormatStyleOptions.html#insertnewlineateof)
This is in line with the already existing newline linter. Can be tested via:
```
truncate --size=-1 src/init.cpp
git diff
# Should fail:
cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=trailing_newline
# Restore newline:
git diff -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
```
ACKs for top commit:
l0rinc:
ACK fa1bf6818f
achow101:
ACK fa1bf6818f
hodlinator:
ACK fa1bf6818f
stickies-v:
ACK fa1bf6818f
hebasto:
ACK fa1bf6818f.
janb84:
ACK fa1bf6818f
Tree-SHA512: 6443f0f9e2710fb31c85e4ece5deb9fa25bf22ef2d5cadad2d54a239194860c3a785ffa5dd97be942e42139336575425499f085aa2e3f29003e529fdc20fce20
6657bcbdb4 kernel: allow null data_directory (stickies-v)
Pull request description:
An empty path may be represented with a `nullptr`. For example, `std::string_view{}.data()` may return nullptr.
Removes the `BITCOINKERNEL_ARG_NONNULL` attribute for `btck_chainstate_manager_options_create` 's `data_directory` parameter, and instead handles such null arguments in the implementation. [Because an empty path is meaningless](https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523930442), `btck_chainstate_manager_options_create` now treats both null and empty directories as invalid, tightening the interface.
Also documents how `BITCOINKERNEL_ARG_NONNULL` should be used.
Follow-up to https://github.com/bitcoin/bitcoin/pull/33853#pullrequestreview-3454620265
ACKs for top commit:
stringintech:
ACK 6657bcb
maflcko:
review ACK 6657bcbdb4🐪
achow101:
ACK 6657bcbdb4
TheCharlatan:
ACK 6657bcbdb4
janb84:
ACK 6657bcbdb4
Tree-SHA512: 11c02b221ff19a5357e94355808e3b503b3a336c16fc5186c9c9137931709e880383ed1f4990fc4cc6b0e23961e2e1e03fc90154a3b546b9490ef66bd63688b7
28a4fcb03c test: check listdescriptors do not return a mix of hardened derivation marker (pythcoiner)
975783cb79 descriptor: account for all StringType in MiniscriptDescriptor::ToStringHelper() (pythcoiner)
Pull request description:
In `MiniscriptDescriptor::ToStringHelper()` only the `StringType::Private` variant of the `type` argument was handled. This PR implements serializing w/ all variants of `StringType` & add a functional test for the descriptor triggering the related issue.
Closes#31694: previously when calling `listdescriptors` RPC on a wallet containing a taproot descriptor w/ a (miniscript) taptree, origins of internal key & taptree were serialized w/ differents hardened derivation markers:
- origin of the internal key were serialized w/ `StringType::Normalized` type (using `h` as marker)
- origins of taptree keys were serialized w/ `StringType::Private` type (using `'` as marker)
Note: Origins in segwit (`wsh()`) miniscript descriptors were also serialized w/ `StringType::Private` type (`'` marker) and are now serialized w/ `StringType::Normalized` type (`h` marker).
ACKs for top commit:
sipa:
Code review ACK 28a4fcb03c
achow101:
ACK 28a4fcb03c
rkrux:
Concept ACK 28a4fcb03c
Tree-SHA512: 15d14000b5951ca69a64a05b9a0b138c48a07b81eaf2fa86b91ac20cc8735533355a787363c64ba88403dd8a56ef5232cba57d34bea80835a0f40774d62fbc2b
f53dbbc505 test: Add functional tests for named argument parsing (zaidmstrr)
694f04e2bd rpc: Handle -named argument parsing where '=' character is used (zaidmstrr)
Pull request description:
Addresses [comment](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628) and [this](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2092039999).
The [PR #31375](https://github.com/bitcoin/bitcoin/pull/31375) got merged and enables `-named` by default in the `bitcoin rpc` interface; `bitcoin rpc` corresponds to `bitcoin-cli -named` as it's just a wrapper. Now, the problem arises when we try to parse the positional paramater which might contain "=" character. This splits the parameter into two parts first, before the "=" character, which treats this as the parameter name, but the other half is mostly passed as an empty string. Here, the first part of the string is an unknown parameter name; thus, an error is thrown. These types of errors are only applicable to those RPCs which might contain the `=` character as a parameter. Some examples are `finalizepsbt`, `decodepsbt`, `verifymessage` etc.
This is the one example of the error in `finalizepsbt` RPC:
```
./bitcoin-cli -named -regtest finalizepsbt cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=
error code: -8
error message:
Unknown named parameter cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA
```
This PR fixes this by updating the `vRPCConvertParams` table that identifies parameters that need special handling in `-named` parameter mode. The parser now recognises these parameters and handles strings with "=" char correctly, preventing them from being incorrectly split as parameter assignments.
ACKs for top commit:
ryanofsky:
Code review ACK f53dbbc505. Just applied comment & test suggestions since last review
kannapoix:
Code review ACK: f53dbbc505
achow101:
ACK f53dbbc505
Tree-SHA512: 1b517144efeff45a4c4256c27a39ddf187f1d6189d133402a45171678214a10ff2925c31edcfd556d67f85bd26d42f63c528b941b68c9880eab443f2c883e681
0aebdac95d init: completely remove `-maxorphantx` option (Sebastian Falbesoner)
Pull request description:
This is a small follow-up for #32941 (commit 1384dbaf6d), removing the `-maxorphantx` option completely, now that v30 has been released. If removing it for v31 is seen as controversial/premature (I personally don't think it is), the merge can be delayed for a future release.
ACKs for top commit:
maflcko:
lgtm ACK 0aebdac95d
achow101:
ACK 0aebdac95d
w0xlt:
ACK https://github.com/bitcoin/bitcoin/pull/33872/commits/0aebdac95da9a7d476264424c0107bd806ce5362
rkrux:
lgtm ACK 0aebdac95d
stickies-v:
ACK 0aebdac95d
Tree-SHA512: 818633b903174387ae259acb1d1e8ce07f78e158de2c150742ef0950b0f5d62af553e4e35ab962432306e04e07c45b1be11dbae459a8b62c4b9a6b5ef1746d26
d31158d364 psbt: clarify PSBT, PSBTInput, PSBTOutput unserialization flows (rkrux)
Pull request description:
The unserialization flows of the PSBT types work based on few underlying assumptions of functions from `serialize.h` & `stream.h` that takes some to understand when read the first time.
Add few comments that highlight these assumptions hopefully making it easier to grasp. Also, mention key/value format types as per BIP 174.
ACKs for top commit:
achow101:
ACK d31158d364
theStack:
ACK d31158d364
Tree-SHA512: 45111ef7f0258ebbc41d058b3ef2a72472774ab2878caf2d71d7b57b27549c46a51ccbeda5fe164bcf4f7ec10627bbae6e7763aa80b1e66912703a2088682817
743abbcbde refactor: inline constant return value of `BlockTreeDB::WriteBatchSync` and `BlockManager::WriteBlockIndexDB` and `BlockTreeDB::WriteFlag` (Lőrinc)
e030240e90 refactor: inline constant return value of `CDBWrapper::Erase` and `BlockTreeDB::WriteReindexing` (Lőrinc)
cdab9480e9 refactor: inline constant return value of `CDBWrapper::Write` (Lőrinc)
d1847cf5b5 refactor: inline constant return value of `TxIndex::DB::WriteTxs` (Lőrinc)
50b63a5698 refactor: inline constant return value of `CDBWrapper::WriteBatch` (Lőrinc)
Pull request description:
Related to https://github.com/bitcoin/bitcoin/pull/31144#discussion_r2223587480
### Summary
`WriteBatch` always returns `true` - the errors are handled by throwing `dbwrapper_error` instead.
### Context
This boolean return value of the `Write` methods is confusing because it's inconsistent with `CDBWrapper::Read`, which catches exceptions and returns a boolean to indicate success/failure. It's bad that `Read` returns and `Write` throws - but it's a lot worse that `Write` advertises a return value when it actually communicates errors through exceptions.
### Solution
This PR removes the constant return values from write methods and inlines `true` at their call sites. Many upstream methods had boolean return values only because they were propagating these constants - those have been cleaned up as well.
Methods that returned a constant `true` value that now return `void`:
- `CDBWrapper::WriteBatch`, `CDBWrapper::Write`, `CDBWrapper::Erase`
- `TxIndex::DB::WriteTxs`
- `BlockTreeDB::WriteReindexing`, `BlockTreeDB::WriteBatchSync`, `BlockTreeDB::WriteFlag`
- `BlockManager::WriteBlockIndexDB`
### Note
`CCoinsView::BatchWrite` (and transitively `CCoinsViewCache::Flush` & `CCoinsViewCache::Sync`) were intentionally not changed here. While all implementations return `true`, the base `CCoinsView::BatchWrite` returns `false`. Changing this would cause `coins_view` tests to fail with:
> terminating due to uncaught exception of type std::logic_error: Not all unspent flagged entries were cleared
We can fix that in a follow-up PR.
ACKs for top commit:
achow101:
ACK 743abbcbde
janb84:
ACK 743abbcbde
TheCharlatan:
ACK 743abbcbde
sipa:
ACK 743abbcbde
Tree-SHA512: b2a550bff066216f1958d2dd9a7ef6a9949de518cc636f8ab9c670e0b7a330c1eb8c838e458a8629acb8ac980cea6616955cd84436a7b8ab9096f6d648073b1e
060bb55508 rpc: add decoded tx details to gettransaction with extra wallet fields (Matthew Zipkin)
ad1c3bdba5 [move only] move DecodeTxDoc() to a common util file for sharing (Matthew Zipkin)
d633db5416 rpc: add "ischange: true" in wallet gettransaction decoded tx output (Matthew Zipkin)
Pull request description:
This change is motivated by external RBF clients like https://github.com/CardCoins/additive-rbf-batcher/. It saves the user a redundant re-looping of tx outputs, calling `getaddressinfo` on each one, looking for the change output in order to adjust the fee.
The field `"ischange"` only appears when `gettransaction` is called on a wallet, and is either `true` or not present at all. I chose not to include `ischange: false` because it is confusing to see that on *received* transactions.
Example of the new field:
```
"vout": [
{
"value": 1.00000000,
"n": 0,
"scriptPubKey": {
"asm": "0 5483235e05c76273b3b50af62519738781aff021",
"desc": "addr(bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu)#d42g84j6",
"hex": "00145483235e05c76273b3b50af62519738781aff021",
"address": "bcrt1q2jpjxhs9ca388va4ptmz2xtns7q6lupppkw7wu",
"type": "witness_v0_keyhash"
}
},
{
"value": 198.99859000,
"n": 1,
"scriptPubKey": {
"asm": "0 870ab1ab58632b05a417d5295f4038500e407592",
"desc": "addr(bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju)#tgapemkv",
"hex": "0014870ab1ab58632b05a417d5295f4038500e407592",
"address": "bcrt1qsu9tr26cvv4stfqh65547spc2q8yqavj7fnlju",
"type": "witness_v0_keyhash"
},
"ischange": true
}
]
```
ACKs for top commit:
furszy:
ACK [060bb55](https://github.com/bitcoin/bitcoin/pull/32517/commits/060bb55508245776bb6a39c8b7849769ee588d69)
maflcko:
review ACK 060bb55508🌛
achow101:
ACK 060bb55508
rkrux:
lgtm ACK 060bb55508
Tree-SHA512: aae4854d2bb4e9a7bc1152691ea90e594e8da8a63c9c7fda72a504fb6a7e54ae274ed5fa98d35d270e0829cc8f8d2fd35a5fc9735c252a10aa42cc22828930e7
01cc20f330 test: improve coverage for a resolved stalling situation (Martin Zumsande)
9af6daf07e test: remove magic number when checking for blocks that have arrived (Martin Zumsande)
3069d66dca p2p: During block download, adjust pindexLastCommonBlock better (Martin Zumsande)
Pull request description:
As described in #32179, `pindexLastCommonBlock` is updated later than necessary
in master.
In case of a linear chain with no forks, it can be moved forward at the beginning of
`FindNextBlocksToDownload`, so that the updated value can be used to better estimate `nWindowEnd`.
This helps the node to request all blocks from peers within the correct 1024-block-window and avoids peers being incorrectly marked as stallers.
I also changed `p2p_ibd_stalling.py` to cover the situation after a resolved situation, making sure that no additional peers are marked for stalling.
Fixes#32179
ACKs for top commit:
Crypt-iQ:
crACK 01cc20f330
stringintech:
re-ACK 01cc20f
achow101:
ACK 01cc20f330
sipa:
utACK 01cc20f330
Tree-SHA512: a97f7a7ef5ded538ee35576e04b3fbcdd46a6d0189c7ba3abacc6e0d81e800aac5b0c2d2565d0462ef6fd4acc751989f577fd6adfd450171a7d6ab26f437df32
Ava Chow