Rework Clique Block Scheduler (#6)

Clique Block Scheduler has been reworked to prevent high rate blocks
being created when the parent block's timestamp is behind the system
clock.

consensus/clique/src/main/java/net/consensys/pantheon/consensus/clique/blockcreation/CliqueBlockScheduler.java

@@ -2,7 +2,7 @@ package net.consensys.pantheon.consensus.clique.blockcreation;
 
 import net.consensys.pantheon.consensus.clique.VoteTallyCache;
 import net.consensys.pantheon.consensus.common.ValidatorProvider;
-import net.consensys.pantheon.ethereum.blockcreation.BaseBlockScheduler;
+import net.consensys.pantheon.ethereum.blockcreation.DefaultBlockScheduler;
 import net.consensys.pantheon.ethereum.core.Address;
 import net.consensys.pantheon.ethereum.core.BlockHeader;
 import net.consensys.pantheon.util.time.Clock;
@@ -11,7 +11,7 @@ import java.util.Random;
 
 import com.google.common.annotations.VisibleForTesting;
 
-public class CliqueBlockScheduler extends BaseBlockScheduler {
+public class CliqueBlockScheduler extends DefaultBlockScheduler {
 
   private final int OUT_OF_TURN_DELAY_MULTIPLIER_MILLIS = 500;
 
@@ -24,7 +24,7 @@ public class CliqueBlockScheduler extends BaseBlockScheduler {
       final VoteTallyCache voteTallyCache,
       final Address localNodeAddress,
       final long secondsBetweenBlocks) {
-    super(clock);
+    super(secondsBetweenBlocks, 0L, clock);
     this.voteTallyCache = voteTallyCache;
     this.localNodeAddress = localNodeAddress;
     this.secondsBetweenBlocks = secondsBetweenBlocks;
@@ -33,24 +33,29 @@ public class CliqueBlockScheduler extends BaseBlockScheduler {
   @Override
   @VisibleForTesting
   public BlockCreationTimeResult getNextTimestamp(final BlockHeader parentHeader) {
-    final long nextHeaderTimestamp = parentHeader.getTimestamp() + secondsBetweenBlocks;
+    final BlockCreationTimeResult result = super.getNextTimestamp(parentHeader);
-    long milliSecondsUntilNextBlock = (nextHeaderTimestamp * 1000) - clock.millisecondsSinceEpoch();
 
-    final CliqueProposerSelector proposerSelector = new CliqueProposerSelector(voteTallyCache);
+    final long milliSecondsUntilNextBlock =
-    final Address nextSelector = proposerSelector.selectProposerForNextBlock(parentHeader);
+        result.getMillisecondsUntilValid() + calculateTurnBasedDelay(parentHeader);
-    if (!nextSelector.equals(localNodeAddress)) {
-      milliSecondsUntilNextBlock +=
-          calculatorOutOfTurnDelay(voteTallyCache.getVoteTallyAtBlock(parentHeader));
-    }
 
     return new BlockCreationTimeResult(
-        nextHeaderTimestamp, Math.max(0, milliSecondsUntilNextBlock));
+        result.getTimestampForHeader(), Math.max(0, milliSecondsUntilNextBlock));
+  }
+
+  private int calculateTurnBasedDelay(final BlockHeader parentHeader) {
+    final CliqueProposerSelector proposerSelector = new CliqueProposerSelector(voteTallyCache);
+    final Address nextProposer = proposerSelector.selectProposerForNextBlock(parentHeader);
+
+    if (nextProposer.equals(localNodeAddress)) {
+      return 0;
+    }
+    return calculatorOutOfTurnDelay(voteTallyCache.getVoteTallyAtBlock(parentHeader));
   }
 
   private int calculatorOutOfTurnDelay(final ValidatorProvider validators) {
     int countSigners = validators.getCurrentValidators().size();
     int maxDelay = ((countSigners / 2) + 1) * OUT_OF_TURN_DELAY_MULTIPLIER_MILLIS;
     Random r = new Random();
-    return r.nextInt((maxDelay) + 1);
+    return r.nextInt(maxDelay + 1);
   }
 }

consensus/clique/src/test/java/net/consensys/pantheon/consensus/clique/blockcreation/CliqueBlockSchedulerTest.java

@@ -8,7 +8,7 @@ import static org.mockito.Mockito.when;
 import net.consensys.pantheon.consensus.clique.VoteTallyCache;
 import net.consensys.pantheon.consensus.common.VoteTally;
 import net.consensys.pantheon.crypto.SECP256K1.KeyPair;
-import net.consensys.pantheon.ethereum.blockcreation.BaseBlockScheduler.BlockCreationTimeResult;
+import net.consensys.pantheon.ethereum.blockcreation.AbstractBlockScheduler.BlockCreationTimeResult;
 import net.consensys.pantheon.ethereum.core.Address;
 import net.consensys.pantheon.ethereum.core.AddressHelpers;
 import net.consensys.pantheon.ethereum.core.BlockHeader;

ethereum/core/src/main/java/net/consensys/pantheon/ethereum/blockcreation/AbstractBlockScheduler.java

@@ -3,11 +3,11 @@ package net.consensys.pantheon.ethereum.blockcreation;
 import net.consensys.pantheon.ethereum.core.BlockHeader;
 import net.consensys.pantheon.util.time.Clock;
 
-public abstract class BaseBlockScheduler {
+public abstract class AbstractBlockScheduler {
 
   protected final Clock clock;
 
-  public BaseBlockScheduler(final Clock clock) {
+  public AbstractBlockScheduler(final Clock clock) {
     this.clock = clock;
   }
 

ethereum/core/src/main/java/net/consensys/pantheon/ethereum/blockcreation/BlockMiner.java

@@ -32,7 +32,7 @@ public class BlockMiner<C> implements Runnable {
   private final ProtocolContext<C> protocolContext;
   private final ProtocolSchedule<C> protocolSchedule;
   private final Subscribers<MinedBlockObserver> observers;
-  private final BaseBlockScheduler scheduler;
+  private final AbstractBlockScheduler scheduler;
   private final BlockHeader parentHeader;
 
   public BlockMiner(
@@ -40,7 +40,7 @@ public class BlockMiner<C> implements Runnable {
       final ProtocolSchedule<C> protocolSchedule,
       final ProtocolContext<C> protocolContext,
       final Subscribers<MinedBlockObserver> observers,
-      final BaseBlockScheduler scheduler,
+      final AbstractBlockScheduler scheduler,
       final BlockHeader parentHeader) {
     this.blockCreator = blockCreator;
     this.protocolContext = protocolContext;

ethereum/core/src/main/java/net/consensys/pantheon/ethereum/blockcreation/DefaultBlockScheduler.java

@@ -7,7 +7,7 @@ import java.util.concurrent.TimeUnit;
 
 import com.google.common.annotations.VisibleForTesting;
 
-public class DefaultBlockScheduler extends BaseBlockScheduler {
+public class DefaultBlockScheduler extends AbstractBlockScheduler {
 
   private final long acceptableClockDriftSeconds;
   private final long minimumSecondsSinceParent;
@@ -25,16 +25,14 @@ public class DefaultBlockScheduler extends BaseBlockScheduler {
   @VisibleForTesting
   public BlockCreationTimeResult getNextTimestamp(final BlockHeader parentHeader) {
     final long msSinceEpoch = clock.millisecondsSinceEpoch();
-    final long secondsSinceEpoch = TimeUnit.SECONDS.convert(msSinceEpoch, TimeUnit.MILLISECONDS);
+    final long now = TimeUnit.SECONDS.convert(msSinceEpoch, TimeUnit.MILLISECONDS);
     final long parentTimestamp = parentHeader.getTimestamp();
 
-    final long nextHeaderTimestamp =
+    final long nextHeaderTimestamp = Long.max(parentTimestamp + minimumSecondsSinceParent, now);
-        Long.max(parentTimestamp + minimumSecondsSinceParent, secondsSinceEpoch);
 
-    final long millisecondsUntilHeaderTimeStampIsValid =
+    final long earliestBlockTransmissionTime = nextHeaderTimestamp - acceptableClockDriftSeconds;
-        (nextHeaderTimestamp * 1000) - (msSinceEpoch + (acceptableClockDriftSeconds * 1000));
+    final long msUntilBlocKTransmission = (earliestBlockTransmissionTime * 1000) - msSinceEpoch;
 
-    return new BlockCreationTimeResult(
+    return new BlockCreationTimeResult(nextHeaderTimestamp, Math.max(0, msUntilBlocKTransmission));
-        nextHeaderTimestamp, Math.max(0, millisecondsUntilHeaderTimeStampIsValid));
   }
 }

ethereum/core/src/main/java/net/consensys/pantheon/ethereum/blockcreation/EthHashBlockMiner.java

@@ -27,7 +27,7 @@ public class EthHashBlockMiner extends BlockMiner<Void> {
       final ProtocolSchedule<Void> protocolSchedule,
       final ProtocolContext<Void> protocolContext,
       final Subscribers<MinedBlockObserver> observers,
-      final BaseBlockScheduler scheduler,
+      final AbstractBlockScheduler scheduler,
       final BlockHeader parentHeader) {
     super(blockCreator, protocolSchedule, protocolContext, observers, scheduler, parentHeader);
     this.blockCreator = blockCreator;

ethereum/core/src/main/java/net/consensys/pantheon/ethereum/blockcreation/EthHashMinerExecutor.java

@@ -25,7 +25,7 @@ public class EthHashMinerExecutor {
   private volatile BytesValue extraData;
   private volatile Optional<Address> coinbase;
   private volatile Wei minTransactionGasPrice;
-  private final BaseBlockScheduler blockScheduler;
+  private final AbstractBlockScheduler blockScheduler;
 
   public EthHashMinerExecutor(
       final ProtocolContext<Void> protocolContext,
@@ -33,7 +33,7 @@ public class EthHashMinerExecutor {
       final ProtocolSchedule<Void> protocolSchedule,
       final PendingTransactions pendingTransactions,
       final MiningParameters miningParams,
-      final BaseBlockScheduler blockScheduler) {
+      final AbstractBlockScheduler blockScheduler) {
     this.protocolContext = protocolContext;
     this.executorService = executorService;
     this.protocolSchedule = protocolSchedule;

ethereum/core/src/test/java/net/consensys/pantheon/ethereum/blockcreation/DefaultBlockSchedulerTest.java

@@ -4,7 +4,7 @@ import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-import net.consensys.pantheon.ethereum.blockcreation.BaseBlockScheduler.BlockCreationTimeResult;
+import net.consensys.pantheon.ethereum.blockcreation.AbstractBlockScheduler.BlockCreationTimeResult;
 import net.consensys.pantheon.ethereum.core.BlockHeader;
 import net.consensys.pantheon.ethereum.core.BlockHeaderTestFixture;
 import net.consensys.pantheon.util.time.Clock;