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

Add Database for tracking key accesses #1918

Merged
merged 5 commits into from Mar 11, 2020
Merged

Add Database for tracking key accesses #1918

merged 5 commits into from Mar 11, 2020

Conversation

@carver
Copy link
Contributor

carver commented Mar 3, 2020

What was wrong?

Wanted an easy way to track all key accesses, for #1917 .

How was it fixed?

Added new KeyAccessLoggerDB (and an atomic database version -- the type checking seems to require both).

Some other bonus tweaks to:

  • clean up types
  • add some of the block hash to str(block)
  • speed up the Makefile, etc.

To-Do

  • Clean up commit history
  • Add a test that key accesses are properly recorded

Cute Animal Picture

put a cute animal picture link inside the parentheses

carver added a commit to carver/py-evm that referenced this pull request Mar 3, 2020
@carver carver requested a review from cburgdorf Mar 3, 2020
@carver carver mentioned this pull request Mar 3, 2020
3 of 5 tasks complete
@carver

This comment has been minimized.

Copy link
Contributor Author

carver commented Mar 4, 2020

Only significant todo is to add a test that accesses are properly logged.

Copy link
Contributor

cburgdorf left a comment

LGTM, some remarks inline.

@@ -39,7 +39,6 @@ coverage:
open htmlcov/index.html

build-docs:
pip install -e .[doc]
cd docs/; sphinx-build -W -T -E . _build/html

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Mar 4, 2020

Contributor

I guess the idea here was that one would be able to run make build-docs without the assumption that docs dependencies were already installed.

This comment has been minimized.

Copy link
@carver

carver Mar 9, 2020

Author Contributor

Yeah, if it was really fast, I probably wouldn't object, but the pip install actually takes a while. So if you run make build-docs over and over, it gets old really fast. Also, a bunch of our other make targets assume that you have the appropriate installed tools (wheel, setuptools, towncrier, etc). We could probably document them all better, though. 👍

@property
def keys_read(self) -> Set[bytes]:
# Make a defensive copy so callers can't modify the list externally
return set(self._keys_read)

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Mar 4, 2020

Contributor

Returning a new set provides the protection we need but maybe that's a good candidate to use frozenset just to drive home the point that this is meant to be readonly.

@@ -29,4 +29,5 @@ def __repr__(self) -> str:
return f'<{self.__class__.__name__}(#{str(self)})>'

def __str__(self) -> str:
return f"Block #{self.number}"
hash_start = self.hash[:3].hex()
return f"Block #{self.number}-0x{hash_start}..."

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Mar 4, 2020

Contributor

Did you intentionally not use humanize_hash from eth_utils here (which produces hashs of the format eb58..84cd)?

This comment has been minimized.

Copy link
@carver

carver Mar 9, 2020

Author Contributor

I guess it basically comes down to not liking that format. I guess I can get over it, and consistency is better than my personal stylistic taste. :)

"""
A VM that does POW mining as well. Should be used only in tests, when we
need to programatically populate a ChainDB.
"""
def finalize_block(self, block: BaseBlock) -> BaseBlock:
block = super().finalize_block(block) # type: ignore

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Mar 4, 2020

Contributor

Always nice to get rid of type: ignore

@@ -0,0 +1,3 @@
Added :cls:`~eth.db.accesslog.KeyAccessLoggerDB` and its atomic twin; faster ``make validate-docs``

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Mar 4, 2020

Contributor

This needs to be :class: (I had to these up in the release notes yesterday, which is why know 😅 )

@@ -0,0 +1,3 @@
Added :cls:`~eth.db.accesslog.KeyAccessLoggerDB` and its atomic twin; faster ``make validate-docs``
(but you have to remember to ``pip install -e .[doc]`` yourself); ``str(block)`` now includes the

This comment has been minimized.

Copy link
@cburgdorf

cburgdorf Mar 4, 2020

Contributor

Ah, ok, you are pointing out what I have commented above. Fine by me.

@carver carver changed the title Add Database for tracking keys Add Database for tracking key accesses Mar 9, 2020
@carver carver force-pushed the carver:key-logger branch from c221daa to 74dbaa8 Mar 11, 2020
@carver carver merged commit f89fa61 into ethereum:master Mar 11, 2020
21 checks passed
21 checks passed
ci/circleci: py36-benchmark Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py36-database Your tests passed on CircleCI!
Details
ci/circleci: py36-docs Your tests passed on CircleCI!
Details
ci/circleci: py36-lint Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-byzantium Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-constantinople Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-frontier Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-homestead Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-istanbul Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-petersburg Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-spurious_dragon Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-tangerine_whistle Your tests passed on CircleCI!
Details
ci/circleci: py36-native-blockchain-transition Your tests passed on CircleCI!
Details
ci/circleci: py36-transactions Your tests passed on CircleCI!
Details
ci/circleci: py36-vm Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
ci/circleci: py37-database Your tests passed on CircleCI!
Details
ci/circleci: py37-lint Your tests passed on CircleCI!
Details
ci/circleci: py37-transactions Your tests passed on CircleCI!
Details
ci/circleci: py37-vm Your tests passed on CircleCI!
Details
@carver carver deleted the carver:key-logger branch Mar 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.