From c555775c1f0413a6f37e683509a21e5dfc34337c Mon Sep 17 00:00:00 2001 From: Sally MacFarlane Date: Wed, 21 Aug 2024 09:50:07 +1000 Subject: [PATCH] Refactor how genesis file overrides are applied (#7489) * added tests that fail * separate getConfigOptions() logic from applying overrides Signed-off-by: Sally MacFarlane --------- Signed-off-by: Sally MacFarlane --- .../org/hyperledger/besu/cli/BesuCommand.java | 12 +++-- .../hyperledger/besu/cli/BesuCommandTest.java | 54 +++++++++++++++++++ .../AbstractBftBesuControllerBuilderTest.java | 1 - .../CliqueBesuControllerBuilderTest.java | 1 - .../MergeBesuControllerBuilderTest.java | 1 - .../besu/config/GenesisConfigFile.java | 36 +++++++------ .../besu/config/GenesisConfigFileTest.java | 42 +++++++++------ .../mainnet/DifficultyCalculatorTests.java | 2 +- 8 files changed, 110 insertions(+), 39 deletions(-) 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 ba61a869ff..00ea6e1efd 100644 --- a/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java +++ b/besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java @@ -1651,15 +1651,17 @@ public class BesuCommand implements DefaultCommandValues, Runnable { } private GenesisConfigFile readGenesisConfigFile() { - return genesisFile != null - ? GenesisConfigFile.fromSource(genesisConfigSource(genesisFile)) - : GenesisConfigFile.fromResource( - Optional.ofNullable(network).orElse(MAINNET).getGenesisFile()); + final GenesisConfigFile effectiveGenesisFile = + genesisFile != null + ? GenesisConfigFile.fromSource(genesisConfigSource(genesisFile)) + : GenesisConfigFile.fromResource( + Optional.ofNullable(network).orElse(MAINNET).getGenesisFile()); + return effectiveGenesisFile.withOverrides(genesisConfigOverrides); } private GenesisConfigOptions readGenesisConfigOptions() { try { - return genesisConfigFileSupplier.get().getConfigOptions(genesisConfigOverrides); + return genesisConfigFileSupplier.get().getConfigOptions(); } catch (final Exception e) { throw new ParameterException( this.commandLine, "Unable to load genesis file. " + e.getCause()); 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 23ba593a38..e6e0e85951 100644 --- a/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java +++ b/besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java @@ -171,6 +171,60 @@ public class BesuCommandTest extends CommandTestAbstract { MergeConfigOptions.setMergeEnabled(false); } + @Test + public void testGenesisOverrideOptions() throws Exception { + parseCommand("--override-genesis-config", "shanghaiTime=123"); + + final ArgumentCaptor networkArg = + ArgumentCaptor.forClass(EthNetworkConfig.class); + + verify(mockControllerBuilderFactory).fromEthNetworkConfig(networkArg.capture(), any()); + verify(mockControllerBuilder).build(); + + final EthNetworkConfig config = networkArg.getValue(); + // mainnet defaults + assertThat(config.networkId()).isEqualTo(BigInteger.valueOf(1)); + + // assert that shanghaiTime override is applied + final GenesisConfigFile actualGenesisConfigFile = (config.genesisConfigFile()); + assertThat(actualGenesisConfigFile).isNotNull(); + assertThat(actualGenesisConfigFile.getConfigOptions().getShanghaiTime()).isNotEmpty(); + assertThat(actualGenesisConfigFile.getConfigOptions().getShanghaiTime().getAsLong()) + .isEqualTo(123); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + } + + @Test + public void testGenesisOverrideOptionsWithCustomGenesis() throws Exception { + final Path genesisFile = createFakeGenesisFile(GENESIS_VALID_JSON); + + parseCommand( + "--genesis-file", genesisFile.toString(), "--override-genesis-config", "shanghaiTime=123"); + + final ArgumentCaptor networkArg = + ArgumentCaptor.forClass(EthNetworkConfig.class); + + verify(mockControllerBuilderFactory).fromEthNetworkConfig(networkArg.capture(), any()); + verify(mockControllerBuilder).build(); + + final EthNetworkConfig config = networkArg.getValue(); + assertThat(config.bootNodes()).isEmpty(); + assertThat(config.dnsDiscoveryUrl()).isNull(); + assertThat(config.networkId()).isEqualTo(BigInteger.valueOf(3141592)); + + // then assert that the shanghaiTime is applied + final GenesisConfigFile actualGenesisConfigFile = (config.genesisConfigFile()); + assertThat(actualGenesisConfigFile).isNotNull(); + assertThat(actualGenesisConfigFile.getConfigOptions().getShanghaiTime()).isNotEmpty(); + assertThat(actualGenesisConfigFile.getConfigOptions().getShanghaiTime().getAsLong()) + .isEqualTo(123); + + assertThat(commandOutput.toString(UTF_8)).isEmpty(); + assertThat(commandErrorOutput.toString(UTF_8)).isEmpty(); + } + @Test public void callingHelpSubCommandMustDisplayUsage() { parseCommand("--help"); diff --git a/besu/src/test/java/org/hyperledger/besu/controller/AbstractBftBesuControllerBuilderTest.java b/besu/src/test/java/org/hyperledger/besu/controller/AbstractBftBesuControllerBuilderTest.java index 731fd2ac56..f9e671f522 100644 --- a/besu/src/test/java/org/hyperledger/besu/controller/AbstractBftBesuControllerBuilderTest.java +++ b/besu/src/test/java/org/hyperledger/besu/controller/AbstractBftBesuControllerBuilderTest.java @@ -104,7 +104,6 @@ public abstract class AbstractBftBesuControllerBuilderTest { lenient().when(genesisConfigFile.getDifficulty()).thenReturn(Bytes.of(0).toHexString()); lenient().when(genesisConfigFile.getMixHash()).thenReturn(Hash.ZERO.toHexString()); lenient().when(genesisConfigFile.getNonce()).thenReturn(Long.toHexString(1)); - lenient().when(genesisConfigFile.getConfigOptions(any())).thenReturn(genesisConfigOptions); lenient().when(genesisConfigFile.getConfigOptions()).thenReturn(genesisConfigOptions); lenient().when(genesisConfigOptions.getCheckpointOptions()).thenReturn(checkpointConfigOptions); lenient() diff --git a/besu/src/test/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilderTest.java b/besu/src/test/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilderTest.java index 82f98ba826..39904df798 100644 --- a/besu/src/test/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilderTest.java +++ b/besu/src/test/java/org/hyperledger/besu/controller/CliqueBesuControllerBuilderTest.java @@ -113,7 +113,6 @@ public class CliqueBesuControllerBuilderTest { "0x0000000000000000000000000000000000000000000000000000000000000000b9b81ee349c3807e46bc71aa2632203c5b4620340000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"); lenient().when(genesisConfigFile.getMixHash()).thenReturn(Hash.ZERO.toHexString()); lenient().when(genesisConfigFile.getNonce()).thenReturn(Long.toHexString(1)); - lenient().when(genesisConfigFile.getConfigOptions(any())).thenReturn(genesisConfigOptions); lenient().when(genesisConfigFile.getConfigOptions()).thenReturn(genesisConfigOptions); lenient().when(genesisConfigOptions.getCheckpointOptions()).thenReturn(checkpointConfigOptions); lenient() diff --git a/besu/src/test/java/org/hyperledger/besu/controller/MergeBesuControllerBuilderTest.java b/besu/src/test/java/org/hyperledger/besu/controller/MergeBesuControllerBuilderTest.java index 0e4948ddd5..544b633015 100644 --- a/besu/src/test/java/org/hyperledger/besu/controller/MergeBesuControllerBuilderTest.java +++ b/besu/src/test/java/org/hyperledger/besu/controller/MergeBesuControllerBuilderTest.java @@ -124,7 +124,6 @@ public class MergeBesuControllerBuilderTest { lenient().when(genesisConfigFile.getExtraData()).thenReturn(Bytes.EMPTY.toHexString()); lenient().when(genesisConfigFile.getMixHash()).thenReturn(Hash.ZERO.toHexString()); lenient().when(genesisConfigFile.getNonce()).thenReturn(Long.toHexString(1)); - lenient().when(genesisConfigFile.getConfigOptions(any())).thenReturn(genesisConfigOptions); lenient().when(genesisConfigFile.getConfigOptions()).thenReturn(genesisConfigOptions); lenient().when(genesisConfigOptions.getCheckpointOptions()).thenReturn(checkpointConfigOptions); when(genesisConfigOptions.getTerminalTotalDifficulty()) diff --git a/config/src/main/java/org/hyperledger/besu/config/GenesisConfigFile.java b/config/src/main/java/org/hyperledger/besu/config/GenesisConfigFile.java index bd1e117773..453c38dac2 100644 --- a/config/src/main/java/org/hyperledger/besu/config/GenesisConfigFile.java +++ b/config/src/main/java/org/hyperledger/besu/config/GenesisConfigFile.java @@ -18,7 +18,6 @@ import org.hyperledger.besu.datatypes.Wei; import java.net.URL; import java.util.Arrays; -import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -41,6 +40,7 @@ public class GenesisConfigFile { private final GenesisReader loader; private final ObjectNode genesisRoot; + private Map overrides; private GenesisConfigFile(final GenesisReader loader) { this.loader = loader; @@ -107,36 +107,42 @@ public class GenesisConfigFile { } /** - * Gets config options. + * Gets config options, including any overrides. * * @return the config options */ public GenesisConfigOptions getConfigOptions() { - return getConfigOptions(Collections.emptyMap()); - } - - /** - * Gets config options. - * - * @param overrides the overrides - * @return the config options - */ - public GenesisConfigOptions getConfigOptions(final Map overrides) { final ObjectNode config = loader.getConfig(); - - Map overridesRef = overrides; + // are there any overrides to apply? + if (this.overrides == null) { + return JsonGenesisConfigOptions.fromJsonObject(config); + } + // otherwise apply overrides + Map overridesRef = this.overrides; // if baseFeePerGas has been explicitly configured, pass it as an override: final var optBaseFee = getBaseFeePerGas(); if (optBaseFee.isPresent()) { // streams and maps cannot handle null values. - overridesRef = new HashMap<>(overrides); + overridesRef = new HashMap<>(this.overrides); overridesRef.put("baseFeePerGas", optBaseFee.get().toShortHexString()); } return JsonGenesisConfigOptions.fromJsonObjectWithOverrides(config, overridesRef); } + /** + * Sets overrides for genesis options. + * + * @param overrides the overrides + * @return the config options + */ + public GenesisConfigFile withOverrides(final Map overrides) { + + this.overrides = overrides; + return this; + } + /** * Stream allocations stream. * diff --git a/config/src/test/java/org/hyperledger/besu/config/GenesisConfigFileTest.java b/config/src/test/java/org/hyperledger/besu/config/GenesisConfigFileTest.java index 9f014b7d4d..3e6b488b9b 100644 --- a/config/src/test/java/org/hyperledger/besu/config/GenesisConfigFileTest.java +++ b/config/src/test/java/org/hyperledger/besu/config/GenesisConfigFileTest.java @@ -187,7 +187,9 @@ class GenesisConfigFileTest { @Test void shouldOverrideConfigOptionsBaseFeeWhenSpecified() { GenesisConfigOptions withOverrides = - EMPTY_CONFIG.getConfigOptions(Map.of("baseFeePerGas", Wei.of(8).toString())); + EMPTY_CONFIG + .withOverrides(Map.of("baseFeePerGas", Wei.of(8).toString())) + .getConfigOptions(); assertThat(withOverrides.getBaseFeePerGas()).contains(Wei.of(8L)); } @@ -229,7 +231,8 @@ class GenesisConfigFileTest { void assertTerminalTotalDifficultyOverride() { GenesisConfigOptions sepoliaOverrideOptions = GenesisConfigFile.fromResource("/sepolia.json") - .getConfigOptions(Map.of("terminalTotalDifficulty", String.valueOf(Long.MAX_VALUE))); + .withOverrides(Map.of("terminalTotalDifficulty", String.valueOf(Long.MAX_VALUE))) + .getConfigOptions(); assertThat(sepoliaOverrideOptions.getTerminalTotalDifficulty()).isPresent(); assertThat(sepoliaOverrideOptions.getTerminalTotalDifficulty()) @@ -355,10 +358,12 @@ class GenesisConfigFileTest { override.put("contractSizeLimit", bigBlockString); assertThat(config.getForkBlockNumbers()).isNotEmpty(); - assertThat(config.getConfigOptions(override).getIstanbulBlockNumber()).hasValue(bigBlock); - assertThat(config.getConfigOptions(override).getChainId()) + assertThat(config.withOverrides(override).getConfigOptions().getIstanbulBlockNumber()) + .hasValue(bigBlock); + assertThat(config.withOverrides(override).getConfigOptions().getChainId()) .hasValue(BigInteger.valueOf(bigBlock)); - assertThat(config.getConfigOptions(override).getContractSizeLimit()).hasValue(bigBlock); + assertThat(config.withOverrides(override).getConfigOptions().getContractSizeLimit()) + .hasValue(bigBlock); } @Test @@ -370,9 +375,11 @@ class GenesisConfigFileTest { override.put("contractSizeLimit", null); assertThat(config.getForkBlockNumbers()).isNotEmpty(); - assertThat(config.getConfigOptions(override).getIstanbulBlockNumber()).isNotPresent(); - assertThat(config.getConfigOptions(override).getChainId()).isNotPresent(); - assertThat(config.getConfigOptions(override).getContractSizeLimit()).isNotPresent(); + assertThat(config.withOverrides(override).getConfigOptions().getIstanbulBlockNumber()) + .isNotPresent(); + assertThat(config.withOverrides(override).getConfigOptions().getChainId()).isNotPresent(); + assertThat(config.withOverrides(override).getConfigOptions().getContractSizeLimit()) + .isNotPresent(); } @Test @@ -388,10 +395,12 @@ class GenesisConfigFileTest { // all lower case override.put("contractsizelimit", bigBlockString); - assertThat(config.getConfigOptions(override).getIstanbulBlockNumber()).hasValue(bigBlock); - assertThat(config.getConfigOptions(override).getChainId()) + assertThat(config.withOverrides(override).getConfigOptions().getIstanbulBlockNumber()) + .hasValue(bigBlock); + assertThat(config.withOverrides(override).getConfigOptions().getChainId()) .hasValue(BigInteger.valueOf(bigBlock)); - assertThat(config.getConfigOptions(override).getContractSizeLimit()).hasValue(bigBlock); + assertThat(config.withOverrides(override).getConfigOptions().getContractSizeLimit()) + .hasValue(bigBlock); } @Test @@ -402,9 +411,11 @@ class GenesisConfigFileTest { override.put("chainId", ""); override.put("contractSizeLimit", ""); - assertThat(config.getConfigOptions(override).getIstanbulBlockNumber()).isNotPresent(); - assertThat(config.getConfigOptions(override).getChainId()).isNotPresent(); - assertThat(config.getConfigOptions(override).getContractSizeLimit()).isNotPresent(); + assertThat(config.withOverrides(override).getConfigOptions().getIstanbulBlockNumber()) + .isNotPresent(); + assertThat(config.withOverrides(override).getConfigOptions().getChainId()).isNotPresent(); + assertThat(config.withOverrides(override).getConfigOptions().getContractSizeLimit()) + .isNotPresent(); } @Test @@ -431,7 +442,8 @@ class GenesisConfigFileTest { override.put("constantinopleFixBlock", "1000"); assertThatExceptionOfType(RuntimeException.class) - .isThrownBy(() -> config.getConfigOptions(override).getPetersburgBlockNumber()) + .isThrownBy( + () -> config.withOverrides(override).getConfigOptions().getPetersburgBlockNumber()) .withMessage( "Genesis files cannot specify both petersburgBlock and constantinopleFixBlock."); } diff --git a/ethereum/referencetests/src/reference-test/java/org/hyperledger/besu/ethereum/mainnet/DifficultyCalculatorTests.java b/ethereum/referencetests/src/reference-test/java/org/hyperledger/besu/ethereum/mainnet/DifficultyCalculatorTests.java index 00733de1f8..cfb56d1ebc 100644 --- a/ethereum/referencetests/src/reference-test/java/org/hyperledger/besu/ethereum/mainnet/DifficultyCalculatorTests.java +++ b/ethereum/referencetests/src/reference-test/java/org/hyperledger/besu/ethereum/mainnet/DifficultyCalculatorTests.java @@ -60,7 +60,7 @@ public class DifficultyCalculatorTests { "/BasicTests/difficultyMainNetwork.json", MainnetProtocolSchedule.fromConfig( GenesisConfigFile.mainnet() - .getConfigOptions(postMergeOverrides), + .withOverrides(postMergeOverrides).getConfigOptions(), EvmConfiguration.DEFAULT, MiningParameters.newDefault(), new BadBlockManager(), false, new NoOpMetricsSystem())), Arguments.of( "/DifficultyTests/dfGrayGlacier/difficultyGrayGlacierForkBlock.json",