From f4f9ea5a4a09276496b1ecb3277ec0b8bff1fae6 Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Thu, 27 Mar 2025 15:43:01 +0000 Subject: [PATCH] feat(client/keys): check multisig key duplicate (backport #18703) (#24153) Co-authored-by: levisyin <150114626+levisyin@users.noreply.github.com> Co-authored-by: aljo242 --- .gitignore | 1 + CHANGELOG.md | 1 + client/keys/add.go | 10 ++++-- client/keys/add_test.go | 68 ++++++++++++++++++++++++++++++++++++++++ client/keys/show.go | 17 +++++++--- client/keys/show_test.go | 12 +++++++ 6 files changed, 103 insertions(+), 6 deletions(-) diff --git a/.gitignore b/.gitignore index e74d19bc51..adf4b7fc2f 100644 --- a/.gitignore +++ b/.gitignore @@ -18,6 +18,7 @@ dist tools-stamp buf-stamp artifacts +simapp/simd/simd # Go go.work diff --git a/CHANGELOG.md b/CHANGELOG.md index 28b8a59130..01bc95b764 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +* (client/keys) [#18703](https://github.com/cosmos/cosmos-sdk/pull/18703) Improve ` keys add` and ` keys show` by checking whether there are duplicate keys in the multisig case. * (client/keys) [#18745](https://github.com/cosmos/cosmos-sdk/pull/18745) Improve ` keys export` and ` keys mnemonic` by adding --yes option to skip interactive confirmation. * (x/bank) [#24106](https://github.com/cosmos/cosmos-sdk/pull/24106) `SendCoins` now checks for `SendRestrictions` before instead of after deducting coins using `subUnlockedCoins`. * (crypto/ledger) [#24036](https://github.com/cosmos/cosmos-sdk/pull/24036) Improve error message when deriving paths using index > 100 diff --git a/client/keys/add.go b/client/keys/add.go index 9e4744e137..81e2071716 100644 --- a/client/keys/add.go +++ b/client/keys/add.go @@ -172,8 +172,14 @@ func runAddCmd(ctx client.Context, cmd *cobra.Command, args []string, inBuf *buf return err } - for i, keyname := range multisigKeys { - k, err := kb.Key(keyname) + seenKeys := make(map[string]struct{}) + for i, keyName := range multisigKeys { + if _, ok := seenKeys[keyName]; ok { + return fmt.Errorf("duplicate multisig keys: %s", keyName) + } + seenKeys[keyName] = struct{}{} + + k, err := kb.Key(keyName) if err != nil { return err } diff --git a/client/keys/add_test.go b/client/keys/add_test.go index 14c593c569..78148f45bf 100644 --- a/client/keys/add_test.go +++ b/client/keys/add_test.go @@ -119,6 +119,74 @@ func Test_runAddCmdBasic(t *testing.T) { require.Error(t, cmd.ExecuteContext(ctx)) } +func Test_runAddCmdMultisigDupKeys(t *testing.T) { + cmd := AddKeyCommand() + cmd.Flags().AddFlagSet(Commands().PersistentFlags()) + + mockIn := testutil.ApplyMockIODiscardOutErr(cmd) + kbHome := t.TempDir() + + cdc := moduletestutil.MakeTestEncodingConfig().Codec + kb, err := keyring.New(sdk.KeyringServiceName(), keyring.BackendTest, kbHome, mockIn, cdc) + require.NoError(t, err) + + clientCtx := client.Context{}. + WithKeyringDir(kbHome). + WithInput(mockIn). + WithCodec(cdc) + + ctx := context.WithValue(context.Background(), client.ClientContextKey, &clientCtx) + + t.Cleanup(func() { + _ = kb.Delete("keyname1") + _ = kb.Delete("keyname2") + _ = kb.Delete("multisigname") + }) + + cmd.SetArgs([]string{ + "keyname1", + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText), + fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + }) + require.NoError(t, cmd.ExecuteContext(ctx)) + + cmd.SetArgs([]string{ + "keyname2", + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText), + fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + }) + require.NoError(t, cmd.ExecuteContext(ctx)) + + cmd.SetArgs([]string{ + "multisigname", + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText), + fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname2"), + fmt.Sprintf("--%s=%s", flagMultiSigThreshold, "2"), + }) + require.NoError(t, cmd.ExecuteContext(ctx)) + + cmd.SetArgs([]string{ + "multisigname", + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", flags.FlagOutput, flags.OutputFormatText), + fmt.Sprintf("--%s=%s", flags.FlagKeyType, hd.Secp256k1Type), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + fmt.Sprintf("--%s=%s", flagMultisig, "keyname1,keyname1"), + fmt.Sprintf("--%s=%s", flagMultiSigThreshold, "2"), + }) + mockIn.Reset("y\n") + require.Error(t, cmd.ExecuteContext(ctx)) + mockIn.Reset("y\n") + require.EqualError(t, cmd.ExecuteContext(ctx), "duplicate multisig keys: keyname1") +} + func Test_runAddCmdDryRun(t *testing.T) { pubkey1 := `{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"AtObiFVE4s+9+RX5SP8TN9r2mxpoaT4eGj9CJfK7VRzN"}` pubkey2 := `{"@type":"/cosmos.crypto.secp256k1.PubKey","key":"A/se1vkqgdQ7VJQCM4mxN+L+ciGhnnJ4XYsQCRBMrdRi"}` diff --git a/client/keys/show.go b/client/keys/show.go index dc57d30deb..03d80a457f 100644 --- a/client/keys/show.go +++ b/client/keys/show.go @@ -70,11 +70,20 @@ func runShowCmd(cmd *cobra.Command, args []string) (err error) { } } else { pks := make([]cryptotypes.PubKey, len(args)) - for i, keyref := range args { - k, err := fetchKey(clientCtx.Keyring, keyref) - if err != nil { - return fmt.Errorf("%s is not a valid name or address: %w", keyref, err) + seenKeys := make(map[string]struct{}) + for i, keyRef := range args { + if _, ok := seenKeys[keyRef]; ok { + // we just show warning message instead of return error in case someone relies on this behavior. + cmd.PrintErrf("WARNING: duplicate keys found: %s.\n\n", keyRef) + } else { + seenKeys[keyRef] = struct{}{} } + + k, err := fetchKey(clientCtx.Keyring, keyRef) + if err != nil { + return fmt.Errorf("%s is not a valid name or address: %w", keyRef, err) + } + key, err := k.GetPubKey() if err != nil { return err diff --git a/client/keys/show_test.go b/client/keys/show_test.go index 286efeadc3..bbde9376a3 100644 --- a/client/keys/show_test.go +++ b/client/keys/show_test.go @@ -127,6 +127,18 @@ func Test_runShowCmd(t *testing.T) { }) require.EqualError(t, cmd.ExecuteContext(ctx), "threshold must be a positive integer") + // Now try multisig key duplicate + _, mockOut := testutil.ApplyMockIO(cmd) + cmd.SetArgs([]string{ + fakeKeyName1, fakeKeyName1, + fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome), + fmt.Sprintf("--%s=%s", FlagBechPrefix, sdk.PrefixAccount), + fmt.Sprintf("--%s=2", flagMultiSigThreshold), + fmt.Sprintf("--%s=%s", flags.FlagKeyringBackend, keyring.BackendTest), + }) + require.NoError(t, cmd.ExecuteContext(ctx)) + require.Contains(t, mockOut.String(), fmt.Sprintf("WARNING: duplicate keys found: %s", fakeKeyName1)) + cmd.SetArgs([]string{ fakeKeyName1, fakeKeyName2, fmt.Sprintf("--%s=%s", flags.FlagKeyringDir, kbHome),