From 664bedca8ee75fb356723ada5a2ece2ac6e62828 Mon Sep 17 00:00:00 2001 From: Jason Frame Date: Fri, 3 Nov 2023 07:00:18 +1000 Subject: [PATCH] Add config option to clique to allow not creating empty blocks (#6082) Signed-off-by: Jason Frame (cherry picked from commit 6a2d41fccc00eac110a8cec27bc16cc2df47da0a) --- CHANGELOG.md | 1 + .../CliqueBesuControllerBuilder.java | 5 +- .../TransitionControllerBuilderTest.java | 2 +- .../besu/config/CliqueConfigOptions.java | 17 +- .../BlockHeaderValidationRulesetFactory.java | 15 +- .../clique/CliqueProtocolSchedule.java | 15 +- .../blockcreation/CliqueBlockMiner.java | 26 ++- .../blockcreation/CliqueMinerExecutor.java | 9 +- .../CliqueNoEmptyBlockValidationRule.java | 47 +++++ .../blockcreation/CliqueBlockMinerTest.java | 180 ++++++++++++++++++ .../CliqueMinerExecutorTest.java | 9 +- .../CliqueNoEmptyBlockValidationRuleTest.java | 57 ++++++ .../ethereum/blockcreation/BlockMiner.java | 8 + .../blockcreation/BlockMinerTest.java | 50 +++++ 14 files changed, 427 insertions(+), 14 deletions(-) create mode 100644 consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/headervalidationrules/CliqueNoEmptyBlockValidationRule.java create mode 100644 consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockMinerTest.java create mode 100644 consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/headervalidationrules/CliqueNoEmptyBlockValidationRuleTest.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 658ffe4cdeb..ae64fa6d951 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ - TraceService: return results for transactions in block [#6086](https://github.com/hyperledger/besu/pull/6086) - New option `--min-priority-fee` that sets the minimum priority fee a transaction must meet to be selected for a block. [#6080](https://github.com/hyperledger/besu/pull/6080) [#6083](https://github.com/hyperledger/besu/pull/6083) - Implement new `miner_setMinPriorityFee` and `miner_getMinPriorityFee` RPC methods [#6080](https://github.com/hyperledger/besu/pull/6080) +- Clique config option `createemptyblocks` to not create empty blocks [#6082](https://github.com/hyperledger/besu/pull/6082) ### Bug fixes diff --git a/besu/src/main/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilder.java b/besu/src/main/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilder.java index baad246a76b..555b3c8a2fb 100644 --- a/besu/src/main/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilder.java +++ b/besu/src/main/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilder.java @@ -53,6 +53,7 @@ public class CliqueBesuControllerBuilder extends BesuControllerBuilder { private Address localAddress; private EpochManager epochManager; private long secondsBetweenBlocks; + private boolean createEmptyBlocks = true; private final BlockInterface blockInterface = new CliqueBlockInterface(); @Override @@ -61,6 +62,7 @@ protected void prepForBuild() { final CliqueConfigOptions cliqueConfig = configOptionsSupplier.get().getCliqueConfigOptions(); final long blocksPerEpoch = cliqueConfig.getEpochLength(); secondsBetweenBlocks = cliqueConfig.getBlockPeriodSeconds(); + createEmptyBlocks = cliqueConfig.getCreateEmptyBlocks(); epochManager = new EpochManager(blocksPerEpoch); } @@ -91,7 +93,8 @@ protected MiningCoordinator createMiningCoordinator( protocolContext.getConsensusContext(CliqueContext.class).getValidatorProvider(), localAddress, secondsBetweenBlocks), - epochManager); + epochManager, + createEmptyBlocks); final CliqueMiningCoordinator miningCoordinator = new CliqueMiningCoordinator( protocolContext.getBlockchain(), diff --git a/besu/src/test/java/org/hyperledger/besu/controller/TransitionControllerBuilderTest.java b/besu/src/test/java/org/hyperledger/besu/controller/TransitionControllerBuilderTest.java index 0b1a98a210f..20a182e6ed6 100644 --- a/besu/src/test/java/org/hyperledger/besu/controller/TransitionControllerBuilderTest.java +++ b/besu/src/test/java/org/hyperledger/besu/controller/TransitionControllerBuilderTest.java @@ -199,7 +199,7 @@ public void assertPostMergeScheduleForPostMergeExactlyAtTerminalDifficultyIfNotF public void assertCliqueDetachedHeaderValidationPreMerge() { BlockHeaderValidator cliqueValidator = BlockHeaderValidationRulesetFactory.cliqueBlockHeaderValidator( - 5L, new EpochManager(5L), Optional.of(FeeMarket.london(1L)), true) + 5L, true, new EpochManager(5L), Optional.of(FeeMarket.london(1L)), true) .build(); assertDetachedRulesForPostMergeBlocks(cliqueValidator); } diff --git a/config/src/main/java/org/hyperledger/besu/config/CliqueConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/CliqueConfigOptions.java index 66acf03e558..c0ba496f892 100644 --- a/config/src/main/java/org/hyperledger/besu/config/CliqueConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/CliqueConfigOptions.java @@ -28,6 +28,7 @@ public class CliqueConfigOptions { private static final long DEFAULT_EPOCH_LENGTH = 30_000; private static final int DEFAULT_BLOCK_PERIOD_SECONDS = 15; + private static final boolean DEFAULT_CREATE_EMPTY_BLOCKS = true; private final ObjectNode cliqueConfigRoot; @@ -59,6 +60,15 @@ public int getBlockPeriodSeconds() { cliqueConfigRoot, "blockperiodseconds", DEFAULT_BLOCK_PERIOD_SECONDS); } + /** + * Whether the creation of empty blocks is allowed. + * + * @return the create empty block status + */ + public boolean getCreateEmptyBlocks() { + return JsonUtil.getBoolean(cliqueConfigRoot, "createemptyblocks", DEFAULT_CREATE_EMPTY_BLOCKS); + } + /** * As map. * @@ -66,6 +76,11 @@ public int getBlockPeriodSeconds() { */ Map asMap() { return ImmutableMap.of( - "epochLength", getEpochLength(), "blockPeriodSeconds", getBlockPeriodSeconds()); + "epochLength", + getEpochLength(), + "blockPeriodSeconds", + getBlockPeriodSeconds(), + "createemptyblocks", + getCreateEmptyBlocks()); } } diff --git a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/BlockHeaderValidationRulesetFactory.java b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/BlockHeaderValidationRulesetFactory.java index 271f6ea3e2f..15346fb4911 100644 --- a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/BlockHeaderValidationRulesetFactory.java +++ b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/BlockHeaderValidationRulesetFactory.java @@ -20,6 +20,7 @@ import org.hyperledger.besu.config.MergeConfigOptions; import org.hyperledger.besu.consensus.clique.headervalidationrules.CliqueDifficultyValidationRule; import org.hyperledger.besu.consensus.clique.headervalidationrules.CliqueExtraDataValidationRule; +import org.hyperledger.besu.consensus.clique.headervalidationrules.CliqueNoEmptyBlockValidationRule; import org.hyperledger.besu.consensus.clique.headervalidationrules.CoinbaseHeaderValidationRule; import org.hyperledger.besu.consensus.clique.headervalidationrules.SignerRateLimitValidationRule; import org.hyperledger.besu.consensus.clique.headervalidationrules.VoteValidationRule; @@ -51,22 +52,29 @@ public class BlockHeaderValidationRulesetFactory { *

Specifically the set of rules provided by this function are to be used for a Clique chain. * * @param secondsBetweenBlocks the minimum number of seconds which must elapse between blocks. + * @param createEmptyBlocks whether clique should allow the creation of empty blocks. * @param epochManager an object which determines if a given block is an epoch block. * @param baseFeeMarket an {@link Optional} wrapping {@link BaseFeeMarket} class if appropriate. * @return the header validator. */ public static BlockHeaderValidator.Builder cliqueBlockHeaderValidator( final long secondsBetweenBlocks, + final boolean createEmptyBlocks, final EpochManager epochManager, final Optional baseFeeMarket) { return cliqueBlockHeaderValidator( - secondsBetweenBlocks, epochManager, baseFeeMarket, MergeConfigOptions.isMergeEnabled()); + secondsBetweenBlocks, + createEmptyBlocks, + epochManager, + baseFeeMarket, + MergeConfigOptions.isMergeEnabled()); } /** * Clique block header validator. Visible for testing. * * @param secondsBetweenBlocks the seconds between blocks + * @param createEmptyBlocks whether clique should allow the creation of empty blocks. * @param epochManager the epoch manager * @param baseFeeMarket the base fee market * @param isMergeEnabled the is merge enabled @@ -75,6 +83,7 @@ public static BlockHeaderValidator.Builder cliqueBlockHeaderValidator( @VisibleForTesting public static BlockHeaderValidator.Builder cliqueBlockHeaderValidator( final long secondsBetweenBlocks, + final boolean createEmptyBlocks, final EpochManager epochManager, final Optional baseFeeMarket, final boolean isMergeEnabled) { @@ -99,6 +108,10 @@ public static BlockHeaderValidator.Builder cliqueBlockHeaderValidator( builder.addRule(new BaseFeeMarketBlockHeaderGasPriceValidationRule(baseFeeMarket.get())); } + if (!createEmptyBlocks) { + builder.addRule(new CliqueNoEmptyBlockValidationRule()); + } + var mixHashRule = new ConstantFieldValidationRule<>("MixHash", BlockHeader::getMixHash, Hash.ZERO); var voteValidationRule = new VoteValidationRule(); diff --git a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/CliqueProtocolSchedule.java b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/CliqueProtocolSchedule.java index 64bca913546..ef2c388cf7e 100644 --- a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/CliqueProtocolSchedule.java +++ b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/CliqueProtocolSchedule.java @@ -78,6 +78,7 @@ public static ProtocolSchedule create( applyCliqueSpecificModifications( epochManager, cliqueConfig.getBlockPeriodSeconds(), + cliqueConfig.getCreateEmptyBlocks(), localNodeAddress, builder)), privacyParameters, @@ -107,16 +108,19 @@ public static ProtocolSchedule create( private static ProtocolSpecBuilder applyCliqueSpecificModifications( final EpochManager epochManager, final long secondsBetweenBlocks, + final boolean createEmptyBlocks, final Address localNodeAddress, final ProtocolSpecBuilder specBuilder) { return specBuilder .blockHeaderValidatorBuilder( baseFeeMarket -> - getBlockHeaderValidator(epochManager, secondsBetweenBlocks, baseFeeMarket)) + getBlockHeaderValidator( + epochManager, secondsBetweenBlocks, createEmptyBlocks, baseFeeMarket)) .ommerHeaderValidatorBuilder( baseFeeMarket -> - getBlockHeaderValidator(epochManager, secondsBetweenBlocks, baseFeeMarket)) + getBlockHeaderValidator( + epochManager, secondsBetweenBlocks, createEmptyBlocks, baseFeeMarket)) .blockBodyValidatorBuilder(MainnetBlockBodyValidator::new) .blockValidatorBuilder(MainnetProtocolSpecs.blockValidatorBuilder()) .blockImporterBuilder(MainnetBlockImporter::new) @@ -128,11 +132,14 @@ private static ProtocolSpecBuilder applyCliqueSpecificModifications( } private static BlockHeaderValidator.Builder getBlockHeaderValidator( - final EpochManager epochManager, final long secondsBetweenBlocks, final FeeMarket feeMarket) { + final EpochManager epochManager, + final long secondsBetweenBlocks, + final boolean createEmptyBlocks, + final FeeMarket feeMarket) { Optional baseFeeMarket = Optional.of(feeMarket).filter(FeeMarket::implementsBaseFee).map(BaseFeeMarket.class::cast); return BlockHeaderValidationRulesetFactory.cliqueBlockHeaderValidator( - secondsBetweenBlocks, epochManager, baseFeeMarket); + secondsBetweenBlocks, createEmptyBlocks, epochManager, baseFeeMarket); } } diff --git a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockMiner.java b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockMiner.java index d9e4c6f38f1..698ff77eccb 100644 --- a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockMiner.java +++ b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockMiner.java @@ -20,16 +20,23 @@ import org.hyperledger.besu.ethereum.blockcreation.AbstractBlockScheduler; import org.hyperledger.besu.ethereum.blockcreation.BlockMiner; import org.hyperledger.besu.ethereum.chain.MinedBlockObserver; +import org.hyperledger.besu.ethereum.core.Block; import org.hyperledger.besu.ethereum.core.BlockHeader; import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; import org.hyperledger.besu.util.Subscribers; import java.util.function.Function; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + /** The Clique block miner. */ public class CliqueBlockMiner extends BlockMiner { + private static final Logger LOG = LoggerFactory.getLogger(CliqueBlockMiner.class); + private static final int WAIT_IN_MS_BETWEEN_EMPTY_BUILD_ATTEMPTS = 1_000; private final Address localAddress; + private final boolean createEmptyBlocks; /** * Instantiates a new Clique block miner. @@ -41,6 +48,7 @@ public class CliqueBlockMiner extends BlockMiner { * @param scheduler the scheduler * @param parentHeader the parent header * @param localAddress the local address + * @param createEmptyBlocks whether clique should allow the creation of empty blocks. */ public CliqueBlockMiner( final Function blockCreator, @@ -49,9 +57,11 @@ public CliqueBlockMiner( final Subscribers observers, final AbstractBlockScheduler scheduler, final BlockHeader parentHeader, - final Address localAddress) { + final Address localAddress, + final boolean createEmptyBlocks) { super(blockCreator, protocolSchedule, protocolContext, observers, scheduler, parentHeader); this.localAddress = localAddress; + this.createEmptyBlocks = createEmptyBlocks; } @Override @@ -63,4 +73,18 @@ protected boolean mineBlock() throws InterruptedException { return true; // terminate mining. } + + @Override + protected boolean shouldImportBlock(final Block block) throws InterruptedException { + if (createEmptyBlocks) { + return true; + } + + final boolean isEmpty = block.getBody().getTransactions().isEmpty(); + if (isEmpty) { + LOG.debug("Skipping creating empty block {}", block.toLogString()); + Thread.sleep(WAIT_IN_MS_BETWEEN_EMPTY_BUILD_ATTEMPTS); + } + return !isEmpty; + } } diff --git a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutor.java b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutor.java index 81b754b267c..060e262dd22 100644 --- a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutor.java +++ b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutor.java @@ -47,6 +47,7 @@ public class CliqueMinerExecutor extends AbstractMinerExecutor private final Address localAddress; private final NodeKey nodeKey; private final EpochManager epochManager; + private final boolean createEmptyBlocks; /** * Instantiates a new Clique miner executor. @@ -58,6 +59,7 @@ public class CliqueMinerExecutor extends AbstractMinerExecutor * @param miningParams the mining params * @param blockScheduler the block scheduler * @param epochManager the epoch manager + * @param createEmptyBlocks whether clique should allow the creation of empty blocks. */ public CliqueMinerExecutor( final ProtocolContext protocolContext, @@ -66,11 +68,13 @@ public CliqueMinerExecutor( final NodeKey nodeKey, final MiningParameters miningParams, final AbstractBlockScheduler blockScheduler, - final EpochManager epochManager) { + final EpochManager epochManager, + final boolean createEmptyBlocks) { super(protocolContext, protocolSchedule, transactionPool, miningParams, blockScheduler); this.nodeKey = nodeKey; this.localAddress = Util.publicKeyToAddress(nodeKey.getPublicKey()); this.epochManager = epochManager; + this.createEmptyBlocks = createEmptyBlocks; miningParams.setCoinbase(localAddress); } @@ -98,7 +102,8 @@ public CliqueBlockMiner createMiner( observers, blockScheduler, parentHeader, - localAddress); + localAddress, + createEmptyBlocks); } @Override diff --git a/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/headervalidationrules/CliqueNoEmptyBlockValidationRule.java b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/headervalidationrules/CliqueNoEmptyBlockValidationRule.java new file mode 100644 index 00000000000..c179a08495a --- /dev/null +++ b/consensus/clique/src/main/java/org/hyperledger/besu/consensus/clique/headervalidationrules/CliqueNoEmptyBlockValidationRule.java @@ -0,0 +1,47 @@ +/* + * Copyright ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ +package org.hyperledger.besu.consensus.clique.headervalidationrules; + +import org.hyperledger.besu.datatypes.Hash; +import org.hyperledger.besu.ethereum.core.BlockHeader; +import org.hyperledger.besu.ethereum.mainnet.DetachedBlockHeaderValidationRule; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** The No empty block validation rule. */ +public class CliqueNoEmptyBlockValidationRule implements DetachedBlockHeaderValidationRule { + + private static final Logger LOG = LoggerFactory.getLogger(CliqueNoEmptyBlockValidationRule.class); + + /** + * Responsible for ensuring there are no empty transactions. This is used when createEmptyBlocks + * is false, to ensure that no empty blocks are created. + * + * @param header the block header to validate + * @param parent the block header corresponding to the parent of the header being validated. + * @return true if the transactionsRoot in the header is not the empty trie hash. + */ + @Override + public boolean validate(final BlockHeader header, final BlockHeader parent) { + final boolean hasTransactions = !header.getTransactionsRoot().equals(Hash.EMPTY_TRIE_HASH); + if (!hasTransactions) { + LOG.info( + "Invalid block header: {} has no transactions but create empty blocks is not enabled", + header.toLogString()); + } + return hasTransactions; + } +} diff --git a/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockMinerTest.java b/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockMinerTest.java new file mode 100644 index 00000000000..ed1cfced5e3 --- /dev/null +++ b/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueBlockMinerTest.java @@ -0,0 +1,180 @@ +/* + * Copyright Hyperledger Besu Contributors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.hyperledger.besu.consensus.clique.blockcreation; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import org.hyperledger.besu.consensus.clique.CliqueContext; +import org.hyperledger.besu.consensus.common.validator.ValidatorProvider; +import org.hyperledger.besu.crypto.KeyPair; +import org.hyperledger.besu.crypto.SignatureAlgorithmFactory; +import org.hyperledger.besu.datatypes.Address; +import org.hyperledger.besu.ethereum.ProtocolContext; +import org.hyperledger.besu.ethereum.blockcreation.BlockCreator; +import org.hyperledger.besu.ethereum.blockcreation.DefaultBlockScheduler; +import org.hyperledger.besu.ethereum.blockcreation.txselection.TransactionSelectionResults; +import org.hyperledger.besu.ethereum.chain.MinedBlockObserver; +import org.hyperledger.besu.ethereum.core.Block; +import org.hyperledger.besu.ethereum.core.BlockBody; +import org.hyperledger.besu.ethereum.core.BlockHeader; +import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture; +import org.hyperledger.besu.ethereum.core.BlockImporter; +import org.hyperledger.besu.ethereum.core.Transaction; +import org.hyperledger.besu.ethereum.core.TransactionTestFixture; +import org.hyperledger.besu.ethereum.mainnet.BlockImportResult; +import org.hyperledger.besu.ethereum.mainnet.DefaultProtocolSchedule; +import org.hyperledger.besu.ethereum.mainnet.HeaderValidationMode; +import org.hyperledger.besu.ethereum.mainnet.ProtocolSchedule; +import org.hyperledger.besu.ethereum.mainnet.ProtocolSpec; +import org.hyperledger.besu.util.Subscribers; + +import java.math.BigInteger; +import java.util.List; +import java.util.Optional; +import java.util.function.Function; + +import com.google.common.collect.Lists; +import org.junit.jupiter.api.Test; + +class CliqueBlockMinerTest { + + @Test + void doesNotMineBlockIfNoTransactionsWhenEmptyBlocksNotAllowed() throws InterruptedException { + final BlockHeaderTestFixture headerBuilder = new BlockHeaderTestFixture(); + + final Block blockToCreate = + new Block( + headerBuilder.buildHeader(), new BlockBody(Lists.newArrayList(), Lists.newArrayList())); + + final ValidatorProvider validatorProvider = mock(ValidatorProvider.class); + when(validatorProvider.getValidatorsAfterBlock(any())).thenReturn(List.of(Address.ZERO)); + + final CliqueContext cliqueContext = new CliqueContext(validatorProvider, null, null); + final ProtocolContext protocolContext = + new ProtocolContext(null, null, cliqueContext, Optional.empty()); + + final CliqueBlockCreator blockCreator = mock(CliqueBlockCreator.class); + final Function blockCreatorSupplier = + (parentHeader) -> blockCreator; + when(blockCreator.createBlock(anyLong())) + .thenReturn( + new BlockCreator.BlockCreationResult(blockToCreate, new TransactionSelectionResults())); + + final BlockImporter blockImporter = mock(BlockImporter.class); + final ProtocolSpec protocolSpec = mock(ProtocolSpec.class); + + final ProtocolSchedule protocolSchedule = singleSpecSchedule(protocolSpec); + + when(protocolSpec.getBlockImporter()).thenReturn(blockImporter); + when(blockImporter.importBlock(any(), any(), any())).thenReturn(new BlockImportResult(true)); + + final MinedBlockObserver observer = mock(MinedBlockObserver.class); + final DefaultBlockScheduler scheduler = mock(DefaultBlockScheduler.class); + when(scheduler.waitUntilNextBlockCanBeMined(any())).thenReturn(5L); + final CliqueBlockMiner miner = + new CliqueBlockMiner( + blockCreatorSupplier, + protocolSchedule, + protocolContext, + subscribersContaining(observer), + scheduler, + headerBuilder.buildHeader(), + Address.ZERO, + false); // parent header is arbitrary for the test. + + final boolean result = miner.mineBlock(); + assertThat(result).isFalse(); + verify(blockImporter, never()) + .importBlock(protocolContext, blockToCreate, HeaderValidationMode.FULL); + verify(observer, never()).blockMined(blockToCreate); + } + + @Test + void minesBlockIfHasTransactionsWhenEmptyBlocksNotAllowed() throws InterruptedException { + final BlockHeaderTestFixture headerBuilder = new BlockHeaderTestFixture(); + + final TransactionTestFixture transactionTestFixture = new TransactionTestFixture(); + final KeyPair keyPair = SignatureAlgorithmFactory.getInstance().generateKeyPair(); + final Transaction transaction = transactionTestFixture.createTransaction(keyPair); + + final Block blockToCreate = + new Block( + headerBuilder.buildHeader(), new BlockBody(List.of(transaction), Lists.newArrayList())); + + final ValidatorProvider validatorProvider = mock(ValidatorProvider.class); + when(validatorProvider.getValidatorsAfterBlock(any())).thenReturn(List.of(Address.ZERO)); + + final CliqueContext cliqueContext = new CliqueContext(validatorProvider, null, null); + final ProtocolContext protocolContext = + new ProtocolContext(null, null, cliqueContext, Optional.empty()); + + final CliqueBlockCreator blockCreator = mock(CliqueBlockCreator.class); + final Function blockCreatorSupplier = + (parentHeader) -> blockCreator; + when(blockCreator.createBlock(anyLong())) + .thenReturn( + new BlockCreator.BlockCreationResult(blockToCreate, new TransactionSelectionResults())); + + final BlockImporter blockImporter = mock(BlockImporter.class); + final ProtocolSpec protocolSpec = mock(ProtocolSpec.class); + + final ProtocolSchedule protocolSchedule = singleSpecSchedule(protocolSpec); + + when(protocolSpec.getBlockImporter()).thenReturn(blockImporter); + when(blockImporter.importBlock(any(), any(), any())).thenReturn(new BlockImportResult(true)); + + final MinedBlockObserver observer = mock(MinedBlockObserver.class); + final DefaultBlockScheduler scheduler = mock(DefaultBlockScheduler.class); + when(scheduler.waitUntilNextBlockCanBeMined(any())).thenReturn(5L); + final CliqueBlockMiner miner = + new CliqueBlockMiner( + blockCreatorSupplier, + protocolSchedule, + protocolContext, + subscribersContaining(observer), + scheduler, + headerBuilder.buildHeader(), + Address.ZERO, + false); // parent header is arbitrary for the test. + + final boolean result = miner.mineBlock(); + assertThat(result).isTrue(); + verify(blockImporter).importBlock(protocolContext, blockToCreate, HeaderValidationMode.FULL); + verify(observer).blockMined(blockToCreate); + } + + private static Subscribers subscribersContaining( + final MinedBlockObserver... observers) { + final Subscribers result = Subscribers.create(); + for (final MinedBlockObserver obs : observers) { + result.subscribe(obs); + } + return result; + } + + private ProtocolSchedule singleSpecSchedule(final ProtocolSpec protocolSpec) { + final DefaultProtocolSchedule protocolSchedule = + new DefaultProtocolSchedule(Optional.of(BigInteger.valueOf(1234))); + protocolSchedule.putBlockNumberMilestone(0, protocolSpec); + return protocolSchedule; + } +} diff --git a/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutorTest.java b/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutorTest.java index 7a089f2ef37..740962e2a41 100644 --- a/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutorTest.java +++ b/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/blockcreation/CliqueMinerExecutorTest.java @@ -113,7 +113,8 @@ public void extraDataCreatedOnEpochBlocksContainsValidators() { proposerNodeKey, miningParameters, mock(CliqueBlockScheduler.class), - new EpochManager(EPOCH_LENGTH)); + new EpochManager(EPOCH_LENGTH), + true); // NOTE: Passing in the *parent* block, so must be 1 less than EPOCH final BlockHeader header = blockHeaderBuilder.number(EPOCH_LENGTH - 1).buildHeader(); @@ -147,7 +148,8 @@ public void extraDataForNonEpochBlocksDoesNotContainValidaors() { proposerNodeKey, miningParameters, mock(CliqueBlockScheduler.class), - new EpochManager(EPOCH_LENGTH)); + new EpochManager(EPOCH_LENGTH), + true); // Parent block was epoch, so the next block should contain no validators. final BlockHeader header = blockHeaderBuilder.number(EPOCH_LENGTH).buildHeader(); @@ -181,7 +183,8 @@ public void shouldUseLatestVanityData() { proposerNodeKey, miningParameters, mock(CliqueBlockScheduler.class), - new EpochManager(EPOCH_LENGTH)); + new EpochManager(EPOCH_LENGTH), + true); executor.setExtraData(modifiedVanityData); final Bytes extraDataBytes = executor.calculateExtraData(blockHeaderBuilder.buildHeader()); diff --git a/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/headervalidationrules/CliqueNoEmptyBlockValidationRuleTest.java b/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/headervalidationrules/CliqueNoEmptyBlockValidationRuleTest.java new file mode 100644 index 00000000000..807aef0b1fc --- /dev/null +++ b/consensus/clique/src/test/java/org/hyperledger/besu/consensus/clique/headervalidationrules/CliqueNoEmptyBlockValidationRuleTest.java @@ -0,0 +1,57 @@ +/* + * Copyright Hyperledger Besu Contributors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.hyperledger.besu.consensus.clique.headervalidationrules; + +import static org.assertj.core.api.Assertions.assertThat; + +import org.hyperledger.besu.crypto.KeyPair; +import org.hyperledger.besu.crypto.SignatureAlgorithmFactory; +import org.hyperledger.besu.datatypes.Hash; +import org.hyperledger.besu.ethereum.core.BlockHeader; +import org.hyperledger.besu.ethereum.core.BlockHeaderTestFixture; +import org.hyperledger.besu.ethereum.core.Transaction; +import org.hyperledger.besu.ethereum.core.TransactionTestFixture; +import org.hyperledger.besu.ethereum.mainnet.BodyValidation; + +import java.util.List; + +import org.junit.jupiter.api.Test; + +class CliqueNoEmptyBlockValidationRuleTest { + + @Test + void headerWithNoTransactionsIsInvalid() { + final BlockHeader blockHeader = new BlockHeaderTestFixture().buildHeader(); + + final CliqueNoEmptyBlockValidationRule noEmptyBlockRule = + new CliqueNoEmptyBlockValidationRule(); + assertThat(noEmptyBlockRule.validate(blockHeader, null)).isFalse(); + } + + @Test + void headerWithTransactionsIsValid() { + final TransactionTestFixture transactionTestFixture = new TransactionTestFixture(); + final KeyPair keyPair = SignatureAlgorithmFactory.getInstance().generateKeyPair(); + final Transaction transaction = transactionTestFixture.createTransaction(keyPair); + final Hash transactionRoot = BodyValidation.transactionsRoot(List.of(transaction)); + final BlockHeader blockHeader = + new BlockHeaderTestFixture().transactionsRoot(transactionRoot).buildHeader(); + + final CliqueNoEmptyBlockValidationRule noEmptyBlockRule = + new CliqueNoEmptyBlockValidationRule(); + assertThat(noEmptyBlockRule.validate(blockHeader, null)).isTrue(); + } +} diff --git a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockMiner.java b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockMiner.java index ccbfb52ab6a..34ac04bf880 100644 --- a/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockMiner.java +++ b/ethereum/blockcreation/src/main/java/org/hyperledger/besu/ethereum/blockcreation/BlockMiner.java @@ -126,6 +126,10 @@ public BlockCreationResult createBlock(final BlockHeader parentHeader, final lon return blockCreator.createBlock(Optional.empty(), Optional.empty(), timestamp); } + protected boolean shouldImportBlock(final Block block) throws InterruptedException { + return true; + } + protected boolean mineBlock() throws InterruptedException { // Ensure the block is allowed to be mined - i.e. the timestamp on the new block is sufficiently // ahead of the parent, and still within allowable clock tolerance. @@ -140,6 +144,10 @@ protected boolean mineBlock() throws InterruptedException { "Block created, importing to local chain, block includes {} transactions", block.getBody().getTransactions().size()); + if (!shouldImportBlock(block)) { + return false; + } + final BlockImporter importer = protocolSchedule.getByBlockHeader(block.getHeader()).getBlockImporter(); final BlockImportResult blockImportResult = diff --git a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockMinerTest.java b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockMinerTest.java index 4222ad476c0..681991ee75c 100644 --- a/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockMinerTest.java +++ b/ethereum/blockcreation/src/test/java/org/hyperledger/besu/ethereum/blockcreation/BlockMinerTest.java @@ -14,6 +14,7 @@ */ package org.hyperledger.besu.ethereum.blockcreation; +import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; import static org.mockito.Mockito.mock; @@ -39,6 +40,7 @@ import java.math.BigInteger; import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; import com.google.common.collect.Lists; @@ -132,6 +134,54 @@ public void failureToImportDoesNotTriggerObservers() throws InterruptedException verify(observer, times(1)).blockMined(blockToCreate); } + @Test + public void blockValidationFailureBeforeImportDoesNotImportBlock() throws InterruptedException { + final BlockHeaderTestFixture headerBuilder = new BlockHeaderTestFixture(); + + final Block blockToCreate = + new Block( + headerBuilder.buildHeader(), new BlockBody(Lists.newArrayList(), Lists.newArrayList())); + + final ProtocolContext protocolContext = new ProtocolContext(null, null, null, Optional.empty()); + + final PoWBlockCreator blockCreator = mock(PoWBlockCreator.class); + final Function blockCreatorSupplier = + (parentHeader) -> blockCreator; + when(blockCreator.createBlock(anyLong())) + .thenReturn(new BlockCreationResult(blockToCreate, new TransactionSelectionResults())); + + final BlockImporter blockImporter = mock(BlockImporter.class); + final ProtocolSpec protocolSpec = mock(ProtocolSpec.class); + final ProtocolSchedule protocolSchedule = singleSpecSchedule(protocolSpec); + + when(protocolSpec.getBlockImporter()).thenReturn(blockImporter); + when(blockImporter.importBlock(any(), any(), any())).thenReturn(new BlockImportResult(true)); + + final MinedBlockObserver observer = mock(MinedBlockObserver.class); + final DefaultBlockScheduler scheduler = mock(DefaultBlockScheduler.class); + when(scheduler.waitUntilNextBlockCanBeMined(any())).thenReturn(5L); + final AtomicInteger importValidationCount = new AtomicInteger(); + final BlockMiner miner = + new BlockMiner<>( + blockCreatorSupplier, + protocolSchedule, + protocolContext, + subscribersContaining(observer), + scheduler, + headerBuilder.buildHeader()) { + @Override + protected boolean shouldImportBlock(final Block block) { + return importValidationCount.getAndIncrement() > 0; + } + }; + + miner.run(); + assertThat(importValidationCount.get()).isEqualTo(2); + verify(blockImporter, times(1)) + .importBlock(protocolContext, blockToCreate, HeaderValidationMode.FULL); + verify(observer, times(1)).blockMined(blockToCreate); + } + private static Subscribers subscribersContaining( final MinedBlockObserver... observers) { final Subscribers result = Subscribers.create();