types: discrepancy between amino and proto3

[ebuchman]

The amino encoding of the types.Header doesn't match the proto3 encoding of the abci.Header. See the failing test in #2681.

Even when we use amino v0.13, it still doesn't match. For the empty time, we get:

expected: []byte{0x22, 0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1}
actual  : []byte{0xa, 0x0, 0x22, 0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1, 0x3a, 0x2, 0x12, 0x0}

where the former is Amino encoding, the later is proto3 encoding. Note the proto3 encoding prepends and appends some bytes to the Amino encoding.

This should probably block v0.26

[ebuchman]

Seems its even worse than this. If I have the following message:

message Header2 {
  int64 height = 1;
}

The following test fails:

func TestABCIHeader2(t *testing.T) {
	header2 := struct {
		Height int64
	}{
		Height: 5,
	}
	fmt.Println(header2)

	cdc := amino.NewCodec()
	header2Bz := cdc.MustMarshalBinaryBare(header2)

	pbHeader2 := abci.Header2{
		Height: 5,
	}
	pbHeaderBz, err := proto.Marshal(&pbHeader2)
	assert.NoError(t, err)
	fmt.Println(header2Bz)
	fmt.Println(pbHeaderBz)

	// assert the encodings match
	assert.EqualValues(t, header2Bz, pbHeaderBz)

}

Namely I get,

{5}
[8 10]
[8 5]

Which suggests our varints are wrong?!

[ebuchman]

Ok I figured it out. The varints were wrong because we weren't using signed ints properly in the abci.Header (ie. int64 in proto3 maps to uint64 in golang). Fixed in #2681

[liamsi]

Sorry, just saw this. I stumbled upon this too (see tendermint/go-amino#224 (comment))

[ebuchman]

Right I saw that but failed to follow up, and then got bit by it.

Proto3 has three types of 64-bit varint: uint64, int64, sint64

For non-negative numbers, my understanding is that uint64 and int64 are encoded the same way, but sint64 uses the zig-zag encoding.

However for amino int64 is sint64, and so it's zig-zag encoded, which means non-negative uint64 and int64 are not encoded the same in Amino.

For this reason I had to change all the int64 in abci.Header to be sint64 since the int64 in the types.Header are zig-zag encoded.

This means there's no way to use a non-zigzag int64 with Amino in Go. We could consider a field tag, but it's probably not a priority.

Meanwhile, we should properly document this.

It's also resurfacing the int vs uint discussion for header values like block height, because while int64 felt ok for header.Height, sint64 feels wrong. If we want to leave it as int64 in proto, we'll need a way to non-zigzag encode int64 values in Go.

[ebuchman]

@liamsi do you think it would make sense to change the default behaviour for Amino to not use zig-zag on ints?

We could consider adding a sint or zigzag tag to still support this.

Also I think that would mean we could upgrade later from int64->uint64 without breaking the encodings

[ValarDragon]

I feel fairly confident that zigzag encoding does not make sense to do from an encoding perspective for input expected to be unsigned. The whole point was to ensure that small magnitude negatives have small varint encodings. I think we should just switch to uint for a lot of this, and do so now with a sensible encoding rather than later with an encoding which doesn't make sense.

[liamsi]

@ValarDragon is right that zigzag encoding doesn't make much sense for unsigned ints. I think changing the default for ints makes sense though. Introducing a tag like in proto to let the dev control the encoding (zigzag or varint) makes sense too.

[liamsi]

I took a first stab on it here: tendermint/go-amino#238

[liamsi]

There is a pending release in tendermint/go-amino#240
I can do the changes in the protos (change from sint(32|64) / zigzag to varint) in tendermint.

[ebuchman]

Fixed, thanks Ismail!