PubKey Post-0.40 Cleanup Effort

[amaury1093]

Summary

The PubKey refactor that has been going on for 0.40 left some follow-up work to be cleaned up.

Problem Definition

#6886 (comment) was the fastest way to get Any PubKeys working for 0.40. However, there are some clean-ups tasks to be done.

Proposal

This are small bits that we discussed in issues, PRs, offline chats. Putting them here so we're able to track them.

• Remove the old message PublicKey proto oneof (@blushi) Remove the old message PublicKey proto oneof #7390

  • The type shouldn't be used anywhere anymore, so it should be as simple as removing the proto file, and moving some files around.

• Decouple the SDK from Tendermint's PubKey interface. Replace tmcrypto.PubKey by our own cryptotypes.PubKey (@blushi) Replace tmcrypto.PubKey by our own cryptotypes.PubKey #7419

  The scope of this item is to not use tmcrypto.PubKey throughout the codebase (except when we interface with TM methods), and use our own cryptotypes.PubKey everywhere. Note: cryptotypes.PubKey may or may not itself extend tmcrypto.PubKey, see next point.

• Rethink our own PubKey interface. e.g. VerifySignature right now doesn't make sense .
  There is only one place where we don't agree with the tendermint.PubKey interface: the VerifySingature method. This is the Tendermint definition:

    VerifySignature(msg []byte, sig []byte) bool
  

  The msg []bytes is serialized with a specific serialization method. Unfortunately, we don't have a single serialization in Cosmos-SDK. We relay on both legacy amino and the protobuf serialization. Hence we need to know what serialization was used when the signature was created, and only then we can serialize the message to validate the signature.

  • ref: Migrate BaseAccount PubKey to use Any #7268 (comment)

• Remove bech32 PubKey support #7447 Remove Bech32 encoding of PubKeys.

  • ref: Migrate BaseAccount PubKey to use Any #7268 (comment)

• Should MarshalYAML return []byte instead of string?

• Shoud PrivKeyLedgerSecp256k1 be a proto message? In order to remove the LedgerPrivKey interface. Replace tmcrypto.PubKey by our own cryptotypes.PubKey #7419 (comment)

• wrapper.GetPubKeys silently ignore unpacked keys wrapper.GetPubKeys silently ignore unpacked keys #8129

cc @blushi Feel free to add more items that you can think of.

---

For Admin Use

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

[robert-zaremba]

I updated the issue to provide more details about tendermint.PubKey interface. In general it's good to compose interfaces if possible. Possible means that we agree with method interface. And it's usually better to have that compatibility.

Here the issue is message serialization.

While updating the description and describing the problem of VerifySignature method interface, I reflected a new idea.

Using concrete types for signature verification

I like the simplicity of the tendermints' interface: VerifySignature(msg []byte, sig []byte) bool. In fact this is how I think about signature verification: here are the bytes, and here is the signature for the bytes. This is exactly what I'm also thinking when signing something: only bytes and a public key.

In the OP, we propose a functional argument, which must be a closure:

type GetSignBytesFunc func(mode signing.SignMode) ([]byte, error)

This function doesn't take a message to serialize it. It must have it in it's context. So, the VerifySignature here expects a message / bytes extractor function.

Instead, maybe we can store the signing.SignMode logic in a concrete types, or a wrapper, and use standard the io.Reader interface?

Or maybe we can have a wrapper, and always call it before calling VerifySignature and be compatible with the tendermint interface?

VerifySignature(objWithEmbeddedCodecKnowledge.GetBytes(mode), sig)

[amaury1093]

Apart from ed25519, secp256k1, the sdk has another privkey type:

cosmos-sdk/crypto/ledger/ledger_secp256k1.go

Lines 40 to 48 in 320a852

	// PrivKeyLedgerSecp256k1 implements PrivKey, calling the ledger nano we
	// cache the PubKey from the first call to use it later.
	PrivKeyLedgerSecp256k1 struct {
	// CachedPubKey should be private, but we want to encode it via
	// go-amino so we can view the address later, even without having the
	// ledger attached.
	CachedPubKey tmcrypto.PubKey
	Path hd.BIP44Params
	}

It's the same as secp256k1, except that the privkey takes a path struct inside of bytes. Does it make sense to convert this one to a proto.Message too?

I would say yes, just wanted to make sure no-one is objecting.

[robert-zaremba]

It's the same as secp256k1, except that the privkey takes a path struct inside of bytes. Does it make sense to convert this one to a proto.Message too?

There is a separate task for that: #7718 -- I was already looking at it.
It's better to do it in that task I would say.

[robert-zaremba]

BTW, why this file is in crypto/ledger package?

[amaury1093]

BTW, why this file is in crypto/ledger package?

This file contains a struct representing the private key from a ledger hw wallet.

If we add convert it to proto, I suppose we can put it in crypto/keys/ledger/secp256k1.

[clevinson]

I think the next step here would be forming a WG (maybe just 1-2 people even) for tackling this together with any desired upgrades to TxBuilder & TxFactory?

I don't think its super high priority, but this is what imo the next steps would be.

[educlerici-zondax]

this will be on hold until the implementation of ADR071 is completed.
After that, we can assess the possibility of applying these changes.

[educlerici-zondax]

This issue will continue open until the implementation of the Cryptoprovider can be finished as explained in this PR #18824 and working on this Repo https://github.com/cosmos/crypto