fix: restore old mempool fee decorator #9045
Conversation
In particular, test for double payment of fee (#9036).
Was retained for testing.
| } | ||
|
|
||
| if !feeCoins.IsAnyGTE(requiredFees) { | ||
| return ctx, sdkioerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, requiredFees) |
There was a problem hiding this comment.
question:
Shouldn't this also be checked when simulate == true. It feels like during simulation a caller would want to know they didn't have enough to cover fees.
There was a problem hiding this comment.
Unfortunately, the gas parameter in line 45 (called gasLimit in the comment) is not known during simulation. Clients running a tx simulation set the input gas parameter to 0, and don't provide any fee parameter either.
Indeed, the only use of simulation is to tell clients a reasonable value for the gas parameter. Then the clients use that value to determine how much of a fee they should attach to the non-simulated (signed) transaction.
There was a problem hiding this comment.
Note that the simulate flag is going away. Instead, there is an "exec mode" in the context. cosmos/cosmos-sdk#19586
| } | ||
|
|
||
| if !feeCoins.IsAnyGTE(requiredFees) { | ||
| return ctx, sdkioerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, requiredFees) |
There was a problem hiding this comment.
Unfortunately, the gas parameter in line 45 (called gasLimit in the comment) is not known during simulation. Clients running a tx simulation set the input gas parameter to 0, and don't provide any fee parameter either.
Indeed, the only use of simulation is to tell clients a reasonable value for the gas parameter. Then the clients use that value to determine how much of a fee they should attach to the non-simulated (signed) transaction.
| // former ante.NewMempoolFeeDecorator() | ||
| // replaced as in https://github.com/provenance-io/provenance/pull/1016 | ||
| ante.NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, nil), | ||
| NewMempoolFeeDecorator(), |
There was a problem hiding this comment.
Looking at fee.go and this line, I agree that this is the minimal fix.
In fairness, the double-charge was the result of using both the replacement decorator (enabling CometBFT mempool prioritization), as well as our forked DeductFee decorator (which allows specifying the fee collector module account). I think we should merge their functionality sometime in the future, and use only that merged version in the ante handler decorators. |
There was a problem hiding this comment.
I think we can avoid reintroducing MempoolFeeDecorator (#9045 (comment) ), but let me know if you feel strongly that we should and I'll change this to an approving review.
| // former ante.NewMempoolFeeDecorator() | ||
| // replaced as in https://github.com/provenance-io/provenance/pull/1016 | ||
| ante.NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, nil), | ||
| NewMempoolFeeDecorator(), | ||
| ante.NewValidateBasicDecorator(), | ||
| ante.NewTxTimeoutHeightDecorator(), | ||
| ante.NewValidateMemoDecorator(opts.AccountKeeper), |
There was a problem hiding this comment.
I think it's a good time to explain our NewDeductFeeDecorator, e.g.
ante.NewValidateMemoDecorator(opts.AccountKeeper),
ante.NewConsumeGasForTxSizeDecorator(opts.AccountKeeper),
+
+ // We use a custom fee-deduction decorator
+ // to support a dynamically-configurable collector destination.
+ // [cosmos-sdk v0.46.16] NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker),
NewInboundDecorator(opts.SwingsetKeeper),
NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, opts.FeeCollectorName),
+
// SetPubKeyDecorator must be called before all signature verification decorators
ante.NewSetPubKeyDecorator(opts.AccountKeeper),There was a problem hiding this comment.
and incorporating the above suggestion to pull in upstream DeductFeeDecorator changes:
ante.NewValidateMemoDecorator(opts.AccountKeeper),
ante.NewConsumeGasForTxSizeDecorator(opts.AccountKeeper),
+
+ // We use a custom fee-deduction decorator
+ // to support a dynamically-configurable collector destination.
+ // [cosmos-sdk v0.46.16] NewDeductFeeDecorator(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper, options.TxFeeChecker),
NewInboundDecorator(opts.SwingsetKeeper),
- NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, opts.FeeCollectorName),
+ NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, opts.FeeCollectorName, options.TxFeeChecker),
+
// SetPubKeyDecorator must be called before all signature verification decorators
ante.NewSetPubKeyDecorator(opts.AccountKeeper),| // former ante.NewMempoolFeeDecorator() | ||
| // replaced as in https://github.com/provenance-io/provenance/pull/1016 | ||
| ante.NewDeductFeeDecorator(opts.AccountKeeper, opts.BankKeeper, opts.FeegrantKeeper, nil), | ||
| NewMempoolFeeDecorator(), |
There was a problem hiding this comment.
Reviewing cosmos/cosmos-sdk@d416ee8 more closely, I think we can drop MempoolFeeDecorator. As documented in upstream API Breaking Changes,
The
MempoolFeeDecoratorhas been removed. Instead, theDeductFeeDecoratortakes a new argument of typeTxFeeChecker, to define custom fee models. Ifnilis passed to thisTxFeeCheckerargument, then it will default tocheckTxFeeWithValidatorMinGasPrices, which is the exact same behavior as the oldMempoolFeeDecorator(i.e. checking fees against validator's own min gas price).
So checking fees against validator-local minimums still happens, it just got moved to a checkTxFeeWithValidatorMinGasPrices function in x/auth/ante/validator_tx_fee.go that is the default value of DeductFeeDecorator txFeeChecker. I think we should copy over x/auth/ante/validator_tx_fee.go to our golang/cosmos/ante/ and update our x/auth/ante/fee.go to bring in the changes of upstream cosmos-sdk x/auth/ante/fee.go.
| NewMempoolFeeDecorator(), |
There was a problem hiding this comment.
Yes, as noted above the updated DeductFeeDecorator appears to subsume MempoolFeeDecorator instead of being a 1:1 replacement for it.
| // TODO: Keep in sync with cosmos-sdk/x/auth/ante/fee.go | ||
|
|
||
| // 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, sdkioerrors.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, sdkioerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, requiredFees) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return next(ctx, tx, simulate) | ||
| } | ||
|
|
There was a problem hiding this comment.
| // TODO: Keep in sync with cosmos-sdk/x/auth/ante/fee.go | |
| // 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, sdkioerrors.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, sdkioerrors.Wrapf(sdkerrors.ErrInsufficientFee, "insufficient fees; got: %s required: %s", feeCoins, requiredFees) | |
| } | |
| } | |
| } | |
| return next(ctx, tx, simulate) | |
| } |
There was a problem hiding this comment.
To pull in upstream changes as mentioned above (and note that this includes the cosmos/cosmos-sdk@397119f backport of cosmos/cosmos-sdk#12416 ):
diff
diff --git i/golang/cosmos/ante/fee.go w/golang/cosmos/ante/fee.go
index dcad5b15e..d1cae775a 100644
--- i/golang/cosmos/ante/fee.go
+++ w/golang/cosmos/ante/fee.go
@@ -57,52 +57,93 @@ func (mfd MempoolFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate b
return next(ctx, tx, simulate)
}
-// DeductFeeDecorator deducts fees from the first signer of the tx
+// 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 is a clone of the same type from cosmos-sdk x/auth/ante/fee.go,
+// extended to support a dynamic fee collector destination via `feeCollectorName`.
+// It 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 {
+ // TODO: Rename `ak` to `accountKeeper` for better alignment with cosmos-sdk.
ak AccountKeeper
bankKeeper types.BankKeeper
feegrantKeeper FeegrantKeeper
feeCollectorName string
+ txFeeChecker TxFeeChecker
}
-func NewDeductFeeDecorator(ak AccountKeeper, bk types.BankKeeper, fk FeegrantKeeper, feeCollectorName string) DeductFeeDecorator {
+func NewDeductFeeDecorator(ak AccountKeeper, bk types.BankKeeper, fk FeegrantKeeper, feeCollectorName string, tfc TxFeeChecker) DeductFeeDecorator {
+ if tfc == nil {
+ tfc = checkTxFeeWithValidatorMinGasPrices
+ }
+
return DeductFeeDecorator{
ak: ak,
bankKeeper: bk,
feegrantKeeper: fk,
feeCollectorName: feeCollectorName,
+ txFeeChecker: tfc,
}
}
-func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (newCtx sdk.Context, err error) {
+func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
feeTx, ok := tx.(sdk.FeeTx)
if !ok {
return ctx, sdkioerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx")
}
- if addr := dfd.ak.GetModuleAddress(dfd.feeCollectorName); addr == nil {
- return ctx, fmt.Errorf("fee collector module account (%s) has not been set", dfd.feeCollectorName)
+ if !simulate && ctx.BlockHeight() > 0 && feeTx.GetGas() == 0 {
+ return ctx, sdkioerrors.Wrap(sdkerrors.ErrInvalidGasLimit, "must provide positive gas")
}
+ var (
+ priority int64
+ err error
+ )
+
fee := feeTx.GetFee()
+ if !simulate {
+ 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) checkDeductFee(ctx sdk.Context, sdkTx sdk.Tx, fee sdk.Coins) error {
+ feeTx, ok := sdkTx.(sdk.FeeTx)
+ if !ok {
+ return sdkioerrors.Wrap(sdkerrors.ErrTxDecode, "Tx must be a FeeTx")
+ }
+
+ if addr := dfd.ak.GetModuleAddress(dfd.feeCollectorName); addr == nil {
+ return fmt.Errorf("fee collector module account (%s) has not been set", dfd.feeCollectorName)
+ }
+
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, sdkioerrors.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, sdkioerrors.Wrapf(err, "%s not allowed to pay fees from %s", feeGranter, feePayer)
+ return sdkioerrors.Wrapf(err, "%s does not not allow to pay fees for %s", feeGranter, feePayer)
}
}
@@ -111,23 +152,27 @@ func (dfd DeductFeeDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bo
deductFeesFromAcc := dfd.ak.GetAccount(ctx, deductFeesFrom)
if deductFeesFromAcc == nil {
- return ctx, sdkioerrors.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(), dfd.feeCollectorName)
+ if !fee.IsZero() {
+ err := DeductFees(dfd.bankKeeper, ctx, deductFeesFromAcc, fee, dfd.feeCollectorName)
if err != nil {
- return ctx, err
+ return err
}
}
- events := sdk.Events{sdk.NewEvent(sdk.EventTypeTx,
- sdk.NewAttribute(sdk.AttributeKeyFee, feeTx.GetFee().String()),
- )}
+ events := sdk.Events{
+ sdk.NewEvent(
+ sdk.EventTypeTx,
+ sdk.NewAttribute(sdk.AttributeKeyFee, fee.String()),
+ sdk.NewAttribute(sdk.AttributeKeyFeePayer, deductFeesFrom.String()),
+ ),
+ }
ctx.EventManager().EmitEvents(events)
- return next(ctx, tx, simulate)
+ return nil
}
// DeductFees deducts fees from the given account.| // ante.NewDeductFeeDecorator(ak, bk, fk, nil), | ||
| NewMempoolFeeDecorator(), | ||
| NewDeductFeeDecorator(ak, bk, fk, feeCollector), |
There was a problem hiding this comment.
| // ante.NewDeductFeeDecorator(ak, bk, fk, nil), | |
| NewMempoolFeeDecorator(), | |
| NewDeductFeeDecorator(ak, bk, fk, feeCollector), | |
| NewDeductFeeDecorator(ak, bk, fk, feeCollector, nil), |
| return nil | ||
| } | ||
|
|
||
| func TestDoubleFee(t *testing.T) { |
| // ante.NewDeductFeeDecorator(ak, bk, fk, nil), | ||
| NewMempoolFeeDecorator(), |
There was a problem hiding this comment.
Note on testing. To reproduce the double-fee-deduction in RC0, uncomment line 62 and comment-out line 63.
|
I agree that that we'd at least want to update our As I noted in this morning's meeting, it might be even better to:
|
|
After a conversation with mfig, it looks like we can just do the one-line change to cosmos-sdk to change the fee collector module name. (We'll want to put a comment in our |
|
Superseded by #9051. |
|
Following up on the above comments suggesting a one-line change to rename the fee collector - implemented as agoric-labs/cosmos-sdk#406 but the Cosmos-designated fee collector account has various automatic withdrawals that we don't want, so a one-line fix is not correct. |
closes: #9036
Description
Restore old
MempoolFeeDecorator.This was removed from cosmos-sdk in v0.46. The replacement suggested in the cosmos-sdk 0.46 upgrade guide does not behave the same and led to Txs being double-charged for fees.
Security Considerations
Usual scrutiny.
Scaling Considerations
N/A
Documentation Considerations
N/A - should preserve pre-upgrade-14 behavior.
Testing Considerations
Tests to be included.
Upgrade Considerations
Should preserve previous behavior. We will want to revisit this solution to find an easier way to stay in sync with the evolution of cosmos-sdk.