There are occasions where a multi-statement tx is begun in one batch,
and a second batch is created which does a normal write (without a
multi-statement tx). These should not conflict with each other and all
of the data should end up being written to disk.
004903ebad test: Add Wallet Unlock Context Manager (Brandon Odiwuor)
Pull request description:
Fixes#28601, see https://github.com/bitcoin/bitcoin/pull/28403#discussion_r1325426430
Add Context Manager to manage the locking and unlocking of locked wallets with a passphrase during testing.
ACKs for top commit:
kevkevinpal:
lgtm ACK [004903e](004903ebad)
maflcko:
lgtm ACK 004903ebad
Tree-SHA512: ab234c167e71531df0d974ff9a31d444f7ce2a1d05aba5ea868cc9452f139845eeb24ca058d88f058bc02482b762adf2d99e63a6640b872cc71a57a0068abfe8
Connection object used as context manager only commits or rollbacks
transactions, so the connection object should be closed manually.
Fixes the following error on Windows:
```
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: ...
```
Instead of passing the datadir and chain name to os.path.join, just use
the existing properties, which are the same.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's|\.datadir, self\.chain, .wallets.|.wallets_path|g' $(git grep -l '\.datadir, self\.chain,')
sed -i --regexp-extended 's|\.datadir, self\.chain,|.chain_path,|g' $(git grep -l '\.datadir, self\.chain,')
-END VERIFY SCRIPT-
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.
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
3198e4239e test: check that loading descriptor wallet with legacy entries throws error (Sebastian Falbesoner)
349ed2a0ee wallet: throw error if legacy entries are present on loading descriptor wallets (Sebastian Falbesoner)
Pull request description:
Loading a descriptor wallet currently leads to a segfault if a legacy key type entry is present that can be deserialized successfully and needs SPKman-interaction. To reproduce with a "cscript" entry (see second commit for details):
```
$ ./src/bitcoin-cli createwallet crashme
$ ./src/bitcoin-cli unloadwallet crashme
$ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat
SQLite version 3.38.2 2022-03-26 13:51:10
Enter ".help" for usage hints.
sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00');
$ ./src/bitcoin-cli loadwallet crashme
--- bitcoind output: ---
2022-11-06T13:51:01Z Using SQLite Version 3.38.2
2022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme
2022-11-06T13:51:01Z init message: Loading wallet…
2022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900
Segmentation fault (core dumped)
```
Background: In the wallet key-value-loading routine, most legacy type entries require a `LegacyScriptPubKeyMan` instance after successful deserialization. On a descriptor wallet, creating that (via method `GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a null-pointer dereference crash. E.g. for CSCRIPT: 50422b770a/src/wallet/walletdb.cpp (L589-L594)
~~This PR fixes this by simply ignoring legacy entries if the wallet flags indicate that we have a descriptor wallet. The second commits adds a regression test to the descriptor wallet's functional test (fortunately Python includes sqlite3 support in the standard library).~~
~~Probably it would be even better to throw a warning to the user if unexpected legacy entries are found in descriptor wallets, but I think as a first mitigation everything is obvisouly better than crashing. As far as I'm aware, descriptor wallets created/migrated by Bitcoin Core should never end up in a state containing legacy type entries though.~~
This PR fixes this by throwing an error if legacy entries are found in descriptor wallets on loading.
ACKs for top commit:
achow101:
ACK 3198e4239e
aureleoules:
ACK 3198e4239e
Tree-SHA512: ee43da3f61248e0fde55d9a705869202cb83df678ebf4816f0e77263f0beac0d7bae9490465d1753159efb093ee37182931d76b2e2b6e8c6f8761285700ace1c
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.
The previous diff touched most files in ./test/, so bump the headers to
avoid having to touch them again for a bump later.
-BEGIN VERIFY SCRIPT-
./contrib/devtools/copyright_header.py update ./test/
-END VERIFY SCRIPT-
de6b389d5d tests: Test getaddressinfo parent_desc (Andrew Chow)
e4ac869a0a rpc: Add parent descriptor to getaddressinfo output (Andrew Chow)
bbe4a36152 wallet: Add GetDescriptorString to DescriptorScriptPubKeyMan (Andrew Chow)
9be1437c49 descriptors: Add ToNormalizedString and tests (Andrew Chow)
Pull request description:
Adds `parent_desc` field to the `getaddressinfo` RPC to export a public descriptor. Using the given address, `getaddressinfo` will look up which `DescriptorScriptPubKeyMan` can be used to produce that address. It will then return the descriptor for that `DescriptorScriptPubKeyMan` in the `parent_desc` field. The descriptor will be in a normalized form where the xpub at the last hardened step is derived so that the descriptor can be imported to other wallets. Tests are added to check that the correct descriptor is being returned for the wallet's addresses and that these descriptors can be imported and used in other wallets.
As part of this PR, a `ToNormalizedString` function is added to the descriptor classes. This really only has an effect on `BIP32PubkeyProvider`s that have hardened derivation steps. Tests are added to check that normalized descriptors are returned.
ACKs for top commit:
Sjors:
utACK de6b389d5d
S3RK:
Tested ACK de6b389
jonatack:
Tested ACK de6b389d5d modulo a few minor comments
fjahr:
Code review ACK de6b389d5d
meshcollider:
Tested ACK de6b389d5d
Tree-SHA512: a633e4a39f2abbd95afd7488484cfa66fdd2651dac59fe59f2b80a0940a2a4a13acf889c534a6948903d701484a2ba1218e3081feafe0b9a720dccfa9e43ca2b
substitutes "for x in range(N):" by "for _ in range(N):"
indicates to the reader that a block is just repeated N times, and
that the loop counter is not used in the body
RPCOverloadWrapper overloads some deprecated or disabled RPCs with
an implementation using other RPCs to avoid having a ton of code churn
around replacing those RPCs.
