search

tendermint / go-amino

Protobuf3 with Interface support - Designed for blockchains (deterministic, upgradeable, fast, and compact)
Other
260 stars 78 forks source link

Proto encoding of registered types #276

Open liamsi opened 5 years ago

liamsi commented 5 years ago

ref: https://github.com/tendermint/go-amino/issues/267

@alexanderbez @jackzampolin this could do with more testing but I'd appreciate a first peek.

tac0turtle commented 4 years ago

TODO:

liamsi commented 4 years ago

As discussed with @marbar3778 the changes here pose a problem when bechifying the consensus pubkey in the SDK: https://github.com/cosmos/cosmos-sdk/blob/f1adb7afd974ee50ac9ac83a315cde932f47d7ef/x/staking/types/validator.go#L181-L182 creates a too large string (92 bytes) which can not be decoded later via: https://github.com/cosmos/cosmos-sdk/blob/f1adb7afd974ee50ac9ac83a315cde932f47d7ef/x/staking/types/validator.go#L203-L208

This increase is due to the overhead introduced here (prefix and value are encoded as fields now). The simplest way to understand and reproduce this is via running TestRandBech32PubkeyConsistency in the sdk. The simulation will also fail (the simulation is where Marko noticed the problem).

liamsi commented 4 years ago

There seems to be another bug in the changes here. Add this test (in types/address_test.go in the sdk for instance):

func TestValAddrMustMarshal(t *testing.T) {
    var pub ed25519.PubKeyEd25519

    tSlice := make([]types.ValAddress, 20)
    for i := 0; i < 20; i++ {
        rand.Read(pub[:])
        acc := types.ValAddress(pub.Address())
        tSlice[i] = acc     
    }
    bz := codec.Cdc.MustMarshalBinaryLengthPrefixed(tSlice)
    timeslice := []types.ValAddress{}
    codec.Cdc.MustUnmarshalBinaryLengthPrefixed(bz, &timeslice)
}
liamsi commented 4 years ago

The latter might be somewhat related to: https://github.com/tendermint/go-amino/issues/272

In UnbondAllMatureValidatorQueue the timeslice := []sdk.ValAddress{} is implicitly of type [][]byte (ValAddress is just bytes). But this should (and previously did) work just fine. Also, repeated bytes are allowed in protobuf.

tessr commented 4 years ago

This linter is INTENSE!

tac0turtle commented 4 years ago

ignore these linters will fix it in a coming pr