Implemented Best PreVote Candidate algorithm #679

osrib · 2025-01-09T07:49:03Z

Implemented Best PreVote Candidate algorithm.
https://spec.polkadot.network/sect-finality#algo-best-prevote-candidate

Checklist:

I have read the contributing guidelines.
My PR title matches the Conventional Commits spec.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

Zurcusa · 2025-01-09T14:14:13Z

src/main/java/com/limechain/grandpa/state/RoundState.java

@@ -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;


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.

Zurcusa · 2025-01-09T14:15:34Z

src/main/java/com/limechain/grandpa/GrandpaService.java

@@ -111,6 +113,27 @@ public Vote getGrandpaGhost() {
        return selectBlockWithMostVotes(blocks);
    }

+    /**
+     * Determines what block is our pre-voted block for the current round


This comment explains the pseudo code of the algo from the spec, but does not reflect the code in the method.

Zurcusa · 2025-01-09T14:28:32Z

src/main/java/com/limechain/grandpa/GrandpaService.java

+     */
+    public Vote getBestPreVoteCandidate() {
+        Vote currentVote = getGrandpaGhost();
+        VoteMessage voteMessage = roundState.getVoteMessage();


The getVoteMessage() would almost never return a correct value here. Check my comment under RoundState

Zurcusa · 2025-01-09T14:44:29Z

src/main/java/com/limechain/grandpa/GrandpaService.java

+        VoteMessage voteMessage = roundState.getVoteMessage();
+        SignedMessage signedMessage = voteMessage.getMessage();
+
+        if (signedMessage != null && signedMessage.getBlockNumber().compareTo(currentVote.getBlockNumber()) > 0) {


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.

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); }

Zurcusa · 2025-01-09T15:04:25Z

The comments that I left may be outside the scope of this PR. What i consider most important for this PR is the following:

Separate set data from round data in the RoundState
Create the GrandpaRound objects and place them in the state.
Change the logic of the Best-PreVote-Candidate method so that it conforms to the spec. For this to happen the primary proposal vote inside of the current GrandpaRound object should be used.

What should be done outside this PR:

Proper vote message handling (based on round type)
Add logic regarding updating sets, rounds etc.

Grigorov-Georgi · 2025-01-15T14:14:58Z

src/main/java/com/limechain/grandpa/state/RoundCache.java

+public class RoundCache {
+
+    private static final int CACHE_SET_CAPACITY = 4;
+    private final Map<BigInteger, List<GrandpaRound>> roundMap = new LinkedHashMap<>(


Have we talked about making RoundCache thread-safe, or there is no need for that?

From the comment above the class I have a feeling a thread save structure would be needed.

Grigorov-Georgi · 2025-01-15T14:20:15Z

Also, I made a branch with refactored GrandpaService methods, where I use RoundCache -> 398-refactoring-grandpa-methods. You don't need to refactor them; I can merge them to your branch when you are ready.

Zurcusa · 2025-01-15T14:22:03Z

src/main/java/com/limechain/grandpa/state/GrandpaRound.java

+
+    private Map<Hash256, SignedVote> preVotes = new ConcurrentHashMap<>();
+    private Map<Hash256, SignedVote> preCommits = new ConcurrentHashMap<>();
+    private Map<Hash256, SignedVote> primaryProposals = new ConcurrentHashMap<>();


The primary proposal can be only one per round. I suggest we either treat it as a prevote as it is counted the same way other prevotes are or we have a single SignedVote field.

Come to think of it I'd include it in the prevote collection so that we don't have to refactor methods that rely on counting prevotes.

Zurcusa · 2025-01-15T14:24:00Z

src/main/java/com/limechain/grandpa/state/RoundCache.java

+
+    private static final int CACHE_SET_CAPACITY = 4;
+    private final Map<BigInteger, List<GrandpaRound>> roundMap = new LinkedHashMap<>(
+            CACHE_SET_CAPACITY, 0.75f, true) {


What do the 2nd and 3rd parameters do here?

Zurcusa · 2025-01-15T14:29:08Z

src/main/java/com/limechain/grandpa/state/RoundCache.java

+public class RoundCache {
+
+    private static final int CACHE_SET_CAPACITY = 4;
+    private final Map<BigInteger, List<GrandpaRound>> roundMap = new LinkedHashMap<>(


From the comment above the class I have a feeling a thread save structure would be needed.

Zurcusa · 2025-01-15T14:30:35Z

There are some new sonar issues. Please have a look.

sonarqubecloud · 2025-01-17T14:14:16Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

osrib requested review from georg-getz, Grigorov-Georgi and Zurcusa January 9, 2025 07:49

nistanimirov added this to the Fruzhin Phase 3 - M2. GRANDPA milestone Jan 9, 2025

nistanimirov linked an issue Jan 9, 2025 that may be closed by this pull request

Best PreVote Candidate #397

Closed

nistanimirov assigned osrib Jan 9, 2025

georg-getz approved these changes Jan 9, 2025

View reviewed changes

Zurcusa reviewed Jan 9, 2025

View reviewed changes

osrib force-pushed the 397-Best-PreVote-Candidate branch from 6c8ecea to b9cfa61 Compare January 15, 2025 11:27

Grigorov-Georgi approved these changes Jan 15, 2025

View reviewed changes

Zurcusa approved these changes Jan 15, 2025

View reviewed changes

Oleksandr added 4 commits January 17, 2025 15:39

Implemented Best PreVote Candidate algorithm

fe3fe18

Implemented Best PreVote Candidate algorithm

a499a9e

Implemented Round Cache and fix comments

0c0c7d9

fixed conflicts

23d716d

osrib force-pushed the 397-Best-PreVote-Candidate branch from b9cfa61 to 23d716d Compare January 17, 2025 14:05

fixed sonar issues

7013a20

osrib merged commit 4bdacc3 into dev Jan 17, 2025
3 checks passed

osrib deleted the 397-Best-PreVote-Candidate branch January 17, 2025 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented Best PreVote Candidate algorithm #679

Implemented Best PreVote Candidate algorithm #679

osrib commented Jan 9, 2025 •

edited

Loading

Zurcusa Jan 9, 2025

Zurcusa Jan 9, 2025

Zurcusa Jan 9, 2025

Zurcusa Jan 9, 2025

Grigorov-Georgi Jan 9, 2025

Zurcusa commented Jan 9, 2025

Grigorov-Georgi Jan 15, 2025

Zurcusa Jan 15, 2025

Grigorov-Georgi commented Jan 15, 2025

Zurcusa Jan 15, 2025

Zurcusa Jan 15, 2025

Zurcusa Jan 15, 2025

Zurcusa Jan 15, 2025

Zurcusa commented Jan 15, 2025

sonarqubecloud bot commented Jan 17, 2025

Implemented Best PreVote Candidate algorithm #679

Implemented Best PreVote Candidate algorithm #679

Conversation

osrib commented Jan 9, 2025 • edited Loading

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zurcusa commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Grigorov-Georgi commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zurcusa commented Jan 15, 2025

sonarqubecloud bot commented Jan 17, 2025

Quality Gate passed

osrib commented Jan 9, 2025 •

edited

Loading