-
Notifications
You must be signed in to change notification settings - Fork 373
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: hardcode max vm cycles in keeper #1807
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1807 +/- ##
==========================================
+ Coverage 47.49% 47.50% +0.01%
==========================================
Files 388 388
Lines 61311 61345 +34
==========================================
+ Hits 29117 29144 +27
- Misses 29756 29761 +5
- Partials 2438 2440 +2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure about these changes.
If you execute these on Gno.land, these will fail, I'm pretty sure, as you'd get "cpu cycle overrun" already. So these fail in gno run
and gno test
, which from this PR would be excluded with the new limit.
The way I see it, we should have a limit on the amount of frames that can be created (ie. a stack overflow error), which is for both local testing and the blockchain context, but nothing more than the existing CPU overrun checks for infinte for loops. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, the bottom of infinite recursion/loop should be when there is no more gas. That means that the check needs to be added in the VM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 💯
Please see @thehowl's suggestions, otherwise good to go
also we need to remove the code that configs VM parameters from command flag. |
This will be the fallback value if, for some reason, something prevents the machine from halting execution when the gas runs out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This hardcodes the maximum VM cycles in the keeper to ten million, the same that is currently being used from genesis. It doesn't seem like there is a reason why we should want people to be able to adjust this.
Contributors' checklist...
BREAKING CHANGE: xxx
message was included in the description