Zebra: Constrain BlockHeight between 0 and BlockHeight::MAX using Add, Sub, and Cmp

[oxarbitrage]

Edit:

We think the best way to implement this change is using the Add, Sub, and Cmp operators.

We could panic on overflow, or we could return optional values. (Since Rust integers panic on overflow, that seems like an ok thing to do here.)

See the Amount type constraints as an example.

---

I'd like to constrain BlockHeight to 0..=BlockHeight::MAX, but I think we should do that in a separate PR.

(And after the release, because it's going to be intrusive.)

https://github.com/ZcashFoundation/zebra/pull/740#discussion_r459763543

[hdevalence]

Currently the BlockHeight is a tuple wrapping a u32. I'm not sure how we can constrain the value to be less than BlockHeight::MAX without hiding the inner value, since otherwise someone can freely construct whatever values they want.

However, hiding the value imposes a cost on the rest of the world (as mentioned by @teor2345 in the linked discussion). It means that we would either have to reimplement most of the functions on a u32, or provide a conversion and force every user of a BlockHeight to do a conversion before doing anything with the value. Both of these have big downsides, although I think that the first is worse than the second. 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. Maybe this worry is misplaced, but that's the reason that the API was designed like that in the first place (trying to make all of the parsed data as strongly typed as possible without making the ergonomics too bad).

[yaahc]

@hdevalence we can mark the struct #[non_exhaustive] which would give people access to the inner values without letting them construct one.

Edit: wait no, this would still let people mutate the inner value. Add a getter?

[daira]

The protocol doesn't actually say that you can't have block heights greater than 499_999_999. It says that the nExpiryHeight field of a transaction MUST be less than or equal to that value, and it says that the lock_time field is "encoded as in Bitcoin", which means that it is interpreted as a time rather than a block height if greater than 499_999_999. However, according to the specification it would be incorrect for an implementation not to continue the chain past that height. zcashd supports heights up to 2³¹-1, I believe. (It will be nonconforming after that point, but we have ~5101 years to fix this bug. To be fair, you also have ~1188 years to fix your bug here.)

https://github.com/ZcashFoundation/zebra/blob/1b0c0f027f03a5b7d703e001e1602329b1cd7aa4/zebra-chain/src/types.rs#L83

[yaahc]

_checks to make sure this issue is not in any immediate milestones_

[teor2345]

I think we could get most of the benefit of this change by constraining addition and subtraction on BlockHeight, so that:

• addition panics if we calculate a BlockHeight past the limit
• subtraction is always checked, even if release builds, so it panics if we underflow

This also has a really nice ergonomic benefit, because it makes calculations easier to read and implement:

• BlockHeight(height.0 + 1) becomes height + 1
• height.0 - previous_block_height.0 becomes height - previous_block_height

So users could still construct a height past the expiry limit, but it would be much harder to do it accidentally.

[dconnolly]

We think we should be constraining during parsing, but that's it. Closing.