Fix overzealous TraversedPartialPath raised on leaves #114
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
carver
force-pushed
the
leaf-traversed-partial-path-bugfix
branch
from
9bba3cc
to
40ed865
Compare
carver
force-pushed
the
leaf-traversed-partial-path-bugfix
branch
from
40ed865
to
fd5fb86
Compare
carver
commented
Jun 16, 2020
trie/tools/builder.py
Outdated
| @@ -11,6 +11,6 @@ def trie_from_keys(keys, prune=False): | |||
| trie = HexaryTrie(node_db, prune=prune) | |||
| with trie.squash_changes() as trie_batch: | |||
| for k in keys: | |||
| trie_batch[k] = k | |||
| trie_batch[k] = k.rjust(minimum_value_length, b'3') | |||
There was a problem hiding this comment.
I can add the same note as in trie.tools.strategies:
# Flood 3's at the end of the value to make it longer. b'3' is encoded to 0x33,
# so the bytes and HexBytes representation look the same. Just a convenience. Oh, I should also switch this to ljust so it's actually putting the 3s at the end, instead of the beginning.
carver
commented
Jun 16, 2020
| for nibble in range(0xf): | ||
| # Note that we do not want to look at the 0xf nibble, because that's the one that | ||
| # should raise the exception above | ||
| assert trie.traverse((nibble,)) == EMPTY_NODE |
There was a problem hiding this comment.
Before the attached bugfix, this traverse() line would also raise a TraversedPartialPath exception.
|
@njgheorghita would you mind reviewing? Only the last two commits are relevant, the others will get reviewed in #113 -- I'm happy to hop on a call if you want to talk it over. |
njgheorghita
approved these changes
Jun 16, 2020
carver
force-pushed
the
leaf-traversed-partial-path-bugfix
branch
from
fd5fb86
to
d80ce9f
Compare
This seems to produce *simpler* test cases that fail, and trigger more code paths more easily. See the upcoming commit about incorrectly raising TraversedPartialPath during a leaf node access.
Shouldn't return leaves as partially traversed if the remaining key doesn't match the leaf. We should treat that as a node simply not being present in the trie. (ie~ return a blank node)
carver
force-pushed
the
leaf-traversed-partial-path-bugfix
branch
from
d80ce9f
to
3dc69b6
Compare
carver
added a commit
to carver/py-trie
that referenced
this pull request
Jun 18, 2020
Recently, there was a fix for incorrectly raising a TraversedPartialPath when traversing into a leaf, even if the suffix of the leaf did not match the traversal path. See: ethereum#114 This bug was actually found during a hypothesis run during a trie walk. Although there are new unit tests making sure the leaf traversal works properly, it seems worth adding the example explicitly as a kind of backup test that the scenario works properly during a trie walk & backfill.
carver
added a commit
to carver/py-trie
that referenced
this pull request
Jun 18, 2020
Recently, there was a fix for incorrectly raising a TraversedPartialPath when traversing into a leaf, even if the suffix of the leaf did not match the traversal path. See: ethereum#114 This bug was actually found during a hypothesis run during a trie walk. Although there are new unit tests making sure the leaf traversal works properly, it seems worth adding the example explicitly as a kind of backup test that the scenario works properly during a trie walk & backfill.
carver
added a commit
to carver/py-trie
that referenced
this pull request
Jun 18, 2020
Recently, there was a fix for incorrectly raising a TraversedPartialPath when traversing into a leaf, even if the suffix of the leaf did not match the traversal path. See: ethereum#114 This bug was actually found during a hypothesis run during a trie walk. Although there are new unit tests making sure the leaf traversal works properly, it seems worth adding the example explicitly as a kind of backup test that the scenario works properly during a trie walk & backfill.
carver
added a commit
to carver/py-trie
that referenced
this pull request
Jun 19, 2020
Recently, there was a fix for incorrectly raising a TraversedPartialPath when traversing into a leaf, even if the suffix of the leaf did not match the traversal path. See: ethereum#114 This bug was actually found during a hypothesis run during a trie walk. Although there are new unit tests making sure the leaf traversal works properly, it seems worth adding the example explicitly as a kind of backup test that the scenario works properly during a trie walk & backfill.
pacrob
pushed a commit
to pacrob/py-trie
that referenced
this pull request
Apr 18, 2024
* Ignore all __pycache__ directories * Newsfragment * Revert Newsfragment
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What was wrong?
.traverse()was raising aTraversedPartialPathwith a leaf node, even if you try to traverse on a path unrelated to the leaf. Instead, if you traverse into an unrelated path, you should just get an empty node.How was it fixed?
Return the empty node if the traversed path doesn't match the beginning of the leaf suffix.
TODO:
Cute Animal Picture