Incorrect canonical block id

[tomtau]

In Go tendermint: https://github.com/tendermint/tendermint/blob/master/types/canonical.go#L18
If block id hash is empty, CanonicalizeBlockID will return nil for CanonicalBlockID.

In current tendermint-rs, one will get an empty struct (as with non-canonical Block ID): https://github.com/informalsystems/tendermint-rs/blob/master/tendermint/src/block/id.rs#L116

[tomtau]

@greg-szabo

[greg-szabo]

Good catch, let me check.

[greg-szabo]

Ok, this is one of the interesting edge-cases. Apparently, a BlockId->CanonicalBlockId transformation can result in a nil value in Go, if the Id hash is empty. This makes "being able to be nil" a property of CanonicalBlockId, which we don't really do in Rust. In Rust, this property is usually defined outside of the struct with an Option<struct>.

If you check the protobuf messages, CanonicalBlockId is used by CanonicalVote and CanonicalProposal and in both cases, it's an optional field. If you encode CanonicalBlockId directly, it needs to inherit the property of being able to be nil, by becoming an enum that has its None and Some(fields) structure. In some cases this is inevitable, but seeing that anywhere we use CanonicalBlockId, it is already enclosed in an Option<>, it becomes unnecessary - and easier to read and understand in Rust.

If we want to strictly adhere to the Go implementation's specifications, a CanonicalBlockId should be able to encode into a None by itself. Which I believe we won't ever do.

Option number 1: Add a CanonicalBlockId domain type that is an enum of an empty None or a Some(CanonicalBlockId_structure). Strictly speaking this is in line with our expectations: our domain knowledge of CanonicalBlockId tells us that it can be a None. The problem is that the protobuf encoding is still done through the tendermint_proto::types::CanonicalBlockId struct which doesn't allow CanonicalBlockId to be None (or nil). So we again end up with an empty struct instead of a nil.
We could change the encoding from CanonicalBlockId->RawCanonicalBlockId to CanonicalBlockId->Option<RawCanonicalBlockId>. Which again introduces an extra Option<> for no real benefit and makes creating the domaintype a hassle.

Option number 2: Keep the BlockId<->RawCanonicalBlockId transformations as is, because really the BlockId and CanonicalBlockId are the same structs and just modify CanonicalVote and CanonicalProposal to change their fields to None if the BlockId hash is empty. This is straightforward.

I've tried implementing both versions but the first option becomes a rabbit hole of caveats for the developer. The second one is straightforward, although sometimes leaves the developer needing to implement their own Option<RawCanonicalBlockId> if the structure is used outside of the CanonicalVote and CanonicalProposal combos. The documentation mentions this, so it's not completely opaque.