Skip to content

Commit

Permalink
fix: use full gas on overflow (#10897)
Browse files Browse the repository at this point in the history
## Description

Investigating missing gas consumption

---

### Author Checklist

*All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.*

I have...

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

### Reviewers Checklist

*All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.*

I have...

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
  • Loading branch information
robert-zaremba authored Jan 7, 2022
1 parent 8a75b95 commit b0d3ef9
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 9 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
ibc-denom.
* [\#10593](https://github.com/cosmos/cosmos-sdk/pull/10593) Update swagger-ui to v4.1.0 to fix xss vulnerability.
* [\#10674](https://github.com/cosmos/cosmos-sdk/pull/10674) Fix issue with `Error.Wrap` and `Error.Wrapf` usage with `errors.Is`.
* [\#10897](https://github.com/cosmos/cosmos-sdk/pull/10897) Fix: set a non-zero value on gas overflow.

### State Machine Breaking

Expand Down
2 changes: 1 addition & 1 deletion store/types/gas.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,9 @@ func addUint64Overflow(a, b uint64) (uint64, bool) {
// ConsumeGas adds the given amount of gas to the gas consumed and panics if it overflows the limit or out of gas.
func (g *basicGasMeter) ConsumeGas(amount Gas, descriptor string) {
var overflow bool
// TODO: Should we set the consumed field after overflow checking?
g.consumed, overflow = addUint64Overflow(g.consumed, amount)
if overflow {
g.consumed = math.MaxUint64
panic(ErrorGasOverflow{descriptor})
}

Expand Down
16 changes: 8 additions & 8 deletions x/auth/middleware/fee.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ func (dfd deductFeeTxHandler) checkDeductFee(ctx context.Context, sdkTx sdk.Tx)
}

if addr := dfd.accountKeeper.GetModuleAddress(types.FeeCollectorName); addr == nil {
panic(fmt.Sprintf("%s module account has not been set", types.FeeCollectorName))
return fmt.Errorf("Fee collector module account (%s) has not been set", types.FeeCollectorName)
}

fee := feeTx.GetFee()
Expand All @@ -123,12 +123,11 @@ func (dfd deductFeeTxHandler) checkDeductFee(ctx context.Context, sdkTx sdk.Tx)
// this works with only when feegrant enabled.
if feeGranter != nil {
if dfd.feegrantKeeper == nil {
return 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(sdkCtx, feeGranter, feePayer, fee, sdkTx.GetMsgs())

if err != nil {
return 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)
}
}

Expand All @@ -137,7 +136,7 @@ func (dfd deductFeeTxHandler) checkDeductFee(ctx context.Context, sdkTx sdk.Tx)

deductFeesFromAcc := dfd.accountKeeper.GetAccount(sdkCtx, deductFeesFrom)
if deductFeesFromAcc == nil {
return 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
Expand Down Expand Up @@ -182,15 +181,16 @@ func (dfd deductFeeTxHandler) SimulateTx(ctx context.Context, req tx.Request) (t
return dfd.next.SimulateTx(ctx, req)
}

// DeductFees deducts fees from the given account.
// Deprecated: DeductFees deducts fees from the given account.
// This function will be private in the next release.
func DeductFees(bankKeeper types.BankKeeper, ctx sdk.Context, acc types.AccountI, fees sdk.Coins) error {
if !fees.IsValid() {
return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFee, "invalid fee amount: %s", fees)
return sdkerrors.ErrInsufficientFee.Wrapf("invalid fee amount: %s", fees)
}

err := bankKeeper.SendCoinsFromAccountToModule(ctx, acc.GetAddress(), types.FeeCollectorName, fees)
if err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInsufficientFunds, err.Error())
return sdkerrors.ErrInsufficientFunds.Wrap(err.Error())
}

return nil
Expand Down
2 changes: 2 additions & 0 deletions x/auth/middleware/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ func NewDefaultTxHandler(options TxHandlerOptions) (tx.Handler, error) {
TxTimeoutHeightMiddleware,
ValidateMemoMiddleware(options.AccountKeeper),
ConsumeTxSizeGasMiddleware(options.AccountKeeper),
// No gas should be consumed in any middleware above in a "post" handler part. See
// ComposeMiddlewares godoc for details.
DeductFeeMiddleware(options.AccountKeeper, options.BankKeeper, options.FeegrantKeeper),
TxPriorityMiddleware,
SetPubKeyMiddleware(options.AccountKeeper),
Expand Down

0 comments on commit b0d3ef9

Please sign in to comment.