Cosmos-sdk: Use protobuf public key in BaseAccount

[aaronc]

As a temporary measure in #5682, we decided to use amino-encoded bytes for the BaseAccount pub_key in protobuf. We can't cut a release without fixing this.

We have a cosmos.crypto.PublicKey proto type which we can use on BaseAccount.

The biggest glitch is that the AccountI interface has Get/SetPubKey methods which take crypto.PubKey interfaces.

I see two solutions:

1. change AccountI.Get/SetPubKey to use cosmos.crypto.PublicKey proto type and then use PublicKeyCodec wherever a conversion to crypto.PubKey is needed
2. find some magic way to cache crypto.PubKey on the BaseAccount struct (maybe with a hidden field)

My preference is to go with approach 1. Thoughts? @alexanderbez ?

[alexanderbez]

I also believe that approach (1) is the cleaner and more intuitive approach. Let's go with that if we can.

[aaronc]

PubKey bech32 encoding is still using amino. I'm assuming we should update that as well to use protobuf?

[aaronc]

Oh no... the whole keyring is still using amino. What do we do? Leave it like that?

[alexanderbez]

Yikes! I would rather avoid using Amino in the Keybase in favor of Proto if we can. However, this may require too much work before Stargate.

I would consider, instead:

1. Quickly tweak keys add to save key material using Proto such that the existing logic can tell the difference between existing amino-encoded vs newly proto-encoded key material (e.g. using a byte prefix or something). This paves the way for use of Proto only since all new keys will be Proto-encoded.
2. Introduce a new command, keys reformat, that takes existing amino-encoded key material and reformates it to proto-encoded key material. Operators will have ample time to reformat all their keys such that in the next major release, we remove amino support entirely.

Other ideas?

/cc @alessio

[aaronc]

Can someone explain how bech32 pubkeys are used? That's one hard piece of this refactoring.

[alexanderbez]

We need to avoid Bech32 pubkey encoding at all costs.

[aaronc]

Oh so we can just delete that code?

[alexanderbez]

Which code specifically?

[aaronc]

Bech32ifyPubKey and all that other pub key stuff in types/address.go

[alexanderbez]

Yeah all of that should be removed IMHO.

[aaronc]

Great 👍 . Now the question is how to serialize the BaseAccount yaml. That is what was using bech32 pubkeys. Any ideas?

[alexanderbez]

so the String method of BaseAccount should just Proto-encode the pubkey?

ie.

pubkey:
  - type: secp256k1
  - value: CAF90...

?

[aaronc]

Alright because it's the proto PublicKey we can just use proto JSON. That makes sense.

[aaronc]

There are a few other complexities I'm running into. Using the protobuf PublicKey will make it so that BaseAccount cannot be amino marshaled at all including JSON because PublicKey is implemented as a proto oneof. We added amino compatibility for Anys but not oneofs... Not sure how to address this.

A few options:

• break amino JSON for BaseAccount altogether (including legacy queriers/REST)
• create a separate amino BaseAccount for those cases (legacy querier/REST in particular)
• encode pub key's as Any on BaseAccount (for amino compatibility) - this diverges from how they work for Txs
• add an amino compatibility layer to PublicKey like we did for Any - this would include probably a cached crypto.PubKey and special decoders/encoders for all the cases

Thoughts @alexanderbez ?

---

Side note: I have been thinking about a larger x/auth & x/bank refactoring for a while now and started writing out an ADR but will probably wait until we have a 0.40 RC before opening a PR. But this complexity for me begs the question of a larger x/auth refactor. For one thing, the way vesting accounts are intertwined definitely makes this more a bit more complicated IMHO because any wrapper structs need to deal with vesting accounts separately.

[aaronc]

Another thought on this. Using Any would in some ways seem like the simplest option but it's complicated by the fact that we don't actually have a different proto type for all of the crypto.PubKey types other than cosmos.crypto.PublicKey... so it's actually not that simple.

[alexanderbez]

Another thought on this. Using Any would in some ways seem like the simplest option but it's complicated by the fact that we don't actually have a different proto type for all of the crypto.PubKey types other than cosmos.crypto.PublicKey... so it's actually not that simple.

My initial thought was, why not just use Any for public keys, but it seems you've addressed this by the above comment? What makes not having the proto messages so difficult?

break amino JSON for BaseAccount altogether (including legacy queriers/REST)

I'm OK with this. We'll just need to make sure we capture this in migration and notify upstream clients.

create a separate amino BaseAccount for those cases (legacy querier/REST in particular)

I don't think we should do this.

encode pub key's as Any on BaseAccount (for amino compatibility) - this diverges from how they work for Txs

How does it diverge? Will this lead to a weird UX, confusion and/or tech debt?

add an amino compatibility layer to PublicKey like we did for Any - this would include probably a cached crypto.PubKey and special decoders/encoders for all the cases

Also seems like a reasonable approach.

[aaronc]

Another thought on this. Using Any would in some ways seem like the simplest option but it's complicated by the fact that we don't actually have a different proto type for all of the crypto.PubKey types other than cosmos.crypto.PublicKey... so it's actually not that simple.

My initial thought was, why not just use Any for public keys, but it seems you've addressed this by the above comment? What makes not having the proto messages so difficult?

Well, then we end up with the whole dependency on tendermint issue and needing to migrate all tm PubKey implementations into the SDK. That may be a good idea but is sort of out of scope of what I'm trying to do here. Basically if we're putting PubKeySecp256k1 into Any as a crypto.PubKey, it doesn't really work because it's defined as type PubKeySecp256k1 [PubKeySecp256k1Size]byte in tm. So we'd need to redefine all those PubKey types everywhere... Maybe it's mainly just PubKeySecp256k1 because multisig is already migrated. I don't know. Wdyt?

If we're doing that we might as well backtrack on the whole cosmos.crypto.PublicKey oneof and just use Any. But I'd rather amino compatibility not be the forcing factor there.

[alexanderbez]

I thought Tendermint was moving away from fixed-sized arrays for public key types @marbar3778? I don't see any harm in redefining key types. This is what I understood that we were doing anyway? Or did I misunderstand?

[aaronc]

Either way I think there are two questions that should be answered separately:

1. how do we encode public keys in Tx and BaseAccount - either the current PublicKey oneof approach or just use Any?
2. how do we deal with the SDK refactoring for `BaseAccount?

What we do about crypto.PubKey and the tendermint dependency is obviously a related question but also a bit separate.

[marbar3778]

I thought Tendermint was moving away from fixed-sized arrays for public key types?

yup you are correct. all key types are no longer sized: [PubKeySecp256k1Size]byte

What we do about crypto.PubKey and the tendermint dependency is obviously a related question but also a bit separate.

I would love for the sdk to absorb secp256k1 as I dont see it being used in tendermint.

[iramiller]

For the use cases that were using Bech32ifyPubKey the Base58 encoding should be considered instead... it was designed for that purpose within BitCoin (see xpub/xprv keys for example).

[aaronc]

So I'm thinking for now to just stick with the existing cosmos.crypto.PublicKey in BaseAccount (unless there are any major objections). And I'll just figure out how to make it work with amino. I have some ideas. It's just annoying, but I made it work for Any, it's doable.

[aaronc]

I think whichever way we go, it would actually be good to have all the concrete PubKey types we use migrated into the SDK so there's no longer a dependency on tendermint for those. I think eventually we'll want our own PubKey interface and this will be helpful for #5694 so that we can override Address or even change the PubKey interface altogether (the amino Bytes method for one is no longer helpful).

secp256k1 is the only one we actually use although ed25519 and sr25519 are referenced and maybe should be migrated too. We already migrated the multisig. How complex do you think it will be to move the others into the SDK @marbar3778 ? It's just copying files and maybe setting up a proto definition right ?

[aaronc]

So after our call last week, I said I would some pros and cons to using a PublicKey oneof vs Any. I'll list those here so we can just come to a decision and unblock this.

oneof

Pros:

• smaller wire/disk size

Cons:

• less consistent with the rest of the SDK where Any is used, so PublicKeyCodec is a separate way of registering public key types from the regular Any InterfaceRegistry

Any

Pros:

• more consistent with the rest of the SDK usage of Any
• we actually get concrete proto types for every pubkey (currently there is no protobuf secp256k1, just a field in the PublicKey oneof for it as mentioned above)
• it will be easier and less painful for me to finish #6928 (definitely not the best reason long term I know... but on my mind for sure lol)

Cons:

• larger wire disk/size although maybe the latter is mitigated by #6193

Thoughts?

[marbar3778]

secp256k1 is the only one we actually use although ed25519 and sr25519 are referenced and maybe should be migrated too. We already migrated the multisig. How complex do you think it will be to move the others into the SDK @marbar3778 ? It's just copying files and maybe setting up a proto definition right ?

I do think secp can be taken out as I don't foresee the Tendermint team iterating on the implementation. For sr25519 and ed25519, since these are both reliant on external libs, we would keep them in Tendermint as we would offer sr25519 as a validator key along side ed25519.

[aaronc]

Would there be any harm in adding ed/sr25519 directly to the SDK so that we can have our own PubKey interface long term? Maybe we just create thin wrappers around what's in tendermint or something.

[alexanderbez]

Yes we should do that @aaronc.

[aaronc]

Just an update, in our last call we agreed to go the Any route with PubKeys for now. I'll be updating BaseAccount in #6928 and Tx will be updated in a future PR. Aiming to get this wrapped up this week. Just want to give you a heads up @webmaster128 @ethanfrey.

[ethanfrey]

Thanks for the heads up @aaronc

We still need a good way to figure out the Any registries on the client side.
We have the issue for Account now. We just check the BaseAccount type and if so, use that to decode manually.

In general, we need to runtime enforce which instances can validly be used in the Any. Or make Any really like TypeScript any.

I assume there is no proto reference to the Any registry and it is just done in the Go code that registers the concrete implementations for the interfaces, right? Very similar to how Amino did it. It would be great if that info could be pulled out into some canonical spec file we could just compile into our JS proto types.

Is there a .proto file representation for Any registries? Or how does this work in other projects?

[aaronc]

@ethanfrey please see #6722 which was just merged.

[ethanfrey]

Thanks, I will add an issue to figure out how to use that (https://github.com/CosmWasm/cosmjs/issues/393)

Ah, it still needs to figure this out dynamically on runtime, right? So we will be returning any or unknown in TypeScript, we can just runtime verify it is one of a legal set, but the calling code has no way to know what that set is at compile time, correct?

There is no language-neutral compile-time information that describes this data? That we can use in our TypeSystem to refine say pubkey: any to pubkey: Secp256k1PubKey | Ed25519PubKey.

[amaurymartiny]

We're seeing the end of the tunnel regarding this issue.

PR tracker (order is important):

• [x] Create proto pubkeys (#7147).
• [x] Remove the old Multisig (#7284).
• [x] Enable Any in transactions (#7276).
• [x] Enable Any in BaseAccount (#7268)

[blushi]

Now that we have added new pub keys proto types for secp256k1 and multisig to cosmos-sdk (see #7147 and #7284), we can use them in BaseAccount as Any as part of https://github.com/cosmos/cosmos-sdk/pull/7268. The current issue is that tests in x/evidence/keeper attempt to create validator accounts with tendermint ed25519.PubKey's, which is not supported by BaseAccount anymore.

Here are some ideas to solve this:

1. quick & hacky but could be ok as long as it's temporary: add a ValidatorPubKey field to BaseAccount (of type bytes or tendermint ed25519.PubKey) to support validator account as long as we don't have our own cosmos-sdk proto ed25519 pub key type.
2. add our own proto ed25519 pub key type, although we've already gone through that route and it seems to involve a larger code refactoring and possibly some upstream changes to tendermint (cf. https://github.com/cosmos/cosmos-sdk/pull/7102#issuecomment-680898985, https://github.com/cosmos/cosmos-sdk/pull/7102#issuecomment-681915876). @amaurymartiny is exploring that route (https://github.com/cosmos/cosmos-sdk/pull/7343, https://github.com/tendermint/tendermint/pull/5374)

UPDATE:

1. use tendermint ed25519.PubKey to create validators and secp256k1.PubKey to create accounts in failing tests, implemented here https://github.com/cosmos/cosmos-sdk/pull/7268/commits/828a5512f8b0d16a009a58f04e781487507fcfa8

CONCLUSION:
Option 2. has been implemented.

cc/ @marbar3778 @amaurymartiny