If a PoS block creation repetition takes less than a configurable dur… (#5048)

* If a PoS block creation repetition takes less than a configurable duration, then waits before next repetition

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

* Update CHANGELOG

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

* Add unit test

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

* Update besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java

Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

* Update besu/src/main/java/org/hyperledger/besu/cli/options/unstable/MiningOptions.java

Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>

---------

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>

CHANGELOG.md

@@ -27,6 +27,7 @@ tests are updated to use EC private keys instead of RSA keys.
 - Besu requires minimum Java 17 and up to build and run [#3320](https://github.com/hyperledger/besu/issues/3320)
 - Add worldstate auto-heal mechanism [#5059](https://github.com/hyperledger/besu/pull/5059)
 - Support for EIP-4895 - Withdrawals for Shanghai fork
+- If a PoS block creation repetition takes less than a configurable duration, then waits before next repetition [#5048](https://github.com/hyperledger/besu/pull/5048)
 
 ### Bug Fixes
 - Mitigation fix for stale bonsai code storage leading to log rolling issues on contract recreates [#4906](https://github.com/hyperledger/besu/pull/4906)

besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java

@@ -1891,6 +1891,13 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
       throw new ParameterException(
           this.commandLine, "--Xpos-block-creation-max-time must be positive and ≤ 12000");
     }
+
+    if (unstableMiningOptions.getPosBlockCreationRepetitionMinDuration() <= 0
+        || unstableMiningOptions.getPosBlockCreationRepetitionMinDuration() > 2000) {
+      throw new ParameterException(
+          this.commandLine,
+          "--Xpos-block-creation-repetition-min-duration must be positive and ≤ 2000");
+    }
   }
 
   /**
@@ -2271,6 +2278,8 @@ public class BesuCommand implements DefaultCommandValues, Runnable {
                 .powJobTimeToLive(unstableMiningOptions.getPowJobTimeToLive())
                 .maxOmmerDepth(unstableMiningOptions.getMaxOmmersDepth())
                 .posBlockCreationMaxTime(unstableMiningOptions.getPosBlockCreationMaxTime())
+                .posBlockCreationRepetitionMinDuration(
+                    unstableMiningOptions.getPosBlockCreationRepetitionMinDuration())
                 .build())
         .transactionPoolConfiguration(buildTransactionPoolConfiguration())
         .nodeKey(new NodeKey(securityModule()))

besu/src/main/java/org/hyperledger/besu/cli/options/unstable/MiningOptions.java

@@ -16,6 +16,7 @@ package org.hyperledger.besu.cli.options.unstable;
 
 import static org.hyperledger.besu.ethereum.core.MiningParameters.DEFAULT_MAX_OMMERS_DEPTH;
 import static org.hyperledger.besu.ethereum.core.MiningParameters.DEFAULT_POS_BLOCK_CREATION_MAX_TIME;
+import static org.hyperledger.besu.ethereum.core.MiningParameters.DEFAULT_POS_BLOCK_CREATION_REPETITION_MIN_DURATION;
 import static org.hyperledger.besu.ethereum.core.MiningParameters.DEFAULT_POW_JOB_TTL;
 import static org.hyperledger.besu.ethereum.core.MiningParameters.DEFAULT_REMOTE_SEALERS_LIMIT;
 import static org.hyperledger.besu.ethereum.core.MiningParameters.DEFAULT_REMOTE_SEALERS_TTL;
@@ -67,6 +68,15 @@ public class MiningOptions {
           "Specifies the maximum time, in milliseconds, a PoS block creation jobs is allowed to run. Must be positive and ≤ 12000 (default: ${DEFAULT-VALUE} milliseconds)")
   private final Long posBlockCreationMaxTime = DEFAULT_POS_BLOCK_CREATION_MAX_TIME;
 
+  @CommandLine.Option(
+      hidden = true,
+      names = {"--Xpos-block-creation-repetition-min-duration"},
+      description =
+          "If a PoS block creation repetition takes less than this duration, in milliseconds,"
+              + " then it waits before next repetition. Must be positive and ≤ 2000 (default: ${DEFAULT-VALUE} milliseconds)")
+  private final Long posBlockCreationRepetitionMinDuration =
+      DEFAULT_POS_BLOCK_CREATION_REPETITION_MIN_DURATION;
+
   /**
    * Create mining options.
    *
@@ -129,4 +139,13 @@ public class MiningOptions {
   public Long getPosBlockCreationMaxTime() {
     return posBlockCreationMaxTime;
   }
+
+  /**
+   * Gets pos block creation repetition min duration.
+   *
+   * @return the pos block creation repetition min duration.
+   */
+  public Long getPosBlockCreationRepetitionMinDuration() {
+    return posBlockCreationRepetitionMinDuration;
+  }
 }

consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java

@@ -344,10 +344,20 @@ public class MergeCoordinator implements MergeMiningCoordinator, BadChainListene
   private Void retryBlockCreationUntilUseful(
       final PayloadIdentifier payloadIdentifier, final Supplier<BlockCreationResult> blockCreator) {
 
+    long lastStartAt;
+
     while (!isBlockCreationCancelled(payloadIdentifier)) {
       try {
-        recoverableBlockCreation(payloadIdentifier, blockCreator, System.currentTimeMillis());
-      } catch (final CancellationException ce) {
+        lastStartAt = System.currentTimeMillis();
+        recoverableBlockCreation(payloadIdentifier, blockCreator, lastStartAt);
+        final long lastDuration = System.currentTimeMillis() - lastStartAt;
+        final long waitBeforeRepetition =
+            miningParameters.getPosBlockCreationRepetitionMinDuration() - lastDuration;
+        if (waitBeforeRepetition > 0) {
+          LOG.debug("Waiting {}ms before repeating block creation", waitBeforeRepetition);
+          Thread.sleep(waitBeforeRepetition);
+        }
+      } catch (final CancellationException | InterruptedException ce) {
         debugLambda(
             LOG,
             "Block creation for payload id {} has been cancelled, reason {}",

consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java

@@ -75,6 +75,7 @@ import org.hyperledger.besu.metrics.StubMetricsSystem;
 import org.hyperledger.besu.testutil.TestClock;
 
 import java.time.ZoneId;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
 import java.util.concurrent.CompletableFuture;
@@ -114,6 +115,8 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
   private static final KeyPair KEYS1 =
       new KeyPair(PRIVATE_KEY1, SIGNATURE_ALGORITHM.get().createPublicKey(PRIVATE_KEY1));
   private static final Optional<List<Withdrawal>> EMPTY_WITHDRAWALS = Optional.empty();
+
+  private static final long REPETITION_MIN_DURATION = 100;
   @Mock MergeContext mergeContext;
   @Mock BackwardSyncContext backwardSyncContext;
 
@@ -121,7 +124,11 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
   private final Address coinbase = genesisAllocations(getPosGenesisConfigFile()).findFirst().get();
 
   @Spy
-  MiningParameters miningParameters = new MiningParameters.Builder().coinbase(coinbase).build();
+  MiningParameters miningParameters =
+      new MiningParameters.Builder()
+          .coinbase(coinbase)
+          .posBlockCreationRepetitionMinDuration(REPETITION_MIN_DURATION)
+          .build();
 
   private MergeCoordinator coordinator;
   private ProtocolContext protocolContext;
@@ -353,6 +360,51 @@ public class MergeCoordinatorTest implements MergeGenesisConfigHelper {
     }
   }
 
+  @Test
+  public void blockCreationRepetitionShouldTakeNotLessThanRepetitionMinDuration()
+      throws InterruptedException, ExecutionException {
+    final AtomicLong retries = new AtomicLong(0);
+    final AtomicLong lastPutAt = new AtomicLong();
+    final List<Long> repetitionDurations = new ArrayList<>();
+
+    doAnswer(
+            invocation -> {
+              final long r = retries.getAndIncrement();
+              if (r == 0) {
+                // ignore first one, that is the empty block
+              } else if (r < 5) {
+                if (lastPutAt.get() > 0) {
+                  // each repetition should take >= REPETITION_MIN_DURATION
+                  repetitionDurations.add(System.currentTimeMillis() - lastPutAt.get());
+                }
+                lastPutAt.set(System.currentTimeMillis());
+              } else {
+                // finalize after 5 repetitions
+                coordinator.finalizeProposalById(
+                    invocation.getArgument(0, PayloadIdentifier.class));
+              }
+              return null;
+            })
+        .when(mergeContext)
+        .putPayloadById(any(), any());
+
+    var payloadId =
+        coordinator.preparePayload(
+            genesisState.getBlock().getHeader(),
+            System.currentTimeMillis() / 1000,
+            Bytes32.ZERO,
+            suggestedFeeRecipient,
+            Optional.empty());
+
+    blockCreationTask.get();
+
+    verify(mergeContext, times(retries.intValue())).putPayloadById(eq(payloadId), any());
+
+    // check with a tolerance
+    assertThat(repetitionDurations)
+        .allSatisfy(d -> assertThat(d).isGreaterThanOrEqualTo(REPETITION_MIN_DURATION - 10));
+  }
+
   @Test
   public void shouldRetryBlockCreationOnRecoverableError()
       throws InterruptedException, ExecutionException {

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/MiningParameters.java

@@ -36,6 +36,9 @@ public class MiningParameters {
 
   public static final long DEFAULT_POS_BLOCK_CREATION_MAX_TIME = Duration.ofSeconds(12).toMillis();
 
+  public static final long DEFAULT_POS_BLOCK_CREATION_REPETITION_MIN_DURATION =
+      Duration.ofMillis(500).toMillis();
+
   private final Optional<Address> coinbase;
   private final Optional<AtomicLong> targetGasLimit;
   private final Wei minTransactionGasPrice;
@@ -52,6 +55,7 @@ public class MiningParameters {
   private final long powJobTimeToLive;
   private final int maxOmmerDepth;
   private final long posBlockCreationMaxTime;
+  private final long posBlockCreationRepetitionMinDuration;
 
   private MiningParameters(
       final Address coinbase,
@@ -69,7 +73,8 @@ public class MiningParameters {
       final long remoteSealersTimeToLive,
       final long powJobTimeToLive,
       final int maxOmmerDepth,
-      final long posBlockCreationMaxTime) {
+      final long posBlockCreationMaxTime,
+      final long posBlockCreationRepetitionMinDuration) {
     this.coinbase = Optional.ofNullable(coinbase);
     this.targetGasLimit = Optional.ofNullable(targetGasLimit).map(AtomicLong::new);
     this.minTransactionGasPrice = minTransactionGasPrice;
@@ -86,6 +91,7 @@ public class MiningParameters {
     this.powJobTimeToLive = powJobTimeToLive;
     this.maxOmmerDepth = maxOmmerDepth;
     this.posBlockCreationMaxTime = posBlockCreationMaxTime;
+    this.posBlockCreationRepetitionMinDuration = posBlockCreationRepetitionMinDuration;
   }
 
   public Optional<Address> getCoinbase() {
@@ -152,6 +158,10 @@ public class MiningParameters {
     return posBlockCreationMaxTime;
   }
 
+  public long getPosBlockCreationRepetitionMinDuration() {
+    return posBlockCreationRepetitionMinDuration;
+  }
+
   @Override
   public boolean equals(final Object o) {
     if (this == o) return true;
@@ -170,7 +180,8 @@ public class MiningParameters {
         && remoteSealersTimeToLive == that.remoteSealersTimeToLive
         && remoteSealersLimit == that.remoteSealersLimit
         && powJobTimeToLive == that.powJobTimeToLive
-        && posBlockCreationMaxTime == that.posBlockCreationMaxTime;
+        && posBlockCreationMaxTime == that.posBlockCreationMaxTime
+        && posBlockCreationRepetitionMinDuration == that.posBlockCreationRepetitionMinDuration;
   }
 
   @Override
@@ -189,7 +200,8 @@ public class MiningParameters {
         remoteSealersLimit,
         remoteSealersTimeToLive,
         powJobTimeToLive,
-        posBlockCreationMaxTime);
+        posBlockCreationMaxTime,
+        posBlockCreationRepetitionMinDuration);
   }
 
   @Override
@@ -227,6 +239,8 @@ public class MiningParameters {
         + powJobTimeToLive
         + ", posBlockCreationMaxTime="
         + posBlockCreationMaxTime
+        + ", posBlockCreationRepetitionMinDuration="
+        + posBlockCreationRepetitionMinDuration
         + '}';
   }
 
@@ -249,6 +263,9 @@ public class MiningParameters {
     private int maxOmmerDepth = DEFAULT_MAX_OMMERS_DEPTH;
     private long posBlockCreationMaxTime = DEFAULT_POS_BLOCK_CREATION_MAX_TIME;
 
+    private long posBlockCreationRepetitionMinDuration =
+        DEFAULT_POS_BLOCK_CREATION_REPETITION_MIN_DURATION;
+
     public Builder() {
       // zero arg
     }
@@ -273,6 +290,8 @@ public class MiningParameters {
       this.powJobTimeToLive = existing.getPowJobTimeToLive();
       this.maxOmmerDepth = existing.getMaxOmmerDepth();
       this.posBlockCreationMaxTime = existing.getPosBlockCreationMaxTime();
+      this.posBlockCreationRepetitionMinDuration =
+          existing.getPosBlockCreationRepetitionMinDuration();
     }
 
     public Builder coinbase(final Address address) {
@@ -355,6 +374,12 @@ public class MiningParameters {
       return this;
     }
 
+    public Builder posBlockCreationRepetitionMinDuration(
+        final long posBlockCreationRepetitionMinDuration) {
+      this.posBlockCreationRepetitionMinDuration = posBlockCreationRepetitionMinDuration;
+      return this;
+    }
+
     public MiningParameters build() {
       return new MiningParameters(
           coinbase,
@@ -372,7 +397,8 @@ public class MiningParameters {
           remoteSealersTimeToLive,
           powJobTimeToLive,
           maxOmmerDepth,
-          posBlockCreationMaxTime);
+          posBlockCreationMaxTime,
+          posBlockCreationRepetitionMinDuration);
     }
   }
 }