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 MCOPY tests #1229

Merged
merged 1 commit into from
Jun 21, 2023
Merged

Add MCOPY tests #1229

merged 1 commit into from
Jun 21, 2023

Conversation

chfast
Copy link
Member

@chfast chfast commented Jun 19, 2023

The MCOPY test file focus on properly copying memory bytes.
The MCOPY_copy_cost test file focus on properly calculating the MCOPY instruction copy gas cost.
The MCOPY_memory_expansion_cost test file focus on properly calculating potential memory expansion cost in the MCOPY instruction.

@chfast chfast changed the title Add MCOPY memory copy test cases Add MCOPY tests Jun 20, 2023
The MCOPY test file focus on properly copying memory bytes.
The MCOPY_copy_cost test file focus on properly calculating the MCOPY
instruction copy gas cost.
The MCOPY_memory_expansion_cost test file focus on properly calculating
potential memory expansion cost in the MCOPY instruction.
@chfast
Copy link
Member Author

chfast commented Jun 20, 2023

This is ready from my side.

@winsvega winsvega merged commit 5153563 into ethereum:develop Jun 21, 2023
@shemnon
Copy link
Contributor

shemnon commented Jun 21, 2023

Besu passes these tests.

@jochem-brouwer
Copy link
Member

EthereumJS passes these tests.

However, there is a case missing (I think).

We had a bug in the gas calculation where the src > dst, and we are going to read out-of-bounds of current memory. The memory should thus be expanded to src + length. We do not do this, but this is not tested. (I believe this should be the case, right?)

@jochem-brouwer
Copy link
Member

So, what should be done in that test, is perform the exact same MCOPY again, to see if memory is properly expanded or not (now memory expansion costs are 0). In our case, memory expansion costs will not be zero.

@jochem-brouwer
Copy link
Member

Actually, never mind, we do extend, but if uncovered it should maybe be added.

@gumb0 gumb0 deleted the mcopy branch June 22, 2023 14:38
@MarekM25
Copy link

Nethermind passes these tests :)

@chfast
Copy link
Member Author

chfast commented Jun 22, 2023

So, what should be done in that test, is perform the exact same MCOPY again, to see if memory is properly expanded or not (now memory expansion costs are 0). In our case, memory expansion costs will not be zero.

All tests in MCOPY.json and MCOPY_copy_cost.json have memory already expanded so the memory expansion cost in MCOPY should be 0.

However, there is a case missing (I think).

We had a bug in the gas calculation where the src > dst, and we are going to read out-of-bounds of current memory. The memory should thus be expanded to src + length. We do not do this, but this is not tested. (I believe this should be the case, right?)

In MCOPY_memory_expansion_cost.json the following test cases have src > dst:

  • dst0_src31_size706
  • dst31_src62_size706
  • dst0_src64_size1344
  • dst33_src64_size1344
  • dst1_src33_size1344
  • huge_src0_size1
  • huge_dst0_size_n256

However, I'm not sure how effective these are for finding bugs out-of-bound or uninitialized-memory bugs. We can add more cases if you have precise values of MCOPY arguments.

@jochem-brouwer
Copy link
Member

I just saw that MSIZE is stored, but the idea would be to, after storing MSIZE, perform the exact same MCOPY operation again. This would detect a case that internally the MSIZE is set correctly, but somehow when resizing it, it takes the wrong MSIZE (for instance if it did not properly expand the memory with extra zeros). However upon writing this, this seems like a super exotic bug. But you could edit the test code to MCOPY again, to ensure that the gas costs are also correct if you call it again (hence now expansion cost is zero). I'll add a comment, but I doubt this will find bugs.

mcopy(calldataload(0), calldataload(32), calldataload(64))

// Put MSIZE in storage.
sstore(0, msize())
Copy link
Member

Choose a reason for hiding this comment

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

After this line, do

mcopy(calldataload(0), calldataload(32), calldataload(64))

again

Copy link
Member Author

Choose a reason for hiding this comment

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

I will add one more test suite for it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok great 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Added in #1234.

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.

5 participants