Merge #20365: wallettool: add parameter to create descriptors wallet

173cc9b test: walettool create descriptors (Ivan Metlushko)
345e88e wallettool: add param to create descriptors wallet (Ivan Metlushko)
6d3af3a wallettool: pass in DatabaseOptions into MakeWallet (Ivan Metlushko)

Pull request description:

  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** bitcoin/bitcoin#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
  ```

ACKs for top commit:
  achow101:
    ACK 173cc9b
  ryanofsky:
    Code review ACK 173cc9b. This seems pretty nicely implemented now, with opportunities to clean up more and dedup later
  MarcoFalke:
    Concept ACK 173cc9b 🌠

Tree-SHA512: cc32ba336ff709de2707ee15f495b4617908e8700ede8401a58e894f44cda485c544d644023c9a6604d88a62db9d92152383ee2e8abf691688c25cf6e222c622

Author: MarcoFalke

src/bitcoin-wallet.cpp

@@ -28,6 +28,7 @@ static void SetupWalletToolArgs(ArgsManager& argsman)
     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);
     argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -debug is true, 0 otherwise).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
 
     argsman.AddArg("info", "Get wallet info", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);

src/wallet/wallettool.cpp

@@ -21,30 +21,27 @@ static void WalletToolReleaseWallet(CWallet* wallet)
     delete wallet;
 }
 
-static void WalletCreate(CWallet* wallet_instance)
+static void WalletCreate(CWallet* wallet_instance, uint64_t wallet_creation_flags)
 {
     LOCK(wallet_instance->cs_wallet);
 
     wallet_instance->SetMinVersion(FEATURE_HD_SPLIT);
+    wallet_instance->AddWalletFlags(wallet_creation_flags);
 
-    // generate a new HD seed
-    auto spk_man = wallet_instance->GetOrCreateLegacyScriptPubKeyMan();
-    CPubKey seed = spk_man->GenerateNewSeed();
-    spk_man->SetHDSeed(seed);
+    if (!wallet_instance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
+        auto spk_man = wallet_instance->GetOrCreateLegacyScriptPubKeyMan();
+        spk_man->SetupGeneration(false);
+    } else {
+        wallet_instance->SetupDescriptorScriptPubKeyMans();
+    }
 
     tfm::format(std::cout, "Topping up keypool...\n");
     wallet_instance->TopUpKeyPool();
 }
 
-static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::path& path, bool create)
+static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::path& path, DatabaseOptions options)
 {
-    DatabaseOptions options;
     DatabaseStatus status;
-    if (create) {
-        options.require_create = true;
-    } else {
-        options.require_existing = true;
-    }
     bilingual_str error;
     std::unique_ptr<WalletDatabase> database = MakeDatabase(path, options, status, error);
     if (!database) {
@@ -85,7 +82,7 @@ static std::shared_ptr<CWallet> MakeWallet(const std::string& name, const fs::pa
         }
     }
 
-    if (create) WalletCreate(wallet_instance.get());
+    if (options.require_create) WalletCreate(wallet_instance.get(), options.create_flags);
 
     return wallet_instance;
 }
@@ -110,14 +107,23 @@ bool ExecuteWalletToolFunc(const std::string& command, const std::string& name)
     fs::path path = fs::absolute(name, GetWalletDir());
 
     if (command == "create") {
-        std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, /* create= */ true);
+        DatabaseOptions options;
+        options.require_create = true;
+        if (gArgs.GetBoolArg("-descriptors", false)) {
+            options.create_flags |= WALLET_FLAG_DESCRIPTORS;
+            options.require_format = DatabaseFormat::SQLITE;
+        }
+
+        std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options);
         if (wallet_instance) {
             WalletShowInfo(wallet_instance.get());
             wallet_instance->Close();
         }
     } else if (command == "info" || command == "salvage") {
         if (command == "info") {
-            std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, /* create= */ false);
+            DatabaseOptions options;
+            options.require_existing = true;
+            std::shared_ptr<CWallet> wallet_instance = MakeWallet(name, path, options);
             if (!wallet_instance) return false;
             WalletShowInfo(wallet_instance.get());
             wallet_instance->Close();

test/functional/tool_wallet.py

@@ -28,8 +28,11 @@ def skip_test_if_missing_module(self):
 
     def bitcoin_wallet_process(self, *args):
         binary = self.config["environment"]["BUILDDIR"] + '/src/bitcoin-wallet' + self.config["environment"]["EXEEXT"]
-        args = ['-datadir={}'.format(self.nodes[0].datadir), '-chain=%s' % self.chain] + list(args)
-        return subprocess.Popen([binary] + args, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
+        default_args = ['-datadir={}'.format(self.nodes[0].datadir), '-chain=%s' % self.chain]
+        if self.options.descriptors:
+            default_args.append('-descriptors')
+
+        return subprocess.Popen([binary] + default_args + list(args), stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, universal_newlines=True)
 
     def assert_raises_tool_error(self, error, *args):
         p = self.bitcoin_wallet_process(*args)
@@ -63,6 +66,36 @@ def log_wallet_timestamp_comparison(self, old, new):
         result = 'unchanged' if new == old else 'increased!'
         self.log.debug('Wallet file timestamp {}'.format(result))
 
+    def get_expected_info_output(self, name="", transactions=0, keypool=2, address=0):
+        wallet_name = self.default_wallet_name if name == "" else name
+        output_types = 3  # p2pkh, p2sh, segwit
+        if self.options.descriptors:
+            return textwrap.dedent('''\
+                Wallet info
+                ===========
+                Name: %s
+                Format: sqlite
+                Descriptors: yes
+                Encrypted: no
+                HD (hd seed available): yes
+                Keypool Size: %d
+                Transactions: %d
+                Address Book: %d
+            ''' % (wallet_name, keypool * output_types, transactions, address))
+        else:
+            return textwrap.dedent('''\
+                Wallet info
+                ===========
+                Name: %s
+                Format: bdb
+                Descriptors: no
+                Encrypted: no
+                HD (hd seed available): yes
+                Keypool Size: %d
+                Transactions: %d
+                Address Book: %d
+            ''' % (wallet_name, keypool, transactions, address * output_types))
+
     def test_invalid_tool_commands_and_args(self):
         self.log.info('Testing that various invalid commands raise with specific error messages')
         self.assert_raises_tool_error('Invalid command: foo', 'foo')
@@ -98,33 +131,7 @@ def test_tool_wallet_info(self):
         # shasum_before = self.wallet_shasum()
         timestamp_before = self.wallet_timestamp()
         self.log.debug('Wallet file timestamp before calling info: {}'.format(timestamp_before))
-        if self.options.descriptors:
-            out = textwrap.dedent('''\
-                Wallet info
-                ===========
-                Name: default_wallet
-                Format: sqlite
-                Descriptors: yes
-                Encrypted: no
-                HD (hd seed available): yes
-                Keypool Size: 6
-                Transactions: 0
-                Address Book: 1
-            ''')
-        else:
-            out = textwrap.dedent('''\
-                Wallet info
-                ===========
-                Name: \
-
-                Format: bdb
-                Descriptors: no
-                Encrypted: no
-                HD (hd seed available): yes
-                Keypool Size: 2
-                Transactions: 0
-                Address Book: 3
-            ''')
+        out = self.get_expected_info_output(address=1)
         self.assert_tool_output(out, '-wallet=' + self.default_wallet_name, 'info')
         timestamp_after = self.wallet_timestamp()
         self.log.debug('Wallet file timestamp after calling info: {}'.format(timestamp_after))
@@ -155,33 +162,7 @@ def test_tool_wallet_info_after_transaction(self):
         shasum_before = self.wallet_shasum()
         timestamp_before = self.wallet_timestamp()
         self.log.debug('Wallet file timestamp before calling info: {}'.format(timestamp_before))
-        if self.options.descriptors:
-            out = textwrap.dedent('''\
-                Wallet info
-                ===========
-                Name: default_wallet
-                Format: sqlite
-                Descriptors: yes
-                Encrypted: no
-                HD (hd seed available): yes
-                Keypool Size: 6
-                Transactions: 1
-                Address Book: 1
-            ''')
-        else:
-            out = textwrap.dedent('''\
-                Wallet info
-                ===========
-                Name: \
-
-                Format: bdb
-                Descriptors: no
-                Encrypted: no
-                HD (hd seed available): yes
-                Keypool Size: 2
-                Transactions: 1
-                Address Book: 3
-            ''')
+        out = self.get_expected_info_output(transactions=1, address=1)
         self.assert_tool_output(out, '-wallet=' + self.default_wallet_name, 'info')
         shasum_after = self.wallet_shasum()
         timestamp_after = self.wallet_timestamp()
@@ -199,19 +180,7 @@ def test_tool_wallet_create_on_existing_wallet(self):
         shasum_before = self.wallet_shasum()
         timestamp_before = self.wallet_timestamp()
         self.log.debug('Wallet file timestamp before calling create: {}'.format(timestamp_before))
-        out = textwrap.dedent('''\
-            Topping up keypool...
-            Wallet info
-            ===========
-            Name: foo
-            Format: bdb
-            Descriptors: no
-            Encrypted: no
-            HD (hd seed available): yes
-            Keypool Size: 2000
-            Transactions: 0
-            Address Book: 0
-        ''')
+        out = "Topping up keypool...\n" + self.get_expected_info_output(name="foo", keypool=2000)
         self.assert_tool_output(out, '-wallet=foo', 'create')
         shasum_after = self.wallet_shasum()
         timestamp_after = self.wallet_timestamp()
@@ -237,9 +206,13 @@ def test_getwalletinfo_on_different_wallet(self):
         self.log.debug('Wallet file timestamp after calling getwalletinfo: {}'.format(timestamp_after))
 
         assert_equal(0, out['txcount'])
-        assert_equal(1000, out['keypoolsize'])
-        assert_equal(1000, out['keypoolsize_hd_internal'])
-        assert_equal(True, 'hdseedid' in out)
+        if not self.options.descriptors:
+            assert_equal(1000, out['keypoolsize'])
+            assert_equal(1000, out['keypoolsize_hd_internal'])
+            assert_equal(True, 'hdseedid' in out)
+        else:
+            assert_equal(3000, out['keypoolsize'])
+            assert_equal(3000, out['keypoolsize_hd_internal'])
 
         self.log_wallet_timestamp_comparison(timestamp_before, timestamp_after)
         assert_equal(timestamp_before, timestamp_after)
@@ -261,10 +234,9 @@ def run_test(self):
         # Warning: The following tests are order-dependent.
         self.test_tool_wallet_info()
         self.test_tool_wallet_info_after_transaction()
+        self.test_tool_wallet_create_on_existing_wallet()
+        self.test_getwalletinfo_on_different_wallet()
         if not self.options.descriptors:
-            # TODO: Wallet tool needs more create options at which point these can be enabled.
-            self.test_tool_wallet_create_on_existing_wallet()
-            self.test_getwalletinfo_on_different_wallet()
             # Salvage is a legacy wallet only thing
             self.test_salvage()