The new helper will be used to fix a crash in the
wallet migration process (watch-only, non-blank,
private keys disabled, empty wallet - no scripts
or addresses imported).
Co-authored-by: Matias Furszyfer <mfurszy@protonmail.com>
28dc118001b061aed42880ff418fde7cc5f6255d fuzz: wallet: fix crypter target (brunoerg)
Pull request description:
The crypter target has an issue, it's calling `DecryptKey` with a random secret and a random public key that will unlikely be related to the key used to encrypt, so it won't have any effect. This PR changes fixes it and also removes the `DecryptSecret` call since this function is already (and only) called within `DecryptKey`.
ACKs for top commit:
maflcko:
lgtm ACK 28dc118001b061aed42880ff418fde7cc5f6255d 🥊
Tree-SHA512: e96b7d33879bf06eeec0726e74e8e0d7020997659bf97dfca5d7c1a7ba65c4d93c78e666b97eebde110564cef2eefc7209d3e3586e4658145827b14d1b01dfc9
0ff66b1c4ab0e5cba00a178b24f2c5de75df360f fuzz: coinselection: cover `SetBumpFeeDiscount` (brunoerg)
Pull request description:
`SetBumpFeeDiscount` sets the bump fee discount which is used to calculate the waste. We currently have no fuzz coverage for this function, so this PR adds it by calling `SetBumpFeeDiscount` before `RecalculateWaste`.
ACKs for top commit:
marcofleon:
ACK 0ff66b1c4ab0e5cba00a178b24f2c5de75df360f
Tree-SHA512: d5c1d97daaeb7f9b096bf9bdf6374b8a674a75f464e2b9bb3e1e1774a5805b22840ca1f31bae63f106640d9ce27a99432c3034524340be91c235f6ec3b185cff
It was highlighted in a PR discussion previously that the recently
moved `Assert` macro call inside the block disconnected loop had
been redundant for quite a while because of the presence of the
`assert` macro call at the start of the function. Therefore, it
is removed now.
refs #https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995416821
Checking for IsArgSet before calling GetArg while providing an arbitrary
default value as fallback is both confusing and fragile.
It is confusing, because the provided fallback is dead code. So it would
be better to just call GetArg without a fallback.
Even better would be to provide the true fallback value and sanitize it
as if it were user-input, but this can be done in a follow-up.
Removing the redundant call to IsArgSet will have to be done either way,
so do it now.
a015b7e13daacdfb6db0eada50563dec70c5afb2 test: Add expected result assertions (yancy)
Pull request description:
~This is a trivial addition to the test suit, however it shouldn't be required to add debug statements and manually run the tests if someone needs to know the results of this test.~
Add an assertion for the values returned. The goal of the test is to show that a minimal weight selection of UTXOs is returned by coin-grinder. Since there are multiple possible solutions, the added assertion shows that coin-grinder finds the solution with the lowest weight. Without this assertion, it's ambiguous whether or not coin-grinder is returning the solution with the lowest weight.
Remove the check that a result is returned since the expected result assertion implies a result.
ACKs for top commit:
janb84:
re ACK [a015b7e](a015b7e13d)
murchandamus:
ACK a015b7e13daacdfb6db0eada50563dec70c5afb2
Tree-SHA512: ee3c2688b4a4a07ab209f7655c3956e62a1084419df5e87c27d751a38ff64d4c3457df2317f8077149a6947cdb05b249975de2b8f0e18ca8b17b41f4735fb1c6
2f2ab47bf74f4da37aad75a186cb0bb16e8af579 Release notes (Pol Espinasa)
bf194c920cf768d1339d41aef1441a78e2f5fcbe wallet, rpc: deprecate settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR deprecates the settxfee RPC and paytxfee setting, marking it for removal in Bitcoin Core 31.0.
**Motivation**
The PR was initially motivated by https://github.com/bitcoin/bitcoin/issues/31088. The intention was to create a new function `settxfeerate` to allow users to set a static fee rate in `sat/vB` instead of `btc/kvB`.
The `settxfee` RPC allows users to set a static fee rate for all transactions created by the wallet. However, in a dynamic fee environment, this can lead to poor fee choices, either overpaying when the mempool is empty or underpaying when congestion is high. The preferred approach is to rely on fee estimation, which is designed to adapt to network conditions, and is the one by default. Same argument apply for `paytxfee` setting.
During discussion the consensus was that static fee settings are a footgun and that users should instead specify the fee rate per transaction if they don't want to rely on the fee estimation. Given this, rather than introducing a `settxfeerate` alternative, this PR goes towards removing `settxfee` and `paytxfee` entirely.
**Key Changes**
`settxfee` and `paytxfee` is now deprecated and will be removed in Bitcoin Core 31.0.
Users should rely on fee estimation or explicitly specify a fee rate when constructing transactions.
**Impact on Users**
If users currently use settxfee or paytxfee, they should transition to specifying fees per transaction.
No immediate breakage in 30.0 (must use `-deprecatedrpc=settxfee`), but `settxfee` and `paytxfee` will be removed in 31.0.
**Alternative Approaches Considered**
A settxfeerate alternative (using sat/vB) was initially proposed but ultimately rejected in favor of deprecating static fee setting entirely.
**Notes for removal**
- When removing paytxfee we should also update txconfirmtarget startup option help text.
- Get back the comment from `rpc_deprecated.py` test. [+info](https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1998876768)
ACKs for top commit:
fjahr:
re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
ismaelsadeeq:
re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
rkrux:
Concept and utACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
Tree-SHA512: 0272812cbe5a519737c5d0683acc2072e67559792b4a6472bca8b23426e5ce9e88a3a1eba987feda70a082b8b474b3126893848628d7bf11e1520357b18c8d3e
ba82240553ddd534287845e10bc76b46b45329fe fuzz: split `coinselection` harness (brunoerg)
Pull request description:
This PR splits the `coinselection` fuzz harness into 3 targets (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`). The goal is to be able to fuzz each algorithm separately (to avoid performance issues) and also all of them together.
ACKs for top commit:
janb84:
Tested ACK [ba82240](ba82240553)
maflcko:
review ACK ba82240553ddd534287845e10bc76b46b45329fe 👐
marcofleon:
reACK ba82240553ddd534287845e10bc76b46b45329fe
zaidmstrr:
reACK [ba82240](ba82240553)
Tree-SHA512: 277cffd524e57d286dbbbcb2aa0a9f1d720b4c56331dfb0f4425e1666246330616508e47977da23f28a72705aa142bbaf536e2cf7fe4703a2cd2e4b2fd441d9d
ffff4a293ad878494e12f8f00108cc99ee2b713e bench: Update span-serialize comment (MarcoFalke)
fa4d6ec97bcb1790a7cd4363a13fda7c80c3dd90 refactor: Avoid false-positive gcc warning (MarcoFalke)
fa942332b40c97375af0722f32f7575bca3af819 scripted-diff: Bump copyright headers after std::span changes (MarcoFalke)
fa0c6b7179c062b7ca92d120455ce02a9f4e9e19 refactor: Remove unused Span alias (MarcoFalke)
fade0b5e5e6e80e3da1ab6448b6212244bafa5d3 scripted-diff: Use std::span over Span (MarcoFalke)
fadccc26c03db00a2be3f703aa7e5eec4312bd2e refactor: Make Span an alias of std::span (MarcoFalke)
fa27e36717ec18d64b7ff7bba71b8f0c202ba31d test: Fix broken span_tests (MarcoFalke)
fadf02ef8bf96ad5b3b8e34fd425b31b555f4371 refactor: Return std::span from MakeUCharSpan (MarcoFalke)
fa720b94be17fa9e7c91188710e6a04939ceab11 refactor: Return std::span from MakeByteSpan (MarcoFalke)
Pull request description:
`Span` has some issues:
* It does not support fixed-size spans, which are available through `std::span`.
* It is confusing to have it available and in use at the same time with `std::span`.
* It does not obey the standard library iterator build hardening flags. See https://github.com/bitcoin/bitcoin/issues/31272 for a discussion. For example, this allows to catch issues like the one fixed in commit fabeca3458b38a3d8930cb0cbc866388c3f120f1.
Both types are type-safe and can even implicitly convert into each other in most contexts.
However, exclusively using `std::span` seems less confusing, so do it here with a scripted-diff.
ACKs for top commit:
l0rinc:
reACK ffff4a293ad878494e12f8f00108cc99ee2b713e
theuni:
ACK ffff4a293ad878494e12f8f00108cc99ee2b713e.
Tree-SHA512: 9cc2f1f43551e2c07cc09f38b1f27d11e57e9e9bc0c6138c8fddd0cef54b91acd8b14711205ff949be874294a121910d0aceffe0e8914c4cff07f1e0e87ad5b8
21e9d39a3725cd6107b742f0cb97f65b3640201b docs: add release notes for 31603 (brunoerg)
a8b548d75d9a376c9bb66e06bb918c876416d615 test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys (brunoerg)
c7afca3d62cf5d3ea9b98d5a76e4e54cac07bc3c test: descriptor: check whitespace into keys (brunoerg)
cb722a3cea16a04844c83e56fd6deaa1f0dc0a7e descriptor: check whitespace in ParsePubkeyInner (brunoerg)
50856695ef6c02ecbaa0cf448567355b6b86b510 test: fix descriptors in `ismine_tests` (brunoerg)
Pull request description:
Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment (e.g. `pk( KEY)`, `pk(KEY )` or `pk( KEY )`). I have noticed that one of the reasons is that the `DecodeBase58` function simply ignore these whitespaces.
This PR changes the `ParsePubkeyInner ` to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. `importdescriptors`), but an already imported descriptor won't be affected by this check, especially because we store descriptors from `ToString`.
For context: https://github.com/brunoerg/bitcoinfuzz/issues/72
ACKs for top commit:
rkrux:
tACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
darosior:
re-ACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
sipa:
utACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
Tree-SHA512: 54f48a89a235517e5cdc29a46dceeb7dabbee93c7616a166288ff3f90131808eb0ece43b0797a11fe827a5f7bd51d65e3e75c16789b0a42020934cabb684cc8f
4cd95a2921805f447a8bcecc6b448a365171eb93 refactor: modernize remaining outdated trait patterns (Lőrinc)
ab2b67fce20fd7d8017f8a26425cab99e91f420d scripted-diff: modernize outdated trait patterns - values (Lőrinc)
8327889f358289f918d04ddb9469fb5562720bf4 scripted-diff: modernize outdated trait patterns - types (Lőrinc)
Pull request description:
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-one (noticed them while investigating https://github.com/bitcoin/bitcoin/pull/31868), and afterwards extracted scripted diff commits to do the trivial ones automatically.
The last commit contains the values that were easier done manually.
I've excluded changes from `src/bench/nanobench.h`, `src/leveldb`, `src/minisketch`, `src/span.h` and `src/sync.h` - let me know if you think they should be included instead.
A few of the code changes can also be reproduced by clang-tidy (but not all of them):
```bash
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)
run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,modernize-type-traits' -fix $(git grep -lE '::(value|type)' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h')
```
ACKs for top commit:
laanwj:
Concept and code review ACK 4cd95a2921805f447a8bcecc6b448a365171eb93
Tree-SHA512: a4bcf0f267c0f4e02983b4d548ed6f58d464ec379ac5cd1f998b9ec0cf698b53a9f2557a05a342b661f1d94adefc9a0ce2dc8f764d49453aaea95451e2c4c581
36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8 build: require sqlite when building the wallet (Sjors Provoost)
Pull request description:
Require that sqlite is available in order to compile the wallet. Removes instances of `USE_SQLITE` since it is no longer possible to not have sqlite available.
The `NO_SQLITE` option is dropped from depends.
This is another step towards dropping the legacy wallet, extracted from #31250.
ACKs for top commit:
m3dwards:
ACK 36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8
davidgumberg:
crACK 36b6f36ac4
hebasto:
re-ACK 36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8.
Tree-SHA512: 870a0135671c80c4f28602119eb8637a1ed43b51b1673bfe88425782fb62ec6ef0f3d6baf0d5984d6a243779b0f63423fd4c4dc324ef87bffba13d63e05ad793
11f8ab140fe63857f6a93b81021efda8f90ceeda test: wallet, coverage for crash on dup block disconnection during unclean shutdown (Martin Zumsande)
9ef429b6ae65f6ad3e9ac11c2d9c0a6c52beb865 wallet: fix crash on double block disconnection (furszy)
Pull request description:
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because `disconnectBlock` provides `TxStateInactive` without the "abandoned"
flag for coinbase transactions to `SyncTransaction`, while `AddToWallet()` internally modifies
it to retain the abandoned state.
The crash flow is as follows:
1) On the first disconnection, the transaction state transitions from "confirmed" to
"inactive," bypassing the state equality check since the provided state differs. Then,
`AddToWallet` internally updates the state to "inactive + abandoned"
2) On the second disconnection, as we provide only the "inactive" state
to `SyncTransaction()`, the state equality assertion fails and crashes the wallet.
Reviewers Note:
The crash can easily be replicated by cherry-picking the test commit in master.
ACKs for top commit:
mzumsande:
Code Review ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
rkrux:
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
pinheadmz:
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
Tree-SHA512: 971069bca562f0afb06c34a2516842d01b5cbc2b18ed851392aa3caa3bb7488f4a84a5d017ea334e6361261d3c44aa597cc67a1d4fa16781f85e081f3d1f8771
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
Require that sqlite is available in order to compile the wallet. Removes
instances of USE_SQLITE since it is no longer possible to not have
sqlite available.
The NO_SQLITE option is dropped from depends.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Transifex interprets the "-0" substring as a number in translatable
strings. Since not all translations preserve "-0," this triggers a
corresponding warning. While this warning could be disabled globally, it
is more reasonable to adjust the original string instead.
This test returns the lowest weight solution. Other possibilities
either exceed allowed weight or result in a higher weight. Add an
assertion which shows that the lowest weight solution is found and
update the test description accordingly.
Remove the check that a result is returned since the expected result
assertion implies a result.
The use of e.g. `std::underlying_type_t<T>` replaces the older `typename std::underlying_type<T>::type`.
The `_t` helper alias template (such as `std::underlying_type_t<T>`) introduced in C++14 offers a cleaner and more concise way to extract the type directly.
See https://en.cppreference.com/w/cpp/types/underlying_type for details.
-BEGIN VERIFY SCRIPT-
sed -i -E 's/(typename )?(std::[a-z_]+)(<[^<>]+>)::type\b/\2_t\3/g' $(git grep -l '::type' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h')
-END VERIFY SCRIPT-
ca6aa0b9bee3fdf355b7154a9a686a80977f2a02 doc: loadwallet loads from relative walletdir (am-sq)
Pull request description:
## Why this change?
https://github.com/bitcoin/bitcoin/issues/30269 describes a need for documentation improvement with the `loadwallet` RPC. Namely, [some users have found](https://bitcoin.stackexchange.com/questions/123331/how-do-you-load-a-regtest-wallet) the usage description confusing when it comes to loading wallets that are not in the normal case of being in the default wallet directory.
The default wallet directory, depending on the machine OS, has the base directory defined here: 9c5cdf07f3/src/common/args.cpp (L699) which is then appended with `/wallets`. So for example, for MacOS, it would be `~/Library/Application Support/Bitcoin/wallets`.
## The changes implemented
1. Change the help text to indicate that the filename (or directory) passed in to `loadwallet` is relative to the base wallet directory
2. Adds additional examples to the help page showing how to fetch a wallet within a subdirectory of the base data directory for wallets, or from an absolute path
ACKs for top commit:
achow101:
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
maflcko:
lgtm ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
rkrux:
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
jonatack:
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
Tree-SHA512: 123ae118c79ee1843ed65861e7a008658a53e47d4d14f2b7612561bba1b1dbdb6744f9aaac1587aac231c62d0c1804de848a6d732f1382788b437d9fe6474012
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because 'disconnectBlock' provides 'TxStateInactive' without the "abandoned"
flag for coinbase transactions to 'SyncTransaction', while 'AddToWallet()' internally
modifies it to retain the abandoned state.
The flow is as follows:
1) On the first disconnection, the transaction state transitions from "confirmed" to
"inactive," bypassing the state equality check since the provided state differs. Then,
'AddToWallet' internally updates the state to "inactive + abandoned"
2) On the second disconnection, as we provide only the "inactive" state
to 'SyncTransaction()', the state equality assertion fails and crashes the wallet.
e4dd5a351bde88a94326824945f4c8b1e4c15df2 test: wallet, abandon coinbase txs and their descendants during startup (furszy)
474139aa9bf7109df78e46936e5a649c70703386 wallet: abandon inactive coinbase tx and their descendants during startup (furszy)
Pull request description:
Since #26499, we mark coinbase transactions and their descendants as abandoned when a reorg arises through the "block disconnection" signal handler. However, this does not cover all scenarios; external wallets could contain coinbase transactions from blocks the node has not seen yet, or the user could have replaced the chain with an earlier or different version (one without the coinbase chain).
This affects balance calculation as well as mempool rebroadcast (descendants shouldn't be relayed).
Fix this by marking orphaned coinbase transactions and their descendants as abandoned during wallet startup.
ACKs for top commit:
achow101:
ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
rkrux:
tACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
mzumsande:
Code Review ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
Tree-SHA512: 461a43de7a6f5a580f2e6e3b56ec9bc92239cd45e850a2ff594ab5488dcd4a507f68fbbf550a33d7173b2add0de80de1e1b3841e1dfab0c95b284212d8ced08a
Improves the documentation of help output for loadwallet
to clarify that filename is relative to the default
wallet directory. Adds examples that get a wallet from
sub-directories.
4818da809f0e300016390dd41e02c6067ffa008f wallet: fix rescanning inconsistency (Martin Zumsande)
Pull request description:
If the chain advances during a rescan, ScanForWalletTransactions would previously process the new blocks without adjusting `m_last_processed_block`, which would leave the wallet in an inconsistent state temporarily, and could lead to crashes in the GUI reported in #31474.
Fix this by not rescanning blocks beyond `m_last_processed_block` - for all blocks beyond that height, there will be pending BlockConnected notifications that will process them after the rescan is finished.
This means that if rescanning was triggered with `cs_wallet` permanently held (`AttachChain`), additional blocks that were connected during the rescan will only be processed with the pending `blockConnected` notifications after the lock is released.
If rescanning without a permanent `cs_wallet` lock (`RescanFromTime`), additional blocks that were connected during the rescan can be re-processed here because `m_last_processed_block` was already updated by `blockConnected`.
Fixes#31474
ACKs for top commit:
psgreco:
Not that it matters much, but UTACK 4818da809f0e300016390dd41e02c6067ffa008f
achow101:
ACK 4818da809f0e300016390dd41e02c6067ffa008f
furszy:
utACK 4818da809f0e300016390dd41e02c6067ffa008f
Tree-SHA512: 8e7dbc9e00019aef4f80a11776f3089cd671e0eadd3c548cc6267b5c722433f80339a9b2b338ff9b611863de75ed0a817a845e1668e729b71af70c9038b075af
fa8e0956c23789fb819ad1b31377711df091ec1b rpc: Remove deprecated dummy alias for listtransactions::label (MarcoFalke)
Pull request description:
The RPC arg is not a dummy, but a label, so offering an undocumented alias is inconsistent with all other label interfaces and confusing at best, if not entirely unused.
Fix it by removing the deprecated alias.
This pull is a breaking change, but it should be limited, because it only affects someone using the deprecated named arg on this RPC. I can't imagine anyone doing this, because in all other places where label args are accepted, they are called `label`. If someone really didn't use `label` here as named arg, it would be trivial and less confusing for them to fix it up.
ACKs for top commit:
achow101:
ACK fa8e0956c23789fb819ad1b31377711df091ec1b
rkrux:
tACK fa8e0956c23789fb819ad1b31377711df091ec1b
ryanofsky:
Code review ACK fa8e0956c23789fb819ad1b31377711df091ec1b
Tree-SHA512: 0d0f3f53237ff9fac8c065b7d0a4245f5ff86efa427dbeeca711765494b7315a9d72b44751d346c76422847daf3d7ff90dbccb5ba200b089fb96128bd95da9f0
af76664b12d8611b606a7e755a103a20542ee539 test: Test migration of a solvable script with no privkeys (Ava Chow)
17f01b0795e1dfa264fb42de65339bd18abb7f97 test: Test migration of taproot output scripts (Ava Chow)
1eb9a2a39fdb20262a5afe21d8feada9bdcbbd18 test: Test migration of miniscript in legacy wallets (Ava Chow)
e8c3efc7d8f06210dfef7c1a47bad6315714f4cd wallet migration: Determine Solvables with CanProvide (Ava Chow)
fa1b7cd6e2cfd581679d1fb6bc0fabcd6ee6e70a migration: Skip descriptors which do not parse (Ava Chow)
440ea1ab6393638a49a2ba6daefe0b29778bea7e legacy spkm: use IsMine() to extract watched output scripts (Ava Chow)
b777e84cd70838fee5e3624a182632e04cc1d24c legacy spkm: Move CanProvide to LegacyDataSPKM (Ava Chow)
b1ab927bbf2a620ad984910c1d8317ec9bb33297 tests: Test migration of additional P2WSH scripts (Ava Chow)
c39b3cfcd1bc5002aa06d1b79c948ce94f3ec8dc test: Extra verification that migratewallet migrates (Ava Chow)
Pull request description:
The legacy wallet `IsMine()` is essentially a black box that would tell us whether the wallet is watching an output script. In order to migrate legacy wallets to descriptor wallets, we need to be able to compute all of the output scripts that a legacy wallet would watch. The original approach for this was to understand `IsMine()` and write a function which would be its inverse. This was partially done in the original migration code, and attempted to be completed in #30328. However, further analysis of `IsMine()` has continued to reveal additional edge cases which make writing an inverse function increasingly difficult to verify correctness.
This PR instead changes migration to utilize `IsMine()` to produce the output scripts by first computing a superset of all of the output scripts that `IsMine()` would watch and testing each script against `IsMine()` to filter for the ones that actually are watched. The superset is constructed by computing all possible output scripts for the keys and scripts in the wallet - for keys, every key could be a P2PK, P2PKH, P2WPKH, and P2SH-P2WPKH; for scripts, every script could be an output script, the redeemScript of a P2SH, the witnessScript of a P2WSH, and the witnessScript of a P2SH-P2WSH.
Additionally, the legacy wallet can contain scripts that are redeemScripts and witnessScripts, while not watching for any output script utilizing that script. These are known as solvable scripts and are migrated to a separate "solvables" wallet. The previous approach to identifying these solvables was similar to identifying output scripts - finding known solvable conditions and computing the scripts. However, this also can miss scripts, so the solvables are now identified in a manner similar to the output scripts but using the function `CanProvide()`. Using the same superset as before, all output scripts which are `ISMINE_NO` are put through `CanProvide()` which will perform a dummy signing and then a key lookup to determine whether the legacy wallet could provide any solving data for the output script. The scripts that pass will have their descriptors inferred and the script included in the solvables wallet.
The main downside of this approach is that `IsMine()` and `CanProvide()` can no longer be deleted. They will need to be refactored to be migration only code instead in #28710.
Lastly, I've added 2 test cases for the edge cases that prompted this change of approach. In particular, miniscript witnessScripts and `rawtr()` output scripts are solvable and signable in a legacy wallet, although never `ISMINE_SPENDABLE`.
ACKs for top commit:
sipa:
Code review ACK af76664b12d8611b606a7e755a103a20542ee539; I did not review the tests in detail.
brunoerg:
code review ACK af76664b12d8611b606a7e755a103a20542ee539
rkrux:
ACK af76664b12d8611b606a7e755a103a20542ee539
Tree-SHA512: 7f58a90de6f38fe9801fb6c2a520627072c8d66358652ad0872ff59deb678a82664b99babcfd874288bebcb1487d099a77821f03ae063c2b4cbf2d316e77d141
LegacySPKM would determine whether it could provide any script data to a
transaction through the use of the CanProvide function. Instead of
partially reversing signing logic to figure out the output scripts of
solvable things, we use the same candidate set approach in
GetScriptPubKeys() and instead filter the candidate set first for
things that are ISMINE_NO, and second with CanProvide(). This should
give a more accurate solvables wallet.
InferDescriptors can sometimes make descriptors which are actually
invalid and cannot be parsed. Detect and skip such descriptors by doing
a Parse() check before adding the descriptor to the wallet.
Instead of (partially) trying to reverse IsMine() to get the output
scripts that a LegacySPKM would track, we can preserve it in migration
only code and utilize it to get an accurate set of output scripts.
This is accomplished by computing a set of output script candidates from
map(Crypted)Keys, mapScripts, and setWatchOnly. This candidate set is an
upper bound on the scripts tracked by the wallet. Then IsMine() is used
to filter to the exact output scripts that LegacySPKM would track.
By changing GetScriptPubKeys() this way, we can avoid complexities in
reversing IsMine() and get a more complete set of output scripts.
9d2d9f7ce29636f08322df70cf6abec8e0ca3727 rpc: Include assumeutxo as a failure reason of rescanblockchain (Fabian Jahr)
595edee169045b6735b76ff9721677f0e43f13e5 test, assumeutxo: import descriptors during background sync (Alfonso Roman Zubeldia)
d73ae603d44f93e4d6c5116f235dd11a0bdbf89c rpc: Improve importdescriptor RPC error messages (Fabian Jahr)
27f99b6d63b7ca2d4fcb9db3e88ed66c024c59d5 validation: Don't assume m_chain_tx_count in GuessVerificationProgress (Fabian Jahr)
42d5d5336319aaf0f07345037db78239d9e012fc interfaces: Add helper function for wallet on pruning (Fabian Jahr)
Pull request description:
A test that is added as part of #30455 uncovered this issue: The `GuessVerificationProgress` function is used during during descriptor import and relies on `m_chain_tx_count`. In #29370 an [`Assume` was added](0fd915ee6b) expecting the `m_chaint_tx_count` to be set. However, as the test uncovered, `GuessVerificationProgress` is called with background sync blocks that have `m_chaint_tx_count = 0` when they have not been downloaded and processed yet.
The simple fix is to remove the `Assume`. Users should not be thrown off by the `Internal bug detected` error. The behavior of `importdescriptor` is kept consistent with the behavior for blocks missing due to pruning.
The test by alfonsoromanz is cherry-picked here to show that the [CI errors](https://cirrus-ci.com/task/5110045812195328?logs=ci#L2535) should be fixed by this change.
This PR also improves error messages returned by the `importdescriptors` and `rescanblockchain` RPCs. The error message now changes depending on the situation of the node, i.e. if pruning is happening or an assumutxo backgroundsync is active.
ACKs for top commit:
achow101:
ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
mzumsande:
Code Review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
furszy:
Code review ACK 9d2d9f7ce29636f08322df70cf6abec8e0ca3727
Tree-SHA512: b841a9b371e5eb8eb3bfebca35645ff2fdded7a3e5e06308d46a33a51ca42cc4c258028c9958fbbb6cda9bb990e07ab8d8504dd9ec6705ef78afe0435912b365
cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e RPC: improve SFFO arg parsing, error catching and coverage (furszy)
4f4cd353194310a8562da6aae27c9c8eda8a4ff0 rpc: decouple sendtoaddress 'subtractfeefromamount' boolean parsing (furszy)
Pull request description:
Following changes were made:
1) Catch and signal error for duplicate string destinations.
2) Catch and signal error for invalid value type.
3) Catch and signal error for string destination not found in tx outputs.
4) Improved `InterpretSubtractFeeFromOutputInstructions()` code organization.
5) Added test coverage for all possible error failures.
Also, fixed two PEP 8 warnings at the 'wallet_sendmany.py' file:
- PEP 8: E302 expected 2 blank lines, found 1 at the SendmanyTest class declaration.
- PEP 8: E303 too many blank lines (2) at skip_test_if_missing_module() and set_test_params()
ACKs for top commit:
achow101:
ACK cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e
murchandamus:
crACK cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e
naiyoma:
TACK [cddcbaf81e)
ismaelsadeeq:
Code review and Tested ACK cddcbaf81e8e3d107cd321ee2c9a7fd786a8917e
Tree-SHA512: c9c15582b81101a93987458d155394ff2c9ca42864624c034ee808a31c3a7d7f55105dea98e86fce17d3c7b2c1a6b5b77942da66b287f8b8881a60cde78c1a3c
18619b473255786b80f27dd33b46eb26a63fffc7 wallet: remove BDB dependency from wallet migration benchmark (furszy)
Pull request description:
Part of the legacy wallet removal working path #20160.
Stops creating a bdb database in the wallet migration benchmark.
Instead, the benchmark now creates the db in memory and re-uses it for the migration process.
ACKs for top commit:
achow101:
ACK 18619b473255786b80f27dd33b46eb26a63fffc7
brunoerg:
code review ACK 18619b473255786b80f27dd33b46eb26a63fffc7
theStack:
Code-review ACK 18619b473255786b80f27dd33b46eb26a63fffc7
Tree-SHA512: a107deee3d2c00b980e3606be07d038ca524b98251442956d702a7996e2ac5e2901f656482018cacbac8ef6a628ac1fb03f677d1658aeaded4036d834a95d7e0
fad83e759a4a6221fb3b067657f847894e5a2f20 doc: Fix incorrect send RPC docs (MarcoFalke)
Pull request description:
It would be good to have accurate RPC docs, so that humans and machines can read them and rely on them.
This fixes one issue.
ACKs for top commit:
fjahr:
utACK fad83e759a4a6221fb3b067657f847894e5a2f20
rkrux:
tACK fad83e759a4a6221fb3b067657f847894e5a2f20
luke-jr:
tACK fad83e759a4
Tree-SHA512: 65d0cc18a62ef44833621464d74b743d24ffe2b853596dce2c4f423df0495142d50387c02ba1b54f5ca77d4ddb083d55116a8ac92698aa6558762d841664911e
f6a6d912059c66792f48689632d2a7f14f8bdad9 test: add check for getting SigningProvider for a CPubKey (Sebastian Falbesoner)
62a95f5af9b998e241eb72c16ba581e77c480126 test: refactor: move `CreateDescriptor` helper to wallet test util module (Sebastian Falbesoner)
493656763f73e5ef1cfb979a513c12983dca99dd desc spkm: Return SigningProvider only if we have the privkey (Ava Chow)
Pull request description:
If we know about a pubkey that's in our descriptor, but we don't have the private key, don't return a SigningProvider for that pubkey.
This is specifically an issue for Taproot outputs that use the H point as the resulting PSBTs may end up containing irrelevant information because the H point was detected as a pubkey each unrelated descriptor knew about.
Split from #29675
ACKs for top commit:
fjahr:
ACK f6a6d912059c66792f48689632d2a7f14f8bdad9
theStack:
re-ACK f6a6d912059c66792f48689632d2a7f14f8bdad9
furszy:
utACK f6a6d912059. Only reviewed the actual change in detail, not the test commit.
Tree-SHA512: 30a196e611a0c5d9ebe5baf6d896caaa6af66f1615463dbb0c31e52604d53cf342922bb9967b3c697b47083d76b0485c77a5f545bd6381247c8bc44321c70f97
fabeca3458b38a3d8930cb0cbc866388c3f120f1 refactor: Avoid UB in SHA3_256::Write (MarcoFalke)
fad4032b219e58300f8d0a74bbcf38ef26fa89d0 refactor: Drop unused UCharCast (MarcoFalke)
Pull request description:
It is UB to apply a distance to a pointer or iterator further than the
end itself, even if the distance is (partially) revoked later on.
Fix the issue by advancing the data pointer at most to the end.
This fix is required to adopt C++ safe buffers https://github.com/bitcoin/bitcoin/issues/31272.
Also included is a somewhat unrelated commit.
ACKs for top commit:
sipa:
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1
theuni:
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1
hebasto:
ACK fabeca3458b38a3d8930cb0cbc866388c3f120f1.
Tree-SHA512: 78c53691322b72c3ba9c25ec94eec275dbbbc3049b0ad45e7d9fb2df0afbbaa905b0d8fa7106a3582f937bb1dc15a7592c4ad2d80fe4cff1062a3acfd3638f08
69e95c2b4f99eb4c2af6b2b6cc6a66abfea753df tests: Test cleanup of mkeys from wallets without privkeys (Andrew Chow)
2b9279b50a3682ab97308888b4f272d3ae379811 wallet: Remove unused encryption keys from watchonly wallets (Andrew Chow)
813a16a463326079472366aed42f5218a57db63b wallet: Add HasCryptedKeys (Andrew Chow)
Pull request description:
An earlier version allowed users to create watchonly wallets (wallets without private keys) that were "encrypted". Such wallets would have a stored encryption keys, but nothing would actually be encrypted with them. This can cause unexpected behavior such as https://github.com/bitcoin-core/gui/issues/772.
We can detect such wallets as they will have the disable private keys flag set, no encrypted keys, and encryption keys. For such wallets, we can remove those encryption keys thereby avoiding any issues that may result from this unexpected situation.
ACKs for top commit:
sipa:
utACK 69e95c2b4f99eb4c2af6b2b6cc6a66abfea753df.
laanwj:
Code review re-ACK 69e95c2b4f99eb4c2af6b2b6cc6a66abfea753df
furszy:
Code review ACK 69e95c2b4f99eb4
Tree-SHA512: 901932cd709c57e66c598f011f0105a243b5a8b539db2ef3fcf370dca4cf35ae09bc1110e8fca8353be470f159468855a4dd96b99bc9c1112adc86ccc50e1b9d
a96b84cb1b76e65a639e62f0224f534f89858c18 fuzz: Abort when calling system time without setting mock time (marcofleon)
ff21870e20b2391b684cc50fdd6879805055d6a1 fuzz: Add SetMockTime() to necessary targets (marcofleon)
Pull request description:
This PR expands the `CheckGlobals` utility that was introduced in https://github.com/bitcoin/bitcoin/pull/31486 and should help with fuzz stability (https://github.com/bitcoin/bitcoin/issues/29018).
System time shouldn't be used when running a fuzz test, as it is likely to introduce instability (non-determinism). This PR identifies and fixes the targets that were calling system time without setting mock time at the start of an iteration.
Removing`SetMockTime()` from any one of these targets should result in a crash and a message describing the issue.
ACKs for top commit:
achow101:
ACK a96b84cb1b76e65a639e62f0224f534f89858c18
dergoegge:
Code review ACK a96b84cb1b76e65a639e62f0224f534f89858c18
brunoerg:
crACK a96b84cb1b76e65a639e62f0224f534f89858c18
Tree-SHA512: e093a9feb8a397954f7b1416dfa8790b2733f09d5ac51fda5a9d225a55ebd8f99135aa52bdf5ab531653ad1a3739c4ca2b5349c1d989bb4b009ec8eaad684f7d
fd2d96d9087be234bdf4a6eb6d563e92b4fb4501 build, test: Build `db_tests.cpp` regardless of `USE_BDB` (Hennadii Stepanov)
Pull request description:
When the building of `db_tests.cpp` was made conditional on `USE_BDB` in commit a58b719cf75e2d97205ec260bcff0d4780fe4fb8, all `db_tests` were indeed specific to BDB wallets.
However, the tests have since been [extended](ba616b932c) to include SQLite wallets as well.
On the master branch @ 433412fd8478923dfdb20044f74c5d1e19fa8dd8, tests specific to SQLite wallets are not built and run if configured with `WITH_BDB=OFF` (the default option).
This PR resolves this issue by guarding BDB-specific code in `db_tests.cpp` and ensuring this source file is compiled regardless of the `WITH_BDB` option.
ACKs for top commit:
achow101:
ACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501
maflcko:
review ACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501 🔺
theuni:
utACK fd2d96d9087be234bdf4a6eb6d563e92b4fb4501
Tree-SHA512: bd9eddf16af60c568e931467d39e9e23a268e82e367ab630c23ac3cfd37e6007c6d78579b69ccbeebc1911c749cdbe75794fd56d7fbdb30c6fea6d2ab11017a3
589ed1a8eafe1daed379ebb383602c8f220aef19 wallet: migration, avoid loading wallet after failure when it wasn't loaded before (furszy)
Pull request description:
Fixes#31447.
During migration failure, only load wallet back into memory when the wallet was
loaded prior to migration. This fixes the case where BDB is not supported, which
implies that no legacy wallet can be loaded into memory due to the lack of db
writing functionality.
Link to error description https://github.com/bitcoin/bitcoin/issues/31447#issuecomment-2528757140.
This PR also improves migration backup related comments to better document the
current workflow.
ACKs for top commit:
achow101:
ACK 589ed1a8eafe1daed379ebb383602c8f220aef19
rkrux:
ACK 589ed1a8eafe1daed379ebb383602c8f220aef19
pablomartin4btc:
tACK 589ed1a8eafe1daed379ebb383602c8f220aef19
Tree-SHA512: c7a489d2b253c574ee0287b691ebe29fe8d026f659f68a3f6108eca8b4e1e420c67ca7803c6bd70c1e1440791833fabca3afbcf8fe8524c6c9fc08de95b618d0
If the chain advances during a rescan, ScanForWalletTransactions
would previously process the new blocks without adjusting m_last_processed_block,
which would leave the wallet in an inconsistent state temporarily, and could lead
to crashes in the GUI.
Fix this by not rescanning blocks beyond the last_processed_block -
for all blocks beyond that height, there will be pending BlockConnected
notifications that will process them after the rescan is finished.
Co-authored-by: Pablo Greco <psgreco@gmail.com>
