Cosmos-sdk: Create PubKey proto types

[aaronc]

Following up on #7008, we need to add .proto definitions of PubKey types to complete #6928 using Any.

My general proposal is to add proto types like this:

// proto/cosmos/crypto/secp256k1/secp256k1.proto

package cosmos.crypto.secp256k1;

message PubKey {
  bytes key = 1;
}

The big problem I see with this approach is that it would break amino marshaling which is needed for the keyring and JSON signing. I can think of two work-arounds

Approach 1: override MarshalAmino/MarshalJSON methods.

The downside to this is that overriding JSON affects proto JSON which might not be a good idea. But maybe it's okay...?

Approach 2: have separate amino and a proto types using struct embedding.

This would look like:

message PubKey {
  bytes key = 1 [(gogoproto.embed) = true, (gogoproto.casttype) = "PubKeyBytes"];
}

where PubKeyBytes (or whatever we call it) is the base type alias used for amino:

type PubKeyBytes []byte

I'm not in love with either approach but it's what we've got. Unless we want to abandon the Any approach and deal with this amino compatibility elsewhere... but that's also annoying.

Thoughts?

/cc @alexanderbez @sahith-narahari

[alexanderbez]

I think approach 2 seems to make the most sense tbh, although I don't quite follow what you mean by embedding. In general, I think it's favorable to be consistent, so that's why I like option 2.

[aaronc]

By embedding I mean that the generated struct will look like:

type PubKey struct {
  PubKeyBytes
}

so that methods get forwarded to PubKeyBytes

[aaronc]

In general I'm leaning towards approach 2 as well because I dislike having to deal with those overrides. They cause too many surprises.

[aaronc]

I have a draft here #7110. I guess this approach could work. It is a bit messy with the Equals methods however...

[sahith-narahari]

I too feel approach 2 is better, it's better to avoid overrides imo.

[aaronc]

What do we do about Equals? Now we have two types for the same thing and Equals is broken.

[alexanderbez]

Can we have a custom Equal method that compares types first?

[aaronc]

Can we have a custom Equal method that compares types first?

Then the two types need to be aware of each other, or do something else entirely like performing equality with only Type() and Bytes().

[aaronc]

Having separate proto and amino types is in general problematic because we still need to convert to/from amino JSON... 🤯

[aaronc]

I'm kind of stuck on this... Not really sure what to do, in case anyone has any ideas.

[blushi]

Following up on https://github.com/cosmos/cosmos-sdk/pull/7102#issuecomment-682637039 and https://hackmd.io/1s1yKkjkQ7aER8uKnCc06Q?view, only secp256k1 pub keys have been migrated from tendermint to cosmos-sdk. In that regard, we are creating new PubKey proto types only for secp256k1 and existing multisig (PubKeyMultisigThreshold) pub key types as part of https://github.com/cosmos/cosmos-sdk/pull/7147 in order to be able to have custom Equals methods that can compare amino and proto types using Type() and Bytes() (cf. https://github.com/cosmos/cosmos-sdk/pull/7102#issuecomment-681915876)

While implementing tendermint crypto.PubKey methods for one of the new multisig proto types, I came across an issue with VerifySignature(msg []byte, sig []byte) bool which doesn't seem to be appropriate to handle MultiSignatureData in which signatures could use different SignMode's. In fact, it worked for the amino based PubKeyMultisigThreshold because it's expecting an AminoMultisignature assuming all signatures were made with SIGN_MODE_LEGACY_AMINO_JSON.

That being said, VerifySignature might not even be used with the new multisig proto types, in favor of VerifyMultisignature which takes a GetSignBytesFunc and then can handle multiple sign modes. So one short term (dirty) solution would be to have VerifySignature panic or always return false.

Another cleaner and long term solution would be to move away from tendermint's crypto.PubKey and have one internal to cosmos-sdk. This was also discussed as part of https://github.com/cosmos/cosmos-sdk/issues/5694 but this has been moved to 0.41 and might evolve a quite large refactoring. So it would be good to get further opinions on that.

/cc @amaurymartiny @robert-zaremba @anilCSE @atheeshp @aaronc @clevinson @alexanderbez

[alexanderbez]

Thanks for the excellent summary @blushi! Is there a way we can know to call VerifyMultisignature when the signature is a multi-sig and call VerifySignature otherwise?

[blushi]

@alexanderbez yeah that's already done in https://github.com/cosmos/cosmos-sdk/blob/8de96d16f9f72afb576af7bf6d9351d37cd1d828/x/auth/signing/verify.go for instance.

[alexanderbez]

Yes, MultisigThresholdPubKey#VerifySignature should panic!

[aaronc]

The approach we have been working on so far is to have separate proto and amino types for secp256k1 and use conversion functions.

An alternative that we haven't considered is making some small upstream changes to go-amino that allows for separate MarshalAminoJSON and UnmarshalAminoJSON methods so that proto and amino JSON can be overridden separately. (go-amino already has Marshal/UnmarshalAmino methods for overriding binary.) This would allow us to not need separate proto and amino pubkey types and just override the amino marshaling methods.

In discussing this with @blushi and @amaurymartiny, we agreed that the first approach of separate proto and amino pubkey's has the downsides of:

• more hackiness in the code base which will maintenance more complex going forward, and
• potentially causing scope bloat causing problems in unexpected places like the key-ring or errors that only show up at runtime from using the wrong type.

So our preference now is to just make small changes to amino (here and here to support Marshal/UnmarshalAminoJSON. Our sense is that these changes will 1) be quite small for go-amino and 2) make maintainability simpler going forward because there will be a non-hacky way to do some special amino marshaling just for backwards compatibility.

@okwme we'll need to chat with you about how to make these upstream changes to that repo.

/cc @alexanderbez

[robert-zaremba]

@aaronc - do we need *Amino* infix? Isn't it enough to use the json.(Unm)Marshaler (MarshalJSON and UnmarshalJSON)?

[alexanderbez]

ACK @aaronc -- In fact, I recall attempting to do this months ago when first starting to migrate to Proto, but abandoned it for reasons I can't recall. I'm sure you can still find my PR.

@robert-zaremba yes because when calling codec.MarshalJSON, we want amino to use the override where codec is either an amino or proto codec.

[aaronc]

@robert-zaremba yes we need the Amino infix. Un/MarshalJSON is already used by amino. It is also used by jsonpb. This creates a problem as overriding Un/MarshalJSON will also affect proto JSON which we don't want. This change will allow us to cleanly isolate amino JSON serialization without affecting proto JSON.

[aaronc]

@alexanderbez if you can find your PR that would be great. I'm not seeing anything.

[alexanderbez]

I can't seem to find it either, but shouldn't be difficult to do again.