From 28347bf5f7369a8ee1673c19be51f723b686b650 Mon Sep 17 00:00:00 2001 From: Aditya Date: Tue, 15 Oct 2019 13:21:54 -0700 Subject: [PATCH] Merge PR #5195: Move Signature Size calculation to TxSizeGasDecorator --- x/auth/ante/ante_test.go | 52 ++++++++++++++++++++++++++++++++++ x/auth/ante/basic.go | 54 +++++++++++++++++++++++++++++++++-- x/auth/ante/basic_test.go | 41 +++++++++++++++++++++++++-- x/auth/ante/fee.go | 4 +++ x/auth/ante/sigverify.go | 59 ++++++++++++++------------------------- 5 files changed, 167 insertions(+), 43 deletions(-) diff --git a/x/auth/ante/ante_test.go b/x/auth/ante/ante_test.go index b3d22061b6..60fe2149cd 100644 --- a/x/auth/ante/ante_test.go +++ b/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 diff --git a/x/auth/ante/basic.go b/x/auth/ante/basic.go index b73374a800..d14f720b2a 100644 --- a/x/auth/ante/basic.go +++ b/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) } diff --git a/x/auth/ante/basic_test.go b/x/auth/ante/basic_test.go index db9c1a8b31..90e7e8c1c0 100644 --- a/x/auth/ante/basic_test.go +++ b/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) } diff --git a/x/auth/ante/fee.go b/x/auth/ante/fee.go index d428cb7135..3d81808b2d 100644 --- a/x/auth/ante/fee.go +++ b/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()) diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index 4b340907e1..f166ad4aaf 100644 --- a/x/auth/ante/sigverify.go +++ b/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) {