Make block txs selection time PoA transaction aware

Signed-off-by: Fabio Di Fabio <fabio.difabio@consensys.net>
hyperledger · Mar 4, 2024 · b6cd237 · b6cd237
1 parent dbc128f
commit b6cd237
Show file tree

Hide file tree

Showing 11 changed files with 110 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@
 - Update commons-compress to 1.26.0 [#6648](https://github.com/hyperledger/besu/pull/6648)
 
 ### Bug fixes
+- Make block transaction selection max time aware of PoA transactions [#6676](https://github.com/hyperledger/besu/pull/6676)
 
 ### Download Links
 

diff --git a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
@@ -2156,10 +2156,10 @@ private TransactionPoolConfiguration buildTransactionPoolConfiguration() {
 
   private MiningParameters getMiningParameters() {
     if (miningParameters == null) {
-      miningOptions.setGenesisBlockPeriodSeconds(
-          getGenesisBlockPeriodSeconds(getActualGenesisConfigOptions()));
       miningOptions.setTransactionSelectionService(transactionSelectionServiceImpl);
       miningParameters = miningOptions.toDomainObject();
+      getGenesisBlockPeriodSeconds(getActualGenesisConfigOptions())
+          .ifPresent(miningParameters::setBlockPeriodSeconds);
       initMiningParametersMetrics(miningParameters);
     }
     return miningParameters;

diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/MiningOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/MiningOptions.java
@@ -42,7 +42,6 @@
 import org.hyperledger.besu.util.number.PositiveNumber;
 
 import java.util.List;
-import java.util.OptionalInt;
 
 import org.apache.tuweni.bytes.Bytes;
 import org.slf4j.Logger;
@@ -191,7 +190,6 @@ static class Unstable {
         DEFAULT_POS_BLOCK_CREATION_REPETITION_MIN_DURATION;
   }
 
-  private OptionalInt maybeGenesisBlockPeriodSeconds;
   private TransactionSelectionService transactionSelectionService;
 
   private MiningOptions() {}
@@ -205,16 +203,6 @@ public static MiningOptions create() {
     return new MiningOptions();
   }
 
-  /**
-   * Set the optional genesis block period per seconds
-   *
-   * @param genesisBlockPeriodSeconds if the network is PoA then the block period in seconds
-   *     specified in the genesis file, otherwise empty.
-   */
-  public void setGenesisBlockPeriodSeconds(final OptionalInt genesisBlockPeriodSeconds) {
-    maybeGenesisBlockPeriodSeconds = genesisBlockPeriodSeconds;
-  }
-
   /**
    * Set the transaction selection service
    *
@@ -311,7 +299,6 @@ public void validate(
 
   static MiningOptions fromConfig(final MiningParameters miningParameters) {
     final MiningOptions miningOptions = MiningOptions.create();
-    miningOptions.setGenesisBlockPeriodSeconds(miningParameters.getGenesisBlockPeriodSeconds());
     miningOptions.setTransactionSelectionService(miningParameters.getTransactionSelectionService());
     miningOptions.isMiningEnabled = miningParameters.isMiningEnabled();
     miningOptions.iStratumMiningEnabled = miningParameters.isStratumMiningEnabled();
@@ -347,9 +334,6 @@ static MiningOptions fromConfig(final MiningParameters miningParameters) {
 
   @Override
   public MiningParameters toDomainObject() {
-    checkNotNull(
-        maybeGenesisBlockPeriodSeconds,
-        "genesisBlockPeriodSeconds must be set before using this object");
     checkNotNull(
         transactionSelectionService,
         "transactionSelectionService must be set before using this object");
@@ -370,7 +354,6 @@ public MiningParameters toDomainObject() {
     }
 
     return ImmutableMiningParameters.builder()
-        .genesisBlockPeriodSeconds(maybeGenesisBlockPeriodSeconds)
         .transactionSelectionService(transactionSelectionService)
         .mutableInitValues(updatableInitValuesBuilder.build())
         .isStratumMiningEnabled(iStratumMiningEnabled)

diff --git a/besu/src/main/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilder.java
@@ -92,7 +92,8 @@ protected MiningCoordinator createMiningCoordinator(
                 clock,
                 protocolContext.getConsensusContext(CliqueContext.class).getValidatorProvider(),
                 localAddress,
-                forksSchedule),
+                forksSchedule,
+                miningParameters),
             epochManager,
             forksSchedule,
             ethProtocolManager.ethContext().getScheduler());

diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/AbstractCLIOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/AbstractCLIOptionsTest.java
@@ -22,6 +22,7 @@
 
 import java.util.Collections;
 import java.util.List;
+import java.util.function.BiFunction;
 import java.util.function.Consumer;
 
 import org.junit.jupiter.api.Test;
@@ -113,10 +114,18 @@ protected List<String> getFieldsToIgnore() {
   protected abstract T getOptionsFromBesuCommand(final TestBesuCommand besuCommand);
 
   protected void internalTestSuccess(final Consumer<D> assertion, final String... args) {
+    internalTestSuccess((bc, conf) -> conf, assertion, args);
+  }
+
+  protected void internalTestSuccess(
+      final BiFunction<TestBesuCommand, D, D> runtimeConf,
+      final Consumer<D> assertion,
+      final String... args) {
     final TestBesuCommand cmd = parseCommand(args);
 
     final T options = getOptionsFromBesuCommand(cmd);
-    final D config = options.toDomainObject();
+    final D config = runtimeConf.apply(cmd, options.toDomainObject());
+
     assertion.accept(config);
 
     assertThat(commandOutput.toString(UTF_8)).isEmpty();

diff --git a/besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java b/besu/src/test/java/org/hyperledger/besu/cli/options/MiningOptionsTest.java
@@ -34,7 +34,6 @@
 import java.nio.file.Path;
 import java.time.Duration;
 import java.util.Optional;
-import java.util.OptionalInt;
 
 import org.apache.tuweni.bytes.Bytes;
 import org.junit.jupiter.api.Test;
@@ -316,6 +315,7 @@ public void posBlockCreationMaxTimeOutOfAllowedRange() {
   @Test
   public void blockTxsSelectionMaxTimeDefaultValue() {
     internalTestSuccess(
+        this::runtimeConfiguration,
         miningParams ->
             assertThat(miningParams.getNonPoaBlockTxsSelectionMaxTime())
                 .isEqualTo(DEFAULT_NON_POA_BLOCK_TXS_SELECTION_MAX_TIME));
@@ -324,6 +324,7 @@ public void blockTxsSelectionMaxTimeDefaultValue() {
   @Test
   public void blockTxsSelectionMaxTimeOption() {
     internalTestSuccess(
+        this::runtimeConfiguration,
         miningParams -> assertThat(miningParams.getBlockTxsSelectionMaxTime()).isEqualTo(1700L),
         "--block-txs-selection-max-time",
         "1700");
@@ -343,6 +344,7 @@ public void blockTxsSelectionMaxTimeIncompatibleWithPoaNetworks() throws IOExcep
   @Test
   public void poaBlockTxsSelectionMaxTimeDefaultValue() {
     internalTestSuccess(
+        this::runtimeConfiguration,
         miningParams ->
             assertThat(miningParams.getPoaBlockTxsSelectionMaxTime())
                 .isEqualTo(DEFAULT_POA_BLOCK_TXS_SELECTION_MAX_TIME));
@@ -352,6 +354,7 @@ public void poaBlockTxsSelectionMaxTimeDefaultValue() {
   public void poaBlockTxsSelectionMaxTimeOption() throws IOException {
     final Path genesisFileIBFT2 = createFakeGenesisFile(VALID_GENESIS_IBFT2_POST_LONDON);
     internalTestSuccess(
+        this::runtimeConfiguration,
         miningParams ->
             assertThat(miningParams.getPoaBlockTxsSelectionMaxTime())
                 .isEqualTo(PositiveNumber.fromInt(80)),
@@ -365,6 +368,7 @@ public void poaBlockTxsSelectionMaxTimeOption() throws IOException {
   public void poaBlockTxsSelectionMaxTimeOptionOver100Percent() throws IOException {
     final Path genesisFileClique = createFakeGenesisFile(VALID_GENESIS_CLIQUE_POST_LONDON);
     internalTestSuccess(
+        this::runtimeConfiguration,
         miningParams -> {
           assertThat(miningParams.getPoaBlockTxsSelectionMaxTime())
               .isEqualTo(PositiveNumber.fromInt(200));
@@ -412,16 +416,19 @@ protected MiningOptions optionsFromDomainObject(final MiningParameters domainObj
 
   @Override
   protected MiningOptions getOptionsFromBesuCommand(final TestBesuCommand besuCommand) {
-    final var miningOptions = besuCommand.getMiningOptions();
-    miningOptions.setGenesisBlockPeriodSeconds(
-        besuCommand.getActualGenesisConfigOptions().isPoa()
-            ? OptionalInt.of(POA_BLOCK_PERIOD_SECONDS)
-            : OptionalInt.empty());
-    return miningOptions;
+    return besuCommand.getMiningOptions();
   }
 
   @Override
   protected String[] getNonOptionFields() {
-    return new String[] {"maybeGenesisBlockPeriodSeconds", "transactionSelectionService"};
+    return new String[] {"transactionSelectionService"};
+  }
+
+  private MiningParameters runtimeConfiguration(
+      final TestBesuCommand besuCommand, final MiningParameters miningParameters) {
+    if (besuCommand.getActualGenesisConfigOptions().isPoa()) {
+      miningParameters.setBlockPeriodSeconds(POA_BLOCK_PERIOD_SECONDS);
+    }
+    return miningParameters;
   }
 }
diff --git a/...c/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockScheduler.java b/...c/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockScheduler.java
@@ -20,6 +20,7 @@
 import org.hyperledger.besu.datatypes.Address;
 import org.hyperledger.besu.ethereum.blockcreation.DefaultBlockScheduler;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
+import org.hyperledger.besu.ethereum.core.MiningParameters;
 
 import java.time.Clock;
 import java.util.Collection;
@@ -43,19 +44,24 @@ public class CliqueBlockScheduler extends DefaultBlockScheduler {
    * @param validatorProvider the validator provider
    * @param localNodeAddress the local node address
    * @param forksSchedule the transitions
+   * @param miningParameters the mining parameters to set the block period in seconds
    */
   public CliqueBlockScheduler(
       final Clock clock,
       final ValidatorProvider validatorProvider,
       final Address localNodeAddress,
-      final ForksSchedule<CliqueConfigOptions> forksSchedule) {
+      final ForksSchedule<CliqueConfigOptions> forksSchedule,
+      final MiningParameters miningParameters) {
     super(
-        parentHeader ->
-            (long)
-                forksSchedule
-                    .getFork(parentHeader.getNumber() + 1)
-                    .getValue()
-                    .getBlockPeriodSeconds(),
+        parentHeader -> {
+          final var blockPeriodSeconds =
+              forksSchedule
+                  .getFork(parentHeader.getNumber() + 1)
+                  .getValue()
+                  .getBlockPeriodSeconds();
+          miningParameters.setBlockPeriodSeconds(blockPeriodSeconds);
+          return (long) blockPeriodSeconds;
+        },
         0L,
         clock);
     this.validatorProvider = validatorProvider;

diff --git a/...st/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockSchedulerTest.java b/...st/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockSchedulerTest.java
@@ -32,7 +32,10 @@
 import org.hyperledger.besu.ethereum.core.AddressHelpers;
 import org.hyperledger.besu.ethereum.core.BlockHeader;
 import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture;
+import org.hyperledger.besu.ethereum.core.ImmutableMiningParameters;
+import org.hyperledger.besu.ethereum.core.MiningParameters;
 import org.hyperledger.besu.ethereum.core.Util;
+import org.hyperledger.besu.util.number.PositiveNumber;
 
 import java.time.Clock;
 import java.util.List;
@@ -42,14 +45,16 @@
 import org.junit.jupiter.api.Test;
 
 public class CliqueBlockSchedulerTest {
-
+  private static final PositiveNumber BLOCK_TXS_SELECTION_MAX_PERCENTAGE =
+      PositiveNumber.fromInt(75);
   private final KeyPair proposerKeyPair = SignatureAlgorithmFactory.getInstance().generateKeyPair();
   private Address localAddr;
 
   private final List<Address> validatorList = Lists.newArrayList();
   private ValidatorProvider validatorProvider;
   private BlockHeaderTestFixture blockHeaderBuilder;
   private ForksSchedule<CliqueConfigOptions> forksSchedule;
+  private MiningParameters miningParameters;
 
   @BeforeEach
   public void setup() {
@@ -67,6 +72,12 @@ public void setup() {
         ImmutableCliqueConfigOptions.builder().from(JsonCliqueConfigOptions.DEFAULT);
     initialTransition.blockPeriodSeconds(5);
     forksSchedule = new ForksSchedule<>(List.of(new ForkSpec<>(0, initialTransition.build())));
+
+    miningParameters =
+        ImmutableMiningParameters.builder()
+            .poaBlockTxsSelectionMaxTime(BLOCK_TXS_SELECTION_MAX_PERCENTAGE)
+            .build()
+            .setBlockPeriodSeconds(10);
   }
 
   @Test
@@ -76,7 +87,8 @@ public void inturnValidatorWaitsExactlyBlockInterval() {
     final int secondsBetweenBlocks = 5;
     when(clock.millis()).thenReturn(currentSecondsSinceEpoch * 1000);
     final CliqueBlockScheduler scheduler =
-        new CliqueBlockScheduler(clock, validatorProvider, localAddr, forksSchedule);
+        new CliqueBlockScheduler(
+            clock, validatorProvider, localAddr, forksSchedule, miningParameters);
 
     // There are 2 validators, therefore block 2 will put localAddr as the in-turn voter, therefore
     // parent block should be number 1.
@@ -88,6 +100,9 @@ public void inturnValidatorWaitsExactlyBlockInterval() {
     assertThat(result.getTimestampForHeader())
         .isEqualTo(currentSecondsSinceEpoch + secondsBetweenBlocks);
     assertThat(result.getMillisecondsUntilValid()).isEqualTo(secondsBetweenBlocks * 1000);
+    assertThat(miningParameters.getBlockTxsSelectionMaxTime())
+        .isEqualTo(
+            secondsBetweenBlocks * 1000 * BLOCK_TXS_SELECTION_MAX_PERCENTAGE.getValue() / 100);
   }
 
   @Test
@@ -109,7 +124,8 @@ public void validatorWithTransitionForBlockTimeWaitsBlockInterval() {
                 new ForkSpec<>(4, decreaseBlockTimeTransition.build())));
 
     final CliqueBlockScheduler scheduler =
-        new CliqueBlockScheduler(clock, validatorProvider, localAddr, forksSchedule);
+        new CliqueBlockScheduler(
+            clock, validatorProvider, localAddr, forksSchedule, miningParameters);
 
     // getNextTimestamp for last block before transition
     // There are 2 validators, therefore block 3 will put localAddr as the out-of-turn voter,
@@ -120,6 +136,8 @@ public void validatorWithTransitionForBlockTimeWaitsBlockInterval() {
     BlockCreationTimeResult result = scheduler.getNextTimestamp(parentHeader);
     assertThat(result.getTimestampForHeader()).isEqualTo(currentSecondsSinceEpoch + 5);
     assertThat(result.getMillisecondsUntilValid()).isGreaterThan(5 * 1000);
+    assertThat(miningParameters.getBlockTxsSelectionMaxTime())
+        .isEqualTo(5 * 1000 * BLOCK_TXS_SELECTION_MAX_PERCENTAGE.getValue() / 100);
 
     // getNextTimestamp for transition block
     // There are 2 validators, therefore block 4 will put localAddr as the in-turn voter, therefore
@@ -128,6 +146,8 @@ public void validatorWithTransitionForBlockTimeWaitsBlockInterval() {
     result = scheduler.getNextTimestamp(parentHeader);
     assertThat(result.getTimestampForHeader()).isEqualTo(currentSecondsSinceEpoch + 1);
     assertThat(result.getMillisecondsUntilValid()).isEqualTo(1000);
+    assertThat(miningParameters.getBlockTxsSelectionMaxTime())
+        .isEqualTo(1 * 1000 * BLOCK_TXS_SELECTION_MAX_PERCENTAGE.getValue() / 100);
 
     // getNextTimestamp for block after transition
     // There are 2 validators, therefore block 5 will put localAddr as the out-of-turn voter,
@@ -137,6 +157,8 @@ public void validatorWithTransitionForBlockTimeWaitsBlockInterval() {
     result = scheduler.getNextTimestamp(parentHeader);
     assertThat(result.getTimestampForHeader()).isEqualTo(currentSecondsSinceEpoch + 1);
     assertThat(result.getMillisecondsUntilValid()).isGreaterThan(1000);
+    assertThat(miningParameters.getBlockTxsSelectionMaxTime())
+        .isEqualTo(1 * 1000 * BLOCK_TXS_SELECTION_MAX_PERCENTAGE.getValue() / 100);
   }
 
   @Test
@@ -145,7 +167,8 @@ public void outOfTurnValidatorWaitsLongerThanBlockInterval() {
     final long currentSecondsSinceEpoch = 10L;
     when(clock.millis()).thenReturn(currentSecondsSinceEpoch * 1000);
     final CliqueBlockScheduler scheduler =
-        new CliqueBlockScheduler(clock, validatorProvider, localAddr, forksSchedule);
+        new CliqueBlockScheduler(
+            clock, validatorProvider, localAddr, forksSchedule, miningParameters);
 
     // There are 2 validators, therefore block 3 will put localAddr as the out-turn voter, therefore
     // parent block should be number 2.
@@ -158,6 +181,9 @@ public void outOfTurnValidatorWaitsLongerThanBlockInterval() {
     assertThat(result.getTimestampForHeader())
         .isEqualTo(currentSecondsSinceEpoch + secondsBetweenBlocks);
     assertThat(result.getMillisecondsUntilValid()).isGreaterThan(secondsBetweenBlocks * 1000);
+    assertThat(miningParameters.getBlockTxsSelectionMaxTime())
+        .isEqualTo(
+            secondsBetweenBlocks * 1000 * BLOCK_TXS_SELECTION_MAX_PERCENTAGE.getValue() / 100);
   }
 
   @Test
@@ -167,7 +193,8 @@ public void inTurnValidatorCreatesBlockNowIFParentTimestampSufficientlyBehindNow
     final long secondsBetweenBlocks = 5L;
     when(clock.millis()).thenReturn(currentSecondsSinceEpoch * 1000);
     final CliqueBlockScheduler scheduler =
-        new CliqueBlockScheduler(clock, validatorProvider, localAddr, forksSchedule);
+        new CliqueBlockScheduler(
+            clock, validatorProvider, localAddr, forksSchedule, miningParameters);
 
     // There are 2 validators, therefore block 2 will put localAddr as the in-turn voter, therefore
     // parent block should be number 1.
@@ -181,5 +208,8 @@ public void inTurnValidatorCreatesBlockNowIFParentTimestampSufficientlyBehindNow
 
     assertThat(result.getTimestampForHeader()).isEqualTo(currentSecondsSinceEpoch);
     assertThat(result.getMillisecondsUntilValid()).isEqualTo(0);
+    assertThat(miningParameters.getBlockTxsSelectionMaxTime())
+        .isEqualTo(
+            secondsBetweenBlocks * 1000 * BLOCK_TXS_SELECTION_MAX_PERCENTAGE.getValue() / 100);
   }
 }
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
@@ -74,6 +74,7 @@ public BftBlockCreator(
         parentHeader,
         Optional.empty(),
         ethScheduler);
+    setBlockPeriodSeconds(miningParameters, forksSchedule, parentHeader);
     this.bftExtraDataCodec = bftExtraDataCodec;
   }
 
@@ -83,6 +84,14 @@ private static MiningBeneficiaryCalculator miningBeneficiaryCalculator(
         forksSchedule.getFork(blockNum).getValue().getMiningBeneficiary().orElse(localAddress);
   }
 
+  private void setBlockPeriodSeconds(
+      final MiningParameters miningParameters,
+      final ForksSchedule<? extends BftConfigOptions> forksSchedule,
+      final BlockHeader parentHeader) {
+    miningParameters.setBlockPeriodSeconds(
+        forksSchedule.getFork(parentHeader.getNumber() + 1).getValue().getBlockPeriodSeconds());
+  }
+
   @Override
   protected BlockHeader createFinalBlockHeader(final SealableBlockHeader sealableBlockHeader) {
     final BlockHeaderBuilder builder =