Fix height serialization inside script of coinbase input

[kostyantyn]

According to the specification https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki,
height is serialized as: "first byte is number of bytes in the number, ...
following bytes are little-endian representation of the number"

However, CScript() << nHeight invokes push_int64 function which has
different serialization strategy for numbers smaller than 17.

The current implementation doesn't cause any issues as BIP34 is enabled from height 227931.
But, if in regtest set consensus.BIP34Height=1 (which can be convenient to test BIP34),
most of the tests will fail as they follow the specification.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

[Gnappuraz]

utACK f151162

[gmaxwell]

It's not clear to me that moving further away from canonical encoding would be a good idea.

[scravy]

ACK f151162

I very much appreciate this change. In the end it does not change anything in bitcoin (height above 16 is serialized the same way), but it adheres to the specification in BIP34 which explicitly states:

The format of the height is "serialized CScript" -- first byte is number of bytes in the number (will be 0x03 on main net for the next 150 or so years with 223-1 blocks), following bytes are little-endian representation of the number (including a sign bit).

That clearly says the serialization is: first byte is the number of bytes in the number.

If I, say, wanted to start a testnet and start from scratch, and have a blockchain explorer for this which parses the height form the coinbase, then this would be one strange issue less I'd have to combat.

[laanwj]

It's not clear to me that moving further away from canonical encoding would be a good idea.

This has been in the implementation so long, and for bitcoin the implementation is the final 'spec'. Might be better to update the BIP at this point.

[sdaftuar]

I tend to agree with @laanwj -- seems to me there is more benefit in updating the BIP (and/or adding a comment to the code, highlighting the issue) than in changing the behavior.

[kostyantyn]

It doesn't change the behavior of the bitcoin as existing implementation also invokes CScriptNum::serialize for numbers larger than 16, I don't see how it can affect the network.

The good thing is that it allows testing BIP34 starting from the first block after the genesis. This can be convenient once it's decided to enable BIP34 from the height=1 in regtest. And it seems there is an intention of doing this as create_coinbase(height, pubkey=None) accepts the height and its value is correctly incremented in tests. Otherwise, we would need to pre-mine 16 blocks with BIP34 off in all functional tests. It looks like a weird workaround that can be avoided.

However, I see it's a minor change, and there are ways to work around it if needed. Let me know if there is anything I can help here.

[luke-jr]

@laanwj The implementation does not enforce BIP34 code for the affected blocks, so the "code trumps the spec" logic does not apply.

• Eloipool implements this according to BIP34.
• ckpool implements this according to BIP34.
• libblkmaker implements this according to BIP34.
• cgminer implements this according to BIP34.

In other words, the only violation of this is in a hypothetical (dead code) path in Bitcoin Core.

Additionally, using the "canonical" encoding would significantly complicate implementation.

For the reasons above, I think fixing this in Core is the correct solution. (Or at least documenting it as a known limitation tolerated because BIP34 isn't active at such heights.)

utACK

[sdaftuar]

(Or at least documenting it as a known limitation tolerated because BIP34 isn't active at such heights.)

I think documentation (in the form of a code comment) is exactly what is called for if we want to improve things, rather than a code change. There is no benefit to users of our software from changing this consensus-critical code as proposed, but there is a higher review burden -- I think we should avoid making these kind of changes.

[laanwj]

Closing this, the change is controversial and (at best) wouldn't make any difference, at worst it'd introduce a consensus bug. Pros/cons just don't weight up.

I think documentation (in the form of a code comment) is exactly what is called for if we want to improve things

Yes.