-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: use full gas on overflow #10897
Changes from all commits
dce994a
682dea1
a3d2771
838b318
45f483c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this doesn't cancel the gas consumption, but cancels other execution. So we will charge a max possible amount of gas. |
||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need to panic - let's use errors! |
||
} | ||
|
||
fee := feeTx.GetFee() | ||
|
@@ -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) | ||
} | ||
} | ||
|
||
|
@@ -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 | ||
|
@@ -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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's good catch, so when overflow, it'll consume less from block gas meter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, though this is super unlikely - requires a weird app config.