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

Remove unnecessary memory size check in JIT validator #1928

Closed
wants to merge 1 commit into from

Conversation

PlasmaPower
Copy link
Collaborator

We don't check memory bounds anywhere else because we don't need to. Panicking on out-of-bounds memory access is perfectly fine here, because it'll just be returned as an error to the validation service via the socket closing (and the panic will be printed to stderr).

@PlasmaPower PlasmaPower requested a review from tsahee October 17, 2023 04:57
@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Oct 17, 2023
@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #1928 (57cebd8) into master (7c8eae8) will decrease coverage by 0.03%.
Report is 3 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1928      +/-   ##
==========================================
- Coverage   28.98%   28.96%   -0.03%     
==========================================
  Files         220      220              
  Lines       32962    32962              
==========================================
- Hits         9554     9547       -7     
- Misses      22345    22352       +7     
  Partials     1063     1063              

Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

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

This change LGTM - however, probably better to do it in stylus ant not here

@PlasmaPower
Copy link
Collaborator Author

Good call. Moved to OffchainLabs/stylus#171

@PlasmaPower PlasmaPower deleted the remove-useless-memory-check branch October 17, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants