Fix OP_CHECKSIG bug with OP_PUSHDATA #349

To delete the signature from scriptSig, OP_CHECKSIG constructs a new CScript object from the signature found on the stack and then looks for an exact copy of this object in scriptSig.

However, if the original signature was pushed using OP_PUSHDATA and the signature is short enough to be pushed using one of the 1-75 opcodes, the two objects will not match and the signature verification will fail even though the values are the same.

This patch fixes this by comparing the data-pushing opcode's payload only (excluding the opcode itself).

I've tested this with valid testnet transactions, which are verified correctly.

As this is my first bitcoin patch and it touches a rather sensitive part of the code, please review/test it for yourselves as well!

This makes me very, very nervous. I want OP_CHECKSIG unit tests before touching this.

Yea, so — since Luke-jr's pool accepts non-standard transactions I thought I'd suggest that he block these transactions until its fixed. Because if one is done under the old style, and then is spent and the spend validates (but won't validate under the fix) the behavior will become unfixable. Or unfixable without hacks.

Luke asserted that it's already unfixable: If it's fixed, then at any time someone can produce either the old or the new style and the chain will irreparably split for old and new clients. Even if the new one becomes longer, the split will remain because the two interpretations of the signature are mutually exclusive.

I didn't have a really good answer to this— it sounds right to me. Is there any way to fix this that doesn't create a split hazard? ... other than perhaps a hack that specifies the old behavior until some sufficiently far future block number, and the new behavior after it, so that enough clients can be updated before the flag day that an intentionally triggered split isn't an issue. (and, hopefully none of the old style make it into the chain before the flag day, so that the legacy code can simply be dropped once the flag day is sufficiently buried)

Do we need to fix this? If you want to generate valid OP_CHECKSIG transactions, why can't you just use one of the 1-75 opcodes instead of OP_PUSHDATA1/2/4?

And aren't ECDSA signatures always a fixed length (or is there more funky DER encoding going on...)?

On 18 July 2011 10:46, gmaxwell reply@reply.github.com wrote:

Yea, so —  since Luke-jr's pool accepts non-standard transactions I thought I'd suggest that he block these transactions until its fixed.  Because if one is done under the old style, and then is spent and the spend validates (but won't validate under the fix) the behavior will become unfixable. Or unfixable without hacks.

My patch doesn't make any currently valid transactions invalid. But it does make some invalid transactions valid.

This is just a compatibility issue with other clients/implementations of the scripting engine that arises from an ambiguity in the protocol.

Another way to solve this is to standardise that OP_PUSHDATA1 must never push less than 76 bytes, OP_PUSHDATA2 must never push less than 256 bytes, etc.

This could also be addresses by making sure the new cscript object is constructed the same was as the prior one.

This would require more code, but I think it would be more robust against implementation differences.

Wouldn't it be better to just add documentation to the effect that scripts should always use the shortest sequence for pushing data? Isn't that easier/safer than trying to change the validation rules?

Consensus seems to be that standardizing OP_PUSHDATA1 mush never push less than 76 bytes / etc is the right way to go. I'm going to close this.