5888b4a2a5 doc: add note for watch-only wallet migration (rkrux)
Pull request description:
This was suggested in a previous PR #31423.
ACKs for top commit:
achow101:
ACK 5888b4a2a5
brunoerg:
reACK 5888b4a2a5
jonatack:
ACK 5888b4a2a5
Tree-SHA512: 96e51eda30a1f31cfd82ae3296ca97c9236599b18e19086dbde3a908f6fe66af8f2de7aa147bdb9ebccb2059c809a25ddfb0c23da57e1a84a35b62ca0a44e3c3
76fe0e59ec test: Migration of a wallet ending in `../` (David Gumberg)
f0bb3d50fe test: Migration of a wallet ending in `/` (David Gumberg)
41faef5f80 test: Migration fail recovery w/ `../` in path (David Gumberg)
63c6d36437 test: Migration of a wallet with `../` in path. (David Gumberg)
70f1c99c90 wallet: Fix migration of wallets with pathnames. (David Gumberg)
f6ee59b6e2 wallet: migration: Make backup in walletdir (David Gumberg)
e22c3599c6 test: wallet: Check direct file backup name. (David Gumberg)
Pull request description:
Support for wallets outside of the default wallet directory was added in #11687, and these external wallets can be specified with paths relative to the wallet directory, e.g. `bitcoin-cli loadwallet ../../mywallet`. In the RPC commands, there is no distinction between a wallet's 'name' and a wallet's 'path'. This PR fixes an issue with wallet backup during migration where the wallet's 'name-path' is used in the backup filename. This goes south when that filename is appended to the directory where we want to put the file and the wallet's 'name' actually gets treated as a path:
```cpp
fs::path backup_filename = fs::PathFromString(strprintf("%s_%d.legacy.bak", (wallet_name.empty() ? "default_wallet" : wallet_name), GetTime()));
fs::path backup_path = this_wallet_dir / backup_filename;
```
Attempting to migrate a wallet with the 'name' `../../../mywallet` results in a backup being placed in `datadir/wallets/../../../mywallet/../../../mywallet_1744683963.legacy.bak`.
If permissions don't exist to write to that folder, migration can fail.
The solution implemented here is to put backup files in the top-level of the node's `walletdir` directory, using the folder name (and in some rare cases the file name) of the wallet to name the backup file:
9fa5480fc4/src/wallet/wallet.cpp (L4254-L4268)
##### Steps to reproduce on master
Build and run `bitcoind` with legacy wallet creation enabled:
```bash
$ cmake -B build -DWITH_BDB=ON && cmake --build build -j $(nproc)
$ ./build/bin/bitcoind -regtest -deprecatedrpc=create_bdb
```
Create a wallet with some relative path specifiers (exercise caution with where this file may be written)
```bash
$ ./build/bin/bitcoin-cli -regtest -named createwallet wallet_name="../../../myrelativewallet" descriptors=false
```
Try to migrate the wallet:
```bash
$ ./build/bin/bitcoin-cli -regtest -named migratewallet wallet_name="../../../myrelativewallet"
```
You will see a message in `debug.log` about trying to backup a file somewhere like: `/home/user/.bitcoin/regtest/wallets/../../../myrelativewallet/../../../myrelativewallet_1744686627.legacy.bak` and migration might fail because `bitcoind` doesn't have permissions to write the backup file.
ACKs for top commit:
pablomartin4btc:
tACK 76fe0e59ec
achow101:
ACK 76fe0e59ec
ryanofsky:
Code review ACK 76fe0e59ec. Nice changes that (1) fix potential errors when names of wallets being migrated contain slashes, and (2) store migration backups in the top-level `-walletdir` instead of in individual wallet subdirectories.
Tree-SHA512: 5cf6ed9f44ac7d204e4e9854edd3fb9b43812e930f76343b142b3c19df3de2ae5ca1548d4a8d26226d537bca231e3a50b3ff0d963c200303fb761f2b4eb3f0d9
fa45ccc15d doc: Add legacy wallet removal release notes (MarcoFalke)
Pull request description:
This spans over several pulls, so add a single note for all of them.
ACKs for top commit:
glozow:
lgtm ACK fa45ccc15d
achow101:
ACK fa45ccc15d
pablomartin4btc:
ACK fa45ccc15d
janb84:
re ACK fa45ccc15d
Tree-SHA512: e753cc3afbd66a88099ff62c2591aa31d32d784098e433e392c20a8dfd40d5c85807e955b264a287c3778d68605cd7022278886a43cd1635c080d563c88fc0cc
aac0b6dd79 test: test sendall and send do anti-fee-sniping (ishaanam)
20802c7b65 wallet, rpc: add anti-fee-sniping to `send` and `sendall` (ishaanam)
Pull request description:
Currently, `send` and `sendall` don't do anti-fee-sniping because they don't use `CreateTransaction`. This PR adds anti-fee-sniping to these RPCs by calling `DiscourageFeeSniping` from `FinishTransaction` when the user does not specify a locktime.
ACKs for top commit:
achow101:
ACK aac0b6dd79
murchandamus:
ACK aac0b6dd79
glozow:
ACK aac0b6dd79
Tree-SHA512: d4f1b43b5bda489bdba46b0af60e50bff0de604a35670e6ea6e1de2b539f16b3f68805492f51d6d2078d421b63432ca22a561a5721d1a37686f2e48284e1e646
1bed0f734b guix: warn SOURCE_DATE_EPOCH set in guix-codesign (will)
Pull request description:
#32678 added a sanity check for this environment variable when running `guix-build` but missed that `guix-codesign` also relies on `SOURCE_DATE_EPOCH`, which can result in non-determinism in the codesigning step: https://github.com/bitcoin-core/guix.sigs/pull/1720#issuecomment-3124332676
To avoid repeating the logic move common functionality into the prelude and call the function in both guix actions.
ACKs for top commit:
fanquake:
ACK 1bed0f734b
Tree-SHA512: ad3de8ab06e7f4ffcee5c02e8185b20879d63a02a614a706ea54da5087cca4ba75817ca1aa95301572c34723317fcc44e4478261ac73dd223ee9fa827e6b35b3
3b23f95e34 ci: limit max stack size to 512 KiB (dergoegge)
2931a87477 ci: limit stack size to 512kb in native macOS jobs (fanquake)
Pull request description:
Picks up #31367.
Closes#29840.
Limit adjustment is moved until after compilation, otherwise compilation might not succeed.
I've used compilation flags to limit the stack size in the native macOS jobs, because trying to use `ulimit` resulted in:
```bash
+ '[' -n 1 ']'
+ ulimit -s 262144
/Users/runner/work/bitcoin/bitcoin/ci/test/03_test_script.sh: line 17: ulimit: stack size: cannot modify limit: Operation not permitted
```
See example failures (`ulimit -s 64`) here: https://github.com/bitcoin/bitcoin/runs/46861548042.
ACKs for top commit:
dergoegge:
utACK 3b23f95e34
Tree-SHA512: 7e00626f3ca9e860d79a301af2427008ce27c329b618e24f95e7a55b284459a446216d2859c2e63be50abb9d4f0d343c12ff50445231652d354f225477928a35
6757052fc4 doc: move `cmake -B build -LH` up in Unix build docs (Bufo)
Pull request description:
#32269 rebased.
> I had trouble building bitcoin core the way I wanted since now more features require a flag while building. IMO it makes sense to make it a bit more prominent in the build docs how to get the needed flags.
> Related issue: https://github.com/bitcoin/bitcoin/issues/32258
ACKs for top commit:
maflcko:
lgtm ACK 6757052fc4
stickies-v:
ACK 6757052fc4
janb84:
ACK 6757052fc4
Tree-SHA512: 8e6dc1e432c067f862560776176112d5c24c4009bdf8e9a4e8d1ea3328b88732188fc4d8a7cd29f8a9ed8a1809a7a8a86d63b7ae3ec4ebae74be466727c8d730
62ed1f92ef txgraph: check that DoWork finds optimal if given high budget (tests) (Pieter Wuille)
f3c2fc867f txgraph: add work limit to DoWork(), try optimal (feature) (Pieter Wuille)
e96b00d99e txgraph: make number of acceptable iterations configurable (feature) (Pieter Wuille)
cfe9958852 txgraph: track amount of work done in linearization (preparation) (Pieter Wuille)
6ba316eaa0 txgraph: 1-or-2-tx split-off clusters are optimal (optimization) (Pieter Wuille)
fad0eb091e txgraph: reset quality when merging clusters (bugfix) (Pieter Wuille)
Pull request description:
Part of #30289. Builds on top of #31553.
So far, the `TxGraph::DoWork()` function took no parameters, and just made all clusters reach the "acceptable" internal quality level by performing a minimum number of improvement iterations on it, but:
* Did not attempt to go beyond that.
* Was broken, as the QualityLevel of optimal clusters that merge together was not being reset.
Fix this by adding an argument to `DoWork()` to control how much work it is allowed to do right now, which will first be used to get all clusters to the acceptable level, and if more budget remains, use it to try to get some or all clusters optimal. The function will now return `true` if all clusters are known to be optimal (and thus no further work remains). This is verified in the tests, by remembering whether the graph is optimal, and if it is at the end of the simulation run, verify that the overall linearization cannot be improved further.
ACKs for top commit:
instagibbs:
ACK 62ed1f92ef
ismaelsadeeq:
Code review ACK 62ed1f92ef
glozow:
ACK 62ed1f92ef
Tree-SHA512: 5f57d4052e369f3444e72e724f04c02004e0f66e365faa59c9f145323e606508380fc97bb038b68783a62ae9c10757f1b628b3b00b2ce9a46161fca2d4336d73
0ce041ea88 tracing: fix pointer argument handling in mempool_monitor.py (deadmanoz)
Pull request description:
The BPF code was incorrectly passing pointer variables by value to `bpf_usdt_readarg()`, causing the function to fail silently and resulting in transaction hashes showing as zeros and reason strings displaying empty strings.
This fix adds the missing reference operator (&) when passing pointer variables to `bpf_usdt_readarg()`, allowing the function to properly write the pointer values and enabling correct display of transaction hashes and removal/rejection reasons.
Fixes the regression introduced in [ec47ba349d](ec47ba349d) where `bpf_usdt_readarg_p` was replaced with `bpf_usdt_readarg` but the calling convention wasn't properly updated for pointer arguments.
**Before: "0000000000000000000000000000000000000000000000000000000000000000" tx hashes, and missing reasons (empty strings) for removal.**
<img width="1683" height="1330" alt="Screenshot 2025-07-29 at 4 30 03 PM" src="https://github.com/user-attachments/assets/71ba88be-dbcc-43a6-bfe7-bd49ae082b13" />
**After: tx hashes show, reasons for removal showing.**
<img width="1682" height="1330" alt="Screenshot 2025-07-29 at 4 29 23 PM" src="https://github.com/user-attachments/assets/03738c75-5526-4c1e-82c2-ba100cdf128a" />
ACKs for top commit:
0xB10C:
tested ACK 0ce041ea88
Tree-SHA512: cb50748fa2cd38be4b0abed36723917c2c82a92f588928bb0650eed0049c121df89b33d53421037b12836a497f30b449fe3d041ff7755a1fd9da43544392cf40
b6d4688f77 [doc] reword comments in test_mid_package_replacement (glozow)
f3a613aa5b [cleanup] delete brittle test_mid_package_eviction (glozow)
c3cd7fcb2c [doc] remove references to now-nonexistent Finalize() function (glozow)
d8140f5f05 don't make a copy of m_non_base_coins (glozow)
98ba2b1db2 [doc] MemPoolAccept coins views (glozow)
ba02c30b8a [doc] always CleanupTemporaryCoins after a mempool trim (glozow)
Pull request description:
Deletes `test_mid_package_eviction` that is brittle and already covered in other places. It was introduced in #28251 addressing 2 issues: (1) calling `LimitMempoolSize()` in the middle of package validation and (2) not updating coins view cache when the mempool contents change, leading to "disappearing coins."
(1) If you let `AcceptSingleTransaction` call `LimitMempoolSize` in the middle of package validation, you should get a failure in `test_mid_package_eviction_success` (the package is rejected):
```
diff --git a/src/validation.cpp b/src/validation.cpp
index f2f6098e214..4bd6f059849 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1485,7 +1485,7 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
FinalizeSubpackage(args);
// Limit the mempool, if appropriate.
- if (!args.m_package_submission && !args.m_bypass_limits) {
+ if (!args.m_bypass_limits) {
LimitMempoolSize(m_pool, m_active_chainstate.CoinsTip());
// If mempool contents change, then the m_view cache is dirty. Given this isn't a package
// submission, we won't be using the cache anymore, but clear it anyway for clarity.
```
Mempool modifications have a pretty narrow interface since #31122 and `TrimToSize()` cannot be called while there is an outstanding mempool changeset. So I think there is a low likelihood of accidentally reintroducing this problem and not immediately hitting e.g. a fuzzer crash on this line b53fab1467/src/txmempool.cpp (L1143)
(2) If you remove the `CleanupTemporaryCoins()` call from `ClearSubPackageState()` you should get a failure from `test_mid_package_replacement`:
```
diff --git a/src/validation.cpp b/src/validation.cpp
index f2f6098e214..01b904b69ef 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -779,7 +779,7 @@ private:
m_subpackage = SubPackageState{};
// And clean coins while at it
- CleanupTemporaryCoins();
+ // CleanupTemporaryCoins();
}
};
```
I also added/cleaned up the documentation about coins views to hopefully make it extremely clear when people should `CleanupTemporaryCoins`.
ACKs for top commit:
instagibbs:
reACK b6d4688f77
sdaftuar:
utACK b6d4688f77
marcofleon:
ACK b6d4688f77
Tree-SHA512: 79c68e263013b1153520f5453e6b579b8fe7e1d6a9952b1ac2c3c3c017034e6d21d7000a140bba4cc9d2ce50ea3a84cc6f91fd5febc52d7b3fa4f797955d987d
The BPF code was incorrectly passing pointer variables by value to
bpf_usdt_readarg(), causing the function to fail silently and resulting
in transaction hashes and reason strings displaying as zeros or garbage.
This fix adds the missing reference operator (&) when passing pointer
variables to bpf_usdt_readarg(), allowing the function to properly
write the pointer values and enabling correct display of transaction
hashes and removal/rejection reasons.
Fixes the regression introduced in ec47ba349d where bpf_usdt_readarg_p
was replaced with bpf_usdt_readarg but the calling convention wasn't
properly updated for pointer arguments.
1c10b7351e RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo (Kristaps Kaupe)
Pull request description:
Other node relay settings like `fullrbf` and `minrelaytxfee` are already returned, makes sense to add these two too.
ACKs for top commit:
ajtowns:
ACK 1c10b7351e
maflcko:
lgtm ACK 1c10b7351e
theStack:
ACK 1c10b7351e
Tree-SHA512: 1750d7d12de511f0ac34922ea9c58c4b9b55c3aaf22109abfd7dbe01ad1eb7b48fb4a6b074a0baf0e55ee2270fcc969b6830e499ff33adbcd0b9c761fb25e563
251d020846 init, wallet: replace hardcoded output types with `FormatAllOutputTypes` (Sebastian Falbesoner)
e3ba0757a9 rpc, wallet: replace remaining hardcoded output types with `FormatAllOutputTypes` (Sebastian Falbesoner)
Pull request description:
This PR takes use of the `FormatAllOutputTypes` helper (introduced in PR #32432, commit 8cc9845b8d) to get rid of the remaining hardcoded output types in wallet RPC and command line arguments documentation [1]. Note that it can't be used in the [`createmultisig` RPC](fc162299f0/src/rpc/output_script.cpp (L100)), as this one is only for pre-taproot output types and hence doesn't contain "bech32m" in the list.
[1] instances were found via `$ git grep legacy.*p2sh-segwit ./src/rpc/ ./src/wallet/`
ACKs for top commit:
nervana21:
tACK [251d020](251d020846)
maflcko:
review ACK 251d020846 🌨
pablomartin4btc:
re-utACK 251d020846
rkrux:
crACK 251d020846
Tree-SHA512: 23d1025d068f3a44b115a34b217b808fcae59fc574e35a899f0d43a85512935c90675d2e98c621287e02adc3a9f4a08289a26596c66e2122262af0cd2dfbde72
8aed477c33 test: fix RPC coverage check (Brandon Odiwuor)
2630b64f81 test: add abortrescan RPC test (Brandon Odiwuor)
Pull request description:
This is #27593 cleaned up / rebased, now that the legacy wallet has been dropped.
Closes#27593.
ACKs for top commit:
maflcko:
lgtm ACK 8aed477c33
cedwies:
ACK 8aed477
Tree-SHA512: 14a28b1ef0c1f63236d04c2ff6c11adddc40642e4a23d30398e8a03fc47f911465af91affc6e66ee2d548515ef4f65fb1cb5d69985c5a771a17b1c9c009f48ad
Currently there is a warning for this in guix-build, but we also need
one in guix-codesign, otherwise the codesigned hashes are not
reproducible.
Move common functionality into prelude and call the function in both
guix actions.
faa1c3e80d Revert "Merge bitcoin/bitcoin#32343: common: Close non-std fds before exec in RunCommandJSON" (MarcoFalke)
Pull request description:
After a fork() in a multithreaded program, the child can safely
call only async-signal-safe functions (see [signal-safety(7)](https://www.man7.org/linux/man-pages/man7/signal-safety.7.html))
until such time as it calls execv.
The standard library (`std` namespace) is not async-signal-safe. Also, `throw`, isn't.
There was an alternative implementation using `readdir` (https://github.com/bitcoin/bitcoin/pull/32529), but that isn't async-signal-safe either, and that implementation was still using `throw`.
So temporarily revert this feature.
A follow-up in the future can add it back, using only async-signal-safe functions, or by using a different approach.
Fixes https://github.com/bitcoin/bitcoin/issues/32524
Fixes https://github.com/bitcoin/bitcoin/issues/33015
Fixes https://github.com/bitcoin/bitcoin/issues/32855
For reference, a failure can manifest in the GCC debug mode:
* While `fork`ing, a debug mode mutex is held (by any other thread).
* The `fork`ed child tries to use the stdard libary before `execv` and deadlocks.
This may look like the following:
```
(gdb) thread apply all bt
Thread 1 (Thread 0xf58f4b40 (LWP 774911) "b-httpworker.2"):
#0 0xf7f4f589 in __kernel_vsyscall ()
#1 0xf79e467e in ?? () from /lib32/libc.so.6
#2 0xf79eb582 in pthread_mutex_lock () from /lib32/libc.so.6
#3 0xf7d93bf2 in ?? () from /lib32/libstdc++.so.6
#4 0xf7d93f36 in __gnu_debug::_Safe_iterator_base::_M_attach(__gnu_debug::_Safe_sequence_base*, bool) () from /lib32/libstdc++.so.6
#5 0x5668810a in __gnu_debug::_Safe_iterator_base::_Safe_iterator_base (this=0xf58f13ac, __seq=0xf58f13f8, __constant=false) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_base.h:91
#6 0x56ddfb50 in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::forward_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:162
#7 0x56ddfacb in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::bidirectional_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:539
#8 0x56ddfa5b in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::random_access_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:687
#9 0x56ddd3f6 in std::__debug::vector<int, std::allocator<int> >::begin (this=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/vector:300
#10 0x57d83701 in subprocess::detail::Child::execute_child (this=0xf58f156c) at ./util/subprocess.h:1372
#11 0x57d80a7c in subprocess::Popen::execute_process (this=0xf58f1cd8) at ./util/subprocess.h:1231
#12 0x57d6d2b4 in subprocess::Popen::Popen<subprocess::input, subprocess::output, subprocess::error, subprocess::close_fds> (this=0xf58f1cd8, cmd_args="fake.py enumerate", args=..., args=..., args=..., args=...) at ./util/subprocess.h:964
#13 0x57d6b597 in RunCommandParseJSON (str_command="fake.py enumerate", str_std_in="") at ./common/run_command.cpp:27
#14 0x57a90547 in ExternalSigner::Enumerate (command="fake.py", signers=std::__debug::vector of length 0, capacity 0, chain="regtest") at ./external_signer.cpp:28
#15 0x56defdab in enumeratesigners()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const (this=0xf58f2ba0, self=..., request=...) at ./rpc/external_signer.cpp:51
...
(truncated, only one thread exists)
```
ACKs for top commit:
fanquake:
ACK faa1c3e80d
darosior:
ACK faa1c3e80d
Tree-SHA512: 602da5f2eba08d7fe01ba19baf411e287ae27fe2d4b82f41734e05b7b1d938ce94cc0041e86ba677284fa92838e96ebee687023ff28047e2b036fd9a53567e0a
The current `prevector` size of 28 bytes (chosen to fill the `sizeof(CScript)` aligned size) was introduced in 2015 (https://github.com/bitcoin/bitcoin/pull/6914) before SegWit and TapRoot.
However, the increasingly common `P2WSH` and `P2TR` scripts are both 34 bytes, and are forced to use heap (re)allocation rather than efficient inline storage.
The core trade-off of this change is to eliminate heap allocations for common 34-36 byte scripts at the cost of increasing the base memory footprint of all `CScript` objects by 8 bytes (while still respecting peak memory usage defined by `-dbcache`).
Increasing the `prevector` size allows these scripts to be stored inline, avoiding extra heap allocations, reducing potential memory fragmentation, and improving performance during cache flushes. Massif analysis confirms a lower stable memory usage after flushing, suggesting the elimination of heap allocations outweighs the larger base size for common workloads.
Due to memory alignment, increasing the `prevector` size to 36 bytes doesn't change the overall `sizeof(CScript)` compared to an increase to 34 bytes, allowing us to include `P2PK` scripts as well at no additional memory cost.
Performance benchmarks for AssumeUTXO load and flush show:
* Small dbcache (450MB): ~1-3% performance improvement (despite more frequent flushes)
* Large dbcache (4500MB): ~6-8% performance improvement due to fewer heap allocations (and basically the number of flushes)
* Very large dbcache (4500MB): ~5-6% performance improvement due to fewer heap allocations (and memory limit not being reached, so there's no memory penalty)
Full IBD and reindex-chainstate with larger `dbcache` values also show an overall ~3-4% speedup.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Verifies that script types are correctly allocated using prevector's direct or indirect storage based on their size:
Direct allocated script types (size ≤ 28 bytes):
* OP_RETURN (small)
* P2WPKH
* P2SH
* P2PKH
Indirect allocated script types (size > 28 bytes):
* P2WSH
* P2TR
* P2PK
* MULTISIG (small)
This test provides a baseline for verifying changes to prevector's inline capacity.
The `CHECK_SCRIPT_STATIC_SIZE` and `CHECK_SCRIPT_DYNAMIC_SIZE` macros were added to differentiate the two cases - while preserving the correct source code line in case of failure.
c5c1960f93 doc: Add release notes for changes in RPCs (pablomartin4btc)
90fd5acbe5 rpc, test: Fix error message in getdescriptoractivity (pablomartin4btc)
39fef1d203 test: Add missing logging info for each test (pablomartin4btc)
53ac704efd rpc, test: Fix error message in unloadwallet (pablomartin4btc)
1fc3a8e8e7 rpc, test: Add EnsureUniqueWalletName tests (pablomartin4btc)
b635bc0896 rpc, util: Add EnsureUniqueWalletName (pablomartin4btc)
Pull request description:
Currently, `unloadwallet` RPC call fails with a JSON parsing error when no `wallet_name` argument is provided. This behavior is misleading because the error originates from a low-level JSON type mismatch, rather than clearly indicating that the wallet name or RPC endpoint (`-rpcwallet=...`) is missing. Also, found out that the [issue](https://github.com/bitcoin/bitcoin/pull/13111#issuecomment-398831543) was noticed during its implementation but never addressed.
In addition, I've verified all RPC commands calls finding that `getdescriptoractivity` had the same problem, but related to the array input types (blockhashes & descriptors), so I've corrected that RPC as well. For consistency I've added the missing logging info for each test case in `test/functional/rpc_getdescriptoractivity.py` in preparation for the new test.
**_-Before_**
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
error code: -3
error message:
JSON value of type number is not of expected type string
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
error code: -3
error message:
JSON value of type null is not of expected type array
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
error code: -3
error message:
JSON value of type null is not of expected type array
```
**_-After_**
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
error code: -8
error message:
Either the RPC endpoint wallet or the wallet name parameter must be provided
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
error code: -1
error message:
getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool )
Get spend and receive activity associated with a set of descriptors for a set of blocks. This command pairs well with the `relevant_blocks` output of `scanblocks()`.
This call may take several minutes. If you encounter timeouts, try specifying no RPC timeout (bitcoin-cli -rpcclienttimeout=0)
Arguments:
1. blockhashes (json array, required) The list of blockhashes to examine for activity. Order doesn't matter. Must be along main chain or an error is thrown.
[
"blockhash", (string) A valid blockhash
...
]
2. scanobjects (json array, required) Array of scan objects. Every scan object is either a string descriptor or an object:
[
"descriptor", (string) An output descriptor
{ (json object) An object with output descriptor and metadata
"desc": "str", (string, required) An output descriptor
"range": n or [n,n], (numeric or array, optional, default=1000) The range of HD chain indexes to explore (either end or [begin,end])
},
...
]
3. include_mempool (boolean, optional, default=true) Whether to include unconfirmed activity
...
```
```
./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
error code: -1
error message:
getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool )
...
```
ACKs for top commit:
achow101:
ACK c5c1960f93
stickies-v:
re-ACK c5c1960f93
furszy:
ACK c5c1960f93
Tree-SHA512: e831ff1acbfd15d2ce3a69bb408cce94664c0b63b2aa2f4627a05c6c052241ae3b5cc238219ef1b30afb489a4a3f4c3030e2168b0c8f08b4d20805d050d810f5
This commit takes use of the `FormatAllOutputTypes` helper
(introduced in PR #32432, commit 8cc9845b8d)
to get rid of the hardcoded output types in wallet RPC documentation.
Note that it can't be used in the `createmultisig` RPC, as this one is
only for pre-taproot output types and hence doesn't contain "bech32m" in the list.
1cb2399703 doc: clarify the GetAddresses/GetAddressesUnsafe documentation (Daniela Brozzoni)
e5a7dfd79f p2p: rename GetAddresses -> GetAddressesUnsafe (Daniela Brozzoni)
Pull request description:
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this function should only be used in trusted contexts. This helps avoid accidental privacy leaks by preventing the uncached version from being used in non-trusted scenarios, like P2P.
Additionally, better reflect in the documentation that the two methods should be used in different contexts.
Also update the outdated "call the function without a parameter" phrasing in the cached version. This wording was accurate when the cache was introduced in #18991, but became outdated after later commits (f26502e9fc, 81b00f8780) added parameters to each
function, and the previous commit changed the function naming completely.
ACKs for top commit:
stickies-v:
re-ACK 1cb2399703
l0rinc:
ACK 1cb2399703
luisschwab:
ACK 1cb2399703
brunoerg:
ACK 1cb2399703
theStack:
Code-review ACK 1cb2399703
mzumsande:
Code review ACK 1cb2399703
Tree-SHA512: 02c05d88436abcdfabad994f47ec5144e9ba47668667a2c1818f57bf8710727505faf8426fd0672c63de14fcf20b96f17cea2acc39fe3c1f56abbc2b1a9e9c23
fa1fd07468 ci: Enable more shellcheck (MarcoFalke)
Pull request description:
shellcheck is often the main "reviewer" of CI code written in Bash, so it seems odd to disable it by putting commands into `bash -c "cmd..."`.
Fix that by removing `bash -c`, where it isn't intended and where the removal is easily possible.
ACKs for top commit:
hebasto:
ACK fa1fd07468.
Tree-SHA512: 6412dd3f8d702bca7762a8f1be3f9d2782132936fcc7ae5c31690b594e04f69708110e6f6233d5a61901289d13c7089ab5646a2c3ef2266fffc36d0543f4b7ae
face8123fd log: [refactor] Use info level for init logs (MarcoFalke)
fa183761cb log: Remove function name from init logs (MarcoFalke)
Pull request description:
Many of the normal, and expected init logs, which are run once after startup use the deprecated alias of `LogInfo`.
Fix that by using `LogInfo` directly, which is a refactor, except for a few log lines that also have `__func__` removed.
(Also remove the unused trailing `\n` char while touching those logs)
ACKs for top commit:
stickies-v:
re-ACK face8123fd
fanquake:
ACK face8123fd
Tree-SHA512: 28c296129c9a31dff04f529c53db75057eae8a73fc7419e2f3068963dbb7b7fb9a457b2653f9120361fdb06ac97d1ee2be815c09ac659780dff01d7cd29f8480
fa1a14a13a fuzz: Reset chainman state in process_message(s) targets (MarcoFalke)
fa9a3de09b fuzz: DisableNextWrite (MarcoFalke)
aeeeeec9f7 fuzz: Reset dirty connman state in process_message(s) targets (MarcoFalke)
fa11eea405 fuzz: Avoid non-determinism in process_message(s) target (PeerMan) (MarcoFalke)
Pull request description:
`process_message(s)` are the least stable fuzz targets, according to OSS-Fuzz.
Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018.
### Testing
Needs coverage compilation, as explained in `./contrib/devtools/README.md`. And then, using 32 threads:
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ process_messages 32
```
Each commit can be reverted to see more non-determinism re-appear.
ACKs for top commit:
marcofleon:
ReACK fa1a14a13a
dergoegge:
reACK fa1a14a13a
Tree-SHA512: 37b5b6dbdde6a39b4f83dc31e92cffb4a62a4b8f5befbf17029d943d0b2fd506f4a0833570dcdbf79a90b42af9caca44e98e838b03213d6bc1c3ecb70a6bb135
It is redundant with -logsourcelocations and the log messages are
clearer without it.
Also, remove a double-space.
Also, add braces around `if` touched in the next commit.
This tiny behavior change requires a test fixup.
6135e0553e wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC (Ava Chow)
Pull request description:
If the locked coin needs to be persisted to the wallet database, insteead of having the RPC figure out when to create a WalletBatch and having LockCoin's behavior depend on it, have LockCoin take whether to persist as a parameter so it makes the batch.
Since unlocking a persisted locked coin requires a database write as well, we need to track whether the locked coin was persisted to the wallet database so that it can erase the locked coin when necessary.
Keeping track of whether a locked coin was persisted is also useful information for future PRs.
Split from #32489
ACKs for top commit:
rkrux:
ACK 6135e05
Sjors:
ACK 6135e0553e
w0xlt:
ACK 6135e0553e
Tree-SHA512: 0e2367fc4d50c62ec41443374b64c4c5ecf679998677df47fb8776cfb44704713bc45547e32e96cd30d1dbed766f5d333efb6f10eb0e71271606638e07e61a01
faa3171ff2 ci: Use optimized Debug build type in test-each-commit (MarcoFalke)
fa21c3401e ci: [doc] reword debug log message (MarcoFalke)
Pull request description:
An optimized debug build is mostly as fast as a release build, because hot loops of heavy debug-only code are rare. So use that setting in the test-each-commit CI, to enable more checks almost "for free".
ACKs for top commit:
Prabhat1308:
re-ACK [`faa3171`](faa3171ff2)
willcl-ark:
crACK faa3171ff2
Tree-SHA512: ca041cf7f79d7abb6f93e17b58b2aea730f3bb9fc51256c1ca1b9f7ce7e7188d18fd99d3754cdbe3f504f4e08d560e72d4b7a75409c214ee2c3771c9a8ba7f96
060695c22a test: Failed load after migrate should restore backup (MarcoFalke)
8a4cfddf23 wallet: Set migrated wallet name only on success (Ava Chow)
Pull request description:
After a wallet is migrated and we are trying to load it, if it could not be loaded, don't try to set the wallet name. Otherwise we have a segfault.
This can be tested by migrated a legacy wallet from another network (e.g. trying to migrate a testnet wallet on mainnet). The fixed behavior is return an error and restore the backup.
ACKs for top commit:
davidgumberg:
ACK 060695c22a
furszy:
ACK 060695c22a
rkrux:
ACK 060695c22a
w0xlt:
reACK 060695c22a
pablomartin4btc:
ACK 060695c22a
Tree-SHA512: f4289e0b3dedef0a3d734c18604f2fd0df0dc65e9641bc342cfa45088d2540a3f6631bbea8bdd394b2631fa83b38e8ac37c83cfc4b53b19dcbd0b820a9beb6e4
b59dc21847 doc: Fix typos in asmap README (nervana21)
Pull request description:
This minor PR fixes some spelling mistakes found while reviewing #33026.
ACKs for top commit:
fanquake:
ACK b59dc21847
Tree-SHA512: e76f7f97c10f3e506d024da0cbf804f4975cf07f31f0dd0abad6fcb97a5fa1032087459dba46de3715f6275d47e2fde4d8db3d38341540110d87fd5669855359
