0390338692 test: check MuSig import private key warnings (woltx)
5e62fbf09c wallet: check descriptor private key completeness on import (woltx)
cd8d01bf47 descriptors: require complete MuSig private keys (woltx)
Pull request description:
`importdescriptors` currently checks whether all private keys are present by expanding the descriptor and verifying that every expanded origin pubkey has a private key.
This is wrong for MuSig descriptors because expansion includes the synthetic aggregate pubkey. There is no individual private key for that aggregate pubkey, so importing a fully private MuSig descriptor such as `rawtr(musig(A_priv,B_priv))` incorrectly returns:
```
Not all private keys provided. Some wallet functionality may return unexpected errors
```
This PR fixes the issue by making descriptor private-key completeness account for MuSig participant keys, and by having `importdescriptors` use `Descriptor::HavePrivateKeys()` instead of duplicating its own manual completeness check.
The functional test covers both cases:
- `rawtr(musig(A_priv,B_priv))` imports without warnings.
- `rawtr(musig(A_priv,B_pub))` still warns that not all private keys were provided.
ACKs for top commit:
achow101:
ACK 0390338692
theStack:
Code-review ACK 0390338692
Tree-SHA512: a55fb084c63f725a0991556acdfb822f3a5a669f745a00b9f0bf0996b639986cdf5e2be2e8d3d0a2ee3fe5744355f20b40df576601792ef3db698e606629ad52
3ac8b806a6 test: test the result order of a multiple import request is correct (Pol Espinasa)
e4732bf018 test: test invalid or missing timestamp throws importdescriptors (Pol Espinasa)
07fb58b9ef test: Test a locked wallet rejects an empty importdescriptors request (Pol Espinasa)
Pull request description:
In addition to #35179 (already merged) this adds more missing test coverage that was detected while rebasing #34861.
The three tests added checks:
- Locked wallet throws because of being locked if giving an empty importdescriptors request.
- Invalid or missing timestamp throws as a top level RPC error and not a per-item error.
- The order of the requests and the response is the same, even if failing or succeeding.
ACKs for top commit:
nebula-21:
ACK 3ac8b806a6
Bicaru20:
re-ACK 3ac8b806a6
brunoerg:
reACK 3ac8b806a6
Tree-SHA512: b6ba9e16bbdbefcab2529f49f9aab0ae8885bd2d381c6eec36ae442dea1aa2361e6fb339ab5bc2c51c3bef6216d8d939db53e57ec577f05fe54c07fc46f8f255
9e62e4b1f3 test: slow down rescaning process (Pol Espinasa)
336f5a738b wallet: reserve walletrescan before checking wallet is at the tip (Pol Espinasa)
Pull request description:
`ImportDescriptors` rpc has a race condition where two imports running in parallel can both succeed or fail one of them.
The race happens when there are two threads A and B trying to importdescriptors at the same time.
1. Thread A calls `BlockUntilSyncedToCurrentChain()` (holding `cs_wallet` fast, no contention) and then `reserve()`, acquiring the `WalletRescanReserver`. It proceeds to `ProcessDescriptorImport()`, which holds `cs_wallet` for an extended time (specially on slow machines) while importing descriptors.
2. B reaches `BlockUntilSyncedToCurrentChain()`, which internally does `WITH_LOCK(cs_wallet, ...)`. Since A holds `cs_wallet`, B blocks here for the entire duration of Thread A's descriptors import.
3. Then A finishes importing, releases `cs_wallet`, rescans (fast in regtest), and sets `fScanningWallet = false`.
4. B can now continue in `BlockUntilSyncedToCurrentChain()` acquiring `cs_wallet`, and then calls `reserve()` which succeeds because `fScanningWallet` is already `false`. Both imports succeed.
I don't think the behavior is problematic at all from a usability PoV, but it can be a bad UX if some imports fails and other's no. It also makes testing difficult as race conditions are not easy to test.
This PR fixes it by calling `reserver.reserve()` before `cs_wallet` is locked, so multiple threads will be aware of currently imports before being stuck at any point. So only one `importdescriptor` call can be done at the same time.
I think this should fix https://github.com/bitcoin/bitcoin/issues/35544#issuecomment-4763488259
ACKs for top commit:
achow101:
ACK 9e62e4b1f3
nebula-21:
ACK 9e62e4b1f3
w0xlt:
lgtm reACK 9e62e4b1f3
Tree-SHA512: be0027e1a7b77252ed9fb514c3b3311d6905903d4b0bfc1021a1e1c2bb06872ef599647ff8a4536929240717ae9db62fb75374f629cd3046c7192e2b8b4d7344
This avoids hardcoding xprvs and xpubs in the test.
Also use key's parent fingerprint and derivation path while creating descriptors
so that all of it is self-documenting and not dependent on hardcoded values,
making them easy to read and update.
Also use descsum_create function more to avoid hardcoding the descriptor
checksum.
So that util is not dependent on authproxy at all and going
forward authproxy can use util methods.
Reviewing with --color-moved=dimmed-zebra option can be helpful.
Some tests combine multiple equality checks in one `assert`.
Split those checks into separate assertions so failures point at the exact mismatch.
This also removes mixed `==` expressions that the later cleanup should not touch.
76c092ff80 wallet: warn against accidental unsafe older() import (Sjors Provoost)
592157b759 test: move SEQUENCE_LOCKTIME flags to script (Sjors Provoost)
Pull request description:
[BIP 379](https://github.com/bitcoin/bips/blob/master/bip-0379.md) ([Miniscript](https://bitcoin.sipa.be/miniscript/)) allows relative height and time locks that have no consensus meaning in [BIP 68](https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki) (relative timelocks) / [BIP 112](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) (`CHECKSEQUENCEVERIFY`). This is (ab)used by some protocols, e.g. [by Lightning to encode extra data](https://delvingbitcoin.org/t/exploring-extended-relative-timelocks/1818/23), but is unsafe when used unintentionally: `older(65536)` is equivalent to `older(1)`.
This PR emits a warning when `importdescriptors` contains such a descriptor.
The first commit makes `SEQUENCE_LOCKTIME` flags reusable by other tests.
The main commit adds the `ForEachNode` helper to `miniscript.h` which is then used in the `MiniscriptDescriptor` constructor to check for `Fragment::OLDER` with unsafe values. These are stored in `m_warnings`, which the RPC code then collects via `Warnings()`.
It adds both a unit and functional test.
---
A previous version of this PR prevented the import, unless the user opted in with an `unsafe` flag. It also used string parsing in the RPC code.
---
Based on:
- [x] https://github.com/bitcoin/bitcoin/pull/33914
ACKs for top commit:
pythcoiner:
reACK 76c092ff80
achow101:
ACK 76c092ff80
rkrux:
lgtm re-ACK 76c092ff80
brunoerg:
reACK 76c092ff80
Tree-SHA512: 8e944e499bd4a43cc27eeb889f262b499b9b07aa07610f4a415ccb4e34a9110f9946646f446a54ac5bf17494d8d96a89e4a1fa278385db9b950468f27283e17a
BIP 379 allows height and time locks that have no consensus meaning in BIP 68 / BIP 112.
This is used by some protocols like Lightning to encode extra data, but is unsafe when
used unintentionally. E.g. older(65536) is equivalent to older(1).
This commit emits a warning when importing such a descriptor.
It introduces a helper ForEachNode to traverse all miniscript nodes.
664657ed13 bugfix: disallow label for ranged descriptors & allow external non-ranged descriptors to have label (scgbckbone)
Pull request description:
Motivation:
* ranged descriptors MUST not be able to have label (current impl allows it)
* external non-ranged descriptor MUST be able to have label (current impl disallows it, **if** `internal=false` is provided via importdescriptor user data)
Repro steps:
* create blank wallet and import descriptors
* external has `label=test` (not internal)
```
conn = bitcoind.create_wallet(wallet_name=w_name, disable_private_keys=True, blank=True,
passphrase=None, avoid_reuse=False, descriptors=True)
descriptors = [
{
"timestamp": "now",
"label": "test",
"active": True,
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/0/*)#erexmnep",
"internal": False
},
{
"desc": "wpkh([0f056943/84h/1h/0h]tpubDC7jGaaSE66Pn4dgtbAAstde4bCyhSUs4r3P8WhMVvPByvcRrzrwqSvpF9Ghx83Z1LfVugGRrSBko5UEKELCz9HoMv5qKmGq3fqnnbS5E9r/1/*)#ghu8xxfe",
"active": True,
"internal": True,
"timestamp": "now"
},
]
r = conn.importdescriptors(descriptors)
print(r)
```
response:
```
[{'error': {'code': -8,
'message': 'Internal addresses should not have a label'},
'success': False,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
But in above, ONLY external has a label.
If you remove `internal: False` from external descriptor import object - it will import no problem:
```
[{'success': True,
'warnings': ['Range not given, using default keypool range']},
{'success': True,
'warnings': ['Range not given, using default keypool range']}]
```
Even tho it should NOT, as the descriptor is ranged. Current implementation relies on checking user provided data to decide whether desc is ranged.
ACKs for top commit:
achow101:
ACK 664657ed13
rkrux:
lgtm crACK 664657ed13
Tree-SHA512: 9e70aea620019c29950ba417d4ae38d65cd94a4f6fcabbc021d67b031de1c44c27d6f6f5cb7e6950a099eb6e58bed9be764d4c6347195daeccb14a5d95c123b2
785e1407b0 wallet: Use util::Error throughout AddWalletDescriptor (Ava Chow)
Pull request description:
#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.
The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of `ScriptPubKeyMan` that can come out of `AddWalletDescriptor` is a `DescriptorScriptPubKeyMan` anyways.
ACKs for top commit:
Sjors:
utACK 785e1407b0
ryanofsky:
Code review ACK 785e1407b0
furszy:
Code review ACK 785e1407b0
Tree-SHA512: 52a48263c8d4161a8c0419b7289c25b0986f8e3bcd10b639eeeb0b6862d08b6c5e70998d20070ab26b39ecd90ab83dc8b71c65d85f70626282cf8cc6abff50e7
32023 changed AddWalletDescriptor to return util::Error, but did not
change all of the failure cases to do so. This may result in some
callers continuing when there was actually an error. Unify all of the
failure cases to use util::Error so that all callers handle
AddWalletDescriptor errors in the same way.
The encapsulated return type is changed from ScriptPubKeyMan* to
std::reference_wrapper<DescriptorScriptPubKeyMan>. This avoids having a
value that can be interpreted as a bool, and also removes the need to
constantly dynamic_cast the returned value. The only kind of
ScriptPubKeyMan that can come out of AddWalletDescriptor is a
DescriptorScriptPubKeyMan anyways.
Descriptor wallets are already created by default since v23.0, but
since the recent legacy wallet removal this parameter *must* be True
(see commit 9f04e02ffa), i.e. still
passing it wouldn't contain any information for test readers
anymore. So simply drop them in the functional tests in order to
reduce code bloat.
-BEGIN VERIFY SCRIPT-
sed -i 's/, descriptors=True//g' $(git ls-files -- 'test/functional' ':(exclude)test/functional/wallet_backwards_compatibility.py')
sed -i '/descriptors=True,/d' ./test/functional/mempool_persist.py
-END VERIFY SCRIPT-
Removes all legacy wallet specific functional tests.
Also removes the --descriptor and --legacy-wallet options as these are
no longer necessary with the legacy wallet removed.
* do not only check user provided range data to decide whether descriptor is ranged
* properly handle std::optional<bool> when checking if descriptor is internal
21e9d39a37 docs: add release notes for 31603 (brunoerg)
a8b548d75d test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys (brunoerg)
c7afca3d62 test: descriptor: check whitespace into keys (brunoerg)
cb722a3cea descriptor: check whitespace in ParsePubkeyInner (brunoerg)
50856695ef 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 21e9d39a37
darosior:
re-ACK 21e9d39a37
sipa:
utACK 21e9d39a37
Tree-SHA512: 54f48a89a235517e5cdc29a46dceeb7dabbee93c7616a166288ff3f90131808eb0ece43b0797a11fe827a5f7bd51d65e3e75c16789b0a42020934cabb684cc8f
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>
a0abcbd382 doc: Mention multipath specifier (Ava Chow)
0019f61fc5 tests: Test importing of multipath descriptors (Ava Chow)
f97d5c137d wallet, rpc: Allow importdescriptors to import multipath descriptors (Ava Chow)
32dcbca3fb rpc: Allow importmulti to import multipath descriptors correctly (Ava Chow)
64dfe3ce4b wallet: Move internal to be per key when importing (Ava Chow)
1692245525 tests: Multipath descriptors for scantxoutset and deriveaddresses (Ava Chow)
cddc0ba9a9 rpc: Have deriveaddresses derive receiving and change (Ava Chow)
360456cd22 tests: Multipath descriptors for getdescriptorinfo (Ava Chow)
a90eee444c tests: Add unit tests for multipath descriptors (Ava Chow)
1bbf46e2da descriptors: Change Parse to return vector of descriptors (Ava Chow)
0d640c6f02 descriptors: Have ParseKeypath handle multipath specifiers (Ava Chow)
a5f39b1034 descriptors: Change ParseScript to return vector of descriptors (Ava Chow)
0d55deae15 descriptors: Add DescriptorImpl::Clone (Ava Chow)
7e86541f72 descriptors: Add PubkeyProvider::Clone (Ava Chow)
Pull request description:
It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in https://github.com/bitcoin/bitcoin/issues/17190#issuecomment-895515768, it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.
To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor `wpkh(xpub.../0/0/*)` and `wpkh(xpub.../0/1/*)` to represent receive and change addresses, this could be written as `wpkh(xpub.../0/<0;1>/*)`. The multipath specifier is of the form `<NUM;NUM>`. Each `NUM` can have its own hardened specifier, e.g. `<0;1h>` is valid. The multipath specifier can also only appear in one path index in the derivation path.
This results in the parser returning two descriptors. The first descriptor uses the first `NUM` in all pairs present, and the second uses the second `NUM`. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is just `nullptr`.
The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).
Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.
Closes#17190
ACKs for top commit:
darosior:
re-ACK a0abcbd382
mjdietzx:
reACK a0abcbd382
pythcoiner:
reACK a0abcbd
furszy:
Code review ACK a0abcbd
glozow:
light code review ACK a0abcbd382
Tree-SHA512: 84ea40b3fd1b762194acd021cae018c2f09b98e595f5e87de5c832c265cfe8a6d0bc4dae25785392fa90db0f6301ddf9aea787980a29c74f81d04b711ac446c2
Timeout issues where encountered when running functional tests with `--jobs=16 --extended`.
Line in `feature_index_prune.py` took 101.6s, 96.6s, 103.0s across 3 runs on my machine, default limit is 60.
Line in the `wallet_importdescriptors.py --descriptors` took 5.4s, 5.7s, 6.0s across 3 runs.
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.
fe49f06c0e doc: clarify PR 26076 release note (Sjors Provoost)
bd13dc2f46 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 fe49f06c0e
darosior:
re-ACK fe49f06c0e
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.
6a5b348f2e test: test rescanning encrypted wallets (ishaanam)
493b813e17 wallet: ensure that the passphrase is not deleted from memory when being used to rescan (ishaanam)
66a86ebabb wallet: keep track of when the passphrase is needed when rescanning (ishaanam)
Pull request description:
Wallet passphrases are needed to top up the keypool of encrypted wallets
during a rescan. The following RPCs need the passphrase when rescanning:
- `importdescriptors`
- `rescanblockchain`
The following RPCs use the information about whether or not the
passphrase is being used to ensure that full rescans are able to
take place (meaning the following RPCs should not be able to run
if a rescan requiring the wallet to be unlocked is taking place):
- `walletlock`
- `encryptwallet`
- `walletpassphrasechange`
`m_relock_mutex` is also introduced so that the passphrase is not
deleted from memory when the timeout provided in
`walletpassphrase` is up and the wallet is still rescanning.
Fixes#25702, #11249
Thanks to achow101 for coming up with the idea of using a new mutex to solve this issue and for answering related questions.
ACKs for top commit:
achow101:
ACK 6a5b348f2e
hernanmarino:
ACK 6a5b348f2e
furszy:
Tested ACK 6a5b348f
Tree-SHA512: 0b6db692714f6f94594fa47249f5ee24f85713bfa70ac295a7e84b9ca6c07dda65df7b47781a2dc73e5b603a8725343a2f864428ae20d3e126c5b4802abc4ab5
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.
