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

The limit is 520, not 512.

upps, fixed. Thanks

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

Sure. Introduced it in the first commit.

assert_equal('multisig', rpc_result['type'])

Nit: This assertion can also be added. I modified the test case to have 21 keys, and the type changed to nonstandard, which is correct. p2sh and desc values are present in that output.

Do you think it's worth adding the test case with 21 pubkeys as well to cover the nonstandard scenario?

assert_equal('multisig', rpc_result['type'])

Nit: This assertion can also be added. I modified the test case to have 21 keys, and the type changed to nonstandard, which is correct. p2sh and desc values are present in that output.

Do you think it's worth adding the test case with 21 pubkeys as well to cover the nonstandard scenario?

Hmm, I find the type return field misleading. Based on the code, it relates to the output script type, which in this case can only be represented as p2wsh and not as a bare multisig. This is because (1) bare multisig scripts with +3 pubkeys are non-standard, and (2) bare multisig scripts with +15 pubkeys are consensus-invalid.

So, I'm not entirely convinced about the type assertion at this point. It seems we should improve the type field RPC description, correct the RPC to actually return "non-standard" for bare multisigs with +3 pubkeys, and perhaps introduce the "consensus-invalid" (or simply "invalid") type too.

+1 for this doc change.

https://github.com/bitcoin/bitcoin/pull/31404/commits/088bbb81e34dfbe56fdeafaba2ddf756017e86e1: Should probably replace // Support up to x-of-3 multisig txns as standard with // Support up to x-of-MAX_BARE_MULTISIG_PUBKEYS_NUM multisig txns as standard

https://github.com/bitcoin/bitcoin/pull/31404/commits/088bbb81e34dfbe56fdeafaba2ddf756017e86e1: MAX_BARE_MULTISIG_PUBKEYS_NUM is an unsigned int so you can use %u format specifier instead of %d

sure. Applied.

https://github.com/bitcoin/bitcoin/pull/31404/commits/50ebd73aeeb853328d1f4eadba77f2ca80a27a8c: AFAICT At this point, the addmultisigaddress operation is already complete (the scripts have been imported into the wallet and the destination is in the address book), throwing an error here may mislead the user into thinking the operation failed.

If you want the operation to fail if an invalid descriptor is inferred then this code needs to be moved up before the scripts are imported into the wallet, OR, you can just make the descriptor field optional

Sure, good catch. Still, this shouldn't happen because we are creating the destination internally. It is purely a sanity check.

It looks like you have resolved this in https://github.com/bitcoin/bitcoin/pull/31404/commits/e1d6179b61a2d4766c5eb6183a839fcaa6e566fa. Mark this as resolved?

https://github.com/bitcoin/bitcoin/pull/31404/commits/50ebd73aeeb853328d1f4eadba77f2ca80a27a8c: The followig callers of ScriptToUniv need to be updated.

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 4cbe4a6062..5fcb9ff0d2 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -1105,7 +1105,7 @@ static RPCHelpMan gettxout()
                 {RPCResult::Type::STR_AMOUNT, "value", "The transaction value in " + CURRENCY_UNIT},
                 {RPCResult::Type::OBJ, "scriptPubKey", "", {
                     {RPCResult::Type::STR, "asm", "Disassembly of the output script"},
-                    {RPCResult::Type::STR, "desc", "Inferred descriptor for the output"},
+                    {RPCResult::Type::STR, "desc", /*optional=*/true, "Inferred top level descriptor for the script (if any)"},
                     {RPCResult::Type::STR_HEX, "hex", "The raw output script bytes, hex-encoded"},
                     {RPCResult::Type::STR, "type", "The type, eg pubkeyhash"},
                     {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address exists)"},
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 14963190f4..df9a8c2f87 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -505,7 +505,7 @@ static RPCHelpMan decodescript()
                      {RPCResult::Type::STR_HEX, "hex", "The raw output script bytes, hex-encoded"},
                      {RPCResult::Type::STR, "type", "The type of the output script (e.g. witness_v0_keyhash or witness_v0_scripthash)"},
                      {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address exists)"},
-                     {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"},
+                     {RPCResult::Type::STR, "desc", /*optional=*/true, "Inferred top level descriptor for the script (if any)"},
                      {RPCResult::Type::STR, "p2sh-segwit", "address of the P2SH script wrapping this witness redeem script"},
                  }},
             },
@@ -827,7 +827,7 @@ const RPCResult decodepsbt_inputs{
                 {RPCResult::Type::OBJ, "scriptPubKey", "",
                 {
                     {RPCResult::Type::STR, "asm", "Disassembly of the output script"},
-                    {RPCResult::Type::STR, "desc", "Inferred descriptor for the output"},
+                    {RPCResult::Type::STR, "desc", /*optional=*/true, "Inferred top level descriptor for the script (if any)"},
                     {RPCResult::Type::STR_HEX, "hex", "The raw output script bytes, hex-encoded"},
                     {RPCResult::Type::STR, "type", "The type, eg 'pubkeyhash'"},
                     {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined a
ddress exists)"},
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index b1fbc25641..ff23225548 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -1412,7 +1412,7 @@ std::vector<RPCResult> ScriptPubKeyDoc() {
     return
          {
              {RPCResult::Type::STR, "asm", "Disassembly of the output script"},
-             {RPCResult::Type::STR, "desc", "Inferred descriptor for the output"},
+             {RPCResult::Type::STR, "desc", /*optional=*/true, "Inferred top level descriptor for the script (if any)"}
,
              {RPCResult::Type::STR_HEX, "hex", "The raw output script bytes, hex-encoded"},
              {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address 
exists)"},
              {RPCResult::Type::STR, "type", "The type (one of: " + GetAllOutputTypes() + ")"},

Sure. Done as suggested. Thanks.

https://github.com/bitcoin/bitcoin/pull/31404/commits/e1d6179b61a2d4766c5eb6183a839fcaa6e566fa: IIUC trying to infer a multisig redeem script that is too large under a p2sh context will return addr(...)#... instead of nullptr. Is this what you were going for?

IIUC trying to infer a multisig redeem script that is too large under a p2sh context will return addr(...)#... instead of nullptr. Is this what you were going for?

This diff ensures that nullptr is returned under p2sh context (if that's what you were going for) and adds a test case that covers this if block.

diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index a2eb764706..7bc2f8cc88 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -2259,6 +2259,7 @@ std::unique_ptr<DescriptorImpl> InferScript(const CScript& script, ParseScriptCo
         if (provider.GetCScript(scriptid, subscript)) {
             auto sub = InferScript(subscript, ParseScriptContext::P2SH, provider);
             if (sub) return std::make_unique<SHDescriptor>(std::move(sub));
+            return nullptr;
         }
     }
     if (txntype == TxoutType::WITNESS_V0_SCRIPTHASH && (ctx == ParseScriptContext::TOP || ctx == ParseScriptContext::P2SH)) {
diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index 24b8f2f793..4c0c0bc273 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -431,17 +431,19 @@ void CheckMultipath(const std::string& prv,
     }
 }
 
-void CheckInferDescriptor(const std::string& script_hex, const std::string& expected_desc, const std::vector<std::string>& hex_scripts, const std::vector<std::pair<std::string, std::string>>& origin_pubkeys)
+// Helper function to set up a FlatSigningProvider with scripts and public keys for descriptor inference testing
+FlatSigningProvider CreateInferSigningProvider(const std::vector<std::string>& hex_scripts, const std::vector<std::pair<std::string, std::string>>& origin_pubkeys)
 {
-    std::vector<unsigned char> script_bytes{ParseHex(script_hex)};
-    const CScript& script{script_bytes.begin(), script_bytes.end()};
-
     FlatSigningProvider provider;
+    
+    // Add scripts to provider
     for (const std::string& prov_script_hex : hex_scripts) {
         std::vector<unsigned char> prov_script_bytes{ParseHex(prov_script_hex)};
         const CScript& prov_script{prov_script_bytes.begin(), prov_script_bytes.end()};
         provider.scripts.emplace(CScriptID(prov_script), prov_script);
     }
+
+    // Add public keys and their origins to provider
     for (const auto& [pubkey_hex, origin_str] : origin_pubkeys) {
         CPubKey origin_pubkey{ParseHex(pubkey_hex)};
         provider.pubkeys.emplace(origin_pubkey.GetID(), origin_pubkey);
@@ -472,12 +474,32 @@ void CheckInferDescriptor(const std::string& script_hex, const std::string& expe
         }
     }
 
+    return provider;
+}
+
+void CheckInferDescriptor(const std::string& script_hex, const std::string& expected_desc, const std::vector<std::string>& hex_scripts, const std::vector<std::pair<std::string, std::string>>& origin_pubkeys)
+{
+    std::vector<unsigned char> script_bytes{ParseHex(script_hex)};
+    const CScript& script{script_bytes.begin(), script_bytes.end()};
+
+    FlatSigningProvider provider = CreateInferSigningProvider(hex_scripts, origin_pubkeys);
     std::string checksum{GetDescriptorChecksum(expected_desc)};
 
     std::unique_ptr<Descriptor> desc = InferDescriptor(script, provider);
     BOOST_CHECK_EQUAL(desc->ToString(), expected_desc + "#" + checksum);
 }
 
+void CheckNoInferDescriptor(const std::string& script_hex, const std::vector<std::string>& hex_scripts, const std::vector<std::pair<std::string, std::string>>& origin_pubkeys)
+{
+    std::vector<unsigned char> script_bytes{ParseHex(script_hex)};
+    const CScript& script{script_bytes.begin(), script_bytes.end()};
+
+    FlatSigningProvider provider = CreateInferSigningProvider(hex_scripts, origin_pubkeys);
+
+    std::unique_ptr<Descriptor> desc = InferDescriptor(script, provider);
+    BOOST_CHECK(!desc);
+}
+
 }
 
 BOOST_FIXTURE_TEST_SUITE(descriptor_tests, BasicTestingSetup)
@@ -988,6 +1010,9 @@ BOOST_AUTO_TEST_CASE(descriptor_test)
     CheckInferDescriptor("76a914a31725c74421fadc50d35520ab8751ed120af80588ac", "pkh(04c56fe4a92d401bcbf1b3dfbe4ac3dac5602ca155a3681497f02c1b9a733b92d704e2da6ec4162e4846af9236ef4171069ac8b7f8234a8405b6cadd96f34f5a31)", {}, {{"04c56fe4a92d401bcbf1b3dfbe4ac3dac5602ca155a3681497f02c1b9a733b92d704e2da6ec4162e4846af9236ef4171069ac8b7f8234a8405b6cadd96f34f5a31", ""}});
     // Infer pk() from p2pk with uncompressed key
     CheckInferDescriptor("4104032540df1d3c7070a8ab3a9cdd304dfc7fd1e6541369c53c4c3310b2537d91059afc8b8e7673eb812a32978dabb78c40f2e423f7757dca61d11838c7aeeb5220ac", "pk(04032540df1d3c7070a8ab3a9cdd304dfc7fd1e6541369c53c4c3310b2537d91059afc8b8e7673eb812a32978dabb78c40f2e423f7757dca61d11838c7aeeb5220)", {}, {{"04032540df1d3c7070a8ab3a9cdd304dfc7fd1e6541369c53c4c3310b2537d91059afc8b8e7673eb812a32978dabb78c40f2e423f7757dca61d11838c7aeeb5220", ""}});
+
+    // Infer sh(very_large_multisig) from p2sh fails 
+    CheckNoInferDescriptor("a914b0d5c60fe453cd72cd4942db2cfe01b2d084b0ee87", {"01142103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb22103b0da749730dc9b4b1f4a14d6902877a92541f5368778853d9c4a0cb7802dcfb20114ae"}, {});
 }
 
 BOOST_AUTO_TEST_SUITE_END()

That's a more delicate topic. InferScript can return nullptr in valid scenarios too. It shouldn't just fail in every case as you're doing there.

For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a addr(). But with your changes, it would return `nullptr instead.

We need a different way to signal errors from inner InferScript() calls. Could be a boolean or.. a different return object like a util::Result.

For example, if the script is a P2SH output and the provider lacks the redeem script (watch-only scenario), the expected behavior is to return a addr(). But with your changes, it would return `nullptr instead.

Are you certain? I checked this and I couldn't verify this claim. If the redeem script is not available in the provider, provider.GetCScript(scriptid, subscript) should evaluate to false and the expected addr() desc will be returned. I tested this by adding the following test case for the example you gave CheckInferDescriptor("a914b0d5c60fe453cd72cd4942db2cfe01b2d084b0ee87", "addr(3Hp2wZXSKK7Zw2hJEE2pU2Qcs6pPt9t5cG)", {}, {}); and it passed.

Have I missed something?

Rather than making the RPC output field optional, can we not fallback to always returning a raw / addr descriptor if inferring returns something that would be invalid?

Rather than making the RPC output field optional, can we not fallback to always returning a raw / addr descriptor if inferring returns something that would be invalid?

Hmm, I would prefer to avoid returning addr() descriptors when the script cannot be redeemed due to consensus rules. Mainly because it opens up the remote possibility of users sending money to it (since the address is there).

This isn't an issue in this gettxout() case because the script comes from an unspent output in the UTXO or mempool, meaning the worst case scenario is a non-standard script. However, it could be problematic in other places that uses InferDescriptor() like createmultisig() (if we ever feed the internal InferDescriptor call with invalid data) or the current migration process (which will no longer be the case after #31495).

We can return raw / addr for non-standard scripts. This would let us assert that the descriptor always exist at gettxout().