From 229150aa48220a327f49a5656e54c6d7f3e765af Mon Sep 17 00:00:00 2001 From: Antonio Mota Date: Thu, 18 Apr 2024 09:42:46 +0100 Subject: [PATCH] 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");