Merge PR #5195: Move Signature Size calculation to TxSizeGasDecorator

x/auth/ante/ante_test.go

@@ -35,6 +35,58 @@ func checkInvalidTx(t *testing.T, anteHandler sdk.AnteHandler, ctx sdk.Context,
 	require.Equal(t, sdk.CodespaceRoot, result.Codespace)
 }
 
+// Test that simulate transaction accurately estimates gas cost
+func TestSimulateGasCost(t *testing.T) {
+	// setup
+	app, ctx := createTestApp(true)
+	ctx = ctx.WithBlockHeight(1)
+	anteHandler := ante.NewAnteHandler(app.AccountKeeper, app.SupplyKeeper, ante.DefaultSigVerificationGasConsumer)
+
+	// keys and addresses
+	priv1, _, addr1 := types.KeyTestPubAddr()
+	priv2, _, addr2 := types.KeyTestPubAddr()
+	priv3, _, addr3 := types.KeyTestPubAddr()
+
+	// set the accounts
+	acc1 := app.AccountKeeper.NewAccountWithAddress(ctx, addr1)
+	acc1.SetCoins(types.NewTestCoins())
+	require.NoError(t, acc1.SetAccountNumber(0))
+	app.AccountKeeper.SetAccount(ctx, acc1)
+	acc2 := app.AccountKeeper.NewAccountWithAddress(ctx, addr2)
+	acc2.SetCoins(types.NewTestCoins())
+	require.NoError(t, acc2.SetAccountNumber(1))
+	app.AccountKeeper.SetAccount(ctx, acc2)
+	acc3 := app.AccountKeeper.NewAccountWithAddress(ctx, addr3)
+	acc3.SetCoins(types.NewTestCoins())
+	require.NoError(t, acc3.SetAccountNumber(2))
+	app.AccountKeeper.SetAccount(ctx, acc3)
+
+	// set up msgs and fee
+	var tx sdk.Tx
+	msg1 := types.NewTestMsg(addr1, addr2)
+	msg2 := types.NewTestMsg(addr3, addr1)
+	msg3 := types.NewTestMsg(addr2, addr3)
+	msgs := []sdk.Msg{msg1, msg2, msg3}
+	fee := types.NewTestStdFee()
+
+	// signers in order. accnums are all 0 because it is in genesis block
+	privs, accnums, seqs := []crypto.PrivKey{priv1, priv2, priv3}, []uint64{0, 1, 2}, []uint64{0, 0, 0}
+	tx = types.NewTestTx(ctx, msgs, privs, accnums, seqs, fee)
+
+	cc, _ := ctx.CacheContext()
+	newCtx, err := anteHandler(cc, tx, true)
+	require.Nil(t, err, "transaction failed on simulate mode")
+
+	simulatedGas := newCtx.GasMeter().GasConsumed()
+	fee.Gas = simulatedGas
+
+	// update tx with simulated gas estimate
+	tx = types.NewTestTx(ctx, msgs, privs, accnums, seqs, fee)
+	_, err = anteHandler(ctx, tx, false)
+
+	require.Nil(t, err, "transaction failed with gas estimate")
+}
+
 // Test various error cases in the AnteHandler control flow.
 func TestAnteHandlerSigErrors(t *testing.T) {
 	// setup

x/auth/ante/basic.go

@@ -5,6 +5,9 @@ import (
 	err "github.com/cosmos/cosmos-sdk/types/errors"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
 
+	"github.com/tendermint/tendermint/crypto"
+	"github.com/tendermint/tendermint/crypto/multisig"
+
 	"github.com/cosmos/cosmos-sdk/x/auth/keeper"
 	"github.com/cosmos/cosmos-sdk/x/auth/types"
 )
@@ -67,8 +70,15 @@ func (vmd ValidateMemoDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate
 	return next(ctx, tx, simulate)
 }
 
-// ConsumeTxSizeGasDecorator will take in parameters and consume gas proportional to the size of tx
-// before calling next AnteHandler
+// ConsumeTxSizeGasDecorator will take in parameters and consume gas proportional
+// to the size of tx before calling next AnteHandler. Note, the gas costs will be
+// slightly over estimated due to the fact that any given signing account may need
+// to be retrieved from state.
+//
+// CONTRACT: If simulate=true, then signatures must either be completely filled
+// in or empty.
+// CONTRACT: To use this decorator, signatures of transaction must be represented
+// as types.StdSignature otherwise simulate mode will incorrectly estimate gas cost.
 type ConsumeTxSizeGasDecorator struct {
 	ak keeper.AccountKeeper
 }
@@ -80,8 +90,48 @@ func NewConsumeGasForTxSizeDecorator(ak keeper.AccountKeeper) ConsumeTxSizeGasDe
 }
 
 func (cgts ConsumeTxSizeGasDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
+	sigTx, ok := tx.(SigVerifiableTx)
+	if !ok {
+		return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "invalid tx type")
+	}
 	params := cgts.ak.GetParams(ctx)
 	ctx.GasMeter().ConsumeGas(params.TxSizeCostPerByte*sdk.Gas(len(ctx.TxBytes())), "txSize")
 
+	// simulate gas cost for signatures in simulate mode
+	if simulate {
+		// in simulate mode, each element should be a nil signature
+		sigs := sigTx.GetSignatures()
+		for i, signer := range sigTx.GetSigners() {
+			// if signature is already filled in, no need to simulate gas cost
+			if sigs[i] != nil {
+				continue
+			}
+			acc := cgts.ak.GetAccount(ctx, signer)
+
+			var pubkey crypto.PubKey
+			// use placeholder simSecp256k1Pubkey if sig is nil
+			if acc == nil || acc.GetPubKey() == nil {
+				pubkey = simSecp256k1Pubkey
+			} else {
+				pubkey = acc.GetPubKey()
+			}
+			// use stdsignature to mock the size of a full signature
+			simSig := types.StdSignature{
+				Signature: simSecp256k1Sig[:],
+				PubKey:    pubkey,
+			}
+			sigBz := types.ModuleCdc.MustMarshalBinaryLengthPrefixed(simSig)
+			cost := sdk.Gas(len(sigBz) + 6)
+
+			// If the pubkey is a multi-signature pubkey, then we estimate for the maximum
+			// number of signers.
+			if _, ok := pubkey.(multisig.PubKeyMultisigThreshold); ok {
+				cost *= params.TxSigLimit
+			}
+
+			ctx.GasMeter().ConsumeGas(params.TxSizeCostPerByte*cost, "txSize")
+		}
+	}
+
 	return next(ctx, tx, simulate)
 }

x/auth/ante/basic_test.go

@@ -1,6 +1,7 @@
 package ante_test
 
 import (
+	"encoding/json"
 	"strings"
 	"testing"
 
@@ -75,11 +76,24 @@ func TestConsumeGasForTxSize(t *testing.T) {
 	// setup
 	app, ctx := createTestApp(true)
 
+	// keys and addresses
+	priv1, _, addr1 := types.KeyTestPubAddr()
+
+	// msg and signatures
+	msg1 := types.NewTestMsg(addr1)
+	fee := types.NewTestStdFee()
+
+	msgs := []sdk.Msg{msg1}
+
+	privs, accNums, seqs := []crypto.PrivKey{priv1}, []uint64{0}, []uint64{0}
+	tx := types.NewTestTxWithMemo(ctx, msgs, privs, accNums, seqs, fee, strings.Repeat("01234567890", 10))
+	txBytes, err := json.Marshal(tx)
+	require.Nil(t, err, "Cannot marshal tx: %v", err)
+
 	cgtsd := ante.NewConsumeGasForTxSizeDecorator(app.AccountKeeper)
 	antehandler := sdk.ChainAnteDecorators(cgtsd)
 
 	params := app.AccountKeeper.GetParams(ctx)
-	txBytes := []byte(strings.Repeat("a", 10))
 	expectedGas := sdk.Gas(len(txBytes)) * params.TxSizeCostPerByte
 
 	// Set ctx with TxBytes manually
@@ -91,12 +105,33 @@ func TestConsumeGasForTxSize(t *testing.T) {
 	afterGas := ctx.GasMeter().GasConsumed()
 	expectedGas += afterGas - beforeGas
 
-	// No need to send tx here since this Decorator will do nothing with it
 	beforeGas = ctx.GasMeter().GasConsumed()
-	ctx, err := antehandler(ctx, nil, false)
+	ctx, err = antehandler(ctx, tx, false)
 	require.Nil(t, err, "ConsumeTxSizeGasDecorator returned error: %v", err)
 
 	// require that decorator consumes expected amount of gas
 	consumedGas := ctx.GasMeter().GasConsumed() - beforeGas
 	require.Equal(t, expectedGas, consumedGas, "Decorator did not consume the correct amount of gas")
+
+	// simulation must not underestimate gas of this decorator even with nil signatures
+	sigTx := tx.(types.StdTx)
+	sigTx.Signatures = []types.StdSignature{{}}
+
+	simTxBytes, err := json.Marshal(sigTx)
+	require.Nil(t, err)
+	// require that simulated tx is smaller than tx with signatures
+	require.True(t, len(simTxBytes) < len(txBytes), "simulated tx still has signatures")
+
+	// Set ctx with smaller simulated TxBytes manually
+	ctx = ctx.WithTxBytes(txBytes)
+
+	beforeSimGas := ctx.GasMeter().GasConsumed()
+
+	// run antehandler with simulate=true
+	ctx, err = antehandler(ctx, sigTx, true)
+	consumedSimGas := ctx.GasMeter().GasConsumed() - beforeSimGas
+
+	// require that antehandler passes and does not underestimate decorator cost
+	require.Nil(t, err, "ConsumeTxSizeGasDecorator returned error: %v", err)
+	require.True(t, consumedSimGas >= expectedGas, "Simulate mode underestimates gas on AnteDecorator. Simulated cost: %d, expected cost: %d", consumedSimGas, expectedGas)
 }

x/auth/ante/fee.go

@@ -97,6 +97,10 @@ func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bo
 	feePayer := feeTx.FeePayer()
 	feePayerAcc := dfd.ak.GetAccount(ctx, feePayer)
 
+	if feePayerAcc == nil {
+		return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "fee payer address: %s does not exist", feePayer)
+	}
+
 	// deduct the fees
 	if !feeTx.GetFee().IsZero() {
 		err = DeductFees(dfd.supplyKeeper, ctx, feePayerAcc, feeTx.GetFee())

x/auth/ante/sigverify.go

@@ -59,9 +59,6 @@ func NewSetPubKeyDecorator(ak keeper.AccountKeeper) SetPubKeyDecorator {
 }
 
 func (spkd SetPubKeyDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
-	if simulate {
-		return next(ctx, tx, simulate)
-	}
 	sigTx, ok := tx.(SigVerifiableTx)
 	if !ok {
 		return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "invalid tx type")
@@ -73,9 +70,13 @@ func (spkd SetPubKeyDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b
 	for i, pk := range pubkeys {
 		// PublicKey was omitted from slice since it has already been set in context
 		if pk == nil {
-			continue
+			if !simulate {
+				continue
+			}
+			pk = simSecp256k1Pubkey
 		}
-		if !bytes.Equal(pk.Address(), signers[i]) {
+		// Only make check if simulate=false
+		if !simulate && !bytes.Equal(pk.Address(), signers[i]) {
 			return ctx, sdkerrors.Wrapf(sdkerrors.ErrInvalidPubKey,
 				"pubKey does not match signer address %s with signer index: %d", signers[i], i)
 		}
@@ -84,6 +85,10 @@ func (spkd SetPubKeyDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b
 		if err != nil {
 			return ctx, err
 		}
+		// account already has pubkey set,no need to reset
+		if acc.GetPubKey() != nil {
+			continue
+		}
 		err = acc.SetPubKey(pk)
 		if err != nil {
 			return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidPubKey, err.Error())
@@ -130,18 +135,19 @@ func (sgcd SigGasConsumeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 		}
 		pubKey := signerAcc.GetPubKey()
 
-		if simulate {
-			// Simulated txs should not contain a signature and are not required to
-			// contain a pubkey, so we must account for tx size of including a
-			// StdSignature (Amino encoding) and simulate gas consumption
-			// (assuming a SECP256k1 simulation key).
-			consumeSimSigGas(ctx.GasMeter(), pubKey, sig, params)
-		} else {
-			err = sgcd.sigGasConsumer(ctx.GasMeter(), sig, pubKey, params)
-			if err != nil {
-				return ctx, err
+		if simulate && pubKey == nil {
+			// In simulate mode the transaction comes with no signatures, thus if the
+			// account's pubkey is nil, both signature verification and gasKVStore.Set()
+			// shall consume the largest amount, i.e. it takes more gas to verify
+			// secp256k1 keys than ed25519 ones.
+			if pubKey == nil {
+				pubKey = simSecp256k1Pubkey
 			}
 		}
+		err = sgcd.sigGasConsumer(ctx.GasMeter(), sig, pubKey, params)
+		if err != nil {
+			return ctx, err
+		}
 	}
 
 	return next(ctx, tx, simulate)
@@ -312,29 +318,6 @@ func consumeMultisignatureVerificationGas(meter sdk.GasMeter,
 	}
 }
 
-// Internal function that simulates gas consumption of signature verification when simulate=true
-// TODO: allow users to simulate signatures other than auth.StdSignature
-func consumeSimSigGas(gasmeter sdk.GasMeter, pubkey crypto.PubKey, sig []byte, params types.Params) {
-	simSig := types.StdSignature{
-		Signature: sig,
-		PubKey:    pubkey,
-	}
-	if len(sig) == 0 {
-		simSig.Signature = simSecp256k1Sig[:]
-	}
-
-	sigBz := types.ModuleCdc.MustMarshalBinaryLengthPrefixed(simSig)
-	cost := sdk.Gas(len(sigBz) + 6)
-
-	// If the pubkey is a multi-signature pubkey, then we estimate for the maximum
-	// number of signers.
-	if _, ok := pubkey.(multisig.PubKeyMultisigThreshold); ok {
-		cost *= params.TxSigLimit
-	}
-
-	gasmeter.ConsumeGas(params.TxSizeCostPerByte*cost, "txSize")
-}
-
 // GetSignerAcc returns an account for a given address that is expected to sign
 // a transaction.
 func GetSignerAcc(ctx sdk.Context, ak keeper.AccountKeeper, addr sdk.AccAddress) (exported.Account, error) {