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

Check access restrictions during tx execution #276

Closed

Conversation

jannikluhn
Copy link
Contributor

@jannikluhn jannikluhn commented Jan 16, 2018

What was wrong?

#263

How was it fixed?

  • Fix Computation.state_db
  • Convert transaction.access_list to prefix list form during initialization and store the result in transaction.prefix_list.
  • Whenever a Computation object is available (i.e. in all opcode functions) use computation.state_db instead of computation.vm_state.state_db to access state.
  • In all other places use vm.state.state_db but pass transaction.prefix_list explicitly.

Cute Animal Picture

put a cute animal picture link inside the parentheses

jannikluhn and others added 30 commits January 4, 2018 12:06
Also don't convert read/write list format in message constructor -- this
should happen only once right after transaction decoding.
1. Set `reads` and `writes` properties for accessing `_reads` and `writes`
2. Modified the `exsits` and `deletes` to store the accessed key-value result in dicts
This operates in a O(n*logn) time complexity because of the sort.

Closes ethereum#212
Encode integers as big endian and code as bytes, instead both as RLP
two layer trie backend -> nested trie backend (storage tries are
referenced in account trie, so we have a nested structure)
one layer trie backend -> flat trie backend (just one trie, no
additional structure)
The methods are
- account_exists
- delete_account
- delete_storage

Those would require additional trie methods, but are likely not needed
anyway, so they are explicitly NotImplemented for now.
@hwwhww
Copy link
Contributor

hwwhww commented Jan 17, 2018

@jannikluhn oh, sad. #269 also added some new test cases, just wondering if we merge #269 first, could help for reducing conflicts for this PR?

@jannikluhn
Copy link
Contributor Author

Looks like the tests in #269 would run into the same problem if this PR (#276) is merged first. So I'd vote to merge #269 first, then I can fix these issues here (specifying correct access lists and applying @xfail's).

@NIC619
Copy link
Contributor

NIC619 commented Jan 17, 2018

I will get the #269 merged as soon as tests passed.

@jannikluhn
Copy link
Contributor Author

Rebased and added access lists to the tests of #269 . Also marked them with xfail because of #281 and old gas payment style. The tests pass if the following lines are commented out:

I'll apply similar fixes to #230 once it is merged, that's why I'm setting this as work in progress again.

@pipermerriam
Copy link
Member

@jannikluhn FYI

This is a common pytest pitfall. xfail is intended for tests that are unreliable, rather than tests that are required to fail. A test marked with xfail that passes doesn't cause the test suite to fail.

In cases where we have an explicit failure, I'd rather stay away from xfail and instead write the test with a with pytest.raises(TheExpectedError) in the place that it's expected to fail. Then at least when we fix the error, we'll have a clearly failing test so that we know that section needs to be updated as well.

In general, I think we should stay way from xfail entirely because it's not that useful.

@jannikluhn
Copy link
Contributor Author

The pytest documentation says the following:

A common example [for xfail] is a test for a feature not yet implemented, or a bug not yet fixed.

so I think xfail is correct here. I'm not a fan of adding pytest.raises because that would change the test itself, although it is correct (it's the tested code that's broken).

I agree though that it would be nicer if the test would fail if it passes unexpectedly. xfail seems to have a strict flag which does exactly that. What do you think about keeping the xfails and adding the strict flag?

@jannikluhn jannikluhn mentioned this pull request Jan 24, 2018
3 tasks
hwwhww pushed a commit to hwwhww/py-evm that referenced this pull request Jan 24, 2018
@pipermerriam
Copy link
Member

@jannikluhn eating my words. Looks like xfail has been improved since I last used it and now does what we/you need. Feel free to continue using it. I think we should add the xfail_strict=true to our pytest.ini to ensure that this is always the case.

Also, I have a preference towards using raises in conjunction with reason since it's more explicit about how the test is expected to fail.

@jannikluhn
Copy link
Contributor Author

Replaced by #313.

@jannikluhn jannikluhn closed this Jan 26, 2018
hwwhww pushed a commit to hwwhww/py-evm that referenced this pull request Feb 1, 2018
hwwhww pushed a commit to hwwhww/py-evm that referenced this pull request Feb 2, 2018
hwwhww pushed a commit to hwwhww/py-evm that referenced this pull request Feb 6, 2018
hwwhww pushed a commit to hwwhww/py-evm that referenced this pull request Feb 18, 2018
hwwhww pushed a commit to hwwhww/py-evm that referenced this pull request Feb 22, 2018
hwwhww pushed a commit to hwwhww/py-evm that referenced this pull request Feb 23, 2018
hwwhww pushed a commit to hwwhww/py-evm that referenced this pull request Mar 2, 2018
hwwhww pushed a commit that referenced this pull request Mar 2, 2018
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