Merge PR #2376: auth: Don't recalculate mempool fees for every msg signer, misc. cleanup

This PR begins improving the godocs for the auth module, and begins cleaning
up the Ante handler.

Additionally we previously calculated if the fee was sufficient for the tx
on every single signer. This is now refactored to be more efficient, and have
a better logical flow. No changelog entry as this is new to this release.

Author: ValarDragon

CHANGELOG.md

@@ -109,6 +109,8 @@ IMPROVEMENTS
   - [baseapp] Allow any alphanumeric character in route
   - [tools] Remove `rm -rf vendor/` from `make get_vendor_deps`
   - [x/auth] Recover ErrorOutOfGas panic in order to set sdk.Result attributes correctly
+  - [x/auth] \#2376 No longer runs any signature in a multi-msg, if any account/sequence number is wrong.
+  - [x/auth] \#2376 No longer charge gas for subtracting fees
   - [x/bank] Unit tests are now table-driven
   - [tests] Add tests to example apps in docs
   - [tests] Fixes ansible scripts to work with AWS too

PENDING.md

@@ -100,6 +100,7 @@ FEATURES
   * [simulation] [\#1924](https://github.com/cosmos/cosmos-sdk/issues/1924) allow operations to specify future operations
   * [simulation] [\#1924](https://github.com/cosmos/cosmos-sdk/issues/1924) Add benchmarking capabilities, with makefile commands "test_sim_gaia_benchmark, test_sim_gaia_profile"
   * [simulation] [\#2349](https://github.com/cosmos/cosmos-sdk/issues/2349) Add time-based future scheduled operations to simulator
+  * [x/auth] \#2376 Remove FeePayer() from StdTx
   * [x/stake] [\#1672](https://github.com/cosmos/cosmos-sdk/issues/1672) Implement
   basis for the validator commission model.
 

cmd/gaia/app/benchmarks/txsize_test.go

@@ -26,7 +26,7 @@ func ExampleTxSendSize() {
 		Outputs: []bank.Output{bank.NewOutput(addr2, coins)},
 	}
 	sig, _ := priv1.Sign(msg1.GetSignBytes())
-	sigs := []auth.StdSignature{auth.StdSignature{nil, sig, 0, 0}}
+	sigs := []auth.StdSignature{{nil, sig, 0, 0}}
 	tx := auth.NewStdTx([]sdk.Msg{msg1}, auth.NewStdFee(0, coins...), sigs, "")
 	fmt.Println(len(cdc.MustMarshalBinaryBare([]sdk.Msg{msg1})))
 	fmt.Println(len(cdc.MustMarshalBinaryBare(tx)))

docs/sdk/core/app3.md

@@ -133,7 +133,7 @@ Now that we have a native model for accounts, it's time to introduce the native
 
 ```go
 // StdTx is a standard way to wrap a Msg with Fee and Signatures.
-// NOTE: the first signature is the FeePayer (Signatures must not be nil).
+// NOTE: the first signature is the fee payer (Signatures must not be nil).
 type StdTx struct {
 	Msgs       []sdk.Msg      `json:"msg"`
 	Fee        StdFee         `json:"fee"`
@@ -259,8 +259,7 @@ the same message could be executed over and over again.
 The PubKey is required for signature verification, but it is only required in
 the StdSignature once. From that point on, it will be stored in the account.
 
-The fee is paid by the first address returned by `msg.GetSigners()` for the first `Msg`, 
-as provided by the `FeePayer(tx Tx) sdk.AccAddress` function.
+The fee is paid by the first address returned by `msg.GetSigners()` for the first `Msg`.
 
 ## CoinKeeper
 

x/auth/account.go

@@ -8,8 +8,12 @@ import (
 	"github.com/tendermint/tendermint/crypto"
 )
 
-// Account is a standard account using a sequence number for replay protection
-// and a pubkey for authentication.
+// Account is an interface used to store coins at a given address within state.
+// It presumes a notion of sequence numbers for replay protection,
+// a notion of account numbers for replay protection for previously pruned accounts,
+// and a pubkey for authentication purposes.
+//
+// Many complex conditions can be used in the concrete struct which implements Account.
 type Account interface {
 	GetAddress() sdk.AccAddress
 	SetAddress(sdk.AccAddress) error // errors if already set.
@@ -35,9 +39,11 @@ type AccountDecoder func(accountBytes []byte) (Account, error)
 
 var _ Account = (*BaseAccount)(nil)
 
-// BaseAccount - base account structure.
-// Extend this by embedding this in your AppAccount.
-// See the examples/basecoin/types/account.go for an example.
+// BaseAccount - a base account structure.
+// This can be extended by embedding within in your AppAccount.
+// There are examples of this in: examples/basecoin/types/account.go.
+// However one doesn't have to use BaseAccount as long as your struct
+// implements Account.
 type BaseAccount struct {
 	Address       sdk.AccAddress `json:"address"`
 	Coins         sdk.Coins      `json:"coins"`
@@ -118,7 +124,7 @@ func (acc *BaseAccount) SetSequence(seq int64) error {
 //----------------------------------------
 // Wire
 
-// Most users shouldn't use this, but this comes handy for tests.
+// Most users shouldn't use this, but this comes in handy for tests.
 func RegisterBaseAccount(cdc *codec.Codec) {
 	cdc.RegisterInterface((*Account)(nil), nil)
 	cdc.RegisterConcrete(&BaseAccount{}, "cosmos-sdk/BaseAccount", nil)

x/auth/ante.go

@@ -12,19 +12,18 @@ import (
 )
 
 const (
-	deductFeesCost        sdk.Gas = 10
-	memoCostPerByte       sdk.Gas = 1
-	ed25519VerifyCost             = 59
-	secp256k1VerifyCost           = 100
-	maxMemoCharacters             = 100
-	feeDeductionGasFactor         = 0.001
+	memoCostPerByte     sdk.Gas = 1
+	ed25519VerifyCost           = 59
+	secp256k1VerifyCost         = 100
+	maxMemoCharacters           = 100
+	// how much gas = 1 atom
+	gasPerUnitCost = 1000
 )
 
 // NewAnteHandler returns an AnteHandler that checks
 // and increments sequence numbers, checks signatures & account numbers,
 // and deducts fees from the first signer.
 func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler {
-
 	return func(
 		ctx sdk.Context, tx sdk.Tx, simulate bool,
 	) (newCtx sdk.Context, res sdk.Result, abort bool) {
@@ -35,13 +34,17 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler {
 			return ctx, sdk.ErrInternal("tx must be StdTx").Result(), true
 		}
 
-		// set the gas meter
-		if simulate {
-			newCtx = ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
-		} else {
-			newCtx = ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas))
+		// Ensure that the provided fees meet a minimum threshold for the validator, if this is a CheckTx.
+		// This is only for local mempool purposes, and thus is only ran on check tx.
+		if ctx.IsCheckTx() && !simulate {
+			res := ensureSufficientMempoolFees(ctx, stdTx)
+			if !res.IsOK() {
+				return newCtx, res, true
+			}
 		}
 
+		newCtx = setGasMeter(simulate, ctx, stdTx)
+
 		// AnteHandlers must have their own defer/recover in order
 		// for the BaseApp to know how much gas was used!
 		// This is because the GasMeter is created in the AnteHandler,
@@ -65,64 +68,49 @@ func NewAnteHandler(am AccountMapper, fck FeeCollectionKeeper) sdk.AnteHandler {
 		if err != nil {
 			return newCtx, err.Result(), true
 		}
-
-		sigs := stdTx.GetSignatures() // When simulating, this would just be a 0-length slice.
-		signerAddrs := stdTx.GetSigners()
-		msgs := tx.GetMsgs()
-
 		// charge gas for the memo
 		newCtx.GasMeter().ConsumeGas(memoCostPerByte*sdk.Gas(len(stdTx.GetMemo())), "memo")
 
-		// Get the sign bytes (requires all account & sequence numbers and the fee)
-		sequences := make([]int64, len(sigs))
-		accNums := make([]int64, len(sigs))
-		for i := 0; i < len(sigs); i++ {
-			sequences[i] = sigs[i].Sequence
-			accNums[i] = sigs[i].AccountNumber
-		}
-		fee := stdTx.Fee
+		// stdSigs contains the sequence number, account number, and signatures
+		stdSigs := stdTx.GetSignatures() // When simulating, this would just be a 0-length slice.
+		signerAddrs := stdTx.GetSigners()
 
-		// Check sig and nonce and collect signer accounts.
-		var signerAccs = make([]Account, len(signerAddrs))
-		for i := 0; i < len(sigs); i++ {
-			signerAddr, sig := signerAddrs[i], sigs[i]
+		// create the list of all sign bytes
+		signBytesList := getSignBytesList(newCtx.ChainID(), stdTx, stdSigs)
+		signerAccs, res := getSignerAccs(newCtx, am, signerAddrs)
+		if !res.IsOK() {
+			return newCtx, res, true
+		}
+		res = validateAccNumAndSequence(signerAccs, stdSigs)
+		if !res.IsOK() {
+			return newCtx, res, true
+		}
 
-			// check signature, return account with incremented nonce
-			signBytes := StdSignBytes(newCtx.ChainID(), accNums[i], sequences[i], fee, msgs, stdTx.GetMemo())
-			signerAcc, res := processSig(newCtx, am, signerAddr, sig, signBytes, simulate)
+		// first sig pays the fees
+		if !stdTx.Fee.Amount.IsZero() {
+			// signerAccs[0] is the fee payer
+			signerAccs[0], res = deductFees(signerAccs[0], stdTx.Fee)
 			if !res.IsOK() {
 				return newCtx, res, true
 			}
+			fck.addCollectedFees(newCtx, stdTx.Fee.Amount)
+		}
 
-			requiredFees := adjustFeesByGas(ctx.MinimumFees(), fee.Gas)
-			// fees must be greater than the minimum set by the validator adjusted by gas
-			if ctx.IsCheckTx() && !simulate && !ctx.MinimumFees().IsZero() && fee.Amount.IsLT(requiredFees) {
-				// validators reject any tx from the mempool with less than the minimum fee per gas * gas factor
-				return newCtx, sdk.ErrInsufficientFee(fmt.Sprintf(
-					"insufficient fee, got: %q required: %q", fee.Amount, requiredFees)).Result(), true
-			}
-
-			// first sig pays the fees
-			// Can this function be moved outside of the loop?
-			if i == 0 && !fee.Amount.IsZero() {
-				newCtx.GasMeter().ConsumeGas(deductFeesCost, "deductFees")
-				signerAcc, res = deductFees(signerAcc, fee)
-				if !res.IsOK() {
-					return newCtx, res, true
-				}
-				fck.addCollectedFees(newCtx, fee.Amount)
+		for i := 0; i < len(stdSigs); i++ {
+			// check signature, return account with incremented nonce
+			signerAccs[i], res = processSig(newCtx, signerAccs[i], stdSigs[i], signBytesList[i], simulate)
+			if !res.IsOK() {
+				return newCtx, res, true
 			}
 
 			// Save the account.
-			am.SetAccount(newCtx, signerAcc)
-			signerAccs[i] = signerAcc
+			am.SetAccount(newCtx, signerAccs[i])
 		}
 
 		// cache the signer accounts in the context
 		newCtx = WithSigners(newCtx, signerAccs)
 
 		// TODO: tx tags (?)
-
 		return newCtx, sdk.Result{GasWanted: stdTx.Fee.Gas}, false // continue...
 	}
 }
@@ -150,42 +138,45 @@ func validateBasic(tx StdTx) (err sdk.Error) {
 	return nil
 }
 
-// verify the signature and increment the sequence.
-// if the account doesn't have a pubkey, set it.
-func processSig(
-	ctx sdk.Context, am AccountMapper,
-	addr sdk.AccAddress, sig StdSignature, signBytes []byte, simulate bool) (
-	acc Account, res sdk.Result) {
-	// Get the account.
-	acc = am.GetAccount(ctx, addr)
-	if acc == nil {
-		return nil, sdk.ErrUnknownAddress(addr.String()).Result()
+func getSignerAccs(ctx sdk.Context, am AccountMapper, addrs []sdk.AccAddress) (accs []Account, res sdk.Result) {
+	accs = make([]Account, len(addrs))
+	for i := 0; i < len(accs); i++ {
+		accs[i] = am.GetAccount(ctx, addrs[i])
+		if accs[i] == nil {
+			return nil, sdk.ErrUnknownAddress(addrs[i].String()).Result()
+		}
 	}
+	return
+}
 
-	accnum := acc.GetAccountNumber()
-	seq := acc.GetSequence()
+func validateAccNumAndSequence(accs []Account, sigs []StdSignature) sdk.Result {
+	for i := 0; i < len(accs); i++ {
+		accnum := accs[i].GetAccountNumber()
+		seq := accs[i].GetSequence()
+		// Check account number.
+		if accnum != sigs[i].AccountNumber {
+			return sdk.ErrInvalidSequence(
+				fmt.Sprintf("Invalid account number. Got %d, expected %d", sigs[i].AccountNumber, accnum)).Result()
+		}
 
-	// Check account number.
-	if accnum != sig.AccountNumber {
-		return nil, sdk.ErrInvalidSequence(
-			fmt.Sprintf("Invalid account number. Got %d, expected %d", sig.AccountNumber, accnum)).Result()
+		// Check sequence number.
+		if seq != sigs[i].Sequence {
+			return sdk.ErrInvalidSequence(
+				fmt.Sprintf("Invalid sequence. Got %d, expected %d", sigs[i].Sequence, seq)).Result()
+		}
 	}
+	return sdk.Result{}
+}
 
-	// Check sequence number.
-	if seq != sig.Sequence {
-		return nil, sdk.ErrInvalidSequence(
-			fmt.Sprintf("Invalid sequence. Got %d, expected %d", sig.Sequence, seq)).Result()
-	}
-	err := acc.SetSequence(seq + 1)
-	if err != nil {
-		// Handle w/ #870
-		panic(err)
-	}
+// verify the signature and increment the sequence.
+// if the account doesn't have a pubkey, set it.
+func processSig(ctx sdk.Context,
+	acc Account, sig StdSignature, signBytes []byte, simulate bool) (updatedAcc Account, res sdk.Result) {
 	pubKey, res := processPubKey(acc, sig, simulate)
 	if !res.IsOK() {
 		return nil, res
 	}
-	err = acc.SetPubKey(pubKey)
+	err := acc.SetPubKey(pubKey)
 	if err != nil {
 		return nil, sdk.ErrInternal("setting PubKey on signer's account").Result()
 	}
@@ -195,7 +186,14 @@ func processSig(
 		return nil, sdk.ErrUnauthorized("signature verification failed").Result()
 	}
 
-	return
+	// increment the sequence number
+	err = acc.SetSequence(acc.GetSequence() + 1)
+	if err != nil {
+		// Handle w/ #870
+		panic(err)
+	}
+
+	return acc, res
 }
 
 var dummySecp256k1Pubkey secp256k1.PubKeySecp256k1
@@ -244,8 +242,9 @@ func consumeSignatureVerificationGas(meter sdk.GasMeter, pubkey crypto.PubKey) {
 }
 
 func adjustFeesByGas(fees sdk.Coins, gas int64) sdk.Coins {
-	gasCost := int64(float64(gas) * feeDeductionGasFactor)
+	gasCost := gas / gasPerUnitCost
 	gasFees := make(sdk.Coins, len(fees))
+	// TODO: Make this not price all coins in the same way
 	for i := 0; i < len(fees); i++ {
 		gasFees[i] = sdk.NewInt64Coin(fees[i].Denom, gasCost)
 	}
@@ -272,5 +271,37 @@ func deductFees(acc Account, fee StdFee) (Account, sdk.Result) {
 	return acc, 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.
+	requiredFees := adjustFeesByGas(ctx.MinimumFees(), stdTx.Fee.Gas)
+
+	if !ctx.MinimumFees().IsZero() && stdTx.Fee.Amount.IsLT(requiredFees) {
+		// validators reject any tx from the mempool with less than the minimum fee per gas * gas factor
+		return sdk.ErrInsufficientFee(fmt.Sprintf(
+			"insufficient fee, got: %q required: %q", stdTx.Fee.Amount, requiredFees)).Result()
+	}
+	return sdk.Result{}
+}
+
+func setGasMeter(simulate bool, ctx sdk.Context, stdTx StdTx) sdk.Context {
+	// set the gas meter
+	if simulate {
+		return ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
+	}
+	return ctx.WithGasMeter(sdk.NewGasMeter(stdTx.Fee.Gas))
+}
+
+func getSignBytesList(chainID string, stdTx StdTx, stdSigs []StdSignature) (signatureBytesList [][]byte) {
+	signatureBytesList = make([][]byte, len(stdSigs))
+	for i := 0; i < len(stdSigs); i++ {
+		signatureBytesList[i] = StdSignBytes(chainID,
+			stdSigs[i].AccountNumber, stdSigs[i].Sequence,
+			stdTx.Fee, stdTx.Msgs, stdTx.Memo)
+	}
+	return
+}
+
 // BurnFeeHandler burns all fees (decreasing total supply)
 func BurnFeeHandler(_ sdk.Context, _ sdk.Tx, _ sdk.Coins) {}

x/auth/stdtx.go

@@ -11,7 +11,7 @@ import (
 var _ sdk.Tx = (*StdTx)(nil)
 
 // StdTx is a standard way to wrap a Msg with Fee and Signatures.
-// NOTE: the first signature is the FeePayer (Signatures must not be nil).
+// NOTE: the first signature is the fee payer (Signatures must not be nil).
 type StdTx struct {
 	Msgs       []sdk.Msg      `json:"msg"`
 	Fee        StdFee         `json:"fee"`
@@ -63,13 +63,6 @@ func (tx StdTx) GetMemo() string { return tx.Memo }
 // .Empty().
 func (tx StdTx) GetSignatures() []StdSignature { return tx.Signatures }
 
-// FeePayer returns the address responsible for paying the fees
-// for the transactions. It's the first address returned by msg.GetSigners().
-// If GetSigners() is empty, this panics.
-func FeePayer(tx sdk.Tx) sdk.AccAddress {
-	return tx.GetMsgs()[0].GetSigners()[0]
-}
-
 //__________________________________________________________
 
 // StdFee includes the amount of coins paid in fees and the maximum

x/auth/stdtx_test.go

@@ -23,7 +23,7 @@ func TestStdTx(t *testing.T) {
 	require.Equal(t, msgs, tx.GetMsgs())
 	require.Equal(t, sigs, tx.GetSignatures())
 
-	feePayer := FeePayer(tx)
+	feePayer := tx.GetSigners()[0]
 	require.Equal(t, addr, feePayer)
 }