Merge bitcoin/bitcoin#23628: Check descriptors returned by external signers

5493e92 Check descriptors returned by external signers (sstone)

Pull request description:

  Check that descriptors returned by external signers have been parsed properly when creating a new wallet.
  See bitcoin/bitcoin#23627 for context.

  The problem is that parsing an invalid descriptor will return `null` which is not checked for in `CWallet::SetupDescriptorScriptPubKeyMans()`.

  I'm not completely sure what the best fix is since there several strategies for dealing with errors in the current codebase but the proposed fix is very simple and consistent with other validation checks in `CWallet::SetupDescriptorScriptPubKeyMans()`.

ACKs for top commit:
  jamesob:
    Code review ACK bitcoin/bitcoin@5493e92
  achow101:
    ACK 5493e92

Tree-SHA512: 63259f4aa519405a86c554b6813efdb741314bdaa18bf005b70ea8bb92a27abc6e2b65f7c584641dc257fc78a6499f42b51b5310c243e611c4663430dccf3d04

Author: fanquake

src/wallet/wallet.cpp

@@ -3208,8 +3208,11 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
             for (const UniValue& desc_val : descriptor_vals.get_array().getValues()) {
                 std::string desc_str = desc_val.getValStr();
                 FlatSigningProvider keys;
-                std::string dummy_error;
-                std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, dummy_error, false);
+                std::string desc_error;
+                std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, desc_error, false);
+                if (desc == nullptr) {
+                    throw std::runtime_error(std::string(__func__) + ": Invalid descriptor \"" + desc_str + "\" (" + desc_error + ")");
+                }
                 if (!desc->GetOutputType()) {
                     continue;
                 }

test/functional/mocks/invalid_signer.py

@@ -0,0 +1,65 @@
+#!/usr/bin/env python3
+# Copyright (c) 2018-2021 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+import os
+import sys
+import argparse
+import json
+
+def perform_pre_checks():
+    mock_result_path = os.path.join(os.getcwd(), "mock_result")
+    if(os.path.isfile(mock_result_path)):
+        with open(mock_result_path, "r", encoding="utf8") as f:
+            mock_result = f.read()
+        if mock_result[0]:
+            sys.stdout.write(mock_result[2:])
+            sys.exit(int(mock_result[0]))
+
+def enumerate(args):
+    sys.stdout.write(json.dumps([{"fingerprint": "b3c19bfc", "type": "trezor", "model": "trezor_t"}, {"fingerprint": "00000002"}]))
+
+def getdescriptors(args):
+    xpub_pkh = "xpub6CRhJvXV8x2AKWvqi1ZSMFU6cbxzQiYrv3dxSUXCawjMJ1JzpqVsveH4way1yCmJm29KzH1zrVZmVwes4Qo6oXVE1HFn4fdiKrYJngqFFc6"
+    xpub_sh = "xpub6CoNoq3Tg4tGSpom2BSwL42gy864KHo3TXkHxLxBbhvCkgmdVXADQmiHbLZhX3Me1cYhRx7s25Lpm4LnT5zu395ANHsXB2QvT9tqJDAibTN"
+    xpub_wpkh = "xpub6DUcLgY1DfgDy2RV6q4djwwsLitaoZDumbribqrR8mP78fEtgZa1XEsqT5MWQ7gwLwKsTQPT28XLoVE5A97rDNTwMXjmzPaNijoCApCbWvp"
+
+    sys.stdout.write(json.dumps({
+        "receive": [
+            "pkh([b3c19bfc/44'/1'/" + args.account + "']" + xpub_pkh + "/0/*)#h26nxtl9",
+            "sh(wpkh([b3c19bfc/49'/1'/" + args.account + "']" + xpub_sh + "/0/*))#32ry02yp",
+            "wpkh([b3c19bfc/84'/1'/" + args.account + "']" + xpub_wpkh + "/0/*)#jftn8ppv"
+        ],
+        "internal": [
+            "pkh([b3c19bfc/44'/1'/" + args.account + "']" + xpub_pkh + "/1/*)#x7ljm70a",
+            "sh(wpkh([b3c19bfc/49'/1'/" + args.account + "']" + xpub_sh + "/1/*))#ytdjh437",
+            "wpkh([b3c19bfc/84'/1'/" + args.account + "']" + xpub_wpkh + "/1/*)#rawj6535"
+        ]
+    }))
+
+parser = argparse.ArgumentParser(prog='./invalid_signer.py', description='External invalid signer mock')
+parser.add_argument('--fingerprint')
+parser.add_argument('--chain', default='main')
+parser.add_argument('--stdin', action='store_true')
+
+subparsers = parser.add_subparsers(description='Commands', dest='command')
+subparsers.required = True
+
+parser_enumerate = subparsers.add_parser('enumerate', help='list available signers')
+parser_enumerate.set_defaults(func=enumerate)
+
+parser_getdescriptors = subparsers.add_parser('getdescriptors')
+parser_getdescriptors.set_defaults(func=getdescriptors)
+parser_getdescriptors.add_argument('--account', metavar='account')
+
+if not sys.stdin.isatty():
+    buffer = sys.stdin.read()
+    if buffer and buffer.rstrip() != "":
+        sys.argv.extend(buffer.rstrip().split(" "))
+
+args = parser.parse_args()
+
+perform_pre_checks()
+
+args.func(args)

test/functional/wallet_signer.py

@@ -25,6 +25,13 @@ def mock_signer_path(self):
         else:
             return path
 
+    def mock_invalid_signer_path(self):
+        path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'invalid_signer.py')
+        if platform.system() == "Windows":
+            return "py " + path
+        else:
+            return path
+
     def set_test_params(self):
         self.num_nodes = 2
         # The experimental syscall sandbox feature (-sandbox) is not compatible with -signer (which
@@ -48,6 +55,11 @@ def clear_mock_result(self, node):
         os.remove(os.path.join(node.cwd, "mock_result"))
 
     def run_test(self):
+        self.test_valid_signer()
+        self.restart_node(1, [f"-signer={self.mock_invalid_signer_path()}", "-keypool=10"])
+        self.test_invalid_signer()
+
+    def test_valid_signer(self):
         self.log.debug(f"-signer={self.mock_signer_path()}")
 
         # Create new wallets for an external signer.
@@ -187,5 +199,10 @@ def run_test(self):
         # )
         # self.clear_mock_result(self.nodes[4])
 
+    def test_invalid_signer(self):
+        self.log.debug(f"-signer={self.mock_invalid_signer_path()}")
+        self.log.info('Test invalid external signer')
+        assert_raises_rpc_error(-1, "Invalid descriptor", self.nodes[1].createwallet, wallet_name='hww_invalid', disable_private_keys=True, descriptors=True, external_signer=True)
+
 if __name__ == '__main__':
     WalletSignerTest().main()