wallet: descriptor wallet release notes and cleanups #18787
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jb55
reviewed
Apr 27, 2020
achow101
force-pushed
the
desc-wallet-followup
branch
from
bfb1ae9 to
2bc2322
Compare
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
flack
reviewed
Apr 28, 2020
flack
reviewed
Apr 28, 2020
sipa
reviewed
Apr 28, 2020
achow101
force-pushed
the
desc-wallet-followup
branch
from
2bc2322 to
7a7209e
Compare
Sjors
approved these changes
Apr 29, 2020
src/wallet/scriptpubkeyman.cpp
Outdated
Member
There was a problem hiding this comment.
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).
laanwj
reviewed
Apr 30, 2020
Member
laanwj
left a comment
laanwj
left a comment
There was a problem hiding this comment.
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 😄
achow101
force-pushed
the
desc-wallet-followup
branch
from
7a7209e to
e765271
Compare
Member
|
re-utACK e765271 |
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 🤷 . Concept ACK on adding release notes. |
Member
|
This has a silent merge conflict with #18699 |
Member
Author
Rebased. |
achow101
force-pushed
the
desc-wallet-followup
branch
from
e765271 to
c6ca17e
Compare
meshcollider
reviewed
May 5, 2020
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.
achow101
force-pushed
the
desc-wallet-followup
branch
from
c6ca17e to
ca2a096
Compare
Member
|
tACK ca2a096 |
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 ca2a096 |
Contributor
|
utACK ca2a096 |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
May 22, 2020
…nups ca2a096 Change SetType to SetInternal and remove m_address_type (Andrew Chow) 89b1ce1 Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan (Andrew Chow) b9073c8 rpc: createwallet warning that descriptor wallets are experimental (Andrew Chow) 610030d docs: Add release notes for descriptor wallets (Andrew Chow) Pull request description: Some docs and cleanup following bitcoin#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: bitcoin#16528 (comment) * Removed `m_address_type` as mentioned in bitcoin#18782 (comment) ACKs for top commit: Sjors: tACK ca2a096 instagibbs: utACK bitcoin@ca2a096 meshcollider: utACK ca2a096 Tree-SHA512: 987188a912c191430e5d3f89bcef54ba6773692fc2d95b16a3ec11d9007ded210466ed980a3857e8b7196beef6422f07f9c85cc157f996c02d16f4dbde2e7b2a
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Feb 23, 2021
Summary: > rpc: createwallet warning that descriptor wallets are experimental > Remove unimplemented SetCrypted from DescriptorScriptPubKeyMan >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. This is a backport of Core [[bitcoin/bitcoin#18787 | PR18787]] It does not include the release notes from the original backport. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D9236
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
11 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some docs and cleanup following #16528.
createwalletthat descriptor wallets are experimental.SetCryptedas suggestioned: Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528 (comment)m_address_typeas mentioned in wallet: Make sure no DescriptorScriptPubKeyMan or WalletDescriptor members are left uninitialized after construction #18782 (comment)