wallet: Make a tr() descriptor by default

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.

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

achow101 force-pushed on Jul 2, 2021

achow101 force-pushed on Jul 3, 2021

achow101 force-pushed on Jul 6, 2021

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

The first commit was split to #22512

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.

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

achow101 force-pushed on Oct 1, 2021

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.

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:

0# 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:

0        # 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)

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

nit, if you rebase and push:

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

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

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

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

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

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

Conflicts