| Age | Commit message (Collapse) | Author |
|---|
|
This commit introduces a helper `create_outpoints` to execute the
`send` RPC and immediately return the target address outpoints as UTXO
dictionary in the common format, making the tests more readable and
avoiding unnecessary duplication.
|
|
|
|
When migrating, create the watchonly and solvables wallets without a
context. Then unload and reload them after migration completes, as we do
for the actual wallet.
There is also additional handling for a failed reload.
|
|
|
|
Descriptors disallows hybrid pubkeys. Anything with hybrid pubkeys
should becomes a raw() descriptor that shows up in the watchonly wallet.
|
|
Loading a wallet with conflicts without a chain (e.g. wallet tool and
migration) would previously result in an assertion due to -1 being both
a valid number of conflict confirmations, and the indicator that that
member has not been set yet.
|
|
scripts
8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9 test: wallet, verify migration doesn't crash for an invalid script (furszy)
1de8a2372ab39386e689b27d15c4d029be239319 wallet: disallow migration of invalid or not-watched scripts (furszy)
Pull request description:
Fixing #28057.
The legacy wallet allows to import any raw script (#28126), without
checking if it was valid or not. Appending it to the watch-only set.
This causes a crash in the migration process because we are only
expecting to find valid scripts inside the legacy spkm.
These stored scripts internally map to `ISMINE_NO` (same as if they
weren't stored at all..).
So we need to check for these special case, and take into account that
the legacy spkm could be storing invalid not watched scripts.
Which, in code words, means `IsMineInner()` returning
`IsMineResult::INVALID` for them.
Note:
To verify this, can run the test commit on top of master.
`wallet_migration.py` will crash without the bugfix commit.
ACKs for top commit:
achow101:
ACK 8e7e3e614955e60d3bf9e9a481ef8916bf9e22d9
Tree-SHA512: c2070e8ba78037a8f573b05bf6caa672803188f05429adf5b93f9fc1493faedadecdf018dee9ead27c656710558c849c5da8ca5f6f3bc9c23b3c4275d2fb50c7
|
|
|
|
The migration process must skip any invalid script inside the legacy
spkm and all the addressbook records linked to them.
These scripts are not being watched by the current wallet, nor should
be watched by the migrated one.
IsMine() returns ISMINE_NO for them.
|
|
|
|
|
|
Seems odd to place the burden on test writers to hardcode the chain or
datadir path for the nodes under test.
|
|
wallet dir
a1e653828bc59351b2a0dd5a70f519e6b61199bc test: Add test for migrating default wallet and plain file wallet (Andrew Chow)
bdbe3fd76b4b9186503dc1926a2fa3f8178d00a5 wallet: Generated migrated wallet's path from walletdir and name (Andrew Chow)
Pull request description:
This PR fixes an assertion error that is hit during the setup of the new database during migration of a wallet that was not contained in a wallet dir. Also added a test for this case as well as one for migrating the default wallet.
ACKs for top commit:
ryanofsky:
Code review ACK a1e653828bc59351b2a0dd5a70f519e6b61199bc
furszy:
ACK a1e65382
Tree-SHA512: 96b218c0de8567d8650ec96e1bf58b0f8ca4c4726f5efc6362453979b56b9d569baea0bb09befb3a5aed8d16d29bf75ed5cd8ffc432bbd4cbcad3ac5574bc479
|
|
|
|
2cd28e9fef5dd743bcd70025196ee311fcfdcae4 rpc: Add check for unintended option/parameter name clashes (Ryan Ofsky)
95d7de0964620a3f7386a4adc5707559868abf84 test: Update python tests to use named parameters instead of options objects (Ryan Ofsky)
96233146dd31c1d99fd1619be4449944623ef750 RPC: Allow RPC methods accepting options to take named parameters (Ryan Ofsky)
702b56d2a8ce48bc3b66a2867d09fa11dcf12fc5 RPC: Add add OBJ_NAMED_PARAMS type (Ryan Ofsky)
Pull request description:
Allow RPC methods which take an `options` parameter (`importmulti`, `listunspent`, `fundrawtransaction`, `bumpfee`, `send`, `sendall`, `walletcreatefundedpsbt`, `simulaterawtransaction`), to accept the options as named parameters, without the need for nested JSON objects.
This makes it possible to make calls like:
```sh
src/bitcoin-cli -named bumpfee txid fee_rate=10
```
instead of
```sh
src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 10}'
```
RPC help is also updated to show options as top level named arguments instead of as nested objects.
<details><summary>diff</summary>
<p>
```diff
@@ -15,16 +15,17 @@
Arguments:
1. txid (string, required) The txid to be bumped
-2. options (json object, optional)
+2. options (json object, optional) Options object that can be used to pass named arguments, listed below.
+
+Named Arguments:
- {
- "conf_target": n, (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
+conf_target (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
- "fee_rate": amount, (numeric or string, optional, default=not set, fall back to wallet fee estimation)
+fee_rate (numeric or string, optional, default=not set, fall back to wallet fee estimation)
Specify a fee rate in sat/vB instead of relying on the built-in fee estimator.
Must be at least 1.000 sat/vB higher than the current transaction fee rate.
WARNING: before version 0.21, fee_rate was in BTC/kvB. As of 0.21, fee_rate is in sat/vB.
- "replaceable": bool, (boolean, optional, default=true) Whether the new transaction should still be
+replaceable (boolean, optional, default=true) Whether the new transaction should still be
marked bip-125 replaceable. If true, the sequence numbers in the transaction will
be left unchanged from the original. If false, any input sequence numbers in the
original transaction that were less than 0xfffffffe will be increased to 0xfffffffe
@@ -32,11 +33,10 @@
still be replaceable in practice, for example if it has unconfirmed ancestors which
are replaceable).
- "estimate_mode": "str", (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
+estimate_mode (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
"unset"
"economical"
"conservative"
- }
Result:
{ (json object)
```
</p>
</details>
**Review suggestion:** To understand this PR, it is probably easiest to review the commits in reverse order because the last commit shows the external API changes, the middle commit shows the internal API changes, and the first commit contains the low-level implementation.
ACKs for top commit:
achow101:
ACK 2cd28e9fef5dd743bcd70025196ee311fcfdcae4
Tree-SHA512: 50f6e78fa622826dab3f810400d8c1a03a98a090b1f2fea79729c58ad8cff955554bd44c2a5975f62a526b900dda68981862fd7d7d05c17f94f5b5d847317436
|
|
fe49f06c0e91b96feb8d8f1bd478c3173f14782c doc: clarify PR 26076 release note (Sjors Provoost)
bd13dc2f46ea10302a928fcf0f53b7aed77ad260 Switch hardened derivation marker to h in descriptors (Sjors Provoost)
Pull request description:
This makes it easier to handle descriptor strings manually, especially when importing from another Bitcoin Core wallet.
For example the `importdescriptors` RPC call is easiest to use `h` as the marker: `'["desc": ".../0h/..."]'`, avoiding the need for escape characters. With this change `listdescriptors` will use `h`, so you can copy-paste the result, without having to add escape characters or switch `'` to 'h' manually.
Both markers can still be parsed.
The `hdkeypath` field in `getaddressinfo` is also impacted by this change, except for legacy wallets. The latter is to prevent accidentally breaking ancient software that uses our legacy wallet.
See discussion in #15740
ACKs for top commit:
achow101:
ACK fe49f06c0e91b96feb8d8f1bd478c3173f14782c
darosior:
re-ACK fe49f06c0e91b96feb8d8f1bd478c3173f14782c
Tree-SHA512: f78bc873b24a6f7a2bf38f5dd58f2b723e35e6b10e4d65c36ec300e2d362d475eeca6e5afa04b3037ab4bee0bf8ebc93ea5fc18102a2111d3d88fc873c08dc89
|
|
|
|
This makes it easier to handle descriptor strings manually. E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.
Both markers can still be parsed. The default for new descriptors is changed to h. In normalized form h is also used. For private keys the chosen marker is preserved in a round trip.
The hdkeypath field in getaddressinfo is also impacted by this change.
|
|
|
|
Since migration reloads the wallet, the wallet will always be locked
unless the passphrase is given. migratewallet can now take the
passphrase in order to unlock the wallet for migration.
|
|
otherwise the process will create a backup file then return
an error when notices that the db is already running sqlite.
|
|
|
|
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./
-END VERIFY SCRIPT-
Commits of previous years:
- 2021: f47dda2c58b5d8d623e0e7ff4e74bc352dfa83d7
- 2020: fa0074e2d82928016a43ca408717154a1c70a4db
- 2019: aaaaad6ac95b402fe18d019d67897ced6b316ee0
|
|
|
|
failure cleanup
5e65a216d1fd00c447757736d4f2899d235e731a wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow)
88afc73ae0c67a4482ecd3d77eb2a8fd2673f82d tests: Test for migrating encrypted wallets (Andrew Chow)
86ef7b3c7be84e4183098f448c77ecc9ea7367ab wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow)
Pull request description:
When `migratewallet` fails, we do an automatic cleanup in order to reset everything so that the user does not experience any interruptions. However, this apparently has a segfault in it, caused by the the pointers to the watchonly and solvables wallets being nullptr. If those wallets are not created (either not needed, or failed early on), we will accidentally attempt to dereference these nullptrs, which causes a segfault.
This failure can be easily reached by trying to migrate an encrypted wallet. Currently, we can't migrate encrypted wallets because of how we unload wallets before migrating, and therefore forget the encryption key if the wallet was unlocked. So any encrypted wallets will fail, entering the cleanup, and because watchonly and solvables wallets don't exist yet, the segfault is reached.
This PR fixes this by not putting those nullptrs in a place that we will end up dereferencing them later. It also adds a test that uses the encrypted wallet issue.
ACKs for top commit:
S3RK:
reACK 5e65a216d1fd00c447757736d4f2899d235e731a
stickies-v:
ACK [5e65a21](https://github.com/bitcoin/bitcoin/commit/5e65a216d1fd00c447757736d4f2899d235e731a)
furszy:
diff ACK 5e65a21
Tree-SHA512: f75643797220d4232ad3ab8cb4b46d0f3667f00486e910ca748c9b6d174d446968f1ec4dd7f907da1be9566088849da7edcd8cd8f12de671c3241b513deb8e80
|
|
|
|
Due to an oversight, we cannot currently migrate encrypted wallets,
regardless of whether they are unlocked. Migrating such wallets will
trigger an error, and result in the cleanup being run. This conveniently
allows us to check some parts of the cleanup code.
|
|
Review note: The changes are complete, because self.options.descriptors
is set to None in parse_args (test_framework.py).
A value of None implies -disablewallet, see the previous commit.
So if a call to add_wallet_options is missing, it will lead to a test
failure when the wallet is compiled in.
|
|
Co-Authored-By: furszy <matiasfurszyfer@protonmail.com>
|
