Remove some validation from QBFT block header rules (#1870)

QBFT no longer validates all fields of the block header (eg nonce mixhash), as these have no bearing on the 
safety model of the protocol.

Signed-off-by: Trent Mohay <trent.mohay@consensys.net>

besu/src/main/java/org/hyperledger/besu/controller/IbftBesuControllerBuilder.java

@@ -44,6 +44,7 @@
 import org.hyperledger.besu.consensus.common.bft.statemachine.BftEventHandler;
 import org.hyperledger.besu.consensus.common.bft.statemachine.BftFinalState;
 import org.hyperledger.besu.consensus.common.bft.statemachine.FutureMessageBuffer;
+import org.hyperledger.besu.consensus.ibft.IbftBlockHeaderValidationRulesetFactory;
 import org.hyperledger.besu.consensus.ibft.IbftGossip;
 import org.hyperledger.besu.consensus.ibft.jsonrpc.IbftJsonRpcMethods;
 import org.hyperledger.besu.consensus.ibft.payload.MessageFactory;
@@ -222,7 +223,8 @@ protected ProtocolSchedule createProtocolSchedule() {
     return BftProtocolSchedule.create(
         genesisConfig.getConfigOptions(genesisConfigOverrides),
         privacyParameters,
-        isRevertReasonEnabled);
+        isRevertReasonEnabled,
+        IbftBlockHeaderValidationRulesetFactory::blockHeaderValidator);
   }
 
   @Override

besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java

@@ -44,6 +44,7 @@
 import org.hyperledger.besu.consensus.common.bft.statemachine.BftEventHandler;
 import org.hyperledger.besu.consensus.common.bft.statemachine.BftFinalState;
 import org.hyperledger.besu.consensus.common.bft.statemachine.FutureMessageBuffer;
+import org.hyperledger.besu.consensus.qbft.QbftBlockHeaderValidationRulesetFactory;
 import org.hyperledger.besu.consensus.qbft.QbftGossip;
 import org.hyperledger.besu.consensus.qbft.jsonrpc.QbftJsonRpcMethods;
 import org.hyperledger.besu.consensus.qbft.payload.MessageFactory;
@@ -222,7 +223,8 @@ protected ProtocolSchedule createProtocolSchedule() {
     return BftProtocolSchedule.create(
         genesisConfig.getConfigOptions(genesisConfigOverrides),
         privacyParameters,
-        isRevertReasonEnabled);
+        isRevertReasonEnabled,
+        QbftBlockHeaderValidationRulesetFactory::blockHeaderValidator);
   }
 
   @Override

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftProtocolSchedule.java

@@ -14,13 +14,12 @@
  */
 package org.hyperledger.besu.consensus.common.bft;
 
-import static org.hyperledger.besu.consensus.common.bft.BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator;
-
 import org.hyperledger.besu.config.BftConfigOptions;
 import org.hyperledger.besu.config.GenesisConfigOptions;
 import org.hyperledger.besu.ethereum.core.Address;
 import org.hyperledger.besu.ethereum.core.PrivacyParameters;
 import org.hyperledger.besu.ethereum.core.Wei;
+import org.hyperledger.besu.ethereum.mainnet.BlockHeaderValidator;
 import org.hyperledger.besu.ethereum.mainnet.MainnetBlockBodyValidator;
 import org.hyperledger.besu.ethereum.mainnet.MainnetBlockImporter;
 import org.hyperledger.besu.ethereum.mainnet.MainnetProtocolSpecs;
@@ -29,6 +28,7 @@
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSpecBuilder;
 
 import java.math.BigInteger;
+import java.util.function.Function;
 
 /** Defines the protocol behaviours for a blockchain using a BFT consensus mechanism. */
 public class BftProtocolSchedule {
@@ -38,31 +38,39 @@ public class BftProtocolSchedule {
   public static ProtocolSchedule create(
       final GenesisConfigOptions config,
       final PrivacyParameters privacyParameters,
-      final boolean isRevertReasonEnabled) {
+      final boolean isRevertReasonEnabled,
+      final Function<Integer, BlockHeaderValidator.Builder> blockHeaderRuleset) {
 
     return new ProtocolScheduleBuilder(
             config,
             DEFAULT_CHAIN_ID,
-            builder -> applyBftChanges(config.getBftConfigOptions(), builder, config.isQuorum()),
+            builder ->
+                applyBftChanges(
+                    config.getBftConfigOptions(), builder, config.isQuorum(), blockHeaderRuleset),
             privacyParameters,
             isRevertReasonEnabled,
             config.isQuorum())
         .createProtocolSchedule();
   }
 
   public static ProtocolSchedule create(
-      final GenesisConfigOptions config, final boolean isRevertReasonEnabled) {
-    return create(config, PrivacyParameters.DEFAULT, isRevertReasonEnabled);
+      final GenesisConfigOptions config,
+      final boolean isRevertReasonEnabled,
+      final Function<Integer, BlockHeaderValidator.Builder> blockHeaderRuleset) {
+    return create(config, PrivacyParameters.DEFAULT, isRevertReasonEnabled, blockHeaderRuleset);
   }
 
-  public static ProtocolSchedule create(final GenesisConfigOptions config) {
-    return create(config, PrivacyParameters.DEFAULT, false);
+  public static ProtocolSchedule create(
+      final GenesisConfigOptions config,
+      final Function<Integer, BlockHeaderValidator.Builder> blockHeaderRuleset) {
+    return create(config, PrivacyParameters.DEFAULT, false, blockHeaderRuleset);
   }
 
   private static ProtocolSpecBuilder applyBftChanges(
       final BftConfigOptions configOptions,
       final ProtocolSpecBuilder builder,
-      final boolean goQuorumMode) {
+      final boolean goQuorumMode,
+      final Function<Integer, BlockHeaderValidator.Builder> blockHeaderRuleset) {
 
     if (configOptions.getEpochLength() <= 0) {
       throw new IllegalArgumentException("Epoch length in config must be greater than zero");
@@ -73,8 +81,10 @@ private static ProtocolSpecBuilder applyBftChanges(
     }
 
     builder
-        .blockHeaderValidatorBuilder(bftBlockHeaderValidator(configOptions.getBlockPeriodSeconds()))
-        .ommerHeaderValidatorBuilder(bftBlockHeaderValidator(configOptions.getBlockPeriodSeconds()))
+        .blockHeaderValidatorBuilder(
+            blockHeaderRuleset.apply(configOptions.getBlockPeriodSeconds()))
+        .ommerHeaderValidatorBuilder(
+            blockHeaderRuleset.apply(configOptions.getBlockPeriodSeconds()))
         .blockBodyValidatorBuilder(MainnetBlockBodyValidator::new)
         .blockValidatorBuilder(MainnetProtocolSpecs.blockValidatorBuilder(goQuorumMode))
         .blockImporterBuilder(MainnetBlockImporter::new)

consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BftProtocolScheduleTest.java

@@ -25,6 +25,7 @@
 import org.hyperledger.besu.ethereum.core.Address;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.Wei;
+import org.hyperledger.besu.ethereum.mainnet.BlockHeaderValidator;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec;
 
@@ -37,6 +38,10 @@ public class BftProtocolScheduleTest {
 
   private final GenesisConfigOptions genesisConfig = mock(GenesisConfigOptions.class);
 
+  private static BlockHeaderValidator.Builder arbitraryRulesetBuilder(final Integer blockSeconds) {
+    return new BlockHeaderValidator.Builder();
+  }
+
   @Test
   public void ensureBlockRewardAndMiningBeneficiaryInProtocolSpecMatchConfig() {
     final BigInteger arbitraryBlockReward = BigInteger.valueOf(5);
@@ -48,7 +53,8 @@ public void ensureBlockRewardAndMiningBeneficiaryInProtocolSpecMatchConfig() {
 
     when(genesisConfig.getBftConfigOptions()).thenReturn(configOptions);
 
-    final ProtocolSchedule schedule = BftProtocolSchedule.create(genesisConfig);
+    final ProtocolSchedule schedule =
+        BftProtocolSchedule.create(genesisConfig, BftProtocolScheduleTest::arbitraryRulesetBuilder);
     final ProtocolSpec spec = schedule.getByBlockNumber(1);
 
     spec.getBlockReward();
@@ -67,7 +73,10 @@ public void illegalMiningBeneficiaryStringThrowsException() {
     when(configOptions.getEpochLength()).thenReturn(3000L);
     when(configOptions.getBlockRewardWei()).thenReturn(BigInteger.ZERO);
 
-    assertThatThrownBy(() -> BftProtocolSchedule.create(genesisConfig))
+    assertThatThrownBy(
+            () ->
+                BftProtocolSchedule.create(
+                    genesisConfig, BftProtocolScheduleTest::arbitraryRulesetBuilder))
         .isInstanceOf(IllegalArgumentException.class)
         .hasMessage("Mining beneficiary in config is not a valid ethereum address");
   }
@@ -81,7 +90,8 @@ public void missingMiningBeneficiaryInConfigWillPayCoinbaseInHeader() {
     when(configOptions.getEpochLength()).thenReturn(3000L);
     when(genesisConfig.getBftConfigOptions()).thenReturn(configOptions);
 
-    final ProtocolSchedule schedule = BftProtocolSchedule.create(genesisConfig);
+    final ProtocolSchedule schedule =
+        BftProtocolSchedule.create(genesisConfig, BftProtocolScheduleTest::arbitraryRulesetBuilder);
     final ProtocolSpec spec = schedule.getByBlockNumber(1);
 
     final Address headerCoinbase = Address.fromHexString("0x123");
@@ -102,7 +112,10 @@ public void negativeBlockRewardThrowsException() {
     when(configOptions.getEpochLength()).thenReturn(3000L);
     when(genesisConfig.getBftConfigOptions()).thenReturn(configOptions);
 
-    assertThatThrownBy(() -> BftProtocolSchedule.create(genesisConfig))
+    assertThatThrownBy(
+            () ->
+                BftProtocolSchedule.create(
+                    genesisConfig, BftProtocolScheduleTest::arbitraryRulesetBuilder))
         .isInstanceOf(IllegalArgumentException.class)
         .hasMessage("Bft Block reward in config cannot be negative");
   }
@@ -116,7 +129,10 @@ public void zeroEpochLengthThrowsException() {
     when(configOptions.getBlockRewardWei()).thenReturn(arbitraryBlockReward);
     when(genesisConfig.getBftConfigOptions()).thenReturn(configOptions);
 
-    assertThatThrownBy(() -> BftProtocolSchedule.create(genesisConfig))
+    assertThatThrownBy(
+            () ->
+                BftProtocolSchedule.create(
+                    genesisConfig, BftProtocolScheduleTest::arbitraryRulesetBuilder))
         .isInstanceOf(IllegalArgumentException.class)
         .hasMessage("Epoch length in config must be greater than zero");
   }
@@ -130,7 +146,10 @@ public void negativeEpochLengthThrowsException() {
     when(configOptions.getBlockRewardWei()).thenReturn(arbitraryBlockReward);
     when(genesisConfig.getBftConfigOptions()).thenReturn(configOptions);
 
-    assertThatThrownBy(() -> BftProtocolSchedule.create(genesisConfig))
+    assertThatThrownBy(
+            () ->
+                BftProtocolSchedule.create(
+                    genesisConfig, BftProtocolScheduleTest::arbitraryRulesetBuilder))
         .isInstanceOf(IllegalArgumentException.class)
         .hasMessage("Epoch length in config must be greater than zero");
   }

consensus/ibft/src/integration-test/java/org/hyperledger/besu/consensus/ibft/support/TestContextBuilder.java

@@ -50,6 +50,7 @@
 import org.hyperledger.besu.consensus.common.bft.statemachine.BftEventHandler;
 import org.hyperledger.besu.consensus.common.bft.statemachine.BftFinalState;
 import org.hyperledger.besu.consensus.common.bft.statemachine.FutureMessageBuffer;
+import org.hyperledger.besu.consensus.ibft.IbftBlockHeaderValidationRulesetFactory;
 import org.hyperledger.besu.consensus.ibft.IbftGossip;
 import org.hyperledger.besu.consensus.ibft.payload.MessageFactory;
 import org.hyperledger.besu.consensus.ibft.statemachine.IbftBlockHeightManagerFactory;
@@ -289,7 +290,9 @@ private static ControllerAndState createControllerAndFinalState(
     final StubGenesisConfigOptions genesisConfigOptions = new StubGenesisConfigOptions();
     genesisConfigOptions.byzantiumBlock(0);
 
-    final ProtocolSchedule protocolSchedule = BftProtocolSchedule.create(genesisConfigOptions);
+    final ProtocolSchedule protocolSchedule =
+        BftProtocolSchedule.create(
+            genesisConfigOptions, IbftBlockHeaderValidationRulesetFactory::blockHeaderValidator);
 
     /////////////////////////////////////////////////////////////////////////////////////
     // From here down is BASICALLY taken from IbftBesuController

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BftBlockHeaderValidationRulesetFactory.java → consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/IbftBlockHeaderValidationRulesetFactory.java

@@ -12,8 +12,9 @@
  *
  * SPDX-License-Identifier: Apache-2.0
  */
-package org.hyperledger.besu.consensus.common.bft;
+package org.hyperledger.besu.consensus.ibft;
 
+import org.hyperledger.besu.consensus.common.bft.BftHelpers;
 import org.hyperledger.besu.consensus.common.bft.headervalidationrules.BftCoinbaseValidationRule;
 import org.hyperledger.besu.consensus.common.bft.headervalidationrules.BftCommitSealsValidationRule;
 import org.hyperledger.besu.consensus.common.bft.headervalidationrules.BftValidatorsValidationRule;
@@ -29,7 +30,7 @@
 
 import org.apache.tuweni.units.bigints.UInt256;
 
-public class BftBlockHeaderValidationRulesetFactory {
+public class IbftBlockHeaderValidationRulesetFactory {
 
   /**
    * Produces a BlockHeaderValidator configured for assessing bft block headers which are to form
@@ -38,8 +39,7 @@ public class BftBlockHeaderValidationRulesetFactory {
    * @param secondsBetweenBlocks the minimum number of seconds which must elapse between blocks.
    * @return BlockHeaderValidator configured for assessing bft block headers
    */
-  public static BlockHeaderValidator.Builder bftBlockHeaderValidator(
-      final long secondsBetweenBlocks) {
+  public static BlockHeaderValidator.Builder blockHeaderValidator(final long secondsBetweenBlocks) {
     return new BlockHeaderValidator.Builder()
         .addRule(new AncestryValidationRule())
         .addRule(new GasUsageValidationRule())

consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BftBlockHeaderValidationRulesetFactoryTest.java → consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/IbftBlockHeaderValidationRulesetFactoryTest.java

@@ -12,13 +12,16 @@
  *
  * SPDX-License-Identifier: Apache-2.0
  */
-package org.hyperledger.besu.consensus.common.bft;
+package org.hyperledger.besu.consensus.ibft;
 
 import static java.util.Collections.emptyList;
 import static java.util.Collections.singletonList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.hyperledger.besu.consensus.common.bft.BftContextBuilder.setupContextWithValidators;
 
+import org.hyperledger.besu.consensus.common.bft.BftExtraData;
+import org.hyperledger.besu.consensus.common.bft.BftExtraDataFixture;
+import org.hyperledger.besu.consensus.common.bft.Vote;
 import org.hyperledger.besu.crypto.NodeKey;
 import org.hyperledger.besu.crypto.NodeKeyUtils;
 import org.hyperledger.besu.ethereum.ProtocolContext;
@@ -38,9 +41,9 @@
 import org.apache.tuweni.bytes.Bytes;
 import org.junit.Test;
 
-public class BftBlockHeaderValidationRulesetFactoryTest {
+public class IbftBlockHeaderValidationRulesetFactoryTest {
 
-  private final ProtocolContext protocolContext(final Collection<Address> validators) {
+  private ProtocolContext protocolContext(final Collection<Address> validators) {
     return new ProtocolContext(null, null, setupContextWithValidators(validators));
   }
 
@@ -57,7 +60,7 @@ public void bftValidateHeaderPasses() {
         getPresetHeaderBuilder(2, proposerNodeKey, validators, parentHeader).buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -78,7 +81,7 @@ public void bftValidateHeaderFailsOnExtraData() {
         getPresetHeaderBuilder(2, proposerNodeKey, emptyList(), parentHeader).buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -104,7 +107,7 @@ public void bftValidateHeaderFailsOnCoinbaseData() {
             .buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -125,7 +128,7 @@ public void bftValidateHeaderFailsOnNonce() {
         getPresetHeaderBuilder(2, proposerNodeKey, validators, parentHeader).nonce(3).buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -148,7 +151,7 @@ public void bftValidateHeaderFailsOnTimestamp() {
             .buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -171,7 +174,7 @@ public void bftValidateHeaderFailsOnMixHash() {
             .buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -194,7 +197,7 @@ public void bftValidateHeaderFailsOnOmmers() {
             .buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -217,7 +220,7 @@ public void bftValidateHeaderFailsOnDifficulty() {
             .buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -238,7 +241,7 @@ public void bftValidateHeaderFailsOnAncestor() {
         getPresetHeaderBuilder(2, proposerNodeKey, validators, null).buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -262,7 +265,7 @@ public void bftValidateHeaderFailsOnGasUsage() {
             .buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(
@@ -285,7 +288,7 @@ public void bftValidateHeaderFailsOnGasLimitRange() {
             .buildHeader();
 
     final BlockHeaderValidator validator =
-        BftBlockHeaderValidationRulesetFactory.bftBlockHeaderValidator(5).build();
+        IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(5).build();
 
     assertThat(
             validator.validateHeader(