Bugfix: Compare deserialized block height in coinbase with expected height #1764

Using a byte-for-byte comparison excludes valid nHeight encodings. Replace with standard script method for decoding integer values on the script stack.

Code change looks OK to my 1am brain. Commit description seems lame. I would say something like:

"Using a byte-for-byte comparison excludes valid nHeight encodings. Replace with standard script method for decoding integer values on the script stack."

Comment updated, also fixed a bug in the error message

ACK... assuming it tests OK

NACK. The format should be strict/canonical. Changing the BIP to make that clear is the right fix, in my humble opinion.

Encoding 24-bit heights instead of 32-bit isn't exactly cheap everywhere. :(

<shrug> Satoshi created CastToBigNum() precisely for reasons like this...

I was under the impression we were gearing up to put a lot of effort into forcing scripts to be canonical, I dont see why we would not do that here, too.

Right. This should be forced to be canonical and the BIP should be fixed to make it clear that it's specifying that. (Gavin pointed out that it already does; but it only does so in a fairly vague manner). I'm glad that Luke caught the lack of clarity though.

Well-formed scripts in the coinbase means it is parseable -- which is not the issue here in this thread. These scripts are parseable in both luke-jr's and gavin's cases.

BigNums may have multiple encodings, which is perfectly normal and accepted and supported in script for script stack values. Thus, universal use of CastToBool and CastToBigNum before calculating with a stack value.

Not-well-formed coinbase scripts are ones which include invalid opcodes and other garbage. Well-formed coinbase scripts may have OP_PUSHDATA<garbage> but not simply <garbage>. That's not at issue here.

I ask that everyone reconsider their position based on a new reason: Bitcoin script number format is rather complicated, and could easily be vulnerable to subtle implementation bugs.

My attempt to explain the format in the shortest possible terms:

• Zero is special-cased to serialize to 00 regardless of everything else herein.
• All numbers with a multiple of 8 bits are padded with an extra 8 bits to give them a sign bit, even if the number is unsigned; numbers with non-multiples-of-8 bits are simply padded to a multiple of 8.
• If a number is signed, the absolute MSB (ie, including padding bits) is set.

And hopefully I didn't miss anything in that rather complicated explanation of the number format. :(

@luke-jr that is a major reason to impose a strict canonicality requirement. If there is only one way to encode/decode each value then it is relatively simple and reliable to write tests cases that that show two implementations agree. If there are multiple ways then an attacker can exploit them to create network splits if there are subtle bugs in some implementation.

Guess I didn't look at it from that perspective :)

Fair enough, closing this.

Just to correct it if anyone else reads this thread and implements based on it: zero is not special-cased to 00, its a zero-length vector. See: the script test-cases which include a number of specific number tests.

Right, by 00 I meant as a serialized script; ie, OP_0