From 97edc67c3058b0a82f2f1bc69dbc87f93b2557cc Mon Sep 17 00:00:00 2001 From: Fabio Di Fabio Date: Tue, 14 Feb 2023 12:26:48 +0100 Subject: [PATCH] =?UTF-8?q?If=20a=20PoS=20block=20creation=20repetition=20?= =?UTF-8?q?takes=20less=20than=20a=20configurable=20dur=E2=80=A6=20(#5048)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * If a PoS block creation repetition takes less than a configurable duration, then waits before next repetition Signed-off-by: Fabio Di Fabio * Update CHANGELOG Signed-off-by: Fabio Di Fabio * Add unit test Signed-off-by: Fabio Di Fabio * Update besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java Co-authored-by: Sally MacFarlane Signed-off-by: Fabio Di Fabio * Update besu/src/main/java/org/hyperledger/besu/cli/options/unstable/MiningOptions.java Co-authored-by: Sally MacFarlane Signed-off-by: Fabio Di Fabio --------- Signed-off-by: Fabio Di Fabio Co-authored-by: Sally MacFarlane --- CHANGELOG.md | 1 + .../org/hyperledger/besu/cli/BesuCommand.java | 9 ++++ .../cli/options/unstable/MiningOptions.java | 19 +++++++ .../merge/blockcreation/MergeCoordinator.java | 14 ++++- .../blockcreation/MergeCoordinatorTest.java | 54 ++++++++++++++++++- .../besu/ethereum/core/MiningParameters.java | 34 ++++++++++-- 6 files changed, 124 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fcb24ce519..872d24eceff 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java index d5501b0b1a7..58cd31ffc4b 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java @@ -1891,6 +1891,13 @@ private void validateMiningParams() { 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 BesuControllerBuilder getControllerBuilder() { .powJobTimeToLive(unstableMiningOptions.getPowJobTimeToLive()) .maxOmmerDepth(unstableMiningOptions.getMaxOmmersDepth()) .posBlockCreationMaxTime(unstableMiningOptions.getPosBlockCreationMaxTime()) + .posBlockCreationRepetitionMinDuration( + unstableMiningOptions.getPosBlockCreationRepetitionMinDuration()) .build()) .transactionPoolConfiguration(buildTransactionPoolConfiguration()) .nodeKey(new NodeKey(securityModule())) diff --git a/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/MiningOptions.java b/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/MiningOptions.java index 926174de166..232554722e2 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/MiningOptions.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/options/unstable/MiningOptions.java @@ -16,6 +16,7 @@ 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 int getMaxOmmersDepth() { 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; + } } diff --git a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java index ac1e25b0254..56030131ea1 100644 --- a/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java +++ b/consensus/merge/src/main/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinator.java @@ -344,10 +344,20 @@ private void tryToBuildBetterBlock( private Void retryBlockCreationUntilUseful( final PayloadIdentifier payloadIdentifier, final Supplier 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 {}", diff --git a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java index 4f7a5a16e62..12d8afd673e 100644 --- a/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java +++ b/consensus/merge/src/test/java/org/hyperledger/besu/consensus/merge/blockcreation/MergeCoordinatorTest.java @@ -75,6 +75,7 @@ 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> 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 void shouldContinueBuildingBlocksUntilFinalizeIsCalled() } } + @Test + public void blockCreationRepetitionShouldTakeNotLessThanRepetitionMinDuration() + throws InterruptedException, ExecutionException { + final AtomicLong retries = new AtomicLong(0); + final AtomicLong lastPutAt = new AtomicLong(); + final List 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 { diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/MiningParameters.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/MiningParameters.java index e5dba01bf59..e90efaba5b9 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/core/MiningParameters.java +++ b/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
coinbase; private final Optional 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 @@ private 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 @@ private MiningParameters( this.powJobTimeToLive = powJobTimeToLive; this.maxOmmerDepth = maxOmmerDepth; this.posBlockCreationMaxTime = posBlockCreationMaxTime; + this.posBlockCreationRepetitionMinDuration = posBlockCreationRepetitionMinDuration; } public Optional
getCoinbase() { @@ -152,6 +158,10 @@ public long getPosBlockCreationMaxTime() { return posBlockCreationMaxTime; } + public long getPosBlockCreationRepetitionMinDuration() { + return posBlockCreationRepetitionMinDuration; + } + @Override public boolean equals(final Object o) { if (this == o) return true; @@ -170,7 +180,8 @@ public boolean equals(final Object o) { && remoteSealersTimeToLive == that.remoteSealersTimeToLive && remoteSealersLimit == that.remoteSealersLimit && powJobTimeToLive == that.powJobTimeToLive - && posBlockCreationMaxTime == that.posBlockCreationMaxTime; + && posBlockCreationMaxTime == that.posBlockCreationMaxTime + && posBlockCreationRepetitionMinDuration == that.posBlockCreationRepetitionMinDuration; } @Override @@ -189,7 +200,8 @@ public int hashCode() { remoteSealersLimit, remoteSealersTimeToLive, powJobTimeToLive, - posBlockCreationMaxTime); + posBlockCreationMaxTime, + posBlockCreationRepetitionMinDuration); } @Override @@ -227,6 +239,8 @@ public String toString() { + powJobTimeToLive + ", posBlockCreationMaxTime=" + posBlockCreationMaxTime + + ", posBlockCreationRepetitionMinDuration=" + + posBlockCreationRepetitionMinDuration + '}'; } @@ -249,6 +263,9 @@ public static class Builder { 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 Builder(final MiningParameters existing) { 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 Builder posBlockCreationMaxTime(final long posBlockCreationMaxTime) { 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 MiningParameters build() { remoteSealersTimeToLive, powJobTimeToLive, maxOmmerDepth, - posBlockCreationMaxTime); + posBlockCreationMaxTime, + posBlockCreationRepetitionMinDuration); } } }