Repair Clique Proposer Selection (#339)

Clique Proposer Selection would choose an incorrect peer
when a signer was removed from the pool, as the algorithm worked
purely on the block count.

The algorithm has now been updated to ensure the next proposer is
incrementally the next signer in the ordered list, based on the
parent's proposer

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/CliqueBlockHashing.java

@@ -50,7 +50,7 @@ public static Address recoverProposerAddress(
       final BlockHeader header, final CliqueExtraData cliqueExtraData) {
     if (!cliqueExtraData.getProposerSeal().isPresent()) {
       throw new IllegalArgumentException(
-          "Supplied cliqueExtraData does not include a proposer " + "seal");
+          "Supplied cliqueExtraData does not include a proposer seal.");
     }
     final Hash proposerHash = calculateDataHashForProposerSeal(header, cliqueExtraData);
     return Util.signatureToAddress(cliqueExtraData.getProposerSeal().get(), proposerHash);

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueProposerSelector.java

@@ -14,13 +14,14 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import tech.pegasys.pantheon.consensus.clique.CliqueBlockInterface;
 import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
 import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 
-import java.util.ArrayList;
-import java.util.List;
+import java.util.NavigableSet;
+import java.util.SortedSet;
 
 /**
  * Responsible for determining which member of the validator pool should create the next block.
@@ -31,6 +32,7 @@
 public class CliqueProposerSelector {
 
   private final VoteTallyCache voteTallyCache;
+  private final CliqueBlockInterface blockInterface = new CliqueBlockInterface();
 
   public CliqueProposerSelector(final VoteTallyCache voteTallyCache) {
     checkNotNull(voteTallyCache);
@@ -46,11 +48,26 @@ public CliqueProposerSelector(final VoteTallyCache voteTallyCache) {
   public Address selectProposerForNextBlock(final BlockHeader parentHeader) {
 
     final VoteTally parentVoteTally = voteTallyCache.getVoteTallyAtBlock(parentHeader);
-    final List<Address> validatorSet = new ArrayList<>(parentVoteTally.getCurrentValidators());
+    final NavigableSet<Address> validatorSet = parentVoteTally.getCurrentValidators();
 
-    final long nextBlockNumber = parentHeader.getNumber() + 1L;
-    final int indexIntoValidators = (int) (nextBlockNumber % validatorSet.size());
+    Address prevBlockProposer = validatorSet.first();
+    if (parentHeader.getNumber() != BlockHeader.GENESIS_BLOCK_NUMBER) {
+      prevBlockProposer = blockInterface.getProposerOfBlock(parentHeader);
+    }
 
-    return validatorSet.get(indexIntoValidators);
+    return selectNextProposer(prevBlockProposer, validatorSet);
+  }
+
+  private Address selectNextProposer(
+      final Address prevBlockProposer, final NavigableSet<Address> validatorSet) {
+    final SortedSet<Address> latterValidators = validatorSet.tailSet(prevBlockProposer, false);
+    if (latterValidators.isEmpty()) {
+      // i.e. prevBlockProposer was at the end of the validator list, so the right validator for
+      // the start of this round is the first.
+      return validatorSet.first();
+    } else {
+      // Else, use the first validator after the dropped entry.
+      return latterValidators.first();
+    }
   }
 }

consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/CliqueDifficultyCalculatorTest.java

@@ -16,6 +16,7 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
+import static tech.pegasys.pantheon.consensus.clique.TestHelpers.createCliqueSignedBlockHeader;
 
 import tech.pegasys.pantheon.consensus.common.VoteProposer;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
@@ -57,25 +58,30 @@ public void setup() {
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);
     cliqueProtocolContext = new ProtocolContext<>(null, null, cliqueContext);
     blockHeaderBuilder = new BlockHeaderTestFixture();
+    blockHeaderBuilder.number(1);
   }
 
   @Test
-  public void inTurnValidatorProducesDifficultyOfTwo() {
+  public void outTurnValidatorProducesDifficultyOfOne() {
+    // The proposer created the last block, so the next block must be a difficulty of 1.
     final CliqueDifficultyCalculator calculator = new CliqueDifficultyCalculator(localAddr);
 
-    final BlockHeader parentHeader = blockHeaderBuilder.number(1).buildHeader();
+    BlockHeader parentHeader =
+        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
 
     assertThat(calculator.nextDifficulty(0, parentHeader, cliqueProtocolContext))
-        .isEqualTo(BigInteger.valueOf(2));
+        .isEqualTo(BigInteger.valueOf(1));
   }
 
   @Test
-  public void outTurnValidatorProducesDifficultyOfOne() {
-    final CliqueDifficultyCalculator calculator = new CliqueDifficultyCalculator(localAddr);
+  public void inTurnValidatorCreatesDifficultyOfTwo() {
+    final CliqueDifficultyCalculator calculator =
+        new CliqueDifficultyCalculator(validatorList.get(1)); // i.e. not the proposer.
 
-    final BlockHeader parentHeader = blockHeaderBuilder.number(2).buildHeader();
+    BlockHeader parentHeader =
+        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
 
     assertThat(calculator.nextDifficulty(0, parentHeader, cliqueProtocolContext))
-        .isEqualTo(BigInteger.valueOf(1));
+        .isEqualTo(BigInteger.valueOf(2));
   }
 }

consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueBlockSchedulerTest.java

@@ -16,6 +16,7 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
+import static tech.pegasys.pantheon.consensus.clique.TestHelpers.createCliqueSignedBlockHeader;
 
 import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
@@ -37,18 +38,18 @@
 public class CliqueBlockSchedulerTest {
 
   private final KeyPair proposerKeyPair = KeyPair.generate();
-  private Address localAddr;
+  private Address localAddr = Util.publicKeyToAddress(proposerKeyPair.getPublicKey());
+  private Address otherAddr = AddressHelpers.calculateAddressWithRespectTo(localAddr, 1);
 
   private final List<Address> validatorList = Lists.newArrayList();
   private VoteTallyCache voteTallyCache;
   private BlockHeaderTestFixture blockHeaderBuilder;
 
   @Before
   public void setup() {
-    localAddr = Util.publicKeyToAddress(proposerKeyPair.getPublicKey());
 
     validatorList.add(localAddr);
-    validatorList.add(AddressHelpers.calculateAddressWithRespectTo(localAddr, 1));
+    validatorList.add(otherAddr);
 
     voteTallyCache = mock(VoteTallyCache.class);
     when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
@@ -63,12 +64,11 @@ public void inturnValidatorWaitsExactlyBlockInterval() {
     final long secondsBetweenBlocks = 5L;
     when(clock.millis()).thenReturn(currentSecondsSinceEpoch * 1000);
     final CliqueBlockScheduler scheduler =
-        new CliqueBlockScheduler(clock, voteTallyCache, localAddr, secondsBetweenBlocks);
+        new CliqueBlockScheduler(clock, voteTallyCache, otherAddr, secondsBetweenBlocks);
 
-    // There are 2 validators, therefore block 2 will put localAddr as the in-turn voter, therefore
-    // parent block should be number 1.
+    blockHeaderBuilder.number(1).timestamp(currentSecondsSinceEpoch);
     final BlockHeader parentHeader =
-        blockHeaderBuilder.number(1).timestamp(currentSecondsSinceEpoch).buildHeader();
+        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
 
     final BlockCreationTimeResult result = scheduler.getNextTimestamp(parentHeader);
 
@@ -86,10 +86,9 @@ public void outOfturnValidatorWaitsLongerThanBlockInterval() {
     final CliqueBlockScheduler scheduler =
         new CliqueBlockScheduler(clock, voteTallyCache, localAddr, secondsBetweenBlocks);
 
-    // There are 2 validators, therefore block 3 will put localAddr as the out-turn voter, therefore
-    // parent block should be number 2.
+    blockHeaderBuilder.number(2).timestamp(currentSecondsSinceEpoch);
     final BlockHeader parentHeader =
-        blockHeaderBuilder.number(2).timestamp(currentSecondsSinceEpoch).buildHeader();
+        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
 
     final BlockCreationTimeResult result = scheduler.getNextTimestamp(parentHeader);
 
@@ -105,16 +104,13 @@ public void inTurnValidatorCreatesBlockNowIFParentTimestampSufficientlyBehindNow
     final long secondsBetweenBlocks = 5L;
     when(clock.millis()).thenReturn(currentSecondsSinceEpoch * 1000);
     final CliqueBlockScheduler scheduler =
-        new CliqueBlockScheduler(clock, voteTallyCache, localAddr, secondsBetweenBlocks);
+        new CliqueBlockScheduler(clock, voteTallyCache, otherAddr, secondsBetweenBlocks);
 
     // There are 2 validators, therefore block 2 will put localAddr as the in-turn voter, therefore
     // parent block should be number 1.
+    blockHeaderBuilder.number(1).timestamp(currentSecondsSinceEpoch - secondsBetweenBlocks);
     final BlockHeader parentHeader =
-        blockHeaderBuilder
-            .number(1)
-            .timestamp(currentSecondsSinceEpoch - secondsBetweenBlocks)
-            .buildHeader();
-
+        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
     final BlockCreationTimeResult result = scheduler.getNextTimestamp(parentHeader);
 
     assertThat(result.getTimestampForHeader()).isEqualTo(currentSecondsSinceEpoch);

consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/blockcreation/CliqueProposerSelectorTest.java

@@ -17,46 +17,87 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+import tech.pegasys.pantheon.consensus.clique.TestHelpers;
 import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.AddressHelpers;
+import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.core.BlockHeaderTestFixture;
+import tech.pegasys.pantheon.ethereum.core.Util;
 
-import java.util.Arrays;
 import java.util.List;
 
+import com.google.common.collect.Lists;
 import org.junit.Before;
 import org.junit.Test;
 
 public class CliqueProposerSelectorTest {
 
+  private final KeyPair proposerKey = KeyPair.generate();
+  private final Address proposerAddress = Util.publicKeyToAddress(proposerKey.getPublicKey());
+
   private final List<Address> validatorList =
-      Arrays.asList(
-          AddressHelpers.ofValue(1),
-          AddressHelpers.ofValue(2),
-          AddressHelpers.ofValue(3),
-          AddressHelpers.ofValue(4));
+      Lists.newArrayList(
+          AddressHelpers.calculateAddressWithRespectTo(proposerAddress, -1),
+          proposerAddress,
+          AddressHelpers.calculateAddressWithRespectTo(proposerAddress, 1),
+          AddressHelpers.calculateAddressWithRespectTo(proposerAddress, 2));
   private final VoteTally voteTally = new VoteTally(validatorList);
   private VoteTallyCache voteTallyCache;
+  private final BlockHeaderTestFixture headerBuilderFixture = new BlockHeaderTestFixture();
 
   @Before
   public void setup() {
     voteTallyCache = mock(VoteTallyCache.class);
     when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(voteTally);
+
+    headerBuilderFixture.number(2);
   }
 
   @Test
-  public void proposerForABlockIsBasedOnModBlockNumber() {
+  public void firstBlockAfterGenesisIsTheSecondValidator() {
     final BlockHeaderTestFixture headerBuilderFixture = new BlockHeaderTestFixture();
+    final CliqueProposerSelector selector = new CliqueProposerSelector(voteTallyCache);
+    headerBuilderFixture.number(0);
+
+    assertThat(selector.selectProposerForNextBlock(headerBuilderFixture.buildHeader()))
+        .isEqualTo(validatorList.get(1));
+  }
+
+  @Test
+  public void selectsNextProposerInValidatorSet() {
+    final BlockHeader parentHeader =
+        TestHelpers.createCliqueSignedBlockHeader(headerBuilderFixture, proposerKey, validatorList);
+    final CliqueProposerSelector selector = new CliqueProposerSelector(voteTallyCache);
+
+    // Proposer is at index 1, so the next proposer is at index 2
+    assertThat(selector.selectProposerForNextBlock(parentHeader)).isEqualTo(validatorList.get(2));
+  }
+
+  @Test
+  public void selectsNextIndexWhenProposerIsNotInValidatorsForBlock() {
+    final BlockHeader parentHeader =
+        TestHelpers.createCliqueSignedBlockHeader(headerBuilderFixture, proposerKey, validatorList);
+    final CliqueProposerSelector selector = new CliqueProposerSelector(voteTallyCache);
+
+    validatorList.remove(proposerAddress);
+
+    // As the proposer was removed (index 1), the next proposer should also be index 1
+    assertThat(selector.selectProposerForNextBlock(parentHeader)).isEqualTo(validatorList.get(1));
+  }
+
+  @Test
+  public void singleValidatorFindsItselfAsNextProposer() {
+    final List<Address> localValidators = Lists.newArrayList(proposerAddress);
+    final VoteTally localVoteTally = new VoteTally(localValidators);
+    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(localVoteTally);
+
+    final BlockHeader parentHeader =
+        TestHelpers.createCliqueSignedBlockHeader(headerBuilderFixture, proposerKey, validatorList);
+    final CliqueProposerSelector selector = new CliqueProposerSelector(voteTallyCache);
 
-    for (int prevBlockNumber = 0; prevBlockNumber < 10; prevBlockNumber++) {
-      headerBuilderFixture.number(prevBlockNumber);
-      final CliqueProposerSelector selector = new CliqueProposerSelector(voteTallyCache);
-      final Address nextProposer =
-          selector.selectProposerForNextBlock(headerBuilderFixture.buildHeader());
-      assertThat(nextProposer)
-          .isEqualTo(validatorList.get((prevBlockNumber + 1) % validatorList.size()));
-    }
+    assertThat(selector.selectProposerForNextBlock(parentHeader)).isEqualTo(proposerAddress);
   }
 }