Support time based forks for BFT chains

CHANGELOG.md

@@ -60,7 +60,13 @@
 - Upgrade netty to 4.2.10-Final - Fixes `setsockopt() failed: Protocol not available` [#9783](https://github.com/hyperledger/besu/pull/9783)
 - Allow nonce to be max value when `isAllowFutureNonce` is true [#9759](https://github.com/hyperledger/besu/pull/9759)
 
+<<<<<<< bft-time-based-forks
+- BFT forks that change block period on time-based forks don't take effect [9681](https://github.com/hyperledger/besu/issues/9681)
+
+## 26.1.0-RC1
+=======
 ## 26.1.0
+>>>>>>> main
 
 ### Breaking Changes
 - Remove experimental CLI flag `--Xenable-extra-debug-tracers`. Call tracer (`callTracer`) is now always available for `debug_trace*` methods.

app/src/main/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilder.java

@@ -114,7 +114,7 @@ protected MiningCoordinator createMiningCoordinator(
             o ->
                 miningConfiguration.setBlockPeriodSeconds(
                     forksSchedule
-                        .getFork(o.getHeader().getNumber() + 1)
+                        .getFork(o.getHeader().getNumber() + 1, o.getHeader().getTimestamp())
                         .getValue()
                         .getBlockPeriodSeconds()));
 

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

@@ -250,7 +250,7 @@ protected MiningCoordinator createMiningCoordinator(
             o ->
                 miningConfiguration.setBlockPeriodSeconds(
                     forksSchedule
-                        .getFork(o.getHeader().getNumber() + 1)
+                        .getFork(o.getHeader().getNumber() + 1, o.getHeader().getTimestamp())
                         .getValue()
                         .getBlockPeriodSeconds()));
 

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

@@ -335,12 +335,12 @@ protected MiningCoordinator createMiningCoordinator(
             o -> {
               miningConfiguration.setBlockPeriodSeconds(
                   qbftForksSchedule
-                      .getFork(o.getHeader().getNumber() + 1)
+                      .getFork(o.getHeader().getNumber() + 1, o.getHeader().getTimestamp())
                       .getValue()
                       .getBlockPeriodSeconds());
               miningConfiguration.setEmptyBlockPeriodSeconds(
                   qbftForksSchedule
-                      .getFork(o.getHeader().getNumber() + 1)
+                      .getFork(o.getHeader().getNumber() + 1, o.getHeader().getTimestamp())
                       .getValue()
                       .getEmptyBlockPeriodSeconds());
             });

app/src/test/java/org/hyperledger/besu/controller/ConsensusScheduleBesuControllerBuilderTest.java

@@ -32,6 +32,7 @@
 import org.hyperledger.besu.ethereum.blockcreation.MiningCoordinator;
 import org.hyperledger.besu.ethereum.chain.Blockchain;
 import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
+import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.MiningConfiguration;
 import org.hyperledger.besu.ethereum.eth.manager.EthProtocolManager;
 import org.hyperledger.besu.ethereum.eth.sync.state.SyncState;
@@ -129,7 +130,9 @@ public void createsMigratingMiningCoordinator() {
     when(besuControllerBuilder2.createMiningCoordinator(any(), any(), any(), any(), any(), any()))
         .thenReturn(miningCoordinator2);
     final ProtocolContext mockProtocolContext = mock(ProtocolContext.class);
-    when(mockProtocolContext.getBlockchain()).thenReturn(mock(MutableBlockchain.class));
+    final MutableBlockchain blockchain = mock(MutableBlockchain.class);
+    when(mockProtocolContext.getBlockchain()).thenReturn(blockchain);
+    when(blockchain.getChainHeadHeader()).thenReturn(mock(BlockHeader.class));
 
     final ConsensusScheduleBesuControllerBuilder builder =
         new ConsensusScheduleBesuControllerBuilder(consensusSchedule);
@@ -150,19 +153,31 @@ public void createsMigratingMiningCoordinator() {
         (softly) -> {
           softly
               .assertThat(
-                  migratingMiningCoordinator.getMiningCoordinatorSchedule().getFork(0L).getValue())
+                  migratingMiningCoordinator
+                      .getMiningCoordinatorSchedule()
+                      .getFork(0L, 0)
+                      .getValue())
               .isSameAs(miningCoordinator1);
           softly
               .assertThat(
-                  migratingMiningCoordinator.getMiningCoordinatorSchedule().getFork(4L).getValue())
+                  migratingMiningCoordinator
+                      .getMiningCoordinatorSchedule()
+                      .getFork(4L, 0)
+                      .getValue())
               .isSameAs(miningCoordinator1);
           softly
               .assertThat(
-                  migratingMiningCoordinator.getMiningCoordinatorSchedule().getFork(5L).getValue())
+                  migratingMiningCoordinator
+                      .getMiningCoordinatorSchedule()
+                      .getFork(5L, 0)
+                      .getValue())
               .isSameAs(miningCoordinator2);
           softly
               .assertThat(
-                  migratingMiningCoordinator.getMiningCoordinatorSchedule().getFork(6L).getValue())
+                  migratingMiningCoordinator
+                      .getMiningCoordinatorSchedule()
+                      .getFork(6L, 0)
+                      .getValue())
               .isSameAs(miningCoordinator2);
         });
   }
@@ -198,11 +213,11 @@ public void createsMigratingContext() {
     expectedConsensusContextSpecs.add(new ForkSpec<>(10L, context2));
     assertThat(contextSchedule.getForks()).isEqualTo(expectedConsensusContextSpecs);
 
-    assertThat(contextSchedule.getFork(0).getValue()).isSameAs(context1);
-    assertThat(contextSchedule.getFork(1).getValue()).isSameAs(context1);
-    assertThat(contextSchedule.getFork(9).getValue()).isSameAs(context1);
-    assertThat(contextSchedule.getFork(10).getValue()).isSameAs(context2);
-    assertThat(contextSchedule.getFork(11).getValue()).isSameAs(context2);
+    assertThat(contextSchedule.getFork(0, 0).getValue()).isSameAs(context1);
+    assertThat(contextSchedule.getFork(1, 0).getValue()).isSameAs(context1);
+    assertThat(contextSchedule.getFork(9, 0).getValue()).isSameAs(context1);
+    assertThat(contextSchedule.getFork(10, 0).getValue()).isSameAs(context2);
+    assertThat(contextSchedule.getFork(11, 0).getValue()).isSameAs(context2);
   }
 
   @Test

consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockMiner.java

@@ -78,7 +78,10 @@ protected boolean mineBlock() throws InterruptedException {
 
   @Override
   protected boolean shouldImportBlock(final Block block) throws InterruptedException {
-    if (forksSchedule.getFork(block.getHeader().getNumber()).getValue().getCreateEmptyBlocks()) {
+    if (forksSchedule
+        .getFork(block.getHeader().getNumber(), block.getHeader().getTimestamp())
+        .getValue()
+        .getCreateEmptyBlocks()) {
       return true;
     }
 

consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockScheduler.java

@@ -53,7 +53,7 @@ public CliqueBlockScheduler(
         parentHeader ->
             (long)
                 forksSchedule
-                    .getFork(parentHeader.getNumber() + 1)
+                    .getFork(parentHeader.getNumber() + 1, parentHeader.getTimestamp())
                     .getValue()
                     .getBlockPeriodSeconds(),
         0L,

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/ForkSpec.java

@@ -14,6 +14,8 @@
  */
 package org.hyperledger.besu.consensus.common;
 
+import org.hyperledger.besu.ethereum.mainnet.ScheduledProtocolSpec;
+
 import java.util.Comparator;
 import java.util.Objects;
 
@@ -28,6 +30,8 @@ public class ForkSpec<C> {
   public static final Comparator<ForkSpec<?>> COMPARATOR = Comparator.comparing(ForkSpec::getBlock);
 
   private final long block;
+  private ScheduledProtocolSpec.ScheduleType forkType =
+      ScheduledProtocolSpec.ScheduleType.BLOCK; // Default type
   private final C value;
 
   /**
@@ -50,6 +54,24 @@ public long getBlock() {
     return block;
   }
 
+  /**
+   * Gets the fork type (block number or timestamp).
+   *
+   * @param forkType the fork type
+   */
+  public void setForkType(final ScheduledProtocolSpec.ScheduleType forkType) {
+    this.forkType = forkType;
+  }
+
+  /**
+   * Gets the fork type (block number or timestamp).
+   *
+   * @return the fork type
+   */
+  public ScheduledProtocolSpec.ScheduleType getForkType() {
+    return forkType;
+  }
+
   /**
    * Gets value.
    *

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/ForksSchedule.java

@@ -14,6 +14,9 @@
  */
 package org.hyperledger.besu.consensus.common;
 
+import org.hyperledger.besu.consensus.common.bft.BftProtocolSchedule;
+import org.hyperledger.besu.ethereum.mainnet.ScheduledProtocolSpec;
+
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
@@ -42,15 +45,33 @@ public ForksSchedule(final Collection<ForkSpec<C>> forks) {
     this.forks.addAll(forks);
   }
 
+  /**
+   * Apply the protocol schedule to the forks to assert their fork type - block or timestamp
+   *
+   * @param protocolSchedule the protocol schedule
+   */
+  public void applyMilestoneTypes(final BftProtocolSchedule protocolSchedule) {
+    forks.forEach(
+        f ->
+            f.setForkType(
+                protocolSchedule.getSpecTypeBlockNumberOrTimestamp(f.getBlock(), f.getBlock())));
+  }
+
   /**
    * Gets fork.
    *
    * @param blockNumber the block number
+   * @param blockTimestamp the block timestamp
    * @return the fork
    */
-  public ForkSpec<C> getFork(final long blockNumber) {
+  public ForkSpec<C> getFork(final long blockNumber, final long blockTimestamp) {
     for (final ForkSpec<C> f : forks) {
-      if (blockNumber >= f.getBlock()) {
+      if (f.getForkType() == ScheduledProtocolSpec.ScheduleType.BLOCK
+          && blockNumber >= f.getBlock()) {
+        return f;
+      }
+      if (f.getForkType() == ScheduledProtocolSpec.ScheduleType.TIME
+          && blockTimestamp >= f.getBlock()) {
         return f;
       }
     }

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/MigratingMiningCoordinator.java

@@ -53,8 +53,13 @@ public MigratingMiningCoordinator(
       final Blockchain blockchain) {
     this.miningCoordinatorSchedule = miningCoordinatorSchedule;
     this.blockchain = blockchain;
+
+    final BlockHeader chainHead = blockchain.getChainHeadHeader();
+
     this.activeMiningCoordinator =
-        this.miningCoordinatorSchedule.getFork(blockchain.getChainHeadBlockNumber() + 1).getValue();
+        this.miningCoordinatorSchedule
+            .getFork(chainHead.getNumber() + 1, chainHead.getTimestamp())
+            .getValue();
   }
 
   @Override
@@ -133,8 +138,9 @@ public void changeTargetGasLimit(final Long targetGasLimit) {
   @Override
   public void onBlockAdded(final BlockAddedEvent event) {
     final long currentBlock = event.getHeader().getNumber();
+    final long parentTimestamp = event.getHeader().getTimestamp();
     final MiningCoordinator nextMiningCoordinator =
-        miningCoordinatorSchedule.getFork(currentBlock + 1).getValue();
+        miningCoordinatorSchedule.getFork(currentBlock + 1, parentTimestamp).getValue();
 
     if (activeMiningCoordinator != nextMiningCoordinator) {
       LOG.trace(

consensus/common/src/main/java/org/hyperledger/besu/consensus/common/MigratingProtocolContext.java

@@ -20,6 +20,7 @@
 import org.hyperledger.besu.ethereum.chain.MutableBlockchain;
 import org.hyperledger.besu.ethereum.worldstate.WorldStateArchive;
 import org.hyperledger.besu.plugin.ServiceManager;
+import org.hyperledger.besu.plugin.data.BlockHeader;
 
 /** The Migrating protocol context. */
 public class MigratingProtocolContext extends ProtocolContext {
@@ -48,13 +49,18 @@ public MigratingProtocolContext(
 
   @Override
   public <C extends ConsensusContext> C getConsensusContext(final Class<C> klass) {
-    final long chainHeadBlockNumber = getBlockchain().getChainHeadBlockNumber();
-    return consensusContextSchedule.getFork(chainHeadBlockNumber + 1).getValue().as(klass);
+    final BlockHeader chainHead = getBlockchain().getChainHeadHeader();
+    return consensusContextSchedule
+        .getFork(chainHead.getNumber() + 1, chainHead.getTimestamp())
+        .getValue()
+        .as(klass);
   }
 
   @Override
   public <C extends ConsensusContext> C getConsensusContext(
       final Class<C> klass, final long blockNumber) {
-    return consensusContextSchedule.getFork(blockNumber).getValue().as(klass);
+    // Block number will be either an actual block number or a timestamp, so we pass it in for both
+    // getFork() args
+    return consensusContextSchedule.getFork(blockNumber, blockNumber).getValue().as(klass);
   }
 }

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

@@ -101,7 +101,13 @@ public BftProtocolSchedule createProtocolSchedule(
                 balConfiguration,
                 metricsSystem)
             .createProtocolSchedule();
-    return new BftProtocolSchedule((DefaultProtocolSchedule) protocolSchedule);
+    final BftProtocolSchedule bftSchedule =
+        new BftProtocolSchedule((DefaultProtocolSchedule) protocolSchedule);
+
+    // Once we have the schedule we can update the fork schedule with the type of each milestone
+    forksSchedule.applyMilestoneTypes(bftSchedule);
+
+    return bftSchedule;
   }
 
   /**

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

@@ -68,6 +68,38 @@ public ProtocolSpec getByBlockNumberOrTimestamp(final long number, final long ti
     return theSpec;
   }
 
+  /**
+   * Look up type of spec by block number or timestamp
+   *
+   * @param number block number
+   * @param timestamp block timestamp
+   * @return the protocol spec type for that block number or timestamp
+   */
+  public ScheduledProtocolSpec.ScheduleType getSpecTypeBlockNumberOrTimestamp(
+      final long number, final long timestamp) {
+    checkArgument(number >= 0, "number must be non-negative");
+    checkArgument(
+        !protocolSpecs.isEmpty(), "At least 1 milestone must be provided to the protocol schedule");
+    checkArgument(
+        protocolSpecs.last().fork().milestone() == 0,
+        "There must be a milestone starting from block 0");
+    // protocolSpecs is sorted in descending block order, so the first one we find that's lower than
+    // the requested level will be the most appropriate spec
+    ScheduledProtocolSpec.ScheduleType specType = null;
+    for (final ScheduledProtocolSpec s : protocolSpecs) {
+      if ((s instanceof ScheduledProtocolSpec.BlockNumberProtocolSpec)
+          && (number >= s.fork().milestone())) {
+        specType = ScheduledProtocolSpec.ScheduleType.BLOCK;
+        break;
+      } else if ((s instanceof ScheduledProtocolSpec.TimestampProtocolSpec)
+          && (timestamp >= s.fork().milestone())) {
+        specType = ScheduledProtocolSpec.ScheduleType.TIME;
+        break;
+      }
+    }
+    return specType;
+  }
+
   /**
    * return the ordered list of scheduled protocol specs
    *