-
Notifications
You must be signed in to change notification settings - Fork 58
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
Accumulate candidates and votes from previous iterations, same round #1038
Closed
Closed
Changes from 23 commits
Commits
Show all changes
35 commits
Select commit
Hold shift + click to select a range
2536c11
consensus: Reduction steps accumulates votes for any iteration
61475de
consensus: Initial impl of RoundCtx struct
b4a4eba
consensus: Integrate RoundCtx into both Reduction steps
426d49e
node-data: Add Copy trait impl
13a7b55
consensus: Use different topic IDs for first and second reduction mes…
ff00966
node-data: Use different topic IDs for first and second reduction mes…
9480e6b
node: Enable FirstReduction and SecondReduction messages in Chain
094a707
consensus: compile block with certificate
c0c7771
consensus: Collect past events and produce agreements from past itera…
784ff09
rusk-recovery: Enable 64 stakers in genesis
e9d99f6
consensus: Fix logging info
7fc0cfe
consensus: Fix double-lock issue
bd2cb8f
consensus: Implement vote_for_former_candidate
3176713
node: Broadcast any blocks equal to local tip
bc7822e
node: Broadcast any blocks equal to local tip
2d1a585
consensus: Vote on former candidate only if am_member of the step com…
d74daa8
node: Print blocks received in in_sync mode
9ef3a84
Merge branch 'master' into collect_past_messages
goshawk-3 1ade3a3
node: Improve fallback procedure
5d33233
node: Randomize the unique counter in network::send call
becdc5e
consensus: Fix clippy warnings
58495c4
consensus: Add step in Aggregator::get_total
3cf757d
consensus: Republish a drained message, if valid
d80c1ae
consensus: Return from firststep::collect call if step_votes belongs …
ad880cc
consensus: Return from secondstep::collect call if step_votes belongs…
38dc58d
consensus: Fix error discription
28f3e7a
node: Request missing blocks from multiple sources if dest_addr is un…
fc02bc7
consensus: Decrease default consensus_timeout to 5s
9f1d7da
node: Request updates from random peers when accept_block_timeout eve…
ce62586
consensus: Decrease default consensus_timeout_ms to 2000
bbcd402
consensus: Apply a delay in block generator accordingly
2b531f6
node: Trace duration of accept/finalize call execution
7ff415f
consensus: Move committee stores from phases into IterCtx
df539c1
consensus: Maintain the iteraton_ctx object throughout the duration o…
f45e193
consensus: Refactor/Rename round_ctx into StepVotesRegistry
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
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
Oops, something went wrong.
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 problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need the "step" part in the key.
There's no collision between the block hashes...
Does it have another goal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not introduced we accumulate votes for empty hash during all iterations. Before introducing this fix, I noticed votes for an empty hash were more than 67%.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
This is due to the ambiguity of votes. The vote should be separate from the block hash so as to distinguish the case of invalid candidate from that of the candidate not being received. Still, in the case of empty candidate, the iteration/step is indeed needed.
That said, I think nodes should not vote on empty hash. Instead, they should vote NIL only on invalid candidates.
That is:
Note that voting nil for not having received a block is also in contrast to waiting (and voting) for previous-iteration blocks, as this could lead to the same provisioner voting NIL (because it didn't receive the block) and then vote the block's hash when receiving it while on a later iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What metrics do you expect to improve by implementing this patch
not voting on empty hash.
? Why do you propose it here as a comment instead of posting a DIP with better and complete explanation?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote it here as part of the discussion. DIP/Issues can always stem from comments :)
I'll do write a proposal for this, but, in short, I think voting on empty hash yields the same issues as not collecting votes from previous iterations.
Either way, if we vote nil when not receiving a block, we shouldn't vote on previous-iteration blocks, or we could produce a double (and opposite) vote on the same block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an Issue here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in the DIP Issue, relative to this PR, if the node votes NIL for a round/iteration after the timeout and then votes on the candidate when receiving it (
vote_for_former_candidate
) it would produce two votes for the same block, creating potential indeterminism (nodes can choice any of the two votes).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply put,
Nil vote means a vote for tuple (empty hash, round, step)
A real vote means a vote for tuple (candidate_block_hash, round, step)
Not the case at all. Nodes can reach quorum for an empty hash at iteration 1 and then move forward and reach quorum for candidate block of the iteration 1 while running the second iteration. The output of this case is a valid finalized block.
There is no indeterminism as these are not the same blocks. Also, voting NIL only helps provisioners to move forward when a candidate block is not generated, it has not other impact other than that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not currently distinguish between NIL for unknown candidate and NIL for invalid block.
In both cases, the vote will be (round, step, empty_hash).
So, a NIL vote is effectively valid as a negative vote on the candidate, whichever that might be.
If we produce both a NIL and a candidate votes, they would be a condtradictive vote for the same block.
And if I receive both votes I can use either for the quorum, and they would be both valid votes.
In fact, in extreme cases, there could be a quorum on both NIL and the candidate.
Example 1:
Now there are two votes for the same round-step.
Let's say the committee is split in half (half voted NIL, half voted the candidate) and my vote is the one deciding the quorum.
Nodes receiving the NIL vote first will reach quorum on NIL, and move to the next iteration;
Nodes receiving the candidate vote first will reach quorum on the block, and move to the next round.
Both nodes will have a valid quorum for the same round and step, but one is NIL and one is for the candidate.
Example 2:
Depending on which votes they receive first, nodes can either reach quorum on the candidate or on the empty hash.
You're only seeing the best-case scenario.
In this case you still have two valid quorums on for the same round/iteration.
If a quorum is reached on iteration 2, it's possible that some nodes will move to the next round while others will accept the first-iteration block and move to the next round.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reaching quorum on a tuple (empty_hash, round, step) does not impact reaching quorum for any candidate block of the same round.
I'd suggest to pair up (when you're available) and review with you the implementation to ensure we're on the same page.