Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
laanwj merged 1 commit into from
Feb 20, 2015
Merged

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

laanwj merged 1 commit into from
Feb 20, 2015

Conversation

@fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Jan 25, 2015

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"
  1. Result
    createmultisig
    or
    addmultisigaddress

About the fix:

  1. function _createmultisig_redeemScriptCScript has line (file src/rpcmisc.cpp)
result = GetScriptForMultisig(nRequired, pubkeys);
  1. function GetScriptForMultisig has lines (file src/script/standard.cpp)
script << CScript::EncodeOP_N(nRequired);

and

script << CScript::EncodeOP_N(keys.size()) << OP_CHECKMULTISIG;
  1. function EncodeOP_N has the assert(file src/script/script.h)
assert(n >= 0 && n <= 16);
  1. 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));
  1. English is not my native, so ,maybe, error message should be improved...

similar to novacoin-project/novacoin#125

@laanwj
Copy link
Member

laanwj commented Jan 26, 2015

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

@luke-jr
Copy link
Member

luke-jr commented Jan 26, 2015

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

@laanwj
Copy link
Member

laanwj commented Jan 26, 2015

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.

@fsb4000
Copy link
Contributor Author

fsb4000 commented Jan 26, 2015

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

@luke-jr
Copy link
Member

luke-jr commented Jan 26, 2015

Not likely, that'd be a hardfork.

@fsb4000 fsb4000 closed this Feb 8, 2015
@fsb4000 fsb4000 deleted the patch-2 branch February 8, 2015 14:42
@fsb4000 fsb4000 restored the patch-2 branch February 8, 2015 14:44
@fsb4000 fsb4000 reopened this Feb 8, 2015
@laanwj laanwj merged commit e5d9d77 into bitcoin:master Feb 20, 2015
laanwj added a commit that referenced this pull request Feb 20, 2015
@laanwj
Merge #5706: fix crash: RPC \
a026a56 
e5d9d77 fix crash: createmultisig and addmultisigaddress (fsb4000)
laanwj pushed a commit that referenced this pull request Feb 20, 2015
@fsb4000 @laanwj
fix crash: createmultisig and addmultisigaddress
7f502be 
Rebased-From: e5d9d77
Github-Pull: #5706
@fsb4000 fsb4000 deleted the patch-2 branch March 11, 2015 13:34
reddink pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this pull request May 27, 2020
@fsb4000 @reddink
fix crash: createmultisig and addmultisigaddress
9d5eb81 
Rebased-From: e5d9d77
Github-Pull: bitcoin#5706
(cherry picked from commit 7f502be)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Bug RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fsb4000 @laanwj @luke-jr