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 Out Of Memory Test #94

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Out Of Memory Test #94

wants to merge 1 commit into from

Conversation

hayesgm
Copy link
Contributor

@hayesgm hayesgm commented Nov 22, 2023

This test looks to be failing with "MemoryLimitOOG," which seems like it's out of gas based on the memory limit, which seems about as close to the solution here as we're going to get. Since Forge isn't exactly the true EVM, there will always be minor discrepencies, but that's making direct reference to the memory limit.

You can see the exact error code with:

forge test --match-test "testRevertsOutOfMemory" -vvvv

This test looks to be failing with "MemoryLimitOOG," which seems like it's out of gas based on the memory limit, which seems about as close to the solution here as we're going to get. Since Forge isn't exactly the true EVM, there will always be minor discrepencies, but that's making direct reference to the memory limit.
Copy link
Collaborator

@kevincheng96 kevincheng96 left a comment

Choose a reason for hiding this comment

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

Oh nice! Wonder why mstore works but mload doesn't. Shouldn't they both expand memory to the same offset?

@hayesgm
Copy link
Contributor Author

hayesgm commented Nov 22, 2023

Oh nice! Wonder why mstore works but mload doesn't. Shouldn't they both expand memory to the same offset?

If I had to guess, it's getting compiled out by the yul optimizer? And it's probably a lot harder to optimize out a write than a read...

@cwang25
Copy link
Contributor

cwang25 commented Dec 19, 2023

Trying to close some old PRs that's < #100
And saw this... have we ever made out of memory test in to our test suites?

@kevincheng96
Copy link
Collaborator

Think we still need to merge this.

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.

4 participants