Skip to content
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.

Sign up for GitHub

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

Implemented Best PreVote Candidate algorithm #679

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Implemented Best PreVote Candidate algorithm #679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/main/java/com/limechain/grandpa/GrandpaService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
import com.limechain.exception.storage.BlockStorageGenericException;
import com.limechain.grandpa.state.RoundState;
import com.limechain.network.protocol.grandpa.messages.commit.Vote;
import com.limechain.network.protocol.grandpa.messages.vote.SignedMessage;
import com.limechain.network.protocol.grandpa.messages.vote.Subround;
import com.limechain.network.protocol.grandpa.messages.vote.VoteMessage;
import com.limechain.network.protocol.warp.dto.BlockHeader;
import com.limechain.storage.block.BlockState;
import io.emeraldpay.polkaj.types.Hash256;
Expand Down Expand Up @@ -111,6 +113,27 @@ public Vote getGrandpaGhost() {
return selectBlockWithMostVotes(blocks);
}

/**
* Determines what block is our pre-voted block for the current round
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* if we receive a vote message from the network with a
* block that's greater than or equal to the current pre-voted block
* and greater than the best final candidate from the last round, we choose that.
* otherwise, we simply choose the head of our chain.
*
* @return the best pre-voted block
*/
public Vote getBestPreVoteCandidate() {
Vote currentVote = getGrandpaGhost();
VoteMessage voteMessage = roundState.getVoteMessage();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SignedMessage signedMessage = voteMessage.getMessage();

if (signedMessage != null && signedMessage.getBlockNumber().compareTo(currentVote.getBlockNumber()) > 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To fully align with the specification, you should also add the result of Best_Final_Candidate(r - 1) in the comparison.
Screenshot 2025-01-09 at 17 43 20
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);
    }

return new Vote(signedMessage.getBlockHash(), signedMessage.getBlockNumber());
}

return currentVote;
}

/**
* Selects the block with the most votes from the provided map of blocks.
* If multiple blocks have the same number of votes, it returns the one with the highest block number.
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/com/limechain/grandpa/state/RoundState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.limechain.chain.lightsyncstate.Authority;
import com.limechain.network.protocol.grandpa.messages.catchup.res.SignedVote;
import com.limechain.network.protocol.grandpa.messages.commit.Vote;
import com.limechain.network.protocol.grandpa.messages.vote.VoteMessage;
import io.libp2p.core.crypto.PubKey;
import lombok.Getter;
import lombok.Setter;
Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


/**
* The threshold is determined as the total weight of authorities
Expand All @@ -57,4 +60,8 @@ public BigInteger derivePrimary() {
var votersCount = BigInteger.valueOf(voters.size());
return roundNumber.remainder(votersCount);
}

public void updateVoteMessage(VoteMessage voteMessage) {
this.voteMessage = voteMessage;
}
}
5 changes: 5 additions & 0 deletions src/main/java/com/limechain/network/protocol/grandpa/GrandpaEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.limechain.network.protocol.grandpa;

import com.limechain.exception.scale.ScaleEncodingException;
import com.limechain.grandpa.state.RoundState;
import com.limechain.network.ConnectionManager;
import com.limechain.network.protocol.blockannounce.messages.BlockAnnounceHandshakeBuilder;
import com.limechain.network.protocol.grandpa.messages.GrandpaMessageType;
Expand Down Expand Up @@ -41,11 +42,13 @@ public class GrandpaEngine {
protected ConnectionManager connectionManager;
protected WarpSyncState warpSyncState;
protected BlockAnnounceHandshakeBuilder handshakeBuilder;
protected RoundState roundState;

public GrandpaEngine() {
connectionManager = ConnectionManager.getInstance();
warpSyncState = AppBean.getBean(WarpSyncState.class);
handshakeBuilder = new BlockAnnounceHandshakeBuilder();
roundState = AppBean.getBean(RoundState.class);
}

/**
Expand Down Expand Up @@ -140,6 +143,8 @@ private void handleNeighbourMessage(byte[] message, PeerId peerId) {
private void handleVoteMessage(byte[] message, PeerId peerId) {
ScaleCodecReader reader = new ScaleCodecReader(message);
VoteMessage voteMessage = reader.read(VoteMessageScaleReader.getInstance());
//Maybe we need to add possible roundNumber check
roundState.updateVoteMessage(voteMessage);
//todo: handle vote message (authoring node responsibility?)
log.log(Level.INFO, "Received vote message from Peer " + peerId + "\n" + voteMessage);
}
Expand Down
93 changes: 93 additions & 0 deletions src/test/java/com/limechain/grandpa/GrandpaServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
import com.limechain.grandpa.state.RoundState;
import com.limechain.network.protocol.grandpa.messages.catchup.res.SignedVote;
import com.limechain.network.protocol.grandpa.messages.commit.Vote;
import com.limechain.network.protocol.grandpa.messages.vote.SignedMessage;
import com.limechain.network.protocol.grandpa.messages.vote.Subround;
import com.limechain.network.protocol.grandpa.messages.vote.VoteMessage;
import com.limechain.network.protocol.warp.dto.BlockHeader;
import com.limechain.network.protocol.warp.dto.ConsensusEngine;
import com.limechain.network.protocol.warp.dto.DigestType;
Expand Down Expand Up @@ -49,6 +51,7 @@ void setUp() {
roundState = mock(RoundState.class);
blockState = mock(BlockState.class);
grandpaService = new GrandpaService(roundState, blockState);

}

@Test
Expand Down Expand Up @@ -157,6 +160,96 @@ void testGetBestFinalCandidateWhereRoundNumberIsZero() {
assertEquals(blockHeader.getBlockNumber(), result.getBlockNumber());
}

@Test
void testGetBestPreVoteCandidate_WithSignedMessage() {
Vote currentVote = new Vote(new Hash256(ONES_ARRAY), BigInteger.valueOf(3));
VoteMessage voteMessage = mock(VoteMessage.class);
SignedMessage signedMessage = mock(SignedMessage.class);

when(roundState.getVoteMessage()).thenReturn(voteMessage);
BlockHeader blockHeader = createBlockHeader();
blockHeader.setBlockNumber(BigInteger.valueOf(1));

when(roundState.getThreshold()).thenReturn(BigInteger.valueOf(1));
when(roundState.getRoundNumber()).thenReturn(BigInteger.valueOf(1));

when(roundState.getPrecommits()).thenReturn(Map.of());
when(roundState.getPrevotes()).thenReturn(Map.of(
Ed25519Utils.generateKeyPair().publicKey(), currentVote
));

when(blockState.getHighestFinalizedHeader()).thenReturn(blockHeader);
when(blockState.isDescendantOf(currentVote.getBlockHash(), currentVote.getBlockHash())).thenReturn(true);
when(voteMessage.getMessage()).thenReturn(signedMessage);
when(signedMessage.getBlockNumber()).thenReturn(BigInteger.valueOf(4));
when(signedMessage.getBlockHash()).thenReturn(new Hash256(TWOS_ARRAY));

Vote result = grandpaService.getBestPreVoteCandidate();


assertNotNull(result);
assertEquals(new Hash256(TWOS_ARRAY), result.getBlockHash());
assertEquals(BigInteger.valueOf(4), result.getBlockNumber());
}

@Test
void testGetBestPreVoteCandidate_WithoutSignedMessage() {
Vote currentVote = new Vote(new Hash256(ONES_ARRAY), BigInteger.valueOf(3));
VoteMessage voteMessage = mock(VoteMessage.class);

when(roundState.getVoteMessage()).thenReturn(voteMessage);
BlockHeader blockHeader = createBlockHeader();
blockHeader.setBlockNumber(BigInteger.valueOf(1));

when(roundState.getThreshold()).thenReturn(BigInteger.valueOf(1));
when(roundState.getRoundNumber()).thenReturn(BigInteger.valueOf(1));

when(roundState.getPrecommits()).thenReturn(Map.of());
when(roundState.getPrevotes()).thenReturn(Map.of(
Ed25519Utils.generateKeyPair().publicKey(), currentVote
));

when(blockState.getHighestFinalizedHeader()).thenReturn(blockHeader);
when(blockState.isDescendantOf(currentVote.getBlockHash(), currentVote.getBlockHash())).thenReturn(true);

Vote result = grandpaService.getBestPreVoteCandidate();

assertNotNull(result);
assertEquals(currentVote.getBlockHash(), result.getBlockHash());
assertEquals(currentVote.getBlockNumber(), result.getBlockNumber());
}

@Test
void testGetBestPreVoteCandidate_WithSignedMessageAndBlockNumberLessThanCurrentVote() {
Vote currentVote = new Vote(new Hash256(ONES_ARRAY), BigInteger.valueOf(4));
VoteMessage voteMessage = mock(VoteMessage.class);
SignedMessage signedMessage = mock(SignedMessage.class);

when(roundState.getVoteMessage()).thenReturn(voteMessage);
BlockHeader blockHeader = createBlockHeader();
blockHeader.setBlockNumber(BigInteger.valueOf(1));

when(roundState.getThreshold()).thenReturn(BigInteger.valueOf(1));
when(roundState.getRoundNumber()).thenReturn(BigInteger.valueOf(1));

when(roundState.getPrecommits()).thenReturn(Map.of());
when(roundState.getPrevotes()).thenReturn(Map.of(
Ed25519Utils.generateKeyPair().publicKey(), currentVote
));

when(blockState.getHighestFinalizedHeader()).thenReturn(blockHeader);
when(blockState.isDescendantOf(currentVote.getBlockHash(), currentVote.getBlockHash())).thenReturn(true);
when(voteMessage.getMessage()).thenReturn(signedMessage);
when(signedMessage.getBlockNumber()).thenReturn(BigInteger.valueOf(3));
when(signedMessage.getBlockHash()).thenReturn(new Hash256(TWOS_ARRAY));

Vote result = grandpaService.getBestPreVoteCandidate();

assertNotNull(result);
assertEquals(currentVote.getBlockHash(), result.getBlockHash());
assertEquals(currentVote.getBlockNumber(), result.getBlockNumber());
}

@Test
void testGetGrandpaGHOSTWhereNoBlocksPassThreshold() {
when(roundState.getThreshold()).thenReturn(BigInteger.valueOf(10));
Expand Down
3 changes: 3 additions & 0 deletions src/test/java/com/limechain/network/protocol/grandpa/GrandpaEngineTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.limechain.network.protocol.grandpa;

import com.limechain.grandpa.state.RoundState;
import com.limechain.network.ConnectionManager;
import com.limechain.network.dto.PeerInfo;
import com.limechain.network.protocol.blockannounce.NodeRole;
Expand Down Expand Up @@ -48,6 +49,8 @@ class GrandpaEngineTest {
@Mock
private WarpSyncState warpSyncState;
@Mock
private RoundState roundState;
@Mock
private BlockAnnounceHandshakeBuilder blockAnnounceHandshakeBuilder;

private final NeighbourMessage neighbourMessage =
Expand Down
Loading