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(v7)!: check transactions succeed after they're included in a block #1087

Merged
merged 1 commit into from
Apr 24, 2024

Conversation

fastfadingviolets
Copy link
Contributor

@fastfadingviolets fastfadingviolets commented Apr 22, 2024

Summary

Sometimes a transaction returns code 0 initially, but then fails when a validator goes to actually include it in a block. This PR adds code to cosmos' ExecTx to make sure transactions are verified after their inclusion in a block; if they fail at any stage of proceedings, an error is returned.

Copy link

vercel bot commented Apr 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
interchaintest-docs ⬜️ Ignored (Inspect) Visit Preview Apr 24, 2024 3:01pm

@Reecepbcups
Copy link
Member

Reecepbcups commented Apr 22, 2024

Yea this is an SDK thing. It always returns code 0 so long as it passes message Validate(), but then once included is when the real failure can occur (like gas, etc)

I do worry that some users may want a tx which is successful on submit but fail upstream (I have this exact case checked with proof of authority). I need to think on this more

@Reecepbcups Reecepbcups self-requested a review April 22, 2024 19:01
@fastfadingviolets
Copy link
Contributor Author

hmm, i think we should maybe introduce different error types then; that way you get an err back and you can make sure it's of type UpstreamFailure or whatever

@Reecepbcups Reecepbcups changed the title Fix v7: check transactions succeed after they're included in a block fix(v7): check transactions succeed after they're included in a block Apr 23, 2024
@Reecepbcups
Copy link
Member

Thinking about it, I think it makes sense to for ExecTx. If someone wants to bypass they can use Exec / ExecBin to ignore

@Reecepbcups Reecepbcups added the BACKPORT backport into all maintained branches label Apr 23, 2024
@Reecepbcups Reecepbcups changed the title fix(v7): check transactions succeed after they're included in a block fix(v7)!: check transactions succeed after they're included in a block Apr 23, 2024
@Reecepbcups
Copy link
Member

Reecepbcups commented Apr 23, 2024

@fastfadingviolets This will need the latest v7 branch merged in on your side, then it will be good to go (does not seem I have maintainer edit perms to this PR)

Ill also port to v8 / main as well

@Reecepbcups Reecepbcups enabled auto-merge (squash) April 23, 2024 21:54
auto-merge was automatically disabled April 24, 2024 15:01

Head branch was pushed to by a user without write access

@Reecepbcups Reecepbcups merged commit a3d5c18 into strangelove-ventures:v7 Apr 24, 2024
13 checks passed
@Reecepbcups
Copy link
Member

Ty!

mergify bot pushed a commit that referenced this pull request Apr 24, 2024
…1087)

(cherry picked from commit a3d5c18)

# Conflicts:
#	examples/cosmos/chain_miscellaneous_test.go
Reecepbcups added a commit that referenced this pull request Apr 25, 2024
…ey're included in a block (#1095)

* Fix v7: check transactions succeed after they're included in a block (#1087)

(cherry picked from commit a3d5c18)

# Conflicts:
#	examples/cosmos/chain_miscellaneous_test.go

* merge conflicts (incoming)

---------

Co-authored-by: violet <[email protected]>
Co-authored-by: Reece Williams <[email protected]>
Co-authored-by: Reece Williams <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BACKPORT backport into all maintained branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants