Integers in Tendermint

[ebuchman]

Some time ago we resolved to use int64 for Height, primarily because signed ints are better when they might be used for arithmetic (https://blog.cosmos.network/choosing-a-type-for-blockchain-height-beware-of-unsigned-integers-714804dddf1d).

That said, recent events brought to light some issues with integers in the Tendermint codebase:

Amino

Ref #2682

Amino uses zig-zag encoding for int64 in Go, which matches sint64 in proto3, but not int64. Proto3 treats int64 and uint64 the same for positive values, but differs for sint64.

Even if we stick with signed values for fields that should never be negative, we probably don't want them zig-zag encoded (the whole point of zig-zag is more efficient negative numbers!)

If we fix this, we should be able to change the type to uint64 later without breaking the encoding

Input Validation

Ref #2683

Many messages contain vanilla int and don't perform any validation before handling, hence negative values can crash the program. Practically all the input validation that should happen is checking for negative numbers. All that code (yet to be written) would go away if we used uint.

Since moving to uint may take some time, we should meanwhile validate all our input. If we throw it away once switching to uint, so be it.

Consistency

We make mixed use of int and int64 throughout - eg. Height.Height and Header.NumTxs are int64, but Header.BlockID.PartSetHeader.Total is int. We also use int for Round.

We should probably be consistent, or at least specify the bit-size and have good reason why we go with 8/16/32/64 each time.

Summary

Arguably, the reason to stick with signed ints is because they are more natural to do arithmetic with.
First, you can't avoid being given int in Go, as it's the default value for integers (eg. x := 5, len(values), and for i, v := range values {). This means if you use anything else you have to cast to it everywhere (but note if we address the consistency issue, we'll have to do that anyways).

Second, even if some value should never be negative, you may still need to subtract two such values, and the difference could be negative. If you accidentally do this with uint you get a massive number, and it will be difficult to find out what went wrong. This can be worked around using strict linters to avoid naive arithmetic, but that makes the code a lot clunkier, though arguably much safer.

The main issue with int is that we have to ensure all objects (both those that we receive and those we create) are validated before being handled in data structures that expect non-negative numbers. It also means those data structures need to be explicit about this expectation in comments. Alternatively, we could add lots more checks for non-negativity, but as we've seen they're easy to forget.

I think we should proceed as follows:

• Write Validation methods for all reactor input to reject negative values
• Change Amino encoding of int64 to match proto3 int64 (and introduce a tag for zigzag, ie. to support sint64)
• Consider iteratively moving to use uint64 everywhere and a linter that rejects any naive arithmetic
  • With the suggested Amino change, I think this should be doable without breaking encodings, but I could be mistaken.

[liamsi]

Thanks for this summary @ebuchman!

I agree on:

Change Amino encoding of int64 to match proto3 int64 (and introduce a tag for zigzag, ie. to support sint64)

Independent from if we change to unsigned ints everywhere in tendermint or not.

Is 0.26.0 blocked on these changes (in amino)?

[tessr]

Is this still relevant now that we've switched so many things to proto? /cc @marbar3778

[tac0turtle]

This still applies. We need to switch all the ints to uints.