wallet: rpc to add automatically generated descriptors

achow101 commented at 11:17 pm on August 22, 2022: member

It is useful to have a RPC that can create and add the automatically generated descriptors (that are normally made during creation) to a wallet. This is especially useful when a new default descriptor has been implemented as it allows wallets created before that time to quickly and easily add that type of descriptor.

In particular, descriptor wallets created before Taproot was implemented can use the new createwalletdescriptor RPC in order to get a Taproot descriptor.

Furthermore, to keep the newly generated descriptor in line with the existing descriptors, this PR uses #26728 as it exposes a “wallet extended key” for us (in addition to upgrading wallets implemented prior to have a wallet xpub). The newly generated descriptors will use the “wallet extended key” stored in CWallet that PR adds.

createwalletdescriptor is generic and so it can also be used with the other address types. Of course, it given the same wallet extended key, address type, and internal-ness, it will create the same descriptor. So some refactoring has been done in order to detect that the same descriptor is about to be created, and to avoid overwriting any existing descriptors.

DrahtBot added the label Wallet on Aug 23, 2022

DrahtBot commented at 1:18 am on August 23, 2022: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	w0xlt, Sjors
Stale ACK	Rspigler

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#bitcoin-core/gui/733 (Deniability - a tool to automatically improve coin ownership privacy by denavila)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
#29130 (wallet: Add createwalletdescriptor and gethdkeys RPCs for adding new automatically generated descriptors by achow101)
#29124 (wallet: Automatically repair corrupted metadata with doubled derivation path by achow101)
#29016 (RPC: add new listmempooltransactions by niftynei)
#28574 (wallet: optimize migration process, batch db transactions by furszy)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)
#28142 (wallet: Allow users to create a wallet that encrypts all database records by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27792 (wallet: Deniability API (Unilateral Transaction Meta-Privacy) by denavila)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)
#26728 (wallet: Have the wallet store the key for automatically generated descriptors by achow101)
#26008 (wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets by achow101)
#24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)
#22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)
#22341 (rpc: add path to gethdkey by Sjors)

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.

luke-jr commented at 10:58 pm on August 26, 2022: member

I would expect upgradewallet to do this. Is there a reason for the approach here?

in src/wallet/rpc/wallet.cpp:759 in 91dbc85d55 outdated

700@@ -701,6 +701,159 @@ RPCHelpMan simulaterawtransaction()
701     };
702 }
703 
704+static RPCHelpMan setdescriptorhdkey()
705+{
706+    return RPCHelpMan{"setdescriptorhdkey",

luke-jr commented at 11:00 pm on August 26, 2022:

Any reason not to overload sethdseed for this?

Sjors commented at 11:42 am on September 2, 2022:

Same question, couldn’t sethdseed just have an option where it doesn’t add fresh descriptors?

achow101 commented at 8:32 pm on September 19, 2022:

Changed to overload sethdseed.

Sjors commented at 11:04 am on September 22, 2022:

Don’t forget to update the PR description and the relevant error message in createwalletdescriptor.

achow101 commented at 11:06 pm on August 26, 2022: member

I would expect upgradewallet to do this. Is there a reason for the approach here?

The wallet version isn’t modified by this. Also this allows for much more granulated control of what is added. While this can be used for upgrading a wallet to support a new kind of descriptor, it’s not strictly just an upgrade function as it can also be used on blank wallets and after rotating the hd key. So I didn’t think it was appropriate for upgradewallet.

w0xlt commented at 2:00 am on August 27, 2022: contributor

Concept ACK

DrahtBot added the label Needs rebase on Sep 1, 2022

achow101 force-pushed on Sep 1, 2022

DrahtBot removed the label Needs rebase on Sep 1, 2022

in src/wallet/rpc/wallet.cpp:827 in b6b99e0ac5 outdated

825+        "Creates the wallet's descriptor for the given address type. "
826+        "Only works on wallets that contain automatically generated descriptors. "
827+        "The address type must be one that the wallet does not already have a descriptor for."
828+        + HELP_REQUIRING_PASSPHRASE,
829+        {
830+            {"address_type", RPCArg::Type::STR, RPCArg::Optional::NO, "The address type the descriptor will produce"},

Sjors commented at 11:44 am on September 2, 2022:

Maybe list the address types here.

Sjors commented at 11:46 am on September 2, 2022: member

Concept ACK on createwalletdescriptor.

Sjors commented at 10:19 am on September 6, 2022: member

The GUI receive address type dropdown is not updated after calling this.

achow101 force-pushed on Sep 19, 2022

achow101 force-pushed on Sep 20, 2022

achow101 force-pushed on Sep 21, 2022

Sjors commented at 11:26 am on September 22, 2022: member

Suggested testing for other reviewers:

Create a blank descriptor wallet
Call sethdseed false to create a seed without any descriptors (check with listdescriptors)
Call createwalletdescriptor bech32m

The GUI receive address type dropdown is not updated after calling this.

This is still not working (I have to close and reload the wallet).

Will review code soon(tm).

achow101 commented at 3:13 pm on September 22, 2022: member

The GUI receive address type dropdown is not updated after calling this.

This is still not working (I have to close and reload the wallet).

I think fixing that is out of scope for this PR. It’s generally an issue for adding descriptors to a wallet.

achow101 force-pushed on Sep 22, 2022

Rspigler commented at 3:08 am on September 24, 2022: contributor

tACK 6db7d39efef93df728efa4c592b5e63d80409e80

Create a blank descriptor wallet

:heavy_check_mark:

Call sethdseed false to create a seed without any descriptors (check with listdescriptors)

sethdseed

null

listdescriptors

{ “wallet_name”: “pr25907”, “descriptors”: [ ] }

createwalletdescriptor bech32m

{ “descs”: [ “tr([4ad34cda/86’/0’/0’]xpub6BxFBZ3fny6ijY6CUp2zudag83XqefXZZa7CdzxEx6K9kbLg9jLEArhWNaxtWsZ5wRPiAU6mJTDnUgt6uLLzfqyDHudbyjvZPBNBLSvwxhG/0/)#x60zmzje”, “tr([4ad34cda/86’/0’/0’]xpub6BxFBZ3fny6ijY6CUp2zudag83XqefXZZa7CdzxEx6K9kbLg9jLEArhWNaxtWsZ5wRPiAU6mJTDnUgt6uLLzfqyDHudbyjvZPBNBLSvwxhG/1/)#hw2rxhzp” ] }

in src/wallet/rpc/wallet.cpp:506 in 5e749f1d5b outdated

457@@ -458,71 +458,140 @@ static RPCHelpMan unloadwallet()
458 static RPCHelpMan sethdseed()
459 {
460     return RPCHelpMan{"sethdseed",
461-                "\nSet or generate a new HD wallet seed. Non-HD wallets will not be upgraded to being a HD wallet. Wallets that are already\n"
462-                "HD will have a new HD seed set so that new keys added to the keypool will be derived from this new seed.\n"
463-                "\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed." +
464+        "Set or generate a new HD wallet seed or key.\n"
465+        "\nLegacy wallets can only have a seed set. Non-HD Legacy wallets will not be upgraded to being a HD wallet."

Sjors commented at 2:54 pm on October 5, 2022:

5e749f1d5bd8409f4fa1a5073c0340c68e12092c: “can only have a seed set” - did you mean “must already have a seed set”? Also maybe change “to being a HD wallet” to “to HD”.

achow101 commented at 9:28 pm on October 10, 2022:

No, Legacy wallets can only take a HD seed, not a key. This refers to the “or key” of the sentence above.

in src/wallet/rpc/wallet.cpp:508 in 5e749f1d5b outdated

462-                "HD will have a new HD seed set so that new keys added to the keypool will be derived from this new seed.\n"
463-                "\nNote that you will need to MAKE A NEW BACKUP of your wallet after setting the HD wallet seed." +
464+        "Set or generate a new HD wallet seed or key.\n"
465+        "\nLegacy wallets can only have a seed set. Non-HD Legacy wallets will not be upgraded to being a HD wallet."
466+        "Legacy wallets that are already HD will have a new HD seed set so that new keys added to the keypool will be derived from this new seed.\n"
467+        "\nDescriptor wallets can have either a HD seed or a HD key set. The seed or key will only be used for new automatically generated descriptors that are created when newkeypool is true, and with `createwalletdescriptors`.\n"

Sjors commented at 2:56 pm on October 5, 2022:

5e749f1d5bd8409f4fa1a5073c0340c68e12092c: what do you mean with “a HD key set”?

achow101 commented at 9:29 pm on October 10, 2022:

The RPC can accept a “HD key” for descriptor wallets.

Sjors commented at 6:11 pm on October 5, 2022: member

5e749f1d5bd8409f4fa1a5073c0340c68e12092c causes a test failure:

 02022-10-05T18:07:50.143000Z TestFramework (INFO): Test disabled RPCs
 12022-10-05T18:07:50.189000Z TestFramework (ERROR): Assertion failed
 2Traceback (most recent call last):
 3  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main
 4    self.run_test()
 5  File "test/functional/wallet_descriptor.py", line 104, in run_test
 6    assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.sethdseed)
 7  File "/Users/sjors/dev/bitcoin/test/functional/test_framework/util.py", line 130, in assert_raises_rpc_error
 8    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
 9AssertionError: No exception raised
102022-10-05T18:07:50.247000Z TestFramework (INFO): Stopping nodes

It would also be a bit easier to review if you can do the whitespace / documentation cleanup separate from the functionality change. Right now I find it easier to just re-review the whole sethdseed function, though that’s small enough, so I don’t mind.

The other commits look mostly fine to me (as of 6db7d39efef93df728efa4c592b5e63d80409e80).

in src/wallet/rpc/wallet.cpp:878 in d13ebb5a43 outdated

830+        "Only works on wallets that contain automatically generated descriptors. "
831+        "The address type must be one that the wallet does not already have a descriptor for."
832+        + HELP_REQUIRING_PASSPHRASE,
833+        {
834+            {"address_type", RPCArg::Type::STR, RPCArg::Optional::NO, "The address type the descriptor will produce. Options are \"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"."},
835+            {"internal", RPCArg::Type::BOOL, RPCArg::DefaultHint{"Both external and internal will be generated unless this parameter is specified"}, "Whether to only make one descriptor that is internal (if parameter is true) or external (if parameter is false)"},

Sjors commented at 6:17 pm on October 5, 2022:

d13ebb5a43733a61245cdf396304c3ff67b64900: slight preference to drop this. It’s confusing and I personally don’t see any use for it. For advanced use cases it seems better to me if the user manually constructs such descriptors (with help from getxpub #22341). But maybe I missed something.

achow101 commented at 9:31 pm on October 10, 2022:

I am ambivalent but I think it is a useful distinction as it can be used to rotate only specific descriptors rather than complete sets.

in test/functional/wallet_descriptor.py:124 in 6db7d39efe outdated

104@@ -101,7 +105,6 @@ def run_test(self):
105         assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.dumpprivkey, recv_wrpc.getnewaddress())
106         assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.dumpwallet, 'wallet.dump')
107         assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.importwallet, 'wallet.dump')
108-        assert_raises_rpc_error(-4, "Only legacy wallets are supported by this command", recv_wrpc.rpc.sethdseed)

Sjors commented at 6:18 pm on October 5, 2022:

6db7d39efef93df728efa4c592b5e63d80409e80: I guess this was supposed to go in 5e749f1d5bd8409f4fa1a5073c0340c68e12092c

achow101 commented at 9:33 pm on October 10, 2022:

Oops, fixed.

achow101 force-pushed on Oct 10, 2022

achow101 force-pushed on Oct 11, 2022

DrahtBot added the label Needs rebase on Oct 13, 2022

achow101 force-pushed on Oct 17, 2022

DrahtBot removed the label Needs rebase on Oct 17, 2022

DrahtBot added the label Needs rebase on Dec 5, 2022

achow101 force-pushed on Dec 6, 2022

DrahtBot removed the label Needs rebase on Dec 6, 2022

S3RK commented at 12:12 pm on December 18, 2022: contributor

I have a question related to sethdseed. I can see why we would want to be able to set a seed for an empty wallet, this is use case is clear to me. However I don’t fully understand yet the use case for being able to change the seed for existing wallet. Is this intended? If yes, can we better describe the use case for this?

My best guess is that changing seed is needed for key rotation, but I think creating new wallet might actually be a better way to rotate keys. Separate wallets provide much better control over your identities by isolating them and avoiding accidentally mixing them up.

achow101 force-pushed on Dec 19, 2022

achow101 commented at 10:37 pm on December 19, 2022: member

I’ve rebased/reimplemented this on top of #26728. The behavior that this PR adds doesn’t change, just how it actually does it under the hood.

achow101 force-pushed on Dec 19, 2022

achow101 force-pushed on Jan 7, 2023

achow101 force-pushed on Jan 25, 2023

achow101 force-pushed on Jan 26, 2023

achow101 force-pushed on Feb 11, 2023

achow101 commented at 1:50 am on February 11, 2023: member

Per the discussion during the IRC meeting on 2023-01-27, I’ve removed the ability to rotate the active hd key. sethdseed can be used to set one on a blank wallet, but if there is already on set, then it cannot be changed.

achow101 force-pushed on Feb 11, 2023

DrahtBot added the label Needs rebase on Feb 17, 2023

achow101 force-pushed on Feb 17, 2023

DrahtBot removed the label Needs rebase on Feb 17, 2023

BowTiedDeployer commented at 1:00 pm on February 18, 2023: none

Is there a way to create a wallet from seed using bitcoin core?

When using sethdseed and the seed, it says Only legacy wallets are supported by this command. I was interested in creating a wallet using a specific seed to also use it to get the keypair in rust and sign transactions. Please tell me if there is a method at the moment or if the createwalletdescriptor will be the first solution.

Thank you in advance.

DrahtBot added the label Needs rebase on Feb 27, 2023

achow101 force-pushed on Mar 1, 2023

DrahtBot removed the label Needs rebase on Mar 1, 2023

DrahtBot added the label Needs rebase on May 1, 2023

achow101 force-pushed on May 1, 2023

DrahtBot removed the label Needs rebase on May 1, 2023

DrahtBot added the label CI failed on May 1, 2023

DrahtBot removed the label CI failed on May 2, 2023

achow101 force-pushed on May 27, 2023

DrahtBot added the label CI failed on May 30, 2023

DrahtBot removed the label CI failed on May 31, 2023

DrahtBot added the label Needs rebase on Jun 28, 2023

achow101 force-pushed on Jun 28, 2023

DrahtBot removed the label Needs rebase on Jun 28, 2023

achow101 force-pushed on Jun 28, 2023

DrahtBot added the label CI failed on Jun 28, 2023

DrahtBot removed the label CI failed on Jun 29, 2023

DrahtBot added the label Needs rebase on Jul 4, 2023

achow101 force-pushed on Jul 4, 2023

DrahtBot removed the label Needs rebase on Jul 4, 2023

DrahtBot added the label CI failed on Jul 4, 2023

DrahtBot removed the label CI failed on Jul 6, 2023

DrahtBot added the label Needs rebase on Sep 6, 2023

achow101 force-pushed on Sep 7, 2023

DrahtBot removed the label Needs rebase on Sep 7, 2023

DrahtBot added the label Needs rebase on Oct 3, 2023

achow101 force-pushed on Oct 3, 2023

DrahtBot removed the label Needs rebase on Oct 3, 2023

DrahtBot added the label Needs rebase on Oct 5, 2023

achow101 force-pushed on Oct 5, 2023

DrahtBot removed the label Needs rebase on Oct 5, 2023

DrahtBot added the label CI failed on Oct 6, 2023

achow101 force-pushed on Oct 24, 2023

achow101 force-pushed on Nov 27, 2023

achow101 commented at 10:00 pm on November 27, 2023: member

#26728 was changed to not be writing the hdkey and hdckey records for backwards compatibility reasons. This PR has added those back in, but with a required wallet flag of WALLET_FLAG_HAS_HDKEY_RECORDS to prevent the downgrade issue described in #26728 (comment)

This flag will only be set when sethdseed is used with a descriptor wallet. createwalletdescriptor will should still work without those records and that flag.

achow101 force-pushed on Nov 27, 2023

DrahtBot removed the label CI failed on Nov 27, 2023

Sjors commented at 1:30 pm on November 28, 2023: member

This new approach makes sense to me. However, IIUC this PR doesn’t require the new sethdseed RPC, so maybe move that to its own PR? (personally I care a lot less about seed rotation than about being able to add taproot descriptors to an existing descriptor wallet)

achow101 commented at 1:51 pm on November 28, 2023: member

However, IIUC this PR doesn’t require the new sethdseed RPC, so maybe move that to its own PR?

It’s mainly there for the test to work.

DrahtBot added the label Needs rebase on Nov 28, 2023

achow101 force-pushed on Nov 28, 2023

DrahtBot removed the label Needs rebase on Nov 28, 2023

achow101 force-pushed on Dec 5, 2023

Sjors commented at 11:21 am on December 7, 2023: member

You could also test createwalletdescriptor by creating a descriptor wallet using a pre-taproot version. You can check the xpub in the resulting descriptor using #22341. That way all the key rotation stuff and the new records can be done in another PR.

In any case, we should probably get the main PR in first…

DrahtBot added the label Needs rebase on Dec 8, 2023

achow101 force-pushed on Dec 9, 2023

DrahtBot removed the label Needs rebase on Dec 9, 2023

achow101 force-pushed on Dec 11, 2023

achow101 commented at 8:00 pm on December 11, 2023: member

I’ve split the sethdseed changes into #29054

walletdb: Read and write activehdkey record 8bb7a8b06f

wallet: Load activehdkey and its private keys

The activehdkey record will now be loaded into the wallet as its own
member variable. Since the private keys will always be in existing
descriptors in the wallet, the key for the active hd key is also pulled
out of those descriptors on loading.

2e840bcfc0

wallet: Store master key used for automatically generated descriptors 2e2545c3a2

wallet: Retrieve active hd pubkey 4569cb1666

key: Add constructor for CExtKey that takes CExtPubKey and CKey

We often need to construct a CExtKey given an CExtPubKey and CKey, so
implement a constructor that does that for us.

116a4252d6

wallet: Implement GetActiveHDPrivkey 77408c0bdb

wallet, rpc: Add gethdkey RPC 9b760c1049

descriptor: Be able to get the pubkeys involved in a descriptor 5800b75a54

wallet: Always set WALLET_FLAG_GLOBAL_HD_KEY for new wallets 4a82d27872

wallet, rpc: Check WALLET_FLAG_GLOBAL_HD_KEY in gethdkey bb02b9eb94

wallet: Automatically upgrade a wallet to have global hd key ce5495a4b2

tests: Test for gethdkey 9ac7fe7da8

test: Test automatic upgrade of descriptor wallets to have hd key 5679b4a885

wallet: Set global hd key for migrated wallets 18879984d1

test: Also do backwards compat test with encrypted wallets

Best reviewed with `git show -w`

af3667a13c

walletdb: Check that unencrypted and crypted keys are mutually exclusive 1bbbb732b5

test: Check that no unencrypted records persist after encrypting 291cb32237

DrahtBot added the label CI failed on Dec 12, 2023

achow101 force-pushed on Dec 19, 2023

DrahtBot removed the label CI failed on Dec 19, 2023

test: add coverage for re-opening a downgraded encrypted wallet on master

The test creates a wallet on master, downgrades and encrypts the wallet.
Then, it tries to open it again on master.

db6b61e9e7

wallet: Refactor function for single DescSPKM setup

We will need access to a function that sets up a singular
DescriptorSPKM, so refactor this out of the multiple DescriptorSPKM
setup function.

f4ebc290b6

wallet, descspkm: Refactor wallet descriptor generation to static func 7d724083ae

wallet, rpc: Add createwalletdescriptor RPC 41a7904522

wallet: Test createwalletdescriptor 8123f936d1

achow101 force-pushed on Dec 19, 2023

achow101 commented at 0:06 am on January 6, 2024: member

Superseded by #29130

I don’t think there’s a reason to come back to this design, but the branch won’t be deleted so we can reopen if desired.

achow101 closed this on Jan 6, 2024

wallet: rpc to add automatically generated descriptors #25907

Code Coverage

Reviews

Conflicts