Skip to content
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: revert tx when block gas limit exceeded (backport: #10770) #10814

Merged
merged 4 commits into from
Jan 12, 2022

Conversation

yihuang
Copy link
Collaborator

@yihuang yihuang commented Dec 20, 2021

Description

backport from #10770, but used a different solution from the master one, because of difference in code structure.


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 in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • 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 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)

@yihuang yihuang changed the title revert tx when block gas limit exceeded (backport: #10770) fix: revert tx when block gas limit exceeded (backport: #10770) Dec 20, 2021
@tomtau tomtau mentioned this pull request Jan 3, 2022
20 tasks
@amaury1093 amaury1093 marked this pull request as draft January 3, 2022 13:35
@amaury1093
Copy link
Contributor

oh nice, thanks a lot. I'm putting in draft for now, let's merge #10770 first.

@yihuang
Copy link
Collaborator Author

yihuang commented Jan 4, 2022

I guess we better write a unit test for this too, but I find it not easy to do that? which test suite is the best place to write a test case for this?

@@ -586,11 +586,6 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
return gInfo, nil, nil, sdkerrors.Wrap(sdkerrors.ErrOutOfGas, "no block gas left to run tx")
}

var startingGas uint64
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to check overflow since it's already handled in ConsumeGas.

@yihuang yihuang marked this pull request as ready for review January 11, 2022 10:29
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems correct to me, @yihuang did you think of how we can write a test for this?

Edit: maybe in TestMaxBlockGasLimits in baseapp_test?

Comment on lines 614 to 616
defer func() {
if mode == runTxModeDeliver {
ctx.BlockGasMeter().ConsumeGas(
ctx.GasMeter().GasConsumedToLimit(), "block gas meter",
)

if ctx.BlockGasMeter().GasConsumed() < startingGas {
panic(sdk.ErrorGasOverflow{Descriptor: "tx gas summation"})
}
consumeBlockGas()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we remove this defer func?

AFAIK, there's no gas consumption after runMsgs. So if we put consumeBlockGas in runTx, before store write, then it should be okay?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need defer function to consume block gas when panic happens I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let's maybe keep it for now

@yihuang
Copy link
Collaborator Author

yihuang commented Jan 11, 2022

this seems correct to me, @yihuang did you think of how we can write a test for this?

Edit: maybe in TestMaxBlockGasLimits in baseapp_test?

I tried before, find it very tedious to set up the environment, let me try it again.

@yihuang
Copy link
Collaborator Author

yihuang commented Jan 11, 2022

this seems correct to me, @yihuang did you think of how we can write a test for this?

Edit: maybe in TestMaxBlockGasLimits in baseapp_test?

We can't do that in baseapp_test, because baseapp module is imported by keeper, if we setup keepers, it'll cause cyclic import. we need to use sth like SimApp.

@yihuang
Copy link
Collaborator Author

yihuang commented Jan 11, 2022

this seems correct to me, @yihuang did you think of how we can write a test for this?

Edit: maybe in TestMaxBlockGasLimits in baseapp_test?

tests added.

Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks a lot

Let's get this released today 😎

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK, left few tiny comments.

baseapp/baseapp.go Outdated Show resolved Hide resolved
baseapp/baseapp.go Outdated Show resolved Hide resolved
@@ -677,6 +677,9 @@ 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 {
// When block gas exceeds, it'll panic and won't commit the cached store.
consumeBlockGas()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to call it here? It's already called in defer (line 614)

Copy link
Collaborator Author

@yihuang yihuang Jan 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to call it here? It's already called in defer (line 614)

That's the main purpose of this PR, call it here so if block gas limit exceeded, it'll panic and won't execute the msCache.Write(), so the tx msg side effects are not persisted.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes sense. Thanks!

yihuang and others added 2 commits January 12, 2022 21:22
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
@amaury1093
Copy link
Contributor

Merging!

@amaury1093 amaury1093 merged commit ba1e099 into cosmos:release/v0.45.x Jan 12, 2022
@yihuang yihuang deleted the block-gas-0.45 branch January 12, 2022 15:28
@faddat faddat mentioned this pull request Feb 28, 2022
8 tasks
zakir-code pushed a commit to PundiAI/cosmos-sdk that referenced this pull request Apr 8, 2022
…cosmos#10814)

* revert tx when block gas limit exceeded (backport: cosmos#10770)

- used a different solution.

changelog

* add unit test

* Update baseapp/baseapp.go

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* simplfy the defer function

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
JimLarson pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Jul 7, 2022
…cosmos#10814)

* revert tx when block gas limit exceeded (backport: cosmos#10770)

- used a different solution.

changelog

* add unit test

* Update baseapp/baseapp.go

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* simplfy the defer function

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Eengineer1 pushed a commit to cheqd/cosmos-sdk that referenced this pull request Aug 26, 2022
…cosmos#10814)

* revert tx when block gas limit exceeded (backport: cosmos#10770)

- used a different solution.

changelog

* add unit test

* Update baseapp/baseapp.go

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* simplfy the defer function

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
…cosmos#10814)

* revert tx when block gas limit exceeded (backport: cosmos#10770)

- used a different solution.

changelog

* add unit test

* Update baseapp/baseapp.go

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

* simplfy the defer function

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants