Refactor how genesis file overrides are applied (#7489)

* added tests that fail
* separate getConfigOptions() logic from applying overrides

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

---------

Signed-off-by: Sally MacFarlane <macfarla.github@gmail.com>

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
+    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());

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<EthNetworkConfig> 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<EthNetworkConfig> 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");

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()

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()

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())

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<String, String> 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<String, String> overrides) {
     final ObjectNode config = loader.getConfig();
-
+    // are there any overrides to apply?
-    Map<String, String> overridesRef = overrides;
+    if (this.overrides == null) {
+      return JsonGenesisConfigOptions.fromJsonObject(config);
+    }
+    // otherwise apply overrides
+    Map<String, String> 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<String, String> overrides) {
+
+    this.overrides = overrides;
+    return this;
+  }
+
   /**
    * Stream allocations stream.
    *

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.withOverrides(override).getConfigOptions().getIstanbulBlockNumber())
-    assertThat(config.getConfigOptions(override).getChainId())
+        .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.withOverrides(override).getConfigOptions().getIstanbulBlockNumber())
-    assertThat(config.getConfigOptions(override).getChainId()).isNotPresent();
+        .isNotPresent();
-    assertThat(config.getConfigOptions(override).getContractSizeLimit()).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.withOverrides(override).getConfigOptions().getIstanbulBlockNumber())
-    assertThat(config.getConfigOptions(override).getChainId())
+        .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.withOverrides(override).getConfigOptions().getIstanbulBlockNumber())
-    assertThat(config.getConfigOptions(override).getChainId()).isNotPresent();
+        .isNotPresent();
-    assertThat(config.getConfigOptions(override).getContractSizeLimit()).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.");
   }

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",