Repair VoteTallyCache incorrectly applying vote to parent (#360)

A problem was identified whereby the vote in a child block was applied to the parent, this was resolved by
duplicating the parent VoteTally before modifying it, and injecting to the child block.

Some refactoring was also conducted to make VoteTally logic simpler.

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

@@ -38,9 +38,9 @@ public static boolean addressIsAllowedToProduceNextBlock(
       final ProtocolContext<CliqueContext> protocolContext,
       final BlockHeader parent) {
     final VoteTally validatorProvider =
-        protocolContext.getConsensusState().getVoteTallyCache().getVoteTallyAtBlock(parent);
+        protocolContext.getConsensusState().getVoteTallyCache().getVoteTallyAfterBlock(parent);
 
-    if (!validatorProvider.getCurrentValidators().contains(candidate)) {
+    if (!validatorProvider.getValidators().contains(candidate)) {
       return false;
     }
 
@@ -72,7 +72,7 @@ public static boolean addressIsAllowedToProduceNextBlock(
   }
 
   private static int minimumBlocksSincePreviousSigning(final ValidatorProvider validatorProvider) {
-    final int validatorCount = validatorProvider.getCurrentValidators().size();
+    final int validatorCount = validatorProvider.getValidators().size();
     // The number of contiguous blocks in which a signer may only sign 1 (as taken from clique spec)
     final int signerLimit = (validatorCount / 2) + 1;
     return signerLimit - 1;

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

@@ -50,7 +50,18 @@ public VoteTallyCache(
     this.epochManager = epochManager;
   }
 
-  public VoteTally getVoteTallyAtBlock(final BlockHeader header) {
+  /**
+   * Determines the VoteTally for a given block header, by back-tracing the blockchain to a
+   * previously cached value or epoch block. Then appyling votes in each intermediate header such
+   * that representative state can be provided. This function assumes the vote cast in {@code
+   * header} is applied, thus the voteTally returned contains the group of validators who are
+   * permitted to partake in the next block's creation.
+   *
+   * @param header the header of the block after which the VoteTally is to be returned
+   * @return The Vote Tally (and therefore validators) following the application of all votes upto
+   *     and including the requested header.
+   */
+  public VoteTally getVoteTallyAfterBlock(final BlockHeader header) {
     try {
       return voteTallyCache.get(header.getHash(), () -> populateCacheUptoAndIncluding(header));
     } catch (final ExecutionException ex) {
@@ -64,7 +75,8 @@ private VoteTally populateCacheUptoAndIncluding(final BlockHeader start) {
     VoteTally voteTally = null;
 
     while (true) { // Will run into an epoch block (and thus a VoteTally) to break loop.
-      voteTally = findMostRecentAvailableVoteTally(header, intermediateBlocks);
+      intermediateBlocks.push(header);
+      voteTally = getValidatorsAfter(header);
       if (voteTally != null) {
         break;
       }
@@ -80,25 +92,23 @@ private VoteTally populateCacheUptoAndIncluding(final BlockHeader start) {
     return constructMissingCacheEntries(intermediateBlocks, voteTally);
   }
 
-  private VoteTally findMostRecentAvailableVoteTally(
-      final BlockHeader header, final Deque<BlockHeader> intermediateBlockHeaders) {
-    intermediateBlockHeaders.push(header);
-    VoteTally voteTally = voteTallyCache.getIfPresent(header.getParentHash());
-    if ((voteTally == null) && (epochManager.isEpochBlock(header.getNumber()))) {
-      final CliqueExtraData extraData = CliqueExtraData.decode(header.getExtraData());
-      voteTally = new VoteTally(extraData.getValidators());
+  private VoteTally getValidatorsAfter(final BlockHeader header) {
+    if (epochManager.isEpochBlock(header.getNumber())) {
+      final CliqueBlockInterface blockInterface = new CliqueBlockInterface();
+      return new VoteTally(blockInterface.validatorsInBlock(header));
     }
 
-    return voteTally;
+    return voteTallyCache.getIfPresent(header.getParentHash());
   }
 
   private VoteTally constructMissingCacheEntries(
       final Deque<BlockHeader> headers, final VoteTally tally) {
+    final VoteTally mutableVoteTally = tally.copy();
     while (!headers.isEmpty()) {
       final BlockHeader h = headers.pop();
-      voteTallyUpdater.updateForBlock(h, tally);
-      voteTallyCache.put(h.getHash(), tally.copy());
+      voteTallyUpdater.updateForBlock(h, mutableVoteTally);
+      voteTallyCache.put(h.getHash(), mutableVoteTally.copy());
     }
-    return tally;
+    return mutableVoteTally;
   }
 }

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

@@ -85,7 +85,8 @@ protected BlockHeader createFinalBlockHeader(final SealableBlockHeader sealableB
             .blockHashFunction(blockHashFunction);
 
     final CliqueContext cliqueContext = protocolContext.getConsensusState();
-    final VoteTally voteTally = cliqueContext.getVoteTallyCache().getVoteTallyAtBlock(parentHeader);
+    final VoteTally voteTally =
+        cliqueContext.getVoteTallyCache().getVoteTallyAfterBlock(parentHeader);
 
     final Optional<ValidatorVote> vote =
         cliqueContext

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

@@ -60,11 +60,11 @@ private int calculateTurnBasedDelay(final BlockHeader parentHeader) {
     if (nextProposer.equals(localNodeAddress)) {
       return 0;
     }
-    return calculatorOutOfTurnDelay(voteTallyCache.getVoteTallyAtBlock(parentHeader));
+    return calculatorOutOfTurnDelay(voteTallyCache.getVoteTallyAfterBlock(parentHeader));
   }
 
   private int calculatorOutOfTurnDelay(final ValidatorProvider validators) {
-    final int countSigners = validators.getCurrentValidators().size();
+    final int countSigners = validators.getValidators().size();
     final double multiplier = (countSigners / 2d) + 1;
     final int maxDelay = (int) (multiplier * OUT_OF_TURN_DELAY_MULTIPLIER_MILLIS);
     return r.nextInt(maxDelay) + 1;

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

@@ -101,8 +101,11 @@ public BytesValue calculateExtraData(final BlockHeader parentHeader) {
     // Building ON TOP of canonical head, if the next block is epoch, include validators.
     if (epochManager.isEpochBlock(parentHeader.getNumber() + 1)) {
       final VoteTally voteTally =
-          protocolContext.getConsensusState().getVoteTallyCache().getVoteTallyAtBlock(parentHeader);
-      validators.addAll(voteTally.getCurrentValidators());
+          protocolContext
+              .getConsensusState()
+              .getVoteTallyCache()
+              .getVoteTallyAfterBlock(parentHeader);
+      validators.addAll(voteTally.getValidators());
     }
 
     final CliqueExtraData extraData = new CliqueExtraData(vanityDataToInsert, null, validators);

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

@@ -45,8 +45,8 @@ 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 VoteTally parentVoteTally = voteTallyCache.getVoteTallyAfterBlock(parentHeader);
+    final List<Address> validatorSet = new ArrayList<>(parentVoteTally.getValidators());
 
     final long nextBlockNumber = parentHeader.getNumber() + 1L;
     final int indexIntoValidators = (int) (nextBlockNumber % validatorSet.size());

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/headervalidationrules/CliqueExtraDataValidationRule.java

@@ -59,9 +59,9 @@ public boolean validate(
       final ProtocolContext<CliqueContext> protocolContext) {
     try {
       final VoteTally validatorProvider =
-          protocolContext.getConsensusState().getVoteTallyCache().getVoteTallyAtBlock(parent);
+          protocolContext.getConsensusState().getVoteTallyCache().getVoteTallyAfterBlock(parent);
 
-      final Collection<Address> storedValidators = validatorProvider.getCurrentValidators();
+      final Collection<Address> storedValidators = validatorProvider.getValidators();
       return extraDataIsValid(storedValidators, header);
 
     } catch (final RLPException ex) {

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/jsonrpc/methods/CliqueGetSigners.java

@@ -53,7 +53,7 @@ public String getName() {
   public JsonRpcResponse response(final JsonRpcRequest request) {
     final Optional<BlockHeader> blockHeader = determineBlockHeader(request);
     return blockHeader
-        .map(bh -> voteTallyCache.getVoteTallyAtBlock(bh).getCurrentValidators())
+        .map(bh -> voteTallyCache.getVoteTallyAfterBlock(bh).getValidators())
         .map(addresses -> addresses.stream().map(Objects::toString).collect(Collectors.toList()))
         .<JsonRpcResponse>map(addresses -> new JsonRpcSuccessResponse(request.getId(), addresses))
         .orElse(new JsonRpcErrorResponse(request.getId(), JsonRpcError.INTERNAL_ERROR));

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/jsonrpc/methods/CliqueGetSignersAtHash.java

@@ -53,7 +53,7 @@ public String getName() {
   public JsonRpcResponse response(final JsonRpcRequest request) {
     final Optional<BlockHeader> blockHeader = determineBlockHeader(request);
     return blockHeader
-        .map(bh -> voteTallyCache.getVoteTallyAtBlock(bh).getCurrentValidators())
+        .map(bh -> voteTallyCache.getVoteTallyAfterBlock(bh).getValidators())
         .map(addresses -> addresses.stream().map(Objects::toString).collect(Collectors.toList()))
         .<JsonRpcResponse>map(addresses -> new JsonRpcSuccessResponse(request.getId(), addresses))
         .orElse(new JsonRpcErrorResponse(request.getId(), JsonRpcError.INTERNAL_ERROR));

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

@@ -51,7 +51,7 @@ public void setup() {
     validatorList.add(AddressHelpers.calculateAddressWithRespectTo(localAddr, 1));
 
     final VoteTallyCache voteTallyCache = mock(VoteTallyCache.class);
-    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(new VoteTally(validatorList));
     final VoteProposer voteProposer = new VoteProposer();
 
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);

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

@@ -73,7 +73,7 @@ public void networkWithOneValidatorIsAllowedToCreateConsecutiveBlocks() {
     blockChain = createInMemoryBlockchain(genesisBlock);
 
     final VoteTallyCache voteTallyCache = mock(VoteTallyCache.class);
-    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(new VoteTally(validatorList));
     final VoteProposer voteProposer = new VoteProposer();
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);
     cliqueProtocolContext = new ProtocolContext<>(blockChain, null, cliqueContext);
@@ -98,7 +98,7 @@ public void networkWithTwoValidatorsIsAllowedToProduceBlockIfNotPreviousBlockPro
     blockChain = createInMemoryBlockchain(genesisBlock);
 
     final VoteTallyCache voteTallyCache = mock(VoteTallyCache.class);
-    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(new VoteTally(validatorList));
     final VoteProposer voteProposer = new VoteProposer();
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);
     cliqueProtocolContext = new ProtocolContext<>(blockChain, null, cliqueContext);
@@ -132,7 +132,7 @@ public void networkWithTwoValidatorsIsNotAllowedToProduceBlockIfIsPreviousBlockP
     blockChain = createInMemoryBlockchain(genesisBlock);
 
     final VoteTallyCache voteTallyCache = mock(VoteTallyCache.class);
-    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(new VoteTally(validatorList));
     final VoteProposer voteProposer = new VoteProposer();
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);
     cliqueProtocolContext = new ProtocolContext<>(blockChain, null, cliqueContext);
@@ -162,7 +162,7 @@ public void withThreeValidatorsMustHaveOneBlockBetweenSignings() {
     blockChain = createInMemoryBlockchain(genesisBlock);
 
     final VoteTallyCache voteTallyCache = mock(VoteTallyCache.class);
-    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(new VoteTally(validatorList));
     final VoteProposer voteProposer = new VoteProposer();
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);
     cliqueProtocolContext = new ProtocolContext<>(blockChain, null, cliqueContext);
@@ -207,7 +207,7 @@ public void signerIsValidIfInsufficientBlocksExistInHistory() {
     blockChain = createInMemoryBlockchain(genesisBlock);
 
     final VoteTallyCache voteTallyCache = mock(VoteTallyCache.class);
-    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(new VoteTally(validatorList));
     final VoteProposer voteProposer = new VoteProposer();
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);
     cliqueProtocolContext = new ProtocolContext<>(blockChain, null, cliqueContext);
@@ -236,7 +236,7 @@ public void exceptionIsThrownIfOnAnOrphanedChain() {
     blockChain = createInMemoryBlockchain(genesisBlock);
 
     final VoteTallyCache voteTallyCache = mock(VoteTallyCache.class);
-    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(new VoteTally(validatorList));
     final VoteProposer voteProposer = new VoteProposer();
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);
     cliqueProtocolContext = new ProtocolContext<>(blockChain, null, cliqueContext);
@@ -260,7 +260,7 @@ public void nonValidatorIsNotAllowedToCreateABlock() {
     blockChain = createInMemoryBlockchain(genesisBlock);
 
     final VoteTallyCache voteTallyCache = mock(VoteTallyCache.class);
-    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(new VoteTally(validatorList));
     final VoteProposer voteProposer = new VoteProposer();
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);
     cliqueProtocolContext = new ProtocolContext<>(blockChain, null, cliqueContext);

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

@@ -20,12 +20,17 @@
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.verifyZeroInteractions;
+import static org.mockito.Mockito.when;
+import static tech.pegasys.pantheon.consensus.common.VoteType.DROP;
 import static tech.pegasys.pantheon.ethereum.core.InMemoryStorageProvider.createInMemoryBlockchain;
 
 import tech.pegasys.pantheon.consensus.common.EpochManager;
+import tech.pegasys.pantheon.consensus.common.ValidatorVote;
+import tech.pegasys.pantheon.consensus.common.VoteTally;
 import tech.pegasys.pantheon.consensus.common.VoteTallyUpdater;
 import tech.pegasys.pantheon.crypto.SECP256K1.Signature;
 import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain;
+import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.AddressHelpers;
 import tech.pegasys.pantheon.ethereum.core.Block;
 import tech.pegasys.pantheon.ethereum.core.BlockBody;
@@ -36,6 +41,8 @@
 
 import java.math.BigInteger;
 import java.util.Arrays;
+import java.util.List;
+import java.util.Optional;
 
 import com.google.common.util.concurrent.UncheckedExecutionException;
 import org.assertj.core.util.Lists;
@@ -58,13 +65,18 @@ private Block createEmptyBlock(final long blockNumber, final Hash parentHash) {
   private Block block_1;
   private Block block_2;
 
+  private final List<Address> validators = Lists.newArrayList();
+
   @Before
   public void constructThreeBlockChain() {
+    for (int i = 0; i < 3; i++) {
+      validators.add(AddressHelpers.ofValue(i));
+    }
     headerBuilder.extraData(
         new CliqueExtraData(
                 BytesValue.wrap(new byte[32]),
                 Signature.create(BigInteger.TEN, BigInteger.TEN, (byte) 1),
-                Lists.emptyList())
+                validators)
             .encode());
 
     genesisBlock = createEmptyBlock(0, Hash.ZERO);
@@ -87,7 +99,7 @@ public void parentBlockVoteTallysAreCachedWhenChildVoteTallyRequested() {
     // The votetallyUpdater should be invoked for the requested block, and all parents including
     // the epoch (genesis) block.
     final ArgumentCaptor<BlockHeader> varArgs = ArgumentCaptor.forClass(BlockHeader.class);
-    cache.getVoteTallyAtBlock(block_2.getHeader());
+    cache.getVoteTallyAfterBlock(block_2.getHeader());
     verify(tallyUpdater, times(3)).updateForBlock(varArgs.capture(), any());
     assertThat(varArgs.getAllValues())
         .isEqualTo(
@@ -97,10 +109,10 @@ public void parentBlockVoteTallysAreCachedWhenChildVoteTallyRequested() {
 
     // Requesting the vote tally to the parent block should not invoke the voteTallyUpdater as the
     // vote tally was cached from previous operation.
-    cache.getVoteTallyAtBlock(block_1.getHeader());
+    cache.getVoteTallyAfterBlock(block_1.getHeader());
     verifyZeroInteractions(tallyUpdater);
 
-    cache.getVoteTallyAtBlock(block_2.getHeader());
+    cache.getVoteTallyAfterBlock(block_2.getHeader());
     verifyZeroInteractions(tallyUpdater);
   }
 
@@ -113,7 +125,7 @@ public void exceptionThrownIfNoParentBlockExists() {
     final Block orphanBlock = createEmptyBlock(4, Hash.ZERO);
 
     assertThatExceptionOfType(UncheckedExecutionException.class)
-        .isThrownBy(() -> cache.getVoteTallyAtBlock(orphanBlock.getHeader()))
+        .isThrownBy(() -> cache.getVoteTallyAfterBlock(orphanBlock.getHeader()))
         .withMessageContaining(
             "Supplied block was on a orphaned chain, unable to generate " + "VoteTally.");
   }
@@ -125,19 +137,47 @@ public void walkBackStopsWhenACachedVoteTallyIsFound() {
         new VoteTallyCache(blockChain, tallyUpdater, new EpochManager(30_000));
 
     // Load the Cache up to block_2
-    cache.getVoteTallyAtBlock(block_2.getHeader());
+    cache.getVoteTallyAfterBlock(block_2.getHeader());
 
     reset(tallyUpdater);
 
     // Append new blocks to the chain, and ensure the walkback only goes as far as block_2.
     final Block block_3 = createEmptyBlock(4, block_2.getHeader().getHash());
     // Load the Cache up to block_2
-    cache.getVoteTallyAtBlock(block_3.getHeader());
+    cache.getVoteTallyAfterBlock(block_3.getHeader());
 
     // The votetallyUpdater should be invoked for the requested block, and all parents including
     // the epoch (genesis) block.
     final ArgumentCaptor<BlockHeader> varArgs = ArgumentCaptor.forClass(BlockHeader.class);
     verify(tallyUpdater, times(1)).updateForBlock(varArgs.capture(), any());
     assertThat(varArgs.getAllValues()).isEqualTo(Arrays.asList(block_3.getHeader()));
   }
+
+  // A bug was identified in VoteTallyCache whereby a vote cast in the next block *could* be applied
+  // to the parent block (depending on cache creation ordering). This test ensure the problem is
+  // resolved.
+  @Test
+  public void integrationTestingVotesBeingApplied() {
+    final EpochManager epochManager = new EpochManager(30_000);
+    final CliqueBlockInterface blockInterface = mock(CliqueBlockInterface.class);
+    final VoteTallyUpdater tallyUpdater = new VoteTallyUpdater(epochManager, blockInterface);
+
+    when(blockInterface.extractVoteFromHeader(block_1.getHeader()))
+        .thenReturn(Optional.of(new ValidatorVote(DROP, validators.get(0), validators.get(2))));
+
+    when(blockInterface.extractVoteFromHeader(block_2.getHeader()))
+        .thenReturn(Optional.of(new ValidatorVote(DROP, validators.get(1), validators.get(2))));
+
+    final VoteTallyCache cache = new VoteTallyCache(blockChain, tallyUpdater, epochManager);
+
+    VoteTally voteTally = cache.getVoteTallyAfterBlock(block_1.getHeader());
+    assertThat(voteTally.getValidators()).containsAll(validators);
+
+    voteTally = cache.getVoteTallyAfterBlock(block_2.getHeader());
+
+    assertThat(voteTally.getValidators()).containsExactly(validators.get(0), validators.get(1));
+
+    voteTally = cache.getVoteTallyAfterBlock(block_1.getHeader());
+    assertThat(voteTally.getValidators()).containsAll(validators);
+  }
 }

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

@@ -75,7 +75,7 @@ public void setup() {
     validatorList.add(otherAddress);
 
     final VoteTallyCache voteTallyCache = mock(VoteTallyCache.class);
-    when(voteTallyCache.getVoteTallyAtBlock(any())).thenReturn(new VoteTally(validatorList));
+    when(voteTallyCache.getVoteTallyAfterBlock(any())).thenReturn(new VoteTally(validatorList));
     voteProposer = new VoteProposer();
     final CliqueContext cliqueContext = new CliqueContext(voteTallyCache, voteProposer, null);