From 61c779f1edd0bd8d13ea04a702f305475fb5893b Mon Sep 17 00:00:00 2001 From: Matt Whitehead Date: Tue, 11 Jul 2023 08:59:14 +0100 Subject: [PATCH] Only validate `--miner-enabled` option for ethash networks (#5669) * Modify the min-gas-price option validation * Check for whether ethash is in use, either from genesis or network config, and use that for miner checks * Add genesis configuration isPoa() convenience function --------- Signed-off-by: Matthew Whitehead Signed-off-by: Matt Whitehead Co-authored-by: Simon Dudley Signed-off-by: Simon Dudley --- CHANGELOG.md | 1 + .../org/hyperledger/besu/cli/BesuCommand.java | 23 ++--- .../hyperledger/besu/cli/BesuCommandTest.java | 85 ++++++++++++++++++- .../besu/config/GenesisConfigOptions.java | 7 ++ .../besu/config/JsonGenesisConfigOptions.java | 5 ++ .../besu/config/StubGenesisConfigOptions.java | 5 ++ .../besu/config/GenesisConfigOptionsTest.java | 11 +++ 7 files changed, 124 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87b5be8d08d..797ab5f5e93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ - Use the node's configuration to determine if DNS enode URLs are allowed in calls to `admin_addPeer` and `admin_removePeer` [#5584](https://github.com/hyperledger/besu/pull/5584) - Align the implementation of Eth/68 `NewPooledTransactionHashes` to other clients, using unsigned int for encoding size. [#5640](https://github.com/hyperledger/besu/pull/5640) - Failure at startup when enabling layered txpool before initial sync done [#5636](https://github.com/hyperledger/besu/issues/5636) +- Remove miner-related option warnings if the change isn't using Ethash consensus algorithm [#5669](https://github.com/hyperledger/besu/pull/5669) ### Download Links diff --git a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java index 330393a5bae..831c52a74f4 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java @@ -2133,7 +2133,7 @@ private void issueOptionWarnings() { "--remote-connections-max-percentage")); // Check that block producer options work - if (!isMergeEnabled()) { + if (!isMergeEnabled() && getActualGenesisConfigOptions().isEthHash()) { CommandLineUtils.checkOptionDependencies( logger, commandLine, @@ -2144,17 +2144,18 @@ private void issueOptionWarnings() { "--min-gas-price", "--min-block-occupancy-ratio", "--miner-extra-data")); + + // Check that mining options are able to work + CommandLineUtils.checkOptionDependencies( + logger, + commandLine, + "--miner-enabled", + !minerOptionGroup.isMiningEnabled, + asList( + "--miner-stratum-enabled", + "--Xminer-remote-sealers-limit", + "--Xminer-remote-sealers-hashrate-ttl")); } - // Check that mining options are able to work - CommandLineUtils.checkOptionDependencies( - logger, - commandLine, - "--miner-enabled", - !minerOptionGroup.isMiningEnabled, - asList( - "--miner-stratum-enabled", - "--Xminer-remote-sealers-limit", - "--Xminer-remote-sealers-hashrate-ttl")); CommandLineUtils.failIfOptionDoesntMeetRequirement( commandLine, diff --git a/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java b/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java index de87890b169..7e634282260 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java @@ -168,6 +168,20 @@ public class BesuCommandTest extends CommandTestAbstract { (new JsonObject()).put("config", new JsonObject().put("ecCurve", "secp256k1")); private static final String ENCLAVE_PUBLIC_KEY_PATH = BesuCommand.class.getResource("/orion_publickey.pub").getPath(); + private static final JsonObject VALID_GENESIS_QBFT_POST_LONDON = + (new JsonObject()) + .put( + "config", + new JsonObject() + .put("londonBlock", 0) + .put("qbft", new JsonObject().put("blockperiodseconds", 5))); + private static final JsonObject VALID_GENESIS_IBFT2_POST_LONDON = + (new JsonObject()) + .put( + "config", + new JsonObject() + .put("londonBlock", 0) + .put("ibft2", new JsonObject().put("blockperiodseconds", 5))); private static final String[] VALID_ENODE_STRINGS = { "enode://" + VALID_NODE_ID + "@192.168.0.1:4567", @@ -3684,7 +3698,7 @@ public void stratumMiningIsEnabledWhenSpecified() throws Exception { @Test public void stratumMiningOptionsRequiresServiceToBeEnabled() { - parseCommand("--miner-stratum-enabled"); + parseCommand("--network", "dev", "--miner-stratum-enabled"); verifyOptionsConstraintLoggerCall("--miner-enabled", "--miner-stratum-enabled"); @@ -3698,7 +3712,7 @@ public void stratumMiningOptionsRequiresServiceToBeEnabled() { public void stratumMiningOptionsRequiresServiceToBeEnabledToml() throws IOException { final Path toml = createTempFile("toml", "miner-stratum-enabled=true\n"); - parseCommand("--config-file", toml.toString()); + parseCommand("--network", "dev", "--config-file", toml.toString()); verifyOptionsConstraintLoggerCall("--miner-enabled", "--miner-stratum-enabled"); @@ -3753,6 +3767,46 @@ public void blockProducingOptionsWarnsMinerShouldBeEnabledToml() throws IOExcept assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); } + @Test + public void blockProducingOptionsDoNotWarnWhenPoA() throws IOException { + + final Path genesisFileQBFT = createFakeGenesisFile(VALID_GENESIS_QBFT_POST_LONDON); + parseCommand( + "--genesis-file", + genesisFileQBFT.toString(), + "--min-gas-price", + "42", + "--miner-extra-data", + "0x1122334455667788990011223344556677889900112233445566778899001122"); + + verify(mockLogger, atMost(0)) + .warn( + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture()); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + + final Path genesisFileIBFT2 = createFakeGenesisFile(VALID_GENESIS_IBFT2_POST_LONDON); + parseCommand( + "--genesis-file", + genesisFileIBFT2.toString(), + "--min-gas-price", + "42", + "--miner-extra-data", + "0x1122334455667788990011223344556677889900112233445566778899001122"); + + verify(mockLogger, atMost(0)) + .warn( + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture()); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + } + @Test public void blockProducingOptionsDoNotWarnWhenMergeEnabled() { @@ -3801,6 +3855,33 @@ public void minGasPriceRequiresMainOptionToml() throws IOException { assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); } + @Test + public void minGasPriceDoesNotRequireMainOptionWhenPoA() throws IOException { + final Path genesisFileQBFT = createFakeGenesisFile(VALID_GENESIS_QBFT_POST_LONDON); + parseCommand("--genesis-file", genesisFileQBFT.toString(), "--min-gas-price", "0"); + + verify(mockLogger, atMost(0)) + .warn( + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture()); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + + final Path genesisFileIBFT2 = createFakeGenesisFile(VALID_GENESIS_IBFT2_POST_LONDON); + parseCommand("--genesis-file", genesisFileIBFT2.toString(), "--min-gas-price", "0"); + + verify(mockLogger, atMost(0)) + .warn( + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture(), + stringArgumentCaptor.capture()); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + } + @Test public void miningParametersAreCaptured() { final Address requestedCoinbase = Address.fromHexString("0000011111222223333344444"); diff --git a/config/src/main/java/org/hyperledger/besu/config/GenesisConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/GenesisConfigOptions.java index 41f385fc4f5..ba1f52769e8 100644 --- a/config/src/main/java/org/hyperledger/besu/config/GenesisConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/GenesisConfigOptions.java @@ -65,6 +65,13 @@ public interface GenesisConfigOptions { */ boolean isClique(); + /** + * Is a Proof of Authority network. + * + * @return the boolean + */ + boolean isPoa(); + /** * Is consensus migration boolean. * diff --git a/config/src/main/java/org/hyperledger/besu/config/JsonGenesisConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/JsonGenesisConfigOptions.java index f9cf0696290..ab269142ba2 100644 --- a/config/src/main/java/org/hyperledger/besu/config/JsonGenesisConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/JsonGenesisConfigOptions.java @@ -146,6 +146,11 @@ public boolean isQbft() { return configRoot.has(QBFT_CONFIG_KEY); } + @Override + public boolean isPoa() { + return isQbft() || isClique() || isIbft2() || isIbftLegacy(); + } + @Override public BftConfigOptions getBftConfigOptions() { final String fieldKey = isIbft2() ? IBFT2_CONFIG_KEY : QBFT_CONFIG_KEY; diff --git a/config/src/main/java/org/hyperledger/besu/config/StubGenesisConfigOptions.java b/config/src/main/java/org/hyperledger/besu/config/StubGenesisConfigOptions.java index 949205e9b04..a0eac55cd8a 100644 --- a/config/src/main/java/org/hyperledger/besu/config/StubGenesisConfigOptions.java +++ b/config/src/main/java/org/hyperledger/besu/config/StubGenesisConfigOptions.java @@ -116,6 +116,11 @@ public boolean isQbft() { return false; } + @Override + public boolean isPoa() { + return false; + } + @Override public CheckpointConfigOptions getCheckpointOptions() { return CheckpointConfigOptions.DEFAULT; diff --git a/config/src/test/java/org/hyperledger/besu/config/GenesisConfigOptionsTest.java b/config/src/test/java/org/hyperledger/besu/config/GenesisConfigOptionsTest.java index 158d9734a4e..d04027dcc10 100644 --- a/config/src/test/java/org/hyperledger/besu/config/GenesisConfigOptionsTest.java +++ b/config/src/test/java/org/hyperledger/besu/config/GenesisConfigOptionsTest.java @@ -48,13 +48,22 @@ public void shouldNotUseEthHashIfEthHashNotPresent() { public void shouldUseIbft2WhenIbft2InConfig() { final GenesisConfigOptions config = fromConfigOptions(singletonMap("ibft2", emptyMap())); assertThat(config.isIbft2()).isTrue(); + assertThat(config.isPoa()).isTrue(); assertThat(config.getConsensusEngine()).isEqualTo("ibft2"); } + public void shouldUseQbftWhenQbftInConfig() { + final GenesisConfigOptions config = fromConfigOptions(singletonMap("qbft", emptyMap())); + assertThat(config.isQbft()).isTrue(); + assertThat(config.isPoa()).isTrue(); + assertThat(config.getConsensusEngine()).isEqualTo("qbft"); + } + @Test public void shouldUseCliqueWhenCliqueInConfig() { final GenesisConfigOptions config = fromConfigOptions(singletonMap("clique", emptyMap())); assertThat(config.isClique()).isTrue(); + assertThat(config.isPoa()).isTrue(); assertThat(config.getCliqueConfigOptions()).isNotSameAs(CliqueConfigOptions.DEFAULT); assertThat(config.getConsensusEngine()).isEqualTo("clique"); } @@ -63,6 +72,7 @@ public void shouldUseCliqueWhenCliqueInConfig() { public void shouldNotUseCliqueIfCliqueNotPresent() { final GenesisConfigOptions config = fromConfigOptions(emptyMap()); assertThat(config.isClique()).isFalse(); + assertThat(config.isPoa()).isFalse(); assertThat(config.getCliqueConfigOptions()).isSameAs(CliqueConfigOptions.DEFAULT); } @@ -230,6 +240,7 @@ public void shouldSupportEmptyGenesisConfig() { final GenesisConfigOptions config = GenesisConfigFile.fromConfig("{}").getConfigOptions(); assertThat(config.isEthHash()).isFalse(); assertThat(config.isClique()).isFalse(); + assertThat(config.isPoa()).isFalse(); assertThat(config.getHomesteadBlockNumber()).isEmpty(); }