Support up to 20 keys for multisig under Segwit context #20867

darosior · 2021-01-06T14:12:05Z

As described in #20620 multisigs are currently limited to 16 keys in descriptors and RPC helpers, even for P2WSH and P2SH-P2WSH.

This adds support for multisig with up to 20 keys (which are already standard) for Segwit v0 context for descriptors (wsh(), sh(wsh())) and RPC helpers.

Fixes #20620

src/rpc/util.cpp

src/script/descriptor.cpp

src/script/standard.cpp

DrahtBot · 2021-01-06T18:56:52Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

wallet: importdescriptors update existing #19651 (wallet: importdescriptors update existing by S3RK)

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.

src/script/standard.cpp

sipa · 2021-01-07T00:29:29Z

Some thing to test:

Descriptors for P2WSH or P2SH-P2WSH multisig can be expanded, signed for, and those spends relay if keys <= 20.
Descriptors for P2SH multisig can be expanded, signed for, and those spends relay if keys <= 15 (if compressed) or <= 7 (if uncompressed).
Inferring descriptor from a multisig script with keys <= 16, but using a direct push rather than OP_n, fails.

src/script/standard.cpp

darosior · 2021-01-09T19:26:56Z

Descriptors for P2WSH or P2SH-P2WSH multisig can be expanded, signed for, and those spends relay if keys <= 20.

I think this one is adressed in the functional tests commit ?

Descriptors for P2SH multisig can be expanded, signed for, and those spends relay if keys <= 15 (if compressed) or <= 7 (if uncompressed).

Added a P2SH multisig standardness check in the functional tests commit.

See the diff

diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index 2cd185720..292ada606 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -459,7 +459,7 @@ class ImportDescriptorsTest(BitcoinTestFramework):
         assert_equal(tx_signed_2['complete'], True)
         self.nodes[1].sendrawtransaction(tx_signed_2['hex'])
 
-        self.log.info("We can create and use a huge multisig")
+        self.log.info("We can create and use a huge multisig under P2WSH")
         self.nodes[1].createwallet(wallet_name='wmulti_priv_big', blank=True, descriptors=True)
         wmulti_priv_big = self.nodes[1].get_wallet_rpc('wmulti_priv_big')
         res = wmulti_priv_big.importdescriptors([
@@ -493,6 +493,43 @@ class ImportDescriptorsTest(BitcoinTestFramework):
         assert_equal(len(decoded['vin'][0]['txinwitness']), 22)
 
 
+        self.log.info("Under P2SH, multisig are standard with up to 15 "
+                      "compressed keys")
+        self.nodes[1].createwallet(wallet_name='multi_priv_big_legacy',
+                                   blank=True, descriptors=True)
+        multi_priv_big = self.nodes[1].get_wallet_rpc('multi_priv_big_legacy')
+        res = multi_priv_big.importdescriptors([
+        {
+            "desc": descsum_create("sh(multi(15,tprv8ZgxMBicQKsPeAjsABad2QinheaMrvqvCkkBZwBRttYcMALE8yVG5GUsQdLmLWNFeYjUuqKiSay7z2bcvhbi1DEdYMRi5JeedkTKbTuJCda/*,tprv8ZgxMBicQKsPeJ59P23D5yq4HpFgfbqph7kYPCnMVPhNdktyAwbusJoMc9QgksGyV9LQwqPQQjMzjTLVtvkwb2Z6EmFiVtTHQyq2WKVrfRT/*,tprv8ZgxMBicQKsPeZ2m1aQaPA1BJ27xPphNfQQ354z77W8mRLKqwyUyonvusrZgQX3bwAQ2WFo1PPjHoAqiWnM1LMvkkmd3ZkXc5HdjxDarhYk/*,tprv8ZgxMBicQKsPeQ2FPA64vJ91G1Bb9kTARbusYSN9QwNmP9dF5M8xhuX8u8coWD8mq24xjwftnT8hteJ4riMYyjeSsxKMaBd1PX1NDDbxqV2/*,tprv8ZgxMBicQKsPe7nycwReBmFGn5HuTJbzLN7iUpDhFESMGvEZWnurvjrV4otDDEnvsrZ2a8X4o19xGXaTkEJsPUApHvMCeVTmAf2i5bBaSez/*,tprv8ZgxMBicQKsPdauRxSzxXTi85dQd4sSwS5sot1gFLUrJUoUGqzohDgPxTpmBjtmDfYjmNidA1hoF6b5gXh83JBdjcosm5rFeN7xUAM4DFxo/*,tprv8ZgxMBicQKsPfAhMFUPNJMkKVMYebxr1A1bdm8itERTuoDXXhpSQvsXjUQmrTmXsM8x5BxuQjaqF2H5tUXMXuGbiPTpzQKj2NZ6hCsUYJNX/*,tprv8ZgxMBicQKsPddHiYzgaoQthYpCCnnG8TQXaBfQEcoqt2xvzCVXHW7FF1n34LDLoJYghmArzW6ej7xiJpWCDn8sng4XT3uPHxc3MysiE3GS/*,tprv8ZgxMBicQKsPdyon5QGdU6tStoUPSJxidv8u75MWCVjCRhEzoFL2zhEHEx2ejmLExRxANbbqGc84jx3EVjWUVxYShEfDvCeMk3a2Fcqk1Qa/*,tprv8ZgxMBicQKsPcv2k7VmW4rKybZ5QpMUtNWdARqs8rYDoZ7tpg27njFquM2GmdXPgCKpqykwKpZ9mqBn7h7umz1sBm6avKmxcbzs1K8C6bVt/*,tprv8ZgxMBicQKsPea6F9YnfAPrzAQbn1Zeb5h9tp1pjnZQ9WqAds1JYhq9mFBumjBhWn3xmsJ4QmBqDGMeJQ5gPXmZrPBjX5hB8NfUgm7HvUyc/*,tprv8ZgxMBicQKsPf2RCn731KSorsuEjA8hwJqwub6BUBJYGpZV5Z6VQPKGEKqGJxEDtgFxeGpkU9zhjNn82SiiiaSqNXnqXofhZAgn8YirN5y2/*,tprv8ZgxMBicQKsPf94VtPAD9Czz4SButGX49W8PM6nxecBrWACbrULsUb9KRBo6gJs9mCQpX4q7DGCWTtkf27htQwXJGhNCDNJ3WCufeV2CE5T/*,tprv8ZgxMBicQKsPdoKuEXhCT2vPyojxxf1C9U75madMvMdqhQf1sET2ToeaDJRP8mzbjYjYq7DV8C233tMmBoPzubVukuNyfu8uxrUJcbdGEhP/*,tprv8ZgxMBicQKsPetrYKgU8APCNusa2Sq2px2TaNNJ6TKhzcBfzCbrVKArA3tXc6JYa3VZN6ose6564Far2ikD2DKguYQahVd7dSa9BAnVbRJe/*))"),
+            "active": True,
+            "range": 1000,
+            "next_index": 0,
+            "timestamp": "now"
+        },
+        {
+            "desc":
+            descsum_create("sh(multi(15,tprv8ZgxMBicQKsPeAjsABad2QinheaMrvqvCkkBZwBRttYcMALE8yVG5GUsQdLmLWNFeYjUuqKiSay7z2bcvhbi1DEdYMRi5JeedkTKbTuJCda/1/*,tprv8ZgxMBicQKsPeJ59P23D5yq4HpFgfbqph7kYPCnMVPhNdktyAwbusJoMc9QgksGyV9LQwqPQQjMzjTLVtvkwb2Z6EmFiVtTHQyq2WKVrfRT/1/*,tprv8ZgxMBicQKsPeZ2m1aQaPA1BJ27xPphNfQQ354z77W8mRLKqwyUyonvusrZgQX3bwAQ2WFo1PPjHoAqiWnM1LMvkkmd3ZkXc5HdjxDarhYk/1/*,tprv8ZgxMBicQKsPeQ2FPA64vJ91G1Bb9kTARbusYSN9QwNmP9dF5M8xhuX8u8coWD8mq24xjwftnT8hteJ4riMYyjeSsxKMaBd1PX1NDDbxqV2/1/*,tprv8ZgxMBicQKsPe7nycwReBmFGn5HuTJbzLN7iUpDhFESMGvEZWnurvjrV4otDDEnvsrZ2a8X4o19xGXaTkEJsPUApHvMCeVTmAf2i5bBaSez/1/*,tprv8ZgxMBicQKsPdauRxSzxXTi85dQd4sSwS5sot1gFLUrJUoUGqzohDgPxTpmBjtmDfYjmNidA1hoF6b5gXh83JBdjcosm5rFeN7xUAM4DFxo/1/*,tprv8ZgxMBicQKsPfAhMFUPNJMkKVMYebxr1A1bdm8itERTuoDXXhpSQvsXjUQmrTmXsM8x5BxuQjaqF2H5tUXMXuGbiPTpzQKj2NZ6hCsUYJNX/1/*,tprv8ZgxMBicQKsPddHiYzgaoQthYpCCnnG8TQXaBfQEcoqt2xvzCVXHW7FF1n34LDLoJYghmArzW6ej7xiJpWCDn8sng4XT3uPHxc3MysiE3GS/1/*,tprv8ZgxMBicQKsPdyon5QGdU6tStoUPSJxidv8u75MWCVjCRhEzoFL2zhEHEx2ejmLExRxANbbqGc84jx3EVjWUVxYShEfDvCeMk3a2Fcqk1Qa/1/*,tprv8ZgxMBicQKsPcv2k7VmW4rKybZ5QpMUtNWdARqs8rYDoZ7tpg27njFquM2GmdXPgCKpqykwKpZ9mqBn7h7umz1sBm6avKmxcbzs1K8C6bVt/1/*,tprv8ZgxMBicQKsPea6F9YnfAPrzAQbn1Zeb5h9tp1pjnZQ9WqAds1JYhq9mFBumjBhWn3xmsJ4QmBqDGMeJQ5gPXmZrPBjX5hB8NfUgm7HvUyc/1/*,tprv8ZgxMBicQKsPf2RCn731KSorsuEjA8hwJqwub6BUBJYGpZV5Z6VQPKGEKqGJxEDtgFxeGpkU9zhjNn82SiiiaSqNXnqXofhZAgn8YirN5y2/1/*,tprv8ZgxMBicQKsPf94VtPAD9Czz4SButGX49W8PM6nxecBrWACbrULsUb9KRBo6gJs9mCQpX4q7DGCWTtkf27htQwXJGhNCDNJ3WCufeV2CE5T/1/*,tprv8ZgxMBicQKsPdoKuEXhCT2vPyojxxf1C9U75madMvMdqhQf1sET2ToeaDJRP8mzbjYjYq7DV8C233tMmBoPzubVukuNyfu8uxrUJcbdGEhP/1/*,tprv8ZgxMBicQKsPetrYKgU8APCNusa2Sq2px2TaNNJ6TKhzcBfzCbrVKArA3tXc6JYa3VZN6ose6564Far2ikD2DKguYQahVd7dSa9BAnVbRJe/1/*))"),
+            "active": True,
+            "internal" : True,
+            "range": 1000,
+            "next_index": 0,
+            "timestamp": "now"
+        }])
+        assert_equal(res[0]['success'], True)
+        assert_equal(res[1]['success'], True)
+
+        addr = multi_priv_big.getnewaddress("", "legacy")
+        w0.sendtoaddress(addr, 10)
+        self.nodes[0].generate(6)
+        self.sync_all()
+        # It is standard and would relay.
+        txid = multi_priv_big.sendtoaddress(w0.getnewaddress(), 10, "", "",
+                                            True)
+        decoded = multi_priv_big.decoderawtransaction(
+            multi_priv_big.gettransaction(txid)['hex']
+        )
+
+
         self.log.info("Combo descriptors cannot be active")
         self.test_importdesc({"desc": descsum_create("combo(tpubDCJtdt5dgJpdhW4MtaVYDhG4T4tF6jcLR1PxL43q9pq1mxvXgMS9Mzw1HnXG15vxUGQJMMSqCQHMTy3F1eW5VkgVroWzchsPD5BUojrcWs8/*)"),
                               "active": True,

Inferring descriptor from a multisig script with keys <= 16, but using a direct push rather than OP_n, fails.

Added a test for this, and fixed the implementation to require minimal encoding.

See the diff

diff --git a/src/script/standard.cpp b/src/script/standard.cpp
index 8a2666e48..422eeef1d 100644
--- a/src/script/standard.cpp
+++ b/src/script/standard.cpp
@@ -110,6 +110,8 @@ static bool GetMultisigKeyCount(opcodetype opcode, valtype data, unsigned int& c
     if (IsPushdataOp(opcode)) {
         try {
             count = CScriptNum(data, true).getint();
+            // Not minimally encoded count.
+            if (count <= 16) return false;
             return IsValidMultisigKeyCount(count);
         } catch (const scriptnum_error&) {
             return false;
diff --git a/src/test/descriptor_tests.cpp b/src/test/descriptor_tests.cpp
index 3a3c30407..9f508a7d6 100644
--- a/src/test/descriptor_tests.cpp
+++ b/src/test/descriptor_tests.cpp
@@ -2,6 +2,7 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <pubkey.h>
 #include <script/descriptor.h>
 #include <script/sign.h>
 #include <script/standard.h>
@@ -26,6 +27,14 @@ void CheckUnparsable(const std::string& prv, const std::string& pub, const std::
     BOOST_CHECK(error == expected_error);
 }
 
+/** Check that the script is inferred as non-standard */
+void CheckInferRaw(const CScript& script)
+{
+    FlatSigningProvider dummy_provider;
+    std::unique_ptr<Descriptor> desc = InferDescriptor(script, dummy_provider);
+    BOOST_CHECK(desc->ToString().rfind("raw(", 0) == 0);
+}
+
 constexpr int DEFAULT = 0;
 constexpr int RANGE = 1; // Expected to be ranged descriptor
 constexpr int HARDENED = 2; // Derivation needs access to private keys
@@ -364,6 +373,27 @@ Check("sh(wsh(multi(20,KzoAz5CanayRKex3fSLQ2BwJpN7U52gZvxMyk78nDMHuqrUxuSJy,KwGN
     CheckUnparsable("", "addr(asdf)", "Address is not valid"); // Invalid address
     CheckUnparsable("", "raw(asdf)", "Raw script is not hex"); // Invalid script
     CheckUnparsable("", "raw(Ü)#00000000", "Invalid characters in payload"); // Invalid chars
+
+    // A 2of4 but using a direct push rather than OP_2
+    CScript nonminimalmultisig;
+    CKey keys[4];
+    nonminimalmultisig << std::vector<unsigned char>{2};
+    for (int i = 0; i < 4; i++) {
+        keys[i].MakeNewKey(true);
+        nonminimalmultisig << ToByteVector(keys[i].GetPubKey());
+    }
+    nonminimalmultisig << 4 << OP_CHECKMULTISIG;
+    CheckInferRaw(nonminimalmultisig);
+
+    // A 2of4 but using a direct push rather than OP_4
+    nonminimalmultisig.clear();
+    nonminimalmultisig << 2;
+    for (int i = 0; i < 4; i++) {
+        keys[i].MakeNewKey(true);
+        nonminimalmultisig << ToByteVector(keys[i].GetPubKey());
+    }
+    nonminimalmultisig << std::vector<unsigned char>{4} << OP_CHECKMULTISIG;
+    CheckInferRaw(nonminimalmultisig);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

darosior · 2021-01-10T11:32:47Z

Changed the first commit's message to clarify the sigop count calculation for 17-20 keys multisigs:

Note that this does not change the sigOpCount calculation (as it would
break consensus). Therefore 1-16 keys multisigs are counted as 1-16 sigops
and 17-20 keys multisigs are counted as 20 sigops.

DrahtBot · 2021-01-13T11:45:39Z

🕵️ @achow101 has been requested to review this pull request as specified in the REVIEWERS file.

achow101

Concept ACK

src/script/standard.cpp

achow101 · 2021-02-19T19:46:56Z

Code Review ACK 773e853dd4e506bf06a1381f95f80523ac66c663

sipa · 2021-02-19T19:54:30Z

src/script/descriptor.cpp

Perhaps clarify that this depends on whether they're compressed or uncompressed (but it's 15 for all compressed)?

sipa · 2021-02-19T19:57:23Z

utACK 773e853dd4e506bf06a1381f95f80523ac66c663

darosior · 2021-02-19T22:06:55Z

Clarified limit is 15 only for compressed key as per @sipa comment, and added release notes for wallet and RPC changes.

range-diff

1:  149948d68 = 1:  858d652a3 script: match multisigs with up to MAX_PUBKEYS_PER_MULTISIG keys
2:  8c24b50a6 ! 2:  af7977d62 script: allow up to 20 keys in wsh() descriptors
    @@ src/script/descriptor.cpp: std::unique_ptr<DescriptorImpl> ParseScript(uint32_t
                  }
              }
              if (ctx == ParseScriptContext::P2SH) {
    -+            // This limits the maximum number of pubkeys to 15.
    ++            // This limits the maximum number of compressed pubkeys to 15.
                  if (script_size + 3 > MAX_SCRIPT_ELEMENT_SIZE) {
                      error = strprintf("P2SH script is too large, %d bytes is larger than %d bytes", script_size + 3, MAX_SCRIPT_ELEMENT_SIZE);
                      return nullptr;
3:  77a336643 = 3:  4f673fec9 test/functional: standarness sanity checks for P2(W)SH multisig
4:  773e853dd = 4:  b5656ce02 rpc/util: multisig: only check redeemScript size is <= 520 for P2SH
-:  --------- > 5:  c6db00c9a doc: add release notes for 20867

darosior · 2021-02-20T00:25:37Z

Force pushed again as the fuzzer found an UB (that can not be triggered under normal conditions though) when using CScriptNum().get_int().

Fixed by just using int instead of unsigned int in MatchMultisig (which is fine, since required to be in [1, 20]).

fuzzer output summary

SUMMARY: UndefinedBehaviorSanitizer: implicit-integer-sign-change script/standard.cpp:111:21 in 
MS: 0 ; base unit: 0000000000000000000000000000000000000000
0x5c,0x1c,0x5c,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x9,0x59,0x0,0xf,0x1,0x1,0x1,0x1,0x1,0x0,0x1,0x1,0x1,0x11,0x1,0x1,0x1,0x1,0x1,0x41,0x21,0x1,0x1,0x1,0x1,0x1,0x5,0x26,0x1,0x1,0x1,0x1,0x0,0x0,0x0,0xf9,0x1,0x1,0x2,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x11,0x1,0xbe,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0x1,0xae,0xae,0xae,0xae,0xae,0xae,0xae,0x1,0x9,0x1,0x1,0x1,0x1,0x1,0x1,0x10,0xff,0xfd,0x0,0xff,0x1,0xf3,0x76,0x32,0x76,0x1,0x1,0x1,0x1,0x1,0x3,0xf4,0xc3,0x0,0x1a,0x55,
\\\x1c\\\x01\x01\x01\x01\x01\x01\x01\x01\x01\x09Y\x00\x0f\x01\x01\x01\x01\x01\x00\x01\x01\x01\x11\x01\x01\x01\x01\x01A!\x01\x01\x01\x01\x01\x05&\x01\x01\x01\x01\x00\x00\x00\xf9\x01\x01\x02\x01\x01\x01\x01\x01\x01\x01\x01\x11\x01\xbe\x01\x01\x01\x01\x01\x01\x01\x01\x01\xae\xae\xae\xae\xae\xae\xae\x01\x09\x01\x01\x01\x01\x01\x01\x10\xff\xfd\x00\xff\x01\xf3v2v\x01\x01\x01\x01\x01\x03\xf4\xc3\x00\x1aU
artifact_prefix='./'; Test unit written to ./crash-c456b1fe61b6ce9ed56247da7294b3be77a12bba
Base64: XBxcAQEBAQEBAQEBCVkADwEBAQEBAAEBAREBAQEBAUEhAQEBAQEFJgEBAQEAAAD5AQECAQEBAQEBAQERAb4BAQEBAQEBAQGurq6urq6uAQkBAQEBAQEQ//0A/wHzdjJ2AQEBAQED9MMAGlU=

diff

diff --git a/src/script/standard.cpp b/src/script/standard.cpp
index 84a3a0557..4ad8d8925 100644
--- a/src/script/standard.cpp
+++ b/src/script/standard.cpp
@@ -99,7 +99,7 @@ static constexpr bool IsValidMultisigKeyCount(unsigned int n_keys)
     return n_keys > 0 && n_keys <= MAX_PUBKEYS_PER_MULTISIG;
 }
 
-static bool GetMultisigKeyCount(opcodetype opcode, valtype data, unsigned int& count)
+static bool GetMultisigKeyCount(opcodetype opcode, valtype data, int& count)
 {
     if (IsSmallInteger(opcode)) {
         count = CScript::DecodeOP_N(opcode);
@@ -120,11 +120,11 @@ static bool GetMultisigKeyCount(opcodetype opcode, valtype data, unsigned int& c
     return false;
 }
 
-static bool MatchMultisig(const CScript& script, unsigned int& required, std::vector<valtype>& pubkeys)
+static bool MatchMultisig(const CScript& script, int& required, std::vector<valtype>& pubkeys)
 {
     opcodetype opcode;
     valtype data;
-    unsigned int keys;
+    int keys;
 
     CScript::const_iterator it = script.begin();
     if (script.size() < 1 || script.back() != OP_CHECKMULTISIG) return false;
@@ -135,7 +135,7 @@ static bool MatchMultisig(const CScript& script, unsigned int& required, std::ve
     }
     if (!GetMultisigKeyCount(opcode, data, keys)) return false;
 
-    if (pubkeys.size() != keys || keys < required) return false;
+    if (pubkeys.size() != static_cast<unsigned long>(keys) || keys < required) return false;
 
     return (it + 1 == script.end());
 }
@@ -197,7 +197,7 @@ TxoutType Solver(const CScript& scriptPubKey, std::vector<std::vector<unsigned c
         return TxoutType::PUBKEYHASH;
     }
 
-    unsigned int required;
+    int required;
     std::vector<std::vector<unsigned char>> keys;
     if (MatchMultisig(scriptPubKey, required, keys)) {
         vSolutionsRet.push_back({static_cast<unsigned char>(required)}); // safe as required is in range 1..20

maflcko · 2021-02-20T07:44:04Z

Thanks, added to https://github.com/bitcoin-core/bitcoin-devwiki/wiki/Fuzz-Trophies-(vulnerabilities)#non-exploitable-issues

darosior · 2021-03-22T08:56:36Z

@achow101 @sipa : small ping as it's been a month since you ACKed and the only changes are addressing Pieter's nit and fixing a fuzzer finding regarding our casts.

achow101 · 2021-03-22T16:18:54Z

re-ACK e839b4c57560ede17d9de94d6814b49576a9450f

instagibbs

Looks good, dropping some comments for now

src/script/standard.cpp

instagibbs · 2021-03-24T06:52:22Z

src/script/standard.cpp

should we be writing to the value before we're sure it succeeds?

The caller should (and does) only regard the value of count if the getter returns true.
I could make sure that on error we always return before writing to count but it would make the logic even more convoluted than it already is in order to support an usage that should not exist in the first place?

src/script/standard.cpp

test/functional/wallet_importdescriptors.py

darosior · 2021-03-24T12:31:35Z

Addressed @instagibbs 's comments.

instagibbs

utACK 1336830

instagibbs · 2021-03-25T06:26:06Z

test/functional/wallet_importdescriptors.py

meshcollider

Concept ACK.

Typo in commit message test/functional: standarness sanity checks for P2(W)SH multisig

I think you should add a test that P2SH multisig is still non-standard with more than 16 sigs.

meshcollider · 2021-04-27T04:49:33Z

src/script/standard.cpp

There is a function called CheckMinimalPush() which is a more complete check, I think you could use that here

Thanks, done:

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index abc0625bb..7e119bb3c 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -225,7 +225,7 @@ bool static CheckPubKeyEncoding(const valtype &vchPubKey, unsigned int flags, co return true; } -bool static CheckMinimalPush(const valtype& data, opcodetype opcode) { +bool CheckMinimalPush(const valtype& data, opcodetype opcode) { // Excludes OP_1NEGATE, OP_1-16 since they are by definition minimal assert(0 <= opcode && opcode <= OP_PUSHDATA4); if (data.size() == 0) { diff --git a/src/script/interpreter.h b/src/script/interpreter.h index c76b3acb2..212de17c7 100644 --- a/src/script/interpreter.h +++ b/src/script/interpreter.h @@ -316,6 +316,8 @@ bool VerifyScript(const CScript& scriptSig, const CScript& scriptPubKey, const C size_t CountWitnessSigOps(const CScript& scriptSig, const CScript& scriptPubKey, const CScriptWitness* witness, unsigned int flags); +bool CheckMinimalPush(const std::vector<unsigned char>& data, opcodetype opcode); + int FindAndDelete(CScript& script, const CScript& b); #endif // BITCOIN_SCRIPT_INTERPRETER_H diff --git a/src/script/standard.cpp b/src/script/standard.cpp index 1cc15bf9b..880bd49f8 100644 --- a/src/script/standard.cpp +++ b/src/script/standard.cpp @@ -106,10 +106,9 @@ static bool GetMultisigKeyCount(opcodetype opcode, valtype data, int& count) } if (IsPushdataOp(opcode)) { + if (!CheckMinimalPush(data, opcode)) return false; try { count = CScriptNum(data, /* fRequireMinimal = */ true).getint(); - // Not minimally encoded count. - if (count <= 16) return false; return IsValidMultisigKeyCount(count); } catch (const scriptnum_error&) { return false;

darosior · 2021-04-27T13:41:02Z

I think you should add a test that P2SH multisig is still non-standard with more than 16 sigs.

There is one already, line 263 of descriptor_tests.cpp

We were previously ruling out 17-20 pubkeys multisig, while they are only invalid under P2SH context. This makes multisigs with up to 20 keys be detected as valid by the solver. This is however *not* a policy change as it would only apply to bare multisigs, which are already limited to 3 pubkeys. Note that this does not change the sigOpCount calculation (as it would break consensus). Therefore 1-16 keys multisigs are counted as 1-16 sigops and 17-20 keys multisigs are counted as 20 sigops. Signed-off-by: Antoine Poinsot <[email protected]>

Signed-off-by: Antoine Poinsot <[email protected]>

Note that it also test for sortedmulti(), which the previous commit didn't. Signed-off-by: Antoine Poinsot <[email protected]>

This increase the maximum number of pubkeys to 20 (valid in P2WSH and P2SH-P2WSH) and only checks the redeemScript doesn't exceed MAX_SCRIPT_ELEMENT_SIZE for P2SH, as this checked is removed under Segwit context. Signed-off-by: Antoine Poinsot <[email protected]>

Signed-off-by: Antoine Poinsot <[email protected]>

meshcollider · 2021-04-30T05:24:59Z

re-utACK ebd4be4

instagibbs

re-ACK ebd4be4

instagibbs · 2021-05-03T02:19:49Z

src/script/standard.cpp

+    if (IsPushdataOp(opcode)) {
+        if (!CheckMinimalPush(data, opcode)) return false;
+        try {
+            count = CScriptNum(data, /* fRequireMinimal = */ true).getint();


just noting the minimal encoding requirement is no longer necessary here due to new check above

…context ebd4be4 doc: add release notes for 20867 (Antoine Poinsot) 5aa50ab rpc/util: multisig: only check redeemScript size is <= 520 for P2SH (Antoine Poinsot) 063df9e test/functional: standardness sanity checks for P2(W)SH multisig (Antoine Poinsot) ae0429d script: allow up to 20 keys in wsh() descriptors (Antoine Poinsot) 9fc68fa script: match multisigs with up to MAX_PUBKEYS_PER_MULTISIG keys (Antoine Poinsot) Pull request description: As described in bitcoin#20620 multisigs are currently limited to 16 keys in descriptors and RPC helpers, even for P2WSH and P2SH-P2WSH. This adds support for multisig with up to 20 keys (which are already standard) for Segwit v0 context for descriptors (`wsh()`, `sh(wsh())`) and RPC helpers. Fixes bitcoin#20620 ACKs for top commit: meshcollider: re-utACK ebd4be4 instagibbs: re-ACK bitcoin@ebd4be4 Tree-SHA512: 36141f10a8288010d17d5c4fe8d24878bcd4533b88a8aba3a44fa8f74ceb3182d70fee01427e0ab7f53ce7fab46c88c1cd3ac3b18ab8a10bd4a6b8b74ed79e46

DrahtBot added the RPC/REST/ZMQ label Jan 6, 2021

darosior force-pushed the descriptor_multi_wsh branch from 6b8e1a0 to 9719808 Compare January 6, 2021 17:00

luke-jr reviewed Jan 6, 2021

View reviewed changes

src/rpc/util.cpp Outdated Show resolved Hide resolved

src/rpc/util.cpp Outdated Show resolved Hide resolved

src/script/descriptor.cpp Outdated Show resolved Hide resolved

src/script/standard.cpp Outdated Show resolved Hide resolved

src/script/standard.cpp Outdated Show resolved Hide resolved

darosior force-pushed the descriptor_multi_wsh branch from 9719808 to bf7f509 Compare January 6, 2021 18:57

DrahtBot mentioned this pull request Jan 6, 2021

wallet: importdescriptors update existing #19651

Merged

mjdietzx reviewed Jan 6, 2021

View reviewed changes