R4R: Consume Gas Proportional to Tx Size

PENDING.md

@@ -50,6 +50,8 @@ IMPROVEMENTS
   * [\#3418](https://github.com/cosmos/cosmos-sdk/issues/3418) Add vesting account
   genesis validation checks to `GaiaValidateGenesisState`.
   * [\#3420](https://github.com/cosmos/cosmos-sdk/issues/3420) Added maximum length to governance proposal descriptions and titles
+  * [\#3256](https://github.com/cosmos/cosmos-sdk/issues/3256) Add gas consumption
+  for tx size in the ante handler and increase the default gas adjustment.
 
 * SDK
   * \#3435 Test that store implementations do not allow nil values

baseapp/baseapp.go

@@ -390,7 +390,7 @@ func handleQueryApp(app *BaseApp, path []string, req abci.RequestQuery) (res abc
 			if err != nil {
 				result = err.Result()
 			} else {
-				result = app.Simulate(tx)
+				result = app.Simulate(txBytes, tx)
 			}
 		case "version":
 			return abci.ResponseQuery{
@@ -719,7 +719,10 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte, tx sdk.Tx) (result sdk
 		if r := recover(); r != nil {
 			switch rType := r.(type) {
 			case sdk.ErrorOutOfGas:
-				log := fmt.Sprintf("out of gas in location: %v", rType.Descriptor)
+				log := fmt.Sprintf(
+					"out of gas in location: %v; gasWanted: %d, gasUsed: %d",
+					rType.Descriptor, gasWanted, ctx.GasMeter().GasConsumed(),
+				)
 				result = sdk.ErrOutOfGas(log).Result()
 			default:
 				log := fmt.Sprintf("recovered: %v\nstack:\n%v", r, string(debug.Stack()))

baseapp/baseapp_test.go

@@ -655,20 +655,20 @@ func TestSimulateTx(t *testing.T) {
 		app.BeginBlock(abci.RequestBeginBlock{})
 
 		tx := newTxCounter(count, count)
+		txBytes, err := cdc.MarshalBinaryLengthPrefixed(tx)
+		require.Nil(t, err)
 
 		// simulate a message, check gas reported
-		result := app.Simulate(tx)
+		result := app.Simulate(txBytes, tx)
 		require.True(t, result.IsOK(), result.Log)
 		require.Equal(t, gasConsumed, result.GasUsed)
 
 		// simulate again, same result
-		result = app.Simulate(tx)
+		result = app.Simulate(txBytes, tx)
 		require.True(t, result.IsOK(), result.Log)
 		require.Equal(t, gasConsumed, result.GasUsed)
 
 		// simulate by calling Query with encoded tx
-		txBytes, err := cdc.MarshalBinaryLengthPrefixed(tx)
-		require.Nil(t, err)
 		query := abci.RequestQuery{
 			Path: "/app/simulate",
 			Data: txBytes,

baseapp/helpers.go

@@ -14,8 +14,8 @@ func (app *BaseApp) Check(tx sdk.Tx) (result sdk.Result) {
 }
 
 // nolint - full tx execution
-func (app *BaseApp) Simulate(tx sdk.Tx) (result sdk.Result) {
-	return app.runTx(runTxModeSimulate, nil, tx)
+func (app *BaseApp) Simulate(txBytes []byte, tx sdk.Tx) (result sdk.Result) {
+	return app.runTx(runTxModeSimulate, txBytes, tx)
 }
 
 // nolint

client/flags.go

@@ -11,10 +11,11 @@ import (
 
 // nolint
 const (
-	// DefaultGasAdjustment is applied to gas estimates to avoid tx
-	// execution failures due to state changes that might
-	// occur between the tx simulation and the actual run.
-	DefaultGasAdjustment = 1.0
+	// DefaultGasAdjustment is applied to gas estimates to avoid tx execution
+	// failures due to state changes that might occur between the tx simulation
+	// and the actual run. In addition, it must also account for tx size
+	// discrepancies when valid signature are provided.
+	DefaultGasAdjustment = 1.03
 	DefaultGasLimit      = 200000
 	GasFlagAuto          = "auto"
 

client/lcd/lcd_test.go

@@ -209,7 +209,9 @@ func TestCoinSend(t *testing.T) {
 	require.Equal(t, http.StatusInternalServerError, res.StatusCode, body)
 
 	// run simulation and test success with estimated gas
-	res, body, _ = doTransferWithGas(t, port, seed, name1, memo, pw, addr, "10000", 1.0, true, false, fees)
+	res, body, _ = doTransferWithGas(
+		t, port, seed, name1, memo, pw, addr, "10000", client.DefaultGasAdjustment, true, false, fees,
+	)
 	require.Equal(t, http.StatusOK, res.StatusCode, body)
 
 	var gasEstResp utils.GasEstimateResponse
@@ -285,7 +287,10 @@ func TestCoinSendGenerateSignAndBroadcast(t *testing.T) {
 	acc := getAccount(t, port, addr)
 
 	// simulate tx
-	res, body, _ := doTransferWithGas(t, port, seed, name1, memo, "", addr, client.GasFlagAuto, 1, true, false, fees)
+	res, body, _ := doTransferWithGas(
+		t, port, seed, name1, memo, "", addr, client.GasFlagAuto,
+		client.DefaultGasAdjustment, true, false, fees,
+	)
 	require.Equal(t, http.StatusOK, res.StatusCode, body)
 
 	var gasEstResp utils.GasEstimateResponse

client/utils/rest.go

@@ -16,11 +16,6 @@ import (
 	authtxb "github.com/cosmos/cosmos-sdk/x/auth/client/txbuilder"
 )
 
-// GasEstimateResponse defines a response definition for tx gas estimation.
-type GasEstimateResponse struct {
-	GasEstimate uint64 `json:"gas_estimate"`
-}
-
 //-----------------------------------------------------------------------------
 // Basic HTTP utilities
 

client/utils/utils.go

@@ -18,6 +18,15 @@ import (
 	authtxb "github.com/cosmos/cosmos-sdk/x/auth/client/txbuilder"
 )
 
+// GasEstimateResponse defines a response definition for tx gas estimation.
+type GasEstimateResponse struct {
+	GasEstimate uint64 `json:"gas_estimate"`
+}
+
+func (gr GasEstimateResponse) String() string {
+	return fmt.Sprintf("gas estimate: %d", gr.GasEstimate)
+}
+
 // CompleteAndBroadcastTxCLI implements a utility function that facilitates
 // sending a series of messages in a signed transaction given a TxBuilder and a
 // QueryContext. It ensures that the account exists, has a proper number and
@@ -38,8 +47,11 @@ func CompleteAndBroadcastTxCLI(txBldr authtxb.TxBuilder, cliCtx context.CLIConte
 		if err != nil {
 			return err
 		}
-		fmt.Fprintf(os.Stderr, "estimated gas = %v\n", txBldr.GetGas())
+
+		gasEst := GasEstimateResponse{GasEstimate: txBldr.GetGas()}
+		fmt.Fprintf(os.Stderr, gasEst.String())
 	}
+
 	if cliCtx.Simulate {
 		return nil
 	}

cmd/gaia/app/sim_test.go

@@ -79,7 +79,7 @@ func appStateFromGenesisFileFn(r *rand.Rand, accs []simulation.Account, genesisT
 		privKey := secp256k1.GenPrivKeySecp256k1(privkeySeed)
 		newAccs = append(newAccs, simulation.Account{privKey, privKey.PubKey(), acc.Address})
 	}
-	return json.RawMessage(genesis.AppState), newAccs, genesis.ChainID
+	return genesis.AppState, newAccs, genesis.ChainID
 }
 
 func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimestamp time.Time) (json.RawMessage, []simulation.Account, string) {
@@ -138,11 +138,11 @@ func appStateRandomizedFn(r *rand.Rand, accs []simulation.Account, genesisTimest
 
 	authGenesis := auth.GenesisState{
 		Params: auth.Params{
-			MemoCostPerByte:        uint64(r.Intn(10) + 1),
-			MaxMemoCharacters:      uint64(r.Intn(200-100) + 100),
+			MaxMemoCharacters:      uint64(randIntBetween(r, 100, 200)),
 			TxSigLimit:             uint64(r.Intn(7) + 1),
-			SigVerifyCostED25519:   uint64(r.Intn(1000-500) + 500),
-			SigVerifyCostSecp256k1: uint64(r.Intn(1000-500) + 500),
+			TxSizeCostPerByte:      uint64(randIntBetween(r, 5, 15)),
+			SigVerifyCostED25519:   uint64(randIntBetween(r, 500, 1000)),
+			SigVerifyCostSecp256k1: uint64(randIntBetween(r, 500, 1000)),
 		},
 	}
 	fmt.Printf("Selected randomly generated auth parameters:\n\t%+v\n", authGenesis)

cmd/gaia/cli_test/cli_test.go

@@ -279,7 +279,7 @@ func TestGaiaCLIGasAuto(t *testing.T) {
 	}{}
 	err := cdc.UnmarshalJSON([]byte(stdout), &sendResp)
 	require.Nil(t, err)
-	require.Equal(t, sendResp.Response.GasWanted, sendResp.Response.GasUsed)
+	require.True(t, sendResp.Response.GasWanted >= sendResp.Response.GasUsed)
 	tests.WaitForNextNBlocksTM(1, f.Port)
 
 	// Check state has changed accordingly
@@ -690,7 +690,7 @@ func TestGaiaCLISendGenerateSignAndBroadcast(t *testing.T) {
 
 	// Unmarshal the response and ensure that gas was properly used
 	require.Nil(t, app.MakeCodec().UnmarshalJSON([]byte(stdout), &result))
-	require.Equal(t, msg.Fee.Gas, uint64(result.Response.GasUsed))
+	require.True(t, msg.Fee.Gas >= uint64(result.Response.GasUsed))
 	require.Equal(t, msg.Fee.Gas, uint64(result.Response.GasWanted))
 	tests.WaitForNextNBlocksTM(1, f.Port)
 

x/auth/ante.go

@@ -15,6 +15,17 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
 
+var (
+	// simulation signature values used to estimate gas consumption
+	simSecp256k1Pubkey  secp256k1.PubKeySecp256k1
+	simSecp256k1SigSize = 64
+)
+
+func init() {
+	bz, _ := hex.DecodeString("035AD6810A47F073553FF30D2FCC7E0D3B1C0B74B61A1AAA2582344037151E143A")
+	copy(simSecp256k1Pubkey[:], bz)
+}
+
 // NewAnteHandler returns an AnteHandler that checks and increments sequence
 // numbers, checks signatures & account numbers, and deducts fees from the first
 // signer.
@@ -54,8 +65,12 @@ func NewAnteHandler(ak AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler {
 			if r := recover(); r != nil {
 				switch rType := r.(type) {
 				case sdk.ErrorOutOfGas:
-					log := fmt.Sprintf("out of gas in location: %v", rType.Descriptor)
+					log := fmt.Sprintf(
+						"out of gas in location: %v; gasWanted: %d, gasUsed: %d",
+						rType.Descriptor, stdTx.Fee.Gas, newCtx.GasMeter().GasConsumed(),
+					)
 					res = sdk.ErrOutOfGas(log).Result()
+
 					res.GasWanted = stdTx.Fee.Gas
 					res.GasUsed = newCtx.GasMeter().GasConsumed()
 					abort = true
@@ -69,7 +84,9 @@ func NewAnteHandler(ak AccountKeeper, fck FeeCollectionKeeper) sdk.AnteHandler {
 			return newCtx, err.Result(), true
 		}
 
-		if res := ValidateMemo(newCtx.GasMeter(), stdTx, params); !res.IsOK() {
+		newCtx.GasMeter().ConsumeGas(params.TxSizeCostPerByte*sdk.Gas(len(newCtx.TxBytes())), "txSize")
+
+		if res := ValidateMemo(stdTx, params); !res.IsOK() {
 			return newCtx, res, true
 		}
 
@@ -131,9 +148,8 @@ func GetSignerAcc(ctx sdk.Context, ak AccountKeeper, addr sdk.AccAddress) (Accou
 	return nil, sdk.ErrUnknownAddress(addr.String()).Result()
 }
 
-// ValidateMemo validates the memo and if successful consumes gas for
-// verification.
-func ValidateMemo(gasMeter sdk.GasMeter, stdTx StdTx, params Params) sdk.Result {
+// ValidateMemo validates the memo size.
+func ValidateMemo(stdTx StdTx, params Params) sdk.Result {
 	memoLength := len(stdTx.GetMemo())
 	if uint64(memoLength) > params.MaxMemoCharacters {
 		return sdk.ErrMemoTooLarge(
@@ -144,7 +160,6 @@ func ValidateMemo(gasMeter sdk.GasMeter, stdTx StdTx, params Params) sdk.Result
 		).Result()
 	}
 
-	gasMeter.ConsumeGas(params.MemoCostPerByte*sdk.Gas(memoLength), "memo")
 	return sdk.Result{}
 }
 
@@ -164,7 +179,7 @@ func processSig(
 		return nil, sdk.ErrInternal("setting PubKey on signer's account").Result()
 	}
 
-	consumeSignatureVerificationGas(ctx.GasMeter(), sig.Signature, pubKey, params)
+	consumeSigVerificationGas(ctx.GasMeter(), sig.Signature, pubKey, params)
 	if !simulate && !pubKey.VerifyBytes(signBytes, sig.Signature) {
 		return nil, sdk.ErrUnauthorized("signature verification failed").Result()
 	}
@@ -178,13 +193,6 @@ func processSig(
 	return acc, res
 }
 
-var dummySecp256k1Pubkey secp256k1.PubKeySecp256k1
-
-func init() {
-	bz, _ := hex.DecodeString("035AD6810A47F073553FF30D2FCC7E0D3B1C0B74B61A1AAA2582344037151E143A")
-	copy(dummySecp256k1Pubkey[:], bz)
-}
-
 // ProcessPubKey verifies that the given account address matches that of the
 // StdSignature. In addition, it will set the public key of the account if it
 // has not been set.
@@ -197,7 +205,7 @@ func ProcessPubKey(acc Account, sig StdSignature, simulate bool) (crypto.PubKey,
 		// shall consume the largest amount, i.e. it takes more gas to verify
 		// secp256k1 keys than ed25519 ones.
 		if pubKey == nil {
-			return dummySecp256k1Pubkey, sdk.Result{}
+			return simSecp256k1Pubkey, sdk.Result{}
 		}
 
 		return pubKey, sdk.Result{}
@@ -218,25 +226,27 @@ func ProcessPubKey(acc Account, sig StdSignature, simulate bool) (crypto.PubKey,
 	return pubKey, sdk.Result{}
 }
 
-// consumeSignatureVerificationGas consumes gas for signature verification based
-// upon the public key type. The cost is fetched from the given params and is
-// matched by the concrete type.
+// consumeSigVerificationGas consumes gas for signature verification based upon
+// the public key type. The cost is fetched from the given params and is matched
+// by the concrete type.
 //
 // TODO: Design a cleaner and flexible way to match concrete public key types.
-func consumeSignatureVerificationGas(meter sdk.GasMeter, sig []byte, pubkey crypto.PubKey, params Params) {
+func consumeSigVerificationGas(meter sdk.GasMeter, sig []byte, pubkey crypto.PubKey, params Params) {
 	pubkeyType := strings.ToLower(fmt.Sprintf("%T", pubkey))
 	switch {
 	case strings.Contains(pubkeyType, "ed25519"):
 		meter.ConsumeGas(params.SigVerifyCostED25519, "ante verify: ed25519")
+
 	case strings.Contains(pubkeyType, "secp256k1"):
 		meter.ConsumeGas(params.SigVerifyCostSecp256k1, "ante verify: secp256k1")
-	case strings.Contains(pubkeyType, "multisigthreshold"):
 
+	case strings.Contains(pubkeyType, "multisigthreshold"):
 		var multisignature multisig.Multisignature
 		codec.Cdc.MustUnmarshalBinaryBare(sig, &multisignature)
-		multisigPubKey := pubkey.(multisig.PubKeyMultisigThreshold)
 
+		multisigPubKey := pubkey.(multisig.PubKeyMultisigThreshold)
 		consumeMultisignatureVerificationGas(meter, multisignature, multisigPubKey, params)
+
 	default:
 		panic(fmt.Sprintf("unrecognized signature type: %s", pubkeyType))
 	}
@@ -250,7 +260,7 @@ func consumeMultisignatureVerificationGas(meter sdk.GasMeter,
 	sigIndex := 0
 	for i := 0; i < size; i++ {
 		if sig.BitArray.GetIndex(i) {
-			consumeSignatureVerificationGas(meter, sig.Sigs[sigIndex], pubkey.PubKeys[i], params)
+			consumeSigVerificationGas(meter, sig.Sigs[sigIndex], pubkey.PubKeys[i], params)
 			sigIndex++
 		}
 	}
@@ -294,8 +304,6 @@ func DeductFees(blockTime time.Time, acc Account, fee StdFee) (Account, sdk.Resu
 // enough fees to cover a proposer's minimum fees. An result object is returned
 // indicating success or failure.
 //
-// TODO: Account for transaction size.
-//
 // Contract: This should only be called during CheckTx as it cannot be part of
 // consensus.
 func EnsureSufficientMempoolFees(ctx sdk.Context, stdFee StdFee) sdk.Result {

x/auth/ante_test.go

@@ -595,9 +595,9 @@ func TestConsumeSignatureVerificationGas(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			if tt.wantPanic {
-				require.Panics(t, func() { consumeSignatureVerificationGas(tt.args.meter, tt.args.sig, tt.args.pubkey, tt.args.params) })
+				require.Panics(t, func() { consumeSigVerificationGas(tt.args.meter, tt.args.sig, tt.args.pubkey, tt.args.params) })
 			} else {
-				consumeSignatureVerificationGas(tt.args.meter, tt.args.sig, tt.args.pubkey, tt.args.params)
+				consumeSigVerificationGas(tt.args.meter, tt.args.sig, tt.args.pubkey, tt.args.params)
 				require.Equal(t, tt.gasConsumed, tt.args.meter.GasConsumed(), fmt.Sprintf("%d != %d", tt.gasConsumed, tt.args.meter.GasConsumed()))
 			}
 		})

x/auth/genesis.go

@@ -54,8 +54,8 @@ func ValidateGenesis(data GenesisState) error {
 	if data.Params.MaxMemoCharacters == 0 {
 		return fmt.Errorf("invalid max memo characters: %d", data.Params.MaxMemoCharacters)
 	}
-	if data.Params.MemoCostPerByte == 0 {
-		return fmt.Errorf("invalid memo cost per byte: %d", data.Params.MemoCostPerByte)
+	if data.Params.TxSizeCostPerByte == 0 {
+		return fmt.Errorf("invalid tx size cost per byte: %d", data.Params.TxSizeCostPerByte)
 	}
 
 	return nil