pipeline: hasher bug #1267
Merged
pipeline: hasher bug #1267
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
acud
commented
Feb 16, 2021
zelig
approved these changes
Feb 19, 2021
| errTrieFull = errors.New("trie full") | ||
| ) | ||
|
|
||
| const maxLevel = 8 | ||
|
|
||
| type hashTrieWriter struct { |
| @@ -12,15 +12,21 @@ import ( | |||
| "github.com/ethersphere/bee/pkg/swarm" | |||
| ) | |||
|
|
|||
| var errInconsistentRefs = errors.New("inconsistent reference lengths in level") | |||
| var ( | |||
| errInconsistentRefs = errors.New("inconsistent references") | |||
| var errInconsistentRefs = errors.New("inconsistent reference lengths in level") | ||
| var ( | ||
| errInconsistentRefs = errors.New("inconsistent references") | ||
| errTrieFull = errors.New("trie full") |
There was a problem hiding this comment.
i thought we agreed this should be handled but its ok
|
This PR is ready for review, but not connected to an issue. The issue which belongs to the PR is not in the |
Let's throttle renaming. I'll do this in a separate PR |
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.
There was a bug in our hashing pipeline that was discovered in #1175.
The problem was that we assumed that the trie depth was known in advanced, however, when wrapping deeper levels under certain circumstances the upper levels would also wrap, resulting in a trie that is deeper than originally assumed. The result was that the trie was being traversed only until the preassumed depth, leaving us with an "empty" level once it has wrapped into the next.
This has been demonstrated with an explicit regression test that is now in the
hashtriepackage. Furthermore, unit tests have now been added to thehashtriepackage. It covers various scenarios and edge cases.The
Summethod logic has now been inlined into the method instead of the helper method, and the logic has been significantly simplified, with extensive comments and explanations on how things work, with the hope that this improves developer onboarding and open-source contributions, which have revealed this bug.The logic of generating the root hash has changed and indeed simplified. In the past, we would wrongly assume which level we're aiming at and try to read the resulting data out of that level. However as mentioned, this did not take into account the case where several subsequent levels were wrapped as a result of one level wrap (causing the trie to balance, basically).
After the simplification we just iterate on all levels and constantly carry over hashes or wrap levels as needed, carrying over the result to higher level. In the end of the operation, we just take the resulting hash from the top level of the trie.
I have used
TestRegressionto verify that this problem exists on currentmasterand is solved with this change, as well as manual tests. In the unit tests added to thehashtriepackage, I have chosen an approach where one predefined hash is constantly being written to the trie, with constant spans of value1. This allows the eventual root chunk span to be compared against the linear number of writes that were done in the test. Also, custom branching and chunk size values were used in tests in order to decrease the number of writes needed in order to trigger a level wrap of the trie.