block id in vote could be empty

[yihuang]

Somehow, the block id field in vote of precommits of block could also be empty.
Real world test case included.

[ebuchman]

Thanks for including the test.

Somehow, the block id field in vote of precommits of block could also be empty.

Yes, this is when a validator hasn't seen enough justification to precommit for the block, so they "precommit nil". We include it in the commit to signal that the validator is still online and participating, so they don't get potentially punished for being "offline".

[ebuchman]

Interesting that the amino_types vote had the Option but the main vote didn't - I would expect that to have been preserved. I'm not sure, but I think that might mean that if we were eg. using this code to feed the KMS, we'd end up signing something incorrect, since we'd end up with a vote for a Some(BlockId) containing an empty hash and a None header instead of a vote with a None block_id.

[yihuang]

As far as I know, the rust version of amino proto is like this:

• None is skiped, just the same as go version skipping a default value.
• Some(value) encodes the value as is, won't skip it if it's the default value.
  And the go version seems to automatically skip default values, so if rust version encodes Some(default_value), it won't match the go version, we need to prevent this from happening. And the new version is actually more correct in this regard.

[ebuchman]

Is there a reason why this is an Option? Shouldn't it be always there and instead we use Option on the BlockId itself?

[yihuang]

I believe there are situation when block_id has valid hash, but don't has parts_header, or have a parts_header with default values(can't recall the case for now), for the go version of protobuf, default value is skipped automatically when encoding, but rust version won't check the default value automatically, so we need an Option to keep the encoding exactly the same as the go version.

[ebuchman]

I'm kind of hoping this will go away with #101. In which case maybe we'll want to use an explicit VoteValue enum with variants BlockId and Nil ...

[yihuang]

I actually don't think enum would help us in these cases, because it's adding a branch just like Option, you just move the branch from Option to your own type.
The core problem lies in the protobuf semantic of optional field and default values, and the inconsistency between json and binary version of protobuf encoding.
In the semantic of protobuf, basically, there's no separation between null value and default value, they can be encoded as the same(skip), but in the json version, the default value is encoded as is, not skipped or null, it is actually correct in protobuf's semantic, because a default value is the same as a skipped or null value, but when we use Option to distingish null value and default value in rust, we need extra effort to handle this.

[yihuang]

Let me rephrase our current situation here:

• The semantic of protobuf(also go-amino) don't distinguish empty values and default ones, and the json encoding doesn't even skip default values, for example:

type subMessage struct {
	Height int
	Name   string
}

type bcMessage struct {
	Sub subMessage
}

// With MarshalBinaryBare, bcMessage{}, bcMessage{Sub: subMessage{}}, bcMessage{Sub: subMessage{Height: 0, Name: ""}} all encodes to empty bytes.

// MarshalJson(bcMessage{}) -> {"type":"bcMessage","value":{"Sub":{"Height":"0","Name":""}}}

• According to my test cases with amino-rs, here's the behavior:
  • If you define Option<i64>, Some(0) is not encoded to empty.
  • message value with all fields set to default values, is not encoded to empty.
  • the json encoding is even messier when dealing with the empty/default values.

#[derive(Clone, PartialEq, Message)]
pub struct OptionalScalar {
    #[prost(int64, optional, tag = "1")]
    pub height: Option<i64>,
    #[prost(string, tag = "2")]
    pub name: String,
}

#[derive(Clone, PartialEq, Message)]
pub struct SubMessage {
    #[prost(int64, tag = "1")]
    pub height: i64,
    #[prost(string, tag = "2")]
    pub name: String,
}

#[derive(Clone, PartialEq, Message)]
pub struct TestMessage {
    #[prost(message, tag = "1")]
    pub sub: Option<SubMessage>,
}

fn check_amino_encoding() {
    let buf = AminoMessage::bytes_vec(&TestMessage {
        sub: Some(SubMessage {
            height: 0,
            name: "".to_owned(),
        }),
    });
    assert_ne!(buf, vec![]);

    assert_ne!(
        AminoMessage::bytes_vec(&OptionalScalar {
            height: Some(0),
            name: "".to_owned(),
        }),
        AminoMessage::bytes_vec(&OptionalScalar {
            height: None,
            name: "".to_owned(),
        })
    );
}

[liamsi]

Can we somehow enable circleci for PRs from forks? That would be amazing!

[liamsi]

merging in master reverted removing parse_non_empty_block_id from header.rs. Trying to fix this now. Not sure I can push to the fork to impact this PR but we will see shortly.

[liamsi]

I think we should merge this as is and deal with how-to properly deal with serialization when we have a clearer picture!

As always, thanks @yihuang!