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

Add gas limit to voice message execution and ACK on out-of-gas. #22

Merged
merged 9 commits into from
Apr 10, 2023

Conversation

0xekez
Copy link
Contributor

@0xekez 0xekez commented Apr 3, 2023

This adds a manually tuned gas limit to voice packet handling so that even if the packet causes an out-of-gas error, the module reserves enough gas to return an ACK informing the caller of the out-of-gas error.

This adds a manually tuned gas limit to voice packet handling so that
even if the packet causes an out-of-gas error, the module reserves
enough gas to return an ACK informing the caller of the out-of-gas error.
Copy link
Member

@JakeHartnell JakeHartnell left a comment

Choose a reason for hiding this comment

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

Nice!

@0xekez
Copy link
Contributor Author

0xekez commented Apr 5, 2023

This is blocked on two things:

  1. At least in the IBC testing framework, gas usage is non-deterministic. See Investigate cause of nondeterminism in gas usage #23 which describes how I was able to write a test to reproduce this.
  2. In response to gas usage being non-dererministic, I want to test this change against a real chain because I am paranoid, but at the moment interchaintest doesn't use the gas limits you specify so I can't instantiate the voice module: Interchaintest v4 does not use gas adjustment from chain spec when executing transaction strangelove-ventures/interchaintest#482

@0xekez
Copy link
Contributor Author

0xekez commented Apr 7, 2023

@Art3miX - lmk what you think of this. i reorganized the tests, set up CI, found a bug, and made some progress reproducing the handshake bug in interchaintest so i'm now pretty sure that's a problem with IBC and not with our handshake process.

@0xekez
Copy link
Contributor Author

0xekez commented Apr 7, 2023

now blocked on more SDK quirks:tm: cosmos/ibc-go#3428

Copy link
Collaborator

@Art3miX Art3miX left a comment

Choose a reason for hiding this comment

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

Awesome as always, few small nits

justfile Show resolved Hide resolved
justfile Show resolved Hide resolved
tests/polytone-tester/src/msg.rs Show resolved Hide resolved
tests/simtests/functionality_test.go Outdated Show resolved Hide resolved
tests/simtests/functionality_test.go Show resolved Hide resolved
tests/simtests/functionality_test.go Show resolved Hide resolved
tests/strangelove/types.go Show resolved Hide resolved
tests/strangelove/incompatible_handshake_test.go Outdated Show resolved Hide resolved
0xekez and others added 4 commits April 10, 2023 11:34
Co-authored-by: Art3miX <40179351+Art3miX@users.noreply.github.com>
Co-authored-by: Art3miX <40179351+Art3miX@users.noreply.github.com>
@0xekez 0xekez merged commit d4b1793 into main Apr 10, 2023
4 of 5 checks passed
@0xekez 0xekez mentioned this pull request Apr 10, 2023
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.

Investigate gas limits for proxy message execution
3 participants