Make signatures with non-canonical data pushes non-standard. #3025

This fixes another malleability problem.

This would constitute a soft-fork, as IsPushOnly is called by P2SH VerifyScript. Closing until I find a workaround.

Why not make a generic "IsCanonicalPushDatas" that just checks arbitrary scripts for pushdata canonicality and apply it to both scriptPubKey and scriptSig?

@petertodd You basically mean applying the canonicality test to output scripts as well, without enforcing it being push-only. Sounds reasonable.

Yup, and the same function can be used for scriptSigs because other mechanisms force them to be only pushdata's anyway.

@petertodd Done.

@petertodd Nice catch. I removed it while investigating a tester error, that lead to discovering IsPushOnly() was used in P2SH VerifyScript. I shouldn't have left that change in, though. Removed.

@sipa Cool. Fix the OP_n case and add more tests and I think this is done.

@petertodd If we both allow OP_n and single-byte pushes, malleability will remain (at least for non-P2SH multisigs with less than 17 keys). Is it really a problem?

@sipa As I said in my comment before IsStandard() tests that OP_n is used where appropriate, so there isn't any way to use a non-standard pushdata anyway. Just leave that decision until later - mark it with a "TODO" for now, and do note in that comment that OP_1NEGATE and OP_RESERVED would have to be handled correctly in addition to the more obvious OP_{0,1-16}

Besides, what do you mean by "malleability" in your comment about non-P2SH multisigs anyway? The scriptPubKey is hashed; no-one other than the sender can change it.

@petertodd Right now, this is just an IsStandard() test as a first step, but my intention is certainly to try to get this (or something similar) as a network rule (requiring a soft fork). Together with a few other changes, I believe it's possible to kill malleability entirely (only for transactions that don't choose to give it up through different hashtypes, of course).

From that perspective, I don't think there is any way around making sure that every potential data push has exactly one representation in the script language. If we can't accept such a strict rule even for just IsStandard(), then there is certainly no way to get it as a network rule, and this whole effort becomes less useful.

I know that, I'm just saying that in this case the rule is meaningless for now because it's a case that can't happen in a standard transaction scriptSig, and we should at least update the rest of the reference client source code to follow this new standard first.

Trying to implement a "pushing a byte between 0x00 and 0x10 uses OP_n, rather than 1-byte data pushes" rule, I hit an odd problem: the coinbase genesis is non-canonical...

Heh, I was waiting for you to notice that. :)

ACK ACK ACK ACK. Rebase and merge. (Or should I just open a new pull with the rebase)

ACK presuming aforementioned coinbase issue addressed... I don't see a check in the current commit.

@sipa What is "the coinbase genesis"?

@luke-jr The coinbase of the genesis block uses a pushdata form that sipa's code would consider non-standard.

Anway, there's no reason to apply these rules to scriptPubKey's, which are signed and aren't mutable. Apply them only to scriptSigs.

Rebased. @petertodd But the genesis coinbase is a scriptSig, not a scriptPubKey. I can't remember why this was a problem - IsStandardTx is never applied to coinbases.

@sipa I mean in general; the code right now applies the test to both when to fix malleability you only need to apply to scriptSig. Potentially by doing both we'll cause extra problems that don't need to be.

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/c6adcd734d0c1f38f370cc7e0cf403ae90aa9de2 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

@petertodd Yes, agree. Removed it from scriptPubKey now.

I'd still like to have all 1-byte pushes (the CScript << operator) cause something we consider canonical ourselves. Doing that breaks the genesis block, however.

Did anyone actually test this? For example that spending a multisig doesn't cause code that gets rejected?

I've been running it (well an earlier copy extracted from it) on my node since yesterday afternoon. I haven't done any multisig spends. But I'll go ahead and try that on testnet.

The genesis block really doesn't really have a coinbase. It's more accurate to say merkleroot 4a5e1e4baab89f3a32518a88c31bc87f618f76673e2cc77ab2127b7afdeda33b represents zero transactions. We could update bitcoind to act on that logic, if it helps..

@luke-jr The problem is this:

• Currently, the CScript::operator<< willl not use OP_1 through OP_16 for 1-byte pushes of bytes 0x01 though 0x10.
• To be consistent with the rules we're setting ourselves for IsStandard here, that would need to change (even though that operator isn't actually used anywhere for creating actual transactions currently).
• Changing that would mean the construction of the coinbase genesis (the hardcoded one in chainparams) becomes inconsistent with the real genesis block.

@sipa yes, i got an error: {"code":-25,"message":"64: non-canonical"} when i trying to spend a multisig (multi-input to multi-output), how to fix?

./bitcoind sendrawtransaction 0100000004aa8e347c91a714d7b94c150eb5029ef6b2275b09e543e26e80d7d97be8d0096801000000fd000100493046022100ffeb7f23cd0561e9f72ba1d69d6de9bf2dd19b34852283e82fd591c3d51985b2022100ebbd06fa9c293c12b659ae6a9b98fcd08ec194d7f0c4de048f676b945f22f5f601493046022100dc974771bbe0674080d1697e7bfe68756ad20416c9d6dd115550fe1c7957bd50022100be019ee76f9f1ef04ccfc8e91e449f18b857fa6ab113f7d295e2fee8915f5f12014c69522103bdbcfa53f42f3f8013e7b3b29907ddcaf7a5e5c5938c38470ddeadcab18a479b210357a44da8b8058914c7c5e2f3647296ed35322a5329275f58d714a8af5764274021025edcf4ce9331f82a6a39857a3b06e685f0b2047a1d4ab83a75a61afd204a28b653aeffffffff5481051ec43a44ab388239a97a2f5edfdcc566c0e72f0b804d1e6b1d181060f601000000fc0047304402200d894cad88d430f16435da3ae283507e0283dec140015b933899708027ce8952022076b0b147531d89a861e58869185c2b3bb2d601878408febbf9a12de4e2c35f3a0147304402206afd23bd3e5e54578ba02732d05c9e36ce7784af757a7484da0e99be85a49e07022036921deef12a038e1e88149c6c53f314434adf5b581264c43dd0b50e29caf7e4014c69522103bdbcfa53f42f3f8013e7b3b29907ddcaf7a5e5c5938c38470ddeadcab18a479b210357a44da8b8058914c7c5e2f3647296ed35322a5329275f58d714a8af5764274021025edcf4ce9331f82a6a39857a3b06e685f0b2047a1d4ab83a75a61afd204a28b653aefffffffff07c8d9591df41cba605bbc980e9549978b7171b1c657c82fc6e6b890ee7274700000000fdfd0000473044022055c68b60a9d065f88438b1622faf4eb61ae1ba3a5463912fd6459986a34ad9de02204555ade4c54ee5ff7db9d8e3967a953ee9b8b85ad922fa3c13135f2ec32e377f014830450220352a7278ecd451c0e0849ce3aa691f00395e3587b7b90611bcdede68a5c2876c022100e72897818e924dfd43bc9435a3441b2aeb32de49fbeba563a79aaf306c0c2e3f014c69522103bdbcfa53f42f3f8013e7b3b29907ddcaf7a5e5c5938c38470ddeadcab18a479b210357a44da8b8058914c7c5e2f3647296ed35322a5329275f58d714a8af5764274021025edcf4ce9331f82a6a39857a3b06e685f0b2047a1d4ab83a75a61afd204a28b653aeffffffff15699c3105c31af55b19af09ec8db8f7648027dd7f88eafa02cdfc90ef1e37fa01000000fdfe000048304502207879be207872bb0edfca7f5be47739fd8fa6b9ba3735030283b941b12bb122dd022100cd8a21ff055532eb38850135542a7e5d5ccd9654868b0b1ba0f368b01477ad2d014830450220051abebe269dab1d78fb26022fff3114ff815b9c518e788cbef342991c733112022100a851506519fbbe4c2b1620b765c2084e46bda027c0f2f1fc59b17fe5252964b7014c69522103bdbcfa53f42f3f8013e7b3b29907ddcaf7a5e5c5938c38470ddeadcab18a479b210357a44da8b8058914c7c5e2f3647296ed35322a5329275f58d714a8af5764274021025edcf4ce9331f82a6a39857a3b06e685f0b2047a1d4ab83a75a61afd204a28b653aeffffffff0280c3c901000000001976a914edfb7e5f37f0e09eb917327fc42d81e62bbf443588ac706f9800000000001976a914bad76037c3c4c31c57806de92bc35417a3f4c04b88ac00000000