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

Avoid double-callbacks in CALL #188

Merged
merged 1 commit into from
Sep 13, 2017
Merged

Avoid double-callbacks in CALL #188

merged 1 commit into from
Sep 13, 2017

Conversation

axic
Copy link
Member

@axic axic commented Aug 20, 2017

This fixes test case Call50000_identity.

@axic
Copy link
Member Author

axic commented Aug 21, 2017

Explanation: the actual call is moved into the successful-case of accountIsEmpty. The way to trigger two callbacks is if the account is empty and thus newAccountGas applies, but deducting that triggers an OOG. In this case the actual call still happens resulting in a double callback.

@axic
Copy link
Member Author

axic commented Aug 21, 2017

This change also enables turning back on a lot of tests, the skip list would need to be reduced to

const skip = [
  'CALLCODE_Bounds', // Uses >270Mb of memory
  'CALL_Bounds', // Uses >270Mb of memory
  'CREATE_Bounds', // Uses >270Mb of memory
  'DELEGATECALL_Bounds', // Uses >270Mb of memory
  'mload32bitBound_return2', // Uses too much memory
  'RevertDepthCreateAddressCollision', // test case is wrong
]

I believe those memory ones could be fixed by #174.

@axic axic force-pushed the double-callback branch from 0d90026 to f59c52e Compare August 21, 2017 10:22
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Looks good to me. This looks generally cleaner to me having the code execution in a deterministic order.

@axic axic force-pushed the double-callback branch from f59c52e to a51546e Compare August 22, 2017 23:21
@axic
Copy link
Member Author

axic commented Sep 13, 2017

@jwasinger @holgerd77 @cdetrio anybody merging this? :)

@holgerd77
Copy link
Member

@axic I though that there is an in progress label attached, therefore I waited, has this some other meaning than the [WIP] pre-string?

@axic
Copy link
Member Author

axic commented Sep 13, 2017

@holgerd77 I don't know how that label ended up there, but no, this is finished.

@axic axic removed the in progress label Sep 13, 2017
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Can you rebase this?

@axic
Copy link
Member Author

axic commented Sep 13, 2017

@holgerd77 done

Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Looks good

@holgerd77 holgerd77 merged commit c85ab76 into master Sep 13, 2017
@axic axic deleted the double-callback branch September 13, 2017 09:46
evertonfraga pushed a commit to evertonfraga/ethereumjs-vm that referenced this pull request Feb 4, 2020
holgerd77 added a commit that referenced this pull request Dec 1, 2020
Early-stage Programmatic API / Custom VM Option
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants