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

achow101 commented at 8:48 PM on June 28, 2021: member

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

DrahtBot added the label Descriptors on Jun 28, 2021

DrahtBot added the label Wallet on Jun 28, 2021

DrahtBot commented at 9:34 AM on June 29, 2021: contributor

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.

DrahtBot cross-referenced this on Jun 29, 2021 from issue A few follow-ups for taproot signing by sipa

Sjors commented at 3:03 PM on June 29, 2021: member

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.

bitcoin deleted a comment on Jun 29, 2021

DrahtBot cross-referenced this on Jun 30, 2021 from issue test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke

DrahtBot cross-referenced this on Jun 30, 2021 from issue wallet, refactor: make DescriptorScriptPubKeyMan agnostic of internal flag by S3RK

achow101 force-pushed on Jul 2, 2021

achow101 force-pushed on Jul 3, 2021

DrahtBot cross-referenced this on Jul 3, 2021 from issue Multiprocess bitcoin by ryanofsky

achow101 force-pushed on Jul 6, 2021

achow101 cross-referenced this on Jul 21, 2021 from issue Consolidate XOnlyPubKey lookup hack by achow101

achow101 commented at 2:13 AM on July 21, 2021: member

The first commit was split to #22512

DrahtBot cross-referenced this on Jul 22, 2021 from issue wallet: Decide which coin selection solution to use based on waste metric by achow101

DrahtBot cross-referenced this on Jul 27, 2021 from issue psbt: Taproot fields for PSBT by achow101

DrahtBot cross-referenced this on Jul 28, 2021 from issue test: Implicitly sync after generate* to preempt races and intermittent test failures by MarcoFalke

michaelfolkson commented at 9:59 AM on August 2, 2021: 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.

DrahtBot cross-referenced this on Aug 19, 2021 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky

DrahtBot cross-referenced this on Aug 19, 2021 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky

achow101 force-pushed on Sep 1, 2021

meshcollider referenced this in commit 9a86327512 on Sep 2, 2021

sidhujag referenced this in commit b39bda7377 on Sep 2, 2021

sangaman cross-referenced this on Sep 23, 2021 from issue feat: add initial p2tr output script support by sangaman

DrahtBot cross-referenced this on Sep 29, 2021 from issue test: update fee rate assertion helper in the functional test framework by jonatack

DrahtBot cross-referenced this on Sep 30, 2021 from issue test: Fix intermitent failure in wallet_basic.py by meshcollider

achow101 force-pushed on Oct 1, 2021

Sjors cross-referenced this on Oct 20, 2021 from issue Make descriptor wallets by default by achow101

Sjors commented at 12:30 PM on October 20, 2021: member

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 cc1f4c98008bba507e1e06c46104a1f4dca012f2 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.

Sjors cross-referenced this on Oct 20, 2021 from issue Make bech32m the default for RPC, opt-in for GUI by Sjors

MarcoFalke commented at 11:13 AM on October 22, 2021: member

Anything left to do here to take this out of draft?

achow101 commented at 5:18 PM on October 22, 2021: member

Anything left to do here to take this out of draft?

Waiting for taproot to activate?

Sjors commented at 5:25 PM on October 22, 2021: member

I don't think we should use PR draft status for that. If the code is ready for review, it probably should not be draft. Just a big warning to maintainers not to merge it yet.

achow101 renamed this:
~~wallet: Make a tr() descriptor by default~~
wallet: Make a tr() descriptor by default [DO NOT MERGE UNTIL TAPROOT ACTIVATES]
on Oct 22, 2021

achow101 marked this as ready for review on Oct 22, 2021

jsarenik commented at 3:33 PM on October 26, 2021: none

@achow101 Is the DEFAULT_ADDRESS_TYPE going to be changed in src/wallet/wallet.h? Not part of this PR yet.

achow101 commented at 5:20 PM on October 26, 2021: member

Is the DEFAULT_ADDRESS_TYPE going to be changed in src/wallet/wallet.h? Not part of this PR yet.

Not yet. This would cause issues with wallets which don't have tr() descriptors.

MarcoFalke added the label Needs release note on Oct 27, 2021

in test/functional/wallet_groups.py:123 in 5f89682ccd outdated

 118 | +            tx4_ungrouped_fee = 2820
 119 | +            tx4_grouped_fee = 4160
 120 | +            tx5_6_ungrouped_fee = 5520
 121 | +            tx5_6_grouped_fee = 8240
 122 | +
 123 | +        self.log.info("Test wallet option maxapsfee")

MarcoFalke commented at 9:31 AM on October 27, 2021:

Why is this line duplicated?

Also, it might be good to add an inline comment explaining why descriptor wallet behave differently?

Maybe:

# Descriptor wallets send the change to taproot addresses...?

achow101 commented at 5:15 PM on October 27, 2021:

Oops, fixed. Added a comment.

in test/functional/wallet_descriptor.py:40 in 5f89682ccd outdated

  36 | @@ -37,12 +37,12 @@ def run_test(self):
  37 |          self.log.info("Making a descriptor wallet")
  38 |          self.nodes[0].createwallet(wallet_name="desc1", descriptors=True)
  39 |  
  40 | -        # A descriptor wallet should have 100 addresses * 3 types = 300 keys
  41 | +        # A descriptor wallet should have 100 addresses * 4 types = 300 keys

MarcoFalke commented at 9:31 AM on October 27, 2021:

        # A descriptor wallet should have 100 addresses * 4 types = 400 keys

?

achow101 commented at 5:15 PM on October 27, 2021:

Done

MarcoFalke commented at 9:44 AM on October 27, 2021: member

Concept ACK. Only took a look at the tests and had some questions.

achow101 force-pushed on Oct 27, 2021

MarcoFalke commented at 1:57 PM on October 28, 2021: member

review ACK 407bab94b91858cd3ea27dd96894fb6dfe2735d2 (only looked at the tests, didn't review the wallet changes)

Sjors cross-referenced this on Oct 29, 2021 from issue Address type dropdown, add Taproot (Receive tab) by Sjors

DrahtBot cross-referenced this on Nov 3, 2021 from issue wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101

MarcoFalke commented at 11:02 AM on November 5, 2021: member

nit, if you rebase and push:

diff --git a/src/wallet/test/fuzz/notifications.cpp b/src/wallet/test/fuzz/notifications.cpp
index 252832785b..0811acca13 100644
--- a/src/wallet/test/fuzz/notifications.cpp
+++ b/src/wallet/test/fuzz/notifications.cpp
@@ -68,9 +68,6 @@ struct FuzzedWallet {
     CScript GetScriptPubKey(FuzzedDataProvider& fuzzed_data_provider)
     {
         auto type{fuzzed_data_provider.PickValueInArray(OUTPUT_TYPES)};
-        if (type == OutputType::BECH32M) {
-            type = OutputType::BECH32; // TODO: Setup taproot descriptor and remove this line
-        }
         CTxDestination dest;
         bilingual_str error;
         if (fuzzed_data_provider.ConsumeBool()) {

DrahtBot cross-referenced this on Nov 6, 2021 from issue rpc: add getxpub by Sjors

DrahtBot cross-referenced this on Nov 10, 2021 from issue test: scripted-diff cleanups after generate* changes by MarcoFalke

MarcoFalke renamed this:
~~wallet: Make a tr() descriptor by default [DO NOT MERGE UNTIL TAPROOT ACTIVATES]~~
wallet: Make a tr() descriptor by default
on Nov 14, 2021

MarcoFalke commented at 6:04 AM on November 14, 2021: member

Adjusted title again for 🥕

Sjors commented at 4:08 PM on November 14, 2021: member

tACK 407bab94b91858cd3ea27dd96894fb6dfe2735d2

MarcoFalke cross-referenced this on Nov 15, 2021 from issue policy: Treat taproot as always active by MarcoFalke

sipa commented at 4:40 PM on November 15, 2021: member

Concept ACK

laanwj added this to the milestone 23.0 on Nov 15, 2021

laanwj commented at 8:39 PM on November 15, 2021: member

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 commented at 8:58 PM on November 15, 2021: member

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.

DrahtBot added the label Needs rebase on Nov 16, 2021

Store pubkeys in TRDescriptor::MakeScripts

When expanding the scripts for a TRDescriptor, also store the pubkeys in
the FlatSigningProvider.

54b3699862

achow101 force-pushed on Nov 16, 2021

DrahtBot removed the label Needs rebase on Nov 16, 2021

MarcoFalke commented at 12:20 PM on November 16, 2021: member

why not #22364 (comment) ?

Create a tr() descriptor bech32m DescriptorScriptPubKeyMan by default 8fb57845ee

Mention bech32m in -addresstype and -changetype help d8abbe119c

Extract Taproot internal keyid with GetKeyFromDestination 4868c9f1b3

in src/script/signingprovider.cpp:208 in c51aa4f5df outdated

 207 | @@ -208,5 +208,15 @@ CKeyID GetKeyForDestination(const SigningProvider& store, const CTxDestination&
 208 |              }

MarcoFalke commented at 12:23 PM on November 16, 2021:

Does the comment in this function need an update?

achow101 commented at 5:20 PM on November 16, 2021:

Done

achow101 commented at 5:20 PM on November 16, 2021: member

why not #22364 (comment) ?

Forgot, done now.

achow101 force-pushed on Nov 16, 2021

MarcoFalke commented at 8:28 AM on November 17, 2021: member

Concept ACK 4868c9f1b39f03adee0009cd41d96598b43e8b78

Sjors commented at 1:19 PM on November 17, 2021: member

re-utACK 4868c9f1b39f03adee0009cd41d96598b43e8b78

achow101 cross-referenced this on Nov 17, 2021 from issue trezor: Implement Taproot support by achow101

DrahtBot cross-referenced this on Nov 18, 2021 from issue rpc, wallet: addhdseed, infer seed when importing descriptor with xpub by Sjors

meshcollider commented at 1:58 AM on November 22, 2021: contributor

Concept + code review ACK 4868c9f1b39f03adee0009cd41d96598b43e8b78

gruve-p commented at 8:44 AM on November 22, 2021: contributor

ACK https://github.com/bitcoin/bitcoin/pull/22364/commits/4868c9f1b39f03adee0009cd41d96598b43e8b78

MarcoFalke merged this on Nov 22, 2021

MarcoFalke closed this on Nov 22, 2021

sidhujag referenced this in commit 0150c8ed3b on Nov 22, 2021

sidhujag referenced this in commit d6e373bb0e on Nov 23, 2021

fanquake removed the label Needs release note on Sep 15, 2022

fanquake commented at 3:16 PM on September 15, 2022: member

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

Sjors cross-referenced this on Jan 19, 2023 from issue wallet: Have the wallet store the key for automatically generated descriptors by achow101

jamesdorfman referenced this in commit 03b8ae6f13 on May 26, 2023

delta1 referenced this in commit 00f2e32c8e on May 29, 2023

bitcoin locked this on Sep 15, 2023