Support large block heights

[upbqdn]

Motivation

Zebra currently supports block heights up to and including 4_999_999 while the spec says "Implementations MUST support block heights up to and including 2^31 −1."

Specifications

This PR also documents the following consensus rules:

Solution

This PR increases the block limit to u32::MAX / 2 == 2^31 - 1 and adjusts the tests. This change leaves enough room for any offsets in order to perform calculations on the height.

This PR will close #1113.

Reviewer Checklist

• Code implements Specs and Designs
• Tests for Expected Behaviour
• Tests for Errors

[codecov]

Codecov Report

Merging #3401 (15444f1) into main (499ae89) will increase coverage by 0.32%.
The diff coverage is 81.60%.

@@            Coverage Diff             @@
##             main    #3401      +/-   ##
==========================================
+ Coverage   78.34%   78.67%   +0.32%     
==========================================
  Files         267      273       +6     
  Lines       31526    32322     +796     
==========================================
+ Hits        24698    25428     +730     
- Misses       6828     6894      +66     

[teor2345]

Looks great, let's merge!

[upbqdn]

I'd wait until we merge #3408 into this PR.

[teor2345]

I merged in the refactor, re-approving

[upbqdn]

@teor2345 do you recommend adding any particular tests to this PR?

We want to add tests

[teor2345]

@teor2345 do you recommend adding any particular tests to this PR?

I think the existing arithmetic tests cover the new height limit pretty well.

But we might want to specifically test that 2^31 - 1 is the last valid height. (That is, we can add Height and i32 to get 2^31 - 1, but 2^31 fails.)

[upbqdn]

I'm currently working on tests that test both valid and invalid lock times and expiration heights in actual transactions. I don't think we have those.

[upbqdn]

I added tests for expiry heights in V4 transactions, I don't think we had them. I'm setting the heights manually in the tests, but now I'm thinking it might be better to utilize proptest. @teor2345 would you please let me know if I should rewrite the tests and use proptest instead?

I also want to add the same tests for V5 transactions. They will be very similar to V4, so I'd like to have the V4 ones approved first.

[teor2345]

I added tests for expiry heights in V4 transactions, I don't think we had them. I'm setting the heights manually in the tests, but now I'm thinking it might be better to utilize proptest. @teor2345 would you please let me know if I should rewrite the tests and use proptest instead?

This looks good - a single test vector is enough to test this code.

We can always make a proptest later if we're not happy with the coverage. But we should get good coverage from running a full sync to tip.

I also want to add the same tests for V5 transactions. They will be very similar to V4, so I'd like to have the V4 ones approved first.

Go for it!

[upbqdn]

I finished the tests. I feel like they're bloated because similar semantics and code repeats in separate tests (which are separate functions), but I didn't come up with a reasonable abstraction to make them more concise. The reason was that the tests differ in various small details.
The advantage is that the tests look explicit in what they do, and failures are easy to identify---for example, there are separate tests for coinbase and non-coinbase transactions as well as V4 and V5 transactions, so in case of a failure, it should be straightforward to tell what's not working.
However, I still feel like a good abstraction would be nice.

[teor2345]

The advantage is that the tests look explicit in what they do, and failures are easy to identify---for example, there are separate tests for coinbase and non-coinbase transactions as well as V4 and V5 transactions, so in case of a failure, it should be straightforward to tell what's not working.

Thanks!

Good test diagnostics are more important than code re-use here, so you've made a good call.

[teor2345]

This all looks good.

I appreciate the minimal code changes, extensive tests, and cleaning up all the outdated comments.

(I removed the Medium priority, so the CI fix and changes in PRs #3503 and #3459 merge first.)