An overload of MigrateLegacyToDescriptor is added which takes the wallet
name. The original that took a wallet pointer is still available, it
just gets the name, closes the wallet, and calls the new overload.
6d31900e52 wallet: migrate wallet, exit early if no legacy data exist (furszy)
Pull request description:
The process first creates a backup file then return an error,
without removing the recently created file, when notices that
the db is already running sqlite.
ACKs for top commit:
john-moffett:
ACK 6d31900e52
achow101:
ACK 6d31900e52
ishaanam:
crACK 6d31900e52
Tree-SHA512: 9fb52e80de96e129487ab91bef13647bc4570a782003b1e37940e2a00ca26283fd24ad39bdb63a984ae0a56140b518fd0d74aa2fc59ab04405b2c179b7d3c54a
b0fa5989e1 test: Check that orphaned coinbase unconf spend is still abandoned (Andrew Chow)
9addbd7890 wallet: Automatically abandon orphaned coinbases and their children (Andrew Chow)
Pull request description:
When a block is reorged out of the main chain, any descendants of the coinbase will no longer be valid. Currently they are only marked as inactive, which means that our balance calculations will still include them. In order to be excluded from the balance calculation, they need to either be abandoned or conflicted. This PR goes with the abandoned method.
Note that even when they are included in balance calculations, coin selection will not select outputs belonging to these transactions because they are not in the mempool.
Fixes #14148
ACKs for top commit:
furszy:
ACK b0fa5989 with a not-blocking nit.
aureleoules:
reACK b0fa5989e1
ishaanam:
ACK b0fa5989e1
Tree-SHA512: 68f12e7aa8df392d8817dc6ac0becce8dbe83837bfa538f47027e6730e5b2e1b1a090cfcea2dc598398fdb66090e02d321d799f087020d7e1badcf96e598c3ac
To directly return a CRIPEMD160 hash from data.
Incidentally, decoding this acronym:
* RIPEMD -> RIPE Message Digest
* RIPE -> RACE Integrity Primitives Evaluation
* RACE -> Research and Development in Advanced Communications Technologies in Europe
fa035fe2d6 Remove unused CDataStream::SetType (MarcoFalke)
fa29e73cda Use DataStream where possible (MarcoFalke)
fa9becfe1c streams: Add DataStream without ser-type and ser-version (MarcoFalke)
Pull request description:
This was done in the context of https://github.com/bitcoin/bitcoin/pull/25284 , but I think it also makes sense standalone.
The basic idea is that serialization type should not be initialized when it is not needed. Same for the serialization version.
So do this here for `DataStream`. `CDataStream` remains in places where it is not yet possible.
ACKs for top commit:
stickies-v:
re-ACK [fa035fe](fa035fe2d6)
aureleoules:
diff re-ACK fa035fe2d6fa0e6640ba..fa035fe2d6
Tree-SHA512: cb5e53d0df7c94319ffadc6ea1d887fc38516decaf43f0673396d79cc62d450a1a61173654a91b8c2b52d2cecea53fe4a500b8f6466596f35731471163fb051c
fab958290b refactor: Remove c_str from util/check (MarcoFalke)
Pull request description:
Seems confusing and fragile to require calling code to call `c_str()` when passing a read-only view of a std::string.
Fix that by using std::string_view, which can be constructed from string literals and std::string.
Also, remove the now unused `c_str()` from `src/wallet/bdb.cpp`.
ACKs for top commit:
stickies-v:
ACK fab958290b
aureleoules:
ACK fab958290b
theStack:
ACK fab958290b
Tree-SHA512: ae39812c6bb8e2ef095e1b843774af2718f48404cb848c3e43b16d3c22240557d69d54a13a038a4a9c48b3ba0e4523e1f87abdd60f91486092f50fd43c0e8483
4aebd832a4 db: Change DatabaseCursor::Next to return status enum (Andrew Chow)
d79e8dcf29 wallet: Have cursor users use DatabaseCursor directly (Andrew Chow)
7a198bba0a wallet: Introduce DatabaseCursor RAII class for managing cursor (Andrew Chow)
69efbc011b Move SafeDbt out of BerkeleyBatch (Andrew Chow)
Pull request description:
Instead of having database cursors be tied to a particular `DatabaseBatch` object and requiring its setup and teardown be separate functions in that batch, we can have cursors be separate RAII classes. This makes it easier to create and destroy cursors as well as having cursors that have slightly different behaviors.
Additionally, since reading data from a cursor is a tri-state, this PR changes the return value of the `Next` function (formerly `ReadAtCursor`) to return an Enum rather than the current system of 2 booleans. This greatly simplifies and unifies the code that deals with cursors as now there is no confusion as to what the function returns when there are no records left to be read.
Extracted from #24914
ACKs for top commit:
furszy:
diff ACK 4aebd83
theStack:
Code-review ACK 4aebd832a4
Tree-SHA512: 5d0be56a18de5b08c777dd5a73ba5a6ef1e696fdb07d1dca952a88ded07887b7c5c04342f9a76feb2f6fe24a45dc31f094f1f5d9500e6bdf4a44f4edb66dcaa1
-BEGIN VERIFY SCRIPT-
sed -i -e "/Deprecated alias for OMITTED, can be removed/d" src/rpc/util.h src/rpc/util.cpp
sed -i -e "s/OMITTED_NAMED_ARG/OMITTED/g" $(git grep -l "OMITTED_NAMED_ARG" src/)
-END VERIFY SCRIPT-
a2ac6f9582 wallet: unify FindNonChangeParentOutput functions (furszy)
b3f4e82737 wallet: simplify ListCoins implementation (furszy)
Pull request description:
Focused on the following changes:
1) Removed the entire locked coins lookup that was inside `ListCoins` by including them directly on the `AvailableCoins` result (where we were skipping them before).
2) Unified both `FindNonChangeParentOutput` functions (only called from `ListCoins`)
ACKs for top commit:
achow101:
ACK a2ac6f9582
aureleoules:
ACK a2ac6f9582, LGTM
theStack:
Code-review ACK a2ac6f9582
Tree-SHA512: f72b21ee1600c5992559b5dcd8ff260527afac2ec696737f998343a0850b84d439e7f86ea52a14cc0cddabf132a30bf5b52fb34950578ac323083d4375b937f1
This enables the type check and fixes the wrong docs.
Otherwise the enabled check would lead to test errors, such as:
> "wallet_labels.py", line 96, in run_test
> node.sendmany(
>
> test_framework.authproxy.JSONRPCException:
> JSON value of type null is not of expected type string (-3)
fa9f6d7bcd rpc: Run type check against RPCArgs (MarcoFalke)
faf96721a6 test: Fix wrong types passed to RPCs (MarcoFalke)
Pull request description:
It seems brittle to require `RPCTypeCheck` being called inside the code logic. Without compile-time enforcement this will lead to places where it is forgotten and thus to inconsistencies and bugs. Fix this by removing the calls to `RPCTypeCheck` and doing the check internally.
The changes should be reviewed as refactoring changes. However, if they change behavior, it will be a bugfix. For example the changes here happen to also detect/fix bugs like the one fixed in commit 3b5fb6e77a.
ACKs for top commit:
ajtowns:
ACK fa9f6d7bcd
Tree-SHA512: fb2c0981fe6e24da3ca7dbc06898730779ea4e02ea485458505a281cf421015e44dad0221a04023fc547ea2c660d94657909843fc85d92b847611ec097532439
cfe5aebc79 rpc: add minconf and maxconf options to sendall (ishaanam)
a07a413466 Wallet/RPC: Allow specifying min & max chain depth for inputs used by fund calls (Juan Pablo Civile)
Pull request description:
This PR adds a "minconf" option to `fundrawtransaction`, `walletcreatefundedpsbt`, and `sendall`.
Alternative implementation of #14641
Fixes #14542
Edit: This PR now also adds this option to `send`
ACKs for top commit:
achow101:
ACK cfe5aebc79
Xekyo:
ACK cfe5aebc79
furszy:
diff ACK cfe5aebc, only a non-blocking nit.
Tree-SHA512: 836e610926eec3a62308fba88ddbd6a13d8f4dac37352d0309599f893cde9c1df5e9c298fda6e076493068e4d213e4afa7290a9e3bdb5a95a5d507da3f7b59e8
f9ce0eadf4 For feebump, ignore abandoned descendant spends (John Moffett)
Pull request description:
Closes #26667
To be eligible for fee-bumping, a transaction must not have any of its outputs (eg - change) spent in other unconfirmed transactions in the wallet. This behavior is currently [enforced](9e229a542f/src/wallet/feebumper.cpp (L25-L28)) and [tested](9e229a542f/test/functional/wallet_bumpfee.py (L270-L286)).
However, this check shouldn't apply to spends in abandoned descendant transactions, as explained by #26667.
`CWallet::IsSpent` already carves out an exception for abandoned transactions, so we can just use that.
I've also added a new test to cover this case.
ACKs for top commit:
Sjors:
re-utACK f9ce0eadf4
achow101:
ACK f9ce0eadf4
furszy:
ACK f9ce0ead
Tree-SHA512: 19d957d1cf6747668bb114e27a305027bfca5a9bed2b1d9cc9e1b0bd4666486c7c4b60b045a7fe677eb9734d746f5de76390781fb1e9e0bceb4a46d20acd1749
3784009534 wallet: Skip rescanning if wallet is more recent than tip (Andrew Chow)
Pull request description:
If a wallet has key birthdates that are more recent than the currrent chain tip, or a bestblock height higher than the current tip, we should not attempt to rescan as there is nothing to scan for.
Fixes #26655
ACKs for top commit:
ishaanam:
re-utACK 3784009534
w0xlt:
utACK 3784009534
furszy:
Code review ACK 37840095
Tree-SHA512: f0d90b62940d97d50f21e1e01fa6dcb54409fad819cea4283612825c4d93d733df323cd92787fed43956b0a8e386a5bf88218f1f5749c913398667a5c8f54470
65e78bda7c test: Invalid label name coverage (Aurèle Oulès)
552b51e682 refactor: Add sanity checks in LabelFromValue (Aurèle Oulès)
67e7ba8e1a rpc: Sanitize label name in various RPCs (Aurèle Oulès)
Pull request description:
The following RPCs did not sanitize the optional label name:
- importprivkey
- importaddress
- importpubkey
- importmulti
- importdescriptors
- listsinceblock
Thus is was possible to import an address with a label `*` which should not be possible.
The wildcard label is used for backwards compatibility in the `listtransactions` rpc.
I added test coverage for these RPCs.
ACKs for top commit:
ajtowns:
ACK 65e78bda7c
achow101:
ACK 65e78bda7c
furszy:
diff ACK 65e78bd
stickies-v:
re-ACK 65e78bda7c
theStack:
re-ACK 65e78bda7c
Tree-SHA512: ad99f2824d4cfae352166b76da4ca0069b7c2eccf81aaa0654be25bbb3c6e5d6b005d93960f3f4154155f80e12be2d0cebd5529922ae3d2a36ee4eed82440b31
730e14a317 test: wallet: check that labels are migrated to watchonly wallet (Sebastian Falbesoner)
d5f4ae7fac wallet: fully migrate address book entries for watchonly/solvable wallets (Sebastian Falbesoner)
Pull request description:
Currently `migratewallet` migrates the address book (i.e. labels and purposes) for watchonly and solvable wallets only in RAM, but doesn't persist them on disk. Fix this by adding another loop for both of the special wallet types after which writes the corresponding NAME and PURPOSE entries to the database in a single batch. Also adds a corresponding test that checks if labels were migrated correctly for a watchonly wallet.
ACKs for top commit:
achow101:
ACK 730e14a317
furszy:
code ACK 730e14a3, left a non-blocking nit.
aureleoules:
ACK 730e14a317
Tree-SHA512: 159487e11e858924ef762e0190ccaea185bdff239e3d2280c8d63c4ac2649ec71714dc4d53dec644f03488f91c3b4bbbbf3434dad23bc0fcecb6657f353ea766
f2fc03ec85 refactor: use braced init for integer constants instead of c style casts (Pasta)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/23810 for more context. This is broken out from that PR, as it is less breaking, and should be trivial to review and merge.
EDIT: Long term, the intention is to remove all C-style casts, as they can dangerously introduce reinterpret_casts. This is one step which removes a number of trivially removable C-style casts
ACKs for top commit:
aureleoules:
ACK f2fc03ec85
Tree-SHA512: 2fd11b92c9147e3f970ec3e130e3b3dce70e707ff02950a8c697d4b111ddcbbfa16915393db20cfc8f384bc76f13241c9b994a187987fcecd16a61f8cc0af14c
21ad4e26ec test: add coverage for cross-chain wallet restore (Sebastian Falbesoner)
8c7222bda3 wallet: fix GUI crash on cross-chain legacy wallet restore (Sebastian Falbesoner)
Pull request description:
Restoring a wallet backup from another chain should result in a dedicated error message (we have _"Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override."_ for that). Unfortunately this is currently not the case for legacy wallet restores, as in the course of cleaning up the newly created wallet directory a `filesystem_error` exception is thrown due to the directory not being empty; the wallet database did indeed load successfully (otherwise we wouldn't know that the chain doesn't match) and hence BDB-related files and directories are already created in the wallet directory.
For bitcoind, this leads to a very confusing error message:
```
$ ./src/bitcoin-cli restorewallet test123 ~/.bitcoin/regtest/wallets/regtest_wallet/wallet.dat
error code: -1
error message: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/test123"]
```
Even worse, the GUI crashes in such a scenario:
```
libc++abi: terminating with uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/foobar"]
Abort trap (core dumped)
```
Fix this by simply deleting the whole folder via `fs::remove_all`. With this, the expected error message appears both for the `restorewallet` RPC call and in the GUI (as a message-box):
```
$ ./src/bitcoin-cli restorewallet test123 ~/.bitcoin/regtest/wallets/regtest_wallet/wallet.dat
error code: -4
error message:
Wallet loading failed. Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override.
```
ACKs for top commit:
achow101:
ACK 21ad4e26ec
aureleoules:
ACK 21ad4e26ec
furszy:
utACK 21ad4e26
Tree-SHA512: 313f6494c2fbe823bff9b975cb2d9410bb518977a1e59a5159ee9836bc012947fa50b56be0e41b1a2f50d9c0c7f4fddfdf4fbe479d8a59a6ee44bb389c804abc
55696a0ac3 wallet: remove `mempool_sequence` from `transactionRemovedFromMempool` (w0xlt)
bf19069c53 wallet: remove `mempool_sequence` from `transactionAddedToMempool` (w0xlt)
Pull request description:
This PR removes `mempool_sequence` from `transactionRemovedFromMempool` and `transactionAddedToMempool`.
`mempool_sequence` is not used in these methods, only in ZMQ notifications.
ACKs for top commit:
instagibbs:
ACK 55696a0ac3
Tree-SHA512: 621e89230bcb6edfed83e2758601a2b093822fc2dc4e9bfb00487e340f2bc4c5ac3bf6df3ca00b7fe55bb3df15858820f2bf698f403d2e48b915dd9eb47b63e0
927b8d4e0c rpc: Correct RPCHelpMan for fundrawtransaction's input_weights field (jdjkelly@gmail.com)
Pull request description:
`input_weights` is incorrectly documented as a fixed length JSON array, but it is actually a JSON array of JSON objects - this commit changes `input_weights` to use `RPCArg::Type::OBJ`
The behavior of `input_weights` as an object exists as a functional test in [wallet_fundrawtransaction.py](https://github.com/bitcoin/bitcoin/blob/master/test/functional/wallet_fundrawtransaction.py).
ACKs for top commit:
achow101:
ACK 927b8d4e0c
Tree-SHA512: 384f5e16be36dba670d64d96f16f1fde2d0d51357e1094ae13eb71d004af0f4dc8bac965b4d2d724ccf64fb671faad37b73055152a9882af24f65dfceaf1e5fb
76dc547ee7 gui: create tx, launch error dialog if backend throws runtime_error (furszy)
f4d79477ff wallet: coin selection, add duplicated inputs checks (furszy)
0aa065b14e wallet: return accurate error messages from Coin Selection (furszy)
7e8340ab1a wallet: make SelectCoins flow return util::Result (furszy)
e5e147fe97 wallet: refactor eight consecutive 'AttemptSelection' calls into a loop (furszy)
Pull request description:
Work decoupled from #25806, which cleanup and improves the Coin Selection flow further.
Adding the capability to propagate specific error messages from the Coin Selection process to the user.
Instead of always returning the general "Insufficient funds" message which is not always accurate to what happened internally.
Letting us instruct the user how to proceed under certain circumstances.
The following error messages were added:
1) If the selection result exceeds the maximum transaction weight,
we now will return:
-> "The inputs size exceeds the maximum weight. Please try sending
a smaller amount or manually consolidating your wallet's UTXOs".
2) If the user pre-selected inputs and disallowed the automatic coin
selection process (no other inputs are allowed), we now will
return:
-> "The preselected coins total amount does not cover the transaction
target. Please allow other inputs to be automatically selected or include
more coins manually".
3) The double-counted preset inputs during Coin Selection error will now
throw an "internal bug detected" message instead of crashing the node.
The essence of this work comes from several comments:
1. https://github.com/bitcoin/bitcoin/pull/26560#discussion_r1037395665
2. https://github.com/bitcoin/bitcoin/pull/25729#discussion_r940619491
3. https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-1135240825
4. https://github.com/bitcoin/bitcoin/issues/23144 (which is connected to #24845)
ACKs for top commit:
ishaanam:
crACK 76dc547ee7
achow101:
ACK 76dc547ee7
aureleoules:
ACK 76dc547ee7
theStack:
ACK 76dc547ee7🌇
Tree-SHA512: 9de30792d7a5849cae77747aa978e70390b66ee9d082779a56088a024f82e725b0af050e6603aece0ac8229f6d73bc471ba97b4ab69dc7eddf419f5f56ae89a5
04609284ad rpc: Improve error when wallet is already loaded (Aurèle Oulès)
Pull request description:
Currently, trying to load a descriptor (sqlite) wallet that is already loaded throws the following error:
> error code: -4
> error message:
> Wallet file verification failed. SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of Bitcoin Core?
I don't think it is very clear what it means for a user.
While a legacy wallet would throw:
> error code: -35
> error message:
> Wallet file verification failed. Refusing to load database. Data file '/home/user/.bitcoin/signet/wallets/test_wallet/wallet.dat' is already loaded.
This PR changes the error message for both types of wallet to:
> error code: -35
> error message:
> Wallet file verification failed. Wallet "test_wallet" is already loaded.
ACKs for top commit:
achow101:
ACK 04609284ad
hernanmarino:
ACK 0460928
theStack:
Tested ACK 04609284ad
Tree-SHA512: a8f3d5133bfaef7417a6c05d160910ea08f32ac62bfdf7f5ec305ff5b62e9113b55f385abab4d5a4ad711aabcb1eb7ef746eb41f841b196e8fb5393ab3ccc01e
9622fe64b8 test: move coins result test to wallet_tests.cpp (furszy)
f69347d058 test: extend and simplify availablecoins_tests (furszy)
212ccdf2c2 wallet: AvailableCoins, add arg to include/skip locked coins (furszy)
Pull request description:
Negative PR with extended test coverage :).
1) Cleaned duplicated code and added coverage for the 'AvailableCoins' incremental result.
2) The class `AvailableCoinsTestingSetup` inside `availablecoins_tests.cpp` is a plain copy
of `ListCoinsTestingSetup` that is inside `wallet_tests.cpp`.
So, deleted the file and moved the `BasicOutputTypesTest` test case to `wallet_tests.cpp`.
3) Added arg to include/skip locked coins from the `AvailableCoins` result. This is needed for point (1) as otherwise the wallet will spend the coins that we recently created due its closeness to the recipient amount.
Note: this last point comes from #25659 where I'm using the same functionality to clean/speedup another flow as well.
ACKs for top commit:
achow101:
ACK 9622fe64b8
theStack:
ACK 9622fe64b8
aureleoules:
reACK 9622fe64b8, nice cleanup!
Tree-SHA512: 1ed9133120bfe8815455d1ad317bb0ff96e11a0cc34ee8098716ab9b001749168fa649212b2fa14b330c1686cb1f29039ff1f88ae306db68881b0428c038f388
81d4a2b14f refactor: Move feerate comparison invariant outside of the loop (yancy)
365aca4045 refactor: Simplify feerate comparison statement (yancy)
Pull request description:
This is a small nit, however I think it's more understandable to write:
`utxo_pool.at(0).fee > utxo_pool.at(0).long_term_fee`
vs
`(utxo_pool.at(0).fee - utxo_pool.at(0).long_term_fee) > 0`
ACKs for top commit:
Xekyo:
ACK 81d4a2b14f
achow101:
ACK 81d4a2b14f
aureleoules:
ACK 81d4a2b14f
Tree-SHA512: 3e89377989c36716b53114fe40178261671dde5688075fab1c21ec173ac310f8c84ed6af90354d7c329176cb7262dfcaa7191fd19847d3b7147a9a10c3e31176
f496528556 walletdb: refactor: drop unused `FindWalletTx` parameter and rename (Sebastian Falbesoner)
Pull request description:
Since commit 3340dbadd3 ("Remove -zapwallettxes"), the `FindWalletTx` helper is only needed to read tx hashes, so drop the other parameter and rename the method accordingly.
ACKs for top commit:
S3RK:
code review ACK f496528556
achow101:
ACK f496528556
vincenzopalazzo:
ACK f496528556
Tree-SHA512: ead85bc724462f9e920f9d7fe89679931361187579ffd6e63427c8bf5305cd5f71da24ed84f3b1bd22a12be46b5abec13f11822e71a3e1a63bf6cf49de950ab5
input_weights is incorrectly documented as a fixed length JSON array,
but it is actually a JSON array of JSON objects - this commit changes
input_weights to use RPCArg::Type::OBJ
The field 'comment' appears twice in TransactionDescriptionString,
incorrectly - this commit removes the instance of the comment field
without a description, preserving the one with a description
Currently `migratewallet` migrates the address book (i.e. labels and
purposes) for watchonly and solvable wallets only in RAM, but doesn't
persist them on disk. Fix this by adding another loop for both of the
special wallet types after which writes the corresponding NAME and
PURPOSE entries to the database in a single batch.
