Protobuf3 compatibility and repeated nulls

[folex]

Hi!

I'm trying to reproduce Amino encoding of the Block and Commit structures in Scala/Java, and I found an obstacle...

When Precommits field contains nilss, it seems that Amino encodes a nil value as 00 (along with type prefix ofc).

But AFAIK in protobuf3, it's impossible to have a null as a value in repeated field.

Here's the code to show 00:

func TestNil(t *testing.T) {
	type H struct { First uint64 }
	type B struct { Hs []*H }
	
	b := B { Hs: []*H{ &H{0xff}, nil, &H{0xff} }, }
	
	bytes, _ := cdc.MarshalBinaryBare(b)
	t.Logf("%+v\n\n", cmn.HexBytes(bytes))
}

Output

0A0308FF010A000A0308FF01

The only way for me to reproduce that behaviour in Scala/Java is to patch protobuf libraries to behave against the Protobuf3 spec.

Example response from Tendermint's RPC Block, containing null in Precommits:

{
    "block_meta": ...,
    "block": {
        "header": ...,
        "data": {
            "txs": ...
        },
        "evidence": {
            "evidence": null
        },
        "last_commit": {
            "block_id": ...,
            "precommits": [
                {
                    "type": 2,
                    "height": 16,
                    "round": 0,
                    "block_id": {
                        "hash": "1E56CF404964AA6B0768E67AD9CBACABCEBCD6A84DC0FC924F1C0AF9043C0188",
                        "parts": {
                            "total": 1,
                            "hash": "D0A00D1902638E1F4FD625568D4A4A7D9FC49E8F3586F257535FC835E7B0B785"
                        }
                    },
                    "timestamp": "2019-04-17T13:30:03.536359799Z",
                    "validator_address": "04C60B72246943675E2F3AADA00E30EC41AA7D4E",
                    "validator_index": 0,
                    "signature": "Z09xcrfz9T6+3q1Yk+gxUo2todPI7mebKed6zO+i1pnIMPdFbSFT9JJjxo5J9HLrn4x2Fqf3QYefQ8lQGNMzBg=="
                },
                null,
                {
                    "type": 2,
                    "height": 16,
                    "round": 0,
                    "block_id": {
                        "hash": "1E56CF404964AA6B0768E67AD9CBACABCEBCD6A84DC0FC924F1C0AF9043C0188",
                        "parts": {
                            "total": 1,
                            "hash": "D0A00D1902638E1F4FD625568D4A4A7D9FC49E8F3586F257535FC835E7B0B785"
                        }
                    },
                    "timestamp": "2019-04-17T13:30:03.536359799Z",
                    "validator_address": "991C9F03698AC07BEB41B71A87715FC4364A994A",
                    "validator_index": 2,
                    "signature": "VkQicfjxbG+EsHimIXr87a7w8KkHnAq/l60Cv+0oY+rthLIw77NpNhjsMRXVBTiMJzZ3abTBvBUb9jrwPClSCA=="
                },
                {
                    "type": 2,
                    "height": 16,
                    "round": 0,
                    "block_id": {
                        "hash": "1E56CF404964AA6B0768E67AD9CBACABCEBCD6A84DC0FC924F1C0AF9043C0188",
                        "parts": {
                            "total": 1,
                            "hash": "D0A00D1902638E1F4FD625568D4A4A7D9FC49E8F3586F257535FC835E7B0B785"
                        }
                    },
                    "timestamp": "2019-04-17T13:30:03.536359799Z",
                    "validator_address": "9F16F63227F11942E6E4A3282B2A293E4BF8206C",
                    "validator_index": 3,
                    "signature": "N9PlulBffWXcX/+ISzAQ23D1aGeXJ+zvYQBEPrv+xFG7Ouu78JaHCT+45Mp+QzdYYfj1+9WhPTpUhIVfk672AA=="
                }
            ]
        }
    }
}

[liamsi]

Thanks for reporting this! I always thought there must be some edge cases where amino still behaves slightly differently than protobuf.

Do you mind sharing the .proto files with us? (Sounds like you’ve created some for Block and Commit?)

Thanks again :)

[folex]

Thanks for the response!

syntax = "proto3";
import "google/protobuf/timestamp.proto";
package proto3;

message Block {
    Header header = 1;
    Data data = 2;
    EvidenceData evidence = 3;
    Commit last_commit = 4;
}

message Header {
    // basic block info
    Version version = 1;
    string chain_id = 2;
    int64 height = 3;
    google.protobuf.Timestamp time = 4;
    int64 num_txs = 5;
    int64 total_txs = 6;

    // prev block info
    BlockID last_block_id = 7;

    // hashes of block data
    bytes last_commit_hash = 8; // commit from validators from the last block
    bytes data_hash = 9;        // transactions

    // hashes from the app output from the prev block
    bytes validators_hash = 10;      // validators for the current block
    bytes next_validators_hash = 11; // validators for the next block
    bytes consensus_hash = 12;       // consensus params for current block
    bytes app_hash = 13;             // state after txs from the previous block
    bytes last_results_hash = 14;    // root hash of all results from the txs from the previous block
    // consensus info
    bytes evidence_hash = 15;    // evidence included in the block
    bytes proposer_address = 16; // original proposer of the block
}

message Data {
    repeated bytes txs = 1;
}

message EvidenceData {
    repeated Evidence evidence = 1;
    bytes hash = 2;
}

message Commit {
    BlockID    block_id = 1;
    repeated Vote precommits = 2;
}

Upd: oh, I missed some structures:

message Version {
    uint64 block = 1;
    uint64 app = 2;
}

message BlockID {
    bytes hash = 1;
    PartSetHeader parts = 2;
}

// From Tendermint's types.proto
message Evidence {
    string type = 1;
    Validator validator = 2;
    int64 height = 3;
    google.protobuf.Timestamp time = 4;
    int64 total_voting_power = 5;
}

message Vote {
    int32 type = 1; // In Go, it's a `byte`, not int32, be careful
    int64 height = 2;
    int32 round = 3;
    BlockID block_id = 4 [json_name = "block_id"];
    google.protobuf.Timestamp timestamp = 5;
    bytes validator_address = 6 [json_name = "validator_address"];
    int32 validator_index = 7 [json_name = "validator_index"];
    bytes signature = 8;
}

[liamsi]

Amino allows you set a flag per field (amino:"empty_elements") which will disallow nil in repeated fields (with nil struct pointers not supported when empty_elements field tag is set). Maybe we should use this one in tendermint 🤔 to have this behaviour:

But AFAIK in protobuf3, it's impossible to have a null as a value in repeated field.

And thanks a lot for the protobuf files! I'm looking into encoding most/everything in tendermint using gogo/protobuf while keeping the go-structs via custom types (see: tendermint/tendermint@develop...ismail/custom_types_revival_or_not)

ref: #206

[folex]

It would be great having encoding in vanilla protobuf! It would be much easier to achieve Tendermint-compatible behaviour in other programming languages :)

As a side question:
If I understood correctly, you're keeping both hand-written Go structs and protobuf-generated Go structs?

Wouldn't it be better to use protobuf-generated structs where possible, or embed them in hand-written structs where additional functionality needed (i.e., mutex in Block)?

And another side question:
In the specific case of null/nil precommits in Block.LastCommit, wouldn't it be a little clearer, if instead of having nil Vote, we'd have a special empty Vote, stating validator address/index there?

E.g. add new SignedMsgType - EmptyPrecommitType SignedMsgType = 0x12, and then

emptyVote := Vote {
	Type: EmptyPrecommitType,
	Height: 17,
	Round: 0,
	BlockID: ..., // real blockID
	Timestamp: ...,
	ValidatorAddress: validator.Address,
	ValidatorIndex: validator.Index,
	signature: nil,
}

This way, in RPC JSON it would look like:

"precommits": [
    {
        "type": 2,
        "height": 16,
        ...,
        "validator_address": "04C60B72246943675E2F3AADA00E30EC41AA7D4E",
        "validator_index": 0,
        "signature": "Z09xcrfz9T6+3q1Yk+gxUo2todPI7mebKed6zO+i1pnIMPdFbSFT9JJjxo5J9HLrn4x2Fqf3QYefQ8lQGNMzBg=="
    },
    {
        "type": 12,
        "height": 16,
        ...,
        "validator_address": "9F16F63227F11942E6E4A3282B2A293E4BF8206C",
        "validator_index": 1,
        "signature": null
    },
    ...
]

Instead of current:

"precommits": [
    {
        "type": 2,
        "height": 16,
        ...,
        "validator_address": "04C60B72246943675E2F3AADA00E30EC41AA7D4E",
        "validator_index": 0,
        "signature": "Z09xcrfz9T6+3q1Yk+gxUo2todPI7mebKed6zO+i1pnIMPdFbSFT9JJjxo5J9HLrn4x2Fqf3QYefQ8lQGNMzBg=="
    },
   null,
   ...
]

[liamsi]

If I understood correctly, you're keeping both hand-written Go structs and protobuf-generated Go structs?

That is the idea, yes.

Wouldn't it be better to use protobuf-generated structs where possible, or embed them in hand-written structs where additional functionality needed (i.e., mutex in Block)?

I agree. There is some pushback by the team on this idea though. Basically due to the following reasons:
Our go-structs aren't merely messages that are passed around but come with additional functionality (e.g validation logic). We would also pass protobuf generated go-code all over the code (the whole tendermint code would be "infested by proto-generated code"). The refactoring would be massive (even if we embedded the proto generated structs). By keeping the go-structs, one could theoretically migrate step by step, (or even keep the go-structs until there is sth. better than protobuf). Of course this approach has downsides too (e.g you would need to keep the proto messages in sync with the go types). Did I miss anything cc @ebuchman @jackzampolin?

In the specific case of null/nil precommits in Block.LastCommit, wouldn't it be a little clearer, if instead of having nil Vote, we'd have a special empty Vote, stating validator address/index there?

It's better to discuss this directly in the tendermint repo. I'll copy over your suggestion (which seems like a good idea on first sight).

[liamsi]

Just to double-check: In your example, the type in the rpc would be different, right (it currently says 2).

[folex]

Yep, type would be 12, my bad. Fixed, thanks :)

[folex]

We would also pass protobuf generated go-code all over the code

Yep, using generated DTOs as a first-class structures sounds pretty bad...

The better way would be to use protobuf-generated structs "at the end", i.e., convert first-class Go structures to protobuf-generated ones, serialize them, and then sign.

This way, the signature verification would be easy to implement in any language, since you could just use existing protobuf library, and binary encoding would be the same out-of-the box.

Note: That's just an idea, I may be missing some important details, and be outright wrong :)

P.S.
But there's still one little quirk, though. Protobuf doc says:

Do not assume the byte output of a serialized message is stable.

The following checks may fail for a protocol buffer message instance foo:
Hash(foo.SerializeAsString()) == Hash(foo.SerializeAsString())

But I'm not sure if one should worry about that in practice, maybe this never happens, or easy to work around.

[liamsi]

The better way would be to use protobuf-generated structs "at the end", i.e., convert first-class Go structures to protobuf-generated ones, serialize them, and then sign.

That is basically the approach I've experimented with (in the branch I've posted). But using gogo/protobuf's custom types too. I'm not sure it is the best idea.

But there's still one little quirk, though. Protobuf doc says:

Yeah, that is a valid concern. In practice I would not worry too much about this though. I think if you use floats or maps you will end up with different byte outputs. amino just doesn't allow those by default. I think the risk is more than manageable and even google's friends at deep mind are trying to use protobuf for hashing: https://github.com/deepmind/objecthash-proto
In any case I would see this as two as separate problems: 1) efficient en-/de-coding and 2) deterministic encoding when signing/hashing.

[jackzampolin]

Great discussion @folex and @liamsi!

Totally agree with the following 👇

It would be great having encoding in vanilla protobuf! It would be much easier to achieve Tendermint-compatible behaviour in other programming languages :)

[liamsi]

ref: #212