PubKey Post-0.40 Cleanup Effort

amaury1093 commented 3 years ago

Summary

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

Problem Definition

https://github.com/cosmos/cosmos-sdk/issues/6886#issuecomment-694111568 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.

[x] Remove the old message PublicKey proto oneof (@blushi) https://github.com/cosmos/cosmos-sdk/pull/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.
[x] Decouple the SDK from Tendermint's PubKey interface. Replace tmcrypto.PubKey by our own cryptotypes.PubKey (@blushi) #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: https://github.com/cosmos/cosmos-sdk/pull/7268#discussion_r492219055
[x] #7447 Remove Bech32 encoding of PubKeys.
- ref: https://github.com/cosmos/cosmos-sdk/pull/7268#discussion_r492217975
[ ] Should MarshalYAML return []byte instead of string?
[ ] Shoud PrivKeyLedgerSecp256k1 be a proto message? In order to remove the LedgerPrivKey interface. https://github.com/cosmos/cosmos-sdk/pull/7419#discussion_r519040559
[ ] 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 commented 3 years ago

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 commented 3 years ago

Apart from ed25519, secp256k1, the sdk has another privkey type: https://github.com/cosmos/cosmos-sdk/blob/320a852ee268bf806baa0a667098442a83da6361/crypto/ledger/ledger_secp256k1.go#L40-L48

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 commented 3 years ago

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: https://github.com/cosmos/cosmos-sdk/issues/7718 -- I was already looking at it. It's better to do it in that task I would say.

robert-zaremba commented 3 years ago

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

amaury1093 commented 3 years ago

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 commented 3 years ago

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.

cosmos / cosmos-sdk