From 16a5df564267a15d2b3294459e3b5908c61b9e2d Mon Sep 17 00:00:00 2001 From: kbhat1 Date: Wed, 21 Sep 2022 09:48:18 -0400 Subject: [PATCH 1/3] Add priority to check tx based on gas fees --- baseapp/abci.go | 5 +- baseapp/baseapp.go | 13 ++-- baseapp/baseapp_test.go | 6 ++ baseapp/test_helpers.go | 6 +- go.mod | 3 +- go.sum | 6 ++ types/context.go | 8 +++ x/auth/ante/ante.go | 11 +--- x/auth/ante/fee.go | 107 ++++++++++++-------------------- x/auth/ante/fee_test.go | 15 +++-- x/auth/ante/feegrant_test.go | 2 +- x/auth/ante/sigverify.go | 4 ++ x/auth/ante/validator_tx_fee.go | 65 +++++++++++++++++++ 13 files changed, 159 insertions(+), 92 deletions(-) create mode 100644 x/auth/ante/validator_tx_fee.go diff --git a/baseapp/abci.go b/baseapp/abci.go index dcd18cc17..aaa56769f 100644 --- a/baseapp/abci.go +++ b/baseapp/abci.go @@ -202,7 +202,7 @@ func (app *BaseApp) CheckTx(ctx context.Context, req *abci.RequestCheckTx) (*abc } sdkCtx := app.getContextForTx(mode, req.Tx) - gInfo, result, _, err := app.runTx(sdkCtx, mode, req.Tx) + gInfo, result, _, priority, err := app.runTx(sdkCtx, mode, req.Tx) if err != nil { res := sdkerrors.ResponseCheckTx(err, gInfo.GasWanted, gInfo.GasUsed, app.trace) return &res, err @@ -211,6 +211,7 @@ func (app *BaseApp) CheckTx(ctx context.Context, req *abci.RequestCheckTx) (*abc return &abci.ResponseCheckTx{ GasWanted: int64(gInfo.GasWanted), // TODO: Should type accept unsigned ints? Data: result.Data, + Priority: priority, }, nil } @@ -232,7 +233,7 @@ func (app *BaseApp) DeliverTx(ctx sdk.Context, req abci.RequestDeliverTx) abci.R telemetry.SetGauge(float32(gInfo.GasWanted), "tx", "gas", "wanted") }() - gInfo, result, anteEvents, err := app.runTx(ctx.WithTxBytes(req.Tx).WithVoteInfos(app.voteInfos), runTxModeDeliver, req.Tx) + gInfo, result, anteEvents, _, err := app.runTx(ctx.WithTxBytes(req.Tx).WithVoteInfos(app.voteInfos), runTxModeDeliver, req.Tx) if err != nil { resultStr = "failed" return sdkerrors.ResponseDeliverTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, anteEvents, app.trace) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 8c8530ff7..9cdb44894 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -652,7 +652,7 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context // Note, gas execution info is always returned. A reference to a Result is // returned if the tx does not run out of gas and if all the messages are valid // and execute successfully. An error is returned otherwise. -func (app *BaseApp) runTx(ctx sdk.Context, mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, err error) { +func (app *BaseApp) runTx(ctx sdk.Context, mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, priority int64, err error) { // NOTE: GasWanted should be returned by the AnteHandler. GasUsed is // determined by the GasMeter. We need access to the context to get the gas // meter so we initialize upfront. @@ -662,7 +662,7 @@ func (app *BaseApp) runTx(ctx sdk.Context, mode runTxMode, txBytes []byte) (gInf // only run the tx if there is block gas remaining if mode == runTxModeDeliver && ctx.BlockGasMeter().IsOutOfGas() { - return gInfo, nil, nil, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx") + return gInfo, nil, nil, 0, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx") } defer func() { @@ -697,12 +697,12 @@ func (app *BaseApp) runTx(ctx sdk.Context, mode runTxMode, txBytes []byte) (gInf tx, err := app.txDecoder(txBytes) if err != nil { - return sdk.GasInfo{}, nil, nil, err + return sdk.GasInfo{}, nil, nil, 0, err } msgs := tx.GetMsgs() if err := validateBasicTxMsgs(msgs); err != nil { - return sdk.GasInfo{}, nil, nil, err + return sdk.GasInfo{}, nil, nil, 0, err } if app.anteHandler != nil { @@ -738,9 +738,10 @@ func (app *BaseApp) runTx(ctx sdk.Context, mode runTxMode, txBytes []byte) (gInf gasWanted = ctx.GasMeter().Limit() if err != nil { - return gInfo, nil, nil, err + return gInfo, nil, nil, 0, err } + priority = ctx.Priority() msCache.Write() anteEvents = events.ToABCIEvents() } @@ -767,7 +768,7 @@ func (app *BaseApp) runTx(ctx sdk.Context, mode runTxMode, txBytes []byte) (gInf } } - return gInfo, result, anteEvents, err + return gInfo, result, anteEvents, priority, err } // runMsgs iterates through a list of messages and executes them with the provided diff --git a/baseapp/baseapp_test.go b/baseapp/baseapp_test.go index e1da7941e..2c4b75894 100644 --- a/baseapp/baseapp_test.go +++ b/baseapp/baseapp_test.go @@ -38,6 +38,9 @@ import ( var ( capKey1 = sdk.NewKVStoreKey("key1") capKey2 = sdk.NewKVStoreKey("key2") + // testTxPriority is the CheckTx priority that we set in the test + // antehandler. + testTxPriority = int64(42) ) type paramStore struct { @@ -834,6 +837,8 @@ func anteHandlerTxTest(t *testing.T, capKey sdk.StoreKey, storeKey []byte) sdk.A counterEvent("ante_handler", txTest.Counter), ) + ctx = ctx.WithPriority(testTxPriority) + return ctx, nil } } @@ -941,6 +946,7 @@ func TestCheckTx(t *testing.T) { txBytes, err := codec.Marshal(tx) require.NoError(t, err) r, _ := app.CheckTx(context.Background(), &abci.RequestCheckTx{Tx: txBytes}) + require.Equal(t, testTxPriority, r.Priority) require.True(t, r.IsOK(), fmt.Sprintf("%v", r)) } diff --git a/baseapp/test_helpers.go b/baseapp/test_helpers.go index dd21c278a..c90faf122 100644 --- a/baseapp/test_helpers.go +++ b/baseapp/test_helpers.go @@ -16,14 +16,14 @@ func (app *BaseApp) Check(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.GasInfo, *sdk return sdk.GasInfo{}, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "%s", err) } ctx := app.checkState.ctx.WithTxBytes(bz).WithVoteInfos(app.voteInfos).WithConsensusParams(app.GetConsensusParams(app.checkState.ctx)) - gasInfo, result, _, err := app.runTx(ctx, runTxModeCheck, bz) + gasInfo, result, _, _, err := app.runTx(ctx, runTxModeCheck, bz) return gasInfo, result, err } func (app *BaseApp) Simulate(txBytes []byte) (sdk.GasInfo, *sdk.Result, error) { ctx := app.checkState.ctx.WithTxBytes(txBytes).WithVoteInfos(app.voteInfos).WithConsensusParams(app.GetConsensusParams(app.checkState.ctx)) ctx, _ = ctx.CacheContext() - gasInfo, result, _, err := app.runTx(ctx, runTxModeSimulate, txBytes) + gasInfo, result, _, _, err := app.runTx(ctx, runTxModeSimulate, txBytes) return gasInfo, result, err } @@ -34,7 +34,7 @@ func (app *BaseApp) Deliver(txEncoder sdk.TxEncoder, tx sdk.Tx) (sdk.GasInfo, *s return sdk.GasInfo{}, nil, sdkerrors.Wrapf(sdkerrors.ErrInvalidRequest, "%s", err) } ctx := app.deliverState.ctx.WithTxBytes(bz).WithVoteInfos(app.voteInfos).WithConsensusParams(app.GetConsensusParams(app.deliverState.ctx)) - gasInfo, result, _, err := app.runTx(ctx, runTxModeDeliver, bz) + gasInfo, result, _, _, err := app.runTx(ctx, runTxModeDeliver, bz) return gasInfo, result, err } diff --git a/go.mod b/go.mod index 6c296db19..4557b38c7 100644 --- a/go.mod +++ b/go.mod @@ -3,6 +3,7 @@ go 1.18 module github.com/cosmos/cosmos-sdk require ( + cosmossdk.io/math v1.0.0-beta.3 github.com/99designs/keyring v1.1.6 github.com/armon/go-metrics v0.3.10 github.com/bgentry/speakeasy v0.1.0 @@ -37,7 +38,7 @@ require ( github.com/spf13/cobra v1.4.0 github.com/spf13/pflag v1.0.5 github.com/spf13/viper v1.12.0 - github.com/stretchr/testify v1.7.2 + github.com/stretchr/testify v1.8.0 github.com/tendermint/btcd v0.1.1 github.com/tendermint/crypto v0.0.0-20191022145703-50d29ede1e15 github.com/tendermint/go-amino v0.16.0 diff --git a/go.sum b/go.sum index f4a39107b..ca501077c 100644 --- a/go.sum +++ b/go.sum @@ -60,6 +60,8 @@ cloud.google.com/go/storage v1.8.0/go.mod h1:Wv1Oy7z6Yz3DshWRJFhqM/UCfaWIRTdp0RX cloud.google.com/go/storage v1.10.0/go.mod h1:FLPqc6j+Ki4BU591ie1oL6qBQGu2Bl/tZ9ullr3+Kg0= cloud.google.com/go/storage v1.14.0/go.mod h1:GrKmX003DSIwi9o29oFT7YDnHYwZoctc3fOKtUw0Xmo= contrib.go.opencensus.io/exporter/stackdriver v0.13.4/go.mod h1:aXENhDJ1Y4lIg4EUaVTwzvYETVNZk10Pu26tevFKLUc= +cosmossdk.io/math v1.0.0-beta.3 h1:TbZxSopz2LqjJ7aXYfn7nJSb8vNaBklW6BLpcei1qwM= +cosmossdk.io/math v1.0.0-beta.3/go.mod h1:3LYasri3Zna4XpbrTNdKsWmD5fHHkaNAod/mNT9XdE4= dmitri.shuralyov.com/gpu/mtl v0.0.0-20190408044501-666a987793e9/go.mod h1:H6x//7gZCb22OMCxBHrMx7a5I7Hp++hsVxbQ4BYO7hU= filippo.io/edwards25519 v1.0.0-beta.2 h1:/BZRNzm8N4K4eWfK28dL4yescorxtO7YG1yun8fy+pI= filippo.io/edwards25519 v1.0.0-beta.2/go.mod h1:X+pm78QAUPtFLi1z9PYIlS/bdDnvbCOGKtZ+ACWEf7o= @@ -1144,6 +1146,8 @@ github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+ github.com/stretchr/objx v0.1.1/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/objx v0.2.0 h1:Hbg2NidpLE8veEBkEZTL3CvlkUIVzuU9jDplZO54c48= github.com/stretchr/objx v0.2.0/go.mod h1:qt09Ya8vawLte6SNmTgCsAVtYtaKzEcn8ATUoHMkEqE= +github.com/stretchr/objx v0.4.0 h1:M2gUjqZET1qApGOWNSnZ49BAIMX4F/1plDv3+l31EJ4= +github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw= github.com/stretchr/testify v0.0.0-20170130113145-4d4bfba8f1d1/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.1.4/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs= @@ -1155,6 +1159,8 @@ github.com/stretchr/testify v1.7.0/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.7.2 h1:4jaiDzPyXQvSd7D0EjG45355tLlV3VOECpq10pLC+8s= github.com/stretchr/testify v1.7.2/go.mod h1:R6va5+xMeoiuVRoj+gSkQ7d3FALtqAAGI1FQKckRals= +github.com/stretchr/testify v1.8.0 h1:pSgiaMZlXftHpm5L7V1+rVB+AZJydKsMxsQBIJw4PKk= +github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/subosito/gotenv v1.2.0/go.mod h1:N0PQaV/YGNqwC0u51sEeR/aUtSLEXKX9iv69rRypqCw= github.com/subosito/gotenv v1.3.0 h1:mjC+YW8QpAdXibNi+vNWgzmgBH4+5l5dCXv8cNysBLI= github.com/subosito/gotenv v1.3.0/go.mod h1:YzJjq/33h7nrwdY+iHMhEOEEbW0ovIz0tB6t6PwAXzs= diff --git a/types/context.go b/types/context.go index 641424a6a..60d3ca267 100644 --- a/types/context.go +++ b/types/context.go @@ -38,6 +38,7 @@ type Context struct { minGasPrice DecCoins consParams *tmproto.ConsensusParams eventManager *EventManager + priority int64 // The tx priority, only relevant in CheckTx } // Proposed rename, not done to avoid API breakage @@ -58,6 +59,7 @@ func (c Context) IsCheckTx() bool { return c.checkTx } func (c Context) IsReCheckTx() bool { return c.recheckTx } func (c Context) MinGasPrices() DecCoins { return c.minGasPrice } func (c Context) EventManager() *EventManager { return c.eventManager } +func (c Context) Priority() int64 { return c.priority } // clone the header before returning func (c Context) BlockHeader() tmproto.Header { @@ -65,6 +67,12 @@ func (c Context) BlockHeader() tmproto.Header { return *msg } +// WithEventManager returns a Context with an updated tx priority +func (c Context) WithPriority(p int64) Context { + c.priority = p + return c +} + // HeaderHash returns a copy of the header hash obtained during abci.RequestBeginBlock func (c Context) HeaderHash() tmbytes.HexBytes { hash := make([]byte, len(c.headerHash)) diff --git a/x/auth/ante/ante.go b/x/auth/ante/ante.go index b6df48e84..b1a0e247f 100644 --- a/x/auth/ante/ante.go +++ b/x/auth/ante/ante.go @@ -16,6 +16,7 @@ type HandlerOptions struct { SignModeHandler authsigning.SignModeHandler BatchVerifier *SR25519BatchVerifier SigGasConsumer func(meter sdk.GasMeter, sig signing.SignatureV2, params types.Params) error + TxFeeChecker TxFeeChecker } // NewAnteHandler returns an AnteHandler that checks and increments sequence @@ -34,11 +35,6 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { return nil, sdkerrors.Wrap(sdkerrors.ErrLogic, "sign mode handler is required for ante builder") } - var sigGasConsumer = options.SigGasConsumer - if sigGasConsumer == nil { - sigGasConsumer = DefaultSigVerificationGasConsumer - } - var sigVerifyDecorator sdk.AnteDecorator sequentialVerifyDecorator := NewSigVerificationDecorator(options.AccountKeeper, options.SignModeHandler) if options.BatchVerifier == nil { @@ -50,15 +46,14 @@ func NewAnteHandler(options HandlerOptions) (sdk.AnteHandler, error) { anteDecorators := []sdk.AnteDecorator{ NewSetUpContextDecorator(), // outermost AnteDecorator. SetUpContext must be called first NewRejectExtensionOptionsDecorator(), - NewMempoolFeeDecorator(), NewValidateBasicDecorator(), NewTxTimeoutHeightDecorator(), NewValidateMemoDecorator(options.AccountKeeper), NewConsumeGasForTxSizeDecorator(options.AccountKeeper), - NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper), + NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker), NewSetPubKeyDecorator(options.AccountKeeper), // SetPubKeyDecorator must be called before all signature verification decorators NewValidateSigCountDecorator(options.AccountKeeper), - NewSigGasConsumeDecorator(options.AccountKeeper, sigGasConsumer), + NewSigGasConsumeDecorator(options.AccountKeeper, options.SigGasConsumer), sigVerifyDecorator, NewIncrementSequenceDecorator(options.AccountKeeper), } diff --git a/x/auth/ante/fee.go b/x/auth/ante/fee.go index 19e8258cf..291dc5c66 100644 --- a/x/auth/ante/fee.go +++ b/x/auth/ante/fee.go @@ -8,121 +8,96 @@ import ( "github.com/cosmos/cosmos-sdk/x/auth/types" ) -// MempoolFeeDecorator will check if the transaction's fee is at least as large -// as the local validator's minimum gasFee (defined in validator config). -// If fee is too low, decorator returns error and tx is rejected from mempool. -// Note this only applies when ctx.CheckTx = true -// If fee is high enough or not CheckTx, then call next AnteHandler -// CONTRACT: Tx must implement FeeTx to use MempoolFeeDecorator -type MempoolFeeDecorator struct{} - -func NewMempoolFeeDecorator() MempoolFeeDecorator { - return MempoolFeeDecorator{} -} - -func (mfd MempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { - feeTx, ok := tx.(sdk.FeeTx) - if !ok { - return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") - } - - feeCoins := feeTx.GetFee() - gas := feeTx.GetGas() - - // 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 { - minGasPrices := ctx.MinGasPrices() - if !minGasPrices.IsZero() { - requiredFees := make(sdk.Coins, len(minGasPrices)) - - // Determine the required fees by multiplying each required minimum gas - // price by the gas limit, where fee = ceil(minGasPrice * gasLimit). - glDec := sdk.NewDec(int64(gas)) - for i, gp := range minGasPrices { - fee := gp.Amount.Mul(glDec) - requiredFees[i] = sdk.NewCoin(gp.Denom, fee.Ceil().RoundInt()) - } - - if !feeCoins.IsAnyGTE(requiredFees) { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, requiredFees) - } - } - } - - return next(ctx, tx, simulate) -} +// TxFeeChecker check if the provided fee is enough and returns the effective fee and tx priority, +// the effective fee should be deducted later, and the priority should be returned in abci response. +type TxFeeChecker func(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) // DeductFeeDecorator deducts fees from the first signer of the tx // If the first signer does not have the funds to pay for the fees, return with InsufficientFunds error // Call next AnteHandler if fees successfully deducted // CONTRACT: Tx must implement FeeTx interface to use DeductFeeDecorator type DeductFeeDecorator struct { - ak AccountKeeper + accountKeeper AccountKeeper bankKeeper types.BankKeeper feegrantKeeper FeegrantKeeper + txFeeChecker TxFeeChecker } -func NewDeductFeeDecorator(ak AccountKeeper, bk types.BankKeeper, fk FeegrantKeeper) DeductFeeDecorator { +func NewDeductFeeDecorator(ak AccountKeeper, bk types.BankKeeper, fk FeegrantKeeper, tfc TxFeeChecker) DeductFeeDecorator { + if tfc == nil { + tfc = checkTxFeeWithValidatorMinGasPrices + } + return DeductFeeDecorator{ - ak: ak, + accountKeeper: ak, bankKeeper: bk, feegrantKeeper: fk, + txFeeChecker: tfc, + } +} + +func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) { + fee, priority, err := dfd.txFeeChecker(ctx, tx) + if err != nil { + return ctx, err } + if err := dfd.checkDeductFee(ctx, tx, fee); err != nil { + return ctx, err + } + + newCtx := ctx.WithPriority(priority) + + return next(newCtx, tx, simulate) } -func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) { - feeTx, ok := tx.(sdk.FeeTx) +func (dfd DeductFeeDecorator) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee sdk.Coins) error { + feeTx, ok := sdkTx.(sdk.FeeTx) if !ok { - return ctx, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") + return sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") } - if addr := dfd.ak.GetModuleAddress(types.FeeCollectorName); addr == nil { - return ctx, fmt.Errorf("Fee collector module account (%s) has not been set", types.FeeCollectorName) + if addr := dfd.accountKeeper.GetModuleAddress(types.FeeCollectorName); addr == nil { + return fmt.Errorf("Fee collector module account (%s) has not been set", types.FeeCollectorName) } - fee := feeTx.GetFee() feePayer := feeTx.FeePayer() feeGranter := feeTx.FeeGranter() - deductFeesFrom := feePayer // if feegranter set deduct fee from feegranter account. // this works with only when feegrant enabled. if feeGranter != nil { if dfd.feegrantKeeper == nil { - return ctx, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "fee grants are not enabled") + return sdkerrors.ErrInvalidRequest.Wrap("fee grants are not enabled") } else if !feeGranter.Equals(feePayer) { - err := dfd.feegrantKeeper.UseGrantedFees(ctx, feeGranter, feePayer, fee, tx.GetMsgs()) - + err := dfd.feegrantKeeper.UseGrantedFees(ctx, feeGranter, feePayer, fee, sdkTx.GetMsgs()) if err != nil { - return ctx, sdkerrors.Wrapf(err, "%s not allowed to pay fees from %s", feeGranter, feePayer) + return sdkerrors.Wrapf(err, "%s does not not allow to pay fees for %s", feeGranter, feePayer) } } deductFeesFrom = feeGranter } - deductFeesFromAcc := dfd.ak.GetAccount(ctx, deductFeesFrom) + deductFeesFromAcc := dfd.accountKeeper.GetAccount(ctx, deductFeesFrom) if deductFeesFromAcc == nil { - return ctx, sdkerrors.Wrapf(sdkerrors.ErrUnknownAddress, "fee payer address: %s does not exist", deductFeesFrom) + return sdkerrors.ErrUnknownAddress.Wrapf("fee payer address: %s does not exist", deductFeesFrom) } // deduct the fees - if !feeTx.GetFee().IsZero() { - err = DeductFees(dfd.bankKeeper, ctx, deductFeesFromAcc, feeTx.GetFee()) + if !fee.IsZero() { + err := DeductFees(dfd.bankKeeper, ctx, deductFeesFromAcc, fee) if err != nil { - return ctx, err + return err } } events := sdk.Events{sdk.NewEvent(sdk.EventTypeTx, - sdk.NewAttribute(sdk.AttributeKeyFee, feeTx.GetFee().String()), + sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()), )} ctx.EventManager().EmitEvents(events) - return next(ctx, tx, simulate) + return nil } // DeductFees deducts fees from the given account. diff --git a/x/auth/ante/fee_test.go b/x/auth/ante/fee_test.go index 7edd8fad9..25cf0d699 100644 --- a/x/auth/ante/fee_test.go +++ b/x/auth/ante/fee_test.go @@ -12,16 +12,18 @@ func (suite *AnteTestSuite) TestEnsureMempoolFees() { suite.SetupTest(true) // setup suite.txBuilder = suite.clientCtx.TxConfig.NewTxBuilder() - mfd := ante.NewMempoolFeeDecorator() + mfd := ante.NewDeductFeeDecorator(suite.app.AccountKeeper, suite.app.BankKeeper, suite.app.FeeGrantKeeper, nil) antehandler := sdk.ChainAnteDecorators(mfd) // keys and addresses priv1, _, addr1 := testdata.KeyTestPubAddr() + coins := sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(300))) + testutil.FundAccount(suite.app.BankKeeper, suite.ctx, addr1, coins) // msg and signatures msg := testdata.NewTestMsg(addr1) feeAmount := testdata.NewTestFeeAmount() - gasLimit := testdata.NewTestGasLimit() + gasLimit := uint64(15) suite.Require().NoError(suite.txBuilder.SetMsgs(msg)) suite.txBuilder.SetFeeAmount(feeAmount) suite.txBuilder.SetGasLimit(gasLimit) @@ -31,7 +33,7 @@ func (suite *AnteTestSuite) TestEnsureMempoolFees() { suite.Require().NoError(err) // Set high gas price so standard test fee fails - atomPrice := sdk.NewDecCoinFromDec("atom", sdk.NewDec(200).Quo(sdk.NewDec(100000))) + atomPrice := sdk.NewDecCoinFromDec("atom", math.LegacyNewDec(20)) highGasPrice := []sdk.DecCoin{atomPrice} suite.ctx = suite.ctx.WithMinGasPrices(highGasPrice) @@ -56,8 +58,11 @@ func (suite *AnteTestSuite) TestEnsureMempoolFees() { lowGasPrice := []sdk.DecCoin{atomPrice} suite.ctx = suite.ctx.WithMinGasPrices(lowGasPrice) - _, err = antehandler(suite.ctx, tx, false) + newCtx, err = antehandler(suite.ctx, tx, false) suite.Require().Nil(err, "Decorator should not have errored on fee higher than local gasPrice") + / Priority is the smallest gas price amount in any denom. Since we have only 1 gas price + // of 10atom, the priority here is 10. + require.Equal(t, int64(10), newCtx.Priority()) } func (suite *AnteTestSuite) TestDeductFees() { @@ -86,7 +91,7 @@ func (suite *AnteTestSuite) TestDeductFees() { err = simapp.FundAccount(suite.app.BankKeeper, suite.ctx, addr1, coins) suite.Require().NoError(err) - dfd := ante.NewDeductFeeDecorator(suite.app.AccountKeeper, suite.app.BankKeeper, nil) + dfd := ante.NewDeductFeeDecorator(suite.app.AccountKeeper, suite.app.BankKeeper, nil, nil) antehandler := sdk.ChainAnteDecorators(dfd) _, err = antehandler(suite.ctx, tx, false) diff --git a/x/auth/ante/feegrant_test.go b/x/auth/ante/feegrant_test.go index b23c2cec6..39e0ce5b5 100644 --- a/x/auth/ante/feegrant_test.go +++ b/x/auth/ante/feegrant_test.go @@ -32,7 +32,7 @@ func (suite *AnteTestSuite) TestDeductFeesNoDelegation() { protoTxCfg := tx.NewTxConfig(codec.NewProtoCodec(app.InterfaceRegistry()), tx.DefaultSignModes) // this just tests our handler - dfd := ante.NewDeductFeeDecorator(app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper) + dfd := ante.NewDeductFeeDecorator(app.AccountKeeper, app.BankKeeper, app.FeeGrantKeeper, nil) feeAnteHandler := sdk.ChainAnteDecorators(dfd) // this tests the whole stack diff --git a/x/auth/ante/sigverify.go b/x/auth/ante/sigverify.go index 2c71cb3d4..583d1aa5b 100644 --- a/x/auth/ante/sigverify.go +++ b/x/auth/ante/sigverify.go @@ -137,6 +137,10 @@ type SigGasConsumeDecorator struct { } func NewSigGasConsumeDecorator(ak AccountKeeper, sigGasConsumer SignatureVerificationGasConsumer) SigGasConsumeDecorator { + if sigGasConsumer == nil { + sigGasConsumer = DefaultSigVerificationGasConsumer + } + return SigGasConsumeDecorator{ ak: ak, sigGasConsumer: sigGasConsumer, diff --git a/x/auth/ante/validator_tx_fee.go b/x/auth/ante/validator_tx_fee.go new file mode 100644 index 000000000..dbb139888 --- /dev/null +++ b/x/auth/ante/validator_tx_fee.go @@ -0,0 +1,65 @@ +package ante + +import ( + "math" + + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// checkTxFeeWithValidatorMinGasPrices implements the default fee logic, where the minimum price per +// unit of gas is fixed and set by each validator, can the tx priority is computed from the gas price. +func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) { + feeTx, ok := tx.(sdk.FeeTx) + if !ok { + return nil, 0, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx") + } + + feeCoins := feeTx.GetFee() + gas := feeTx.GetGas() + + // 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() { + minGasPrices := ctx.MinGasPrices() + if !minGasPrices.IsZero() { + requiredFees := make(sdk.Coins, len(minGasPrices)) + + // Determine the required fees by multiplying each required minimum gas + // price by the gas limit, where fee = ceil(minGasPrice * gasLimit). + glDec := sdk.NewDec(int64(gas)) + for i, gp := range minGasPrices { + fee := gp.Amount.Mul(glDec) + requiredFees[i] = sdk.NewCoin(gp.Denom, fee.Ceil().RoundInt()) + } + + if !feeCoins.IsAnyGTE(requiredFees) { + return nil, 0, sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, requiredFees) + } + } + } + + priority := getTxPriority(feeCoins, int64(gas)) + return feeCoins, priority, nil +} + +// getTxPriority returns a naive tx priority based on the amount of the smallest denomination of the gas price +// provided in a transaction. +// NOTE: This implementation should be used with a great consideration as it opens potential attack vectors +// where txs with multiple coins could not be prioritize as expected. +func getTxPriority(fee sdk.Coins, gas int64) int64 { + var priority int64 + for _, c := range fee { + p := int64(math.MaxInt64) + gasPrice := c.Amount.QuoRaw(gas) + if gasPrice.IsInt64() { + p = gasPrice.Int64() + } + if priority == 0 || p < priority { + priority = p + } + } + + return priority +} From 44e9219c264915f256335f769b653eec3f40cf46 Mon Sep 17 00:00:00 2001 From: kbhat1 Date: Wed, 21 Sep 2022 10:19:59 -0400 Subject: [PATCH 2/3] Update fee test --- x/auth/ante/fee_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/x/auth/ante/fee_test.go b/x/auth/ante/fee_test.go index 25cf0d699..494f14618 100644 --- a/x/auth/ante/fee_test.go +++ b/x/auth/ante/fee_test.go @@ -18,7 +18,8 @@ func (suite *AnteTestSuite) TestEnsureMempoolFees() { // keys and addresses priv1, _, addr1 := testdata.KeyTestPubAddr() coins := sdk.NewCoins(sdk.NewCoin("atom", sdk.NewInt(300))) - testutil.FundAccount(suite.app.BankKeeper, suite.ctx, addr1, coins) + err := simapp.FundAccount(suite.app.BankKeeper, suite.ctx, addr1, coins) + suite.Require().NoError(err) // msg and signatures msg := testdata.NewTestMsg(addr1) @@ -33,7 +34,7 @@ func (suite *AnteTestSuite) TestEnsureMempoolFees() { suite.Require().NoError(err) // Set high gas price so standard test fee fails - atomPrice := sdk.NewDecCoinFromDec("atom", math.LegacyNewDec(20)) + atomPrice := sdk.NewDecCoinFromDec("atom", sdk.NewDec(20)) highGasPrice := []sdk.DecCoin{atomPrice} suite.ctx = suite.ctx.WithMinGasPrices(highGasPrice) @@ -58,11 +59,11 @@ func (suite *AnteTestSuite) TestEnsureMempoolFees() { lowGasPrice := []sdk.DecCoin{atomPrice} suite.ctx = suite.ctx.WithMinGasPrices(lowGasPrice) - newCtx, err = antehandler(suite.ctx, tx, false) + newCtx, err := antehandler(suite.ctx, tx, false) suite.Require().Nil(err, "Decorator should not have errored on fee higher than local gasPrice") - / Priority is the smallest gas price amount in any denom. Since we have only 1 gas price + // Priority is the smallest gas price amount in any denom. Since we have only 1 gas price // of 10atom, the priority here is 10. - require.Equal(t, int64(10), newCtx.Priority()) + suite.Equal(int64(10), newCtx.Priority()) } func (suite *AnteTestSuite) TestDeductFees() { From 50ca01a031b69be32d85b40ca261bf6229bc360c Mon Sep 17 00:00:00 2001 From: kbhat1 Date: Thu, 22 Sep 2022 17:12:49 -0400 Subject: [PATCH 3/3] Update priority in error + simapp --- baseapp/baseapp.go | 2 +- simapp/app.go | 1 + x/auth/ante/fee.go | 4 ++-- x/auth/ante/validator_tx_fee.go | 2 +- 4 files changed, 5 insertions(+), 4 deletions(-) diff --git a/baseapp/baseapp.go b/baseapp/baseapp.go index 9cdb44894..d4ccac089 100644 --- a/baseapp/baseapp.go +++ b/baseapp/baseapp.go @@ -662,7 +662,7 @@ func (app *BaseApp) runTx(ctx sdk.Context, mode runTxMode, txBytes []byte) (gInf // only run the tx if there is block gas remaining if mode == runTxModeDeliver && ctx.BlockGasMeter().IsOutOfGas() { - return gInfo, nil, nil, 0, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx") + return gInfo, nil, nil, -1, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx") } defer func() { diff --git a/simapp/app.go b/simapp/app.go index 6c556ec4e..a92cd9759 100644 --- a/simapp/app.go +++ b/simapp/app.go @@ -416,6 +416,7 @@ func NewSimApp( FeegrantKeeper: app.FeeGrantKeeper, BatchVerifier: app.batchVerifier, SigGasConsumer: ante.DefaultSigVerificationGasConsumer, + TxFeeChecker: ante.CheckTxFeeWithValidatorMinGasPrices, }, ) diff --git a/x/auth/ante/fee.go b/x/auth/ante/fee.go index 291dc5c66..4939f1f74 100644 --- a/x/auth/ante/fee.go +++ b/x/auth/ante/fee.go @@ -20,12 +20,12 @@ type DeductFeeDecorator struct { accountKeeper AccountKeeper bankKeeper types.BankKeeper feegrantKeeper FeegrantKeeper - txFeeChecker TxFeeChecker + txFeeChecker TxFeeChecker } func NewDeductFeeDecorator(ak AccountKeeper, bk types.BankKeeper, fk FeegrantKeeper, tfc TxFeeChecker) DeductFeeDecorator { if tfc == nil { - tfc = checkTxFeeWithValidatorMinGasPrices + tfc = CheckTxFeeWithValidatorMinGasPrices } return DeductFeeDecorator{ diff --git a/x/auth/ante/validator_tx_fee.go b/x/auth/ante/validator_tx_fee.go index dbb139888..6fbfbf253 100644 --- a/x/auth/ante/validator_tx_fee.go +++ b/x/auth/ante/validator_tx_fee.go @@ -9,7 +9,7 @@ import ( // checkTxFeeWithValidatorMinGasPrices implements the default fee logic, where the minimum price per // unit of gas is fixed and set by each validator, can the tx priority is computed from the gas price. -func checkTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) { +func CheckTxFeeWithValidatorMinGasPrices(ctx sdk.Context, tx sdk.Tx) (sdk.Coins, int64, error) { feeTx, ok := tx.(sdk.FeeTx) if !ok { return nil, 0, sdkerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx")