Fix height serialization inside script of coinbase input

kostyantyn commented at 6:21 pm on November 1, 2018: contributor

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 commented at 7:34 pm on November 1, 2018: member

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

Conflicts

No conflicts as of last run.

MarcoFalke added the label Consensus on Nov 1, 2018

kostyantyn force-pushed on Nov 1, 2018

Fix height serialization inside script of coinbase input

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.

f151162603

kostyantyn force-pushed on Nov 1, 2018

kostyantyn referenced this in commit c4c077141b on Nov 2, 2018

Gnappuraz commented at 10:33 am on November 2, 2018: contributor

utACK f151162

gmaxwell commented at 9:19 pm on November 3, 2018: contributor

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

scravy commented at 1:21 pm on November 5, 2018: contributor

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.

scravy approved

scravy referenced this in commit 5f8a050c55 on Nov 5, 2018

laanwj commented at 3:09 pm on November 6, 2018: member

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 commented at 4:36 pm on November 7, 2018: member

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 commented at 5:41 pm on November 7, 2018: contributor

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 commented at 5:07 am on December 20, 2018: member

@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 commented at 1:57 pm on January 7, 2019: member

(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 commented at 10:15 am on July 3, 2019: member

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.

laanwj closed this on Jul 3, 2019

MarcoFalke referenced this in commit 62117f9f36 on Aug 5, 2019

sidhujag referenced this in commit f44d5064d6 on Aug 10, 2019

ShengguangXiao referenced this in commit efa1e6e6cc on Aug 28, 2020

Munkybooty referenced this in commit 2c0929fa2f on Nov 25, 2021

Munkybooty referenced this in commit 0b1215a5fc on Nov 25, 2021

Munkybooty referenced this in commit d0f1663305 on Nov 30, 2021

DrahtBot locked this on Dec 16, 2021

Fix height serialization inside script of coinbase input #14633

Conflicts