wallettool: add parameter to create descriptors wallet #20365

S3RK · 2020-11-11T05:02:04Z

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 (comment)

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

DrahtBot · 2020-11-11T12:34:58Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallettool: Add dump and createfromdump commands #19137 (wallettool: Add dump and createfromdump commands by achow101)
test: Extend salvage test for wallet tool #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 · 2020-11-12T12:51:39Z

Concept ACK. cc @achow101

achow101 · 2020-11-12T17:01:32Z

src/wallet/wallettool.cpp

+        spk_man->SetupGeneration(false);
+    } else {
+        wallet_instance->SetupDescriptorScriptPubKeyMans();
+    }


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

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?

re: #20365 (comment)

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

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

@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?

re: #20365 (comment)

@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 · 2020-11-12T17:02:24Z

Concept ACK, could use some tests.

ryanofsky

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

ryanofsky · 2020-12-02T18:37:54Z

src/bitcoin-wallet.cpp

    argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    argsman.AddArg("-wallet=<wallet-name>", "Specify wallet name", ArgsManager::ALLOW_ANY | ArgsManager::NETWORK_ONLY, OptionsCategory::OPTIONS);
    argsman.AddArg("-debug=<category>", "Output debugging information (default: 0).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
+    argsman.AddArg("-descriptors", "Create descriptors wallet. Only for create", ArgsManager::ALLOW_BOOL, OptionsCategory::OPTIONS);


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

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 · 2020-12-02T18:38:17Z

src/wallet/wallettool.cpp

+        spk_man->SetupGeneration(false);
+    } else {
+        wallet_instance->SetupDescriptorScriptPubKeyMans();
+    }


re: #20365 (comment)

@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 · 2020-12-02T22:55:25Z

ACK 173cc9b

ryanofsky · 2020-12-11T15:15:21Z

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?

maflcko

Concept ACK 173cc9b 🌠

Show signature and timestamp

Signature:

Timestamp of file with hash 6bb35783fc2ba72a28707a449591029618cd6ad829e5c0b8651fa1f1655dab27 -

…wallet

489ebb7 wallet: make chain optional for CWallet::Create (Ivan Metlushko) d73ae93 CWallet::Create move chain init message up into calling code (Ivan Metlushko) 44c430f refactor: Add CWallet:::AttachChain method (Russell Yanofsky) e2a47ce refactor: move first run detection to client code (Ivan Metlushko) Pull request description: This is a followup for bitcoin/bitcoin#20365 (comment) First part of a refactoring with overall goal to simplify `CWallet` and de-duplicate code with `wallettool` **Rationale**: split `CWallet::Create` and create `CWallet::AttachChain`. `CWallet::AttachChain` takes chain as first parameter on purpose. In future I suggest we can remove `chain` from `CWallet` constructor. The second commit is based on be164f9cf89b123f03b926aa980996919924ee64 from #15719 (thanks ryanofsky) cc ryanofsky achow101 ACKs for top commit: ryanofsky: Code review ACK 489ebb7. Only changes since last review were adding a const variable declaration, and implementing suggestion not to move feerate option checks to AttachChain. Thanks for updates and fast responses! Tree-SHA512: 00235abfe1b00874c56c449adcab8a36582424abb9ba27440bf750af8f3f217b68c11ca74eb30f78a2109ad1d9009315480effc78345e16a3074a1b5d8128721

Github-Pull: bitcoin#20365 Rebased-From: 6d3af3a

Github-Pull: bitcoin#20365 Rebased-From: 345e88e

Github-Pull: bitcoin#20365 Rebased-From: 173cc9b

S3RK added 2 commits November 11, 2020 11:40

wallettool: pass in DatabaseOptions into MakeWallet

6d3af3a

wallettool: add param to create descriptors wallet

345e88e

fanquake added the Wallet label Nov 11, 2020

DrahtBot mentioned this pull request Nov 11, 2020

wallettool: Add dump and createfromdump commands #19137

Merged

achow101 reviewed Nov 12, 2020

View reviewed changes

test: walettool create descriptors

173cc9b

DrahtBot mentioned this pull request Nov 14, 2020

test: Extend salvage test for wallet tool #19078

Closed

This was referenced Nov 26, 2020

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

ryanofsky approved these changes Dec 2, 2020

View reviewed changes

fanquake requested a review from meshcollider December 13, 2020 03:47

maflcko reviewed Dec 16, 2020

View reviewed changes

maflcko merged commit 69f1ee1 into bitcoin:master Dec 16, 2020

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 17, 2020

Merge bitcoin#20365: wallettool: add parameter to create descriptors …

a97ef5b

…wallet

S3RK deleted the wallet_tool_create_descriptors branch December 26, 2020 15:45

S3RK mentioned this pull request Dec 26, 2020

refactor: split CWallet::Create #20773

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

wallettool: pass in DatabaseOptions into MakeWallet

b4601d9

Github-Pull: bitcoin#20365 Rebased-From: 6d3af3a

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

wallettool: add param to create descriptors wallet

936e66c

Github-Pull: bitcoin#20365 Rebased-From: 345e88e

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

test: walettool create descriptors

c74d6c3

Github-Pull: bitcoin#20365 Rebased-From: 173cc9b

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

wallettool: add parameter to create descriptors wallet #20365

wallettool: add parameter to create descriptors wallet #20365

Uh oh!

Conversation

S3RK commented Nov 11, 2020 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Sjors commented Nov 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Nov 12, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Dec 2, 2020

Uh oh!

ryanofsky commented Dec 11, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

S3RK commented Nov 11, 2020 •

edited by maflcko

Loading

DrahtBot commented Nov 11, 2020 •

edited

Loading