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

@@ -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
 

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"

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
 }
 
 //__________________________________________________________

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
-}

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.

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)
+}