-
Notifications
You must be signed in to change notification settings - Fork 11
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
State Breaking: Backport fix to prevent failed transactions from commiting state when block gas limit is reached #21
State Breaking: Backport fix to prevent failed transactions from commiting state when block gas limit is reached #21
Conversation
that fail do to block gas limits from committing state
}() | ||
} | ||
if mode == runTxModeDeliver { | ||
defer consumeBlockGas() |
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.
This defer behavior is unchanged and still runs if we do not make it to runMessages for a transaction
if mode == runTxModeDeliver { | ||
blockGasConsumed := false | ||
consumeBlockGas := func() { | ||
if !blockGasConsumed { |
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.
This check ensures block gas is only consumed once
@@ -678,6 +683,8 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re | |||
// Result if any single message fails or does not have a registered Handler. | |||
result, err = app.runMsgs(runMsgCtx, msgs, mode) | |||
if err == nil && mode == runTxModeDeliver { | |||
consumeBlockGas() |
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.
This is the main fix -- gas is consumed before calling msCache.Write()
, ensuring a panic is thrown when if the transaction consumes more than the block limit
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.
Fix looks good, stepped through tests locally as well.
Based off of @yihuang's work in cosmos#10814.