Skip to content

Commit

Permalink
Fix multisig LegacyAminoPubKey Amino marshaling (#8841)
Browse files Browse the repository at this point in the history
* Use v034auth RegisterCrypto

* Add custom amino for LegacyAminoPubKey

* Fix registercrypto

* Revert old PR

* revert some genutil stuff

* Add comment

* Add changelog

* Remove binary marshalling

* Fix lint

* Fix lint again

* Fix lint, 3rd time's a charm

* ignore wrong linter warning

* Fix UnmarshalAmioJSON

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Alessio Treglia <[email protected]>
Co-authored-by: Anil Kumar Kammari <[email protected]>
  • Loading branch information
4 people authored Mar 15, 2021
1 parent 5f71e93 commit d4d27e1
Show file tree
Hide file tree
Showing 7 changed files with 172 additions and 13 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (x/bank) [\#8434](https://github.com/cosmos/cosmos-sdk/pull/8434) Fix legacy REST API `GET /bank/total` and `GET /bank/total/{denom}` in swagger
* (x/slashing) [\#8427](https://github.com/cosmos/cosmos-sdk/pull/8427) Fix query signing infos command
* (server) [\#8399](https://github.com/cosmos/cosmos-sdk/pull/8399) fix gRPC-web flag default value
* (crypto) [\#8841](https://github.com/cosmos/cosmos-sdk/pull/8841) Fix legacy multisig amino marshaling, allowing migrations to work between v0.39 and v0.40+.

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

Expand Down
88 changes: 88 additions & 0 deletions crypto/keys/multisig/amino.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package multisig

import (
types "github.com/cosmos/cosmos-sdk/codec/types"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
)

// tmMultisig implements a K of N threshold multisig. It is used for
// Amino JSON marshaling of LegacyAminoPubKey (see below for details).
//
// This struct is copy-pasted from:
// https://github.com/tendermint/tendermint/blob/v0.33.9/crypto/multisig/threshold_pubkey.go
//
// This struct was used in the SDK <=0.39. In 0.40 and the switch to protobuf,
// it has been converted to LegacyAminoPubKey. However, there's one difference:
// the threshold field was an `uint` before, and an `uint32` after. This caused
// amino marshaling to be breaking: amino marshals `uint32` as a JSON number,
// and `uint` as a JSON string.
//
// In this file, we're overriding LegacyAminoPubKey's default JSON Amino
// marshaling by using this struct. Please note that we are NOT overriding the
// Amino binary marshaling, as that _might_ introduce breaking changes in the
// keyring, where multisigs are amino-binary-encoded.
//
// ref: https://github.com/cosmos/cosmos-sdk/issues/8776
type tmMultisig struct {
K uint `json:"threshold"`
PubKeys []cryptotypes.PubKey `json:"pubkeys"`
}

// protoToTm converts a LegacyAminoPubKey into a tmMultisig.
func protoToTm(protoPk *LegacyAminoPubKey) (tmMultisig, error) {
var ok bool
pks := make([]cryptotypes.PubKey, len(protoPk.PubKeys))
for i, pk := range protoPk.PubKeys {
pks[i], ok = pk.GetCachedValue().(cryptotypes.PubKey)
if !ok {
return tmMultisig{}, sdkerrors.Wrapf(sdkerrors.ErrInvalidType, "expected %T, got %T", (cryptotypes.PubKey)(nil), pk.GetCachedValue())
}
}

return tmMultisig{
K: uint(protoPk.Threshold),
PubKeys: pks,
}, nil
}

// tmToProto converts a tmMultisig into a LegacyAminoPubKey.
func tmToProto(tmPk tmMultisig) (*LegacyAminoPubKey, error) {
var err error
pks := make([]*types.Any, len(tmPk.PubKeys))
for i, pk := range tmPk.PubKeys {
pks[i], err = types.NewAnyWithValue(pk)
if err != nil {
return nil, err
}
}

return &LegacyAminoPubKey{
Threshold: uint32(tmPk.K),
PubKeys: pks,
}, nil
}

// MarshalAminoJSON overrides amino JSON unmarshaling.
func (m LegacyAminoPubKey) MarshalAminoJSON() (tmMultisig, error) { //nolint:golint
return protoToTm(&m)
}

// UnmarshalAminoJSON overrides amino JSON unmarshaling.
func (m *LegacyAminoPubKey) UnmarshalAminoJSON(tmPk tmMultisig) error {
protoPk, err := tmToProto(tmPk)
if err != nil {
return err
}

// 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.
for i := range m.PubKeys {
m.PubKeys[i].TypeUrl = protoPk.PubKeys[i].TypeUrl
m.PubKeys[i].Value = protoPk.PubKeys[i].Value
}
m.Threshold = protoPk.Threshold

return nil
}
3 changes: 3 additions & 0 deletions crypto/keys/multisig/codec.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,9 @@ const (
PubKeyAminoRoute = "tendermint/PubKeyMultisigThreshold"
)

//nolint
// Deprecated: Amino is being deprecated in the SDK. But even if you need to
// use Amino for some reason, please use `codec/legacy.Cdc` instead.
var AminoCdc = codec.NewLegacyAmino()

func init() {
Expand Down
78 changes: 76 additions & 2 deletions crypto/keys/multisig/multisig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ import (
"github.com/stretchr/testify/require"

"github.com/cosmos/cosmos-sdk/codec"
"github.com/cosmos/cosmos-sdk/codec/legacy"
"github.com/cosmos/cosmos-sdk/codec/types"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
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 Down Expand Up @@ -272,12 +274,12 @@ func TestPubKeyMultisigThresholdAminoToIface(t *testing.T) {
pubkeys, _ := generatePubKeysAndSignatures(5, msg)
multisigKey := kmultisig.NewLegacyAminoPubKey(2, pubkeys)

ab, err := kmultisig.AminoCdc.MarshalBinaryLengthPrefixed(multisigKey)
ab, err := legacy.Cdc.MarshalBinaryLengthPrefixed(multisigKey)
require.NoError(t, err)
// like other cryptotypes.Pubkey implementations (e.g. ed25519.PubKey),
// LegacyAminoPubKey should be deserializable into a cryptotypes.LegacyAminoPubKey:
var pubKey kmultisig.LegacyAminoPubKey
err = kmultisig.AminoCdc.UnmarshalBinaryLengthPrefixed(ab, &pubKey)
err = legacy.Cdc.UnmarshalBinaryLengthPrefixed(ab, &pubKey)
require.NoError(t, err)

require.Equal(t, multisigKey.Equals(&pubKey), true)
Expand Down Expand Up @@ -329,3 +331,75 @@ func reorderPubKey(pk *kmultisig.LegacyAminoPubKey) (other *kmultisig.LegacyAmin
other = &kmultisig.LegacyAminoPubKey{Threshold: 2, PubKeys: pubkeysCpy}
return
}

func TestAminoBinary(t *testing.T) {
pubKey1 := secp256k1.GenPrivKey().PubKey()
pubKey2 := secp256k1.GenPrivKey().PubKey()
multisigKey := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pubKey1, pubKey2})

// Do a round-trip key->bytes->key.
bz, err := legacy.Cdc.MarshalBinaryBare(multisigKey)
require.NoError(t, err)
var newMultisigKey cryptotypes.PubKey
err = legacy.Cdc.UnmarshalBinaryBare(bz, &newMultisigKey)
require.NoError(t, err)
require.Equal(t, multisigKey.Threshold, newMultisigKey.(*kmultisig.LegacyAminoPubKey).Threshold)
}

func TestAminoMarshalJSON(t *testing.T) {
pubKey1 := secp256k1.GenPrivKey().PubKey()
pubKey2 := secp256k1.GenPrivKey().PubKey()
multisigKey := kmultisig.NewLegacyAminoPubKey(2, []cryptotypes.PubKey{pubKey1, pubKey2})

bz, err := legacy.Cdc.MarshalJSON(multisigKey)
require.NoError(t, err)

// Note the quotes around `"2"`. They are present because we are overriding
// the Amino JSON marshaling of LegacyAminoPubKey (using tmMultisig).
// Without the override, there would not be any quotes.
require.Contains(t, string(bz), "\"threshold\":\"2\"")
}

func TestAminoUnmarshalJSON(t *testing.T) {
// This is a real multisig from the Akash chain. It has been exported from
// v0.39, hence the `threshold` field as a string.
// We are testing that when unmarshaling this JSON into a LegacyAminoPubKey
// with amino, there's no error.
// ref: https://github.com/cosmos/cosmos-sdk/issues/8776
pkJSON := `{
"type": "tendermint/PubKeyMultisigThreshold",
"value": {
"pubkeys": [
{
"type": "tendermint/PubKeySecp256k1",
"value": "AzYxq2VNeD10TyABwOgV36OVWDIMn8AtI4OFA0uQX2MK"
},
{
"type": "tendermint/PubKeySecp256k1",
"value": "A39cdsrm00bTeQ3RVZVqjkH8MvIViO9o99c8iLiNO35h"
},
{
"type": "tendermint/PubKeySecp256k1",
"value": "A/uLLCZph8MkFg2tCxqSMGwFfPHdt1kkObmmrqy9aiYD"
},
{
"type": "tendermint/PubKeySecp256k1",
"value": "A4mOMhM5gPDtBAkAophjRs6uDGZm4tD4Dbok3ai4qJi8"
},
{
"type": "tendermint/PubKeySecp256k1",
"value": "A90icFucrjNNz2SAdJWMApfSQcARIqt+M2x++t6w5fFs"
}
],
"threshold": "3"
}
}`

cdc := codec.NewLegacyAmino()
cryptocodec.RegisterCrypto(cdc)

var pk cryptotypes.PubKey
err := cdc.UnmarshalJSON([]byte(pkJSON), &pk)
require.NoError(t, err)
require.Equal(t, uint32(3), pk.(*kmultisig.LegacyAminoPubKey).Threshold)
}
2 changes: 1 addition & 1 deletion x/auth/legacy/legacytx/stdtx.go
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ func (tx StdTx) UnpackInterfaces(unpacker codectypes.AnyUnpacker) error {

// Signatures contain PubKeys, which need to be unpacked.
for _, s := range tx.Signatures {
err := codectypes.UnpackInterfaces(s, unpacker)
err := s.UnpackInterfaces(unpacker)
if err != nil {
return err
}
Expand Down
11 changes: 1 addition & 10 deletions x/auth/legacy/v040/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package v040

import (
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
multisigtypes "github.com/cosmos/cosmos-sdk/crypto/keys/multisig"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
v039auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v039"
v040auth "github.com/cosmos/cosmos-sdk/x/auth/types"
Expand All @@ -13,15 +12,7 @@ import (
func convertBaseAccount(old *v039auth.BaseAccount) *v040auth.BaseAccount {
var any *codectypes.Any

_, ok := old.PubKey.(*multisigtypes.LegacyAminoPubKey)

// If pubkey is multisig type, then leave it as nil for now
// Ref: https://github.com/cosmos/cosmos-sdk/issues/8776#issuecomment-790552126
// Else if the old genesis had a pubkey, we pack it inside an Any.
// Or else, we just leave it nil.
if ok {
// TODO migrate multisig public_keys
} else if old.PubKey != nil {
if old.PubKey != nil {
var err error
any, err = codectypes.NewAnyWithValue(old.PubKey)
if err != nil {
Expand Down
2 changes: 2 additions & 0 deletions x/genutil/legacy/v038/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package v038
import (
"github.com/cosmos/cosmos-sdk/client"
"github.com/cosmos/cosmos-sdk/codec"
cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
v036auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v036"
v038auth "github.com/cosmos/cosmos-sdk/x/auth/legacy/v038"
v036distr "github.com/cosmos/cosmos-sdk/x/distribution/legacy/v036"
Expand All @@ -19,6 +20,7 @@ import (
// Migrate migrates exported state from v0.36/v0.37 to a v0.38 genesis state.
func Migrate(appState types.AppMap, _ client.Context) types.AppMap {
v036Codec := codec.NewLegacyAmino()
cryptocodec.RegisterCrypto(v036Codec)
v036gov.RegisterLegacyAminoCodec(v036Codec)
v036distr.RegisterLegacyAminoCodec(v036Codec)
v036params.RegisterLegacyAminoCodec(v036Codec)
Expand Down

0 comments on commit d4d27e1

Please sign in to comment.