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

Validate stack items after operations #222

Merged
merged 1 commit into from
Dec 18, 2017
Merged

Conversation

axic
Copy link
Member

@axic axic commented Oct 13, 2017

This helped find some of the bugs already fixed, though it may be useful in the future too.

@axic axic force-pushed the stack-length-checks branch from 462d5ad to 71c341c Compare October 18, 2017 09:22
@axic
Copy link
Member Author

axic commented Oct 18, 2017

This needs to be updated after #219 (or vice versa).

Copy link
Contributor

@jwasinger jwasinger left a comment

Choose a reason for hiding this comment

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

Hey @axic @Silur I've looked at this PR twice trying to figure out what the problem is. I'm thinking perhaps we can let this PR sit because the code it touches will become irrelevant after EWASM is implemented. Thoughts?

@jwasinger jwasinger mentioned this pull request Dec 8, 2017
@axic
Copy link
Member Author

axic commented Dec 8, 2017

I think this will work well after #219 is properly finished and this one rebased.

@axic axic force-pushed the stack-length-checks branch from 35e8ddc to a694a86 Compare December 10, 2017 03:11
@jwasinger
Copy link
Contributor

jwasinger commented Dec 12, 2017

I took another look at this one. Still couldn't figure out what is causing the callback error that we are seeing here.

gotta love async....

@axic
Copy link
Member Author

axic commented Dec 12, 2017

@jwasinger probably missed one of the functions since there were so many rebases over 1.5 years :)

@axic axic force-pushed the stack-length-checks branch 2 times, most recently from 46ded1f to 332b5ca Compare December 12, 2017 05:29
@axic
Copy link
Member Author

axic commented Dec 12, 2017

I think I should have fixed this now.

@hugo-dc
Copy link
Contributor

hugo-dc commented Dec 14, 2017

Hi @axic , @jwasinger ,

I looked at this PR, I found out that SWAP opcode has this configuration:

//         name, baseCost, offStack, onStack, dynamic
0x90: ['SWAP', 3, 0, 0, false],

onStack is 0, swap opcode returns the swaped element https://github.com/ethereumjs/ethereumjs-vm/blob/master/lib/opFns.js#L483 , which has to be pushed again to the stack.

I changed onStack from 0 to 1 for the SWAP opcode, and the failed tests went from 868 to 43.

The remaining failing tests are related to the async opcodes (CALL, CALLCODE, and DELEGATECALL)

@axic
Copy link
Member Author

axic commented Dec 15, 2017

@hugo-dc good find! I think though that swap is broken and it shouldn't return the new top stack item. It should just do the stack swap internally (since we don't have a better code base).

@axic axic force-pushed the stack-length-checks branch from 332b5ca to 6033239 Compare December 17, 2017 21:55
@axic
Copy link
Member Author

axic commented Dec 17, 2017

@hugo-dc @jwasinger rebased after the other fix, this should work now?

@axic axic closed this Dec 17, 2017
@axic axic reopened this Dec 17, 2017
@Silur
Copy link
Contributor

Silur commented Dec 17, 2017

isn't INTERNAL_ERROR is relevant in #219 where there was runState.vmError having a non enum value exception?

@hugo-dc
Copy link
Contributor

hugo-dc commented Dec 18, 2017

@axic I confirm all state tests are passing after the fix 👍

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.

6 participants