Ensure empty withdrawal lists are set in BFT blocks when the protocol…

… spec is shanghai or higher (hyperledger#6765) * Ensure empty withdrawal lists are set in BFT blocks when the protocol schedule is shanghai or higher Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Unit tests missing mock Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Unit tests missing mock Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> * Refactor and reduce code duplication Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> --------- Signed-off-by: Matthew Whitehead <matthew1001@gmail.com> Signed-off-by: Matt Whitehead <matthew.whitehead@kaleido.io> Signed-off-by: amsmota <antonio.mota@citi.com>
Citi · Apr 16, 2024 · 8e66290 · 8e66290
1 parent 3922b75
commit 8e66290
Show file tree

Hide file tree

Showing 11 changed files with 250 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -33,6 +33,7 @@
 - Make block transaction selection max time aware of PoA transitions [#6676](https://github.com/hyperledger/besu/pull/6676)
 - Don't enable the BFT mining coordinator when running sub commands such as `blocks export` [#6675](https://github.com/hyperledger/besu/pull/6675)
 - In JSON-RPC return optional `v` fields for type 1 and type 2 transactions [#6762](https://github.com/hyperledger/besu/pull/6762)
+- Fix Shanghai/QBFT block import bug when syncing new nodes [#6765](https://github.com/hyperledger/besu/pull/6765)
 
 ### Download Links
 

diff --git a/...c/main/java/org/hyperledger/besu/consensus/common/bft/BaseBftProtocolScheduleBuilder.java b/...c/main/java/org/hyperledger/besu/consensus/common/bft/BaseBftProtocolScheduleBuilder.java
@@ -30,7 +30,6 @@
 import org.hyperledger.besu.ethereum.mainnet.ProtocolScheduleBuilder;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSpecAdapters;
 import org.hyperledger.besu.ethereum.mainnet.ProtocolSpecBuilder;
-import org.hyperledger.besu.ethereum.mainnet.WithdrawalsValidator;
 import org.hyperledger.besu.ethereum.mainnet.feemarket.FeeMarket;
 import org.hyperledger.besu.evm.internal.EvmConfiguration;
 
@@ -125,9 +124,6 @@ private ProtocolSpecBuilder applyBftChanges(
         .skipZeroBlockRewards(true)
         .blockHeaderFunctions(BftBlockHeaderFunctions.forOnchainBlock(bftExtraDataCodec))
         .blockReward(Wei.of(configOptions.getBlockRewardWei()))
-        .withdrawalsValidator(
-            new WithdrawalsValidator
-                .ProhibitedWithdrawals()) // QBFT/IBFT doesn't support withdrawals
         .miningBeneficiaryCalculator(
             header -> configOptions.getMiningBeneficiary().orElseGet(header::getCoinbase));
   }

diff --git a/...rc/main/java/org/hyperledger/besu/consensus/common/bft/blockcreation/BftBlockCreator.java b/...rc/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,7 +30,10 @@
 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;
 
 /** The Bft block creator. */
@@ -77,12 +81,31 @@ 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 ->
         forksSchedule.getFork(blockNum).getValue().getMiningBeneficiary().orElse(localAddress);
   }
 
+  @Override
+  public BlockCreationResult createEmptyWithdrawalsBlock(final long timestamp) {
+    return createBlock(
+        Optional.empty(), Optional.empty(), Optional.of(Collections.emptyList()), timestamp);
+  }
+
   @Override
   protected BlockHeader createFinalBlockHeader(final SealableBlockHeader sealableBlockHeader) {
     final BlockHeaderBuilder builder =

diff --git a/.../src/test/java/org/hyperledger/besu/consensus/ibft/blockcreation/BftBlockCreatorTest.java b/.../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();
+  }
 }
diff --git a/...us/ibft/src/test/java/org/hyperledger/besu/consensus/ibft/statemachine/IbftRoundTest.java b/...us/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;
@@ -273,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(

diff --git a/.../src/main/java/org/hyperledger/besu/consensus/qbft/blockcreation/PkiQbftBlockCreator.java b/.../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);
   }
 
@@ -114,6 +143,13 @@ public BlockCreationResult createBlock(
         timestamp);
   }
 
+  @Override
+  public BlockCreationResult createEmptyWithdrawalsBlock(final long timestamp) {
+    final BlockCreationResult blockCreationResult =
+        blockCreator.createEmptyWithdrawalsBlock(timestamp);
+    return replaceCmsInBlock(blockCreationResult);
+  }
+
   private BlockCreationResult replaceCmsInBlock(final BlockCreationResult blockCreationResult) {
     final Block block = blockCreationResult.getBlock();
     final BlockHeader blockHeader = block.getHeader();

diff --git a/.../main/java/org/hyperledger/besu/consensus/qbft/blockcreation/QbftBlockCreatorFactory.java b/.../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);
     }
   }