Implemented Best PreVote Candidate algorithm #679
Conversation
|
| @@ -34,6 +35,8 @@ public class RoundState { | |||
| private Map<PubKey, Vote> prevotes = new ConcurrentHashMap<>(); | |||
| private Map<PubKey, SignedVote> pvEquivocations = new ConcurrentHashMap<>(); | |||
| private Map<PubKey, SignedVote> pcEquivocations = new ConcurrentHashMap<>(); | |||
| //TODO: Figure out if we need to store vote messages in round state | |||
| private VoteMessage voteMessage; | |||
There was a problem hiding this comment.
This field serves the purpose of a primary proposal message from the network or from ourselves if we are the primary authority/voter. However it currently gets updated on every handled vote message in the GrandpaEngine. The correct flow of this would be as follows:
GrandpaEngine receives a vote message -> if the message of of type PREVOTE or PRIMARY_PROPOSAL it gets added to the appropriate collection inside of RoundState, same goes for PRECOMMIT.
Regarding the TODO. The state currently holds data that belongs to different aspects of grandpa:
- Data related to the set: voters, setId and roundNumber
- Data related to the round: votes and equivocation votes
Imo we should separate this. We can rename this class to GrandpaSetState and keep the data related to sets. Create an object that holds round data eg GrandpaRound that holds the data related to rounds. The state would hold the current and previous rounds as those are the only ones we need per spec. In this way we can apply a more structured update of sets and rounds:
GrandpaSetState will update its voters, setId and set roundNumber to 0 every time we receive an appropriate consensus message from the runtime (similar to BABE).
The current and previous GrandpaRound fields will be updated every time we finalize a round in the grandpa play round algo. This way we'd have an appropriate place to store the votes, equivocation votes and the primary proposal.
| @@ -111,6 +113,27 @@ public Vote getGrandpaGhost() { | |||
| return selectBlockWithMostVotes(blocks); | |||
| } | |||
|
|
|||
| /** | |||
| * Determines what block is our pre-voted block for the current round | |||
There was a problem hiding this comment.
This comment explains the pseudo code of the algo from the spec, but does not reflect the code in the method.
| */ | ||
| public Vote getBestPreVoteCandidate() { | ||
| Vote currentVote = getGrandpaGhost(); | ||
| VoteMessage voteMessage = roundState.getVoteMessage(); |
There was a problem hiding this comment.
The getVoteMessage() would almost never return a correct value here. Check my comment under RoundState
| VoteMessage voteMessage = roundState.getVoteMessage(); | ||
| SignedMessage signedMessage = voteMessage.getMessage(); | ||
|
|
||
| if (signedMessage != null && signedMessage.getBlockNumber().compareTo(currentVote.getBlockNumber()) > 0) { |
There was a problem hiding this comment.
This doesn't correctly check if we have a primary proposal vote for the current round. What's happening now is we invoke GHOST and get the current pre-vote block with most votes (B r pv in spec) and a wrong value for primary proposed block (B in spec).
signedMessage.getBlockNumber().compareTo(currentVote.getBlockNumber()) > 0
This would translate to pseudo code as following (considering we have correct B r pv and B prim):
B > B r pv.
There was a problem hiding this comment.
To fully align with the specification, you should also add the result of Best_Final_Candidate(r - 1) in the comparison.
I added collection for holding previous result of BEST_FINAL_CANDIDATE to the RoundState in order to have access to the previous rounds data. This collection should probably also be removed form the RoundState and added to the GrandpaSetState.
//TODO: Refactor if this map is accessed/modified concurrently
private Map<BigInteger, Vote> bestFinalCandidateArchive = new HashMap<>();
...
public void addBestFinalCandidateToArchive(BigInteger roundNumber, Vote vote) {
this.bestFinalCandidateArchive.put(roundNumber, vote);
}
public Vote getBestFinalCandidateForRound(BigInteger roundNumber) {
return this.bestFinalCandidateArchive.get(roundNumber);
}
|
The comments that I left may be outside the scope of this PR. What i consider most important for this PR is the following:
What should be done outside this PR:
|
Implemented Best PreVote Candidate algorithm.
https://spec.polkadot.network/sect-finality#algo-best-prevote-candidate
Checklist: