osmosis icon indicating copy to clipboard operation
osmosis copied to clipboard

Add 30M gas limit to sudo helper

Open AlpinYukseloglu opened this issue 1 year ago • 4 comments

Closes: #7526

What is the purpose of the change

This PR adds a 30M gas limit to sudo calls that use our helper. This can be tightened in the future by requiring callers to specify a gas limit, but in the meantime should serve as an upperbound to minimize the attack surface from unbounded CW contract calls.

Testing and Verifying

Existing tests related to sudo calls pass, and the implementation uses the same gas limit as Cosmwasm has for queries.

No direct tests for this, as it seems all our CW helpers are not directly tested and setting this up will be a meaningful lift. Happy to table this PR until we complete this if people are concerned. Documented this here: #7528

Documentation and Release Note

  • [ ] Does this pull request introduce a new feature or user-facing behavior changes?
  • [ ] Changelog entry added to Unreleased section of CHANGELOG.md?

Where is the change documented?

  • [ ] Specification (x/{module}/README.md)
  • [ ] Osmosis documentation site
  • [ ] Code comments?
  • [ ] N/A

AlpinYukseloglu avatar Feb 17 '24 23:02 AlpinYukseloglu

I don't think this is complete. If the child thing out of gas's, you get a panic, no? So we have to catch the panic, and return an error.

This definitely needs tests to ensure we got the flow right

ValarDragon avatar Feb 19 '24 06:02 ValarDragon

I think the panic gets caught by the top level defer: https://github.com/osmosis-labs/cosmos-sdk/blob/osmo/v0.47.5/baseapp/baseapp.go#L639-L647

nicolaslara avatar Feb 19 '24 07:02 nicolaslara

Thats not true for beginblock/endblock code!

And more importantly, CW contracts may appear in things that assume a contract they won't panic. (In swaps, we do catch all panics though)

ValarDragon avatar Feb 19 '24 14:02 ValarDragon

ahh, I keep forgetting we're calling this in beginblock/endblock. Yeah, prob good to have the same defer here

nicolaslara avatar Feb 19 '24 14:02 nicolaslara

This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed if no further activity occurs. Thank you!

github-actions[bot] avatar Mar 16 '24 00:03 github-actions[bot]

[!WARNING]

Rate Limit Exceeded

@AlpinYukseloglu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 6 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information.

Commits Files that changed from the base of the PR and between 114ca628b25d4806d2262eb8965aecf4680270e9 and 84abef61f9d0dc431334af3f6eb524bd358a9dcb.

Walkthrough

The recent update in version v23.0.7-iavl-v1 introduces a significant enhancement to the Osmosis blockchain environment, specifically targeting the osmoutils/cosmwasm package. It establishes a gas limit for contract calls within the CW pool, aiming to mitigate the risk of unbounded gas consumption. This is achieved by introducing a default gas limit constant and modifying the Sudo function to enforce this limit, thereby preventing contracts from running indefinitely. Additionally, new tests validate this functionality, ensuring contracts adhere to the specified gas consumption boundaries.

Changes

File(s) Change Summary
CHANGELOG.md Added a 30M gas limit to CW pool contract calls.
osmoutils/cosmwasm/helpers.go Introduced DefaultContractCallGasLimit and modified Sudo to handle gas limits for contract calls.
osmoutils/cosmwasm/helpers_test.go Added tests for gas consumption limits of contract execution in the Osmosis blockchain environment.

Assessment against linked issues

Objective Addressed Explanation
Add gas limit to osmoutils Sudo helper (#7526)

🎉🐇🎉

In a land where contracts run wild and free,
A rabbit set limits, as wise as can be.
"No more," it declared, with a hop and a skip,
"Shall gas run unbound, on this blockchain trip."
With a code and a test, it set the scene,
Ensuring the blockchain stayed efficient and clean.
🎉🐇🎉

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share

Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

coderabbitai[bot] avatar Mar 19 '24 19:03 coderabbitai[bot]

Important Notice

This PR modifies an in-repo Go module. It is one of:

  • osmomath
  • osmoutils
  • x/ibc-hooks
  • x/epochs

The dependent Go modules, especially the root one, will have to be updated to reflect the changes. Failing to do so might cause e2e to fail.

Please follow the instructions below:

  1. Open https://github.com/osmosis-labs/osmosis/actions/workflows/go-mod-auto-bump.yml
  2. Provide the current branch name
  3. On success, confirm if an automated commit corretly updated the go.mod and go.sum files

Please let us know if you need any help.

github-actions[bot] avatar Mar 19 '24 19:03 github-actions[bot]