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

Tests where data is copied to a memory range longer than the data itself #538

Merged
merged 7 commits into from Nov 9, 2018

Conversation

ajsutton
Copy link
Contributor

@ajsutton ajsutton commented Nov 3, 2018

Adds tests to cover cases where the result of an EVM opcode is written to a specified memory range and the result is shorter than the specified range. This includes:

  • CALL to ecrecover precompile with a completely invalid key
  • CALL to ecrecover precompile with a sensible key but a signature can't be recovered
  • CALL to identity precompile
  • EXTCODECOPY where code being copied is shorter than the target memory range

These cover cases where Pantheon behaviour differed from other clients - initially because EXTCODECOPY did not zero out memory beyond the end of the data and then we fixed that which broke CALL which expects the output length to be treated as a maximum length so remaining memory is not zeroed out. The reference tests don't appear to have any coverage of this category of behaviour.

@ajsutton
Copy link
Contributor Author

ajsutton commented Nov 3, 2018

Note that there are some other opcodes that may need tests like these too (at minimum CODECOPY). Wanted to check this is a sensible approach to this kind of test before going too far.

@holgerd77
Copy link
Contributor

Hi @ajsutton, to follow up the mail exchange: great that you also want to help along the PR to improve documentation!

I have just submitted a PR with a transition of the creation docs to this repository and some initial updates #539, this is nevertheless just a starting point.

Feel free to write initial questions you have directly in the code of the PR, I will try to update the PR accordingly. We can nevertheless also do subsequent PRs once this initial PR is merged.

@holgerd77
Copy link
Contributor

Oh, tests are passing, great! 👍

@ajsutton
Copy link
Contributor Author

ajsutton commented Nov 6, 2018

Yeah I misinterpreted the error message - it was actually telling me that I’d missed refilling one test so just had to do that and I also generated the blockchain variants based on the intructions in the PR you mentioned.

@@ -0,0 +1,60 @@
{
"CallEcrecoverInvalidSignature" : {
"env" : {
Copy link
Collaborator

@winsvega winsvega Nov 8, 2018

Choose a reason for hiding this comment

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

could you please add

"_info" : {
     "comment" : " Test Description "
}

section describing the logic behind LLL code. please?
in other test fillers too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know that was possible but definitely good to have. I've added some, let me know if they explain things well enough.

@winsvega
Copy link
Collaborator

winsvega commented Nov 9, 2018

ready to merge?

@ajsutton
Copy link
Contributor Author

ajsutton commented Nov 9, 2018

Yep I’m happy if you are.

@winsvega winsvega merged commit 6f95752 into ethereum:develop Nov 9, 2018
@holgerd77
Copy link
Contributor

We just did a first test run with your tests added and directly stumbled over, thanks for catching this! 👍

@ajsutton
Copy link
Contributor Author

Well I'm glad it's not just me that got this wrong then. :)

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.

None yet

3 participants