From 514ef91f2e022a0dc70889367d577033ea20f5fa Mon Sep 17 00:00:00 2001 From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com> Date: Sat, 18 Nov 2023 00:56:54 +0100 Subject: [PATCH] fix(client/tx): simulate with correct pk (backport #18472) (#18502) --- CHANGELOG.md | 3 +- client/tx/aux_builder_test.go | 15 ++++++ client/tx/factory.go | 40 +++++++++++---- client/tx/factory_test.go | 96 ++++++++++++++++++++++++++++++++--- client/tx/legacy_test.go | 21 -------- client/tx/tx_test.go | 21 ++++---- simapp/simd/cmd/commands.go | 2 +- 7 files changed, 148 insertions(+), 50 deletions(-) delete mode 100644 client/tx/legacy_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index f4df1c9c048f..809057644030 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,7 +51,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Bug Fixes -* (baseapp) [#18486](https://github.com/cosmos/cosmos-sdk/pull/18486) Fixed FinalizeBlock calls not being passed to ABCIListeners +* (client/tx) [#18472](https://github.com/cosmos/cosmos-sdk/pull/18472) Utilizes the correct Pubkey when simulating a transaction. +* (baseapp) [#18486](https://github.com/cosmos/cosmos-sdk/pull/18486) Fixed FinalizeBlock calls not being passed to ABCIListeners. ## [v0.50.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.50.1) - 2023-11-07 diff --git a/client/tx/aux_builder_test.go b/client/tx/aux_builder_test.go index af61fc098187..f34902b9a998 100644 --- a/client/tx/aux_builder_test.go +++ b/client/tx/aux_builder_test.go @@ -15,6 +15,21 @@ import ( typestx "github.com/cosmos/cosmos-sdk/types/tx" "github.com/cosmos/cosmos-sdk/types/tx/signing" "github.com/cosmos/cosmos-sdk/x/bank" + banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" +) + +const ( + memo = "waboom" + timeoutHeight = uint64(5) +) + +var ( + _, pub1, addr1 = testdata.KeyTestPubAddr() + _, _, addr2 = testdata.KeyTestPubAddr() + rawSig = []byte("dummy") + msg1 = banktypes.NewMsgSend(addr1, addr2, sdk.NewCoins(sdk.NewInt64Coin("wack", 2))) + + chainID = "test-chain" ) func TestAuxTxBuilder(t *testing.T) { diff --git a/client/tx/factory.go b/client/tx/factory.go index ab73a550d245..7ab4fe5cbb84 100644 --- a/client/tx/factory.go +++ b/client/tx/factory.go @@ -15,6 +15,7 @@ import ( "github.com/cosmos/cosmos-sdk/client/flags" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/crypto/keyring" + "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types" sdk "github.com/cosmos/cosmos-sdk/types" @@ -33,6 +34,7 @@ type Factory struct { timeoutHeight uint64 gasAdjustment float64 chainID string + fromName string offline bool generateOnly bool memo string @@ -84,6 +86,7 @@ func NewFactoryCLI(clientCtx client.Context, flagSet *pflag.FlagSet) (Factory, e accountRetriever: clientCtx.AccountRetriever, keybase: clientCtx.Keyring, chainID: clientCtx.ChainID, + fromName: clientCtx.FromName, offline: clientCtx.Offline, generateOnly: clientCtx.GenerateOnly, gas: gasSetting.Gas, @@ -398,10 +401,8 @@ func (f Factory) BuildSimTx(msgs ...sdk.Msg) ([]byte, error) { // Create an empty signature literal as the ante handler will populate with a // sentinel pubkey. sig := signing.SignatureV2{ - PubKey: pk, - Data: &signing.SingleSignatureData{ - SignMode: f.signMode, - }, + PubKey: pk, + Data: f.getSimSignatureData(pk), Sequence: f.Sequence(), } if err := txb.SetSignatures(sig); err != nil { @@ -427,16 +428,13 @@ func (f Factory) getSimPK() (cryptotypes.PubKey, error) { pk cryptotypes.PubKey = &secp256k1.PubKey{} // use default public key type ) - // Use the first element from the list of keys in order to generate a valid - // pubkey that supports multiple algorithms. if f.simulateAndExecute && f.keybase != nil { - records, _ := f.keybase.List() - if len(records) == 0 { - return nil, errors.New("cannot build signature for simulation, key records slice is empty") + record, err := f.keybase.Key(f.fromName) + if err != nil { + return nil, err } - // take the first record just for simulation purposes - pk, ok = records[0].PubKey.GetCachedValue().(cryptotypes.PubKey) + pk, ok = record.PubKey.GetCachedValue().(cryptotypes.PubKey) if !ok { return nil, errors.New("cannot build signature for simulation, failed to convert proto Any to public key") } @@ -445,6 +443,26 @@ func (f Factory) getSimPK() (cryptotypes.PubKey, error) { return pk, nil } +// getSimSignatureData based on the pubKey type gets the correct SignatureData type +// to use for building a simulation tx. +func (f Factory) getSimSignatureData(pk cryptotypes.PubKey) signing.SignatureData { + multisigPubKey, ok := pk.(*multisig.LegacyAminoPubKey) + if !ok { + return &signing.SingleSignatureData{SignMode: f.signMode} + } + + multiSignatureData := make([]signing.SignatureData, 0, multisigPubKey.Threshold) + for i := uint32(0); i < multisigPubKey.Threshold; i++ { + multiSignatureData = append(multiSignatureData, &signing.SingleSignatureData{ + SignMode: f.SignMode(), + }) + } + + return &signing.MultiSignatureData{ + Signatures: multiSignatureData, + } +} + // Prepare ensures the account defined by ctx.GetFromAddress() exists and // if the account number and/or the account sequence number are zero (not set), // they will be queried for and set on the provided Factory. diff --git a/client/tx/factory_test.go b/client/tx/factory_test.go index 3b055781bd0b..402d178a98dc 100644 --- a/client/tx/factory_test.go +++ b/client/tx/factory_test.go @@ -1,4 +1,4 @@ -package tx_test +package tx import ( "testing" @@ -6,30 +6,114 @@ import ( "github.com/stretchr/testify/require" "github.com/cosmos/cosmos-sdk/client" - "github.com/cosmos/cosmos-sdk/client/tx" + "github.com/cosmos/cosmos-sdk/codec" + codectypes "github.com/cosmos/cosmos-sdk/codec/types" + cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" + "github.com/cosmos/cosmos-sdk/crypto/hd" + "github.com/cosmos/cosmos-sdk/crypto/keyring" + "github.com/cosmos/cosmos-sdk/crypto/keys/multisig" + "github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1" + "github.com/cosmos/cosmos-sdk/crypto/types" + "github.com/cosmos/cosmos-sdk/testutil/testdata" + "github.com/cosmos/cosmos-sdk/types/tx/signing" ) -func TestFactoryPrepate(t *testing.T) { +func TestFactoryPrepare(t *testing.T) { t.Parallel() - factory := tx.Factory{} + factory := Factory{} clientCtx := client.Context{} output, err := factory.Prepare(clientCtx.WithOffline(true)) require.NoError(t, err) require.Equal(t, output, factory) - factory = tx.Factory{}.WithAccountRetriever(client.MockAccountRetriever{ReturnAccNum: 10, ReturnAccSeq: 1}).WithAccountNumber(5) + factory = Factory{}.WithAccountRetriever(client.MockAccountRetriever{ReturnAccNum: 10, ReturnAccSeq: 1}).WithAccountNumber(5) output, err = factory.Prepare(clientCtx.WithFrom("foo")) require.NoError(t, err) require.NotEqual(t, output, factory) require.Equal(t, output.AccountNumber(), uint64(5)) require.Equal(t, output.Sequence(), uint64(1)) - factory = tx.Factory{}.WithAccountRetriever(client.MockAccountRetriever{ReturnAccNum: 10, ReturnAccSeq: 1}) + factory = Factory{}.WithAccountRetriever(client.MockAccountRetriever{ReturnAccNum: 10, ReturnAccSeq: 1}) output, err = factory.Prepare(clientCtx.WithFrom("foo")) require.NoError(t, err) require.NotEqual(t, output, factory) require.Equal(t, output.AccountNumber(), uint64(10)) require.Equal(t, output.Sequence(), uint64(1)) } + +func TestFactory_getSimPKType(t *testing.T) { + // setup keyring + registry := codectypes.NewInterfaceRegistry() + cryptocodec.RegisterInterfaces(registry) + k := keyring.NewInMemory(codec.NewProtoCodec(registry)) + + tests := []struct { + name string + fromName string + genKey func(fromName string, k keyring.Keyring) error + wantType types.PubKey + }{ + { + name: "simple key", + fromName: "testKey", + genKey: func(fromName string, k keyring.Keyring) error { + _, err := k.NewAccount(fromName, testdata.TestMnemonic, "", "", hd.Secp256k1) + return err + }, + wantType: (*secp256k1.PubKey)(nil), + }, + { + name: "multisig key", + fromName: "multiKey", + genKey: func(fromName string, k keyring.Keyring) error { + pk := multisig.NewLegacyAminoPubKey(1, []types.PubKey{&multisig.LegacyAminoPubKey{}}) + _, err := k.SaveMultisig(fromName, pk) + return err + }, + wantType: (*multisig.LegacyAminoPubKey)(nil), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tt.genKey(tt.fromName, k) + require.NoError(t, err) + f := Factory{ + keybase: k, + fromName: tt.fromName, + simulateAndExecute: true, + } + got, err := f.getSimPK() + require.NoError(t, err) + require.IsType(t, tt.wantType, got) + }) + } +} + +func TestFactory_getSimSignatureData(t *testing.T) { + tests := []struct { + name string + pk types.PubKey + wantType any + }{ + { + name: "simple pubkey", + pk: &secp256k1.PubKey{}, + wantType: (*signing.SingleSignatureData)(nil), + }, + { + name: "multisig pubkey", + pk: &multisig.LegacyAminoPubKey{}, + wantType: (*signing.MultiSignatureData)(nil), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := Factory{}.getSimSignatureData(tt.pk) + require.IsType(t, tt.wantType, got) + }) + } +} diff --git a/client/tx/legacy_test.go b/client/tx/legacy_test.go deleted file mode 100644 index 53751d4d90da..000000000000 --- a/client/tx/legacy_test.go +++ /dev/null @@ -1,21 +0,0 @@ -package tx_test - -import ( - "github.com/cosmos/cosmos-sdk/testutil/testdata" - "github.com/cosmos/cosmos-sdk/types" - banktypes "github.com/cosmos/cosmos-sdk/x/bank/types" -) - -const ( - memo = "waboom" - timeoutHeight = uint64(5) -) - -var ( - _, pub1, addr1 = testdata.KeyTestPubAddr() - _, _, addr2 = testdata.KeyTestPubAddr() - rawSig = []byte("dummy") - msg1 = banktypes.NewMsgSend(addr1, addr2, types.NewCoins(types.NewInt64Coin("wack", 2))) - - chainID = "test-chain" -) diff --git a/client/tx/tx_test.go b/client/tx/tx_test.go index 36ece26b92de..c234739293a5 100644 --- a/client/tx/tx_test.go +++ b/client/tx/tx_test.go @@ -1,4 +1,4 @@ -package tx_test +package tx import ( "context" @@ -10,7 +10,6 @@ import ( "google.golang.org/grpc" "github.com/cosmos/cosmos-sdk/client" - "github.com/cosmos/cosmos-sdk/client/tx" "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/crypto/hd" @@ -80,7 +79,7 @@ func TestCalculateGas(t *testing.T) { defaultSignMode, err := signing.APISignModeToInternal(txCfg.SignModeHandler().DefaultMode()) require.NoError(t, err) - txf := tx.Factory{}. + txf := Factory{}. WithChainID("test-chain"). WithTxConfig(txCfg).WithSignMode(defaultSignMode) @@ -89,7 +88,7 @@ func TestCalculateGas(t *testing.T) { gasUsed: tc.args.mockGasUsed, wantErr: tc.args.mockWantErr, } - simRes, gotAdjusted, err := tx.CalculateGas(mockClientCtx, txf.WithGasAdjustment(stc.args.adjustment)) + simRes, gotAdjusted, err := CalculateGas(mockClientCtx, txf.WithGasAdjustment(stc.args.adjustment)) if stc.expPass { require.NoError(t, err) require.Equal(t, simRes.GasInfo.GasUsed, stc.wantEstimate) @@ -103,8 +102,8 @@ func TestCalculateGas(t *testing.T) { } } -func mockTxFactory(txCfg client.TxConfig) tx.Factory { - return tx.Factory{}. +func mockTxFactory(txCfg client.TxConfig) Factory { + return Factory{}. WithTxConfig(txCfg). WithAccountNumber(50). WithSequence(23). @@ -195,7 +194,7 @@ func TestMnemonicInMemo(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - txf := tx.Factory{}. + txf := Factory{}. WithTxConfig(txConfig). WithAccountNumber(50). WithSequence(23). @@ -266,7 +265,7 @@ func TestSign(t *testing.T) { testCases := []struct { name string - txf tx.Factory + txf Factory txb client.TxBuilder from string overwrite bool @@ -353,7 +352,7 @@ func TestSign(t *testing.T) { var prevSigs []signingtypes.SignatureV2 for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - err = tx.Sign(context.TODO(), tc.txf, tc.from, tc.txb, tc.overwrite) + err = Sign(context.TODO(), tc.txf, tc.from, tc.txb, tc.overwrite) if len(tc.expectedPKs) == 0 { requireT.Error(err) } else { @@ -369,6 +368,8 @@ func TestSign(t *testing.T) { } func TestPreprocessHook(t *testing.T) { + _, _, addr2 := testdata.KeyTestPubAddr() + txConfig, cdc := newTestTxConfig() requireT := require.New(t) path := hd.CreateHDPath(118, 0, 0).String() @@ -408,7 +409,7 @@ func TestPreprocessHook(t *testing.T) { txb, err := txfDirect.BuildUnsignedTx(msg1, msg2) requireT.NoError(err) - err = tx.Sign(context.TODO(), txfDirect, from, txb, false) + err = Sign(context.TODO(), txfDirect, from, txb, false) requireT.NoError(err) // Run preprocessing diff --git a/simapp/simd/cmd/commands.go b/simapp/simd/cmd/commands.go index 3607670e5369..224c4d67c66b 100644 --- a/simapp/simd/cmd/commands.go +++ b/simapp/simd/cmd/commands.go @@ -76,7 +76,7 @@ func initAppConfig() (string, interface{}) { // In summary: // - if you leave srvCfg.MinGasPrices = "", all validators MUST tweak their // own app.toml config, - // - if you set srvCfg.MinGasPrices non-empty, validatorcan be used to extend the app.toml.s CAN tweak their + // - if you set srvCfg.MinGasPrices non-empty, validators CAN tweak their // own app.toml to override, or use this default value. // // In simapp, we set the min gas prices to 0.