wallettool: add parameter to create descriptors wallet #20365

S3RK commented at 5:02 AM on November 11, 2020: member

Rationale: expose and promote descriptor wallets in more places; make cli tool more consistent with createwallet rpc.

Add -descriptors parameter which is off by default. When specified it will create a new descriptors wallet with sqlite backend, which is consistent with createwallet rpc.

This PR is based on a suggestion from ryanofsky #19137 (review)

Example:

$ ./src/bitcoin-wallet  -wallet=fewty -descriptors create
Topping up keypool...
Wallet info
===========
Name: fewty
Format: sqlite
Descriptors: yes
Encrypted: no
HD (hd seed available): yes
Keypool Size: 6000
Transactions: 0
Address Book: 0

$ ./src/bitcoin-wallet  -wallet=fewty create
Topping up keypool...
Wallet info
===========
Name: fewty
Format: bdb
Descriptors: no
Encrypted: no
HD (hd seed available): yes
Keypool Size: 2000
Transactions: 0
Address Book: 0

wallettool: pass in DatabaseOptions into MakeWallet 6d3af3ab62

wallettool: add param to create descriptors wallet 345e88eecf

fanquake added the label Wallet on Nov 11, 2020

DrahtBot commented at 12:34 PM on November 11, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19137 (wallettool: Add dump and createfromdump commands by achow101)
#19078 (test: Add salvage test for wallet tool by MarcoFalke)

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 12:51 PM on November 12, 2020: member

Concept ACK. cc @achow101

in src/wallet/wallettool.cpp:36 in 345e88eecf outdated

  36 | +    if (!wallet_instance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
  37 | +        auto spk_man = wallet_instance->GetOrCreateLegacyScriptPubKeyMan();
  38 | +        spk_man->SetupGeneration(false);
  39 | +    } else {
  40 | +        wallet_instance->SetupDescriptorScriptPubKeyMans();
  41 | +    }

achow101 commented at 5:01 PM on November 12, 2020:

For a followup, we should de-duplicate this logic with the setup in CWallet::Create.

S3RK commented at 8:10 AM on November 13, 2020:

That is what I was thinking too. The main challenge is that CWallet::Create requires a chain object which is used in quite a few places. Other than that it does what we need. I have a few ideas how to deal with it:

a) create dummy chain object that would return only zero/null values b) make chain argument optional and check every time before using it c) chip away a piece of CWallet::Create that could be used without chain into a separate function @achow101 What do you think is the best option in your opinion?

ryanofsky commented at 6:55 PM on November 16, 2020:

re: #20365 (review)

Offhand (c) sounds great, (b) would be also be good, and it would be nice to avoid (a). The CWallet class is huge and to the extent you can pull out more functionality into standalone functions that's a good thing. Especially any functionality that does not require access to a chain state and a running node is a good candidate for being pulled out of CWallet.

Commit be164f9cf89b123f03b926aa980996919924ee64 from #15719 is kind of related (best viewed with git log -p -n1 -w --color-moved=dimmed_zebra be164f9cf89b123f03b926aa980996919924ee64 since github display doesn't detect moved code). It moves some online functionality out of CWallet::Create into a new CWallet::AttachChain method.

achow101 commented at 7:02 PM on November 16, 2020:

Agree with @ryanofsky. The best option I think is to break apart CWallet::Create and refactor pieces of it into standalone functions.

S3RK commented at 3:07 AM on November 17, 2020:

@ryanofsky @achow101 thanks for the advice and the reference. I'll start working on the refactoring in separate PR as I think it's better not to mix things up.

I've extended functional tests to cover for the new parameter. Any comments on this PR?

ryanofsky commented at 6:38 PM on December 2, 2020:

re: #20365 (review)

@ryanofsky @achow101 thanks for the advice and the reference. I'll start working on the refactoring in separate PR as I think it's better not to mix things up.

I've extended functional tests to cover for the new parameter. Any comments on this PR?

Looks good!

achow101 commented at 5:02 PM on November 12, 2020: member

Concept ACK, could use some tests.

test: walettool create descriptors 173cc9b7be

in src/bitcoin-wallet.cpp:30 in 345e88eecf outdated

  26 | @@ -27,6 +27,7 @@ static void SetupWalletToolArgs(ArgsManager& argsman)
  27 |      argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
  28 |      argsman.AddArg("-wallet=<wallet-name>", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS);
  29 |      argsman.AddArg("-debug=<category>", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
  30 | +    argsman.AddArg("-descriptors", "Create descriptors wallet. Only for create", ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS);

ryanofsky commented at 6:37 PM on December 2, 2020:

In commit "wallettool: add param to create descriptors wallet" (345e88eecf1b28607d5da3af38e19794a8a115ce)

It would be good to have enforcement in ExecuteWalletToolFunc that option is only allowed for create command so existing "info" and "salvage" commands don't start accepting but ignoring this option now, and so future commands like "createfromdump" don't start accepting it in the future. Something like the following should be sufficient for now (could generalize later):

if (gArgs.IsArgSet("-descriptors") && command != "create") {
   tfm::format(std::cerr, "Invalid parameter %s\n", "-descriptors");
   return false;
}

ryanofsky approved

ryanofsky commented at 6:45 PM on December 2, 2020: member

Code review ACK 173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12. This seems pretty nicely implemented now, with opportunities to clean up more and dedup later

achow101 commented at 10:55 PM on December 2, 2020: member

ACK 173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12

ryanofsky commented at 3:15 PM on December 11, 2020: member

Seems like this might be ready for merge. This is a straightforward change just adding a wallet tool create option. Does it need more review than it's had?

fanquake requested review from meshcollider on Dec 13, 2020

MarcoFalke commented at 4:42 PM on December 16, 2020: member

Concept ACK 173cc9b7be335b5dd2cc1bb112dfa6ec5c13ec12 🌠

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 6bb35783fc2ba72a28707a449591029618cd6ad829e5c0b8651fa1f1655dab27 -

</details>

MarcoFalke merged this on Dec 16, 2020

MarcoFalke closed this on Dec 16, 2020

sidhujag referenced this in commit a97ef5bdae on Dec 17, 2020

S3RK deleted the branch on Dec 26, 2020

laanwj referenced this in commit d4c409cf09 on May 19, 2021

luke-jr referenced this in commit b4601d99fc on Jun 27, 2021

luke-jr referenced this in commit 936e66c0a9 on Jun 27, 2021

luke-jr referenced this in commit c74d6c32f7 on Jun 27, 2021

DrahtBot locked this on Feb 15, 2022