Backport #101: fix find_ancestor. #105
Merged
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
* Some cleanup for the fuzzing code with a few more assertions.
* In order for the round fuzzing to check the eventual completability
of the round, all remaining prevotes must be imported. This is a
good illustration of the fact that in the general case a single
receive omission (of a prevote or precommit) may lead to a
voter being "stuck" in a round due not seeing the estimate at
the right spot in the block tree (i.e. missed prevotes can cause the
estimate to not move "down" far enough and missed precommits can
cause it to not move "up" far enough, in a block tree that grows
downwards).
* The graph fuzzing was always returning early when starting to
record precommit weights, thus not checking much.
* The (generalised) graph fuzzing showed a problem with
find_ancestor whereby it may not return the highest block
in the chain satisfying the condition due to a (hidden)
assumption that the condition must only be true of at most
one child of each vote-node on the chain whose head is the
given starting block (this assumption comes from ghost_find_merge_point).
In the context of a Round, where this function is used to
determine the estimate, this assumption seems satisfied only
because the estimate is not computed before a supermajority of
precommits have been observed. Since all (honest) precommit
weight must be somewhere downwards from the prevote-ghost any
voter sees, it is impossible for a chain not descending from
the prevote-ghost to have supermajority, thus satisfying the
assumption that find_ancestor needs to work correctly. However,
since this is very subtle and not documented, the implementation of
find_ancestor is instead changed here to a simpler version
in which this assumption is not necessary and is arguably
easier to see correct.
* Regression tests for the previous point have been added.
rphmeier
approved these changes
Jan 17, 2020
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.
Dropped the fuzzing changes as the v0.10 branch doesn't include the fuzzing setup.