fix crash: RPC "createmultisig" and "addmultisigaddress" #5706

Bug description:

1. Open Bitcoin-Qt(or bitcoind)
2. Open "Debug window"
3. Enter

createmultisig 17 '["0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014"]'

or

addmultisigaddress 17 '["0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014","0227d09df34a6eca0e2009663d2f0c6edc97314f78f308f53b7c8ffa7c552c1014"]' "bug"

4. Result or

About the fix:

1. function _createmultisig_redeemScriptCScript has line (file src/rpcmisc.cpp)

result = GetScriptForMultisig(nRequired, pubkeys);

2. function GetScriptForMultisig has lines (file src/script/standard.cpp)

script << CScript::EncodeOP_N(nRequired);

and

script << CScript::EncodeOP_N(keys.size()) << OP_CHECKMULTISIG;

3. function EncodeOP_N has the assert(file src/script/script.h)

assert(n >= 0 && n <= 16);

4. We don't need check nRequired because it checks here:

if ((int)keys.size() < nRequired)
        throw runtime_error(
            strprintf("not enough keys supplied "
                      "(got %" PRIszu " keys, but need at least %d to redeem)", keys.size(), nRequired));

5. English is not my native, so ,maybe, error message should be improved...

similar to https://github.com/novacoin-project/novacoin/pull/125

Thanks for reporting this in such detail. I'm not sure this limitation is intended, so we may go for a different fix.

Pretty sure it should fail with >15 due to the P2SH limitations.

Right, I remember now, we already check the resulting script against MAX_SCRIPT_ELEMENT_SIZE for that later.

The issue is that some assertion before that fails while constructing the script

In any case, the proposed fix to hardcode a limit at 16 (because the underlying code fails) is not so bad then. Though it may make sense to move the check down to GetScriptForMultisig instead of in the RPC code, so that potential other usages of it get a clear error.

luke-jr:

Pretty sure it should fail with >15 due to the P2SH limitations.

Yes, it should. if keys.size equal 16 then error message looks like:

redeemScript exceeds size limit: 547 > 520 (code -1)

But I left a restriction on 16 because

static const unsigned int MAX_SCRIPT_ELEMENT_SIZE = 520; // bytes

may change in the future

Not likely, that'd be a hardfork.