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: remove hardcoded pubkey from tx simulation #11282

Merged
merged 3 commits into from
Feb 28, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Bug Fixes

* (client) [\#11283](https://github.com/cosmos/cosmos-sdk/issues/11283) Support multiple keys for tx simulation and setting automatic gas for txs.
* (store) [\#11177](https://github.com/cosmos/cosmos-sdk/pull/11177) Update the prune `everything` strategy to store the last two heights.
* [\#10844](https://github.com/cosmos/cosmos-sdk/pull/10844) Automatic recovering non-consistent keyring storage during public key import.
* (store) [\#11117](https://github.com/cosmos/cosmos-sdk/pull/11117) Fix data race in store trace component
Expand Down
24 changes: 23 additions & 1 deletion client/tx/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/crypto/keyring"
"github.com/cosmos/cosmos-sdk/crypto/keys/secp256k1"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/tx"
"github.com/cosmos/cosmos-sdk/types/tx/signing"
Expand Down Expand Up @@ -306,10 +307,31 @@ func (f Factory) BuildSimTx(msgs ...sdk.Msg) ([]byte, error) {
return nil, err
}

// use the first element from the list of keys in order to generate a valid
// pubkey that supports multiple algorithms

Choose a reason for hiding this comment

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

nit: extra line?

var (
ok bool
pk cryptotypes.PubKey = &secp256k1.PubKey{} // use default public key type
)

if 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")
}

// take the first record just for simulation purposes
pk, ok = records[0].PubKey.GetCachedValue().(cryptotypes.PubKey)
if !ok {
return nil, errors.New("cannot build signature for simulation, failed to convert proto Any to public key")
}
}

// Create an empty signature literal as the ante handler will populate with a
// sentinel pubkey.
sig := signing.SignatureV2{
PubKey: &secp256k1.PubKey{},
PubKey: pk,
Data: &signing.SingleSignatureData{
SignMode: f.signMode,
},
Expand Down
120 changes: 92 additions & 28 deletions client/tx/tx_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func (m mockContext) Invoke(grpcCtx gocontext.Context, method string, req, reply

return nil
}

func (mockContext) NewStream(gocontext.Context, *grpc.StreamDesc, string, ...grpc.CallOption) (grpc.ClientStream, error) {
panic("not implemented")
}
Expand Down Expand Up @@ -96,6 +97,14 @@ func TestCalculateGas(t *testing.T) {

func TestBuildSimTx(t *testing.T) {
txCfg := NewTestTxConfig()
encCfg := simapp.MakeTestEncodingConfig()

kb, err := keyring.New(t.Name(), "test", t.TempDir(), nil, encCfg.Codec)
require.NoError(t, err)

path := hd.CreateHDPath(118, 0, 0).String()
_, _, err = kb.NewMnemonic("test_key1", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
require.NoError(t, err)

txf := tx.Factory{}.
WithTxConfig(txCfg).
Expand All @@ -104,7 +113,8 @@ func TestBuildSimTx(t *testing.T) {
WithFees("50stake").
WithMemo("memo").
WithChainID("test-chain").
WithSignMode(txCfg.SignModeHandler().DefaultMode())
WithSignMode(txCfg.SignModeHandler().DefaultMode()).
WithKeybase(kb)

msg := banktypes.NewMsgSend(sdk.AccAddress("from"), sdk.AccAddress("to"), nil)
bz, err := txf.BuildSimTx(msg)
Expand All @@ -113,13 +123,23 @@ func TestBuildSimTx(t *testing.T) {
}

func TestBuildUnsignedTx(t *testing.T) {
encCfg := simapp.MakeTestEncodingConfig()
kb, err := keyring.New(t.Name(), "test", t.TempDir(), nil, encCfg.Codec)
require.NoError(t, err)

path := hd.CreateHDPath(118, 0, 0).String()

_, _, err = kb.NewMnemonic("test_key1", keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
require.NoError(t, err)

txf := tx.Factory{}.
WithTxConfig(NewTestTxConfig()).
WithAccountNumber(50).
WithSequence(23).
WithFees("50stake").
WithMemo("memo").
WithChainID("test-chain")
WithChainID("test-chain").
WithKeybase(kb)

msg := banktypes.NewMsgSend(sdk.AccAddress("from"), sdk.AccAddress("to"), nil)
tx, err := txf.BuildUnsignedTx(msg)
Expand All @@ -138,8 +158,8 @@ func TestSign(t *testing.T) {
kb, err := keyring.New(t.Name(), "test", t.TempDir(), nil, encCfg.Codec)
requireT.NoError(err)

var from1 = "test_key1"
var from2 = "test_key2"
from1 := "test_key1"
from2 := "test_key2"

// create a new key using a mnemonic generator and test if we can reuse seed to recreate that account
_, seed, err := kb.NewMnemonic(from1, keyring.English, path, keyring.DefaultBIP39Passphrase, hd.Secp256k1)
Expand Down Expand Up @@ -192,37 +212,81 @@ func TestSign(t *testing.T) {
expectedPKs []cryptotypes.PubKey
matchingSigs []int // if not nil, check matching signature against old ones.
}{
{"should fail if txf without keyring",
txfNoKeybase, txb, from1, true, nil, nil},
{"should fail for non existing key",
txfAmino, txb, "unknown", true, nil, nil},
{"amino: should succeed with keyring",
txfAmino, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
{"direct: should succeed with keyring",
txfDirect, txbSimple, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
{
"should fail if txf without keyring",
txfNoKeybase, txb, from1, true, nil, nil,
},
{
"should fail for non existing key",
txfAmino, txb, "unknown", true, nil, nil,
},
{
"amino: should succeed with keyring",
txfAmino, txbSimple, from1, true,
[]cryptotypes.PubKey{pubKey1},
nil,
},
{
"direct: should succeed with keyring",
txfDirect, txbSimple, from1, true,
[]cryptotypes.PubKey{pubKey1},
nil,
},

/**** test double sign Amino mode ****/
{"amino: should sign multi-signers tx",
txfAmino, txb, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
{"amino: should append a second signature and not overwrite",
txfAmino, txb, from2, false, []cryptotypes.PubKey{pubKey1, pubKey2}, []int{0, 0}},
{"amino: should overwrite a signature",
txfAmino, txb, from2, true, []cryptotypes.PubKey{pubKey2}, []int{1, 0}},
{
"amino: should sign multi-signers tx",
txfAmino, txb, from1, true,
[]cryptotypes.PubKey{pubKey1},
nil,
},
{
"amino: should append a second signature and not overwrite",
txfAmino, txb, from2, false,
[]cryptotypes.PubKey{pubKey1, pubKey2},
[]int{0, 0},
},
{
"amino: should overwrite a signature",
txfAmino, txb, from2, true,
[]cryptotypes.PubKey{pubKey2},
[]int{1, 0},
},

/**** test double sign Direct mode
signing transaction with 2 or more DIRECT signers should fail in DIRECT mode ****/
{"direct: should append a DIRECT signature with existing AMINO",
{
"direct: should append a DIRECT signature with existing AMINO",
// txb already has 1 AMINO signature
txfDirect, txb, from1, false, []cryptotypes.PubKey{pubKey2, pubKey1}, nil},
{"direct: should add single DIRECT sig in multi-signers tx",
txfDirect, txb2, from1, false, []cryptotypes.PubKey{pubKey1}, nil},
{"direct: should fail to append 2nd DIRECT sig in multi-signers tx",
txfDirect, txb2, from2, false, []cryptotypes.PubKey{}, nil},
{"amino: should append 2nd AMINO sig in multi-signers tx with 1 DIRECT sig",
txfDirect, txb, from1, false,
[]cryptotypes.PubKey{pubKey2, pubKey1},
nil,
},
{
"direct: should add single DIRECT sig in multi-signers tx",
txfDirect, txb2, from1, false,
[]cryptotypes.PubKey{pubKey1},
nil,
},
{
"direct: should fail to append 2nd DIRECT sig in multi-signers tx",
txfDirect, txb2, from2, false,
[]cryptotypes.PubKey{},
nil,
},
{
"amino: should append 2nd AMINO sig in multi-signers tx with 1 DIRECT sig",
// txb2 already has 1 DIRECT signature
txfAmino, txb2, from2, false, []cryptotypes.PubKey{}, nil},
{"direct: should overwrite multi-signers tx with DIRECT sig",
txfDirect, txb2, from1, true, []cryptotypes.PubKey{pubKey1}, nil},
txfAmino, txb2, from2, false,
[]cryptotypes.PubKey{},
nil,
},
{
"direct: should overwrite multi-signers tx with DIRECT sig",
txfDirect, txb2, from1, true,
[]cryptotypes.PubKey{pubKey1},
nil,
},
}

var prevSigs []signingtypes.SignatureV2
Expand Down