R4R: Merge PRs: 2797, 2800 and 2863

app/sim_test.go

@@ -53,7 +53,6 @@ func appStateFn(r *rand.Rand, accs []simulation.Account) json.RawMessage {
 	var genesisAccounts []GenesisFileAccount
 
 	amount := sdk.NewIntWithDecimal(100, 18)
-	stakeAmount := sdk.NewIntWithDecimal(1, 2)
 	numInitiallyBonded := int64(r.Intn(250))
 	//numInitiallyBonded := int64(4)
 	numAccs := int64(len(accs))
@@ -64,16 +63,7 @@ func appStateFn(r *rand.Rand, accs []simulation.Account) json.RawMessage {
 
 	// Randomly generate some genesis accounts
 	for _, acc := range accs {
-		coins := sdk.Coins{
-			{
-				Denom:  stakeTypes.StakeDenom,
-				Amount: amount,
-			},
-			{
-				Denom:  stakeGenesis.Params.BondDenom,
-				Amount: stakeAmount,
-			},
-		}
+		coins := sdk.Coins{sdk.NewCoin(stakeTypes.StakeDenom, amount)}
 		var coinsStringArray []string
 		for _, coin := range coins {
 			coinsStringArray = append(coinsStringArray, coin.String())

modules/auth/ante.go

@@ -17,6 +17,8 @@ const (
 	maxMemoCharacters           = 100
 	// how much gas = 1 atom
 	gasPerUnitCost = 1000
+	// max total number of sigs per tx
+	txSigLimit = 7
 )
 
 // NewAnteHandler returns an AnteHandler that checks
@@ -63,15 +65,15 @@ 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()
 
 		// create the list of all sign bytes
@@ -114,29 +116,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++ {
@@ -263,8 +242,11 @@ func deductFees(acc Account, fee StdFee) (Account, sdk.Result) {
 	coins := acc.GetCoins()
 	feeAmount := fee.Amount
 
-	newCoins := coins.Minus(feeAmount)
-	if !newCoins.IsNotNegative() {
+	if !feeAmount.IsValid() {
+		return nil, sdk.ErrInsufficientFee(fmt.Sprintf("invalid fee amount: %s", feeAmount)).Result()
+	}
+	newCoins, ok := coins.SafeMinus(feeAmount)
+	if ok {
 		errMsg := fmt.Sprintf("%s < %s", coins, feeAmount)
 		return nil, sdk.ErrInsufficientFunds(errMsg).Result()
 	}
@@ -279,7 +261,13 @@ func deductFees(acc Account, fee StdFee) (Account, sdk.Result) {
 func ensureSufficientMempoolFees(ctx sdk.Context, stdTx StdTx) sdk.Result {
 	// currently we use a very primitive gas pricing model with a constant gasPrice.
 	// adjustFeesByGas handles calculating the amount of fees required based on the provided gas.
-	// TODO: Make the gasPrice not a constant, and account for tx size.
+	//
+	// TODO:
+	// - Make the gasPrice not a constant, and account for tx size.
+	// - Make Gas an unsigned integer and use tx basic validation
+	if stdTx.Fee.Gas <= 0 {
+		return sdk.ErrInternal(fmt.Sprintf("invalid gas supplied: %d", stdTx.Fee.Gas)).Result()
+	}
 	requiredFees := adjustFeesByGas(ctx.MinimumFees(), stdTx.Fee.Gas)
 
 	// NOTE: !A.IsAllGTE(B) is not the same as A.IsAllLT(B).

modules/auth/ante_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/tendermint/tendermint/crypto/ed25519"
 	"github.com/tendermint/tendermint/crypto/secp256k1"
 	"github.com/tendermint/tendermint/libs/log"
+	"github.com/tendermint/tendermint/crypto/multisig"
 )
 
 func newTestMsg(addrs ...sdk.AccAddress) *sdk.TestMsg {
@@ -706,11 +707,79 @@ func TestAdjustFeesByGas(t *testing.T) {
 	}{
 		{"nil coins", args{sdk.Coins{}, 10000}, sdk.Coins{}},
 		{"nil coins", args{sdk.Coins{sdk.NewInt64Coin("A", 10), sdk.NewInt64Coin("B", 0)}, 10000}, sdk.Coins{sdk.NewInt64Coin("A", 20), sdk.NewInt64Coin("B", 10)}},
-		{"negative coins", args{sdk.Coins{sdk.NewInt64Coin("A", -10), sdk.NewInt64Coin("B", 10)}, 10000}, sdk.Coins{sdk.NewInt64Coin("B", 20)}},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			require.True(t, tt.want.IsEqual(adjustFeesByGas(tt.args.fee, tt.args.gas)))
 		})
 	}
 }
+
+func TestCountSubkeys(t *testing.T) {
+	genPubKeys := func(n int) []crypto.PubKey {
+		var ret []crypto.PubKey
+		for i := 0; i < n; i++ {
+			ret = append(ret, secp256k1.GenPrivKey().PubKey())
+		}
+		return ret
+	}
+	genMultiKey := func(n, k int, keysGen func(n int) []crypto.PubKey) crypto.PubKey {
+		return multisig.NewPubKeyMultisigThreshold(k, keysGen(n))
+	}
+	type args struct {
+		pub crypto.PubKey
+	}
+	mkey := genMultiKey(5, 4, genPubKeys)
+	mkeyType := mkey.(*multisig.PubKeyMultisigThreshold)
+	mkeyType.PubKeys = append(mkeyType.PubKeys, genMultiKey(6, 5, genPubKeys))
+	tests := []struct {
+		name string
+		args args
+		want int
+	}{
+		{"single key", args{secp256k1.GenPrivKey().PubKey()}, 1},
+		{"multi sig key", args{genMultiKey(5, 4, genPubKeys)}, 5},
+		{"multi multi sig", args{mkey}, 11},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(T *testing.T) {
+			require.Equal(t, tt.want, countSubKeys(tt.args.pub))
+		})
+	}
+}
+func TestAnteHandlerSigLimitExceeded(t *testing.T) {
+	// setup
+	ms, capKey, capKey2 := setupMultiStore()
+	cdc := codec.New()
+	RegisterBaseAccount(cdc)
+	mapper := NewAccountKeeper(cdc, capKey, ProtoBaseAccount)
+	feeCollector := NewFeeCollectionKeeper(cdc, capKey2)
+	anteHandler := NewAnteHandler(mapper, feeCollector)
+	ctx := sdk.NewContext(ms, abci.Header{ChainID: "mychainid"}, false, log.NewNopLogger())
+	ctx = ctx.WithBlockHeight(1)
+	// 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()
+	// set the accounts
+	acc1 := mapper.NewAccountWithAddress(ctx, addr1)
+	acc1.SetCoins(newCoins())
+	mapper.SetAccount(ctx, acc1)
+	acc2 := mapper.NewAccountWithAddress(ctx, addr2)
+	acc2.SetCoins(newCoins())
+	mapper.SetAccount(ctx, acc2)
+	var tx sdk.Tx
+	msg := newTestMsg(addr1, addr2, addr3, addr4, addr5, addr6, addr7, addr8)
+	msgs := []sdk.Msg{msg}
+	fee := newStdFee()
+	// test rejection logic
+	privs, accnums, seqs := []crypto.PrivKey{priv1, priv2, priv3, priv4, priv5, priv6, priv7, priv8},
+		[]int64{0, 0, 0, 0, 0, 0, 0, 0}, []int64{0, 0, 0, 0, 0, 0, 0, 0}
+	tx = newTestTx(ctx, msgs, privs, accnums, seqs, fee)
+	checkInvalidTx(t, anteHandler, ctx, tx, false, sdk.CodeTooManySignatures)
+}

modules/auth/stdtx.go

@@ -6,6 +6,8 @@ import (
 	"github.com/irisnet/irishub/codec"
 	sdk "github.com/irisnet/irishub/types"
 	"github.com/tendermint/tendermint/crypto"
+	"fmt"
+	"github.com/tendermint/tendermint/crypto/multisig"
 )
 
 var _ sdk.Tx = (*StdTx)(nil)
@@ -29,10 +31,55 @@ 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.

modules/auth/stdtx_test.go

@@ -2,11 +2,15 @@ package auth
 
 import (
 	"fmt"
+	"strings"
 	"testing"
 
 	sdk "github.com/irisnet/irishub/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,60 @@ 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)
+}

modules/bank/keeper.go

@@ -197,8 +197,8 @@ func hasCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt s
 func subtractCoins(ctx sdk.Context, am auth.AccountKeeper, addr sdk.AccAddress, amt sdk.Coins) (sdk.Coins, sdk.Tags, sdk.Error) {
 	ctx.GasMeter().ConsumeGas(costSubtractCoins, "subtractCoins")
 	oldCoins := getCoins(ctx, am, addr)
-	newCoins := oldCoins.Minus(amt)
-	if !newCoins.IsNotNegative() {
+	newCoins, hasNeg := oldCoins.SafeMinus(amt)
+	if hasNeg {
 		return amt, nil, sdk.ErrInsufficientCoins(fmt.Sprintf("%s < %s", oldCoins, amt))
 	}
 	err := setCoins(ctx, am, addr, newCoins)

modules/bank/msgs_test.go

@@ -34,8 +34,6 @@ func TestInputValidation(t *testing.T) {
 	emptyCoins := sdk.Coins{}
 	emptyCoins2 := sdk.Coins{sdk.NewInt64Coin("eth", 0)}
 	someEmptyCoins := sdk.Coins{sdk.NewInt64Coin("eth", 10), sdk.NewInt64Coin("atom", 0)}
-	minusCoins := sdk.Coins{sdk.NewInt64Coin("eth", -34)}
-	someMinusCoins := sdk.Coins{sdk.NewInt64Coin("atom", 20), sdk.NewInt64Coin("eth", -34)}
 	unsortedCoins := sdk.Coins{sdk.NewInt64Coin("eth", 1), sdk.NewInt64Coin("atom", 1)}
 
 	cases := []struct {
@@ -51,8 +49,6 @@ func TestInputValidation(t *testing.T) {
 		{false, NewInput(addr1, emptyCoins)},     // invalid coins
 		{false, NewInput(addr1, emptyCoins2)},    // invalid coins
 		{false, NewInput(addr1, someEmptyCoins)}, // invalid coins
-		{false, NewInput(addr1, minusCoins)},     // negative coins
-		{false, NewInput(addr1, someMinusCoins)}, // negative coins
 		{false, NewInput(addr1, unsortedCoins)},  // unsorted coins
 	}
 
@@ -76,8 +72,6 @@ func TestOutputValidation(t *testing.T) {
 	emptyCoins := sdk.Coins{}
 	emptyCoins2 := sdk.Coins{sdk.NewInt64Coin("eth", 0)}
 	someEmptyCoins := sdk.Coins{sdk.NewInt64Coin("eth", 10), sdk.NewInt64Coin("atom", 0)}
-	minusCoins := sdk.Coins{sdk.NewInt64Coin("eth", -34)}
-	someMinusCoins := sdk.Coins{sdk.NewInt64Coin("atom", 20), sdk.NewInt64Coin("eth", -34)}
 	unsortedCoins := sdk.Coins{sdk.NewInt64Coin("eth", 1), sdk.NewInt64Coin("atom", 1)}
 
 	cases := []struct {
@@ -93,8 +87,6 @@ func TestOutputValidation(t *testing.T) {
 		{false, NewOutput(addr1, emptyCoins)},     // invalid coins
 		{false, NewOutput(addr1, emptyCoins2)},    // invalid coins
 		{false, NewOutput(addr1, someEmptyCoins)}, // invalid coins
-		{false, NewOutput(addr1, minusCoins)},     // negative coins
-		{false, NewOutput(addr1, someMinusCoins)}, // negative coins
 		{false, NewOutput(addr1, unsortedCoins)},  // unsorted coins
 	}