Check descriptors returned by external signers #23628

pull sstone wants to merge 1 commits into bitcoin:master from sstone:external-signer-catch-invalid-scriptors changing 3 files +87 −2
  1. sstone commented at 3:12 PM on November 29, 2021: contributor

    Check that descriptors returned by external signers have been parsed properly when creating a new wallet. See #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().

  2. laanwj added the label Wallet on Nov 29, 2021
  3. sstone force-pushed on Nov 29, 2021
  4. DrahtBot commented at 5:03 PM on November 29, 2021: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #22838 (descriptors: Be able to specify change and receiving in a single descriptor string by achow101)

    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.

  5. sstone force-pushed on Nov 29, 2021
  6. sstone force-pushed on Nov 29, 2021
  7. fanquake requested review from Sjors on Dec 2, 2021
  8. in src/wallet/wallet.cpp:3209 in 0221b2cb47 outdated 
    3204 | @@ -3205,6 +3205,9 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3205 |                  FlatSigningProvider keys;
3206 |                  std::string dummy_error;
3207 |                  std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, dummy_error, false);
3208 | +                if (desc == NULL) {
3209 | +                    throw std::runtime_error(std::string(__func__) + ": Invalid descriptor");
    Sjors commented at 3:30 AM on December 6, 2021:

    Adding desc_str to the result would be useful, so the user doesn't have to manually call HWI and doesn't have to guess which of the descriptors failed.

    You could rename dummy_error to desc_error and include that in the message too.

  9. in test/functional/test_runner.py:118 in 0221b2cb47 outdated 
     114 | @@ -115,6 +115,7 @@
 115 |      'feature_taproot.py',
 116 |      'rpc_signer.py',
 117 |      'wallet_signer.py --descriptors',
 118 | +    'wallet_signer.py --descriptors --invalid',
    Sjors commented at 3:34 AM on December 6, 2021:

    Let's not fill this file up with too many options. wallet_signer.py can test both scenario's without needing an argument.

  10. in test/functional/wallet_signer.py:27 in 0221b2cb47 outdated 
      23 | +        parser.add_argument("--invalid", dest="invalid", default=False, action="store_true",
  24 | +                            help="Test invalid external signer")
  25 | +
  26 |      def mock_signer_path(self):
  27 | -        path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'signer.py')
  28 | +        path = os.path.join(os.path.dirname(os.path.realpath(__file__)), 'mocks', 'invalid_signer.py' if self.options.invalid else 'signer.py')
    Sjors commented at 3:35 AM on December 6, 2021:

    Adding a new invalid_signer.py is fine with me, though an alternative approach would be to keep one mock, and then use the device fingerprint to decide the behavior.

    In that case you would expand the enumerate mock to add a signer with e.g. "fingerprint": "00000003", and then getdescriptors would return an invalid descriptor for 00000003.

    sstone commented at 12:56 PM on December 8, 2021:

    I could not get that to work, it seems that it is not supported yet ? (see https://github.com/bitcoin/bitcoin/blob/master/src/wallet/external_signer_scriptpubkeyman.cpp#L47) so instead I re-start the node and change signer path to point to the invalid signer mock.

    Sjors commented at 2:54 AM on December 9, 2021:

    createwallet doesn't let you specify a fingerprint, so this workaround is probably fine for now.

  11. in test/functional/mocks/invalid_signer.py:59 in 0221b2cb47 outdated 
      54 | +parser_getdescriptors.add_argument('--account', metavar='account')
  55 | +
  56 | +if not sys.stdin.isatty():
  57 | +    buffer = sys.stdin.read()
  58 | +    if buffer and buffer.rstrip() != "":
  59 | +        sys.argv.extend(buffer.rstrip().split(" "))
    ysangkok commented at 3:00 AM on December 8, 2021:

    python3 -c "import sys; print(type(sys.stdin))" reports that this is a TextIOWrapper (even when I pipe something in such that stdin isn't a tty), so that BufferedIOBase.read will be used, which cannot return None. Why not buffer = sys.stdin.read().rstrip(), then? Then it wouldn't be necessary to strip it twice. And if buffer and buffer.rstrip() != "" could be simply if buffer or maybe if buffer != "" if you prefer to be verbose.

    sstone commented at 12:58 PM on December 8, 2021:

    This was copied from the original https://github.com/bitcoin/bitcoin/blob/master/test/functional/mocks/signer.py code.

  12. sstone force-pushed on Dec 8, 2021
  13. in src/wallet/wallet.cpp:3208 in a5f53c3694 outdated 
    3202 | @@ -3203,8 +3203,11 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3203 |              for (const UniValue& desc_val : descriptor_vals.get_array().getValues()) {
3204 |                  std::string desc_str = desc_val.getValStr();
3205 |                  FlatSigningProvider keys;
3206 | -                std::string dummy_error;
3207 | -                std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, dummy_error, false);
3208 | +                std::string desc_error;
3209 | +                std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, desc_error, false);
3210 | +                if (desc == NULL) {
    achow101 commented at 1:55 PM on December 8, 2021:

    In a5f53c36942bf82c056380df6c15adf0b12190f2 "Check descriptors returned by external signers"

    nit: nullptr is preferred over NULL

  14. in test/functional/wallet_signer.py:60 in a5f53c3694 outdated 
      55 | @@ -48,6 +56,11 @@ def clear_mock_result(self, node):
  56 |          os.remove(os.path.join(node.cwd, "mock_result"))
  57 |  
  58 |      def run_test(self):
  59 | +        self.test_valid_signer()
  60 | +        self.restart_node(0, ["-signer={self.mock_invalid_signer_path()}", "-keypool=10"])
    achow101 commented at 1:56 PM on December 8, 2021:

    In a5f53c36942bf82c056380df6c15adf0b12190f2 "Check descriptors returned by external signers"

    Missing leading f for f-string. This line should also be inside of test_invalid_signer rather than in the caller. Also, the wrong node is being restarted, it should be node 1.

            self.restart_node(1, [f"-signer={self.mock_invalid_signer_path()}", "-keypool=10"])
  15. in test/functional/wallet_signer.py:213 in a5f53c3694 outdated 
     208 | +        assert_raises_rpc_error(-4, "Private keys must be disabled when using an external signer", self.nodes[1].createwallet, wallet_name='not_hww_invalid', disable_private_keys=False, descriptors=True, external_signer=True)
 209 | +        if self.is_bdb_compiled():
 210 | +            assert_raises_rpc_error(-4, "Descriptor support must be enabled when using an external signer", self.nodes[1].createwallet, wallet_name='not_hww_invalid', disable_private_keys=True, descriptors=False, external_signer=True)
 211 | +        else:
 212 | +            assert_raises_rpc_error(-4, "Compiled without bdb support (required for legacy wallets)", self.nodes[1].createwallet, wallet_name='not_hww_invalid', disable_private_keys=True, descriptors=False, external_signer=True)
 213 | +
    achow101 commented at 1:58 PM on December 8, 2021:

    In a5f53c36942bf82c056380df6c15adf0b12190f2 "Check descriptors returned by external signers"

    There's no need to duplicate these tests.

  16. in test/functional/wallet_signer.py:218 in a5f53c3694 outdated 
     213 | +
 214 | +        self.log.info('Test invalid external signer')
 215 | +        try:
 216 | +            self.nodes[1].createwallet(wallet_name='hww_invalid', disable_private_keys=True, descriptors=True, external_signer=True)
 217 | +        except JSONRPCException:
 218 | +            pass
    achow101 commented at 2:05 PM on December 8, 2021:

    In a5f53c36942bf82c056380df6c15adf0b12190f2 "Check descriptors returned by external signers"

    This isn't testing the invalid behavior. It should also use assert_raises_rpc_error.

            assert_raises_rpc_error(-1, "Invalid descriptor", self.nodes[1].createwallet, wallet_name='hww_invalid', disable_private_keys=True, descriptors=True, external_signer=True)
  17. in test/functional/wallet_signer.py:232 in a5f53c3694 outdated 
     227 | +        # TODO: Handle error thrown by script
 228 | +        # self.set_mock_result(self.nodes[1], "2")
 229 | +        # assert_raises_rpc_error(-1, 'Unable to parse JSON',
 230 | +        #     self.nodes[1].createwallet, wallet_name='not_hww2', disable_private_keys=True, descriptors=True, external_signer=False
 231 | +        # )
 232 | +        # self.clear_mock_result(self.nodes[1])
    achow101 commented at 2:06 PM on December 8, 2021:

    In a5f53c36942bf82c056380df6c15adf0b12190f2 "Check descriptors returned by external signers"

    It is not necessary to duplicate these tests.

  18. sstone force-pushed on Dec 8, 2021
  19. achow101 commented at 11:41 PM on December 8, 2021: member

    ACK 96846ef7817257326d51c4f05047ca46c3f7f3c7

  20. fanquake requested review from Sjors on Dec 9, 2021
  21. in src/wallet/wallet.cpp:3209 in 96846ef781 outdated 
    3202 | @@ -3203,8 +3203,11 @@ void CWallet::SetupDescriptorScriptPubKeyMans()
3203 |              for (const UniValue& desc_val : descriptor_vals.get_array().getValues()) {
3204 |                  std::string desc_str = desc_val.getValStr();
3205 |                  FlatSigningProvider keys;
3206 | -                std::string dummy_error;
3207 | -                std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, dummy_error, false);
3208 | +                std::string desc_error;
3209 | +                std::unique_ptr<Descriptor> desc = Parse(desc_str, keys, desc_error, false);
3210 | +                if (desc == nullptr) {
3211 | +                    throw std::runtime_error(std::string(__func__) + ": Invalid descriptor '" + desc_str + "' (" + desc_error + ")");
    Sjors commented at 3:02 AM on December 9, 2021:

    nit: use a double quote: Invalid descriptor \"" because descriptors can already have ' in them.

  22. Sjors approved
  23. Sjors commented at 3:02 AM on December 9, 2021: member

    tACK 96846ef7817257326d51c4f05047ca46c3f7f3c7

  24. Check descriptors returned by external signers
    Check that descriptors returned by external signers have been parsed properly when creating a new wallet.
    5493e92501
  25. sstone force-pushed on Dec 9, 2021
  26. jamesob commented at 3:02 PM on December 9, 2021: member

    Code review ACK https://github.com/bitcoin/bitcoin/pull/23628/commits/5493e925013245d5ad0f7ea8784fe07f531803d0

    Nice first contribution! Fix is straightforward and well-covered by tests. I did not test this locally.

  27. achow101 commented at 4:34 PM on December 9, 2021: member

    ACK 5493e925013245d5ad0f7ea8784fe07f531803d0

  28. fanquake merged this on Dec 10, 2021
  29. fanquake closed this on Dec 10, 2021
  30. sidhujag referenced this in commit b54e03ff54 on Dec 10, 2021
  31. RandyMcMillan referenced this in commit d6f13712dd on Dec 23, 2021
  32. DrahtBot locked this on Dec 10, 2022
Contributors
sstoneDrahtBotSjorsysangkokachow101jamesob
Labels
Wallet
Linked (view graph)
#23627 In "external signer mode", invalid descriptors will crash Bitcoin Core
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me