chg: implement cometBFT changes and adapt tests accordingly #8

marcello33 · 2024-02-27T13:39:30Z

This PR reflects the changes of cometBFT (peppermint rebase) into cosmos-sdk.

crypto/codec/cmt.go

crypto/keys/secp256k1/secp256k1.go

sergio-mena

🙏 LGTM

crypto/keys/secp256k1/secp256k1.go

proto/cosmos/crypto/secp256k1/keys.proto

Raneet10 · 2024-03-05T11:09:07Z

proto/cosmos/crypto/secp256k1/keys.proto

  option (amino.name)             = "tendermint/PrivKeySecp256k1";
  option (amino.message_encoding) = "key_field";

  bytes key = 1;
 }
+
+// PrivKey defines a comet uncompressed secp256k1 private key.


I don't think we have (un)compressed private keys 😄

Yeah but porting the changes from here https://github.com/0xPolygon/cometbft/blob/develop/crypto/secp256k1/secp256k1.go#L22 where we explicitly call PrivKeyName = "comet/PrivKeySecp256k1Uncompressed"
What do you suggest as a comment?

I think Uncompressed was added as a suffix to have consistency with the naming; I don't think calling it that means there's actually an uncompressed priv key.
Again, it's just a nitpick. As a comment we might just say PrivKey defines a comet secp256k1 private key. 🤷

WDYS @sergio-mena ?

Yeah for sure it's not an "uncompressed private key", I kept there for reference to the naming.
But I see it can create confusion. Addressed here

Sorry for delay coming here. In the PR (cometbft/cometbft#2534) I'm working on to upstream these changes to (vanilla) CometBFT I'm not using "uncompressed" because I felt in a similar way to what motivated this thread: I renamed it to cometbft/PrivKeySecp256k1Eth.
Let me know if you'd like me to open two PRs (one here and one on peppermint) to keep the naming consistent.

Yes, please.
Thanks @sergio-mena

simapp/go.mod

tests/go.mod

marcello33 · 2024-03-07T11:06:12Z

some tests are failing under crypto/keys
Will fix them

UPDATE: FIXED.
Only waiting for updates on this (armor string for pubKeys)

sergio-mena · 2024-03-13T16:07:59Z

crypto/keys/secp256k1/secp256k1.go

 )

+func (privKey *PrivKeyOld) Bytes() []byte {


I performed an ad-hoc diff-of-diffs of this directory comparing it to the analogous code from 0xPolygon/cometbft#2.
I think it would ease maintenance if these functions resembled the ones here.

Yeah that was my direction as well, but I tried to keep the diffs with upstream as small as possible.
For example, I did not change the order of the methods in crypto/keys/secp256k1/secp256k1.go.
Also, some functions are left to crypto/keys/secp256k1/secp256k1_nocgo.go where the original methods of the type (privKey *PrivKey) were defined (e.g. Sign and VerifySignature)

sergio-mena · 2024-03-13T16:24:27Z

crypto/keys/secp256k1/secp256k1.go

+func (privKey *PrivKeyOld) Type() string {
+	return keyType
+}
+func (privKey *PrivKeyOld) Sign(msg []byte) ([]byte, error) {


PrivKey doesn't have method Sign. Do we need it here?

It is there, but not in the same file. I kept as it was originally for comos-sdk, meaning crypto/keys/secp256k1/secp256k1_nocgo.go, see here

sergio-mena · 2024-03-13T16:32:07Z

crypto/keys/secp256k1/secp256k1.go

+
+// Address returns an ethereum style addresses
+func (pubKey *PubKeyOld) Address() crypto.Address {
+	return pubKey.Address()


Isn't this method calling itself?

Oh, forgot to push this 🤦
Here it is, thanks

sergio-mena · 2024-03-13T16:35:18Z

crypto/keys/secp256k1/secp256k1.go

+// Address returns an ethereum style addresses
+func (pubKey *PubKeyOld) Address() crypto.Address {
+	return pubKey.Address()
+}
+
+// Bytes returns the pubkey byte format.
+func (pubKey *PubKeyOld) Bytes() []byte {
+	return pubKey.Key
+}
+
+func (pubKey *PubKeyOld) String() string {
+	return fmt.Sprintf("PubKeySecp256k1{%X}", pubKey.Key)
+}
+
+func (pubKey *PubKeyOld) Type() string {
+	return keyType
+}
+
+func (pubKey *PubKeyOld) Equals(other cryptotypes.PubKey) bool {
+	return pubKey.Type() == other.Type() && bytes.Equal(pubKey.Bytes(), other.Bytes())
+}
+
+func (pubKey *PubKeyOld) VerifySignature(msg []byte, sigStr []byte) bool {
+	return pubKey.VerifySignature(msg, sigStr)
+}
+
+// MarshalAmino overrides Amino binary marshaling.
+func (pubKey PubKeyOld) MarshalAmino() ([]byte, error) {
+	return pubKey.Key, nil
+}
+
+// UnmarshalAmino overrides Amino binary marshaling.
+func (pubKey *PubKeyOld) UnmarshalAmino(bz []byte) error {
+	if len(bz) != PubKeySize {
+		return errorsmod.Wrap(errors.ErrInvalidPubKey, "invalid pubkey size")
+	}
+	pubKey.Key = bz
+
+	return nil
+}
+
+// MarshalAminoJSON overrides Amino JSON marshaling.
+func (pubKey PubKeyOld) MarshalAminoJSON() ([]byte, error) {
+	// When we marshal to Amino JSON, we don't marshal the "key" field itself,
+	// just its contents (i.e. the key bytes).
+	return pubKey.MarshalAmino()
+}
+
+// UnmarshalAminoJSON overrides Amino JSON marshaling.
+func (pubKey *PubKeyOld) UnmarshalAminoJSON(bz []byte) error {
+	return pubKey.UnmarshalAmino(bz)
+}


Similarly to the comment above, it might be simpler to maintain if this code resembled this one.

See #8 (comment)

sergio-mena · 2024-03-13T16:35:55Z

crypto/keys/secp256k1/secp256k1.go

+}
+
+func (pubKey *PubKeyOld) VerifySignature(msg []byte, sigStr []byte) bool {
+	return pubKey.VerifySignature(msg, sigStr)


I have the impression this method is calling itself

True, forgot to push this 🤦
Here it is, thanks

crypto/keys/secp256k1/secp256k1.go

api/cosmos/crypto/secp256k1/keys.pulsar.go

+		x.Key = value.Bytes()
+	default:
+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKeyOld"))


api/cosmos/crypto/secp256k1/keys.pulsar.go

+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKeyOld"))
+		}
+		panic(fmt.Errorf("message cosmos.crypto.secp256k1.PubKeyOld does not contain field %s", fd.FullName()))


api/cosmos/crypto/secp256k1/keys.pulsar.go

+		x.Key = value.Bytes()
+	default:
+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKey"))


api/cosmos/crypto/secp256k1/keys.pulsar.go

+		if fd.IsExtension() {
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKey"))
+		}
+		panic(fmt.Errorf("message cosmos.crypto.secp256k1.PubKey does not contain field %s", fd.FullName()))


api/cosmos/crypto/secp256k1/keys.pulsar.go

 		x.Key = value.Bytes()
 	default:
 		if fd.IsExtension() {
-			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PubKey"))
+			panic(fmt.Errorf("proto3 declared messages do not support extensions: cosmos.crypto.secp256k1.PrivKeyOld"))


api/cosmos/crypto/secp256k1/keys.pulsar.go

 		}
-		panic(fmt.Errorf("message cosmos.crypto.secp256k1.PubKey does not contain field %s", fd.FullName()))
+		panic(fmt.Errorf("message cosmos.crypto.secp256k1.PrivKeyOld does not contain field %s", fd.FullName()))


crypto/keys/secp256k1/secp256k1.go

 func (pubKey *PubKey) Address() crypto.Address {
 	if len(pubKey.Key) != PubKeySize {
-		panic("length of pubkey is incorrect")
+		panic(fmt.Sprintf("length of pubkey is incorrect %d != %d", len(pubKey.Key), PubKeySize))


crypto/keys/ed25519/ed25519.go

@@ -164,7 +164,7 @@
 // in SDK except in a tendermint validator context.
 func (pubKey *PubKey) Address() crypto.Address {
 	if len(pubKey.Key) != PubKeySize {
-		panic("pubkey is incorrect size")
+		panic(fmt.Sprintf("length of pubkey is incorrect %d != %d", len(pubKey.Key), PubKeySize))


crypto/keys/secp256k1/secp256k1.go

+// Address returns an ethereum style addresses
+func (pubKey *PubKeyOld) Address() crypto.Address {
+	if len(pubKey.Key) != PubKeySize {
+		panic(fmt.Sprintf("length of pubkey is incorrect %d != %d", len(pubKey.Key), PubKeySize))


marcello33 · 2024-03-15T10:34:26Z

FYI @Raneet10 @sergio-mena

This PR has been merged to mardizzone/POS-1956-auth branch, so you can find the changes in this PR too https://github.com/0xPolygon/cosmos-sdk/pull/3/.

This has been done because, after solving the conflicts from devel for mardizzone/POS-1956-auth, due to the changes in this PR #6 the tests in auth and crypto packages were failing again.

Feel free to keep commenting on this PR, especially for open discussions, and I'll make sure to port the changes upstream.

marcello33 added 2 commits February 27, 2024 14:35

chg: implement cometBFT changes and adapt tests accordingly

c49d478

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2511-auth

7509eb1

sergio-mena reviewed Feb 29, 2024

View reviewed changes

crypto/codec/cmt.go Outdated Show resolved Hide resolved

sergio-mena reviewed Feb 29, 2024

View reviewed changes

crypto/codec/cmt.go Show resolved Hide resolved

sergio-mena reviewed Feb 29, 2024

View reviewed changes

crypto/keys/secp256k1/secp256k1.go Show resolved Hide resolved

marcello33 added 3 commits February 29, 2024 13:26

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2511-auth

1a27c66

chg: address comments to remove PublicKey_Secp256K1 cases

e3a7a8e

chg: updated cometBFT version to first 0xPolygon release

5090ab2

sergio-mena approved these changes Mar 1, 2024

View reviewed changes

Raneet10 reviewed Mar 5, 2024

View reviewed changes

crypto/keys/secp256k1/secp256k1.go Show resolved Hide resolved

Raneet10 reviewed Mar 5, 2024

View reviewed changes

proto/cosmos/crypto/secp256k1/keys.proto Outdated Show resolved Hide resolved

Raneet10 reviewed Mar 5, 2024

View reviewed changes

simapp/go.mod Outdated Show resolved Hide resolved

Raneet10 reviewed Mar 5, 2024

View reviewed changes

simapp/go.mod Outdated Show resolved Hide resolved

Raneet10 reviewed Mar 5, 2024

View reviewed changes

tests/go.mod Outdated Show resolved Hide resolved

marcello33 added 3 commits March 5, 2024 15:36

chg: address PR comments

d3b8a83

chg: address PR comments

fdf810e

chg: merge auth

0e2036e

marcello33 added 5 commits March 7, 2024 12:09

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2511-auth

6e6c75e

chg: go mod tidy

c548cd3

chg: fix tests

018f92a

chg: fix TestAminoUnmarshalJSON

5e0b70e

Merge branch 'mardizzone/POS-1956-auth' into mardizzone/POS-2511-auth

f98b3ae

marcello33 force-pushed the mardizzone/POS-2511-auth branch from ba398d3 to f98b3ae Compare March 13, 2024 13:35

sergio-mena reviewed Mar 13, 2024

View reviewed changes

crypto/keys/secp256k1/secp256k1.go Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Mar 13, 2024

View reviewed changes

marcello33 force-pushed the mardizzone/POS-2511-auth branch from 345b042 to f98b3ae Compare March 13, 2024 17:27

marcello33 added 2 commits March 13, 2024 18:27

chg: address comments

722da0a

chg: better comment on panic

a852574

github-advanced-security bot found potential problems Mar 13, 2024

View reviewed changes

marcello33 merged commit b48bfdf into mardizzone/POS-1956-auth Mar 15, 2024
28 of 46 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chg: implement cometBFT changes and adapt tests accordingly #8

chg: implement cometBFT changes and adapt tests accordingly #8

marcello33 commented Feb 27, 2024

sergio-mena left a comment

Raneet10 Mar 5, 2024

marcello33 Mar 5, 2024

Raneet10 Mar 6, 2024 •

edited

Loading

Raneet10 Mar 6, 2024

marcello33 Mar 6, 2024 •

edited

Loading

sergio-mena Mar 13, 2024 •

edited

Loading

marcello33 Mar 13, 2024

marcello33 commented Mar 7, 2024 •

edited

Loading

sergio-mena Mar 13, 2024 •

edited

Loading

marcello33 Mar 13, 2024

sergio-mena Mar 13, 2024

marcello33 Mar 13, 2024

sergio-mena Mar 13, 2024

marcello33 Mar 13, 2024

sergio-mena Mar 13, 2024

marcello33 Mar 13, 2024

sergio-mena Mar 13, 2024

marcello33 Mar 13, 2024

marcello33 commented Mar 15, 2024

chg: implement cometBFT changes and adapt tests accordingly #8

chg: implement cometBFT changes and adapt tests accordingly #8

Conversation

marcello33 commented Feb 27, 2024

sergio-mena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Raneet10 Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcello33 Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

sergio-mena Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcello33 commented Mar 7, 2024 • edited Loading

sergio-mena Mar 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcello33 commented Mar 15, 2024

Raneet10 Mar 6, 2024 •

edited

Loading

marcello33 Mar 6, 2024 •

edited

Loading

sergio-mena Mar 13, 2024 •

edited

Loading

marcello33 commented Mar 7, 2024 •

edited

Loading

sergio-mena Mar 13, 2024 •

edited

Loading