IBFT to use VoteTallyCache (#907)

It was identified during a demonstration that Pantheon, when running
in IBFT would show a "Bad Block Import" when a validator was added or
removed from the validator pool.

It was determined this was due to IBFT maintaining a single, 'global'
copy of the curent list of validators, which was updated when a block
was imported - thus when a block which had been imported vi IBFT
was then received via Eth block propogation, the validator list would
not align with the global list (as it had been updated in the IBFT
import).

The solution has been to utilise the VoteTallyCache as used in the
Clique implementation.

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

@@ -14,6 +14,7 @@
 
 import tech.pegasys.pantheon.consensus.common.EpochManager;
 import tech.pegasys.pantheon.consensus.common.VoteProposer;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 
 /**
  * Holds the data which lives "in parallel" with the importation of blocks etc. when using the

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

@@ -15,6 +15,7 @@
 import tech.pegasys.pantheon.consensus.clique.blockcreation.CliqueProposerSelector;
 import tech.pegasys.pantheon.consensus.common.ValidatorProvider;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.chain.Blockchain;
 import tech.pegasys.pantheon.ethereum.core.Address;

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

@@ -12,8 +12,8 @@
  */
 package tech.pegasys.pantheon.consensus.clique.blockcreation;
 
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.ValidatorProvider;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.ethereum.blockcreation.DefaultBlockScheduler;
 import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;

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

@@ -14,8 +14,8 @@
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 

consensus/clique/src/main/java/tech/pegasys/pantheon/consensus/clique/jsonrpc/CliqueJsonRpcMethodsFactory.java

@@ -14,14 +14,14 @@
 
 import tech.pegasys.pantheon.consensus.clique.CliqueBlockInterface;
 import tech.pegasys.pantheon.consensus.clique.CliqueContext;
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.clique.jsonrpc.methods.CliqueGetSigners;
 import tech.pegasys.pantheon.consensus.clique.jsonrpc.methods.CliqueGetSignersAtHash;
 import tech.pegasys.pantheon.consensus.clique.jsonrpc.methods.CliqueProposals;
 import tech.pegasys.pantheon.consensus.clique.jsonrpc.methods.Discard;
 import tech.pegasys.pantheon.consensus.clique.jsonrpc.methods.Propose;
 import tech.pegasys.pantheon.consensus.common.EpochManager;
 import tech.pegasys.pantheon.consensus.common.VoteProposer;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteTallyUpdater;
 import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain;
@@ -73,6 +73,7 @@ private VoteTallyCache createVoteTallyCache(
     final EpochManager epochManager = context.getConsensusState().getEpochManager();
     final VoteTallyUpdater voteTallyUpdater =
         new VoteTallyUpdater(epochManager, new CliqueBlockInterface());
-    return new VoteTallyCache(blockchain, voteTallyUpdater, epochManager);
+    return new VoteTallyCache(
+        blockchain, voteTallyUpdater, epochManager, new CliqueBlockInterface());
   }
 }

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

@@ -12,7 +12,7 @@
  */
 package tech.pegasys.pantheon.consensus.clique.jsonrpc.methods;
 
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.jsonrpc.internal.JsonRpcRequest;
 import tech.pegasys.pantheon.ethereum.jsonrpc.internal.methods.JsonRpcMethod;

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

@@ -12,7 +12,7 @@
  */
 package tech.pegasys.pantheon.consensus.clique.jsonrpc.methods;
 
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.core.Hash;
 import tech.pegasys.pantheon.ethereum.jsonrpc.internal.JsonRpcRequest;

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

@@ -19,6 +19,7 @@
 
 import tech.pegasys.pantheon.consensus.common.VoteProposer;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.core.Address;

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

@@ -22,6 +22,7 @@
 import tech.pegasys.pantheon.consensus.clique.headervalidationrules.SignerRateLimitValidationRule;
 import tech.pegasys.pantheon.consensus.common.VoteProposer;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain;

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

@@ -26,10 +26,10 @@
 import tech.pegasys.pantheon.consensus.clique.CliqueHelpers;
 import tech.pegasys.pantheon.consensus.clique.CliqueProtocolSchedule;
 import tech.pegasys.pantheon.consensus.clique.TestHelpers;
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.EpochManager;
 import tech.pegasys.pantheon.consensus.common.VoteProposer;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.chain.GenesisState;

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

@@ -17,8 +17,8 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.ethereum.blockcreation.AbstractBlockScheduler.BlockCreationTimeResult;
 import tech.pegasys.pantheon.ethereum.core.Address;

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

@@ -22,10 +22,10 @@
 import tech.pegasys.pantheon.consensus.clique.CliqueContext;
 import tech.pegasys.pantheon.consensus.clique.CliqueExtraData;
 import tech.pegasys.pantheon.consensus.clique.CliqueProtocolSchedule;
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.EpochManager;
 import tech.pegasys.pantheon.consensus.common.VoteProposer;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.core.Address;

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

@@ -25,8 +25,8 @@
 import tech.pegasys.pantheon.consensus.clique.CliqueContext;
 import tech.pegasys.pantheon.consensus.clique.CliqueMiningTracker;
 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.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.chain.MutableBlockchain;

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

@@ -17,8 +17,8 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.AddressHelpers;
 import tech.pegasys.pantheon.ethereum.core.BlockHeaderTestFixture;

consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/headervalidationrules/CliqueDifficultyValidationRuleTest.java

@@ -20,9 +20,9 @@
 import tech.pegasys.pantheon.consensus.clique.CliqueBlockHashing;
 import tech.pegasys.pantheon.consensus.clique.CliqueContext;
 import tech.pegasys.pantheon.consensus.clique.CliqueExtraData;
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteProposer;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.crypto.SECP256K1;
 import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.crypto.SECP256K1.Signature;

consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/headervalidationrules/CliqueExtraDataValidationRuleTest.java

@@ -20,9 +20,9 @@
 import tech.pegasys.pantheon.consensus.clique.CliqueContext;
 import tech.pegasys.pantheon.consensus.clique.CliqueExtraData;
 import tech.pegasys.pantheon.consensus.clique.TestHelpers;
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.EpochManager;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.crypto.SECP256K1.KeyPair;
 import tech.pegasys.pantheon.ethereum.ProtocolContext;
 import tech.pegasys.pantheon.ethereum.core.Address;

consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/jsonrpc/methods/CliqueGetSignersAtHashTest.java

@@ -19,8 +19,8 @@
 import static org.mockito.Mockito.when;
 import static tech.pegasys.pantheon.ethereum.core.Address.fromHexString;
 
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.core.BlockHeaderTestFixture;

consensus/clique/src/test/java/tech/pegasys/pantheon/consensus/clique/jsonrpc/methods/CliqueGetSignersTest.java

@@ -19,8 +19,8 @@
 import static org.mockito.Mockito.when;
 import static tech.pegasys.pantheon.ethereum.core.Address.fromHexString;
 
-import tech.pegasys.pantheon.consensus.clique.VoteTallyCache;
 import tech.pegasys.pantheon.consensus.common.VoteTally;
+import tech.pegasys.pantheon.consensus.common.VoteTallyCache;
 import tech.pegasys.pantheon.ethereum.core.Address;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.core.BlockHeaderTestFixture;

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

@@ -10,13 +10,10 @@
  * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
  * specific language governing permissions and limitations under the License.
  */
-package tech.pegasys.pantheon.consensus.clique;
+package tech.pegasys.pantheon.consensus.common;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import tech.pegasys.pantheon.consensus.common.EpochManager;
-import tech.pegasys.pantheon.consensus.common.VoteTally;
-import tech.pegasys.pantheon.consensus.common.VoteTallyUpdater;
 import tech.pegasys.pantheon.ethereum.chain.Blockchain;
 import tech.pegasys.pantheon.ethereum.core.BlockHeader;
 import tech.pegasys.pantheon.ethereum.core.Hash;
@@ -37,17 +34,26 @@ public class VoteTallyCache {
 
   private final Cache<Hash, VoteTally> voteTallyCache =
       CacheBuilder.newBuilder().maximumSize(100).build();
+  private BlockInterface blockInterface;
 
   public VoteTallyCache(
       final Blockchain blockchain,
       final VoteTallyUpdater voteTallyUpdater,
-      final EpochManager epochManager) {
+      final EpochManager epochManager,
+      final BlockInterface blockInterface) {
+
     checkNotNull(blockchain);
     checkNotNull(voteTallyUpdater);
     checkNotNull(epochManager);
+    checkNotNull(blockInterface);
     this.blockchain = blockchain;
     this.voteTallyUpdater = voteTallyUpdater;
     this.epochManager = epochManager;
+    this.blockInterface = blockInterface;
+  }
+
+  public VoteTally getVoteTallyAtHead() {
+    return getVoteTallyAfterBlock(blockchain.getChainHeadHeader());
   }
 
   /**
@@ -94,7 +100,6 @@ private VoteTally populateCacheUptoAndIncluding(final BlockHeader start) {
 
   private VoteTally getValidatorsAfter(final BlockHeader header) {
     if (epochManager.isEpochBlock(header.getNumber())) {
-      final CliqueBlockInterface blockInterface = new CliqueBlockInterface();
       return new VoteTally(blockInterface.validatorsInBlock(header));
     }
 

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

@@ -10,7 +10,7 @@
  * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
  * specific language governing permissions and limitations under the License.
  */
-package tech.pegasys.pantheon.consensus.clique;
+package tech.pegasys.pantheon.consensus.common;
 
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
@@ -24,11 +24,6 @@
 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;
@@ -39,7 +34,6 @@
 import tech.pegasys.pantheon.ethereum.core.Hash;
 import tech.pegasys.pantheon.util.bytes.BytesValue;
 
-import java.math.BigInteger;
 import java.util.Arrays;
 import java.util.List;
 import java.util.Optional;
@@ -52,32 +46,29 @@
 
 public class VoteTallyCacheTest {
 
-  BlockHeaderTestFixture headerBuilder = new BlockHeaderTestFixture();
+  private final BlockHeaderTestFixture headerBuilder = new BlockHeaderTestFixture();
 
   private Block createEmptyBlock(final long blockNumber, final Hash parentHash) {
     headerBuilder.number(blockNumber).parentHash(parentHash).coinbase(AddressHelpers.ofValue(0));
     return new Block(
         headerBuilder.buildHeader(), new BlockBody(Lists.emptyList(), Lists.emptyList()));
   }
 
-  MutableBlockchain blockChain;
+  private MutableBlockchain blockChain;
   private Block genesisBlock;
   private Block block_1;
   private Block block_2;
 
   private final List<Address> validators = Lists.newArrayList();
 
+  private final BlockInterface blockInterface = mock(BlockInterface.class);
+
   @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),
-                validators)
-            .encode());
+    headerBuilder.extraData(BytesValue.wrap(new byte[32]));
 
     genesisBlock = createEmptyBlock(0, Hash.ZERO);
 
@@ -88,13 +79,15 @@ public void constructThreeBlockChain() {
 
     blockChain.appendBlock(block_1, Lists.emptyList());
     blockChain.appendBlock(block_2, Lists.emptyList());
+
+    when(blockInterface.validatorsInBlock(any())).thenReturn(validators);
   }
 
   @Test
   public void parentBlockVoteTallysAreCachedWhenChildVoteTallyRequested() {
     final VoteTallyUpdater tallyUpdater = mock(VoteTallyUpdater.class);
     final VoteTallyCache cache =
-        new VoteTallyCache(blockChain, tallyUpdater, new EpochManager(30_000));
+        new VoteTallyCache(blockChain, tallyUpdater, new EpochManager(30_000), blockInterface);
 
     // The votetallyUpdater should be invoked for the requested block, and all parents including
     // the epoch (genesis) block.
@@ -120,7 +113,7 @@ public void parentBlockVoteTallysAreCachedWhenChildVoteTallyRequested() {
   public void exceptionThrownIfNoParentBlockExists() {
     final VoteTallyUpdater tallyUpdater = mock(VoteTallyUpdater.class);
     final VoteTallyCache cache =
-        new VoteTallyCache(blockChain, tallyUpdater, new EpochManager(30_000));
+        new VoteTallyCache(blockChain, tallyUpdater, new EpochManager(30_000), blockInterface);
 
     final Block orphanBlock = createEmptyBlock(4, Hash.ZERO);
 
@@ -134,7 +127,7 @@ public void exceptionThrownIfNoParentBlockExists() {
   public void walkBackStopsWhenACachedVoteTallyIsFound() {
     final VoteTallyUpdater tallyUpdater = mock(VoteTallyUpdater.class);
     final VoteTallyCache cache =
-        new VoteTallyCache(blockChain, tallyUpdater, new EpochManager(30_000));
+        new VoteTallyCache(blockChain, tallyUpdater, new EpochManager(30_000), blockInterface);
 
     // Load the Cache up to block_2
     cache.getVoteTallyAfterBlock(block_2.getHeader());
@@ -159,7 +152,6 @@ public void walkBackStopsWhenACachedVoteTallyIsFound() {
   @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()))
@@ -168,7 +160,8 @@ public void integrationTestingVotesBeingApplied() {
     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);
+    final VoteTallyCache cache =
+        new VoteTallyCache(blockChain, tallyUpdater, epochManager, blockInterface);
 
     VoteTally voteTally = cache.getVoteTallyAfterBlock(block_1.getHeader());
     assertThat(voteTally.getValidators()).containsAll(validators);

consensus/ibft/build.gradle

@@ -55,6 +55,8 @@ dependencies {
   testImplementation 'org.awaitility:awaitility'
   testImplementation 'org.assertj:assertj-core'
   testImplementation 'org.mockito:mockito-core'
+
+  testSupportImplementation 'org.mockito:mockito-core'
 }