Remove bech32 PubKey support

[aaronc]

Summary

Let's remove support for bech32 pubkeys.

Problem Definition

Bech32 PubKey's depend on amino encoding and as I understand it were never a good idea in the first place. Can you say more on this @alexanderbez ?

Proposal

We can just use google.protobuf.Any for representing PubKeys where needed (for instance in x/staking and x/slashing).

Work

• Refactor x/staking Validation and Delegation tests based on MsgCreateValidator.Pubkey type change. #7526
• Use any as validator pubkey #7597
• Bech32 PubKey: use protobuf in slashing keeper #7979
• Remove bech32 PubKey support #7477
• Update relevant docs to remove usage of bech32 pubkeys, and document new Any usage  #8053 Update relevant docs to remove usage of bech32 pubkeys, and document new Any usage
  • Probably more places, but atleast we should update here: https://docs.cosmos.network/master/basics/accounts.html

Followup tasks

• Move key presentation function outside of keyring Move key presentation function outside of keyring #8046
• Remove client/keys.KeysCdc Remove client/keys.KeysCdc #8047
• Move codec.MarshalIfc* functions to codec.Marshaler` interface Move codec.MarshalAny functions to codec.Marshaler interface #8049

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[sunnya97]

Why do we want to remove bech32 pubkeys? Bech32 is independent of amino encoding. It's just about how to encode a pubkey that's represented in bytes, regardless of amino or not.

Also, I'm okay with removing amino encoding of pubkeys, but it must be noted that one of its benefits is that the amino encoding (and thus its bech32 representation as well) implicitly include what type of signature algorithm it is. Any replacement should try to keep this property.

[alexanderbez]

Public keys are not meant to be bech32 encoded. @sunnya97 amino is directly tied to the bech32 encoding of public keys. How is it independent if the bytes come from Amino? Not to mention multi-sigs aren't even supported and will panic when you try to decode because of length restrictions. I have zero clue why this was done initially @aaronc.

[sunnya97]

Bech32 can be used to encode any series of bytes. I'm saying that we can still use bech32 for human-readable pubkeys, even if its not amino pubkeys that are being bech32-ed.

[robert-zaremba]

Bech32 can be used to encode any series of bytes. I'm saying that we can still use bech32 for human-readable pubkeys, even if its not amino pubkeys that are being bech32-ed.

We have addresses for that. PubKeys are long (usually 32 bytes) and with Bech32 (which makes them even longer) are not meant to be human readable.

[alexanderbez]

Right, but if you try to bech32-decode that blob @sunnya97, most binaries will panic because of the hard-capped 90 char limit. This is why we can't bech32 encode pubkeys in JSON output in the SDK right now, because the standard bech32 library panics when the string > 90 chars.

[tarcieri]

Note that parsing Bech32 that's longer than the 90 character limit specified in BIP-0173 is already a part of the BOLT11 Encoding used by the Lightning Network. So there are quite a few Bech32 parsers/serializers which support exceeding this limit in order to support BOLT11.

That said, it seems like another option here is to retain Bech32 encoding for public keys, but remove the Amino prefixes so that the Bech32 encoding is less than 90 characters.

[alexanderbez]

I don't think the amino prefixes is what makes it more than 90 chars. If you have a multisig with >2-3 keypairs, you'll get an encoding that results in more than 90 characters.

[tarcieri]

Aah, well in that case, the BOLT11-style encoding should suffice (really it's just BIP-0173 without the 90-char limit)

[ethanfrey]

There was a long discussion about this (which I cannot find) on the launchpad migration which hit bech32 key issues with multsigs.

There was a nice long discussion here as well: #6886 (which should be good background reading).

@iramiller made some good arguments about base58 being a much more accepted standard for public keys. Just one comment on github #6886 (comment) but I know there were some nice discord discussions. Maybe you want to add your comments here?

In general, if a spec and many libraries enforce a 90 char limit, it seems best not to abuse it. Sure, you can find a go library that doesn't enforce the limit. But what about JS clients, or Rust clients, or Dart clients? Everyone has to go through the same issues the first time they encounter a multisig address, which is not in the general happy path testing. Let's just use an encoding compatible with all major libraries.

[iramiller]

The base 58 encoding (and base58check implementation) is a useful option for public key encoding for all of the reasons highlighted in the bitcoin source. The encoding was specifically created for the purpose of encoding public keys which is a large part of why I offered it up originally as a solution.

https://github.com/bitcoin/bitcoin/blob/master/src/base58.h#L7-L12

[iramiller]

@ethanfrey mentioned a Discord discussion above. Below are the relevant quotes from my conversation with Ethan I believe:

Ira [07/24/2020]: why not standardize on public key encoding using base58 like BIP32 specifies?

Ira [07/24/2020]: xpub6DpKcTRrVcqYS6rXyicDg52FveMhMLr3SSbBA5iFhZPLdVdAMnC597Y9HmV157MqRJJgnDA3tAvX7SjrKmgatuLUctnwNz4rnQYTSLoW9hy
for example ... it has more space that base 32, is well known and supported... and even supports the concept of HRP

has more space that base 32 should read: is more space efficient than base 32

Ira [07/24/2020]: bech32 is useful for addresses per spec ... the base58 approach was specified explictily for encoding those HD wallet extended keys ... would be well understood by the industry outside of cosmos and likely has a fair amount of documentation that could be quickly pulled into standardize this

And the mentioned specification is here:
https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#Serialization_format

[alexanderbez]

ACK on Base58 👍

[amaury1093]

In #7597 we're working to use protobuf Any for pubkeys throughout the SDK, which should remove the usage of custom pubkey encoding altogether. I am having trouble seeing where the Base58 format would come in, once that PR is merged.

[aaronc]

PubKey encoding could be reconsidered post Stargate before we settle on v1 proto files but for now our plan is to stick with the decision to use Any.

One thing that we would need to resolve if we use something like base58 is how we define PubKey prefixes. Is there a registry of key prefixes that we maintain and a standard way to prepend the prefix before converting to base58? Using Any while definitely not as space efficient (because of the type URL) provides us with a convenient way to do this that is consistent with the rest of the SDK's usage of Any.

[robert-zaremba]

I'm also for sticking to protobuf / Any. All clients have to support protobuf anyway. Adding more encoding options for few structures won't help. It will only makes things more complex / twisted.

[ethanfrey]

Why do we want a prefix for the pubkey encoding?

If we just use any and encode them as normal byte field - base64 bytes in json/toml - that seems fine with me.

If we add some custom encoding, I think something like xpub (which is base58 with a prefix and checksum) will give us a prefix

[robert-zaremba]

Why do we want a prefix for the pubkey encoding?

With Any we will go with the protobuf any encoding without custom prefix. Note: Any uses a type url.

[amaury1093]

Re-opening. Let's wait on #7477 before closing this (as per original post).

[robert-zaremba]

Yes, it shouldn't be closed. The tracking PRs are listed in the OP. (Work section)

[aaronc]

Just so everyone is on the same page here. Our team is working on removing bech32 pubkey support and replacing it with Any which we are using for public keys elsewhere.

If there is desire for a different format in the future, can we address that post-stargate? Currently we are not considering some other format like base58.

[clevinson]

Just flagging that we still have bech32 pubkeys expected on the slashing queries for validator SigningInfo and SigningInfos (see here). Proto defs here.

@robert-zaremba is this already on your radar?

I see this as higher priority than keyring, as it's client facing.

[amaury1093]

Aren't these addresses? afaiu we're keeping them as bech32.

[robert-zaremba]

gRPC queries don't use bech32 pubkey.

REST pubkey queries will stay in v0.40 and are marked deprecated. We are planning to remove them in v0.41

[clevinson]

Yep these are addresses. My bad!

[robert-zaremba]

While working on this task I encounter few problems from the DevX perspective. In #7477 I added few helper functions and tests on the PubKey serialization. For other things, I've updated the description of this task with list of followup tasks.
They will be blocked until #7477 get merged (after Stargate release?).

[robert-zaremba]

Question:
I found that we also use bech32 pubkey prefixes in config. If we delete it it may break some client code who is still relaying on this serialization format. For the moment I added a TODO for that in the legacybech32 package. WDYT?

[clevinson]

Once ADR028 and secp256r1 work is merged, this may get looked at again by @robert-zaremba, but we're considering it a nice-to-have for v0.42