Skip to content
/ besu Public
forked from hyperledger/besu

Commit

Permalink
Implementing support for emptyBlockPeriodSeconds in QBFT (Issue hyper…
Browse files Browse the repository at this point in the history
…ledger#3810)

Signed-off-by: amsmota <[email protected]>
  • Loading branch information
amsmota committed Apr 16, 2024
1 parent 4bd2f5f commit beb2756
Show file tree
Hide file tree
Showing 11 changed files with 133 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ 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; // TODO: IS THIS APPLICABLE FOR POA?
protected static final JsonObject VALID_GENESIS_QBFT_POST_LONDON =
(new JsonObject())
.put(
Expand All @@ -144,7 +145,11 @@ public abstract class CommandTestAbstract {
.put("londonBlock", 0)
.put(
"qbft",
new JsonObject().put("blockperiodseconds", POA_BLOCK_PERIOD_SECONDS)));
new JsonObject().put("blockperiodseconds", POA_BLOCK_PERIOD_SECONDS)
.put(
"qbft",
new JsonObject().put("emptyblockperiodseconds", POA_EMPTY_BLOCK_PERIOD_SECONDS)
));
protected static final JsonObject VALID_GENESIS_IBFT2_POST_LONDON =
(new JsonObject())
.put(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,9 @@ public Map<String, Object> asMap() {
if (bftConfigRoot.has("blockperiodseconds")) {
builder.put("blockPeriodSeconds", getBlockPeriodSeconds());
}
if (bftConfigRoot.has("emptyblockperiodseconds")) {
builder.put("emptyblockperiodseconds", getBlockPeriodSeconds());
}
if (bftConfigRoot.has("requesttimeoutseconds")) {
builder.put("requestTimeoutSeconds", getRequestTimeoutSeconds());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -61,25 +63,51 @@ 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));
assertThatThrownBy(() -> config.getBlockPeriodSeconds())
.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));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ public class BftForksScheduleFactoryTest {
@SuppressWarnings("unchecked")
public void throwsErrorIfHasForkForGenesisBlock() {
final BftConfigOptions genesisConfigOptions = JsonBftConfigOptions.DEFAULT;
final BftFork fork = createFork(0, 10);
final BftFork fork = createFork(0, 10, 30);
final BftSpecCreator<BftConfigOptions, BftFork> specCreator =
Mockito.mock(BftSpecCreator.class);

Expand All @@ -51,9 +51,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 BftSpecCreator<BftConfigOptions, BftFork> specCreator =
Mockito.mock(BftSpecCreator.class);

Expand All @@ -69,13 +69,13 @@ public void throwsErrorIfHasForksWithDuplicateBlock() {
public void createsScheduleUsingSpecCreator() {
final BftConfigOptions genesisConfigOptions = JsonBftConfigOptions.DEFAULT;
final ForkSpec<BftConfigOptions> genesisForkSpec = new ForkSpec<>(0, genesisConfigOptions);
final BftFork fork1 = createFork(1, 10);
final BftFork fork2 = createFork(2, 20);
final BftFork fork1 = createFork(1, 10, 30);
final BftFork fork2 = createFork(2, 20, 60;
final BftSpecCreator<BftConfigOptions, BftFork> specCreator =
Mockito.mock(BftSpecCreator.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);

Expand All @@ -86,10 +86,11 @@ 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;
}

Expand All @@ -98,6 +99,7 @@ private BftFork createFork(final long block, final long blockPeriodSeconds) {
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)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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);

Expand All @@ -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 =
Expand Down Expand Up @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@ private record ControllerAndState(
private boolean useZeroBaseFee = false;
public static final int EPOCH_LENGTH = 10_000;
public static final int BLOCK_TIMER_SEC = 3;
public static final int EMPTY_BLOCK_TIMER_SEC = 30;
public static final int ROUND_TIMER_SEC = 12;
public static final int MESSAGE_QUEUE_LIMIT = 1000;
public static final int GOSSIPED_HISTORY_LIMIT = 100;
Expand Down Expand Up @@ -557,6 +558,7 @@ private static QbftConfigOptions createGenesisConfig(final boolean useValidatorC
final MutableQbftConfigOptions qbftConfigOptions =
new MutableQbftConfigOptions(JsonQbftConfigOptions.DEFAULT);
qbftConfigOptions.setBlockPeriodSeconds(BLOCK_TIMER_SEC);
qbftConfigOptions.setEmptyBlockPeriodSeconds(EMPTY_BLOCK_TIMER_SEC);
if (useValidatorContract) {
qbftConfigOptions.setValidatorContractAddress(
Optional.of(VALIDATOR_CONTRACT_ADDRESS.toHexString()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,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(createdBlock, new TransactionSelectionResults()));

Expand Down Expand Up @@ -567,4 +568,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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -78,6 +80,7 @@ public void createsScheduleWithForkThatOverridesGenesisValues() {

final Map<String, Object> 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");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ public PositiveNumber getPoaBlockTxsSelectionMaxTime() {

public abstract OptionalInt getGenesisBlockPeriodSeconds();

public abstract OptionalInt getGenesisEmptyBlockPeriodSeconds(); // TODO: WILL THIS BE NEEDED?

@Value.Derived
public long getBlockTxsSelectionMaxTime() {
if (getGenesisBlockPeriodSeconds().isPresent()) {
Expand Down

0 comments on commit beb2756

Please sign in to comment.