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

tx committed successfully but return failure result #10769

Closed
4 tasks
yihuang opened this issue Dec 14, 2021 · 3 comments · Fixed by #10770
Closed
4 tasks

tx committed successfully but return failure result #10769

yihuang opened this issue Dec 14, 2021 · 3 comments · Fixed by #10770
Assignees

Comments

@yihuang
Copy link
Collaborator

yihuang commented Dec 14, 2021

When the last tx in a block exceeds the block gas limit, it returns an error result, but the tx is actually committed successfully.

Summary of Bug

Version

Steps to Reproduce


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@amaury1093
Copy link
Contributor

@yihuang did you check if this is present on 0.42 and 0.44 too? Or it this related only to the middleware refactor (#9996)?

@tomtau
Copy link
Contributor

tomtau commented Dec 15, 2021

@tomtau
Copy link
Contributor

tomtau commented Dec 15, 2021

a write up from @colin-axner:

Summary of Bug

A tx being executed which causes the block gas limit to be reached will be persisted, but its response information will be wiped and replaced with a code 11 error: out of gas in location: block gas meter; gasWanted: 0, gasUsed: 224156: out of gas

No events are emitted.

How the bug was found

An issue was reported of a IBC packet being relayed but no events existing for that packet sequence. Our state information indicated that the packet had been relayed. No associated transaction could be found by querying for events.

After debugging the issue, the block the packet was relayed in was determined and the associated tx was found. The transaction had caused state changes, but all events were wiped from the response. The response might look something like:

gaiad q tx --type=hash C1B2FB06C8FA42E36D4C1147B2E7C8BFDA74A960CE245E917339EAC1F2D5D47C --node http://localhost:27000
code: 11
codespace: sdk
data: ""
gas_used: "224156"
gas_wanted: "0"
height: "2"
info: ""
logs: []
raw_log: 'out of gas in location: block gas meter; gasWanted: 0, gasUsed: 224156:
  out of gas'

I confirmed this issue by replicating my suspicions. I created 2 chains and set 1 chain to have a max block gas of 1. I sent a create client IBC tx and got the expected response. The client had been created, but no events existed in state for the transaction.

Version

v0.44.3

Steps to Reproduce

  1. Construct a chain with max_gas genesis parameter set to 1
  2. Submit any tx that is expected to succeed (failing txs untested). Note the error message and the persisted state changes.

Expected Behaviour

The tx should either by rejected (and the state changes discarded) or the tx should be accepted and the full response returned.

yihuang added a commit to yihuang/cosmos-sdk that referenced this issue Dec 16, 2021
Closes: cosmos#10769
Solution:
- create `PersistTxMiddleware`
- tx state change is reverted if any error happens

Update x/auth/middleware/recovery.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

snapshot middleware

SimulateTx
yihuang added a commit to yihuang/cosmos-sdk that referenced this issue Jan 8, 2022
Closes: cosmos#10769
Solution:
- create a `WithBranchedStore` to handle state snapshot and revert
- extract `ConsumeBlockGasMiddleware` out from `RecoveryTxMiddleware`.
- order the middlewares properly.

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>

Apply suggestions from code review

Co-authored-by: Robert Zaremba <robert@zaremba.ch>
@mergify mergify bot closed this as completed in #10770 Jan 11, 2022
mergify bot pushed a commit that referenced this issue Jan 11, 2022
Closes: #10769

## Description

Solution:
- create a `WithBranchedStore ` to handle state snapshot and revert
- extract `ConsumeBlockGasMiddleware ` out from `RecoveryTxMiddleware`.
- order the middlewares properly.





---

### 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants