Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: unmarshalling issue with multisig keys in master (backport #10061) #10113

Merged
merged 10 commits into from
Nov 9, 2021
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,13 @@ Ref: https://keepachangelog.com/en/1.0.0/

## Unreleased

### Bug Fixes

+ [\#10061](https://github.com/cosmos/cosmos-sdk/pull/10061) Ensure that `LegacyAminoPubKey` struct correctly unmarshals from JSON

### API Breaking Changes

* [\#10077](https://github.com/cosmos/cosmos-sdk/pull/10077) Remove telemetry on `GasKV` and `CacheKV` store Get/Set operations, significantly improving their performance.
+ [\#10077](https://github.com/cosmos/cosmos-sdk/pull/10077) Remove telemetry on `GasKV` and `CacheKV` store Get/Set operations, significantly improving their performance.

## [v0.42.9](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.42.9) - 2021-08-04

Expand Down
2 changes: 2 additions & 0 deletions crypto/keys/multisig/amino.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ func (m *LegacyAminoPubKey) UnmarshalAminoJSON(tmPk tmMultisig) error {
// Instead of just doing `*m = *protoPk`, we prefer to modify in-place the
// existing Anys inside `m` (instead of allocating new Anys), as so not to
// break the `.compat` fields in the existing Anys.
m.PubKeys = make([]*types.Any, len(protoPk.PubKeys))
for i := range m.PubKeys {
m.PubKeys[i] = &types.Any{}
Copy link
Contributor

@amaury1093 amaury1093 Sep 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this fix is not correct. Reading the comment just above, we actually don't want to create new Anys, because we want to keep the existing Anys' .compat field.

The unmarshalling error comes from the fact that .compat is empty on the new Anys.

m.PubKeys[i].TypeUrl = protoPk.PubKeys[i].TypeUrl
m.PubKeys[i].Value = protoPk.PubKeys[i].Value
}
Expand Down
48 changes: 47 additions & 1 deletion crypto/keys/multisig/multisig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/cosmos/cosmos-sdk/codec/legacy"
"github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
kmultisig "github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
Expand All @@ -17,6 +18,15 @@ import (
"github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx"
)

func generatePubKeys(n int) []cryptotypes.PubKey {
pks := make([]cryptotypes.PubKey, n)
for i := 0; i < n; i++ {
pks[i] = secp256k1.GenPrivKey().PubKey()
}

return pks
}

func TestAddress(t *testing.T) {
msg := []byte{1, 2, 3, 4}
pubKeys, _ := generatePubKeysAndSignatures(5, msg)
Expand Down Expand Up @@ -379,5 +389,41 @@ func TestAminoUnmarshalJSON(t *testing.T) {
var pk cryptotypes.PubKey
err := cdc.UnmarshalJSON([]byte(pkJSON), &pk)
require.NoError(t, err)
require.Equal(t, uint32(3), pk.(*kmultisig.LegacyAminoPubKey).Threshold)
lpk := pk.(*kmultisig.LegacyAminoPubKey)
require.Equal(t, uint32(3), lpk.Threshold)
require.Equal(t, 5, len(pk.(*kmultisig.LegacyAminoPubKey).PubKeys))

for _, key := range pk.(*kmultisig.LegacyAminoPubKey).PubKeys {
require.NotNil(t, key)
pk := secp256k1.PubKey{}
err := pk.Unmarshal(key.Value)
require.NoError(t, err)
}
}

func TestProtoMarshalJSON(t *testing.T) {
require := require.New(t)
pubkeys := generatePubKeys(3)
msig := kmultisig.NewLegacyAminoPubKey(2, pubkeys)

registry := types.NewInterfaceRegistry()
cryptocodec.RegisterInterfaces(registry)
cdc := codec.NewProtoCodec(registry)

bz, err := cdc.MarshalInterfaceJSON(msig)
require.NoError(err)

var pk2 cryptotypes.PubKey
err = cdc.UnmarshalInterfaceJSON(bz, &pk2)
require.NoError(err)
require.True(pk2.Equals(msig))

// Test that we can correctly unmarshal key from keyring output
info, err := keyring.NewMultiInfo("my multisig", msig)
require.NoError(err)

ko, err := keyring.MkAccKeyOutput(info)
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
require.NoError(err)
require.Equal(ko.Address, sdk.AccAddress(pk2.Address()).String())
require.Equal(ko.PubKey, string(bz))
amaury1093 marked this conversation as resolved.
Show resolved Hide resolved
}