Support the large block heights required by the spec

[teor2345]

Motivation

Zebra limits block heights to 500,000,000 based on the transaction lock time consensus rule. But Zcash has a consensus rule requiring support for larger heights.

Zebra has code that adds 1 to the block height, and panics if the result is over the maximum. So these panics could be triggered by malicious nodes that send fake blocks at the maximum height. But this issue also happens if we increase the limit to u32::MAX. So we should fix those calculations instead.

Scheduling

This ticket is a medium security priority due to panics:

• the coinbase script height is set by miners, and we do height calculations that add and subtract small numbers (up to 100), so we need to check all our Height arithmetic for unwrap and expect, and add tests

This ticket is a low priority for the current height fields:

• the coinbase script height won't reach block height 500 million until the year 3000, and we only do height calculations with small numbers, so we can wait until then to remove the limit
• the expiry height is not checked against the limit during parsing, and we don't do calculations on this height, so it is already implemented correctly
• the consensus rules for parsing the lock time require this limit, and we don't do calculations on this height, so it is already implemented correctly

If future changes add or subtract from an expiry height, we'll need to remove the 500 million limit on those calculations. Because an expiry height of u32::MAX is valid, and could be committed to the chain right now.

Consensus Rule

Implementations MUST support block heights in the range {0 .. 2^31 − 1}. As of NU5, there is a consensus rule that all coinbase transactions MUST have the nExpiryHeight field set to the block height , and this limits the maximum block height to 2^32 − 1, absent future consensus changes.

https://zips.z.cash/protocol/protocol.pdf#blockchain

Solution

• Reject blocks and transactions above u32::MAX at parse time (and generation time)
• Add tests for heights near 0 and u32::MAX
• Delete all code that used to check the previous maximum height limit, except for the lock time, where the height limit is part of the time/height split
• Fix block height calculations that panic or overflow on max-height blocks

Optional:

• As a defence-in-depth, reject blocks with coinbase or expiry heights greater than or equal to u32::MAX - MIN_TRANSPARENT_COINBASE_MATURITY (the largest number we use in height calculations). That way, if we accidentally unwrap or expect on Height calculations, Zebra won't panic.

Already completed in other tickets:

• Change Zebra's code and database to use u32 for block heights
  • we use an unsigned type because negative heights are always invalid
• Add tests for 5 byte compact sizes

Rejected Alternatives

This alternative is not allowed by the spec, due to recent spec changes:

We don't have to do it as part of this PR but we should probably change Height to be like Amount where we guarantee that heights can never be invalid. We'll need to make the inner value of the height private and create new TryFrom impls and probably fix a bunch of code that this breaks across zebra.

Originally posted by @yaahc in #1051 (comment)

[teor2345]

I'm going to put this in the first stable release milestone, because it will change our interfaces - so it would be good to make a decision about it before stable.

[hdevalence]

Previous discussion: #751 (comment)

[yaahc]

Building on this

The benefit we get from paying this cost is increased misuse resistance, but I'm not sure that we have a great deal of exposure to misuse right now, so I'm worried that the tradeoff might not be worthwhile.

The other benefit that came up in the recent review was not needing to always handle the case where the height could be invalid when working with heights. I think we might be able to constrain the need for those checks to just the extra ops traits on Height so it's probably fine either way but I'd like to try it out. I don't think the conversion issue will be as burdensome as we feared before. I'm fairly confident we can implement enough traits to make it easy to work with heights for the types of operations people would need to do without ever converting them to the inner value and for the cases where they do need to work with the inner value an as_i32 should make it pretty easy and then you just have to go back and re-construct a height if you need one again, which people already have to do. If anything I think this will reduce much of the height.0 heavy code we currently have throughout our codebase.

[teor2345]

Marking this as a security issue, because calculations on block heights near the maximum height can panic.

[teor2345]

We might also want to reject blocks with really large heights near u32::MAX, when we parse the coinbase height and expiry height.

That way, if we ever accidentally expect or unwrap the results of Height arithmetic, Zebra won't panic.

I think the largest height we subtract is 100 (coinbase maturity), and the largest height we add is 1 (or maybe 100).

[upbqdn]

We might also want to reject blocks with really large heights near u32::MAX, when we parse the coinbase height and expiry height.

The spec says: "Implementations MUST support block heights up to and including 2^31 −1."

I would therefore suggest rejecting blocks above u32::MAX / 2. This gives another u32::MAX / 2 - 1 of room for offsets.

Regarding the expiry height, there are a few consensus rules that apply here:

and

Regarding the third consensus rule, the spec further clarifies: "• [NU5 onward] ... In order to avoid the block height being limited to 499999999, we also remove that bound on nExpiryHeight for coinbase transactions. For consistency, these changes apply to all coinbase transactions, not just v5 coinbase transactions."

This implies we should keep the old check on the expiry height for non-coinbase transactions, and remove it for coinbase transactions.