Handle empty block id

[yihuang]

Fix panic when dealing with empty block id in the first block.

[tarcieri]

This seems like a weird way of addressing the problem... doesn't it result in an invalid BlockId?

I'd suggest changing this whole impl to either:

impl From<&block::Id> for Option<BlockId>

or

impl TryFrom<&block::Id> for BlockId

[yihuang]

This is how golang tendermint handled this. I agree we should do better. But we have to handle the serialization identical to golang version to compute signing bytes, what do you think of that?

[tarcieri]

In Rust I think it's more idiomatic to reflect the distinction using the type system

[yihuang]

I think we can also remove the Option in the parts_header field. I think when hash is not empty, parts_header can't be empty.

[yihuang]

I've changed From trait to TryFrom, represent empty block id as Err.
Also fixed block hash calculation when block_id is empty, make it identical to golang version.

[yihuang]

I've removed branch Hash::Null, represent both optional Hash and block::Id as Option.

[liamsi]

Thanks!

[ebuchman]

Thanks for this, though it seems a bit unfortunate. I opened an issue on the Tendermint repo to consider if we should address this more fundamentally: tendermint/tendermint#4241

[ebuchman]

To be honest I don't fully understand this. Why couldn't we somehow utilize the fact that Hash is an enum with a Null variant to deal with this? We changed all the header fields to Options but Options are themselves just enum types so I'd imagine we could do this more cleanly using the Hash type with a Null variant ...

[tarcieri]

@ebuchman it's definitely another option (there and back again). It looks like Hash::Null was removed in #84 as well (possibly at my suggestion).

In retrospect, I can see how Hash::Null might be a better fit for how Tendermint serialization works today. Option is generally the preferred way to represent nullable values in Rust, but it seems like trying to do so might be causing some undue complexity here.

[ebuchman]

#84 included this PR so that's why it was removed there too. I think for this case where we have these possibly null types where the possibility of being null is almost always only related to the first block (rather than normal operation) we could have a cleaner solution by encapsulating the possibility of null in the underlying types rather than in their wrappers. Still not sure, but it does seem it'd be a bit cleaner and simpler.

[ebuchman]

Option is generally the preferred way to represent nullable values in Rust ... is this also true for enum values? Ie. Option<MyEnum> is preferred to a MyEnum::Null variant ?

[tarcieri]

Could go either way on that, but I don't frequently see Null variants of enums. It might make sense here though.

[yihuang]

Put a Null variant into Hash force all the code handling Hash needs to consider Null variant. with Option<Hash>, the block related logic needs to consider the None variant. So I think the later one makes more sense.
And it's not only Hash could be empty, things like BlockId, LastCommit also could be empty. And unfortunately, tendermint serialize the empty values as, arbitrary empty values, rather than null, I guess because of protobuf behavior. That's why rust deserialization code needs to take special care for it.
It would be great if we can have some fundamental solution in the tendermint, like trying to find valid values that make sense to the first block.

[ebuchman]

Would it be better to explicitly code the type of the first block without all the fields that will be empty and then use a BlockEnum that can be either FirstBlock or Block? That would probably best represent what to expect and when fields can actually be empty!

[ebuchman]

Ok opened an issue: #101

[yihuang]

@ebuchman it's definitely another option (there and back again). It looks like Hash::Null was removed in #84 as well (possibly at my suggestion).

In retrospect, I can see how Hash::Null might be a better fit for how Tendermint serialization works today. Option is generally the preferred way to represent nullable values in Rust, but it seems like trying to do so might be causing some undue complexity here.

I think it's only the deserialisation code needs some extra care here, other than that, Option<Hash> should be similar as Hash with Null variant if not a little bit easier, for example, with Option<Hash>, as_bytes and algorithm methods don't have to return an Option, the internal code who only care about valid Hash don't need to handle Option or Null variant.

[yihuang]

What's the problem with all zero hashes then? It seems that's what rust version of cardano did:
(https://github.com/input-output-hk/chain-libs/blob/master/chain-impl-mockchain/src/key.rs#L225).

[tarcieri]

@yihuang IMO an all-zero hash is about the worst thing you can do... the outputs of digest functions are supposed to be random oracles, and all zeroes are anything but.

The risk is an all-zero hash value is inadvertently included in something like a Merkle tree computation.