Implementation of Issue #3810 QBFT no empty block

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

@@ -411,8 +411,9 @@ private static MinedBlockObserver blockLogger(
     return block ->
         LOG.info(
             String.format(
-                "%s #%,d / %d tx / %d pending / %,d (%01.1f%%) gas / (%s)",
+                "%s %s #%,d / %d tx / %d pending / %,d (%01.1f%%) gas / (%s)",
                 block.getHeader().getCoinbase().equals(localAddress) ? "Produced" : "Imported",
+                block.getBody().getTransactions().size() == 0 ? "empty block" : "block",
                 block.getHeader().getNumber(),
                 block.getBody().getTransactions().size(),
                 transactionPool.count(),

besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java

@@ -139,15 +139,18 @@ public abstract class CommandTestAbstract {
   private static final Logger TEST_LOGGER = LoggerFactory.getLogger(CommandTestAbstract.class);
 
   protected static final int POA_BLOCK_PERIOD_SECONDS = 5;
+  protected static final int POA_EMPTY_BLOCK_PERIOD_SECONDS = 50;
   protected static final JsonObject VALID_GENESIS_QBFT_POST_LONDON =
       (new JsonObject())
           .put(
               "config",
               new JsonObject()
                   .put("londonBlock", 0)
+                  .put("qbft", new JsonObject().put("blockperiodseconds", POA_BLOCK_PERIOD_SECONDS))
                   .put(
                       "qbft",
-                      new JsonObject().put("blockperiodseconds", POA_BLOCK_PERIOD_SECONDS)));
+                      new JsonObject()
+                          .put("emptyblockperiodseconds", POA_EMPTY_BLOCK_PERIOD_SECONDS)));
   protected static final JsonObject VALID_GENESIS_IBFT2_POST_LONDON =
       (new JsonObject())
           .put(

besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java

@@ -307,7 +307,7 @@ public void posBlockCreationMaxTimeOption() {
   @Test
   public void posBlockCreationMaxTimeOutOfAllowedRange() {
     internalTestFailure(
-        "--Xpos-block-creation-max-time must be positive and ≤ 12000",
+        "--Xpos-block-creation-max-time must be positive and less or equal 12000",
         "--Xpos-block-creation-max-time",
         "17000");
   }

config/src/main/java/org/hyperledger/besu/config/BftConfigOptions.java

@@ -37,6 +37,13 @@ public interface BftConfigOptions {
    */
   int getBlockPeriodSeconds();
 
+  /**
+   * Gets empty block period seconds.
+   *
+   * @return the empty block period seconds
+   */
+  int getEmptyBlockPeriodSeconds();
+
   /**
    * Gets request timeout seconds.
    *

config/src/main/java/org/hyperledger/besu/config/BftFork.java

@@ -40,6 +40,9 @@ public class BftFork implements Fork {
   /** The constant BLOCK_PERIOD_SECONDS_KEY. */
   public static final String BLOCK_PERIOD_SECONDS_KEY = "blockperiodseconds";
 
+  /** The constant EMPTY_BLOCK_PERIOD_SECONDS_KEY. */
+  public static final String EMPTY_BLOCK_PERIOD_SECONDS_KEY = "emptyblockperiodseconds";
+
   /** The constant BLOCK_REWARD_KEY. */
   public static final String BLOCK_REWARD_KEY = "blockreward";
 
@@ -82,6 +85,16 @@ public OptionalInt getBlockPeriodSeconds() {
     return JsonUtil.getPositiveInt(forkConfigRoot, BLOCK_PERIOD_SECONDS_KEY);
   }
 
+  /**
+   * Gets empty block period seconds.
+   *
+   * @return the empty block period seconds
+   */
+  public OptionalInt getEmptyBlockPeriodSeconds() {
+    // It can be 0 to disable custom empty block periods
+    return JsonUtil.getInt(forkConfigRoot, EMPTY_BLOCK_PERIOD_SECONDS_KEY);
+  }
+
   /**
    * Gets block reward wei.
    *

config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java

@@ -34,6 +34,8 @@ public class JsonBftConfigOptions implements BftConfigOptions {
 
   private static final long DEFAULT_EPOCH_LENGTH = 30_000;
   private static final int DEFAULT_BLOCK_PERIOD_SECONDS = 1;
+  private static final int DEFAULT_EMPTY_BLOCK_PERIOD_SECONDS = 0;
+  // 0 keeps working as before, increase to activate it
   private static final int DEFAULT_ROUND_EXPIRY_SECONDS = 1;
   // In a healthy network this can be very small. This default limit will allow for suitable
   // protection for on a typical 20 node validator network with multiple rounds
@@ -66,6 +68,12 @@ public int getBlockPeriodSeconds() {
         bftConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS);
   }
 
+  @Override
+  public int getEmptyBlockPeriodSeconds() {
+    return JsonUtil.getInt(
+            bftConfigRoot, "emptyblockperiodseconds", DEFAULT_EMPTY_BLOCK_PERIOD_SECONDS);
+  }
+
   @Override
   public int getRequestTimeoutSeconds() {
     return JsonUtil.getInt(bftConfigRoot, "requesttimeoutseconds", DEFAULT_ROUND_EXPIRY_SECONDS);
@@ -133,6 +141,9 @@ public Map<String, Object> asMap() {
     if (bftConfigRoot.has("blockperiodseconds")) {
       builder.put("blockPeriodSeconds", getBlockPeriodSeconds());
     }
+    if (bftConfigRoot.has("emptyblockperiodseconds")) {
+      builder.put("emptyblockperiodseconds", getEmptyBlockPeriodSeconds());
+    }
     if (bftConfigRoot.has("requesttimeoutseconds")) {
       builder.put("requestTimeoutSeconds", getRequestTimeoutSeconds());
     }

config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java

@@ -17,6 +17,7 @@
 import static java.util.Collections.emptyMap;
 import static java.util.Collections.singletonMap;
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 
 import org.hyperledger.besu.datatypes.Address;
@@ -30,6 +31,7 @@ public class JsonBftConfigOptionsTest {
 
   private static final int EXPECTED_DEFAULT_EPOCH_LENGTH = 30_000;
   private static final int EXPECTED_DEFAULT_BLOCK_PERIOD = 1;
+  private static final int EXPECTED_EMPTY_DEFAULT_BLOCK_PERIOD = 0;
   private static final int EXPECTED_DEFAULT_REQUEST_TIMEOUT = 1;
   private static final int EXPECTED_DEFAULT_GOSSIPED_HISTORY_LIMIT = 1000;
   private static final int EXPECTED_DEFAULT_MESSAGE_QUEUE_LIMIT = 1000;
@@ -61,25 +63,51 @@ public void shouldGetBlockPeriodFromConfig() {
     assertThat(config.getBlockPeriodSeconds()).isEqualTo(5);
   }
 
+  @Test
+  public void shouldGetEmptyBlockPeriodFromConfig() {
+    final BftConfigOptions config = fromConfigOptions(singletonMap("emptyblockperiodseconds", 60));
+    assertThat(config.getEmptyBlockPeriodSeconds()).isEqualTo(60);
+  }
+
   @Test
   public void shouldFallbackToDefaultBlockPeriod() {
     final BftConfigOptions config = fromConfigOptions(emptyMap());
     assertThat(config.getBlockPeriodSeconds()).isEqualTo(EXPECTED_DEFAULT_BLOCK_PERIOD);
   }
 
+  @Test
+  public void shouldFallbackToEmptyDefaultBlockPeriod() {
+    final BftConfigOptions config = fromConfigOptions(emptyMap());
+    assertThat(config.getEmptyBlockPeriodSeconds()).isEqualTo(EXPECTED_EMPTY_DEFAULT_BLOCK_PERIOD);
+  }
+
   @Test
   public void shouldGetDefaultBlockPeriodFromDefaultConfig() {
     assertThat(JsonBftConfigOptions.DEFAULT.getBlockPeriodSeconds())
         .isEqualTo(EXPECTED_DEFAULT_BLOCK_PERIOD);
   }
 
+  @Test
+  public void shouldGetDefaultEmptyBlockPeriodFromDefaultConfig() {
+
+    assertThat(JsonBftConfigOptions.DEFAULT.getEmptyBlockPeriodSeconds())
+            .isEqualTo(EXPECTED_EMPTY_DEFAULT_BLOCK_PERIOD);
+  }
+
   @Test
   public void shouldThrowOnNonPositiveBlockPeriod() {
     final BftConfigOptions config = fromConfigOptions(singletonMap("blockperiodseconds", -1));
     assertThatThrownBy(() -> config.getBlockPeriodSeconds())
         .isInstanceOf(IllegalArgumentException.class);
   }
 
+  @Test
+  public void shouldNotThrowOnNonPositiveEmptyBlockPeriod() {
+    // can be 0 to be compatible with older versions
+    final BftConfigOptions config = fromConfigOptions(singletonMap("emptyblockperiodseconds", 0));
+    assertThatCode(() -> config.getEmptyBlockPeriodSeconds()).doesNotThrowAnyException();
+  }
+
   @Test
   public void shouldGetRequestTimeoutFromConfig() {
     final BftConfigOptions config = fromConfigOptions(singletonMap("requesttimeoutseconds", 5));

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

@@ -24,14 +24,20 @@
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /** Class for starting and keeping organised block timers */
 public class BlockTimer {
 
+  private static final Logger LOG = LoggerFactory.getLogger(BlockTimer.class);
   private final ForksSchedule<? extends BftConfigOptions> forksSchedule;
   private final BftExecutors bftExecutors;
   private Optional<ScheduledFuture<?>> currentTimerTask;
   private final BftEventQueue queue;
   private final Clock clock;
+  private long blockPeriodSeconds;
+  private long emptyBlockPeriodSeconds;
 
   /**
    * Construct a BlockTimer with primed executor service ready to start timers
@@ -51,6 +57,8 @@ public BlockTimer(
     this.bftExecutors = bftExecutors;
     this.currentTimerTask = Optional.empty();
     this.clock = clock;
+    this.blockPeriodSeconds = forksSchedule.getFork(0).getValue().getBlockPeriodSeconds();
+    this.emptyBlockPeriodSeconds = forksSchedule.getFork(0).getValue().getEmptyBlockPeriodSeconds();
   }
 
   /** Cancels the current running round timer if there is one */
@@ -76,16 +84,36 @@ public synchronized boolean isRunning() {
    */
   public synchronized void startTimer(
       final ConsensusRoundIdentifier round, final BlockHeader chainHeadHeader) {
-    cancelTimer();
-
-    final long now = clock.millis();
 
     // absolute time when the timer is supposed to expire
-    final int blockPeriodSeconds =
+    final int currentBlockPeriodSeconds =
         forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodSeconds();
-    final long minimumTimeBetweenBlocksMillis = blockPeriodSeconds * 1000L;
+    final long minimumTimeBetweenBlocksMillis = currentBlockPeriodSeconds * 1000L;
     final long expiryTime = chainHeadHeader.getTimestamp() * 1_000 + minimumTimeBetweenBlocksMillis;
 
+    setBlockTimes(round, false);
+
+    startTimer(round, expiryTime);
+  }
+
+  public synchronized void startEmptyBlockTimer(
+          final ConsensusRoundIdentifier round, final BlockHeader chainHeadHeader) {
+
+    // absolute time when the timer is supposed to expire
+    final int currentBlockPeriodSeconds =
+            forksSchedule.getFork(round.getSequenceNumber()).getValue().getEmptyBlockPeriodSeconds();
+    final long minimumTimeBetweenBlocksMillis = currentBlockPeriodSeconds * 1000L;
+    final long expiryTime = chainHeadHeader.getTimestamp() * 1_000 + minimumTimeBetweenBlocksMillis;
+
+    setBlockTimes(round, true);
+
+    startTimer(round, expiryTime);
+  }
+
+  private synchronized void startTimer(final ConsensusRoundIdentifier round, final long expiryTime) {
+    cancelTimer();
+    final long now = clock.millis();
+
     if (expiryTime > now) {
       final long delay = expiryTime - now;
 
@@ -98,4 +126,27 @@ public synchronized void startTimer(
       queue.add(new BlockTimerExpiry(round));
     }
   }
+
+  private synchronized void setBlockTimes(final ConsensusRoundIdentifier round, final boolean isEmpty) {
+    final BftConfigOptions currentConfigOptions =  forksSchedule.getFork(round.getSequenceNumber()).getValue();
+    this.blockPeriodSeconds = currentConfigOptions.getBlockPeriodSeconds();
+    this.emptyBlockPeriodSeconds = currentConfigOptions.getEmptyBlockPeriodSeconds();
+
+    long currentBlockPeriodSeconds = isEmpty && this.emptyBlockPeriodSeconds > 0
+            ? this.emptyBlockPeriodSeconds
+            : this.blockPeriodSeconds;
+
+    LOG.debug(
+            "NEW CURRENTBLOCKPERIODSECONDS SET TO {}:  {}"
+            , isEmpty?"EMPTYBLOCKPERIODSECONDS":"BLOCKPERIODSECONDS"
+            , currentBlockPeriodSeconds);
+  }
+
+  public synchronized long getBlockPeriodSeconds(){
+    return blockPeriodSeconds;
+  }
+
+  public synchronized long getEmptyBlockPeriodSeconds(){
+    return emptyBlockPeriodSeconds;
+  }
 }

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

@@ -24,13 +24,19 @@
 import java.util.Map;
 import java.util.Optional;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * A mutable {@link BftConfigOptions} that is used for building config for transitions in the {@link
  * ForksSchedule}*.
  */
 public class MutableBftConfigOptions implements BftConfigOptions {
+
+  private static final Logger LOG = LoggerFactory.getLogger(MutableBftConfigOptions.class);
   private long epochLength;
   private int blockPeriodSeconds;
+  private int emptyBlockPeriodSeconds;
   private int requestTimeoutSeconds;
   private int gossipedHistoryLimit;
   private int messageQueueLimit;
@@ -48,6 +54,7 @@ public class MutableBftConfigOptions implements BftConfigOptions {
   public MutableBftConfigOptions(final BftConfigOptions bftConfigOptions) {
     this.epochLength = bftConfigOptions.getEpochLength();
     this.blockPeriodSeconds = bftConfigOptions.getBlockPeriodSeconds();
+    this.emptyBlockPeriodSeconds = bftConfigOptions.getEmptyBlockPeriodSeconds();
     this.requestTimeoutSeconds = bftConfigOptions.getRequestTimeoutSeconds();
     this.gossipedHistoryLimit = bftConfigOptions.getGossipedHistoryLimit();
     this.messageQueueLimit = bftConfigOptions.getMessageQueueLimit();
@@ -65,9 +72,16 @@ public long getEpochLength() {
 
   @Override
   public int getBlockPeriodSeconds() {
+    LOG.debug("GET BLOCKPERIODSECONDS: " + blockPeriodSeconds);
     return blockPeriodSeconds;
   }
 
+  @Override
+  public int getEmptyBlockPeriodSeconds() {
+    LOG.debug("GET EMPTYBLOCKPERIODSECONDS: " + emptyBlockPeriodSeconds);
+    return emptyBlockPeriodSeconds;
+  }
+
   @Override
   public int getRequestTimeoutSeconds() {
     return requestTimeoutSeconds;
@@ -128,9 +142,20 @@ public void setEpochLength(final long epochLength) {
    * @param blockPeriodSeconds the block period seconds
    */
   public void setBlockPeriodSeconds(final int blockPeriodSeconds) {
+    LOG.info("SET BLOCKPERIODSECONDS: " + blockPeriodSeconds);
     this.blockPeriodSeconds = blockPeriodSeconds;
   }
 
+  /**
+   * Sets empty block period seconds.
+   *
+   * @param emptyBlockPeriodSeconds the empty block period seconds
+   */
+  public void setEmptyBlockPeriodSeconds(final int emptyBlockPeriodSeconds) {
+    LOG.info("SET EMPTYBLOCKPERIODSECONDS: " + emptyBlockPeriodSeconds);
+    this.emptyBlockPeriodSeconds = emptyBlockPeriodSeconds;
+  }
+
   /**
    * Sets request timeout seconds.
    *

consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java

@@ -37,7 +37,7 @@ public class ForksScheduleFactoryTest {
   @SuppressWarnings("unchecked")
   public void throwsErrorIfHasForkForGenesisBlock() {
     final BftConfigOptions genesisConfigOptions = JsonBftConfigOptions.DEFAULT;
-    final BftFork fork = createFork(0, 10);
+    final BftFork fork = createFork(0, 10, 30);
     final SpecCreator<BftConfigOptions, BftFork> specCreator = Mockito.mock(SpecCreator.class);
 
     assertThatThrownBy(
@@ -49,9 +49,9 @@ public void throwsErrorIfHasForkForGenesisBlock() {
   @SuppressWarnings("unchecked")
   public void throwsErrorIfHasForksWithDuplicateBlock() {
     final BftConfigOptions genesisConfigOptions = JsonBftConfigOptions.DEFAULT;
-    final BftFork fork1 = createFork(1, 10);
-    final BftFork fork2 = createFork(1, 20);
-    final BftFork fork3 = createFork(2, 30);
+    final BftFork fork1 = createFork(1, 10, 30);
+    final BftFork fork2 = createFork(1, 20, 60);
+    final BftFork fork3 = createFork(2, 30, 90);
     final SpecCreator<BftConfigOptions, BftFork> specCreator = Mockito.mock(SpecCreator.class);
 
     assertThatThrownBy(
@@ -82,18 +82,20 @@ public void createsScheduleUsingSpecCreator() {
     assertThat(schedule.getFork(2)).isEqualTo(new ForkSpec<>(2, configOptions2));
   }
 
-  private MutableBftConfigOptions createBftConfigOptions(final int blockPeriodSeconds) {
+  private MutableBftConfigOptions createBftConfigOptions(final int blockPeriodSeconds, final int emptyBlockPeriodSeconds) {
     final MutableBftConfigOptions bftConfigOptions =
         new MutableBftConfigOptions(JsonBftConfigOptions.DEFAULT);
     bftConfigOptions.setBlockPeriodSeconds(blockPeriodSeconds);
+    bftConfigOptions.setEmptyBlockPeriodSeconds(emptyBlockPeriodSeconds);
     return bftConfigOptions;
   }
 
-  private BftFork createFork(final long block, final long blockPeriodSeconds) {
+  private BftFork createFork(final long block, final long blockPeriodSeconds, final long emptyBlockPeriodSeconds) {
     return new BftFork(
         JsonUtil.objectNodeFromMap(
             Map.of(
                 BftFork.FORK_BLOCK_KEY, block,
-                BftFork.BLOCK_PERIOD_SECONDS_KEY, blockPeriodSeconds)));
+                    BftFork.BLOCK_PERIOD_SECONDS_KEY, blockPeriodSeconds,
+                    BftFork.EMPTY_BLOCK_PERIOD_SECONDS_KEY, emptyBlockPeriodSeconds)));
   }
 }