WIP: Baseapp to not run messages/msgHandler on CheckTx

CHANGELOG.md

@@ -9,6 +9,7 @@ BREAKING CHANGES
 * AltBytes renamed to Memo, now a string, max 100 characters, costs a bit of gas
 * Transactions now take a list of Messages
 * Signers of a transaction now only sign over their account and sequence number
+* Msg Handlers no longer run in CheckTx(). They only run in DeliverTx()
 
 FEATURES
 * [gaiacli] You can now attach a simple text-only memo to any transaction, with the `--memo` flag

baseapp/baseapp.go

@@ -474,21 +474,6 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
 		}
 	}()
 
-	// Get the Msg.
-	var msgs = tx.GetMsgs()
-	if msgs == nil || len(msgs) == 0 {
-		return sdk.ErrInternal("Tx.GetMsgs() must return at least one message in list").Result()
-	}
-
-	for _, msg := range msgs {
-		// Validate the Msg
-		err := msg.ValidateBasic()
-		if err != nil {
-			err = err.WithDefaultCodespace(sdk.CodespaceRoot)
-			return err.Result()
-		}
-	}
-
 	// Get the context
 	var ctx sdk.Context
 	if mode == runTxModeCheck || mode == runTxModeSimulate {
@@ -513,7 +498,6 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
 			ctx = newCtx
 		}
 	}
-
 	// Get the correct cache
 	var msCache sdk.CacheMultiStore
 	if mode == runTxModeCheck || mode == runTxModeSimulate {
@@ -526,53 +510,74 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
 		ctx = ctx.WithMultiStore(msCache)
 	}
 
-	finalResult := sdk.Result{}
-	var logs []string
-	for i, msg := range msgs {
-		// Match route.
-		msgType := msg.Type()
-		handler := app.router.Route(msgType)
-		if handler == nil {
-			return sdk.ErrUnknownRequest("Unrecognized Msg type: " + msgType).Result()
-		}
+	// No need to run Msg checks when running CheckTx(), since CheckTx() only needs to validate that fees can be paid
+	// Then when DeliverTx() is called, this will run, preventing us from doing the same checks twice
+	if mode != runTxModeCheck {
 
-		result = handler(ctx, msg)
-
-		// Set gas utilized
-		finalResult.GasUsed += ctx.GasMeter().GasConsumed()
-		finalResult.GasWanted += result.GasWanted
+		// Get the Msg.
+		var msgs = tx.GetMsgs()
+		if msgs == nil || len(msgs) == 0 {
+			return sdk.ErrInternal("Tx.GetMsgs() must return at least one message in list").Result()
+		}
 
-		// Append Data and Tags
-		finalResult.Data = append(finalResult.Data, result.Data...)
-		finalResult.Tags = append(finalResult.Tags, result.Tags...)
+		for _, msg := range msgs {
+			// Validate the Msg
+			err := msg.ValidateBasic()
+			if err != nil {
+				err = err.WithDefaultCodespace(sdk.CodespaceRoot)
+				return err.Result()
+			}
+		}
 
-		// Construct usable logs in multi-message transactions. Messages are 1-indexed in logs.
-		logs = append(logs, fmt.Sprintf("Msg %d: %s", i+1, finalResult.Log))
+		finalResult := sdk.Result{}
+		var logs []string
+		for i, msg := range msgs {
+			// Match route.
+			msgType := msg.Type()
+			handler := app.router.Route(msgType)
+			if handler == nil {
+				return sdk.ErrUnknownRequest("Unrecognized Msg type: " + msgType).Result()
+			}
 
-		// Stop execution and return on first failed message.
-		if !result.IsOK() {
-			if len(msgs) == 1 {
+			result = handler(ctx, msg)
+
+			// Set gas utilized
+			finalResult.GasUsed += ctx.GasMeter().GasConsumed()
+			finalResult.GasWanted += result.GasWanted
+
+			// Append Data and Tags
+			finalResult.Data = append(finalResult.Data, result.Data...)
+			finalResult.Tags = append(finalResult.Tags, result.Tags...)
+
+			// Construct usable logs in multi-message transactions. Messages are 1-indexed in logs.
+			logs = append(logs, fmt.Sprintf("Msg %d: %s", i+1, finalResult.Log))
+
+			// Stop execution and return on first failed message.
+			if !result.IsOK() {
+				if len(msgs) == 1 {
+					return result
+				}
+				result.GasUsed = finalResult.GasUsed
+				if i == 0 {
+					result.Log = fmt.Sprintf("Msg 1 failed: %s", result.Log)
+				} else {
+					result.Log = fmt.Sprintf("Msg 1-%d Passed. Msg %d failed: %s", i, i+1, result.Log)
+				}
 				return result
 			}
-			result.GasUsed = finalResult.GasUsed
-			if i == 0 {
-				result.Log = fmt.Sprintf("Msg 1 failed: %s", result.Log)
-			} else {
-				result.Log = fmt.Sprintf("Msg 1-%d Passed. Msg %d failed: %s", i, i+1, result.Log)
-			}
-			return result
 		}
+		finalResult.Log = strings.Join(logs, "\n")
+		result = finalResult
+
 	}
 
 	// If not a simulated run and result was successful, write to app.checkState.ms or app.deliverState.ms
 	// Only update state if all messages pass.
 	if mode != runTxModeSimulate && result.IsOK() {
 		msCache.Write()
 	}
+	return result
 
-	finalResult.Log = strings.Join(logs, "\n")
-
-	return finalResult
 }
 
 // Implements ABCI

baseapp/baseapp_test.go

@@ -3,10 +3,11 @@ package baseapp
 import (
 	"encoding/json"
 	"fmt"
-	"github.com/cosmos/cosmos-sdk/x/bank"
 	"os"
 	"testing"
 
+	"github.com/cosmos/cosmos-sdk/x/bank"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
@@ -426,6 +427,8 @@ func TestRunInvalidTransaction(t *testing.T) {
 }
 
 // Test that transactions exceeding gas limits fail
+// With an ante handler that takes 0 fee, and a msgHandler that consumes 10 gas
+// It therefore only fails on app.Deliver(tx), since msgHandler is only run on Deliver
 func TestTxGasLimits(t *testing.T) {
 	logger := defaultLogger()
 	db := dbm.NewMemDB()
@@ -450,8 +453,129 @@ func TestTxGasLimits(t *testing.T) {
 	header := abci.Header{AppHash: []byte("apphash")}
 
 	app.BeginBlock(abci.RequestBeginBlock{Header: header})
-	res := app.Deliver(tx)
-	assert.Equal(t, res.Code, sdk.ToABCICode(sdk.CodespaceRoot, sdk.CodeOutOfGas), "Expected transaction to run out of gas")
+	resCheck := app.Check(tx)
+	assert.Equal(t, resCheck.Code, sdk.ToABCICode(sdk.CodespaceRoot, sdk.CodeOK), "Expected abci Codetype == 0, for CodeOK")
+	resDeliver := app.Deliver(tx)
+	assert.Equal(t, resDeliver.Code, sdk.ToABCICode(sdk.CodespaceRoot, sdk.CodeOutOfGas), "Expected transaction to run out of gas due to fee in msg handler")
+	app.EndBlock(abci.RequestEndBlock{})
+	app.Commit()
+}
+
+// Tests failure on sending multiple checkTx, where the fee.Amount empties the accounts checkState coins, and limits the txs to 4
+func TestMultiCheckTxFailFee(t *testing.T) {
+	app := newTestApp(t.Name())
+	capKey := sdk.NewKVStoreKey("key")
+	capKey2 := sdk.NewKVStoreKey("feekeeper")
+
+	app.MountStoresIAVL(capKey)
+	app.MountStoresIAVL(capKey2)
+	app.SetTxDecoder(func(txBytes []byte) (sdk.Tx, sdk.Error) {
+		var tx auth.StdTx
+		fromJSON(txBytes, &tx)
+		return tx, nil
+	})
+
+	err := app.LoadLatestVersion(capKey)
+	if err != nil {
+		panic(err)
+	}
+
+	app.accountMapper = auth.NewAccountMapper(app.cdc, capKey, &auth.BaseAccount{})
+	app.accountKeeper = bank.NewKeeper(app.accountMapper)
+	fck := auth.NewFeeCollectionKeeper(app.cdc, capKey2)
+
+	app.SetAnteHandler(auth.NewAnteHandler(app.accountMapper, fck))
+
+	app.InitChain(abci.RequestInitChain{})
+	app.BeginBlock(abci.RequestBeginBlock{})
+
+	app.checkState.ctx = app.checkState.ctx.WithChainID(t.Name())
+
+	priv := makePrivKey("secret")
+	addr := priv.PubKey().Address()
+
+	// We add 20 coins to the account. tx takes 5. 4 will be successful, and the last will fail
+	app.accountKeeper.AddCoins(app.checkState.ctx, addr, sdk.Coins{{"foocoin", sdk.NewInt(20)}})
+	assert.Equal(t, sdk.Coins{{"foocoin", sdk.NewInt(20)}}, app.accountKeeper.GetCoins(app.checkState.ctx, addr), "Balance did not update")
+
+
+	//won't actually run in checkTx(), but needed to generate the tx
+	msg := testBurnMsg{addr, sdk.Coins{{"foocoin", sdk.NewInt(1)}}}
+
+
+	// First four pass, last one fails
+	for i := 0; i < 5; i++ {
+		tx := GenTx(5, 100000, t.Name(), []sdk.Msg{msg}, []int64{0}, []int64{int64(i)}, priv) //single message
+		resCheck := app.Check(tx)
+		if i < 4 {
+			assert.Equal(t, sdk.ToABCICode(sdk.CodespaceRoot, sdk.CodeOK), resCheck.Code, "Expected abci Codetype == 0, for CodeOK")
+		}
+		if i == 4 {
+			assert.Equal(t, sdk.ToABCICode(sdk.CodespaceRoot, sdk.CodeInsufficientFunds), resCheck.Code, "Expected tx to fail since the account ran out of coins")
+		}
+	}
+
+	app.EndBlock(abci.RequestEndBlock{})
+	app.Commit()
+}
+
+// Tests failure on sending multiple checkTx, where the fee.Gas continues to get lower each tx, until it triggers an out of gas error
+func TestMultiCheckTxFailGas(t *testing.T) {
+	app := newTestApp(t.Name())
+	capKey := sdk.NewKVStoreKey("key")
+	capKey2 := sdk.NewKVStoreKey("feekeeper")
+
+	app.MountStoresIAVL(capKey)
+	app.MountStoresIAVL(capKey2)
+	app.SetTxDecoder(func(txBytes []byte) (sdk.Tx, sdk.Error) {
+		var tx auth.StdTx
+		fromJSON(txBytes, &tx)
+		return tx, nil
+	})
+
+	err := app.LoadLatestVersion(capKey)
+	if err != nil {
+		panic(err)
+	}
+
+	app.accountMapper = auth.NewAccountMapper(app.cdc, capKey, &auth.BaseAccount{})
+	app.accountKeeper = bank.NewKeeper(app.accountMapper)
+	fck := auth.NewFeeCollectionKeeper(app.cdc, capKey2)
+
+	app.SetAnteHandler(auth.NewAnteHandler(app.accountMapper, fck))
+
+	app.InitChain(abci.RequestInitChain{})
+	app.BeginBlock(abci.RequestBeginBlock{})
+
+	app.checkState.ctx = app.checkState.ctx.WithChainID(t.Name())
+
+	priv := makePrivKey("secret")
+	addr := priv.PubKey().Address()
+
+	//enough coins so it won't fail on running out of coins
+	app.accountKeeper.AddCoins(app.checkState.ctx, addr, sdk.Coins{{"foocoin", sdk.NewInt(100)}})
+	assert.Equal(t, sdk.Coins{{"foocoin", sdk.NewInt(100)}}, app.accountKeeper.GetCoins(app.checkState.ctx, addr), "Balance did not update")
+
+	//won't actually run in checkTx(), but needed to generate the tx
+	msg := testBurnMsg{addr, sdk.Coins{{"foocoin", sdk.NewInt(1)}}}
+
+	// Draining gas from this variable, as right now there isn't a variable for an account in accountKeeper that states how much gas it has to do txs
+	// the ante handler right now burns around 1300-1400 gas per tx for this GenTx(). So with 6000 gas we expect to fail on the forth
+	var accountGas int64 = 1500*4
+
+	// First four pass, last one fails
+	for i := 0; i < 5; i++ {
+		tx := GenTx(5, accountGas, t.Name(), []sdk.Msg{msg}, []int64{0}, []int64{int64(i)}, priv) //single message
+		resCheck := app.Check(tx)
+		if i < 4 {
+			assert.Equal(t, sdk.ToABCICode(sdk.CodespaceRoot, sdk.CodeOK), resCheck.Code, "Expected abci Codetype == 0, for CodeOK")
+		}
+		if i == 4 {
+			assert.Equal(t, sdk.ToABCICode(sdk.CodespaceRoot, sdk.CodeOutOfGas), resCheck.Code, "Expected tx to fail since the account ran out of gas")
+		}
+		accountGas -=1500 //represents gas being drained from the keeper account
+	}
+
 	app.EndBlock(abci.RequestEndBlock{})
 	app.Commit()
 }
@@ -716,11 +840,11 @@ func newHandleSpend(keeper bank.Keeper) sdk.Handler {
 }
 
 // generate a signed transaction
-func GenTx(chainID string, msgs []sdk.Msg, accnums []int64, seq []int64, priv ...crypto.PrivKey) auth.StdTx {
+func GenTx(feeAmount int64, gasAmount int64, chainID string, msgs []sdk.Msg, accnums []int64, seq []int64, priv ...crypto.PrivKey) auth.StdTx {
 	// make the transaction free
 	fee := auth.StdFee{
-		sdk.Coins{{"foocoin", sdk.NewInt(0)}},
-		100000,
+		sdk.Coins{{"foocoin", sdk.NewInt(feeAmount)}},
+		gasAmount,
 	}
 
 	sigs := make([]auth.StdSignature, len(priv))
@@ -796,7 +920,7 @@ func TestMultipleBurn(t *testing.T) {
 	assert.Equal(t, sdk.Coins{{"foocoin", sdk.NewInt(100)}}, app.accountKeeper.GetCoins(app.deliverState.ctx, addr), "Balance did not update")
 
 	msg := testBurnMsg{addr, sdk.Coins{{"foocoin", sdk.NewInt(50)}}}
-	tx := GenTx(t.Name(), []sdk.Msg{msg, msg}, []int64{0}, []int64{0}, priv)
+	tx := GenTx(0, 100000, t.Name(), []sdk.Msg{msg, msg}, []int64{0}, []int64{0}, priv)
 
 	res := app.Deliver(tx)
 
@@ -853,7 +977,7 @@ func TestBurnMultipleOwners(t *testing.T) {
 	msg2 := testBurnMsg{addr2, sdk.Coins{{"foocoin", sdk.NewInt(100)}}}
 
 	// test wrong signers: Address 1 signs both messages
-	tx := GenTx(t.Name(), []sdk.Msg{msg1, msg2}, []int64{0, 0}, []int64{0, 0}, priv1, priv1)
+	tx := GenTx(0, 100000, t.Name(), []sdk.Msg{msg1, msg2}, []int64{0, 0}, []int64{0, 0}, priv1, priv1)
 
 	res := app.Deliver(tx)
 	assert.Equal(t, sdk.ABCICodeType(0x10003), res.Code, "Wrong signatures passed")
@@ -862,7 +986,7 @@ func TestBurnMultipleOwners(t *testing.T) {
 	assert.Equal(t, sdk.Coins{{"foocoin", sdk.NewInt(100)}}, app.accountKeeper.GetCoins(app.deliverState.ctx, addr2), "Balance2 changed after invalid sig")
 
 	// test valid tx
-	tx = GenTx(t.Name(), []sdk.Msg{msg1, msg2}, []int64{0, 1}, []int64{1, 0}, priv1, priv2)
+	tx = GenTx(0, 100000, t.Name(), []sdk.Msg{msg1, msg2}, []int64{0, 1}, []int64{1, 0}, priv1, priv2)
 
 	res = app.Deliver(tx)
 	assert.Equal(t, true, res.IsOK(), res.Log)
@@ -922,7 +1046,7 @@ func TestSendBurn(t *testing.T) {
 	msg2 := testBurnMsg{addr2, sdk.Coins{{"foocoin", sdk.NewInt(50)}}}
 
 	// send then burn
-	tx := GenTx(t.Name(), []sdk.Msg{sendMsg, msg2, msg1}, []int64{0, 1}, []int64{0, 0}, priv1, priv2)
+	tx := GenTx(0, 100000, t.Name(), []sdk.Msg{sendMsg, msg2, msg1}, []int64{0, 1}, []int64{0, 0}, priv1, priv2)
 
 	res := app.Deliver(tx)
 	assert.Equal(t, true, res.IsOK(), res.Log)
@@ -934,7 +1058,7 @@ func TestSendBurn(t *testing.T) {
 	app.accountKeeper.AddCoins(app.deliverState.ctx, addr1, sdk.Coins{{"foocoin", sdk.NewInt(50)}})
 
 	// burn then send
-	tx = GenTx(t.Name(), []sdk.Msg{msg1, sendMsg}, []int64{0}, []int64{1}, priv1)
+	tx = GenTx(0, 100000, t.Name(), []sdk.Msg{msg1, sendMsg}, []int64{0}, []int64{1}, priv1)
 
 	res = app.Deliver(tx)
 

docs/core/messages.md

@@ -74,3 +74,8 @@ func (msg MsgIssue) GetSigners() []sdk.Address {
 }
 ```
 
+Message logic is only handled by the `DeliverTx()` function of a transaction. For example,
+when the ante handler runs, it only checks if the account has enough fees to pay for the
+basic handling of any message. This is done in `CheckTx()`. Then after `CheckTx()` is
+confirmed, `DeliverTx()` is run, and will charge fees based on the type of `msg` being
+delivered.

docs/core/transactions.md

@@ -125,6 +125,10 @@ app.SetTxDecoder(CustomTxDecodeFn)
 The AnteHandler is used to do all transaction-level processing (i.e. Fee payment, signature verification) 
 before passing the message to its respective handler.
 
+The AnteHandler only runs `CheckTx()`, and `DeliverTx()` will only be run after `CheckTx()` has passed.
+This is done to separate the message logic away from the ante handler, since the ante handler is
+independent from the type of message that is being committed.
+
 ```go
 type AnteHandler func(ctx Context, tx Tx) (newCtx Context, result Result, abort bool)
 ```