From 1ea0e4c457fc105b48131a60e3d28c6c1bb32cc0 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Wed, 21 Nov 2018 05:16:56 -0500 Subject: [PATCH] Merge PR #2863: Transaction ValidateBasic * Add ValidateBasic to Tx interface * Update BaseApp unit tests * Add missing return in ValidateBasic * Update ValidateBasic to use IsNotNegative * Add pending log entry * Add unit test TestTxValidateBasic * Fix broken lint regression * Add sig count check to validation * Add test case to TestTxValidateBasic --- PENDING.md | 5 ++- baseapp/baseapp_test.go | 3 +- types/tx_msg.go | 7 ++-- x/auth/ante.go | 56 +++--------------------------- x/auth/stdtx.go | 56 ++++++++++++++++++++++++++++-- x/auth/stdtx_test.go | 76 +++++++++++++++++++++++++++++++++++++++++ 6 files changed, 146 insertions(+), 57 deletions(-) diff --git a/PENDING.md b/PENDING.md index a3e505c51565..b6273d186a39 100644 --- a/PENDING.md +++ b/PENDING.md @@ -66,7 +66,10 @@ IMPROVEMENTS - [types] #2776 Improve safety of `Coin` and `Coins` types. Various functions and methods will panic when a negative amount is discovered. - #2815 Gas unit fields changed from `int64` to `uint64`. - + - #2821 Codespaces are now strings + - #2779 Introduce `ValidateBasic` to the `Tx` interface and call it in the ante + handler. + * Tendermint - #2796 Update to go-amino 0.14.1 diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index ba3830e920ad..ab6916f82a9c 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -298,7 +298,8 @@ func (tx *txTest) setFailOnHandler(fail bool) { } // Implements Tx -func (tx txTest) GetMsgs() []sdk.Msg { return tx.Msgs } +func (tx txTest) GetMsgs() []sdk.Msg { return tx.Msgs } +func (tx txTest) ValidateBasic() sdk.Error { return nil } const ( routeMsgCounter = "msgCounter" diff --git a/types/tx_msg.go b/types/tx_msg.go index 7882b25d3193..791ab9c3539c 100644 --- a/types/tx_msg.go +++ b/types/tx_msg.go @@ -32,9 +32,12 @@ type Msg interface { // Transactions objects must fulfill the Tx type Tx interface { - - // Gets the Msg. + // Gets the all the transaction's messages. GetMsgs() []Msg + + // ValidateBasic does a simple and lightweight validation check that doesn't + // require access to any other information. + ValidateBasic() Error } //__________________________________________________________ diff --git a/x/auth/ante.go b/x/auth/ante.go index 9a7a15e3e9e9..685d60949f23 100644 --- a/x/auth/ante.go +++ b/x/auth/ante.go @@ -8,7 +8,6 @@ import ( sdk "github.com/cosmos/cosmos-sdk/types" "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/ed25519" - "github.com/tendermint/tendermint/crypto/multisig" "github.com/tendermint/tendermint/crypto/secp256k1" ) @@ -67,28 +66,18 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { } }() - err := validateBasic(stdTx) - if err != nil { + if err := tx.ValidateBasic(); err != nil { return newCtx, err.Result(), true } // charge gas for the memo newCtx.GasMeter().ConsumeGas(memoCostPerByte*sdk.Gas(len(stdTx.GetMemo())), "memo") - // stdSigs contains the sequence number, account number, and signatures - stdSigs := stdTx.GetSignatures() // When simulating, this would just be a 0-length slice. + // stdSigs contains the sequence number, account number, and signatures. + // When simulating, this would just be a 0-length slice. + stdSigs := stdTx.GetSignatures() signerAddrs := stdTx.GetSigners() - sigCount := 0 - for i := 0; i < len(stdSigs); i++ { - sigCount += countSubKeys(stdSigs[i].PubKey) - if sigCount > txSigLimit { - return newCtx, sdk.ErrTooManySignatures(fmt.Sprintf( - "signatures: %d, limit: %d", sigCount, txSigLimit), - ).Result(), true - } - } - // create the list of all sign bytes signBytesList := getSignBytesList(newCtx.ChainID(), stdTx, stdSigs) signerAccs, res := getSignerAccs(newCtx, am, signerAddrs) @@ -129,29 +118,6 @@ func NewAnteHandler(am AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler { } } -// Validate the transaction based on things that don't depend on the context -func validateBasic(tx StdTx) (err sdk.Error) { - // Assert that there are signatures. - sigs := tx.GetSignatures() - if len(sigs) == 0 { - return sdk.ErrUnauthorized("no signers") - } - - // Assert that number of signatures is correct. - var signerAddrs = tx.GetSigners() - if len(sigs) != len(signerAddrs) { - return sdk.ErrUnauthorized("wrong number of signers") - } - - memo := tx.GetMemo() - if len(memo) > maxMemoCharacters { - return sdk.ErrMemoTooLarge( - fmt.Sprintf("maximum number of characters is %d but received %d characters", - maxMemoCharacters, len(memo))) - } - return nil -} - func getSignerAccs(ctx sdk.Context, am AccountKeeper, addrs []sdk.AccAddress) (accs []Account, res sdk.Result) { accs = make([]Account, len(addrs)) for i := 0; i < len(accs); i++ { @@ -310,7 +276,7 @@ func ensureSufficientMempoolFees(ctx sdk.Context, stdTx StdTx) sdk.Result { if stdTx.Fee.Gas <= 0 { return sdk.ErrInternal(fmt.Sprintf("invalid gas supplied: %d", stdTx.Fee.Gas)).Result() } - requiredFees := adjustFeesByGas(ctx.MinimumFees(), uint64(stdTx.Fee.Gas)) + requiredFees := adjustFeesByGas(ctx.MinimumFees(), stdTx.Fee.Gas) // NOTE: !A.IsAllGTE(B) is not the same as A.IsAllLT(B). if !ctx.MinimumFees().IsZero() && !stdTx.Fee.Amount.IsAllGTE(requiredFees) { @@ -340,15 +306,3 @@ func getSignBytesList(chainID string, stdTx StdTx, stdSigs []StdSignature) (sign } return } - -func countSubKeys(pub crypto.PubKey) int { - v, ok := pub.(*multisig.PubKeyMultisigThreshold) - if !ok { - return 1 - } - nkeys := 0 - for _, subkey := range v.PubKeys { - nkeys += countSubKeys(subkey) - } - return nkeys -} diff --git a/x/auth/stdtx.go b/x/auth/stdtx.go index ba1c65b84593..e8b9461fd982 100644 --- a/x/auth/stdtx.go +++ b/x/auth/stdtx.go @@ -2,10 +2,12 @@ package auth import ( "encoding/json" + "fmt" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/tendermint/tendermint/crypto" + "github.com/tendermint/tendermint/crypto/multisig" ) var _ sdk.Tx = (*StdTx)(nil) @@ -28,11 +30,61 @@ func NewStdTx(msgs []sdk.Msg, fee StdFee, sigs []StdSignature, memo string) StdT } } -//nolint +// GetMsgs returns the all the transaction's messages. func (tx StdTx) GetMsgs() []sdk.Msg { return tx.Msgs } +// ValidateBasic does a simple and lightweight validation check that doesn't +// require access to any other information. +func (tx StdTx) ValidateBasic() sdk.Error { + stdSigs := tx.GetSignatures() + + if !tx.Fee.Amount.IsNotNegative() { + return sdk.ErrInsufficientFee(fmt.Sprintf("invalid fee %s amount provided", tx.Fee.Amount)) + } + if len(stdSigs) == 0 { + return sdk.ErrUnauthorized("no signers") + } + if len(stdSigs) != len(tx.GetSigners()) { + return sdk.ErrUnauthorized("wrong number of signers") + } + if len(tx.GetMemo()) > maxMemoCharacters { + return sdk.ErrMemoTooLarge( + fmt.Sprintf( + "maximum number of characters is %d but received %d characters", + maxMemoCharacters, len(tx.GetMemo()), + ), + ) + } + + sigCount := 0 + for i := 0; i < len(stdSigs); i++ { + sigCount += countSubKeys(stdSigs[i].PubKey) + if sigCount > txSigLimit { + return sdk.ErrTooManySignatures( + fmt.Sprintf("signatures: %d, limit: %d", sigCount, txSigLimit), + ) + } + } + + return nil +} + +func countSubKeys(pub crypto.PubKey) int { + v, ok := pub.(*multisig.PubKeyMultisigThreshold) + if !ok { + return 1 + } + + numKeys := 0 + for _, subkey := range v.PubKeys { + numKeys += countSubKeys(subkey) + } + + return numKeys +} + // GetSigners returns the addresses that must sign the transaction. -// Addresses are returned in a determistic order. +// Addresses are returned in a deterministic order. // They are accumulated from the GetSigners method for each Msg // in the order they appear in tx.GetMsgs(). // Duplicate addresses will be omitted. diff --git a/x/auth/stdtx_test.go b/x/auth/stdtx_test.go index 26bd792455a1..a3267ac192a8 100644 --- a/x/auth/stdtx_test.go +++ b/x/auth/stdtx_test.go @@ -2,11 +2,15 @@ package auth import ( "fmt" + "strings" "testing" sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" + abci "github.com/tendermint/tendermint/abci/types" + "github.com/tendermint/tendermint/crypto" "github.com/tendermint/tendermint/crypto/ed25519" + "github.com/tendermint/tendermint/libs/log" ) var ( @@ -51,3 +55,75 @@ func TestStdSignBytes(t *testing.T) { require.Equal(t, tc.want, got, "Got unexpected result on test case i: %d", i) } } + +func TestTxValidateBasic(t *testing.T) { + ctx := sdk.NewContext(nil, abci.Header{ChainID: "mychainid"}, false, log.NewNopLogger()) + + // keys and addresses + priv1, addr1 := privAndAddr() + priv2, addr2 := privAndAddr() + priv3, addr3 := privAndAddr() + priv4, addr4 := privAndAddr() + priv5, addr5 := privAndAddr() + priv6, addr6 := privAndAddr() + priv7, addr7 := privAndAddr() + priv8, addr8 := privAndAddr() + + // msg and signatures + msg1 := newTestMsg(addr1, addr2) + fee := newStdFee() + + msgs := []sdk.Msg{msg1} + + // require to fail validation upon invalid fee + badFee := newStdFee() + badFee.Amount[0].Amount = sdk.NewInt(-5) + tx := newTestTx(ctx, nil, nil, nil, nil, badFee) + + err := tx.ValidateBasic() + require.Error(t, err) + require.Equal(t, sdk.CodeInsufficientFee, err.Result().Code) + + // require to fail validation when no signatures exist + privs, accNums, seqs := []crypto.PrivKey{}, []int64{}, []int64{} + tx = newTestTx(ctx, msgs, privs, accNums, seqs, fee) + + err = tx.ValidateBasic() + require.Error(t, err) + require.Equal(t, sdk.CodeUnauthorized, err.Result().Code) + + // require to fail validation when signatures do not match expected signers + privs, accNums, seqs = []crypto.PrivKey{priv1}, []int64{0, 1}, []int64{0, 0} + tx = newTestTx(ctx, msgs, privs, accNums, seqs, fee) + + err = tx.ValidateBasic() + require.Error(t, err) + require.Equal(t, sdk.CodeUnauthorized, err.Result().Code) + + // require to fail validation when memo is too large + badMemo := strings.Repeat("bad memo", 50) + privs, accNums, seqs = []crypto.PrivKey{priv1, priv2}, []int64{0, 1}, []int64{0, 0} + tx = newTestTxWithMemo(ctx, msgs, privs, accNums, seqs, fee, badMemo) + + err = tx.ValidateBasic() + require.Error(t, err) + require.Equal(t, sdk.CodeMemoTooLarge, err.Result().Code) + + // require to fail validation when there are too many signatures + privs = []crypto.PrivKey{priv1, priv2, priv3, priv4, priv5, priv6, priv7, priv8} + accNums, seqs = []int64{0, 0, 0, 0, 0, 0, 0, 0}, []int64{0, 0, 0, 0, 0, 0, 0, 0} + badMsg := newTestMsg(addr1, addr2, addr3, addr4, addr5, addr6, addr7, addr8) + badMsgs := []sdk.Msg{badMsg} + tx = newTestTx(ctx, badMsgs, privs, accNums, seqs, fee) + + err = tx.ValidateBasic() + require.Error(t, err) + require.Equal(t, sdk.CodeTooManySignatures, err.Result().Code) + + // require to pass when above criteria are matched + privs, accNums, seqs = []crypto.PrivKey{priv1, priv2}, []int64{0, 1}, []int64{0, 0} + tx = newTestTx(ctx, msgs, privs, accNums, seqs, fee) + + err = tx.ValidateBasic() + require.NoError(t, err) +}