descriptors: inference process, do not return unparsable multisig descriptors #31404

1. 
   furszy commented at 4:04 pm on December 2, 2024: member

   The internal script-to-descriptor inference procedure shouldn’t return invalid descriptors or ones that fail the string-to-descriptor parsing process. These two procedures should always support seamless roundtrips.

   This inlines InferScript/InferDescriptor to the actual descriptors ParseScript logic for the multisig path, ensuring only valid descriptors are produced.

   Examples where this presents an issue:

   1. Because the legacy wallet allowed the import of invalid scripts, the process could end up inferring and storing an unparsable descriptor, which crashes the software during any subsequent wallet load.

      Note: This issue will no longer exist after #31378, because we will discard multisig scripts that are consensus-invalid or descriptors-incompatible prior to calling the descriptors inference procedure but fixing this also at the descriptors level prevents future misuses.

   2. The decodescript() RPC command currently return unusable multisig descriptors. For example, providing a P2SH multisig redeem script with more than 520 bytes results in a descriptor when it shouldn’t due to its large size.

2. 
   DrahtBot commented at 4:04 pm on December 2, 2024: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31404.

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #28710 (Remove the legacy wallet and BDB dependency 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.

3. in test/functional/rpc_decodescript.py:101 in 34f42d34ce outdated

    95@@ -96,6 +96,15 @@ def decodescript_script_pub_key(self):
    96         assert_equal('witness_v0_scripthash', rpc_result['segwit']['type'])
    97         assert_equal('0 ' + multisig_script_hash, rpc_result['segwit']['asm'])
    98 
    99+        # Check decoding a multisig redeem script doesn't return an invalid descriptor
   100+        self.log.info("- multisig invalid")
   101+        # Decode a +512 bytes a consensus-invalid p2sh multisig - provided script translates to multi(20, keys)
   

   ---

   ---

   

   sipa commented at 4:07 pm on December 2, 2024:

   The limit is 520, not 512.

   ---

   

   furszy commented at 4:19 pm on December 2, 2024:

   upps, fixed. Thanks
4. in src/script/descriptor.cpp:2244 in 34f42d34ce outdated

   2238@@ -2239,6 +2239,17 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
   2239                 break;
   2240             }
   2241         }
   2242+
   2243+        // Bare multisig pubkeys are limited to 3
   2244+        if (ctx == ParseScriptContext::TOP && providers.size() > 3) {
   

   ---

   ---

   

   sipa commented at 4:07 pm on December 2, 2024:

   Can you introduce a constant for this number 3 (if it doesn’t exist already)?

   ---

   

   furszy commented at 4:20 pm on December 2, 2024:

   Sure. Introduced it in the first commit.
5. refactor: replace hardcoded number, introduce 'MAX_BARE_MULTISIG_PUBKEYS_NUM' 088bbb81e3
6. furszy force-pushed on Dec 2, 2024
7. descriptors: inference process, do not return unparsable descriptors

   The internal script-to-descriptor inference procedure shouldn't return invalid
   descriptors or ones that fail the string-to-descriptor parsing process. These
   two procedures should always support seamless roundtrips.
   
   This inlines `InferScript`/`InferDescriptor` to the actual descriptors
   `ParseScript` logic for the multisig path, ensuring only valid descriptors are
   produced.
   
   Examples where this presents an issue:
   1) Because the legacy wallet allowed the import of invalid scripts, the process
      could end up inferring and storing an unparsable descriptor, which crashes
      the software during any subsequent wallet load
   
   2) The `decodescript()` RPC command currently return unusable multisig descriptors.
      For example, providing a P2SH multisig redeem script with more than 520 bytes
      results in a descriptor when it shouldn't due to its large size.

   50ebd73aee
8. furszy force-pushed on Dec 2, 2024
9. furszy renamed this:
   [RFC] descriptors: inference process, do not return unparsable multisig descriptors
   descriptors: inference process, do not return unparsable multisig descriptors
   on Dec 4, 2024

Contributors
furszy DrahtBot sipa