From 940243ad01ec8a6a5060f7500cd96a53f8f4c75f Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Thu, 18 Apr 2024 10:42:26 +0100 Subject: [PATCH 01/27] Implementing support for emptyBlockPeriodSeconds in QBFT (Issue #3810) Signed-off-by: Antonio Mota --- .../controller/QbftBesuControllerBuilder.java | 3 +- .../besu/cli/CommandTestAbstract.java | 5 +- .../besu/config/BftConfigOptions.java | 7 +++ .../org/hyperledger/besu/config/BftFork.java | 13 ++++ .../besu/config/JsonBftConfigOptions.java | 11 ++++ .../besu/config/JsonBftConfigOptionsTest.java | 28 +++++++++ .../besu/consensus/common/bft/BlockTimer.java | 61 +++++++++++++++++-- .../common/bft/MutableBftConfigOptions.java | 25 ++++++++ .../common/ForksScheduleFactoryTest.java | 16 ++--- .../consensus/common/bft/BlockTimerTest.java | 44 ++++++++++--- ...ftBlockHeaderValidationRulesetFactory.java | 4 +- .../qbft/QbftForksSchedulesFactory.java | 1 + .../statemachine/QbftBlockHeightManager.java | 50 ++++++++++++--- .../qbft/statemachine/QbftController.java | 2 +- .../qbft/statemachine/QbftRound.java | 24 ++++++++ .../qbft/MutableQbftConfigOptionsTest.java | 20 ++++++ .../QbftBlockHeightManagerTest.java | 21 +++++++ .../QbftForksSchedulesFactoryTest.java | 3 + 18 files changed, 303 insertions(+), 35 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java index 747949aac12..57a4ee16426 100644 --- a/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java +++ b/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(), diff --git a/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java b/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java index deef4f648ca..f8e39c80831 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java +++ b/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( diff --git a/config/src/main/java/org/hyperledger/besu/config/BftConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/BftConfigOptions.java index c94752b598d..a30c54cab1e 100644 --- a/config/src/main/java/org/hyperledger/besu/config/BftConfigOptions.java +++ b/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. * diff --git a/config/src/main/java/org/hyperledger/besu/config/BftFork.java b/config/src/main/java/org/hyperledger/besu/config/BftFork.java index 30f8e1c5d5b..95ef7fd2400 100644 --- a/config/src/main/java/org/hyperledger/besu/config/BftFork.java +++ b/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. * diff --git a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java index a3791a6e584..5e688bd5b00 100644 --- a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java +++ b/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 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()); } diff --git a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java b/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java index 5f530cebb5b..1150aea3058 100644 --- a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java +++ b/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,18 +63,37 @@ 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)); @@ -80,6 +101,13 @@ public void shouldThrowOnNonPositiveBlockPeriod() { .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)); diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java index 5649b69e8f1..5f2b609d167 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java +++ b/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 forksSchedule; private final BftExecutors bftExecutors; private Optional> 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; + } } diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java index ee8f546bd77..205a4231afb 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java +++ b/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. * diff --git a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java index 0d10c67f21b..fb50c2684b1 100644 --- a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java +++ b/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 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 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))); } } diff --git a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java index 23026148114..7f474cf2854 100644 --- a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java +++ b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java @@ -63,6 +63,13 @@ public void initialise() { @Test public void cancelTimerCancelsWhenNoTimer() { + final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 15; + final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; + + when(mockForksSchedule.getFork(anyLong())) + .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); + + final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); // Starts with nothing running assertThat(timer.isRunning()).isFalse(); @@ -75,12 +82,13 @@ public void cancelTimerCancelsWhenNoTimer() { @Test public void startTimerSchedulesCorrectlyWhenExpiryIsInTheFuture() { final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 15; + final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; final long NOW_MILLIS = 505_000L; final long BLOCK_TIME_STAMP = 500L; final long EXPECTED_DELAY = 10_000L; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS))); + .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -104,12 +112,13 @@ public void startTimerSchedulesCorrectlyWhenExpiryIsInTheFuture() { @Test public void aBlockTimerExpiryEventIsAddedToTheQueueOnExpiry() throws InterruptedException { final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 1; + final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 10; final long NOW_MILLIS = 300_500L; final long BLOCK_TIME_STAMP = 300; final long EXPECTED_DELAY = 500; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS))); + .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); when(mockClock.millis()).thenReturn(NOW_MILLIS); final BlockHeader header = @@ -149,11 +158,12 @@ public void aBlockTimerExpiryEventIsAddedToTheQueueOnExpiry() throws Interrupted @Test public void eventIsImmediatelyAddedToTheQueueIfAbsoluteExpiryIsEqualToNow() { final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 15; + final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; final long NOW_MILLIS = 515_000L; final long BLOCK_TIME_STAMP = 500; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS))); + .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -179,11 +189,12 @@ public void eventIsImmediatelyAddedToTheQueueIfAbsoluteExpiryIsEqualToNow() { @Test public void eventIsImmediatelyAddedToTheQueueIfAbsoluteExpiryIsInThePast() { final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 15; + final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; final long NOW_MILLIS = 520_000L; final long BLOCK_TIME_STAMP = 500L; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS))); + .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -209,11 +220,12 @@ public void eventIsImmediatelyAddedToTheQueueIfAbsoluteExpiryIsInThePast() { @Test public void startTimerCancelsExistingTimer() { final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 15; + final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; final long NOW_MILLIS = 500_000L; final long BLOCK_TIME_STAMP = 500L; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS))); + .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -237,11 +249,12 @@ public void startTimerCancelsExistingTimer() { @Test public void runningFollowsTheStateOfTheTimer() { final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 15; + final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; final long NOW_MILLIS = 500_000L; final long BLOCK_TIME_STAMP = 500L; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS))); + .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -263,10 +276,25 @@ public void runningFollowsTheStateOfTheTimer() { assertThat(timer.isRunning()).isFalse(); } - private BftConfigOptions createBftFork(final int blockPeriodSeconds) { + @Test + public void checkBlockTimerEmptyAndNonEmptyPeriodSecods() { + final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 15; + final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; + + when(mockForksSchedule.getFork(anyLong())) + .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); + + final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); + + assertThat(timer.getBlockPeriodSeconds()).isEqualTo(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS); + assertThat(timer.getEmptyBlockPeriodSeconds()).isEqualTo(MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS); + } + + private BftConfigOptions createBftFork(final int blockPeriodSeconds, final int emptyBlockPeriodSeconds) { final MutableBftConfigOptions bftConfigOptions = - new MutableBftConfigOptions(JsonBftConfigOptions.DEFAULT); + new MutableBftConfigOptions(JsonBftConfigOptions.DEFAULT); bftConfigOptions.setBlockPeriodSeconds(blockPeriodSeconds); + bftConfigOptions.setEmptyBlockPeriodSeconds(emptyBlockPeriodSeconds); return bftConfigOptions; } } diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftBlockHeaderValidationRulesetFactory.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftBlockHeaderValidationRulesetFactory.java index 028f939d32c..74be52342dc 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftBlockHeaderValidationRulesetFactory.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftBlockHeaderValidationRulesetFactory.java @@ -29,7 +29,6 @@ import org.hyperledger.besu.ethereum.mainnet.headervalidationrules.GasLimitRangeAndDeltaValidationRule; import org.hyperledger.besu.ethereum.mainnet.headervalidationrules.GasUsageValidationRule; import org.hyperledger.besu.ethereum.mainnet.headervalidationrules.TimestampBoundedByFutureParameter; -import org.hyperledger.besu.ethereum.mainnet.headervalidationrules.TimestampMoreRecentThanParent; import java.util.Optional; @@ -58,7 +57,8 @@ public static BlockHeaderValidator.Builder blockHeaderValidator( new GasLimitRangeAndDeltaValidationRule( DEFAULT_MIN_GAS_LIMIT, DEFAULT_MAX_GAS_LIMIT, baseFeeMarket)) .addRule(new TimestampBoundedByFutureParameter(1)) - .addRule(new TimestampMoreRecentThanParent(secondsBetweenBlocks)) + //.addRule(new TimestampMoreRecentThanParent(secondsBetweenBlocks)) + // removed because of conflict with emptyblockperiodseconds .addRule( new ConstantFieldValidationRule<>( "MixHash", BlockHeader::getMixHash, BftHelpers.EXPECTED_MIX_HASH)) diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftForksSchedulesFactory.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftForksSchedulesFactory.java index d66897e405d..2b52438dbdb 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftForksSchedulesFactory.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftForksSchedulesFactory.java @@ -47,6 +47,7 @@ private static QbftConfigOptions createQbftConfigOptions( new MutableQbftConfigOptions(lastSpec.getValue()); fork.getBlockPeriodSeconds().ifPresent(bftConfigOptions::setBlockPeriodSeconds); + fork.getEmptyBlockPeriodSeconds().ifPresent(bftConfigOptions::setEmptyBlockPeriodSeconds); fork.getBlockRewardWei().ifPresent(bftConfigOptions::setBlockRewardWei); if (fork.isMiningBeneficiaryConfigured()) { diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java index 0940d35df6b..d8ecff79a74 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java @@ -28,6 +28,7 @@ import org.hyperledger.besu.consensus.qbft.validation.FutureRoundProposalMessageValidator; import org.hyperledger.besu.consensus.qbft.validation.MessageValidatorFactory; import org.hyperledger.besu.datatypes.Address; +import org.hyperledger.besu.ethereum.core.Block; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.plugin.services.securitymodule.SecurityModuleException; @@ -130,19 +131,48 @@ public void handleBlockTimerExpiry(final ConsensusRoundIdentifier roundIdentifie logValidatorChanges(qbftRound); + if (roundIdentifier.equals(qbftRound.getRoundIdentifier())) { + buildBlockAndMaybePropose(roundIdentifier, qbftRound); + } else { + LOG.trace( + "Block timer expired for a round ({}) other than current ({})", + roundIdentifier, + qbftRound.getRoundIdentifier()); + } + } + + private void buildBlockAndMaybePropose( + final ConsensusRoundIdentifier roundIdentifier, final QbftRound qbftRound) { + // mining will be checked against round 0 as the current round is initialised to 0 above final boolean isProposer = - finalState.isLocalNodeProposerForRound(qbftRound.getRoundIdentifier()); - - if (isProposer) { - if (roundIdentifier.equals(qbftRound.getRoundIdentifier())) { - final long headerTimeStampSeconds = Math.round(clock.millis() / 1000D); - qbftRound.createAndSendProposalMessage(headerTimeStampSeconds); + finalState.isLocalNodeProposerForRound(qbftRound.getRoundIdentifier()); + + final long headerTimeStampSeconds = Math.round(clock.millis() / 1000D); + final Block block = qbftRound.createBlock(headerTimeStampSeconds); + final boolean blockHasTransactions = !block.getBody().getTransactions().isEmpty(); + if (blockHasTransactions) { + if (isProposer) { + LOG.debug("Block has transactions and this node is a proposer so it will send a proposal"); + qbftRound.sendProposalMessage(block); + }else{ + LOG.debug("Block has transactions but this node is not a proposer so it will not send a proposal"); + } + } else { + final long emptyBlockPeriodSeconds = finalState.getBlockTimer().getEmptyBlockPeriodSeconds(); + final long emptyBlockPeriodExpiryTime = parentHeader.getTimestamp() + emptyBlockPeriodSeconds; + final long nowInSeconds = finalState.getClock().millis() / 1000; + if (nowInSeconds >= emptyBlockPeriodExpiryTime) { + if (isProposer) { + LOG.debug("Block has no transactions and this node is a proposer so it will send a proposal"); + qbftRound.sendProposalMessage(block); + } else { + LOG.debug("Block has no transactions but this node is not a proposer so it will not send a proposal"); + } } else { - LOG.trace( - "Block timer expired for a round ({}) other than current ({})", - roundIdentifier, - qbftRound.getRoundIdentifier()); + finalState.getRoundTimer().cancelTimer(); + finalState.getBlockTimer().startEmptyBlockTimer(roundIdentifier, parentHeader); + currentRound = Optional.empty(); } } } diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftController.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftController.java index bd2751ff321..c639a0f2057 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftController.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftController.java @@ -48,7 +48,7 @@ public class QbftController extends BaseBftController { * @param gossiper the gossiper * @param duplicateMessageTracker the duplicate message tracker * @param futureMessageBuffer the future message buffer - * @param sychronizerUpdater the synchronizer updater + * @param sychronizerUpdater the sychronizer updater * @param bftExtraDataCodec the bft extra data codec */ public QbftController( diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java index 47db570acc0..c86078773b7 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java @@ -127,6 +127,30 @@ public ConsensusRoundIdentifier getRoundIdentifier() { return roundState.getRoundIdentifier(); } + /** + * Create a block + * + * @return a Block + */ + public Block createBlock(final long headerTimeStampSeconds) { + LOG.debug( + "Creating proposed block. round={}", + roundState.getRoundIdentifier()); + return blockCreator.createBlock(headerTimeStampSeconds).getBlock(); + } + + /** + * Send proposal message. + * + * @param block to send + */ + public void sendProposalMessage(final Block block) { + LOG.debug( + "Creating proposed block blockHeader={}", + block.getHeader()); + updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); + } + /** * Create and send proposal message. * diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/MutableQbftConfigOptionsTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/MutableQbftConfigOptionsTest.java index d41996d11db..4871648be94 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/MutableQbftConfigOptionsTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/MutableQbftConfigOptionsTest.java @@ -41,4 +41,24 @@ public void getValidatorContractAddressNormalization() { assertThat(mutableQbftConfigOptions.getValidatorContractAddress()).hasValue("0xabc"); } + + @Test + public void checkBlockPeriodSeconds() { + when(qbftConfigOptions.getBlockPeriodSeconds()).thenReturn(2); + + final MutableQbftConfigOptions mutableQbftConfigOptions = + new MutableQbftConfigOptions(qbftConfigOptions); + + assertThat(mutableQbftConfigOptions.getBlockPeriodSeconds()).isEqualTo(2); + } + + @Test + public void checkEmptyBlockPeriodSeconds() { + when(qbftConfigOptions.getEmptyBlockPeriodSeconds()).thenReturn(60); + + final MutableQbftConfigOptions mutableQbftConfigOptions = + new MutableQbftConfigOptions(qbftConfigOptions); + + assertThat(mutableQbftConfigOptions.getEmptyBlockPeriodSeconds()).isEqualTo(60); + } } diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java index a0479860845..6753ed9617b 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java @@ -157,6 +157,7 @@ public void setup() { when(finalState.getBlockTimer()).thenReturn(blockTimer); when(finalState.getQuorum()).thenReturn(3); when(finalState.getValidatorMulticaster()).thenReturn(validatorMulticaster); + when(finalState. getClock()).thenReturn(clock); when(blockCreator.createBlock(anyLong())) .thenReturn( new BlockCreationResult( @@ -571,4 +572,24 @@ public void illegalFutureRoundProposalDoesNotTriggerNewRound() { manager.handleProposalPayload(futureRoundProposal); verify(roundFactory, never()).createNewRound(any(), anyInt()); } + + @Test + public void checkOnlyEmptyBlockPeriodSecondsIsInvokedForBlocksWithNoTransactions() { + when(finalState.isLocalNodeProposerForRound(roundIdentifier)).thenReturn(true); + + final QbftBlockHeightManager manager = + new QbftBlockHeightManager( + headerTestFixture.buildHeader(), + finalState, + roundChangeManager, + roundFactory, + clock, + messageValidatorFactory, + messageFactory); + + manager.handleBlockTimerExpiry(roundIdentifier); + + verify(blockTimer, atLeastOnce()).getEmptyBlockPeriodSeconds(); + verify(blockTimer, times(0)).getBlockPeriodSeconds(); + } } diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/validator/QbftForksSchedulesFactoryTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/validator/QbftForksSchedulesFactoryTest.java index abb75426d0a..7be9e96f488 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/validator/QbftForksSchedulesFactoryTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/validator/QbftForksSchedulesFactoryTest.java @@ -63,6 +63,8 @@ public void createsScheduleWithForkThatOverridesGenesisValues() { List.of("1", "2", "3"), BftFork.BLOCK_PERIOD_SECONDS_KEY, 10, + BftFork.EMPTY_BLOCK_PERIOD_SECONDS_KEY, + 60, BftFork.BLOCK_REWARD_KEY, "5", QbftFork.VALIDATOR_SELECTION_MODE_KEY, @@ -78,6 +80,7 @@ public void createsScheduleWithForkThatOverridesGenesisValues() { final Map forkOptions = new HashMap<>(configOptions.asMap()); forkOptions.put(BftFork.BLOCK_PERIOD_SECONDS_KEY, 10); + forkOptions.put(BftFork.EMPTY_BLOCK_PERIOD_SECONDS_KEY, 60); forkOptions.put(BftFork.BLOCK_REWARD_KEY, "5"); forkOptions.put(QbftFork.VALIDATOR_SELECTION_MODE_KEY, "5"); forkOptions.put(QbftFork.VALIDATOR_CONTRACT_ADDRESS_KEY, "10"); From 3af8a6ae7a523454b5d05c7b685b571f6263519e Mon Sep 17 00:00:00 2001 From: amsmota Date: Tue, 23 Apr 2024 21:22:25 +0100 Subject: [PATCH 02/27] Fixing integration tests Signed-off-by: amsmota --- .../org/hyperledger/besu/config/JsonBftConfigOptions.java | 2 +- .../besu/consensus/common/ForksScheduleFactoryTest.java | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java index 5e688bd5b00..14718db2fbf 100644 --- a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java @@ -34,8 +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_EMPTY_BLOCK_PERIOD_SECONDS = 0; 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 diff --git a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java index fb50c2684b1..72b556de764 100644 --- a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java +++ b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java @@ -66,12 +66,12 @@ public void throwsErrorIfHasForksWithDuplicateBlock() { public void createsScheduleUsingSpecCreator() { final BftConfigOptions genesisConfigOptions = JsonBftConfigOptions.DEFAULT; final ForkSpec genesisForkSpec = new ForkSpec<>(0, genesisConfigOptions); - final BftFork fork1 = createFork(1, 10); - final BftFork fork2 = createFork(2, 20); + final BftFork fork1 = createFork(1, 10, 20); + final BftFork fork2 = createFork(2, 20, 40); final SpecCreator specCreator = Mockito.mock(SpecCreator.class); - final BftConfigOptions configOptions1 = createBftConfigOptions(10); - final BftConfigOptions configOptions2 = createBftConfigOptions(20); + final BftConfigOptions configOptions1 = createBftConfigOptions(10, 30); + final BftConfigOptions configOptions2 = createBftConfigOptions(20, 60); when(specCreator.create(genesisForkSpec, fork1)).thenReturn(configOptions1); when(specCreator.create(new ForkSpec<>(1, configOptions1), fork2)).thenReturn(configOptions2); From 12090c1e4adbd0911d4ac595fdf0daf3f99187e4 Mon Sep 17 00:00:00 2001 From: amsmota Date: Wed, 24 Apr 2024 13:46:52 +0100 Subject: [PATCH 03/27] Reinstated TimestampMoreRecentThanParent header rule Signed-off-by: amsmota --- .../qbft/QbftBlockHeaderValidationRulesetFactory.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftBlockHeaderValidationRulesetFactory.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftBlockHeaderValidationRulesetFactory.java index 74be52342dc..028f939d32c 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftBlockHeaderValidationRulesetFactory.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/QbftBlockHeaderValidationRulesetFactory.java @@ -29,6 +29,7 @@ import org.hyperledger.besu.ethereum.mainnet.headervalidationrules.GasLimitRangeAndDeltaValidationRule; import org.hyperledger.besu.ethereum.mainnet.headervalidationrules.GasUsageValidationRule; import org.hyperledger.besu.ethereum.mainnet.headervalidationrules.TimestampBoundedByFutureParameter; +import org.hyperledger.besu.ethereum.mainnet.headervalidationrules.TimestampMoreRecentThanParent; import java.util.Optional; @@ -57,8 +58,7 @@ public static BlockHeaderValidator.Builder blockHeaderValidator( new GasLimitRangeAndDeltaValidationRule( DEFAULT_MIN_GAS_LIMIT, DEFAULT_MAX_GAS_LIMIT, baseFeeMarket)) .addRule(new TimestampBoundedByFutureParameter(1)) - //.addRule(new TimestampMoreRecentThanParent(secondsBetweenBlocks)) - // removed because of conflict with emptyblockperiodseconds + .addRule(new TimestampMoreRecentThanParent(secondsBetweenBlocks)) .addRule( new ConstantFieldValidationRule<>( "MixHash", BlockHeader::getMixHash, BftHelpers.EXPECTED_MIX_HASH)) From 2ed8d677c0a20e746024ecbaa14df89a713132ca Mon Sep 17 00:00:00 2001 From: amsmota Date: Wed, 24 Apr 2024 18:39:44 +0100 Subject: [PATCH 04/27] Refactored some QbftRound methods Signed-off-by: amsmota --- .../besu/consensus/qbft/statemachine/QbftRound.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java index c86078773b7..9f5b08bf472 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java @@ -158,10 +158,9 @@ public void sendProposalMessage(final Block block) { */ public void createAndSendProposalMessage(final long headerTimeStampSeconds) { LOG.debug("Creating proposed block. round={}", roundState.getRoundIdentifier()); - final Block block = blockCreator.createBlock(headerTimeStampSeconds).getBlock(); + final Block block = createBlock(headerTimeStampSeconds); - LOG.trace("Creating proposed block blockHeader={}", block.getHeader()); - updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); + sendProposalMessage(block); } /** @@ -178,7 +177,7 @@ public void startRoundWith( final Block blockToPublish; if (bestPreparedCertificate.isEmpty()) { LOG.debug("Sending proposal with new block. round={}", roundState.getRoundIdentifier()); - blockToPublish = blockCreator.createBlock(headerTimestamp).getBlock(); + blockToPublish = createBlock(headerTimestamp); } else { LOG.debug( "Sending proposal from PreparedCertificate. round={}", roundState.getRoundIdentifier()); From b38e417e330e42ff2d5d82fd05463fdb768ee1b7 Mon Sep 17 00:00:00 2001 From: amsmota Date: Wed, 24 Apr 2024 18:46:34 +0100 Subject: [PATCH 05/27] Refactored some QbftRound methods Signed-off-by: amsmota --- .../besu/consensus/qbft/statemachine/QbftRound.java | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java index 9f5b08bf472..789c95b0b1b 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java @@ -148,7 +148,7 @@ public void sendProposalMessage(final Block block) { LOG.debug( "Creating proposed block blockHeader={}", block.getHeader()); - updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); + updateStateWithProposalAndTransmit(block); } /** @@ -190,6 +190,17 @@ public void startRoundWith( bestPreparedCertificate.map(PreparedCertificate::getPrepares).orElse(emptyList())); } + /** + * Update state with proposal and transmit. + * + * @param block the block + */ + protected void updateStateWithProposalAndTransmit( + final Block block) { + + updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); + } + /** * Update state with proposal and transmit. * From 2a7a61ac04d06785e63fffa2e8ecff0c327d3b81 Mon Sep 17 00:00:00 2001 From: amsmota Date: Wed, 24 Apr 2024 18:52:21 +0100 Subject: [PATCH 06/27] Removed log comments for all block period seconds Signed-off-by: amsmota --- .../besu/consensus/common/bft/MutableBftConfigOptions.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java index 205a4231afb..f802e97bc24 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java @@ -72,13 +72,11 @@ 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; } @@ -142,7 +140,6 @@ 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; } @@ -152,7 +149,6 @@ public void setBlockPeriodSeconds(final int blockPeriodSeconds) { * @param emptyBlockPeriodSeconds the empty block period seconds */ public void setEmptyBlockPeriodSeconds(final int emptyBlockPeriodSeconds) { - LOG.info("SET EMPTYBLOCKPERIODSECONDS: " + emptyBlockPeriodSeconds); this.emptyBlockPeriodSeconds = emptyBlockPeriodSeconds; } From 59a5bd159dd979d8707cac5aaaa3fb3a6aeda9b7 Mon Sep 17 00:00:00 2001 From: amsmota Date: Wed, 24 Apr 2024 19:47:24 +0100 Subject: [PATCH 07/27] Removed log comments for all block period seconds Signed-off-by: amsmota --- .../besu/consensus/common/bft/MutableBftConfigOptions.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java index f802e97bc24..6f26781fd7e 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java @@ -32,8 +32,7 @@ * ForksSchedule}*. */ public class MutableBftConfigOptions implements BftConfigOptions { - - private static final Logger LOG = LoggerFactory.getLogger(MutableBftConfigOptions.class); + private long epochLength; private int blockPeriodSeconds; private int emptyBlockPeriodSeconds; From fc515973e0bddf92063bc924dd49f437ec2d9049 Mon Sep 17 00:00:00 2001 From: amsmota Date: Mon, 29 Apr 2024 17:01:08 +0100 Subject: [PATCH 08/27] Run spotlessApply Signed-off-by: amsmota --- .../besu/config/JsonBftConfigOptions.java | 2 +- .../besu/config/JsonBftConfigOptionsTest.java | 2 +- .../besu/consensus/common/bft/BlockTimer.java | 26 ++++---- .../common/bft/MutableBftConfigOptions.java | 5 +- .../common/ForksScheduleFactoryTest.java | 15 +++-- .../consensus/common/bft/BlockTimerTest.java | 65 +++++++++++++++---- .../statemachine/QbftBlockHeightManager.java | 21 +++--- .../qbft/statemachine/QbftRound.java | 11 +--- .../qbft/MutableQbftConfigOptionsTest.java | 4 +- .../QbftBlockHeightManagerTest.java | 18 ++--- .../eth/sync/tasks/PersistBlockTask.java | 3 + 11 files changed, 110 insertions(+), 62 deletions(-) diff --git a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java index 14718db2fbf..c5532a6a964 100644 --- a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java @@ -71,7 +71,7 @@ public int getBlockPeriodSeconds() { @Override public int getEmptyBlockPeriodSeconds() { return JsonUtil.getInt( - bftConfigRoot, "emptyblockperiodseconds", DEFAULT_EMPTY_BLOCK_PERIOD_SECONDS); + bftConfigRoot, "emptyblockperiodseconds", DEFAULT_EMPTY_BLOCK_PERIOD_SECONDS); } @Override diff --git a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java b/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java index 1150aea3058..ff376ef442d 100644 --- a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java +++ b/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java @@ -91,7 +91,7 @@ public void shouldGetDefaultBlockPeriodFromDefaultConfig() { public void shouldGetDefaultEmptyBlockPeriodFromDefaultConfig() { assertThat(JsonBftConfigOptions.DEFAULT.getEmptyBlockPeriodSeconds()) - .isEqualTo(EXPECTED_EMPTY_DEFAULT_BLOCK_PERIOD); + .isEqualTo(EXPECTED_EMPTY_DEFAULT_BLOCK_PERIOD); } @Test diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java index 5f2b609d167..1624252fb97 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java @@ -97,11 +97,11 @@ public synchronized void startTimer( } public synchronized void startEmptyBlockTimer( - final ConsensusRoundIdentifier round, final BlockHeader chainHeadHeader) { + final ConsensusRoundIdentifier round, final BlockHeader chainHeadHeader) { // absolute time when the timer is supposed to expire final int currentBlockPeriodSeconds = - forksSchedule.getFork(round.getSequenceNumber()).getValue().getEmptyBlockPeriodSeconds(); + forksSchedule.getFork(round.getSequenceNumber()).getValue().getEmptyBlockPeriodSeconds(); final long minimumTimeBetweenBlocksMillis = currentBlockPeriodSeconds * 1000L; final long expiryTime = chainHeadHeader.getTimestamp() * 1_000 + minimumTimeBetweenBlocksMillis; @@ -110,7 +110,8 @@ public synchronized void startEmptyBlockTimer( startTimer(round, expiryTime); } - private synchronized void startTimer(final ConsensusRoundIdentifier round, final long expiryTime) { + private synchronized void startTimer( + final ConsensusRoundIdentifier round, final long expiryTime) { cancelTimer(); final long now = clock.millis(); @@ -127,26 +128,29 @@ private synchronized void startTimer(final ConsensusRoundIdentifier round, final } } - private synchronized void setBlockTimes(final ConsensusRoundIdentifier round, final boolean isEmpty) { - final BftConfigOptions currentConfigOptions = forksSchedule.getFork(round.getSequenceNumber()).getValue(); + 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 + long currentBlockPeriodSeconds = + isEmpty && this.emptyBlockPeriodSeconds > 0 ? this.emptyBlockPeriodSeconds : this.blockPeriodSeconds; LOG.debug( - "NEW CURRENTBLOCKPERIODSECONDS SET TO {}: {}" - , isEmpty?"EMPTYBLOCKPERIODSECONDS":"BLOCKPERIODSECONDS" - , currentBlockPeriodSeconds); + "NEW CURRENTBLOCKPERIODSECONDS SET TO {}: {}", + isEmpty ? "EMPTYBLOCKPERIODSECONDS" : "BLOCKPERIODSECONDS", + currentBlockPeriodSeconds); } - public synchronized long getBlockPeriodSeconds(){ + public synchronized long getBlockPeriodSeconds() { return blockPeriodSeconds; } - public synchronized long getEmptyBlockPeriodSeconds(){ + public synchronized long getEmptyBlockPeriodSeconds() { return emptyBlockPeriodSeconds; } } diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java index 6f26781fd7e..e96091ed68a 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/MutableBftConfigOptions.java @@ -24,15 +24,12 @@ 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 long epochLength; private int blockPeriodSeconds; private int emptyBlockPeriodSeconds; diff --git a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java index 72b556de764..568a8c82ff6 100644 --- a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java +++ b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/ForksScheduleFactoryTest.java @@ -82,7 +82,8 @@ public void createsScheduleUsingSpecCreator() { assertThat(schedule.getFork(2)).isEqualTo(new ForkSpec<>(2, configOptions2)); } - private MutableBftConfigOptions createBftConfigOptions(final int blockPeriodSeconds, final int emptyBlockPeriodSeconds) { + private MutableBftConfigOptions createBftConfigOptions( + final int blockPeriodSeconds, final int emptyBlockPeriodSeconds) { final MutableBftConfigOptions bftConfigOptions = new MutableBftConfigOptions(JsonBftConfigOptions.DEFAULT); bftConfigOptions.setBlockPeriodSeconds(blockPeriodSeconds); @@ -90,12 +91,16 @@ private MutableBftConfigOptions createBftConfigOptions(final int blockPeriodSeco return bftConfigOptions; } - private BftFork createFork(final long block, final long blockPeriodSeconds, final long emptyBlockPeriodSeconds) { + 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.EMPTY_BLOCK_PERIOD_SECONDS_KEY, emptyBlockPeriodSeconds))); + BftFork.FORK_BLOCK_KEY, + block, + BftFork.BLOCK_PERIOD_SECONDS_KEY, + blockPeriodSeconds, + BftFork.EMPTY_BLOCK_PERIOD_SECONDS_KEY, + emptyBlockPeriodSeconds))); } } diff --git a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java index 7f474cf2854..1e03c9cb4c3 100644 --- a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java +++ b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java @@ -67,8 +67,12 @@ public void cancelTimerCancelsWhenNoTimer() { final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); - + .thenReturn( + new ForkSpec<>( + 0, + createBftFork( + MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, + MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); // Starts with nothing running @@ -88,7 +92,12 @@ public void startTimerSchedulesCorrectlyWhenExpiryIsInTheFuture() { final long EXPECTED_DELAY = 10_000L; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); + .thenReturn( + new ForkSpec<>( + 0, + createBftFork( + MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, + MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -118,7 +127,12 @@ public void aBlockTimerExpiryEventIsAddedToTheQueueOnExpiry() throws Interrupted final long EXPECTED_DELAY = 500; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); + .thenReturn( + new ForkSpec<>( + 0, + createBftFork( + MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, + MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); when(mockClock.millis()).thenReturn(NOW_MILLIS); final BlockHeader header = @@ -163,7 +177,12 @@ public void eventIsImmediatelyAddedToTheQueueIfAbsoluteExpiryIsEqualToNow() { final long BLOCK_TIME_STAMP = 500; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); + .thenReturn( + new ForkSpec<>( + 0, + createBftFork( + MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, + MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -194,7 +213,12 @@ public void eventIsImmediatelyAddedToTheQueueIfAbsoluteExpiryIsInThePast() { final long BLOCK_TIME_STAMP = 500L; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); + .thenReturn( + new ForkSpec<>( + 0, + createBftFork( + MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, + MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -225,7 +249,12 @@ public void startTimerCancelsExistingTimer() { final long BLOCK_TIME_STAMP = 500L; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); + .thenReturn( + new ForkSpec<>( + 0, + createBftFork( + MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, + MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -254,7 +283,12 @@ public void runningFollowsTheStateOfTheTimer() { final long BLOCK_TIME_STAMP = 500L; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); + .thenReturn( + new ForkSpec<>( + 0, + createBftFork( + MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, + MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); @@ -282,17 +316,24 @@ public void checkBlockTimerEmptyAndNonEmptyPeriodSecods() { final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; when(mockForksSchedule.getFork(anyLong())) - .thenReturn(new ForkSpec<>(0, createBftFork(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); + .thenReturn( + new ForkSpec<>( + 0, + createBftFork( + MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, + MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); assertThat(timer.getBlockPeriodSeconds()).isEqualTo(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS); - assertThat(timer.getEmptyBlockPeriodSeconds()).isEqualTo(MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS); + assertThat(timer.getEmptyBlockPeriodSeconds()) + .isEqualTo(MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS); } - private BftConfigOptions createBftFork(final int blockPeriodSeconds, final int emptyBlockPeriodSeconds) { + private BftConfigOptions createBftFork( + final int blockPeriodSeconds, final int emptyBlockPeriodSeconds) { final MutableBftConfigOptions bftConfigOptions = - new MutableBftConfigOptions(JsonBftConfigOptions.DEFAULT); + new MutableBftConfigOptions(JsonBftConfigOptions.DEFAULT); bftConfigOptions.setBlockPeriodSeconds(blockPeriodSeconds); bftConfigOptions.setEmptyBlockPeriodSeconds(emptyBlockPeriodSeconds); return bftConfigOptions; diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java index d8ecff79a74..5f99c92e1e6 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java @@ -135,18 +135,18 @@ public void handleBlockTimerExpiry(final ConsensusRoundIdentifier roundIdentifie buildBlockAndMaybePropose(roundIdentifier, qbftRound); } else { LOG.trace( - "Block timer expired for a round ({}) other than current ({})", - roundIdentifier, - qbftRound.getRoundIdentifier()); + "Block timer expired for a round ({}) other than current ({})", + roundIdentifier, + qbftRound.getRoundIdentifier()); } } private void buildBlockAndMaybePropose( - final ConsensusRoundIdentifier roundIdentifier, final QbftRound qbftRound) { + final ConsensusRoundIdentifier roundIdentifier, final QbftRound qbftRound) { // mining will be checked against round 0 as the current round is initialised to 0 above final boolean isProposer = - finalState.isLocalNodeProposerForRound(qbftRound.getRoundIdentifier()); + finalState.isLocalNodeProposerForRound(qbftRound.getRoundIdentifier()); final long headerTimeStampSeconds = Math.round(clock.millis() / 1000D); final Block block = qbftRound.createBlock(headerTimeStampSeconds); @@ -155,8 +155,9 @@ private void buildBlockAndMaybePropose( if (isProposer) { LOG.debug("Block has transactions and this node is a proposer so it will send a proposal"); qbftRound.sendProposalMessage(block); - }else{ - LOG.debug("Block has transactions but this node is not a proposer so it will not send a proposal"); + } else { + LOG.debug( + "Block has transactions but this node is not a proposer so it will not send a proposal"); } } else { final long emptyBlockPeriodSeconds = finalState.getBlockTimer().getEmptyBlockPeriodSeconds(); @@ -164,10 +165,12 @@ private void buildBlockAndMaybePropose( final long nowInSeconds = finalState.getClock().millis() / 1000; if (nowInSeconds >= emptyBlockPeriodExpiryTime) { if (isProposer) { - LOG.debug("Block has no transactions and this node is a proposer so it will send a proposal"); + LOG.debug( + "Block has no transactions and this node is a proposer so it will send a proposal"); qbftRound.sendProposalMessage(block); } else { - LOG.debug("Block has no transactions but this node is not a proposer so it will not send a proposal"); + LOG.debug( + "Block has no transactions but this node is not a proposer so it will not send a proposal"); } } else { finalState.getRoundTimer().cancelTimer(); diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java index 789c95b0b1b..f6e5f143689 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java @@ -133,9 +133,7 @@ public ConsensusRoundIdentifier getRoundIdentifier() { * @return a Block */ public Block createBlock(final long headerTimeStampSeconds) { - LOG.debug( - "Creating proposed block. round={}", - roundState.getRoundIdentifier()); + LOG.debug("Creating proposed block. round={}", roundState.getRoundIdentifier()); return blockCreator.createBlock(headerTimeStampSeconds).getBlock(); } @@ -145,9 +143,7 @@ public Block createBlock(final long headerTimeStampSeconds) { * @param block to send */ public void sendProposalMessage(final Block block) { - LOG.debug( - "Creating proposed block blockHeader={}", - block.getHeader()); + LOG.debug("Creating proposed block blockHeader={}", block.getHeader()); updateStateWithProposalAndTransmit(block); } @@ -195,8 +191,7 @@ public void startRoundWith( * * @param block the block */ - protected void updateStateWithProposalAndTransmit( - final Block block) { + protected void updateStateWithProposalAndTransmit(final Block block) { updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); } diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/MutableQbftConfigOptionsTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/MutableQbftConfigOptionsTest.java index 4871648be94..cf4676dac90 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/MutableQbftConfigOptionsTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/MutableQbftConfigOptionsTest.java @@ -47,7 +47,7 @@ public void checkBlockPeriodSeconds() { when(qbftConfigOptions.getBlockPeriodSeconds()).thenReturn(2); final MutableQbftConfigOptions mutableQbftConfigOptions = - new MutableQbftConfigOptions(qbftConfigOptions); + new MutableQbftConfigOptions(qbftConfigOptions); assertThat(mutableQbftConfigOptions.getBlockPeriodSeconds()).isEqualTo(2); } @@ -57,7 +57,7 @@ public void checkEmptyBlockPeriodSeconds() { when(qbftConfigOptions.getEmptyBlockPeriodSeconds()).thenReturn(60); final MutableQbftConfigOptions mutableQbftConfigOptions = - new MutableQbftConfigOptions(qbftConfigOptions); + new MutableQbftConfigOptions(qbftConfigOptions); assertThat(mutableQbftConfigOptions.getEmptyBlockPeriodSeconds()).isEqualTo(60); } diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java index 6753ed9617b..7973a1d6b82 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java @@ -157,7 +157,7 @@ public void setup() { when(finalState.getBlockTimer()).thenReturn(blockTimer); when(finalState.getQuorum()).thenReturn(3); when(finalState.getValidatorMulticaster()).thenReturn(validatorMulticaster); - when(finalState. getClock()).thenReturn(clock); + when(finalState.getClock()).thenReturn(clock); when(blockCreator.createBlock(anyLong())) .thenReturn( new BlockCreationResult( @@ -578,14 +578,14 @@ public void checkOnlyEmptyBlockPeriodSecondsIsInvokedForBlocksWithNoTransactions when(finalState.isLocalNodeProposerForRound(roundIdentifier)).thenReturn(true); final QbftBlockHeightManager manager = - new QbftBlockHeightManager( - headerTestFixture.buildHeader(), - finalState, - roundChangeManager, - roundFactory, - clock, - messageValidatorFactory, - messageFactory); + new QbftBlockHeightManager( + headerTestFixture.buildHeader(), + finalState, + roundChangeManager, + roundFactory, + clock, + messageValidatorFactory, + messageFactory); manager.handleBlockTimerExpiry(roundIdentifier); diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java index 85a478c9aaf..08fe3582793 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java @@ -231,6 +231,9 @@ protected void cleanup() { case ALREADY_IMPORTED: LOG.info("Block {} is already imported", block.toLogString()); break; + case NOT_IMPORTED: + LOG.info("Block {} was not imported", block.toLogString()); + break; default: break; } From 5ada709d7330d47d3638b0ca57b797c90a47097e Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Mon, 13 May 2024 13:02:44 +0100 Subject: [PATCH 09/27] Corrected the way the BlockTimer handles emptyBlockPeriodSeconds and other stuff Signed-off-by: Antonio Mota --- .../controller/QbftBesuControllerBuilder.java | 25 +++++++----- .../besu/consensus/common/bft/BlockTimer.java | 40 ++++++++----------- .../statemachine/QbftBlockHeightManager.java | 27 +++++++------ .../besu/ethereum/core/MiningParameters.java | 12 +++++- 4 files changed, 55 insertions(+), 49 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java index 57a4ee16426..8d5533c081c 100644 --- a/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java +++ b/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java @@ -278,12 +278,18 @@ protected MiningCoordinator createMiningCoordinator( protocolContext .getBlockchain() .observeBlockAdded( - o -> - miningParameters.setBlockPeriodSeconds( - qbftForksSchedule - .getFork(o.getBlock().getHeader().getNumber() + 1) - .getValue() - .getBlockPeriodSeconds())); + o -> { + miningParameters.setBlockPeriodSeconds( + qbftForksSchedule + .getFork(o.getBlock().getHeader().getNumber() + 1) + .getValue() + .getBlockPeriodSeconds()); + miningParameters.setEmptyBlockPeriodSeconds( + qbftForksSchedule + .getFork(o.getBlock().getHeader().getNumber() + 1) + .getValue() + .getEmptyBlockPeriodSeconds()); + }); if (syncState.isInitialSyncPhaseDone()) { miningCoordinator.enable(); @@ -411,14 +417,11 @@ private static MinedBlockObserver blockLogger( return block -> LOG.info( String.format( - "%s %s #%,d / %d tx / %d pending / %,d (%01.1f%%) gas / (%s)", + "%s %s #%,d / %d tx / %d pending /", block.getHeader().getCoinbase().equals(localAddress) ? "Produced" : "Imported", block.getBody().getTransactions().size() == 0 ? "empty block" : "block", block.getHeader().getNumber(), block.getBody().getTransactions().size(), - transactionPool.count(), - block.getHeader().getGasUsed(), - (block.getHeader().getGasUsed() * 100.0) / block.getHeader().getGasLimit(), - block.getHash().toHexString())); + transactionPool.count())); } } diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java index 1624252fb97..c4fbfc3d0f3 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java @@ -57,8 +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(); + this.blockPeriodSeconds = 0; + this.emptyBlockPeriodSeconds = 0; } /** Cancels the current running round timer if there is one */ @@ -91,23 +91,25 @@ public synchronized void startTimer( final long minimumTimeBetweenBlocksMillis = currentBlockPeriodSeconds * 1000L; final long expiryTime = chainHeadHeader.getTimestamp() * 1_000 + minimumTimeBetweenBlocksMillis; - setBlockTimes(round, false); + setBlockTimes(round); 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); + public synchronized boolean checkEmptyBlockExpired(final BlockHeader chainHeadHeader, final long currentTimeInMillis) { + final long emptyBlockPeriodExpiryTime = (chainHeadHeader.getTimestamp() + emptyBlockPeriodSeconds) * 1000; + if (currentTimeInMillis >= emptyBlockPeriodExpiryTime) { + LOG.info("Empty Block expired"); + return true; + } + LOG.info("Empty Block NOT expired"); + return false; + } - startTimer(round, expiryTime); + public void resetTimerForEmptyBlock(final ConsensusRoundIdentifier roundIdentifier, final long currentTimeInMillis){ + final long newExpiry = currentTimeInMillis + blockPeriodSeconds * 1000; + startTimer(roundIdentifier, newExpiry); } private synchronized void startTimer( @@ -129,21 +131,11 @@ private synchronized void startTimer( } private synchronized void setBlockTimes( - final ConsensusRoundIdentifier round, final boolean isEmpty) { + final ConsensusRoundIdentifier round) { 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() { diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java index 5f99c92e1e6..8069073b5b5 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java @@ -153,28 +153,29 @@ private void buildBlockAndMaybePropose( final boolean blockHasTransactions = !block.getBody().getTransactions().isEmpty(); if (blockHasTransactions) { if (isProposer) { - LOG.debug("Block has transactions and this node is a proposer so it will send a proposal"); + LOG.info("Block has transactions and this node is a proposer so it will send a proposal: " + roundIdentifier); qbftRound.sendProposalMessage(block); } else { - LOG.debug( - "Block has transactions but this node is not a proposer so it will not send a proposal"); + LOG.info( + "Block has transactions but this node is not a proposer so it will not send a proposal: " + roundIdentifier); } - } else { - final long emptyBlockPeriodSeconds = finalState.getBlockTimer().getEmptyBlockPeriodSeconds(); - final long emptyBlockPeriodExpiryTime = parentHeader.getTimestamp() + emptyBlockPeriodSeconds; - final long nowInSeconds = finalState.getClock().millis() / 1000; - if (nowInSeconds >= emptyBlockPeriodExpiryTime) { + } else { // CHECK IF EmptyBlockExpired IS 0 AND SKIP // SHOULD WE FORCE EmptyBlockExpired > BlockExpired + final long currentTimeInMillis = finalState.getClock().millis(); + boolean emptyBlockExpired = finalState.getBlockTimer().checkEmptyBlockExpired(parentHeader, currentTimeInMillis); + if (emptyBlockExpired) { if (isProposer) { - LOG.debug( - "Block has no transactions and this node is a proposer so it will send a proposal"); + LOG.info( + "Block has no transactions and this node is a proposer so it will send a proposal: " + roundIdentifier); qbftRound.sendProposalMessage(block); } else { - LOG.debug( - "Block has no transactions but this node is not a proposer so it will not send a proposal"); + LOG.info( + "Block has no transactions but this node is not a proposer so it will not send a proposal: " + roundIdentifier); } } else { + LOG.info( + "Block has no transactions but emptyBlockPeriodSeconds did not expired yet: " + roundIdentifier); + finalState.getBlockTimer().resetTimerForEmptyBlock(roundIdentifier, currentTimeInMillis); finalState.getRoundTimer().cancelTimer(); - finalState.getBlockTimer().startEmptyBlockTimer(roundIdentifier, parentHeader); currentRound = Optional.empty(); } } 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 febce15fe13..e5dd813b367 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 @@ -131,6 +131,11 @@ public MiningParameters setBlockPeriodSeconds(final int blockPeriodSeconds) { return this; } + public MiningParameters setEmptyBlockPeriodSeconds(final int emptyBlockPeriodSeconds) { + getMutableRuntimeValues().emptyBlockPeriodSeconds = OptionalInt.of(emptyBlockPeriodSeconds); + return this; + } + @Value.Default public boolean isStratumMiningEnabled() { return false; @@ -231,6 +236,8 @@ default double getMinBlockOccupancyRatio() { OptionalInt getBlockPeriodSeconds(); + OptionalInt getEmptyBlockPeriodSeconds(); + Optional
getCoinbase(); OptionalLong getTargetGasLimit(); @@ -248,6 +255,7 @@ static class MutableRuntimeValues { private volatile OptionalLong targetGasLimit; private volatile Optional> nonceGenerator; private volatile OptionalInt blockPeriodSeconds; + private volatile OptionalInt emptyBlockPeriodSeconds; private MutableRuntimeValues(final MutableInitValues initValues) { miningEnabled = initValues.isMiningEnabled(); @@ -259,6 +267,7 @@ private MutableRuntimeValues(final MutableInitValues initValues) { targetGasLimit = initValues.getTargetGasLimit(); nonceGenerator = initValues.nonceGenerator(); blockPeriodSeconds = initValues.getBlockPeriodSeconds(); + emptyBlockPeriodSeconds = initValues.getEmptyBlockPeriodSeconds(); } @Override @@ -274,7 +283,8 @@ public boolean equals(final Object o) { && Objects.equals(minPriorityFeePerGas, that.minPriorityFeePerGas) && Objects.equals(targetGasLimit, that.targetGasLimit) && Objects.equals(nonceGenerator, that.nonceGenerator) - && Objects.equals(blockPeriodSeconds, that.blockPeriodSeconds); + && Objects.equals(blockPeriodSeconds, that.blockPeriodSeconds) + && Objects.equals(emptyBlockPeriodSeconds, that.emptyBlockPeriodSeconds); } @Override From b85476f234bde53e550162debdafb52f47c9e6fc Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Mon, 13 May 2024 13:07:15 +0100 Subject: [PATCH 10/27] Corrected the way the BlockTimer handles emptyBlockPeriodSeconds and other stuff Signed-off-by: Antonio Mota --- .../besu/controller/QbftBesuControllerBuilder.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java index 8d5533c081c..0da97e2821a 100644 --- a/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java +++ b/besu/src/main/java/org/hyperledger/besu/controller/QbftBesuControllerBuilder.java @@ -417,11 +417,14 @@ private static MinedBlockObserver blockLogger( return block -> LOG.info( String.format( - "%s %s #%,d / %d tx / %d pending /", + "%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())); + transactionPool.count(), + block.getHeader().getGasUsed(), + (block.getHeader().getGasUsed() * 100.0) / block.getHeader().getGasLimit(), + block.getHash().toHexString())); } } From 7f6bc5037bb5572faa94f5782a81315f3cd9deee Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Mon, 13 May 2024 15:00:04 +0100 Subject: [PATCH 11/27] Corrected the way the BlockTimer handles emptyBlockPeriodSeconds and other stuff Signed-off-by: Antonio Mota --- .../consensus/qbft/statemachine/QbftBlockHeightManager.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java index 8069073b5b5..abc395c3556 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java @@ -159,7 +159,7 @@ private void buildBlockAndMaybePropose( LOG.info( "Block has transactions but this node is not a proposer so it will not send a proposal: " + roundIdentifier); } - } else { // CHECK IF EmptyBlockExpired IS 0 AND SKIP // SHOULD WE FORCE EmptyBlockExpired > BlockExpired + } else { final long currentTimeInMillis = finalState.getClock().millis(); boolean emptyBlockExpired = finalState.getBlockTimer().checkEmptyBlockExpired(parentHeader, currentTimeInMillis); if (emptyBlockExpired) { From de4e5344d56014d2bcc7032d28e880dcfbb05f00 Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Tue, 14 May 2024 14:29:17 +0100 Subject: [PATCH 12/27] Corrected the way the BlockTimer handles emptyBlockPeriodSeconds and other stuff Signed-off-by: Antonio Mota --- .../besu/consensus/common/bft/BlockTimer.java | 10 ++++++++-- .../besu/ethereum/eth/sync/tasks/PersistBlockTask.java | 3 --- network/build.sh | 4 ++++ 3 files changed, 12 insertions(+), 5 deletions(-) create mode 100644 network/build.sh diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java index c4fbfc3d0f3..88252e33200 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java @@ -99,7 +99,12 @@ public synchronized void startTimer( public synchronized boolean checkEmptyBlockExpired(final BlockHeader chainHeadHeader, final long currentTimeInMillis) { final long emptyBlockPeriodExpiryTime = (chainHeadHeader.getTimestamp() + emptyBlockPeriodSeconds) * 1000; - if (currentTimeInMillis >= emptyBlockPeriodExpiryTime) { + + final long newExpiry = currentTimeInMillis + blockPeriodSeconds * 1000; + final long newEmptyExpiry = currentTimeInMillis + (emptyBlockPeriodSeconds - blockPeriodSeconds) * 1000; + final long nextBlockPeriodExpiryTime = Math.min(newExpiry, newEmptyExpiry); + + if (nextBlockPeriodExpiryTime > emptyBlockPeriodExpiryTime) { LOG.info("Empty Block expired"); return true; } @@ -109,7 +114,8 @@ public synchronized boolean checkEmptyBlockExpired(final BlockHeader chainHeadHe public void resetTimerForEmptyBlock(final ConsensusRoundIdentifier roundIdentifier, final long currentTimeInMillis){ final long newExpiry = currentTimeInMillis + blockPeriodSeconds * 1000; - startTimer(roundIdentifier, newExpiry); + final long newEmptyExpiry = currentTimeInMillis + (emptyBlockPeriodSeconds - blockPeriodSeconds) * 1000; + startTimer(roundIdentifier, Math.max(newExpiry, newEmptyExpiry)); } private synchronized void startTimer( diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java index 08fe3582793..85a478c9aaf 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java @@ -231,9 +231,6 @@ protected void cleanup() { case ALREADY_IMPORTED: LOG.info("Block {} is already imported", block.toLogString()); break; - case NOT_IMPORTED: - LOG.info("Block {} was not imported", block.toLogString()); - break; default: break; } diff --git a/network/build.sh b/network/build.sh new file mode 100644 index 00000000000..12477ac36ea --- /dev/null +++ b/network/build.sh @@ -0,0 +1,4 @@ +cd .. +./gradlew installDist +cd network +./start.sh --no-data \ No newline at end of file From 5529a23fd2d0b00b3bfefe41e0ff4c1d550a1734 Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Tue, 14 May 2024 14:40:11 +0100 Subject: [PATCH 13/27] Corrected the way the BlockTimer handles emptyBlockPeriodSeconds and other stuff Signed-off-by: Antonio Mota --- network/build.sh | 4 ---- 1 file changed, 4 deletions(-) delete mode 100644 network/build.sh diff --git a/network/build.sh b/network/build.sh deleted file mode 100644 index 12477ac36ea..00000000000 --- a/network/build.sh +++ /dev/null @@ -1,4 +0,0 @@ -cd .. -./gradlew installDist -cd network -./start.sh --no-data \ No newline at end of file From a45b725261660a917a56d9ec715e65979ffe6b1a Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Fri, 21 Jun 2024 08:24:58 +0100 Subject: [PATCH 14/27] Corrected the way the BlockTimer handles the next block expire block Signed-off-by: Antonio Mota --- .../besu/consensus/common/bft/BlockTimer.java | 15 ++++++--------- .../qbft/statemachine/QbftBlockHeightManager.java | 2 +- .../ethereum/eth/sync/tasks/PersistBlockTask.java | 3 ++- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java index 88252e33200..4ecdca9e02f 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java @@ -100,11 +100,7 @@ public synchronized void startTimer( public synchronized boolean checkEmptyBlockExpired(final BlockHeader chainHeadHeader, final long currentTimeInMillis) { final long emptyBlockPeriodExpiryTime = (chainHeadHeader.getTimestamp() + emptyBlockPeriodSeconds) * 1000; - final long newExpiry = currentTimeInMillis + blockPeriodSeconds * 1000; - final long newEmptyExpiry = currentTimeInMillis + (emptyBlockPeriodSeconds - blockPeriodSeconds) * 1000; - final long nextBlockPeriodExpiryTime = Math.min(newExpiry, newEmptyExpiry); - - if (nextBlockPeriodExpiryTime > emptyBlockPeriodExpiryTime) { + if (currentTimeInMillis > emptyBlockPeriodExpiryTime) { LOG.info("Empty Block expired"); return true; } @@ -112,10 +108,11 @@ public synchronized boolean checkEmptyBlockExpired(final BlockHeader chainHeadHe return false; } - public void resetTimerForEmptyBlock(final ConsensusRoundIdentifier roundIdentifier, final long currentTimeInMillis){ - final long newExpiry = currentTimeInMillis + blockPeriodSeconds * 1000; - final long newEmptyExpiry = currentTimeInMillis + (emptyBlockPeriodSeconds - blockPeriodSeconds) * 1000; - startTimer(roundIdentifier, Math.max(newExpiry, newEmptyExpiry)); + public void resetTimerForEmptyBlock(final ConsensusRoundIdentifier roundIdentifier, final BlockHeader chainHeadHeader, final long currentTimeInMillis){ + final long emptyBlockPeriodExpiryTime = (chainHeadHeader.getTimestamp() + emptyBlockPeriodSeconds) * 1000; + final long nextBlockPeriodExpiryTime = currentTimeInMillis + blockPeriodSeconds * 1000; + + startTimer(roundIdentifier, Math.min(emptyBlockPeriodExpiryTime, nextBlockPeriodExpiryTime)); } private synchronized void startTimer( diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java index abc395c3556..349994f15c2 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java @@ -174,7 +174,7 @@ private void buildBlockAndMaybePropose( } else { LOG.info( "Block has no transactions but emptyBlockPeriodSeconds did not expired yet: " + roundIdentifier); - finalState.getBlockTimer().resetTimerForEmptyBlock(roundIdentifier, currentTimeInMillis); + finalState.getBlockTimer().resetTimerForEmptyBlock(roundIdentifier, parentHeader, currentTimeInMillis); finalState.getRoundTimer().cancelTimer(); currentRound = Optional.empty(); } diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java index 85a478c9aaf..6333f0d1913 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/tasks/PersistBlockTask.java @@ -218,7 +218,8 @@ protected void cleanup() { case IMPORTED: LOG.info( String.format( - "Imported #%,d / %d tx / %d om / %,d (%01.1f%%) gas / (%s) in %01.3fs. Peers: %d", + "Imported %s #%,d / %d tx / %d om / %,d (%01.1f%%) gas / (%s) in %01.3fs. Peers: %d", + block.getBody().getTransactions().size() == 0 ? "empty block" : "block", block.getHeader().getNumber(), block.getBody().getTransactions().size(), block.getBody().getOmmers().size(), From 26bfeed22875df06c3a353ebbb1302b0bdbfcf43 Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Fri, 21 Jun 2024 09:18:38 +0100 Subject: [PATCH 15/27] Corrected the way the BlockTimer handles the next block expire block Signed-off-by: Antonio Mota --- .../besu/consensus/common/bft/BlockTimer.java | 42 +++++++++++++++---- .../statemachine/QbftBlockHeightManager.java | 23 ++++++---- 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java index 4ecdca9e02f..dba1a022f39 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java @@ -96,9 +96,17 @@ public synchronized void startTimer( startTimer(round, expiryTime); } - - public synchronized boolean checkEmptyBlockExpired(final BlockHeader chainHeadHeader, final long currentTimeInMillis) { - final long emptyBlockPeriodExpiryTime = (chainHeadHeader.getTimestamp() + emptyBlockPeriodSeconds) * 1000; + /** + * Checks if the empty block timer is expired + * + * @param chainHeadHeader The header of the chain head + * @param currentTimeInMillis The current time + * @return a boolean value + */ + public synchronized boolean checkEmptyBlockExpired( + final BlockHeader chainHeadHeader, final long currentTimeInMillis) { + final long emptyBlockPeriodExpiryTime = + (chainHeadHeader.getTimestamp() + emptyBlockPeriodSeconds) * 1000; if (currentTimeInMillis > emptyBlockPeriodExpiryTime) { LOG.info("Empty Block expired"); @@ -108,8 +116,19 @@ public synchronized boolean checkEmptyBlockExpired(final BlockHeader chainHeadHe return false; } - public void resetTimerForEmptyBlock(final ConsensusRoundIdentifier roundIdentifier, final BlockHeader chainHeadHeader, final long currentTimeInMillis){ - final long emptyBlockPeriodExpiryTime = (chainHeadHeader.getTimestamp() + emptyBlockPeriodSeconds) * 1000; + /** + * Resets the empty block timer + * + * @param roundIdentifier The current round identifier + * @param chainHeadHeader The header of the chain head + * @param currentTimeInMillis The current time + */ + public void resetTimerForEmptyBlock( + final ConsensusRoundIdentifier roundIdentifier, + final BlockHeader chainHeadHeader, + final long currentTimeInMillis) { + final long emptyBlockPeriodExpiryTime = + (chainHeadHeader.getTimestamp() + emptyBlockPeriodSeconds) * 1000; final long nextBlockPeriodExpiryTime = currentTimeInMillis + blockPeriodSeconds * 1000; startTimer(roundIdentifier, Math.min(emptyBlockPeriodExpiryTime, nextBlockPeriodExpiryTime)); @@ -133,18 +152,27 @@ private synchronized void startTimer( } } - private synchronized void setBlockTimes( - final ConsensusRoundIdentifier round) { + private synchronized void setBlockTimes(final ConsensusRoundIdentifier round) { final BftConfigOptions currentConfigOptions = forksSchedule.getFork(round.getSequenceNumber()).getValue(); this.blockPeriodSeconds = currentConfigOptions.getBlockPeriodSeconds(); this.emptyBlockPeriodSeconds = currentConfigOptions.getEmptyBlockPeriodSeconds(); } + /** + * Retrieves the Block Period Seconds + * + * @return the Block Period Seconds + */ public synchronized long getBlockPeriodSeconds() { return blockPeriodSeconds; } + /** + * Retrieves the Empty Block Period Seconds + * + * @return the Empty Block Period Seconds + */ public synchronized long getEmptyBlockPeriodSeconds() { return emptyBlockPeriodSeconds; } diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java index 349994f15c2..b147294a88f 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java @@ -153,28 +153,37 @@ private void buildBlockAndMaybePropose( final boolean blockHasTransactions = !block.getBody().getTransactions().isEmpty(); if (blockHasTransactions) { if (isProposer) { - LOG.info("Block has transactions and this node is a proposer so it will send a proposal: " + roundIdentifier); + LOG.info( + "Block has transactions and this node is a proposer so it will send a proposal: " + + roundIdentifier); qbftRound.sendProposalMessage(block); } else { LOG.info( - "Block has transactions but this node is not a proposer so it will not send a proposal: " + roundIdentifier); + "Block has transactions but this node is not a proposer so it will not send a proposal: " + + roundIdentifier); } } else { final long currentTimeInMillis = finalState.getClock().millis(); - boolean emptyBlockExpired = finalState.getBlockTimer().checkEmptyBlockExpired(parentHeader, currentTimeInMillis); + boolean emptyBlockExpired = + finalState.getBlockTimer().checkEmptyBlockExpired(parentHeader, currentTimeInMillis); if (emptyBlockExpired) { if (isProposer) { LOG.info( - "Block has no transactions and this node is a proposer so it will send a proposal: " + roundIdentifier); + "Block has no transactions and this node is a proposer so it will send a proposal: " + + roundIdentifier); qbftRound.sendProposalMessage(block); } else { LOG.info( - "Block has no transactions but this node is not a proposer so it will not send a proposal: " + roundIdentifier); + "Block has no transactions but this node is not a proposer so it will not send a proposal: " + + roundIdentifier); } } else { LOG.info( - "Block has no transactions but emptyBlockPeriodSeconds did not expired yet: " + roundIdentifier); - finalState.getBlockTimer().resetTimerForEmptyBlock(roundIdentifier, parentHeader, currentTimeInMillis); + "Block has no transactions but emptyBlockPeriodSeconds did not expired yet: " + + roundIdentifier); + finalState + .getBlockTimer() + .resetTimerForEmptyBlock(roundIdentifier, parentHeader, currentTimeInMillis); finalState.getRoundTimer().cancelTimer(); currentRound = Optional.empty(); } From 41d7c4b7a49452be5e7055c238f8a4e1efad001c Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Sat, 13 Jul 2024 14:51:54 +0100 Subject: [PATCH 16/27] Changes after review, reviweing failing tests Signed-off-by: Antonio Mota --- .../besu/consensus/common/bft/BlockTimer.java | 4 +- .../statemachine/QbftBlockHeightManager.java | 38 +++++++++---------- .../qbft/statemachine/QbftRound.java | 4 +- .../QbftBlockHeightManagerTest.java | 4 +- gradle.properties | 9 ++++- 5 files changed, 31 insertions(+), 28 deletions(-) diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java index dba1a022f39..29d5551441b 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java @@ -109,10 +109,10 @@ public synchronized boolean checkEmptyBlockExpired( (chainHeadHeader.getTimestamp() + emptyBlockPeriodSeconds) * 1000; if (currentTimeInMillis > emptyBlockPeriodExpiryTime) { - LOG.info("Empty Block expired"); + LOG.debug("Empty Block expired"); return true; } - LOG.info("Empty Block NOT expired"); + LOG.debug("Empty Block NOT expired"); return false; } diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java index 6a8535d64a8..44860890ffe 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java @@ -148,37 +148,33 @@ private void buildBlockAndMaybePropose( final boolean isProposer = finalState.isLocalNodeProposerForRound(qbftRound.getRoundIdentifier()); + if (!isProposer) { + // nothing to do here... + LOG.trace("This node is not a proposer so it will not send a proposal: " + roundIdentifier); + return; + } + final long headerTimeStampSeconds = Math.round(clock.millis() / 1000D); final Block block = qbftRound.createBlock(headerTimeStampSeconds); final boolean blockHasTransactions = !block.getBody().getTransactions().isEmpty(); if (blockHasTransactions) { - if (isProposer) { - LOG.info( - "Block has transactions and this node is a proposer so it will send a proposal: " - + roundIdentifier); - qbftRound.sendProposalMessage(block); - } else { - LOG.info( - "Block has transactions but this node is not a proposer so it will not send a proposal: " - + roundIdentifier); - } + LOG.trace( + "Block has transactions and this node is a proposer so it will send a proposal: " + + roundIdentifier); + qbftRound.sendProposalMessage(block); + } else { + // handle the block times period final long currentTimeInMillis = finalState.getClock().millis(); boolean emptyBlockExpired = finalState.getBlockTimer().checkEmptyBlockExpired(parentHeader, currentTimeInMillis); if (emptyBlockExpired) { - if (isProposer) { - LOG.info( - "Block has no transactions and this node is a proposer so it will send a proposal: " - + roundIdentifier); - qbftRound.sendProposalMessage(block); - } else { - LOG.info( - "Block has no transactions but this node is not a proposer so it will not send a proposal: " - + roundIdentifier); - } + LOG.trace( + "Block has no transactions and this node is a proposer so it will send a proposal: " + + roundIdentifier); + qbftRound.sendProposalMessage(block); } else { - LOG.info( + LOG.trace( "Block has no transactions but emptyBlockPeriodSeconds did not expired yet: " + roundIdentifier); finalState diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java index f6e5f143689..7fd5da9c1b6 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java @@ -130,6 +130,7 @@ public ConsensusRoundIdentifier getRoundIdentifier() { /** * Create a block * + * @param headerTimeStampSeconds of the block * @return a Block */ public Block createBlock(final long headerTimeStampSeconds) { @@ -143,7 +144,7 @@ public Block createBlock(final long headerTimeStampSeconds) { * @param block to send */ public void sendProposalMessage(final Block block) { - LOG.debug("Creating proposed block blockHeader={}", block.getHeader()); + LOG.trace("Creating proposed block blockHeader={}", block.getHeader()); updateStateWithProposalAndTransmit(block); } @@ -192,7 +193,6 @@ public void startRoundWith( * @param block the block */ protected void updateStateWithProposalAndTransmit(final Block block) { - updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); } diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java index 7973a1d6b82..7f7b0c3ca1d 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java @@ -155,6 +155,7 @@ public void setup() { when(messageValidator.validateCommit(any())).thenReturn(true); when(messageValidator.validatePrepare(any())).thenReturn(true); when(finalState.getBlockTimer()).thenReturn(blockTimer); + when(finalState.getRoundTimer()).thenReturn(roundTimer); when(finalState.getQuorum()).thenReturn(3); when(finalState.getValidatorMulticaster()).thenReturn(validatorMulticaster); when(finalState.getClock()).thenReturn(clock); @@ -576,6 +577,7 @@ public void illegalFutureRoundProposalDoesNotTriggerNewRound() { @Test public void checkOnlyEmptyBlockPeriodSecondsIsInvokedForBlocksWithNoTransactions() { when(finalState.isLocalNodeProposerForRound(roundIdentifier)).thenReturn(true); + final QbftBlockHeightManager manager = new QbftBlockHeightManager( @@ -589,7 +591,7 @@ public void checkOnlyEmptyBlockPeriodSecondsIsInvokedForBlocksWithNoTransactions manager.handleBlockTimerExpiry(roundIdentifier); - verify(blockTimer, atLeastOnce()).getEmptyBlockPeriodSeconds(); + verify(blockTimer, times(0)).getEmptyBlockPeriodSeconds(); verify(blockTimer, times(0)).getBlockPeriodSeconds(); } } diff --git a/gradle.properties b/gradle.properties index 93f56e01379..246aea04e9b 100644 --- a/gradle.properties +++ b/gradle.properties @@ -4,8 +4,12 @@ org.gradle.welcome=never # version=24.5.6-acme # versionappendcommit=true +test.maxParallelForks = 1 +test.forkEvery = 100 + + # Set exports/opens flags required by Google Java Format and ErrorProne plugins. (JEP-396) -org.gradle.jvmargs=-Xmx4g \ +org.gradle.jvmargs=-Xmx3g \ --add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \ --add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \ --add-exports jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED \ @@ -18,4 +22,5 @@ org.gradle.jvmargs=-Xmx4g \ --add-opens jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED \ --add-opens jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED # Could be moved to sonar properties after https://sonarsource.atlassian.net/browse/SONARGRADL-134 -systemProp.sonar.gradle.skipCompile=true \ No newline at end of file +systemProp.sonar.gradle.skipCompile=true +#sonar.gradle.skipCompile=true From 5cacd7ce3872347fbde55e5ac5dfeb347cc4c9db Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Sun, 14 Jul 2024 20:59:30 +0100 Subject: [PATCH 17/27] Fixed formatting Signed-off-by: Antonio Mota --- .../consensus/qbft/statemachine/QbftBlockHeightManagerTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java index 7f7b0c3ca1d..20761da1af2 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java @@ -577,7 +577,6 @@ public void illegalFutureRoundProposalDoesNotTriggerNewRound() { @Test public void checkOnlyEmptyBlockPeriodSecondsIsInvokedForBlocksWithNoTransactions() { when(finalState.isLocalNodeProposerForRound(roundIdentifier)).thenReturn(true); - final QbftBlockHeightManager manager = new QbftBlockHeightManager( From 20f5e430211d2f08bc9e0bda1f1930b5cbe4f829 Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Mon, 15 Jul 2024 20:23:10 +0100 Subject: [PATCH 18/27] Fixed failing tests in QbftBlockHeightManagerTest Signed-off-by: Antonio Mota --- .../qbft/statemachine/QbftBlockHeightManagerTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java index 20761da1af2..2a39808d4fa 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java @@ -263,6 +263,7 @@ public void doesNotStartRoundTimerOnStart() { @Test public void onBlockTimerExpiryRoundTimerIsStartedAndProposalMessageIsTransmitted() { when(finalState.isLocalNodeProposerForRound(roundIdentifier)).thenReturn(true); + when(blockTimer.checkEmptyBlockExpired(any(), eq(0l))).thenReturn(true); final QbftBlockHeightManager manager = new QbftBlockHeightManager( @@ -459,6 +460,7 @@ public void messagesForFutureRoundsAreBufferedAndUsedToPreloadNewRoundWhenItIsSt public void messagesForCurrentRoundAreBufferedAndUsedToPreloadRoundWhenItIsStarted() { when(finalState.getQuorum()).thenReturn(1); when(finalState.isLocalNodeProposerForRound(roundIdentifier)).thenReturn(true); + when(blockTimer.checkEmptyBlockExpired(any(), eq(0l))).thenReturn(true); final QbftBlockHeightManager manager = new QbftBlockHeightManager( @@ -496,6 +498,7 @@ public void messagesForCurrentRoundAreBufferedAndUsedToPreloadRoundWhenItIsStart @Test public void preparedCertificateIncludedInRoundChangeMessageOnRoundTimeoutExpired() { when(finalState.isLocalNodeProposerForRound(any())).thenReturn(true); + when(blockTimer.checkEmptyBlockExpired(any(), eq(0l))).thenReturn(true); final QbftBlockHeightManager manager = new QbftBlockHeightManager( From 78ed724657a5b48241105e17c97d3de3c6d60387 Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Tue, 16 Jul 2024 09:04:23 +0100 Subject: [PATCH 19/27] Fixed failing tests in BlockerTimerTest Signed-off-by: Antonio Mota --- .../consensus/common/bft/BlockTimerTest.java | 21 +++++++++---------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java index 02e7500e404..7521e5eb301 100644 --- a/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java +++ b/consensus/common/src/test/java/org/hyperledger/besu/consensus/common/bft/BlockTimerTest.java @@ -63,17 +63,6 @@ public void initialise() { @Test public void cancelTimerCancelsWhenNoTimer() { - final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 15; - final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; - - when(mockForksSchedule.getFork(anyLong())) - .thenReturn( - new ForkSpec<>( - 0, - createBftFork( - MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS, - MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); - final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); // Starts with nothing running assertThat(timer.isRunning()).isFalse(); @@ -314,6 +303,15 @@ public void runningFollowsTheStateOfTheTimer() { public void checkBlockTimerEmptyAndNonEmptyPeriodSecods() { final int MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS = 15; final int MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS = 60; + final long BLOCK_TIME_STAMP = 500L; + final ConsensusRoundIdentifier round = + new ConsensusRoundIdentifier(0xFEDBCA9876543210L, 0x12345678); + final BlockHeader header = + new BlockHeaderTestFixture().timestamp(BLOCK_TIME_STAMP).buildHeader(); + final ScheduledFuture mockedFuture = mock(ScheduledFuture.class); + Mockito.>when( + bftExecutors.scheduleTask(any(Runnable.class), anyLong(), any())) + .thenReturn(mockedFuture); when(mockForksSchedule.getFork(anyLong())) .thenReturn( @@ -324,6 +322,7 @@ public void checkBlockTimerEmptyAndNonEmptyPeriodSecods() { MINIMAL_TIME_BETWEEN_EMPTY_BLOCKS_SECONDS))); final BlockTimer timer = new BlockTimer(mockQueue, mockForksSchedule, bftExecutors, mockClock); + timer.startTimer(round, header); assertThat(timer.getBlockPeriodSeconds()).isEqualTo(MINIMAL_TIME_BETWEEN_BLOCKS_SECONDS); assertThat(timer.getEmptyBlockPeriodSeconds()) From f238d37c5c24700f4e9b59a51baeebdcd88d43b8 Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Wed, 11 Sep 2024 21:23:11 +0100 Subject: [PATCH 20/27] Fixed conflicts Signed-off-by: Antonio Mota --- .../besu/consensus/qbft/statemachine/QbftRound.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java index 18699a9e997..72772081a1d 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java @@ -139,7 +139,7 @@ public ConsensusRoundIdentifier getRoundIdentifier() { */ public Block createBlock(final long headerTimeStampSeconds) { LOG.debug("Creating proposed block. round={}", roundState.getRoundIdentifier()); - return blockCreator.createBlock(headerTimeStampSeconds).getBlock(); + return blockCreator.createBlock(headerTimeStampSeconds, this.parentHeader).getBlock(); } /** @@ -162,7 +162,8 @@ public void createAndSendProposalMessage(final long headerTimeStampSeconds) { final Block block = blockCreator.createBlock(headerTimeStampSeconds, this.parentHeader).getBlock(); - sendProposalMessage(block); + LOG.trace("Creating proposed block blockHeader={}", block.getHeader()); + updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); } /** From 0c6fc856713a1b08c8ca23f6ae6ca69de356dba6 Mon Sep 17 00:00:00 2001 From: amsmota Date: Fri, 20 Sep 2024 14:43:04 +0100 Subject: [PATCH 21/27] Fixed conflicts from merge, implemented suggested changes, reverted gradle props. Signed-off-by: amsmota --- .../org/hyperledger/besu/config/BftFork.java | 2 +- .../besu/config/JsonBftConfigOptions.java | 2 +- .../besu/config/JsonBftConfigOptionsTest.java | 2 +- .../besu/consensus/common/bft/BlockTimer.java | 45 ++-- .../statemachine/QbftBlockHeightManager.java | 4 +- .../qbft/statemachine/QbftRound.java | 26 +-- .../qbft/statemachine/QbftRoundTest.java | 207 +++++++++--------- gradle.properties | 9 +- 8 files changed, 142 insertions(+), 155 deletions(-) diff --git a/config/src/main/java/org/hyperledger/besu/config/BftFork.java b/config/src/main/java/org/hyperledger/besu/config/BftFork.java index 3b9df1981d3..9083bf339ec 100644 --- a/config/src/main/java/org/hyperledger/besu/config/BftFork.java +++ b/config/src/main/java/org/hyperledger/besu/config/BftFork.java @@ -42,7 +42,7 @@ public class BftFork implements Fork { 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"; + public static final String EMPTY_BLOCK_PERIOD_SECONDS_KEY = "xemptyblockperiodseconds"; /** The constant BLOCK_PERIOD_MILLISECONDS_KEY. */ public static final String BLOCK_PERIOD_MILLISECONDS_KEY = "xblockperiodmilliseconds"; diff --git a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java index db9071cbcf2..fb9c8d6c796 100644 --- a/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/JsonBftConfigOptions.java @@ -72,7 +72,7 @@ public int getBlockPeriodSeconds() { @Override public int getEmptyBlockPeriodSeconds() { return JsonUtil.getInt( - bftConfigRoot, "emptyblockperiodseconds", DEFAULT_EMPTY_BLOCK_PERIOD_SECONDS); + bftConfigRoot, "xemptyblockperiodseconds", DEFAULT_EMPTY_BLOCK_PERIOD_SECONDS); } @Override diff --git a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java b/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java index ff376ef442d..db7d3957908 100644 --- a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java +++ b/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java @@ -65,7 +65,7 @@ public void shouldGetBlockPeriodFromConfig() { @Test public void shouldGetEmptyBlockPeriodFromConfig() { - final BftConfigOptions config = fromConfigOptions(singletonMap("emptyblockperiodseconds", 60)); + final BftConfigOptions config = fromConfigOptions(singletonMap("xemptyblockperiodseconds", 60)); assertThat(config.getEmptyBlockPeriodSeconds()).isEqualTo(60); } diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java index 7d5b9ab5d2a..380aa034b5f 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java @@ -85,29 +85,32 @@ public synchronized boolean isRunning() { */ public synchronized void startTimer( final ConsensusRoundIdentifier round, final BlockHeader chainHeadHeader) { - cancelTimer(); - - final long now = clock.millis(); - final long expiryTime; + // cancelTimer(); + // + // final long now = clock.millis(); + // final long expiryTime; // Experimental option for test scenarios only. Not for production use. - final long blockPeriodMilliseconds = - forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodMilliseconds(); - - if (blockPeriodMilliseconds > 0) { - // Experimental mode for setting < 1 second block periods e.g. for CI/CD pipelines - // running tests against Besu - expiryTime = clock.millis() + blockPeriodMilliseconds; - LOG.warn( - "Test-mode only xblockperiodmilliseconds has been set to {} millisecond blocks. Do not use in a production system.", - blockPeriodMilliseconds); - } else { - // absolute time when the timer is supposed to expire - final int blockPeriodSeconds = - forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodSeconds(); - final long minimumTimeBetweenBlocksMillis = blockPeriodSeconds * 1000L; - expiryTime = chainHeadHeader.getTimestamp() * 1_000 + minimumTimeBetweenBlocksMillis; - } + // final long blockPeriodMilliseconds = + // + // forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodMilliseconds(); + // + // if (blockPeriodMilliseconds > 0) { + // // Experimental mode for setting < 1 second block periods e.g. for CI/CD pipelines + // // running tests against Besu + // expiryTime = clock.millis() + blockPeriodMilliseconds; + // LOG.warn( + // "Test-mode only xblockperiodmilliseconds has been set to {} millisecond blocks. Do + // not use in a production system.", + // blockPeriodMilliseconds); + // } else { + // // absolute time when the timer is supposed to expire + // final int blockPeriodSeconds = + // forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodSeconds(); + // final long minimumTimeBetweenBlocksMillis = blockPeriodSeconds * 1000L; + // expiryTime = chainHeadHeader.getTimestamp() * 1_000 + minimumTimeBetweenBlocksMillis; + // } + // absolute time when the timer is supposed to expire final int currentBlockPeriodSeconds = forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodSeconds(); diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java index 44860890ffe..757998b5ca6 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManager.java @@ -161,7 +161,7 @@ private void buildBlockAndMaybePropose( LOG.trace( "Block has transactions and this node is a proposer so it will send a proposal: " + roundIdentifier); - qbftRound.sendProposalMessage(block); + qbftRound.updateStateWithProposalAndTransmit(block); } else { // handle the block times period @@ -172,7 +172,7 @@ private void buildBlockAndMaybePropose( LOG.trace( "Block has no transactions and this node is a proposer so it will send a proposal: " + roundIdentifier); - qbftRound.sendProposalMessage(block); + qbftRound.updateStateWithProposalAndTransmit(block); } else { LOG.trace( "Block has no transactions but emptyBlockPeriodSeconds did not expired yet: " diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java index 72772081a1d..e3f5f6257fa 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java @@ -142,29 +142,19 @@ public Block createBlock(final long headerTimeStampSeconds) { return blockCreator.createBlock(headerTimeStampSeconds, this.parentHeader).getBlock(); } - /** - * Send proposal message. - * - * @param block to send - */ - public void sendProposalMessage(final Block block) { - LOG.trace("Creating proposed block blockHeader={}", block.getHeader()); - updateStateWithProposalAndTransmit(block); - } - /** * Create and send proposal message. * * @param headerTimeStampSeconds the header time stamp seconds */ - public void createAndSendProposalMessage(final long headerTimeStampSeconds) { - LOG.debug("Creating proposed block. round={}", roundState.getRoundIdentifier()); - final Block block = - blockCreator.createBlock(headerTimeStampSeconds, this.parentHeader).getBlock(); - - LOG.trace("Creating proposed block blockHeader={}", block.getHeader()); - updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); - } +// public void createAndSendProposalMessage(final long headerTimeStampSeconds) { +// LOG.debug("Creating proposed block. round={}", roundState.getRoundIdentifier()); +// final Block block = +// blockCreator.createBlock(headerTimeStampSeconds, this.parentHeader).getBlock(); +// +// LOG.trace("Creating proposed block blockHeader={}", block.getHeader()); +// updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); +// } /** * Start round with. diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundTest.java index 514fd653a1d..827779f6988 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundTest.java @@ -29,7 +29,6 @@ import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; -import org.hyperledger.besu.consensus.common.bft.BftBlockHashing; import org.hyperledger.besu.consensus.common.bft.BftContext; import org.hyperledger.besu.consensus.common.bft.BftExtraData; import org.hyperledger.besu.consensus.common.bft.BftExtraDataCodec; @@ -48,7 +47,6 @@ import org.hyperledger.besu.crypto.SignatureAlgorithmFactory; import org.hyperledger.besu.cryptoservices.NodeKey; import org.hyperledger.besu.cryptoservices.NodeKeyUtils; -import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.ProtocolContext; import org.hyperledger.besu.ethereum.blockcreation.BlockCreationTiming; import org.hyperledger.besu.ethereum.blockcreation.BlockCreator.BlockCreationResult; @@ -196,89 +194,90 @@ public void onReceptionOfValidProposalSendsAPrepareToNetworkPeers() { verify(transmitter, never()).multicastCommit(any(), any(), any()); } - @Test - public void sendsAProposalAndPrepareWhenSendProposalRequested() { - final RoundState roundState = new RoundState(roundIdentifier, 3, messageValidator); - final QbftRound round = - new QbftRound( - roundState, - blockCreator, - protocolContext, - protocolSchedule, - subscribers, - nodeKey, - messageFactory, - transmitter, - roundTimer, - bftExtraDataCodec, - parentHeader); - - round.createAndSendProposalMessage(15); - verify(transmitter, times(1)) - .multicastProposal( - roundIdentifier, proposedBlock, Collections.emptyList(), Collections.emptyList()); - verify(transmitter, times(1)).multicastPrepare(roundIdentifier, proposedBlock.getHash()); - verify(transmitter, never()).multicastCommit(any(), any(), any()); - } - - @Test - public void singleValidatorImportBlocksImmediatelyOnProposalCreation() { - final RoundState roundState = new RoundState(roundIdentifier, 1, messageValidator); - final QbftRound round = - new QbftRound( - roundState, - blockCreator, - protocolContext, - protocolSchedule, - subscribers, - nodeKey, - messageFactory, - transmitter, - roundTimer, - bftExtraDataCodec, - parentHeader); - round.createAndSendProposalMessage(15); - verify(transmitter, times(1)) - .multicastProposal( - roundIdentifier, proposedBlock, Collections.emptyList(), Collections.emptyList()); - verify(transmitter, times(1)).multicastPrepare(roundIdentifier, proposedBlock.getHash()); - verify(transmitter, times(1)).multicastCommit(any(), any(), any()); - } - - @Test - public void localNodeProposesToNetworkOfTwoValidatorsImportsOnReceptionOfCommitFromPeer() { - final RoundState roundState = new RoundState(roundIdentifier, 2, messageValidator); - final QbftRound round = - new QbftRound( - roundState, - blockCreator, - protocolContext, - protocolSchedule, - subscribers, - nodeKey, - messageFactory, - transmitter, - roundTimer, - bftExtraDataCodec, - parentHeader); - - final Hash commitSealHash = - new BftBlockHashing(new QbftExtraDataCodec()) - .calculateDataHashForCommittedSeal(proposedBlock.getHeader(), proposedExtraData); - final SECPSignature localCommitSeal = nodeKey.sign(commitSealHash); - - round.createAndSendProposalMessage(15); - verify(transmitter, never()).multicastCommit(any(), any(), any()); - - round.handlePrepareMessage( - messageFactory2.createPrepare(roundIdentifier, proposedBlock.getHash())); - - verify(transmitter, times(1)) - .multicastCommit(roundIdentifier, proposedBlock.getHash(), localCommitSeal); - - round.handleCommitMessage( - messageFactory.createCommit(roundIdentifier, proposedBlock.getHash(), remoteCommitSeal)); - } + // @Test + // public void sendsAProposalAndPrepareWhenSendProposalRequested() { + // final RoundState roundState = new RoundState(roundIdentifier, 3, messageValidator); + // final QbftRound round = + // new QbftRound( + // roundState, + // blockCreator, + // protocolContext, + // protocolSchedule, + // subscribers, + // nodeKey, + // messageFactory, + // transmitter, + // roundTimer, + // bftExtraDataCodec, + // parentHeader); + // + // round.createAndSendProposalMessage(15); + // verify(transmitter, times(1)) + // .multicastProposal( + // roundIdentifier, proposedBlock, Collections.emptyList(), Collections.emptyList()); + // verify(transmitter, times(1)).multicastPrepare(roundIdentifier, proposedBlock.getHash()); + // verify(transmitter, never()).multicastCommit(any(), any(), any()); + // } + + // @Test + // public void singleValidatorImportBlocksImmediatelyOnProposalCreation() { + // final RoundState roundState = new RoundState(roundIdentifier, 1, messageValidator); + // final QbftRound round = + // new QbftRound( + // roundState, + // blockCreator, + // protocolContext, + // protocolSchedule, + // subscribers, + // nodeKey, + // messageFactory, + // transmitter, + // roundTimer, + // bftExtraDataCodec, + // parentHeader); + // round.createAndSendProposalMessage(15); + // verify(transmitter, times(1)) + // .multicastProposal( + // roundIdentifier, proposedBlock, Collections.emptyList(), Collections.emptyList()); + // verify(transmitter, times(1)).multicastPrepare(roundIdentifier, proposedBlock.getHash()); + // verify(transmitter, times(1)).multicastCommit(any(), any(), any()); + // } + + // @Test + // public void localNodeProposesToNetworkOfTwoValidatorsImportsOnReceptionOfCommitFromPeer() { + // final RoundState roundState = new RoundState(roundIdentifier, 2, messageValidator); + // final QbftRound round = + // new QbftRound( + // roundState, + // blockCreator, + // protocolContext, + // protocolSchedule, + // subscribers, + // nodeKey, + // messageFactory, + // transmitter, + // roundTimer, + // bftExtraDataCodec, + // parentHeader); + // + // final Hash commitSealHash = + // new BftBlockHashing(new QbftExtraDataCodec()) + // .calculateDataHashForCommittedSeal(proposedBlock.getHeader(), proposedExtraData); + // final SECPSignature localCommitSeal = nodeKey.sign(commitSealHash); + // + // round.createAndSendProposalMessage(15); + // verify(transmitter, never()).multicastCommit(any(), any(), any()); + // + // round.handlePrepareMessage( + // messageFactory2.createPrepare(roundIdentifier, proposedBlock.getHash())); + // + // verify(transmitter, times(1)) + // .multicastCommit(roundIdentifier, proposedBlock.getHash(), localCommitSeal); + // + // round.handleCommitMessage( + // messageFactory.createCommit(roundIdentifier, proposedBlock.getHash(), + // remoteCommitSeal)); + // } @Test public void aProposalWithAnewBlockIsSentUponReceptionOfARoundChangeWithNoCertificate() { @@ -393,25 +392,25 @@ public void creatingNewBlockFromEmptyPreparedCertificateUpdatesInternalState() { assertThat(roundState.isPrepared()).isTrue(); } - @Test - public void creatingNewBlockNotifiesBlockMiningObservers() { - final RoundState roundState = new RoundState(roundIdentifier, 1, messageValidator); - final QbftRound round = - new QbftRound( - roundState, - blockCreator, - protocolContext, - protocolSchedule, - subscribers, - nodeKey, - messageFactory, - transmitter, - roundTimer, - bftExtraDataCodec, - parentHeader); - round.createAndSendProposalMessage(15); - verify(minedBlockObserver).blockMined(any()); - } + // @Test + // public void creatingNewBlockNotifiesBlockMiningObservers() { + // final RoundState roundState = new RoundState(roundIdentifier, 1, messageValidator); + // final QbftRound round = + // new QbftRound( + // roundState, + // blockCreator, + // protocolContext, + // protocolSchedule, + // subscribers, + // nodeKey, + // messageFactory, + // transmitter, + // roundTimer, + // bftExtraDataCodec, + // parentHeader); + // round.createAndSendProposalMessage(15); + // verify(minedBlockObserver).blockMined(any()); + // } @Test public void blockIsOnlyImportedOnceWhenCommitsAreReceivedBeforeProposal() { diff --git a/gradle.properties b/gradle.properties index 246aea04e9b..93f56e01379 100644 --- a/gradle.properties +++ b/gradle.properties @@ -4,12 +4,8 @@ org.gradle.welcome=never # version=24.5.6-acme # versionappendcommit=true -test.maxParallelForks = 1 -test.forkEvery = 100 - - # Set exports/opens flags required by Google Java Format and ErrorProne plugins. (JEP-396) -org.gradle.jvmargs=-Xmx3g \ +org.gradle.jvmargs=-Xmx4g \ --add-exports jdk.compiler/com.sun.tools.javac.api=ALL-UNNAMED \ --add-exports jdk.compiler/com.sun.tools.javac.file=ALL-UNNAMED \ --add-exports jdk.compiler/com.sun.tools.javac.main=ALL-UNNAMED \ @@ -22,5 +18,4 @@ org.gradle.jvmargs=-Xmx3g \ --add-opens jdk.compiler/com.sun.tools.javac.comp=ALL-UNNAMED \ --add-opens jdk.compiler/com.sun.tools.javac.parser=ALL-UNNAMED # Could be moved to sonar properties after https://sonarsource.atlassian.net/browse/SONARGRADL-134 -systemProp.sonar.gradle.skipCompile=true -#sonar.gradle.skipCompile=true +systemProp.sonar.gradle.skipCompile=true \ No newline at end of file From 62e4e24c804944432156fd560ecd06a81ca1cb2a Mon Sep 17 00:00:00 2001 From: amsmota Date: Fri, 20 Sep 2024 14:54:22 +0100 Subject: [PATCH 22/27] Fixed conflicts from merge, implemented suggested changes, reverted gradle props. Signed-off-by: amsmota --- .../test/java/org/hyperledger/besu/cli/CommandTestAbstract.java | 2 +- .../org/hyperledger/besu/config/JsonBftConfigOptionsTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java b/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java index fc09ebaba5d..65e51c92139 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/CommandTestAbstract.java @@ -149,7 +149,7 @@ public abstract class CommandTestAbstract { .put( "qbft", new JsonObject() - .put("emptyblockperiodseconds", POA_EMPTY_BLOCK_PERIOD_SECONDS))); + .put("xemptyblockperiodseconds", POA_EMPTY_BLOCK_PERIOD_SECONDS))); protected static final JsonObject VALID_GENESIS_IBFT2_POST_LONDON = (new JsonObject()) .put( diff --git a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java b/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java index db7d3957908..bbb4c4a6ceb 100644 --- a/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java +++ b/config/src/test/java/org/hyperledger/besu/config/JsonBftConfigOptionsTest.java @@ -104,7 +104,7 @@ public void shouldThrowOnNonPositiveBlockPeriod() { @Test public void shouldNotThrowOnNonPositiveEmptyBlockPeriod() { // can be 0 to be compatible with older versions - final BftConfigOptions config = fromConfigOptions(singletonMap("emptyblockperiodseconds", 0)); + final BftConfigOptions config = fromConfigOptions(singletonMap("xemptyblockperiodseconds", 0)); assertThatCode(() -> config.getEmptyBlockPeriodSeconds()).doesNotThrowAnyException(); } From 0b34185c1eb4047bce679b1a2589ae556dc54b6b Mon Sep 17 00:00:00 2001 From: amsmota Date: Fri, 20 Sep 2024 16:23:48 +0100 Subject: [PATCH 23/27] Fixed conflicts from merge, implemented suggested changes, reverted gradle props. Signed-off-by: amsmota --- .../besu/consensus/common/bft/BlockTimer.java | 48 ++++---- .../qbft/statemachine/QbftRound.java | 14 --- .../qbft/statemachine/QbftRoundTest.java | 105 ------------------ 3 files changed, 19 insertions(+), 148 deletions(-) diff --git a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java index 380aa034b5f..8b98a2ddec5 100644 --- a/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java +++ b/consensus/common/src/main/java/org/hyperledger/besu/consensus/common/bft/BlockTimer.java @@ -85,37 +85,27 @@ public synchronized boolean isRunning() { */ public synchronized void startTimer( final ConsensusRoundIdentifier round, final BlockHeader chainHeadHeader) { - // cancelTimer(); - // - // final long now = clock.millis(); - // final long expiryTime; + cancelTimer(); + + final long expiryTime; // Experimental option for test scenarios only. Not for production use. - // final long blockPeriodMilliseconds = - // - // forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodMilliseconds(); - // - // if (blockPeriodMilliseconds > 0) { - // // Experimental mode for setting < 1 second block periods e.g. for CI/CD pipelines - // // running tests against Besu - // expiryTime = clock.millis() + blockPeriodMilliseconds; - // LOG.warn( - // "Test-mode only xblockperiodmilliseconds has been set to {} millisecond blocks. Do - // not use in a production system.", - // blockPeriodMilliseconds); - // } else { - // // absolute time when the timer is supposed to expire - // final int blockPeriodSeconds = - // forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodSeconds(); - // final long minimumTimeBetweenBlocksMillis = blockPeriodSeconds * 1000L; - // expiryTime = chainHeadHeader.getTimestamp() * 1_000 + minimumTimeBetweenBlocksMillis; - // } - - // absolute time when the timer is supposed to expire - final int currentBlockPeriodSeconds = - forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodSeconds(); - final long minimumTimeBetweenBlocksMillis = currentBlockPeriodSeconds * 1000L; - final long expiryTime = chainHeadHeader.getTimestamp() * 1_000 + minimumTimeBetweenBlocksMillis; + final long blockPeriodMilliseconds = + forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodMilliseconds(); + if (blockPeriodMilliseconds > 0) { + // Experimental mode for setting < 1 second block periods e.g. for CI/CD pipelines + // running tests against Besu + expiryTime = clock.millis() + blockPeriodMilliseconds; + LOG.warn( + "Test-mode only xblockperiodmilliseconds has been set to {} millisecond blocks. Do not use in a production system.", + blockPeriodMilliseconds); + } else { + // absolute time when the timer is supposed to expire + final int currentBlockPeriodSeconds = + forksSchedule.getFork(round.getSequenceNumber()).getValue().getBlockPeriodSeconds(); + final long minimumTimeBetweenBlocksMillis = currentBlockPeriodSeconds * 1000L; + expiryTime = chainHeadHeader.getTimestamp() * 1_000 + minimumTimeBetweenBlocksMillis; + } setBlockTimes(round); diff --git a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java index e3f5f6257fa..e0a8cb1c726 100644 --- a/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java +++ b/consensus/qbft/src/main/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRound.java @@ -142,20 +142,6 @@ public Block createBlock(final long headerTimeStampSeconds) { return blockCreator.createBlock(headerTimeStampSeconds, this.parentHeader).getBlock(); } - /** - * Create and send proposal message. - * - * @param headerTimeStampSeconds the header time stamp seconds - */ -// public void createAndSendProposalMessage(final long headerTimeStampSeconds) { -// LOG.debug("Creating proposed block. round={}", roundState.getRoundIdentifier()); -// final Block block = -// blockCreator.createBlock(headerTimeStampSeconds, this.parentHeader).getBlock(); -// -// LOG.trace("Creating proposed block blockHeader={}", block.getHeader()); -// updateStateWithProposalAndTransmit(block, emptyList(), emptyList()); -// } - /** * Start round with. * diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundTest.java index 827779f6988..fb84c1c38fa 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftRoundTest.java @@ -194,91 +194,6 @@ public void onReceptionOfValidProposalSendsAPrepareToNetworkPeers() { verify(transmitter, never()).multicastCommit(any(), any(), any()); } - // @Test - // public void sendsAProposalAndPrepareWhenSendProposalRequested() { - // final RoundState roundState = new RoundState(roundIdentifier, 3, messageValidator); - // final QbftRound round = - // new QbftRound( - // roundState, - // blockCreator, - // protocolContext, - // protocolSchedule, - // subscribers, - // nodeKey, - // messageFactory, - // transmitter, - // roundTimer, - // bftExtraDataCodec, - // parentHeader); - // - // round.createAndSendProposalMessage(15); - // verify(transmitter, times(1)) - // .multicastProposal( - // roundIdentifier, proposedBlock, Collections.emptyList(), Collections.emptyList()); - // verify(transmitter, times(1)).multicastPrepare(roundIdentifier, proposedBlock.getHash()); - // verify(transmitter, never()).multicastCommit(any(), any(), any()); - // } - - // @Test - // public void singleValidatorImportBlocksImmediatelyOnProposalCreation() { - // final RoundState roundState = new RoundState(roundIdentifier, 1, messageValidator); - // final QbftRound round = - // new QbftRound( - // roundState, - // blockCreator, - // protocolContext, - // protocolSchedule, - // subscribers, - // nodeKey, - // messageFactory, - // transmitter, - // roundTimer, - // bftExtraDataCodec, - // parentHeader); - // round.createAndSendProposalMessage(15); - // verify(transmitter, times(1)) - // .multicastProposal( - // roundIdentifier, proposedBlock, Collections.emptyList(), Collections.emptyList()); - // verify(transmitter, times(1)).multicastPrepare(roundIdentifier, proposedBlock.getHash()); - // verify(transmitter, times(1)).multicastCommit(any(), any(), any()); - // } - - // @Test - // public void localNodeProposesToNetworkOfTwoValidatorsImportsOnReceptionOfCommitFromPeer() { - // final RoundState roundState = new RoundState(roundIdentifier, 2, messageValidator); - // final QbftRound round = - // new QbftRound( - // roundState, - // blockCreator, - // protocolContext, - // protocolSchedule, - // subscribers, - // nodeKey, - // messageFactory, - // transmitter, - // roundTimer, - // bftExtraDataCodec, - // parentHeader); - // - // final Hash commitSealHash = - // new BftBlockHashing(new QbftExtraDataCodec()) - // .calculateDataHashForCommittedSeal(proposedBlock.getHeader(), proposedExtraData); - // final SECPSignature localCommitSeal = nodeKey.sign(commitSealHash); - // - // round.createAndSendProposalMessage(15); - // verify(transmitter, never()).multicastCommit(any(), any(), any()); - // - // round.handlePrepareMessage( - // messageFactory2.createPrepare(roundIdentifier, proposedBlock.getHash())); - // - // verify(transmitter, times(1)) - // .multicastCommit(roundIdentifier, proposedBlock.getHash(), localCommitSeal); - // - // round.handleCommitMessage( - // messageFactory.createCommit(roundIdentifier, proposedBlock.getHash(), - // remoteCommitSeal)); - // } - @Test public void aProposalWithAnewBlockIsSentUponReceptionOfARoundChangeWithNoCertificate() { final RoundState roundState = new RoundState(roundIdentifier, 2, messageValidator); @@ -392,26 +307,6 @@ public void creatingNewBlockFromEmptyPreparedCertificateUpdatesInternalState() { assertThat(roundState.isPrepared()).isTrue(); } - // @Test - // public void creatingNewBlockNotifiesBlockMiningObservers() { - // final RoundState roundState = new RoundState(roundIdentifier, 1, messageValidator); - // final QbftRound round = - // new QbftRound( - // roundState, - // blockCreator, - // protocolContext, - // protocolSchedule, - // subscribers, - // nodeKey, - // messageFactory, - // transmitter, - // roundTimer, - // bftExtraDataCodec, - // parentHeader); - // round.createAndSendProposalMessage(15); - // verify(minedBlockObserver).blockMined(any()); - // } - @Test public void blockIsOnlyImportedOnceWhenCommitsAreReceivedBeforeProposal() { final ConsensusRoundIdentifier roundIdentifier = new ConsensusRoundIdentifier(1, 0); From ceaf0a22645596fbb8a57cd769f576d5109cb9f8 Mon Sep 17 00:00:00 2001 From: amsmota Date: Fri, 20 Sep 2024 19:25:13 +0100 Subject: [PATCH 24/27] Added entry in CHANGELOG Signed-off-by: amsmota --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 246df1d91cd..e4c9a710e1c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ ### Additions and Improvements - Remove privacy test classes support [#7569](https://github.com/hyperledger/besu/pull/7569) - Add Blob Transaction Metrics [#7622](https://github.com/hyperledger/besu/pull/7622) +- Implemented support for emptyBlockPeriodSeconds in QBFT [#6965](https://github.com/hyperledger/besu/pull/6965) + ### Bug fixes - Fix mounted data path directory permissions for besu user [#7575](https://github.com/hyperledger/besu/pull/7575) From 73c7451cb09645cb340d4d345bb26613295642e6 Mon Sep 17 00:00:00 2001 From: amsmota Date: Tue, 24 Sep 2024 10:49:08 +0100 Subject: [PATCH 25/27] Fixed QbftBlockHeightManagerTest Signed-off-by: amsmota --- .../qbft/statemachine/QbftBlockHeightManagerTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java index 01bec796788..484734a7c2e 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java @@ -160,6 +160,7 @@ public void setup() { when(finalState.getRoundTimer()).thenReturn(roundTimer); when(finalState.getQuorum()).thenReturn(3); when(finalState.getValidatorMulticaster()).thenReturn(validatorMulticaster); + when(finalState. getClock()).thenReturn(clock); when(blockCreator.createBlock(anyLong(), any())) .thenReturn( new BlockCreationResult( @@ -172,7 +173,7 @@ public void setup() { protocolContext = new ProtocolContext( - blockchain, + blockchain,QbftBlockHeightManagerTest null, setupContextWithBftExtraDataEncoder( BftContext.class, validators, new QbftExtraDataCodec()), From dd39cc29df963e9df2002c4b929c73912cd4fc72 Mon Sep 17 00:00:00 2001 From: amsmota Date: Tue, 24 Sep 2024 10:54:20 +0100 Subject: [PATCH 26/27] Fixed QbftBlockHeightManagerTest Signed-off-by: amsmota --- .../qbft/statemachine/QbftBlockHeightManagerTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java index 484734a7c2e..5b30426abc5 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java @@ -173,7 +173,7 @@ public void setup() { protocolContext = new ProtocolContext( - blockchain,QbftBlockHeightManagerTest + blockchain, null, setupContextWithBftExtraDataEncoder( BftContext.class, validators, new QbftExtraDataCodec()), @@ -293,6 +293,7 @@ public void onBlockTimerExpiryRoundTimerIsStartedAndProposalMessageIsTransmitted public void onBlockTimerExpiryForNonProposerRoundTimerIsStartedAndNoProposalMessageIsTransmitted() { when(finalState.isLocalNodeProposerForRound(roundIdentifier)).thenReturn(false); + when(blockTimer.checkEmptyBlockExpired(any(), eq(0l))).thenReturn(true); final QbftBlockHeightManager manager = new QbftBlockHeightManager( From fa8a27152c72a214d2681656c85780356e2fce4b Mon Sep 17 00:00:00 2001 From: amsmota Date: Tue, 24 Sep 2024 11:11:15 +0100 Subject: [PATCH 27/27] Fixed QbftBlockHeightManagerTest Signed-off-by: amsmota --- .../consensus/qbft/statemachine/QbftBlockHeightManagerTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java index 5b30426abc5..190c87e020f 100644 --- a/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java +++ b/consensus/qbft/src/test/java/org/hyperledger/besu/consensus/qbft/statemachine/QbftBlockHeightManagerTest.java @@ -160,7 +160,7 @@ public void setup() { when(finalState.getRoundTimer()).thenReturn(roundTimer); when(finalState.getQuorum()).thenReturn(3); when(finalState.getValidatorMulticaster()).thenReturn(validatorMulticaster); - when(finalState. getClock()).thenReturn(clock); + when(finalState.getClock()).thenReturn(clock); when(blockCreator.createBlock(anyLong(), any())) .thenReturn( new BlockCreationResult(