wallet: descriptor wallet release notes and cleanups

achow101 commented at 7:51 pm on April 27, 2020: member

Some docs and cleanup following #16528.

Added release notes to explain a bit of motivation for descriptor wallets, what was changed, and how users will be effected by it. Also mentions the caveats regarding multsigs and watchonly that we have discussed on IRC.
Adds a warning to createwallet that descriptor wallets are experimental.
Removed unused SetCrypted as suggestioned: #16528 (review)
Removed m_address_type as mentioned in #18782 (comment)

fanquake added the label Wallet on Apr 27, 2020

in doc/release-notes-16528.md:9 in 5cab281b45 outdated

0@@ -0,0 +1,108 @@
1+Wallet
2+------
3+
4+### Experimental Descriptor Wallets
5+
6+Please note that Descriptor Wallets are still experimental and not all expected functionality
7+is available. Additionally there may be some bugs and current functions may change in the future.
8+
9+0.21 introduces a new type of wallet - Native Descriptor Wallets. Descriptor Wallets store

jb55 commented at 9:10 pm on April 27, 2020:

nit: I wonder if this can just be simplified from Native Descriptor Wallets to Descriptor Wallets

achow101 commented at 9:27 pm on April 27, 2020:

Done.

in src/wallet/scriptpubkeyman.cpp:1513 in bfb1ae95dc outdated

1509@@ -1510,7 +1510,9 @@ bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDest
1510     {
1511         LOCK(cs_desc_man);
1512         assert(m_wallet_descriptor.descriptor->IsSingleType()); // This is a combo descriptor which should not be an active descriptor
1513-        if (type != m_address_type) {
1514+        Optional<OutputType> desc_addr_type = m_wallet_descriptor.descriptor->GetOutputType();
1515+        assert(desc_addr_type);

jb55 commented at 9:19 pm on April 27, 2020:

just curious, when would this assert?

achow101 commented at 9:28 pm on April 27, 2020:

If the descriptor was an addr(), raw(), or combo() descriptor. But it shouldn’t be possible to reach this point with those descriptors.

achow101 force-pushed on Apr 27, 2020

DrahtBot commented at 11:03 pm on April 27, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

in doc/release-notes-16528.md:13 in 2bc232214c outdated

 8+
 9+0.21 introduces a new type of wallet - Descriptor Wallets. Descriptor Wallets store
10+scriptPubKey information using descriptors. This is in contrast to the Legacy Wallet
11+structure where keys are used to generate scriptPubKeys and addresses. Because of this
12+shift to being script based instead of key based, many of the confusing things that legacy
13+wallets are not possible with Descriptor Wallets. Descriptor Wallets determine use a definition

flack commented at 8:33 am on April 28, 2020:

many of the confusing things that legacy wallets are not possible with Descriptor Wallets

There is a word missing in that sentence. Not exactly sure which one, maybe “the confusing things that legacy wallets does”?

sipa commented at 5:24 pm on April 28, 2020:

I think “determine” is superfluous here.

achow101 commented at 6:58 pm on April 28, 2020:

Fixed.

achow101 commented at 6:58 pm on April 28, 2020:

Fixed.

in doc/release-notes-16528.md:35 in 2bc232214c outdated

30+Setting `descriptors` to `true` will create a Descriptor Wallet instead of a Legacy Wallet.
31+
32+In the GUI, a checkbox has been added to the Create Wallet Dialog do indicate that a
33+Descriptor Wallet should be created.
34+
35+Without those options being set, a Legacy Wallet will be created instead. Additionaly the

flack commented at 8:35 am on April 28, 2020:

0Without those options being set, a Legacy Wallet will be created instead. Additionally the

achow101 commented at 6:58 pm on April 28, 2020:

Fixed.

in doc/release-notes-16528.md:12 in 2bc232214c outdated

 7+is available. Additionally there may be some bugs and current functions may change in the future.
 8+
 9+0.21 introduces a new type of wallet - Descriptor Wallets. Descriptor Wallets store
10+scriptPubKey information using descriptors. This is in contrast to the Legacy Wallet
11+structure where keys are used to generate scriptPubKeys and addresses. Because of this
12+shift to being script based instead of key based, many of the confusing things that legacy

sipa commented at 5:24 pm on April 28, 2020:

maybe “that” -> “about” ?

achow101 commented at 6:59 pm on April 28, 2020:

It was supposed to be “things that Legacy Wallets do”. Fixed

in doc/release-notes-16528.md:17 in 2bc232214c outdated

12+shift to being script based instead of key based, many of the confusing things that legacy
13+wallets are not possible with Descriptor Wallets. Descriptor Wallets determine use a definition
14+of "mine" for scripts which is simpler and more intuitive than that used by Legacy Wallets.
15+Descriptor Wallets also uses different semantics for watch-only things and imports.
16+
17+As Descriptor Wallets are a new type of wallet, their introduction does not effect existing wallets.

sipa commented at 5:25 pm on April 28, 2020:

“effect” -> “affect”

achow101 commented at 6:58 pm on April 28, 2020:

Fixed.

in doc/release-notes-16528.md:50 in 2bc232214c outdated

45+
46+With Descriptor Wallets, descriptors explicitly specify the set of scripts that are owned by
47+the wallet. Since descriptors are deterministic and easily enumerable, users will know exactly
48+what scripts the wallet will consider to belong to it. Additionally the implementation of `IsMine`
49+in Descriptor Wallets is far simpler than for Legacy Wallets. Notably, in Legacy Wallets, `IsMine`
50+allowed for users to take on type of address (e.g. P2PKH), mutate it into another address type

sipa commented at 5:44 pm on April 28, 2020:

“take on” -> “take in the” ?

achow101 commented at 6:59 pm on April 28, 2020:

It was supposed to be “take one type of address”. fixed

in doc/release-notes-16528.md:98 in 2bc232214c outdated

93+Descriptor Wallets move to a split watchonly model. Instead an entire wallet is considered to be
94+watchonly depending on whether it was created with private keys disabled. This eliminates the need
95+to distinguish between things that are watchonly and things that are not within a wallet itself.
96+
97+This change does have a caveat. If a Descriptor Wallet with private keys *enabled* and has
98+both single key (e.g. wpkh(...)) and multiple key descriptors (e.g. multi(...) with only one private

sipa commented at 5:46 pm on April 28, 2020:

Is this inherent to mixed-use descriptor wallets?

For example, if you’d have a DW with private keys enabled and a multi() descriptor with only one private key, then the wallet would always fail to sign transactions, I expect?

achow101 commented at 7:00 pm on April 28, 2020:

I’ve added a paragraph discussing that case

achow101 force-pushed on Apr 28, 2020

in src/wallet/scriptpubkeyman.cpp:1514 in 7a7209ebce outdated

1509@@ -1510,7 +1510,9 @@ bool DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDest
1510     {
1511         LOCK(cs_desc_man);
1512         assert(m_wallet_descriptor.descriptor->IsSingleType()); // This is a combo descriptor which should not be an active descriptor
1513-        if (type != m_address_type) {
1514+        Optional<OutputType> desc_addr_type = m_wallet_descriptor.descriptor->GetOutputType();
1515+        assert(desc_addr_type);
1516+        if (type != *desc_addr_type) {

Sjors commented at 8:13 am on April 29, 2020:

Light preference for using desc_addr_type.value(), which looks nicer and throws an exception if there is no value (though that can’t happen given the assert above).

Sjors approved

Sjors commented at 8:18 am on April 29, 2020: member

utACK 7a7209e

in doc/release-notes-16528.md:61 in 7a7209ebce outdated

56+actually be watching for in outputs. However for the vast majority of users, this change is
57+largely transparent and will not have noticeable effect.
58+
59+#### Imports and Exports
60+
61+Descriptor Wallets handles imports differently from Legacy Wallets. In Legacy Wallets, imports

laanwj commented at 2:11 pm on April 30, 2020:

handles → handle imports → importing (maybe)

achow101 commented at 4:27 pm on April 30, 2020:

I’ve changed it to “handle importing scripts and keys”

in doc/release-notes-16528.md:62 in 7a7209ebce outdated

57+largely transparent and will not have noticeable effect.
58+
59+#### Imports and Exports
60+
61+Descriptor Wallets handles imports differently from Legacy Wallets. In Legacy Wallets, imports
62+are treated separately from the keys generated by the wallet. This separation is part of the

laanwj commented at 2:13 pm on April 30, 2020:

I would reorder this: first mention that it is confusing, then why (that it is part of watch-only behavior). This makes the motivation for changing it more apparent.

achow101 commented at 4:27 pm on April 30, 2020:

I’ve tried to rework this section

in doc/release-notes-16528.md:65 in 7a7209ebce outdated

60+
61+Descriptor Wallets handles imports differently from Legacy Wallets. In Legacy Wallets, imports
62+are treated separately from the keys generated by the wallet. This separation is part of the
63+watchonly behavior for Legacy Wallets and is also confusing, especially in combination with
64+the `IsMine` behavior. With Descriptor Wallets, only complete descriptors can be imported. These
65+descriptors are then just added to the wallet as if it were a descriptor generated by the wallet

laanwj commented at 2:14 pm on April 30, 2020:

No need for ‘just’ here IMO

achow101 commented at 4:27 pm on April 30, 2020:

Removed

in doc/release-notes-16528.md:70 in 7a7209ebce outdated

65+descriptors are then just added to the wallet as if it were a descriptor generated by the wallet
66+itself. To avoid issues with existing import and export RPCs, these have been disabled. To import
67+into a Descriptor Wallet, a new `importdescriptors` RPC has been added that uses a syntax similar
68+to that of `importmulti`.
69+
70+Because Descriptor Wallets are storing different records and data from Legacy Wallets, existing

laanwj commented at 2:17 pm on April 30, 2020:

“storing different records and data than legacy wallets” maybe?

achow101 commented at 4:27 pm on April 30, 2020:

Done

in doc/release-notes-16528.md:74 in 7a7209ebce outdated

69+
70+Because Descriptor Wallets are storing different records and data from Legacy Wallets, existing
71+export RPCs are disabled for Descriptor Wallets. New export RPCs for Descriptor Wallets have
72+not yet been added.
73+
74+The following RPCs have been disabled for Descriptor Wallets:

laanwj commented at 2:18 pm on April 30, 2020:

Maybe “are disabled for Descriptor Wallets”, to highlight that they’re disabled for descriptor wallets specifically and not in general.

achow101 commented at 4:28 pm on April 30, 2020:

Done

in doc/release-notes-16528.md:88 in 7a7209ebce outdated

83+* addmultisigaddress
84+* sethdseed
85+
86+#### Watchonly Wallets
87+
88+In Legacy Wallets, a wallet could contain both private keys and scripts that were being watched.

laanwj commented at 2:20 pm on April 30, 2020:

maybe shorten to “A Legacy wallet could contain…” (also: I’m not sure we should be writing about legacy wallets in past tense yet?)

achow101 commented at 4:28 pm on April 30, 2020:

Done. Also made it present tense.

in doc/release-notes-16528.md:90 in 7a7209ebce outdated

85+
86+#### Watchonly Wallets
87+
88+In Legacy Wallets, a wallet could contain both private keys and scripts that were being watched.
89+Those watched scripts would not contribute to your normal balance. In order to see the watchonly
90+balance and to use watchonly things in transactions, an `include_watchonly` option as added

laanwj commented at 2:21 pm on April 30, 2020:

as added to→provided to

achow101 commented at 4:29 pm on April 30, 2020:

This was supposed to say “was added to”

in doc/release-notes-16528.md:93 in 7a7209ebce outdated

88+In Legacy Wallets, a wallet could contain both private keys and scripts that were being watched.
89+Those watched scripts would not contribute to your normal balance. In order to see the watchonly
90+balance and to use watchonly things in transactions, an `include_watchonly` option as added
91+to many RPCs that would allow users to do that. However this is confusing and easy to mess up.
92+
93+Descriptor Wallets move to a split watchonly model. Instead an entire wallet is considered to be

laanwj commented at 2:24 pm on April 30, 2020:

I’m not sure “split watchonly model” is a good way to call this. I’d associate the legacy way of doing things with “split”, e.g. a single wallet is split into a spendable and a watch-only part. The new wallets are either the one or the other. Maybe “per-wallet watchonly”?

achow101 commented at 4:29 pm on April 30, 2020:

Done

in doc/release-notes-16528.md:97 in 7a7209ebce outdated

92+
93+Descriptor Wallets move to a split watchonly model. Instead an entire wallet is considered to be
94+watchonly depending on whether it was created with private keys disabled. This eliminates the need
95+to distinguish between things that are watchonly and things that are not within a wallet itself.
96+
97+This change does have a caveat. If a Descriptor Wallet with private keys *enabled* and has

laanwj commented at 2:25 pm on April 30, 2020:

“and” seems redundant here

achow101 commented at 4:29 pm on April 30, 2020:

Removed

in doc/release-notes-16528.md:98 in 7a7209ebce outdated

93+Descriptor Wallets move to a split watchonly model. Instead an entire wallet is considered to be
94+watchonly depending on whether it was created with private keys disabled. This eliminates the need
95+to distinguish between things that are watchonly and things that are not within a wallet itself.
96+
97+This change does have a caveat. If a Descriptor Wallet with private keys *enabled* and has
98+a multiple key descriptor without all of the private keys (e.g. multi(...) with only one private key),

laanwj commented at 2:26 pm on April 30, 2020:

let’s surround the multi(...) with backticks

achow101 commented at 4:29 pm on April 30, 2020:

Done

in doc/release-notes-16528.md:103 in 7a7209ebce outdated

 98+a multiple key descriptor without all of the private keys (e.g. multi(...) with only one private key),
 99+then the wallet will fail to sign and broadcast transactions. Such wallets would need to use the PSBT
100+workflow but the typical GUI Send, `sendtoaddress`, etc. workflows would still be available, just
101+non-functional.
102+
103+This issue is worsened if the wallet contains both single key (e.g. wpkh(...)) descriptors and such

laanwj commented at 2:27 pm on April 30, 2020:

same as above: let’s surround wpkh(...) with backticks, to make the nesting of parenthesis a bit less jarring

achow101 commented at 4:29 pm on April 30, 2020:

Done

in doc/release-notes-16528.md:106 in 7a7209ebce outdated

101+non-functional.
102+
103+This issue is worsened if the wallet contains both single key (e.g. wpkh(...)) descriptors and such
104+multiple key descriptors as some transactions could be signed and broadast and others not. This is
105+due to some transactions containing only single key inputs, while others would contain both single
106+key and multiple key inputs, depending on what are available and how the coin selection algorithm

laanwj commented at 2:29 pm on April 30, 2020:

“depending on which are available” maybe?

achow101 commented at 4:29 pm on April 30, 2020:

Done

in doc/release-notes-16528.md:41 in 7a7209ebce outdated

36+Default Wallet created upon first startup of Bitcoin Core will be a Legacy Wallet.
37+
38+#### `IsMine` Semantics
39+
40+`IsMine` refers to the function used to determine whether a script belongs to the wallet.
41+This is then used to determine whether an output belongs to the wallet. Legacy Wallets

laanwj commented at 2:32 pm on April 30, 2020:

“then” seems redundant here

achow101 commented at 4:29 pm on April 30, 2020:

Removed

in doc/release-notes-16528.md:42 in 7a7209ebce outdated

37+
38+#### `IsMine` Semantics
39+
40+`IsMine` refers to the function used to determine whether a script belongs to the wallet.
41+This is then used to determine whether an output belongs to the wallet. Legacy Wallets
42+have a confusing definition of `IsMine` due to using keys. Keys can be involved in a variety

laanwj commented at 2:36 pm on April 30, 2020:

“using keys” is a bit vague to me, all wallets use keys at some level; “due to using a list of keys as their internal representation” maybe?

achow101 commented at 4:30 pm on April 30, 2020:

I’ve tried to rework this to explain what it actually does. Also removed a “confusing”

in doc/release-notes-16528.md:7 in 7a7209ebce outdated

0@@ -0,0 +1,115 @@
1+Wallet
2+------
3+
4+### Experimental Descriptor Wallets
5+
6+Please note that Descriptor Wallets are still experimental and not all expected functionality
7+is available. Additionally there may be some bugs and current functions may change in the future.

laanwj commented at 2:43 pm on April 30, 2020:

Maybe add that users should report any issues or missing functionality (at least if not yet planned) to the issue tracker. After all, the idea behind having experimental functionality in the release is to test it and make sure it fits practical use cases.

achow101 commented at 4:30 pm on April 30, 2020:

Done

in doc/release-notes-16528.md:22 in 7a7209ebce outdated

17+As Descriptor Wallets are a new type of wallet, their introduction does not affect existing wallets.
18+Users who already have a Bitcoin Core wallet can continue to use it as they did before without
19+any change in behavior. Newly created Legacy Wallets (which is the default type of wallet) will
20+behave as they did in previous versions of Bitcoin Core.
21+
22+For users who decide to use Descriptor Wallets, the differences between them and Legacy Wallets

laanwj commented at 2:49 pm on April 30, 2020:

I think this paragraph could be shortened to;

The differences between Descriptor Wallets and Legacy Wallets is largely limited to non user facing things. They are intended to behave similarly except for import/export and watch-only functionality, as elaborated below.

achow101 commented at 4:30 pm on April 30, 2020:

Done

laanwj commented at 2:52 pm on April 30, 2020: member

Thanks for writing this. Some comments on the text (I’m not a native English speaker, so feel free to ignore them where they don’t make sense). I noticed you use the word “confusing” a lot, which sound somewhat condescending, maybe look for some synonym :smile:

achow101 force-pushed on Apr 30, 2020

Sjors commented at 9:32 am on May 4, 2020: member

re-utACK e765271

MarcoFalke commented at 11:39 am on May 4, 2020: member

I see that you use “things” throughout the release notes, and “things” have different meaning based on context, but I couldn’t find a rewrite that wasn’t overly verbose, so :shrug: . Concept ACK on adding release notes.

Sjors commented at 7:42 pm on May 4, 2020: member

This has a silent merge conflict with #18699

achow101 commented at 11:00 pm on May 4, 2020: member

This has a silent merge conflict with #18699

Rebased.

achow101 force-pushed on May 4, 2020

in doc/release-notes-16528.md:33 in 6c90eed51c outdated

28+
29+Descriptor Wallets are not created by default. They must be explicitly created using the
30+`createwallet` RPC or via the GUI. A `descriptors` option has been added to `createwallet`.
31+Setting `descriptors` to `true` will create a Descriptor Wallet instead of a Legacy Wallet.
32+
33+In the GUI, a checkbox has been added to the Create Wallet Dialog do indicate that a

meshcollider commented at 3:36 am on May 5, 2020:

to -> do

achow101 commented at 4:24 am on May 5, 2020:

Fixed

in doc/release-notes-16528.md:76 in 6c90eed51c outdated

71+
72+To import into a Descriptor Wallet, a new `importdescriptors` RPC has been added that uses a syntax
73+similar to that of `importmulti`.
74+
75+As Legacy Wallets and Descriptor Wallets use different mechanisms for importing scripts and keys,
76+the existing import RPCs have been disabled. Because Descriptor Wallets are storing different

meshcollider commented at 3:38 am on May 5, 2020:

disabled for descriptor wallets (we haven’t totally disabled them)

achow101 commented at 4:25 am on May 5, 2020:

Done.

we haven’t totally disabled them

I sure wish we had.

DrahtBot added the label Needs rebase on May 5, 2020

docs: Add release notes for descriptor wallets 610030d95c

rpc: createwallet warning that descriptor wallets are experimental b9073c8f13

Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan 89b1ce1140

Change SetType to SetInternal and remove m_address_type

m_address_type was used for two things:
1. Determine the type of descriptor to generate during
   SetupDescriptorGeneration
2. Sanity check during GetNewDestination.

There is no need to have this variable to accomplish those things.
1. Add a argument to SetupDescriptorGeneration indicating the address
   type to use
2. Use Descriptor::GetOutputType for the sanity check.

ca2a09640f

achow101 force-pushed on May 5, 2020

DrahtBot removed the label Needs rebase on May 5, 2020

Sjors commented at 7:52 pm on May 7, 2020: member

tACK ca2a09640fe976b1e74a33d29d9381895e71b347

instagibbs commented at 12:59 pm on May 21, 2020: member

I have similar feelings to @MarcoFalke re:“things” language but nothing I thought of was similarly concise.

If people really care they’ll ask specific questions I guess.

utACK https://github.com/bitcoin/bitcoin/commit/ca2a09640fe976b1e74a33d29d9381895e71b347

laanwj added this to the "Blockers" column in a project

meshcollider commented at 1:50 am on May 22, 2020: contributor

utACK ca2a09640fe976b1e74a33d29d9381895e71b347

meshcollider merged this on May 22, 2020

meshcollider closed this on May 22, 2020

sidhujag referenced this in commit 5c5677cfef on May 22, 2020

meshcollider removed this from the "Blockers" column in a project

Fabcien referenced this in commit daabc6926d on Feb 23, 2021

DrahtBot locked this on Feb 15, 2022

wallet: descriptor wallet release notes and cleanups #18787

Conflicts