Handle empty block id by yihuang · Pull Request #66 · cometbft/tendermint-rs

yihuang · 2019-11-13T06:11:07Z

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

tarcieri · 2019-11-14T21:26:00Z

tendermint/src/amino_types/block_id.rs

 impl From<&block::Id> for BlockId {
    fn from(bid: &block::Id) -> Self {
-        let bid_hash = bid.hash.as_bytes().unwrap().to_vec();
+        let bid_hash = bid.hash.as_bytes().unwrap_or(&[]).to_vec();


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

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?

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

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 · 2019-11-19T09:14:35Z

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 · 2019-11-25T07:25:30Z

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

and correctly compute header hash when block id is empty. Represent empty Hash with 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 · 2019-12-17T23:13:11Z

@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 · 2019-12-17T23:27:50Z

#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 · 2019-12-17T23:29:41Z

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 · 2019-12-18T00:10:47Z

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

yihuang · 2019-12-18T02:23:39Z

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 · 2019-12-20T18:20:22Z

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 · 2019-12-20T18:25:26Z

Ok opened an issue: #101

yihuang · 2019-12-21T02:19:01Z

@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 · 2019-12-21T02:29:23Z

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 · 2019-12-21T03:15:37Z

@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.

tarcieri reviewed Nov 14, 2019

View reviewed changes

tarcieri mentioned this pull request Nov 16, 2019

Remove secret_connection and ring dependency #60

Merged

yihuang force-pushed the lite_impl_simple_merkle_merged branch from 1c34c3b to 54da372 Compare November 19, 2019 09:01

yihuang mentioned this pull request Nov 25, 2019

add mock testing for lite client #72

Merged

yihuang force-pushed the lite_impl_simple_merkle_merged branch 2 times, most recently from 1508abb to 23b0a08 Compare November 25, 2019 07:23

yihuang requested a review from tarcieri November 25, 2019 07:25

yihuang force-pushed the lite_impl_simple_merkle_merged branch from 23b0a08 to e6d188d Compare November 25, 2019 07:27

Represent optional hash and block id as Option

8b1d3dc

and correctly compute header hash when block id is empty. Represent empty Hash with Option

yihuang force-pushed the lite_impl_simple_merkle_merged branch from e6d188d to 8b1d3dc Compare November 25, 2019 07:45

tarcieri approved these changes Nov 25, 2019

View reviewed changes

liamsi approved these changes Nov 27, 2019

View reviewed changes

liamsi merged commit 39f295e into cometbft:lite_impl_simple_merkle_merged Nov 27, 2019

liamsi mentioned this pull request Nov 27, 2019

Light client: further trait impls, improvements, and tests #63

Closed

liamsi mentioned this pull request Dec 10, 2019

Light client: further trait impls, improvements, and tests #84

Merged

ebuchman mentioned this pull request Dec 11, 2019

Better encoding for empty hashes? tendermint/tendermint#4241

Closed

ebuchman reviewed Dec 11, 2019

View reviewed changes

ebuchman reviewed Dec 17, 2019

View reviewed changes

ebuchman mentioned this pull request Dec 20, 2019

Exceptions for the "first block" #101

Open

Conversation

yihuang commented Nov 13, 2019

Uh oh!

tarcieri Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihuang Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

tarcieri Nov 15, 2019

Choose a reason for hiding this comment

Uh oh!

yihuang Nov 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yihuang commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yihuang commented Nov 25, 2019

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

ebuchman left a comment

Choose a reason for hiding this comment

Uh oh!

tarcieri commented Dec 17, 2019

Uh oh!

ebuchman commented Dec 17, 2019

Uh oh!

ebuchman commented Dec 17, 2019

Uh oh!

tarcieri commented Dec 18, 2019

Uh oh!

yihuang commented Dec 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ebuchman commented Dec 20, 2019

Uh oh!

ebuchman commented Dec 20, 2019

Uh oh!

yihuang commented Dec 21, 2019

Uh oh!

yihuang commented Dec 21, 2019

Uh oh!

tarcieri commented Dec 21, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tarcieri Nov 14, 2019 •

edited

Loading

yihuang Nov 15, 2019 •

edited

Loading

yihuang commented Nov 19, 2019 •

edited

Loading

yihuang commented Dec 18, 2019 •

edited

Loading