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

Port `check_for_impurity.se` to Vyper LLL #143

Merged
merged 19 commits into from Jun 14, 2018

Conversation

Projects
None yet
5 participants
@ralexstokes
Contributor

ralexstokes commented May 14, 2018

Fixes #69.

Work in progress for @gitcoinbot.

Ports https://github.com/ethereum/research/blob/master/impurity/check_for_impurity.se from the deprecated Serpent to Vyper LLL.

I'm placing it under the test utils for now as a separate file. I'll check w/ repo maintainers to see if they want to keep the source here or just include the contract creation transaction in conftest.py

@ralexstokes

This comment has been minimized.

Contributor

ralexstokes commented May 14, 2018

For anyone following along, the next step is to manually compile the repeat form as the Vyper compiler requires a static number of rounds in the loop (that ranges over the valcode bytecode) with this form. There should not be a restriction on the next lower-level form. Then I will add some tests to confirm behavior and then can finally integrate with the rest of this repo.

@djrtwo

This comment has been minimized.

Contributor

djrtwo commented May 15, 2018

We might want to put it in /contracts even though it is a .py file.
Thoughts @ChihChengLiang ?

@ChihChengLiang

This comment has been minimized.

Contributor

ChihChengLiang commented May 15, 2018

agree that this file should go to /contracts

ralexstokes added some commits May 16, 2018

Use static upper-bound for valid `repeat` form
Vyper doesn't allow the number of rounds in the `repeat` form to be dynamic.

This commit instead just ranges over the maximum size of any contract `2**256`
and simply breaks out of the loop if we run past the current contract's bytecode
@ralexstokes

This comment has been minimized.

Contributor

ralexstokes commented May 26, 2018

Ok, I need to sort out some issue that arose after rebasing master and trying to run the tests but as long as I verify I don't need to update anything then the purity_checker is ready to go.

Once I'm ready I'll remove the [WIP] tag and submit this PR.

@ralexstokes ralexstokes deleted the ralexstokes:migrate-purity-checker branch May 29, 2018

@ralexstokes ralexstokes restored the ralexstokes:migrate-purity-checker branch May 29, 2018

@ralexstokes ralexstokes reopened this May 29, 2018

@ralexstokes ralexstokes changed the title from [WIP] Port `check_for_impurity.se` to Vyper LLL to Port `check_for_impurity.se` to Vyper LLL May 29, 2018

@ralexstokes

This comment has been minimized.

Contributor

ralexstokes commented May 29, 2018

Ok, I've integrated w/ the latest testing updates. This is ready to go!

@paulhauner

This comment has been minimized.

Contributor

paulhauner commented Jun 3, 2018

Hey, I've been having a look at this and it's epic. Nice work @ralexstokes.

I started making some notes, they might be useful for newcomers trying to understand the LLL. I'll turn it into a blog post once the code settles. If you've got feedback (or solutions to my TODOs) I'd be keen to hear them in the comments of that gist. They're incomplete, but I post them prematurely in the hope they might save someone some time.

https://gist.github.com/paulhauner/f9915100e80305ba81ebd977b88c616c

I was asked have a look at this as I wrote the existing tests for the purity stuff. I spent the last several hours nutting out the LLL, I'll come back with any comments tomorrow 😄

@ChihChengLiang

This comment has been minimized.

Contributor

ChihChengLiang commented Jun 4, 2018

Thanks to @paulhauner, your post is a lifesaver that really helps the review. And really awesome work @ralexstokes

@ralexstokes

This comment has been minimized.

Contributor

ralexstokes commented Jun 4, 2018

@paulhauner yes thanks for that! i'll comment w/ answers now -- great summary of the code which is admittedly opaque :D

@ralexstokes

This comment has been minimized.

Contributor

ralexstokes commented Jun 4, 2018

@paulhauner awesome summary! I made some edits you can see here: https://gist.github.com/ralexstokes/2c0f91c7cb7543656a055b6577d82297/revisions

Let me know if you have any further questions -- there are a lot of things that go into the code that aren't clear from just looking at it. A lot of this stuff was just me looking at the Vyper or Serpent source code and porting the existing idioms over (like the array implementation).

@paulhauner

This comment has been minimized.

Contributor

paulhauner commented Jun 5, 2018

Awesome, thanks @ralexstokes! I have merged your updates into my Gist. I will format this into a tidy blog post in the near future.

I'll use this info to finish my review then leave any comments here 😄

@djrtwo

This comment has been minimized.

Contributor

djrtwo commented Jun 5, 2018

Reading through the code and the summary. Excellent work @ralexstokes and @paulhauner. When I originally put up this issue, I did not quite comprehend the task that I was asking.. 😬

This is killer.

Sorry @ralexstokes that it's taken a while for us to review, but as you know it's complicated and we want to give it the review it deserves.

@paulhauner

This comment has been minimized.

Contributor

paulhauner commented Jun 7, 2018

Hey all,
I've had a look over the LLL and the tests. I am still impressed! I just have a comment about the tests in the context of this repo.

There are two sets of tests:

  1. test_purity_checker.py: tests the purity checker in isolation
  2. test_valcode_purity.py: tests the purity checker when used by deposit().

Given that both sets of tests have the same outcome (assuming valid deposit() params) it makes sense to me that the test params are unified and applied to the purity checker both in isolation and in implementation.

There are cases where test_valcode_purity.py tests things that test_purity_checker.py does not (e.g., list of invalid opcodes) and vice-versa (e.g., externally-owned address) , so unification would "kill two birds with one stone" (for lack of a less violent metaphor).

I can see the test structure you have added @ralexstokes is more sophisticated in some aspects than the structure currently used in test_valcode_purity.py, so if unification is desired I would be happy to unify these two structures, ensuring nothing is lost from the tests you have written. I wrote the existing structure so I am familiar with it already.

I also have a few minor points/queries about the purity checker operation (which are inherited from the original serpent version), but I think they may extend beyond the scope of this pull request, which I understand to be "port the existing purity checker to LLL", not "produce an ideal LLL purity checker". If we're aiming for the latter, let me know.

Keen to hear thoughts from the official reviewers. 😄

@ralexstokes

This comment has been minimized.

Contributor

ralexstokes commented Jun 7, 2018

great @paulhauner !

Yeah I started developing the new tests in isolation from this repo and then later merged them in and it seemed different enough at the time to keep them separate. I see the test_purity_checker tests as "unit tests" for the contract itself and then the test_valcode_purity tests as more "integration tests" that are end-to-end, in some sense. But I'm also open to merging the tests however makes the most sense!

And I'm happy to go over the remaining points although I'm guessing they get to some of the unresolved questions around what direction this thing should take. Would love to hear everyone's thoughts.

@paulhauner

This comment has been minimized.

Contributor

paulhauner commented Jun 12, 2018

I was getting pretty deep into this thing over the long weekend. It stood up to all the attack vectors I could come up with during that time.

One thing I did notice is that it seems to rely on an implicit assumption that the 0x0 address will always be impure. This is also the case in the Serpent version.

I gather this from lines 70, 71: If the penultimate opcode (relative to the call-type opcode) does not have args, then an index_pushargs() call on that opcode will return 0. Because of this, 0 will be assumed to be the address supplied to the call and then tested against the approved_addrs map, where it should fail. (TL;DR: address defaults to zero in some cases.)

This seems like a pretty reasonable assumption to me. Vitalik wrote the original Serpent version so I guess it seemed reasonable to him too. I just wanted to bring it up for the sake of information synchronicity. ☺️

@vs77bb

This comment has been minimized.

vs77bb commented Jun 13, 2018

@ralexstokes Great work here! Does this make sense to pay out the Gitcoin bounty at this time, @djrtwo? Whenever it does free to submit work here and use the Gitcoin faucet for a bit of ETH if needed 🙂

@djrtwo

This comment has been minimized.

Contributor

djrtwo commented Jun 14, 2018

Finally reviewed code and @paulhauner's doc. Doing the merge. Thanks everyone!

@djrtwo djrtwo merged commit 89dd98f into ethereum:master Jun 14, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@djrtwo

This comment has been minimized.

Contributor

djrtwo commented Jun 14, 2018

How do I submit work?
I think @gdipri01 has to do it because he funded it.

@ralexstokes ralexstokes deleted the ralexstokes:migrate-purity-checker branch Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment