Replace amino with protobuf types

[greg-szabo]

Closes #504 and #535 .

This PR is the almost complete work to remove amino from the code, replace it with protobuf and introduce domain types for easier handling of data.

This PR introduces API breaking changes and protocol breaking changes. It is written with Tendermint Core 0.34 in mind and breaks some of the JSON tests (tracked in #505) and the validator set test (tracked in #506).
Additionally, #536 was opened to clean up all amino naming conventions.

Edit: if you are reviewing this with fresh eyes, you might want to start with the unit test that shows the example of how the DomainType trait is implemented: https://github.com/informalsystems/tendermint-rs/pull/527/files#diff-0023eb97912199b06cf76d6255490076

Breaking in amino_types:

• PartsSetHeader was renamed to PartSetHeader
  • parse_parts_header was renamed to parse_part_set_header
• BlockId.parts_header was renamed to BlockId.part_set_header
• CanonicalPartSetHeader.
  • total was changed to u32
  • parse_parts_header was renamed to parse_part_set_header
• PubKeyResponse:
  • pub_key_ed25519 was temporarily changed to tendermint_proto::crypto::PublicKey before we change it into an enum in an upcoming PR
  • Error field was added in line with the protnbuf definitions
• PubKeyRequest:
  • Added chain_id as defined in protobuf
• Proposal and CanonicalProposal:
  • msg_type was changed to u16.
  • timestamp was changed from type MsgTime to Prost::Timestamp.
  • pol_round was changed to u16 (u32 in the case of CanonicalProposal because of the protobuf definition); -1 is translated to None
  • round was changed u32
• SignedMsgType was removed, tendermint_proto implements it correctly
• MsgTime was removed and replaced with Prost::Timestamp
• Vote and CanonicalVote:
  • vote_type was changed to u16.
  • round was changed to u16 (only in Vote. CanonicalVote defines i64.)
  • timestamp was changed from type Option to the built-in Option.
  • validator_index was changed from i64 to i32
• PingRequest, PingResponse removed
  Breaking in validator:
• InfoHashable was renamed to SimpleValidator and the pub_key is a tendermint_proto::crypto::PublicKey type now

API-breaking:

• validator_index is changed to u16. Protobuf defines it as i32 but it should never be negative, hence it fits u16.
• Vote round was changed to u32 for similar reasons.

Things to do in followup PRs (I'll open new issues for these to try to keep the scope somewhat smaller):

• Cleanup/remove AminoMessage and put its functionality into the DomainType trait

• Redo PubKeyResponse into an enum and make the TryFrom implementation nicer

• Implement a better pub_key for the SimpleValidator domain type instead of tendermint_proto::crypto::PublicKey

• Fix integration tests (Update JSON formatting to 0.34 #505)

• Referenced an issue explaining the need for the change

• Updated all relevant documentation in docs

• Updated all code comments where relevant

• Wrote tests

• Updated CHANGELOG.md

[greg-szabo]

I'm interested in feedback from downstream projects (@yihuang @tarcieri ). Is it ok, if a bunch of structs change a lot?
We could try to use our current structs as the domain types and the protobuf structs as the "Raw" types. That would mean that downstream projects would have slightly less impact.

But, this is the best time to fix some typos and field type issues: round is defined as i32 in the protobuf, but our structs use u32, the Proposal message type is supposed to be i32 according to the proto code, while we use u32.

So, I'd rather overhaul some of the structs to make it more streamlined to the proto definitions, but that's why I'm asking what would make life better for downstream projects.

[tarcieri]

But, this is the best time to fix some typos and field type issues: round is defined as i32 in the protobuf, but our structs use u32, the Proposal message type is supposed to be i32 according to the proto code, while we use u32.

These are very much deliberate: these values are invalid when negative, and this crate presently uses the type system to enforce that by making the the invalid states unrepresentable.

I think it's a mistake that the upstream code is using i32 in places where negative values do not make sense. Using signed integers where negative values are invalid has lead to critical security vulnerabilities in cosmos-sdk in the past.

Switching to signed integers will invalidate this type-level safety and may introduce security vulnerabilities in the double-signing detection code unless runtime checks are added to compensate. That said, changing the types will inform via the compiler where these runtime checks need to be added because the type system will detect the changes by rejecting the code, however this has an additional problem that operations that were previously infallible may become fallible because they need to perform a runtime check that a value isn't negative.

I think unless negative values are well-defined for a particular field, using a signed integer instead of an unsigned integer is a mistake which will lead to (potentially security critical) bugs which would otherwise be eliminated by using the type system to make the invalid states unrepresentable.

You can solve this problem without any changes to the upstream Proto definitions by treating the Proto-generated structs as "wire types" and having a fallible conversion to well-typed "domain types" which reject negative values in these fields at the type system level. Especially with Proto3 there are several other reasons you might want to do this, e.g. to make certain fields required.

[greg-szabo]

I love your reasoning, one more point for domain types.

Incidentally, I found one case where our code was doing a runtime check for negative values. That should be cleaned up too.

[greg-szabo]

Please note that at this point prost_amino was removed and the code successfully encodes/decodes protobuf. As mentioned in the PR description, the protobuf structs are copied in place of the amino structs currently.

It is worth reviewing at this point to see the protobuf implementation changes.

I'm in the middle of reintroducing domain types, instead of copying the protobuf types.

@liamsi , if you want to merge this change we can do it, and I can open a new one for reintroducing the domain types. Or if you wait a few more days, I'll have it merged on top of this one. Whichever you feel is better.

[greg-szabo]

Heads up: I'm going to merge #537 into this PR. This will apply the DomainType trait and derive macro on top of the protobuf types. I'll clean up the description and sync up the code with master to prepare for a review and merging.

I'll move the PR to draft until then.

[greg-szabo]

My apologies for the forced push. With the release of v0.16, so many files changed that a rebase made the whole review experience unbearable.

I tihnk when this PR is ready, it's worth reviewing it from scratch.

[greg-szabo]

@tarcieri @liamsi @yihuang this PR is ready for review. I think your input is important because of downstream dependencies.

You will find some tests failing because of light-client and RPC. For the sake of keeping the change manageable/reviewable I would like to move fixing those into a separate PR.

I'm open for other solutions too.

[brapse]

I think this looks really good.

I would love to discuss the impact on downstream consumers synchronously and then look to merge this once test pass. 👍

[liamsi]

Agree with @brapse. I don't fully understand why a macro is needed here and why we can't convert using From and Into only. IMO, it would be good if the choices where explained briefly in an issue.

[greg-szabo]

Ok, I've added some extensive documentation to the DomainType trait to describe what is it and how it is used. To address the specific questions that came up here:
The macro is mostly syntactic sugar as it's supposed to be :) , to make the code more readable. It also allows me to get away from generics which would incur another set of constraints.

Instead of adding the DomainType trait implementation to each and every struct like this:

impl DomainType<MyRawType> for MyDomainType {}

and then worrying about generics, we have:

#[derive(DomainType)]
#[rawtype(MyRawType)]

on top of all structs. It is the same as how serde does Serialization using its with= attribute for interim types. (The syntax is slightly simpler here.)

The From/TryFrom trait by itself wouldn't be enough to do the whole conversion, because these traits have to be implemented by the developer for all structs and we don't want them adding extra boilerplate at the end of each implementation. The From/TryFrom traits ensure that the developers only focus on what they really need: the definition of the domain type in relation to the raw type.
Also, the encoding/decoding is currently in Prost and we might want to change it to something else. It's nice if we don't need to overhaul all the domain types for that.

Lastly, decode uses the TryFrom trait to decode from a Raw type, because the wire might contain an invalid bytestream that the developer wants to indicate (by implementing their own errors during the decode conversion) while encode uses the From trait because we expect the structs in memory to be valid. (Essentially From<MyDomainType> for MyRawType is really Into<MyRawType> for MyDomainType. We define the From trait because of Rust's internal reasoning: we get a free Into trait out of it.)

[romac]

@greg-szabo Lovely documentation :)

[brapse]

👍