When loading an older wallet without the last hardened cache, an
automatic upgrade should be performed. Check this in
wallet_backwards_compatibility.py
When migrating a wallet, the migrated wallet should always have the last
hardened cache, so verify in wallet_migration.py
5888b4a2a5566c64141b78a0e7660a166ec99775 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 5888b4a2a5566c64141b78a0e7660a166ec99775
brunoerg:
reACK 5888b4a2a5566c64141b78a0e7660a166ec99775
jonatack:
ACK 5888b4a2a5566c64141b78a0e7660a166ec99775
Tree-SHA512: 96e51eda30a1f31cfd82ae3296ca97c9236599b18e19086dbde3a908f6fe66af8f2de7aa147bdb9ebccb2059c809a25ddfb0c23da57e1a84a35b62ca0a44e3c3
76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77 test: Migration of a wallet ending in `../` (David Gumberg)
f0bb3d50fef08d9f981eac45841ef2df7444031b test: Migration of a wallet ending in `/` (David Gumberg)
41faef5f80d69ed984af685bd9d2a43268009fb6 test: Migration fail recovery w/ `../` in path (David Gumberg)
63c6d364376907c10b9baa3c6f4d72e3f1881abc test: Migration of a wallet with `../` in path. (David Gumberg)
70f1c99c901de64d6ccea793b7e267e20dfa49cf wallet: Fix migration of wallets with pathnames. (David Gumberg)
f6ee59b6e2995a3916fb4f0d4cbe15ece2054494 wallet: migration: Make backup in walletdir (David Gumberg)
e22c3599c6772730e72e17fc68c99feea09b4d29 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 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77
achow101:
ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77
ryanofsky:
Code review ACK 76fe0e59ec4a5b0c5b18f46bfcbbf99628d74d77. 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
fa45ccc15dfc52e798da62548dc43d1bd7889c9a 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 fa45ccc15dfc52e798da62548dc43d1bd7889c9a
achow101:
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
pablomartin4btc:
ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
janb84:
re ACK fa45ccc15dfc52e798da62548dc43d1bd7889c9a
Tree-SHA512: e753cc3afbd66a88099ff62c2591aa31d32d784098e433e392c20a8dfd40d5c85807e955b264a287c3778d68605cd7022278886a43cd1635c080d563c88fc0cc
aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f test: test sendall and send do anti-fee-sniping (ishaanam)
20802c7b65f4b196a3ed0aca876974153db55d9d 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 aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
murchandamus:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
glozow:
ACK aac0b6dd79b0db1e9d42a6f466709a61cfd1f69f
Tree-SHA512: d4f1b43b5bda489bdba46b0af60e50bff0de604a35670e6ea6e1de2b539f16b3f68805492f51d6d2078d421b63432ca22a561a5721d1a37686f2e48284e1e646
1bed0f734b3f2dd876193b5cad303bfab1d250d5 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 1bed0f734b3f2dd876193b5cad303bfab1d250d5
Tree-SHA512: ad3de8ab06e7f4ffcee5c02e8185b20879d63a02a614a706ea54da5087cca4ba75817ca1aa95301572c34723317fcc44e4478261ac73dd223ee9fa827e6b35b3
3b23f95e3463bf0ab5e293654bb362d07330ff9a ci: limit max stack size to 512 KiB (dergoegge)
2931a874776073e7967bc5081306935b8abf3fd6 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 3b23f95e3463bf0ab5e293654bb362d07330ff9a
Tree-SHA512: 7e00626f3ca9e860d79a301af2427008ce27c329b618e24f95e7a55b284459a446216d2859c2e63be50abb9d4f0d343c12ff50445231652d354f225477928a35
6757052fc439bedd1fa88ee1d23b4f17cf2c4f7a 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 6757052fc439bedd1fa88ee1d23b4f17cf2c4f7a
stickies-v:
ACK 6757052fc439bedd1fa88ee1d23b4f17cf2c4f7a
janb84:
ACK 6757052fc439bedd1fa88ee1d23b4f17cf2c4f7a
Tree-SHA512: 8e6dc1e432c067f862560776176112d5c24c4009bdf8e9a4e8d1ea3328b88732188fc4d8a7cd29f8a9ed8a1809a7a8a86d63b7ae3ec4ebae74be466727c8d730
62ed1f92efff42bc79c50935e6dbd9da4e072020 txgraph: check that DoWork finds optimal if given high budget (tests) (Pieter Wuille)
f3c2fc867fc4332dfed0a3766997433e1676dbe3 txgraph: add work limit to DoWork(), try optimal (feature) (Pieter Wuille)
e96b00d99ebe27eeadba88841db32b2b8e741433 txgraph: make number of acceptable iterations configurable (feature) (Pieter Wuille)
cfe9958852be0e0763c924bdbadc37e784f5aee5 txgraph: track amount of work done in linearization (preparation) (Pieter Wuille)
6ba316eaa0321faf94eb31769deb18781ff9667c txgraph: 1-or-2-tx split-off clusters are optimal (optimization) (Pieter Wuille)
fad0eb091e58f345b922a49335c60bbeae6d5c6f 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 62ed1f92efff42bc79c50935e6dbd9da4e072020
ismaelsadeeq:
Code review ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
glozow:
ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
Tree-SHA512: 5f57d4052e369f3444e72e724f04c02004e0f66e365faa59c9f145323e606508380fc97bb038b68783a62ae9c10757f1b628b3b00b2ce9a46161fca2d4336d73
0ce041ea88dbea2bdfaf21fd0e60a86012498f0a 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 0ce041ea88dbea2bdfaf21fd0e60a86012498f0a
Tree-SHA512: cb50748fa2cd38be4b0abed36723917c2c82a92f588928bb0650eed0049c121df89b33d53421037b12836a497f30b449fe3d041ff7755a1fd9da43544392cf40
b6d4688f77df9e31fd64e2be300f55bb8e944bd0 [doc] reword comments in test_mid_package_replacement (glozow)
f3a613aa5bb780f5d85821fa00742f5a99df81b5 [cleanup] delete brittle test_mid_package_eviction (glozow)
c3cd7fcb2cd9f9911f8251a6f29120f65c63ea37 [doc] remove references to now-nonexistent Finalize() function (glozow)
d8140f5f050076bfa129169c1223cab94b243a6e don't make a copy of m_non_base_coins (glozow)
98ba2b1db2eb81e08b550ba0d5069a9289f30e13 [doc] MemPoolAccept coins views (glozow)
ba02c30b8a635db88100916a1bb9c40fb0b47ab6 [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 b6d4688f77df9e31fd64e2be300f55bb8e944bd0
marcofleon:
ACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0
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.
1c10b7351e194fc788766347f65f4512f61f05e8 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 1c10b7351e194fc788766347f65f4512f61f05e8
maflcko:
lgtm ACK 1c10b7351e194fc788766347f65f4512f61f05e8
theStack:
ACK 1c10b7351e194fc788766347f65f4512f61f05e8
Tree-SHA512: 1750d7d12de511f0ac34922ea9c58c4b9b55c3aaf22109abfd7dbe01ad1eb7b48fb4a6b074a0baf0e55ee2270fcc969b6830e499ff33adbcd0b9c761fb25e563
251d02084688c67523e9ec92ec79ee657454ab93 init, wallet: replace hardcoded output types with `FormatAllOutputTypes` (Sebastian Falbesoner)
e3ba0757a9410336e904a1b108d86165f347fc52 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 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640) 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 251d02084688c67523e9ec92ec79ee657454ab93 🌨
pablomartin4btc:
re-utACK 251d02084688c67523e9ec92ec79ee657454ab93
rkrux:
crACK 251d02084688c67523e9ec92ec79ee657454ab93
Tree-SHA512: 23d1025d068f3a44b115a34b217b808fcae59fc574e35a899f0d43a85512935c90675d2e98c621287e02adc3a9f4a08289a26596c66e2122262af0cd2dfbde72
8aed477c3322212a636ab69d4923f89e2d9a63a2 test: fix RPC coverage check (Brandon Odiwuor)
2630b64f8100b9a6652c99d5466b73a666d8cfca 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 8aed477c3322212a636ab69d4923f89e2d9a63a2
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.
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
faa1c3e80d95552bdc2c0e717065ebf8d510138f 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 faa1c3e80d95552bdc2c0e717065ebf8d510138f
darosior:
ACK faa1c3e80d95552bdc2c0e717065ebf8d510138f
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.
c5c1960f9350d6315cadbdc95fface5f85f25806 doc: Add release notes for changes in RPCs (pablomartin4btc)
90fd5acbe57edb219a5dcbdc1095e11ae5398da5 rpc, test: Fix error message in getdescriptoractivity (pablomartin4btc)
39fef1d203678291020aa1adb2e420a117f86169 test: Add missing logging info for each test (pablomartin4btc)
53ac704efd668f7d4ad74158e628023c9a34141f rpc, test: Fix error message in unloadwallet (pablomartin4btc)
1fc3a8e8e7ae4698ac5cd5292a7e7e37097d37ce rpc, test: Add EnsureUniqueWalletName tests (pablomartin4btc)
b635bc0896294af5afa1b18a35f307dfae441bb8 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 c5c1960f9350d6315cadbdc95fface5f85f25806
stickies-v:
re-ACK c5c1960f9350d6315cadbdc95fface5f85f25806
furszy:
ACK c5c1960f9350d6315cadbdc95fface5f85f25806
Tree-SHA512: e831ff1acbfd15d2ce3a69bb408cce94664c0b63b2aa2f4627a05c6c052241ae3b5cc238219ef1b30afb489a4a3f4c3030e2168b0c8f08b4d20805d050d810f5
This commit takes use of the `FormatAllOutputTypes` helper
(introduced in PR #32432, commit 8cc9845b8ddf4f93a02c622e7df8d1095dc1a640)
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.
1cb23997033c395d9ecd7bf2f54787b134485f41 doc: clarify the GetAddresses/GetAddressesUnsafe documentation (Daniela Brozzoni)
e5a7dfd79f618b04e0140ec2c50c6e95c2a2e2e4 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 (f26502e9fc8a669b30717525597e3f468eaecf79, 81b00f87800f40cb14f2131ff27668bd2bb9e551) added parameters to each
function, and the previous commit changed the function naming completely.
ACKs for top commit:
stickies-v:
re-ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
l0rinc:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
luisschwab:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
brunoerg:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
theStack:
Code-review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
mzumsande:
Code review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
Tree-SHA512: 02c05d88436abcdfabad994f47ec5144e9ba47668667a2c1818f57bf8710727505faf8426fd0672c63de14fcf20b96f17cea2acc39fe3c1f56abbc2b1a9e9c23
fa1fd074685ca96b9bd3855e9e6fe730a4f6462c 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 fa1fd074685ca96b9bd3855e9e6fe730a4f6462c.
Tree-SHA512: 6412dd3f8d702bca7762a8f1be3f9d2782132936fcc7ae5c31690b594e04f69708110e6f6233d5a61901289d13c7089ab5646a2c3ef2266fffc36d0543f4b7ae
face8123fdc10549676c6679ee3225c178a7f30c log: [refactor] Use info level for init logs (MarcoFalke)
fa183761cb09d916ed2a3bbab71b80c5c7942a30 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 face8123fdc10549676c6679ee3225c178a7f30c
fanquake:
ACK face8123fdc10549676c6679ee3225c178a7f30c
Tree-SHA512: 28c296129c9a31dff04f529c53db75057eae8a73fc7419e2f3068963dbb7b7fb9a457b2653f9120361fdb06ac97d1ee2be815c09ac659780dff01d7cd29f8480
fa1a14a13a15ecfb7587a94ee86b4ace7c819519 fuzz: Reset chainman state in process_message(s) targets (MarcoFalke)
fa9a3de09b4c6eef40f1073f09c9a0bd1858adf2 fuzz: DisableNextWrite (MarcoFalke)
aeeeeec9f749dddeaf8eaa357b69cd45ed3dd76c fuzz: Reset dirty connman state in process_message(s) targets (MarcoFalke)
fa11eea4059a608f591db4469c07a341fd33a031 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 fa1a14a13a15ecfb7587a94ee86b4ace7c819519
dergoegge:
reACK fa1a14a13a15ecfb7587a94ee86b4ace7c819519
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.
6135e0553e6e58fcf506700991fa178f2c50a266 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 6135e0553e6e58fcf506700991fa178f2c50a266
w0xlt:
ACK 6135e0553e
Tree-SHA512: 0e2367fc4d50c62ec41443374b64c4c5ecf679998677df47fb8776cfb44704713bc45547e32e96cd30d1dbed766f5d333efb6f10eb0e71271606638e07e61a01
faa3171ff22fea1c001e5a9b01d964aa425a3387 ci: Use optimized Debug build type in test-each-commit (MarcoFalke)
fa21c3401e15db578a2acee033ec036ce0f3b74a 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 faa3171ff22fea1c001e5a9b01d964aa425a3387
Tree-SHA512: ca041cf7f79d7abb6f93e17b58b2aea730f3bb9fc51256c1ca1b9f7ce7e7188d18fd99d3754cdbe3f504f4e08d560e72d4b7a75409c214ee2c3771c9a8ba7f96
060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542 test: Failed load after migrate should restore backup (MarcoFalke)
8a4cfddf23a4575a1042dfa97d3478727775e8dd 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 060695c22ae7b2b0f2a1d
furszy:
ACK 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542
rkrux:
ACK 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542
w0xlt:
reACK 060695c22a
pablomartin4btc:
ACK 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542
Tree-SHA512: f4289e0b3dedef0a3d734c18604f2fd0df0dc65e9641bc342cfa45088d2540a3f6631bbea8bdd394b2631fa83b38e8ac37c83cfc4b53b19dcbd0b820a9beb6e4
b59dc21847d3bb20c0d77af5b4ca0ae5d8e56221 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 b59dc21847d3bb20c0d77af5b4ca0ae5d8e56221
Tree-SHA512: e76f7f97c10f3e506d024da0cbf804f4975cf07f31f0dd0abad6fcb97a5fa1032087459dba46de3715f6275d47e2fde4d8db3d38341540110d87fd5669855359
