Refactor and reduce code duplication

Signed-off-by: Matthew Whitehead <[email protected]>

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftBlockCreator.java

@@ -19,6 +19,7 @@
 import org.hyperledger.besu.consensus.common.bft.BftBlockHeaderFunctions;
 import org.hyperledger.besu.consensus.common.bft.BftExtraDataCodec;
 import org.hyperledger.besu.consensus.common.bft.BftHelpers;
+import org.hyperledger.besu.consensus.common.bft.BftProtocolSchedule;
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.ethereum.ProtocolContext;
 import org.hyperledger.besu.ethereum.blockcreation.AbstractBlockCreator;
@@ -29,6 +30,8 @@
 import org.hyperledger.besu.ethereum.eth.manager.EthScheduler;
 import org.hyperledger.besu.ethereum.eth.transactions.TransactionPool;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
+import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec;
+import org.hyperledger.besu.ethereum.mainnet.WithdrawalsValidator;
 
 import java.util.Collections;
 import java.util.Optional;
@@ -78,6 +81,19 @@ public BftBlockCreator(
     this.bftExtraDataCodec = bftExtraDataCodec;
   }
 
+  @Override
+  public BlockCreationResult createBlock(final long timestamp) {
+    ProtocolSpec protocolSpec =
+        ((BftProtocolSchedule) protocolSchedule)
+            .getByBlockNumberOrTimestamp(parentHeader.getNumber() + 1, timestamp);
+
+    if (protocolSpec.getWithdrawalsValidator() instanceof WithdrawalsValidator.AllowedWithdrawals) {
+      return createEmptyWithdrawalsBlock(timestamp);
+    } else {
+      return createBlock(Optional.empty(), Optional.empty(), timestamp);
+    }
+  }
+
   private static MiningBeneficiaryCalculator miningBeneficiaryCalculator(
       final Address localAddress, final ForksSchedule<? extends BftConfigOptions> forksSchedule) {
     return blockNum ->

consensus/ibft/src/main/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftRound.java

@@ -21,7 +21,6 @@
 import org.hyperledger.besu.consensus.common.bft.BftExtraData;
 import org.hyperledger.besu.consensus.common.bft.BftExtraDataCodec;
 import org.hyperledger.besu.consensus.common.bft.BftHelpers;
-import org.hyperledger.besu.consensus.common.bft.BftProtocolSchedule;
 import org.hyperledger.besu.consensus.common.bft.ConsensusRoundIdentifier;
 import org.hyperledger.besu.consensus.common.bft.RoundTimer;
 import org.hyperledger.besu.consensus.ibft.messagewrappers.Commit;
@@ -42,8 +41,6 @@
 import org.hyperledger.besu.ethereum.mainnet.BlockImportResult;
 import org.hyperledger.besu.ethereum.mainnet.HeaderValidationMode;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
-import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec;
-import org.hyperledger.besu.ethereum.mainnet.WithdrawalsValidator;
 import org.hyperledger.besu.plugin.services.securitymodule.SecurityModuleException;
 import org.hyperledger.besu.util.Subscribers;
 
@@ -121,20 +118,7 @@ public ConsensusRoundIdentifier getRoundIdentifier() {
    * @param headerTimeStampSeconds the header time stamp seconds
    */
   public void createAndSendProposalMessage(final long headerTimeStampSeconds) {
-
-    // Determine if we're at a protocol spec that requires withdrawals (even if empty) to be present
-    ProtocolSpec protocolSpec =
-        ((BftProtocolSchedule) protocolSchedule)
-            .getByBlockNumberOrTimestamp(
-                roundState.getRoundIdentifier().getSequenceNumber(), headerTimeStampSeconds);
-
-    final Block block;
-    if (protocolSpec.getWithdrawalsValidator() instanceof WithdrawalsValidator.AllowedWithdrawals) {
-      block = blockCreator.createEmptyWithdrawalsBlock(headerTimeStampSeconds).getBlock();
-    } else {
-      block = blockCreator.createBlock(headerTimeStampSeconds).getBlock();
-    }
-
+    final Block block = blockCreator.createBlock(headerTimeStampSeconds).getBlock();
     final BftExtraData extraData = bftExtraDataCodec.decode(block.getHeader());
     LOG.debug("Creating proposed block. round={}", roundState.getRoundIdentifier());
     LOG.trace(
@@ -157,20 +141,7 @@ public void startRoundWith(
     final Block blockToPublish;
     if (!bestBlockFromRoundChange.isPresent()) {
       LOG.debug("Sending proposal with new block. round={}", roundState.getRoundIdentifier());
-
-      // Determine if we're at a protocol spec that requires withdrawals (even if empty) to be
-      // present
-      ProtocolSpec protocolSpec =
-          ((BftProtocolSchedule) protocolSchedule)
-              .getByBlockNumberOrTimestamp(
-                  roundState.getRoundIdentifier().getSequenceNumber(), headerTimestamp);
-
-      if (protocolSpec.getWithdrawalsValidator()
-          instanceof WithdrawalsValidator.AllowedWithdrawals) {
-        blockToPublish = blockCreator.createEmptyWithdrawalsBlock(headerTimestamp).getBlock();
-      } else {
-        blockToPublish = blockCreator.createBlock(headerTimestamp).getBlock();
-      }
+      blockToPublish = blockCreator.createBlock(headerTimestamp).getBlock();
     } else {
       LOG.debug(
           "Sending proposal from PreparedCertificate. round={}", roundState.getRoundIdentifier());

consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/blockcreation/BftBlockCreatorTest.java

@@ -213,4 +213,125 @@ public BlockHeaderValidator.Builder createBlockHeaderRuleset(
             new BftBlockHashing(bftExtraDataEncoder)
                 .calculateDataHashForCommittedSeal(header, extraData));
   }
+
+  @Test
+  public void testBlockCreationResultsInEmptyWithdrawalsListShanghaiOnwards() {
+    // Construct a parent block.
+    final BlockHeaderTestFixture blockHeaderBuilder = new BlockHeaderTestFixture();
+    blockHeaderBuilder.gasLimit(5000); // required to pass validation rule checks.
+    final BlockHeader parentHeader = blockHeaderBuilder.buildHeader();
+    final Optional<BlockHeader> optionalHeader = Optional.of(parentHeader);
+
+    // Construct a blockchain and world state
+    final MutableBlockchain blockchain = mock(MutableBlockchain.class);
+    when(blockchain.getChainHeadHash()).thenReturn(parentHeader.getHash());
+    when(blockchain.getBlockHeader(any())).thenReturn(optionalHeader);
+    final BlockHeader blockHeader = mock(BlockHeader.class);
+    when(blockHeader.getBaseFee()).thenReturn(Optional.empty());
+    when(blockchain.getChainHeadHeader()).thenReturn(blockHeader);
+
+    final List<Address> initialValidatorList = Lists.newArrayList();
+    for (int i = 0; i < 4; i++) {
+      initialValidatorList.add(AddressHelpers.ofValue(i));
+    }
+
+    final IbftExtraDataCodec bftExtraDataEncoder = new IbftExtraDataCodec();
+
+    final BaseBftProtocolScheduleBuilder bftProtocolSchedule =
+        new BaseBftProtocolScheduleBuilder() {
+          @Override
+          public BlockHeaderValidator.Builder createBlockHeaderRuleset(
+              final BftConfigOptions config, final FeeMarket feeMarket) {
+            return IbftBlockHeaderValidationRulesetFactory.blockHeaderValidator(
+                5, Optional.empty());
+          }
+        };
+    final GenesisConfigOptions configOptions =
+        GenesisConfigFile.fromConfig("{\"config\": {\"shanghaiTime\":0}}").getConfigOptions();
+    final ForksSchedule<BftConfigOptions> forksSchedule =
+        new ForksSchedule<>(List.of(new ForkSpec<>(0, configOptions.getBftConfigOptions())));
+    final ProtocolSchedule protocolSchedule =
+        bftProtocolSchedule.createProtocolSchedule(
+            configOptions,
+            forksSchedule,
+            PrivacyParameters.DEFAULT,
+            false,
+            bftExtraDataEncoder,
+            EvmConfiguration.DEFAULT,
+            MiningParameters.MINING_DISABLED,
+            new BadBlockManager());
+    final ProtocolContext protContext =
+        new ProtocolContext(
+            blockchain,
+            createInMemoryWorldStateArchive(),
+            setupContextWithBftExtraDataEncoder(initialValidatorList, bftExtraDataEncoder),
+            new BadBlockManager());
+
+    final TransactionPoolConfiguration poolConf =
+        ImmutableTransactionPoolConfiguration.builder().txPoolMaxSize(1).build();
+
+    final GasPricePendingTransactionsSorter pendingTransactions =
+        new GasPricePendingTransactionsSorter(
+            poolConf,
+            TestClock.system(ZoneId.systemDefault()),
+            metricsSystem,
+            blockchain::getChainHeadHeader);
+
+    final EthContext ethContext = mock(EthContext.class, RETURNS_DEEP_STUBS);
+    when(ethContext.getEthPeers().subscribeConnect(any())).thenReturn(1L);
+
+    final TransactionPool transactionPool =
+        new TransactionPool(
+            () -> pendingTransactions,
+            protocolSchedule,
+            protContext,
+            mock(TransactionBroadcaster.class),
+            ethContext,
+            new TransactionPoolMetrics(metricsSystem),
+            poolConf);
+
+    transactionPool.setEnabled();
+
+    final MiningParameters miningParameters =
+        ImmutableMiningParameters.builder()
+            .mutableInitValues(
+                MutableInitValues.builder()
+                    .extraData(
+                        bftExtraDataEncoder.encode(
+                            new BftExtraData(
+                                Bytes.wrap(new byte[32]),
+                                Collections.emptyList(),
+                                Optional.empty(),
+                                0,
+                                initialValidatorList)))
+                    .minTransactionGasPrice(Wei.ZERO)
+                    .coinbase(AddressHelpers.ofValue(1))
+                    .build())
+            .build();
+
+    final BftBlockCreator blockCreator =
+        new BftBlockCreator(
+            miningParameters,
+            forksSchedule,
+            initialValidatorList.get(0),
+            parent ->
+                bftExtraDataEncoder.encode(
+                    new BftExtraData(
+                        Bytes.wrap(new byte[32]),
+                        Collections.emptyList(),
+                        Optional.empty(),
+                        0,
+                        initialValidatorList)),
+            transactionPool,
+            protContext,
+            protocolSchedule,
+            parentHeader,
+            bftExtraDataEncoder,
+            new DeterministicEthScheduler());
+
+    final Block block = blockCreator.createBlock(parentHeader.getTimestamp() + 1).getBlock();
+
+    // Test that a BFT block contains an empty withdrawals list (not an optional empty)
+    assertThat(block.getBody().getWithdrawals().isPresent()).isTrue();
+  }
 }

consensus/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftRoundTest.java

@@ -59,6 +59,7 @@
 import org.hyperledger.besu.ethereum.core.BlockImporter;
 import org.hyperledger.besu.ethereum.mainnet.BlockImportResult;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec;
+import org.hyperledger.besu.ethereum.mainnet.WithdrawalsValidator;
 import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
 import org.hyperledger.besu.plugin.services.securitymodule.SecurityModuleException;
 import org.hyperledger.besu.util.Subscribers;
@@ -139,10 +140,6 @@ public void setup() {
         .when(blockImporter.importBlock(any(), any(), any()))
         .thenReturn(new BlockImportResult(BlockImportResult.BlockImportStatus.IMPORTED));
 
-    lenient()
-        .when(protocolSchedule.getByBlockNumberOrTimestamp(anyLong(), anyLong()))
-        .thenReturn(protocolSpec);
-
     subscribers.subscribe(minedBlockObserver);
   }
 
@@ -277,6 +274,9 @@ public void twoValidatorNetworkSendsPrepareOnProposalReceptionThenSendsCommitOnC
 
   @Test
   public void localNodeProposesToNetworkOfTwoValidatorsImportsOnReceptionOfCommitFromPeer() {
+    lenient()
+        .when(protocolSpec.getWithdrawalsValidator())
+        .thenReturn(new WithdrawalsValidator.AllowedWithdrawals());
     final RoundState roundState = new RoundState(roundIdentifier, 2, messageValidator);
     final IbftRound round =
         new IbftRound(

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/blockcreation/PkiQbftBlockCreator.java

@@ -19,6 +19,7 @@
 import org.hyperledger.besu.consensus.common.bft.BftBlockHeaderFunctions;
 import org.hyperledger.besu.consensus.common.bft.BftExtraData;
 import org.hyperledger.besu.consensus.common.bft.BftExtraDataCodec;
+import org.hyperledger.besu.consensus.common.bft.BftProtocolSchedule;
 import org.hyperledger.besu.consensus.qbft.pki.PkiBlockCreationConfiguration;
 import org.hyperledger.besu.consensus.qbft.pki.PkiQbftBlockHeaderFunctions;
 import org.hyperledger.besu.consensus.qbft.pki.PkiQbftExtraData;
@@ -29,6 +30,9 @@
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.BlockHeaderBuilder;
 import org.hyperledger.besu.ethereum.core.Transaction;
+import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule;
+import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec;
+import org.hyperledger.besu.ethereum.mainnet.WithdrawalsValidator;
 import org.hyperledger.besu.pki.cms.CmsCreator;
 
 import java.util.Collections;
@@ -48,24 +52,33 @@ public class PkiQbftBlockCreator implements BlockCreator {
   private final BlockCreator blockCreator;
   private final PkiQbftExtraDataCodec pkiQbftExtraDataCodec;
   private final CmsCreator cmsCreator;
+  private final BlockHeader parentHeader;
+  private final ProtocolSchedule protocolSchedule;
 
   /**
    * Instantiates a new Pki qbft block creator.
    *
    * @param blockCreator the block creator
    * @param pkiBlockCreationConfiguration the pki block creation configuration
    * @param pkiQbftExtraDataCodec the pki qbft extra data codec
+   * @param parentHeader the block header of the parent block
+   * @param protocolSchedule the protocol schedule (the type of block can vary based on the current
+   *     protocol spec)
    */
   public PkiQbftBlockCreator(
       final BlockCreator blockCreator,
       final PkiBlockCreationConfiguration pkiBlockCreationConfiguration,
-      final BftExtraDataCodec pkiQbftExtraDataCodec) {
+      final BftExtraDataCodec pkiQbftExtraDataCodec,
+      final BlockHeader parentHeader,
+      final ProtocolSchedule protocolSchedule) {
     this(
         blockCreator,
         new CmsCreator(
             pkiBlockCreationConfiguration.getKeyStore(),
             pkiBlockCreationConfiguration.getCertificateAlias()),
-        pkiQbftExtraDataCodec);
+        pkiQbftExtraDataCodec,
+        parentHeader,
+        protocolSchedule);
   }
 
   /**
@@ -74,14 +87,21 @@ public PkiQbftBlockCreator(
    * @param blockCreator the block creator
    * @param cmsCreator the cms creator
    * @param bftExtraDataCodec the bft extra data codec
+   * @param parentHeader the block header of the parent block
+   * @param protocolSchedule the protocol schedule (the type of block can vary based on the current
+   *     protocol spec)
    */
   @VisibleForTesting
   public PkiQbftBlockCreator(
       final BlockCreator blockCreator,
       final CmsCreator cmsCreator,
-      final BftExtraDataCodec bftExtraDataCodec) {
+      final BftExtraDataCodec bftExtraDataCodec,
+      final BlockHeader parentHeader,
+      final ProtocolSchedule protocolSchedule) {
     this.blockCreator = blockCreator;
     this.cmsCreator = cmsCreator;
+    this.protocolSchedule = protocolSchedule;
+    this.parentHeader = parentHeader;
 
     checkArgument(
         bftExtraDataCodec instanceof PkiQbftExtraDataCodec,
@@ -91,7 +111,16 @@ public PkiQbftBlockCreator(
 
   @Override
   public BlockCreationResult createBlock(final long timestamp) {
-    final BlockCreationResult blockCreationResult = blockCreator.createBlock(timestamp);
+    ProtocolSpec protocolSpec =
+        ((BftProtocolSchedule) protocolSchedule)
+            .getByBlockNumberOrTimestamp(parentHeader.getNumber() + 1, timestamp);
+
+    final BlockCreationResult blockCreationResult;
+    if (protocolSpec.getWithdrawalsValidator() instanceof WithdrawalsValidator.AllowedWithdrawals) {
+      blockCreationResult = blockCreator.createEmptyWithdrawalsBlock(timestamp);
+    } else {
+      blockCreationResult = blockCreator.createBlock(timestamp);
+    }
     return replaceCmsInBlock(blockCreationResult);
   }
 

consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/blockcreation/QbftBlockCreatorFactory.java

@@ -77,7 +77,11 @@ public BlockCreator create(final BlockHeader parentHeader, final int round) {
       return blockCreator;
     } else {
       return new PkiQbftBlockCreator(
-          blockCreator, qbftContext.getPkiBlockCreationConfiguration().get(), bftExtraDataCodec);
+          blockCreator,
+          qbftContext.getPkiBlockCreationConfiguration().get(),
+          bftExtraDataCodec,
+          parentHeader,
+          protocolSchedule);
     }
   }