Some docs and cleanup following #16528.
createwallet that descriptor wallets are experimental.SetCrypted as suggestioned: #16528 (review)m_address_type as mentioned in #18782 (comment)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
nit: I wonder if this can just be simplified from Native Descriptor Wallets to Descriptor Wallets
Done.
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);
just curious, when would this assert?
If the descriptor was an addr(), raw(), or combo() descriptor. But it shouldn't be possible to reach this point with those descriptors.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
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
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"?
I think "determine" is superfluous here.
Fixed.
Fixed.
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
Without those options being set, a Legacy Wallet will be created instead. Additionally the
Fixed.
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
maybe "that" -> "about" ?
It was supposed to be "things that Legacy Wallets do". Fixed
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.
"effect" -> "affect"
Fixed.
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
"take on" -> "take in the" ?
It was supposed to be "take one type of address". fixed
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
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?
I've added a paragraph discussing that case
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) {
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).
utACK 7a7209e
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
handles → handle imports → importing (maybe)
I've changed it to "handle importing scripts and keys"
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
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.
I've tried to rework this section
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
No need for 'just' here IMO
Removed
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
"storing different records and data than legacy wallets" maybe?
Done
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:
Maybe "are disabled for Descriptor Wallets", to highlight that they're disabled for descriptor wallets specifically and not in general.
Done
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.
maybe shorten to "A Legacy wallet could contain…" (also: I'm not sure we should be writing about legacy wallets in past tense yet?)
Done. Also made it present tense.
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
as added to→provided to
This was supposed to say "was added to"
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
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"?
Done
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
"and" seems redundant here
Removed
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),
let's surround the multi(...) with backticks
Done
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
same as above: let's surround wpkh(...) with backticks, to make the nesting of parenthesis a bit less jarring
Done
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
"depending on which are available" maybe?
Done
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
"then" seems redundant here
Removed
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
"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?
I've tried to rework this to explain what it actually does. Also removed a "confusing"
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.
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.
Done
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
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.
Done
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:
re-utACK e765271
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.
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
to -> do
Fixed
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
disabled for descriptor wallets (we haven't totally disabled them)
Done.
we haven't totally disabled them
I sure wish we had.
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.
tACK ca2a09640fe976b1e74a33d29d9381895e71b347
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
utACK ca2a09640fe976b1e74a33d29d9381895e71b347