Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: Make a tr() descriptor by default #22364

Merged
merged 4 commits into from Nov 22, 2021

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Jun 28, 2021

Make a tr() descriptor by default in descriptor wallets so that users will be able to make and use segwit v1 bech32m addresses.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 29, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23544 (rpc, wallet: addhdseed, infer seed when importing descriptor with xpub by Sjors)
  • #23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
  • #22558 (psbt: Taproot fields for PSBT by achow101)
  • #22341 (rpc: add getxpub by Sjors)
  • #10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@Sjors
Copy link
Member

Sjors commented Jun 29, 2021

The first couple of commits can be merged earlier with a separate PR.

I should try how this interacts with #22260.

Also, it'd be nice to have this on Signet, so explictly check for that chain and then make a followup to do it on testnet and mainnet?

How do you want to go about upgrading existing descriptor wallets? Given that it's still marked experimental, having some manual RPC / wallet tool command would be fine I think. But with future soft-forks such an upgrade should be automatically, just like how we added Segwit addresses to existing wallets.

When I tried getting the master HD key to derive some new private keys in #22341 I got thoroughly confused. It might make more sense to store the seed in the main wallet payload rather than in each SPKman.

@achow101
Copy link
Member Author

The first commit was split to #22512

@michaelfolkson
Copy link
Contributor

Concept ACK

I misunderstood what the "default" aspect of this PR was referring to. As explained to me in the July 30th Core wallet meeting there is no universal "default" descriptor in the wallet, just a default descriptor per address type. This PR is allowing a Taproot descriptor be constructed (and will be merged post Taproot activation) rather than constructing a Taproot descriptor as a universal "default" descriptor.

@achow101 achow101 force-pushed the default-tr-desc branch 2 times, most recently from 20ed3ab to b9f72b3 Compare September 1, 2021 21:53
meshcollider added a commit that referenced this pull request Sep 2, 2021
d9d3ec0 Consolidate XOnlyPubKey lookup hack (Andrew Chow)

Pull request description:

  The places where we need to lookup information for a XOnlyPubKey
  currently implement a hack which makes both serializations of the full
  pubkey in order to try the CKeyIDs for the lookup functions. Instead of
  duplicating this everywhere it is needed, we can consolidate the CKeyID
  generation into a function, and then have wrappers around GetPubKey,
  GetKey, and GetKeyOrigin which takes the XOnlyPubKey, retrieves all of
  the CKeyIDs (using the new GetKeyIDs() function in XOnlyPubKey), and
  tries their respective underlying lookup function.

  Split from #22364

ACKs for top commit:
  S3RK:
    Code Review reACK d9d3ec0
  Zero-1729:
    re-crACK d9d3ec0
  theStack:
    re-ACK d9d3ec0
  meshcollider:
    Code review + functional test run ACK d9d3ec0

Tree-SHA512: 21a7f6d37fad74483a38006f82b3558337fe9ed30e0b4392e6fff82c22251a42ac996b43f06cdaa9289ee34a768e181d87aa4208b5538e36ae4977954e1fa6a0
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 2, 2021
d9d3ec0 Consolidate XOnlyPubKey lookup hack (Andrew Chow)

Pull request description:

  The places where we need to lookup information for a XOnlyPubKey
  currently implement a hack which makes both serializations of the full
  pubkey in order to try the CKeyIDs for the lookup functions. Instead of
  duplicating this everywhere it is needed, we can consolidate the CKeyID
  generation into a function, and then have wrappers around GetPubKey,
  GetKey, and GetKeyOrigin which takes the XOnlyPubKey, retrieves all of
  the CKeyIDs (using the new GetKeyIDs() function in XOnlyPubKey), and
  tries their respective underlying lookup function.

  Split from bitcoin#22364

ACKs for top commit:
  S3RK:
    Code Review reACK d9d3ec0
  Zero-1729:
    re-crACK d9d3ec0
  theStack:
    re-ACK d9d3ec0
  meshcollider:
    Code review + functional test run ACK d9d3ec0

Tree-SHA512: 21a7f6d37fad74483a38006f82b3558337fe9ed30e0b4392e6fff82c22251a42ac996b43f06cdaa9289ee34a768e181d87aa4208b5538e36ae4977954e1fa6a0
@Sjors
Copy link
Member

Sjors commented Oct 20, 2021

I made a branch that combines this PR with #22260, so you get bech32m addresses in the GUI for new descriptor wallets. It seems to work fine.

Given that this PR has to wait for Taproot, I suggest splitting off all commits except 449ce29. Code looks reasonable at first glance, though it's unclear to me what problem 9a86327 and cc1f4c9 are solving.

Suggest rewording the PR title so it's more clear the tr() is created, but does not become the default. We could additionally mention in the release notes that creating a taproot address is possible using the console: getnewaddress label bech32m, and explain that the GUI doesn't do this automatically yet due to lack of broad bech32m support.

@sipa
Copy link
Member

sipa commented Nov 15, 2021

Concept ACK

@laanwj laanwj added this to the 23.0 milestone Nov 15, 2021
@laanwj
Copy link
Member

laanwj commented Nov 15, 2021

How do you want to go about upgrading existing descriptor wallets? Given that it's still marked experimental, having some manual RPC / wallet tool command would be fine I think. But with future soft-forks such an upgrade should be automatically, just like how we added Segwit addresses to existing wallets.

Same question (couldn't find an answer in this PR)—does this add the descriptor to existing wallets, or only to new ones? If not, what is the upgrade command, and should it be added to the release notes?

@achow101
Copy link
Member Author

Same question (couldn't find an answer in this PR)—does this add the descriptor to existing wallets, or only to new ones? If not, what is the upgrade command, and should it be added to the release notes?

This PR only applies to newly created descriptor wallets. Current wallet will need to be upgraded through some method that I haven't quite implemented yet. It will probably be some extension of upgradewallet and make use of #23417.

When expanding the scripts for a TRDescriptor, also store the pubkeys in
the FlatSigningProvider.
@maflcko
Copy link
Member

maflcko commented Nov 16, 2021

why not #22364 (comment) ?

@achow101
Copy link
Member Author

why not #22364 (comment) ?

Forgot, done now.

@maflcko
Copy link
Member

maflcko commented Nov 17, 2021

Concept ACK 4868c9f

@Sjors
Copy link
Member

Sjors commented Nov 17, 2021

re-utACK 4868c9f

Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept + code review ACK 4868c9f

@gruve-p
Copy link
Contributor

gruve-p commented Nov 22, 2021

ACK 4868c9f

@maflcko maflcko merged commit 47fe744 into bitcoin:master Nov 22, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 22, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 23, 2021
@fanquake
Copy link
Member

This was part of 23.0 and got a release note. Removing "Needs release note".

jamesdorfman added a commit to jamesdorfman/elements that referenced this pull request May 26, 2023
delta1 added a commit to delta1/elements that referenced this pull request May 29, 2023
- some fixmes in this commit, related to invalid schnorr signatures
  which need investigation
@bitcoin bitcoin locked and limited conversation to collaborators Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet