Revert "Repair Clique Proposer Selection" #339 - Breaks Görli testnet (…

…#343)

Reverts #339

Görli's testnet regularly has proposers miss their turn and their
genesis block actually had a bad validator specified (that was voted out
as soon as they could) So many blocks do not have the proper "proposer"
as their block generator. As a consequence the
CliqueDifficultyValidationRule fails regularly, in fact it fails at
block 3, so this should be easy to re-create locally.

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,14 +14,13 @@
 
 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.NavigableSet;
-import java.util.SortedSet;
+import java.util.ArrayList;
+import java.util.List;
 
 /**
  * Responsible for determining which member of the validator pool should create the next block.
@@ -32,7 +31,6 @@
 public class CliqueProposerSelector {
 
   private final VoteTallyCache voteTallyCache;
-  private final CliqueBlockInterface blockInterface = new CliqueBlockInterface();
 
   public CliqueProposerSelector(final VoteTallyCache voteTallyCache) {
     checkNotNull(voteTallyCache);
@@ -48,26 +46,11 @@ public CliqueProposerSelector(final VoteTallyCache voteTallyCache) {
   public Address selectProposerForNextBlock(final BlockHeader parentHeader) {
 
     final VoteTally parentVoteTally = voteTallyCache.getVoteTallyAtBlock(parentHeader);
-    final NavigableSet<Address> validatorSet = parentVoteTally.getCurrentValidators();
+    final List<Address> validatorSet = new ArrayList<>(parentVoteTally.getCurrentValidators());
 
-    Address prevBlockProposer = validatorSet.first();
-    if (parentHeader.getNumber() != BlockHeader.GENESIS_BLOCK_NUMBER) {
-      prevBlockProposer = blockInterface.getProposerOfBlock(parentHeader);
-    }
+    final long nextBlockNumber = parentHeader.getNumber() + 1L;
+    final int indexIntoValidators = (int) (nextBlockNumber % validatorSet.size());
 
-    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();
-    }
+    return validatorSet.get(indexIntoValidators);
   }
 }

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

@@ -16,7 +16,6 @@
 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;
@@ -58,30 +57,25 @@ 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 outTurnValidatorProducesDifficultyOfOne() {
-    // The proposer created the last block, so the next block must be a difficulty of 1.
+  public void inTurnValidatorProducesDifficultyOfTwo() {
     final CliqueDifficultyCalculator calculator = new CliqueDifficultyCalculator(localAddr);
 
-    BlockHeader parentHeader =
-        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
+    final BlockHeader parentHeader = blockHeaderBuilder.number(1).buildHeader();
 
     assertThat(calculator.nextDifficulty(0, parentHeader, cliqueProtocolContext))
-        .isEqualTo(BigInteger.valueOf(1));
+        .isEqualTo(BigInteger.valueOf(2));
   }
 
   @Test
-  public void inTurnValidatorCreatesDifficultyOfTwo() {
-    final CliqueDifficultyCalculator calculator =
-        new CliqueDifficultyCalculator(validatorList.get(1)); // i.e. not the proposer.
+  public void outTurnValidatorProducesDifficultyOfOne() {
+    final CliqueDifficultyCalculator calculator = new CliqueDifficultyCalculator(localAddr);
 
-    BlockHeader parentHeader =
-        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
+    final BlockHeader parentHeader = blockHeaderBuilder.number(2).buildHeader();
 
     assertThat(calculator.nextDifficulty(0, parentHeader, cliqueProtocolContext))
-        .isEqualTo(BigInteger.valueOf(2));
+        .isEqualTo(BigInteger.valueOf(1));
   }
 }

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

@@ -16,7 +16,6 @@
 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;
@@ -38,18 +37,18 @@
 public class CliqueBlockSchedulerTest {
 
   private final KeyPair proposerKeyPair = KeyPair.generate();
-  private Address localAddr = Util.publicKeyToAddress(proposerKeyPair.getPublicKey());
-  private Address otherAddr = AddressHelpers.calculateAddressWithRespectTo(localAddr, 1);
+  private Address localAddr;
 
   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(otherAddr);
+    validatorList.add(AddressHelpers.calculateAddressWithRespectTo(localAddr, 1));
 
     voteTallyCache = mock(VoteTallyCache.class);
     when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
@@ -64,11 +63,12 @@ public void inturnValidatorWaitsExactlyBlockInterval() {
     final long secondsBetweenBlocks = 5L;
     when(clock.millis()).thenReturn(currentSecondsSinceEpoch * 1000);
     final CliqueBlockScheduler scheduler =
-        new CliqueBlockScheduler(clock, voteTallyCache, otherAddr, secondsBetweenBlocks);
+        new CliqueBlockScheduler(clock, voteTallyCache, localAddr, secondsBetweenBlocks);
 
-    blockHeaderBuilder.number(1).timestamp(currentSecondsSinceEpoch);
+    // There are 2 validators, therefore block 2 will put localAddr as the in-turn voter, therefore
+    // parent block should be number 1.
     final BlockHeader parentHeader =
-        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
+        blockHeaderBuilder.number(1).timestamp(currentSecondsSinceEpoch).buildHeader();
 
     final BlockCreationTimeResult result = scheduler.getNextTimestamp(parentHeader);
 
@@ -86,9 +86,10 @@ public void outOfturnValidatorWaitsLongerThanBlockInterval() {
     final CliqueBlockScheduler scheduler =
         new CliqueBlockScheduler(clock, voteTallyCache, localAddr, secondsBetweenBlocks);
 
-    blockHeaderBuilder.number(2).timestamp(currentSecondsSinceEpoch);
+    // There are 2 validators, therefore block 3 will put localAddr as the out-turn voter, therefore
+    // parent block should be number 2.
     final BlockHeader parentHeader =
-        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
+        blockHeaderBuilder.number(2).timestamp(currentSecondsSinceEpoch).buildHeader();
 
     final BlockCreationTimeResult result = scheduler.getNextTimestamp(parentHeader);
 
@@ -104,13 +105,16 @@ public void inTurnValidatorCreatesBlockNowIFParentTimestampSufficientlyBehindNow
     final long secondsBetweenBlocks = 5L;
     when(clock.millis()).thenReturn(currentSecondsSinceEpoch * 1000);
     final CliqueBlockScheduler scheduler =
-        new CliqueBlockScheduler(clock, voteTallyCache, otherAddr, secondsBetweenBlocks);
+        new CliqueBlockScheduler(clock, voteTallyCache, localAddr, 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 =
-        createCliqueSignedBlockHeader(blockHeaderBuilder, proposerKeyPair, validatorList);
+        blockHeaderBuilder
+            .number(1)
+            .timestamp(currentSecondsSinceEpoch - secondsBetweenBlocks)
+            .buildHeader();
+
     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,87 +17,46 @@
 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 =
-      Lists.newArrayList(
-          AddressHelpers.calculateAddressWithRespectTo(proposerAddress, -1),
-          proposerAddress,
-          AddressHelpers.calculateAddressWithRespectTo(proposerAddress, 1),
-          AddressHelpers.calculateAddressWithRespectTo(proposerAddress, 2));
+      Arrays.asList(
+          AddressHelpers.ofValue(1),
+          AddressHelpers.ofValue(2),
+          AddressHelpers.ofValue(3),
+          AddressHelpers.ofValue(4));
   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 firstBlockAfterGenesisIsTheSecondValidator() {
+  public void proposerForABlockIsBasedOnModBlockNumber() {
     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);
 
-    assertThat(selector.selectProposerForNextBlock(parentHeader)).isEqualTo(proposerAddress);
+    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()));
+    }
   }
 }