diff --git a/acceptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/GenesisGenerator.java b/acceptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/GenesisGenerator.java index 85fab383a44..d8b0a53c1ee 100644 --- a/acceptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/GenesisGenerator.java +++ b/acceptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/GenesisGenerator.java @@ -131,7 +131,7 @@ public GenesisGenerator genesisExecutionPayloadHeaderSource( } public InitialStateData generate() { - final Spec spec = SpecFactory.create(network, specConfigModifier); + final Spec spec = SpecFactory.create(network, false, specConfigModifier); final GenesisStateBuilder genesisBuilder = new GenesisStateBuilder(); genesisBuilder .spec(spec) diff --git a/acceptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/TekuBeaconNode.java b/acceptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/TekuBeaconNode.java index a9119f9aba3..7e66c26a73f 100644 --- a/acceptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/TekuBeaconNode.java +++ b/acceptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/TekuBeaconNode.java @@ -100,7 +100,7 @@ private TekuBeaconNode( final Network network, final TekuDockerVersion version, final TekuNodeConfig tekuNodeConfig) { super(network, TEKU_DOCKER_IMAGE_NAME, version, LOG); this.config = tekuNodeConfig; - this.spec = SpecFactory.create(config.getNetworkName(), config.getSpecConfigModifier()); + this.spec = SpecFactory.create(config.getNetworkName(), true, config.getSpecConfigModifier()); if (config.getConfigMap().containsKey("validator-api-enabled")) { container.addExposedPort(VALIDATOR_API_PORT); } diff --git a/ethereum/networks/src/main/java/tech/pegasys/teku/networks/Eth2NetworkConfiguration.java b/ethereum/networks/src/main/java/tech/pegasys/teku/networks/Eth2NetworkConfiguration.java index 006d377913c..98a9d91b056 100644 --- a/ethereum/networks/src/main/java/tech/pegasys/teku/networks/Eth2NetworkConfiguration.java +++ b/ethereum/networks/src/main/java/tech/pegasys/teku/networks/Eth2NetworkConfiguration.java @@ -412,6 +412,7 @@ public static class Builder { DEFAULT_FORK_CHOICE_UPDATED_ALWAYS_SEND_PAYLOAD_ATTRIBUTES; private OptionalInt pendingAttestationsMaxQueue = OptionalInt.empty(); private boolean rustKzgEnabled = DEFAULT_RUST_KZG_ENABLED; + private boolean strictConfigLoadingEnabled; public void spec(final Spec spec) { this.spec = spec; @@ -427,6 +428,7 @@ public Eth2NetworkConfiguration build() { spec = SpecFactory.create( constants, + strictConfigLoadingEnabled, builder -> { // Ephemery network field change periodically, update to current if (constants.equals(EPHEMERY.configName())) { @@ -1058,5 +1060,10 @@ public Builder pendingAttestationsMaxQueue(final int pendingAttestationsMaxQueue this.pendingAttestationsMaxQueue = OptionalInt.of(pendingAttestationsMaxQueue); return this; } + + public Builder strictConfigLoadingEnabled(final boolean strictConfigLoadingEnabled) { + this.strictConfigLoadingEnabled = strictConfigLoadingEnabled; + return this; + } } } diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/SpecFactory.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/SpecFactory.java index ec3742a778c..e476b1e933e 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/SpecFactory.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/SpecFactory.java @@ -38,12 +38,15 @@ public class SpecFactory { public static Spec create(final String configName) { - return create(configName, __ -> {}); + return create(configName, false, __ -> {}); } - public static Spec create(final String configName, final Consumer modifier) { + public static Spec create( + final String configName, + final boolean strictConfigLoadingEnabled, + final Consumer modifier) { final SpecConfigAndParent config = - SpecConfigLoader.loadConfig(configName, modifier); + SpecConfigLoader.loadConfig(configName, strictConfigLoadingEnabled, modifier); return create(config); } diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigLoader.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigLoader.java index c80c1e2ea68..620f195ece5 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigLoader.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigLoader.java @@ -63,6 +63,13 @@ public static SpecConfigAndParent loadConfig( return loadConfig(configName, true, modifier); } + public static SpecConfigAndParent loadConfig( + final String configName, + final boolean strictConfigLoadingEnabled, + final Consumer modifier) { + return loadConfig(configName, strictConfigLoadingEnabled, true, modifier); + } + public static SpecConfigAndParent loadRemoteConfig( final Map config) { final SpecConfigReader reader = new SpecConfigReader(); @@ -76,7 +83,7 @@ public static SpecConfigAndParent loadRemoteConfig( if (config.containsKey(SpecConfigReader.CONFIG_NAME_KEY)) { final String configNameKey = (String) config.get(SpecConfigReader.CONFIG_NAME_KEY); try { - processConfig(configNameKey, reader, true); + processConfig(configNameKey, reader, false, true); } catch (IllegalArgumentException exception) { LOG.debug( "Failed to load base configuration from {}, {}", @@ -90,26 +97,31 @@ public static SpecConfigAndParent loadRemoteConfig( static SpecConfigAndParent loadConfig( final String configName, - final boolean isForgiving, + final boolean strictConfigLoadingEnabled, + final boolean isIgnoreUnknownConfigItems, final Consumer modifier) { final SpecConfigReader reader = new SpecConfigReader(); - processConfig(configName, reader, isForgiving); + processConfig(configName, reader, strictConfigLoadingEnabled, isIgnoreUnknownConfigItems); return reader.build(modifier); } // A little extra configuration is able to be loaded from builtin configs. - // isForgiving + // isIgnoreUnknownConfigItems // - if config in source has entries we don't need, we will silently ignore them - // - if `CONFIG_NAME` is set in config, and we're missing fields, we will copy them + // isFailRatherThanDefaultFromBuiltin + // - if configuration requires a key and it's not specified, fail rather than default it. // Presets will be loaded if specified also, this happens after defaulting from config_name private static void processConfig( - final String source, final SpecConfigReader reader, final boolean isForgiving) { + final String source, + final SpecConfigReader reader, + final boolean isFailRatherThanDefaultFromBuiltin, + final boolean isIgnoreUnknownConfigItems) { try { final Map configValues = readConfigToMap(source, reader); if (!BUILTIN_NETWORKS.contains(source) && configValues.containsKey(SpecConfigReader.CONFIG_NAME_KEY) - && isForgiving) { + && !isFailRatherThanDefaultFromBuiltin) { final String builtinConfigName = (String) configValues.get(SpecConfigReader.CONFIG_NAME_KEY); if (BUILTIN_NETWORKS.contains(builtinConfigName)) { @@ -140,9 +152,9 @@ private static void processConfig( // Legacy config files won't have a preset field if (maybePreset.isPresent()) { final String preset = maybePreset.get(); - applyPreset(source, reader, isForgiving, preset); + applyPreset(source, reader, isIgnoreUnknownConfigItems, preset); } - reader.loadFromMap(configValues, isForgiving); + reader.loadFromMap(configValues, isIgnoreUnknownConfigItems); } catch (IOException e) { throw new IllegalArgumentException( "Unable to load configuration for network \"" + source + "\": " + e.getMessage(), e); diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigLoaderTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigLoaderTest.java index 5baf03973a8..08c9747f15a 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigLoaderTest.java +++ b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigLoaderTest.java @@ -20,9 +20,12 @@ import static tech.pegasys.teku.spec.config.SpecConfigAssertions.assertAllFieldsSet; import com.google.common.io.Resources; +import java.io.FileWriter; +import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.URL; +import java.nio.charset.StandardCharsets; import java.nio.file.Files; import java.nio.file.Path; import java.util.Map; @@ -75,18 +78,45 @@ public void shouldReadConfigByDefaults(@TempDir final Path tempDir) throws Excep PRESET_BASE: 'mainnet' """); final SpecConfig config = - SpecConfigLoader.loadConfig(configFile.toString(), true, __ -> {}).specConfig(); + SpecConfigLoader.loadConfig(configFile.toString(), false, true, __ -> {}).specConfig(); assertAllBellatrixFieldsSet(config); assertAllBellatrixFieldsSet(config); } + @Test + public void shouldFailToReadConfigWithMissingItemsWhenStrict(@TempDir final Path tempDir) + throws IOException { + final Path configFile = tempDir.resolve("config.yaml"); + Files.writeString(configFile, "CONFIG_NAME: 'mainnet'"); + assertThatThrownBy( + () -> SpecConfigLoader.loadConfig(configFile.toString(), true, true, __ -> {})) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining( + "The specified network configuration had missing or invalid values for constants"); + } + + @Test + public void shouldFailToReadConfigIfNotForgivingAndHasExtraItems(@TempDir final Path tempDir) + throws Exception { + final Path file = tempDir.resolve("mainnet.yml"); + try (final InputStream inputStream = getMainnetConfigAsStream()) { + writeStreamToFile(inputStream, file); + } + try (FileWriter writer = new FileWriter(file.toFile(), StandardCharsets.UTF_8, true)) { + writer.write("\nUNKNOWN_OPTION: FOO"); + } + assertThatThrownBy(() -> SpecConfigLoader.loadConfig(file.toString(), true, false, __ -> {})) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("Detected unknown spec config entries: UNKNOWN_OPTION"); + } + @Test public void shouldReadConfigByDefaultsWithoutPreset(@TempDir final Path tempDir) throws Exception { final Path configFile = tempDir.resolve("config.yaml"); Files.writeString(configFile, "CONFIG_NAME: 'mainnet'\n"); final SpecConfig config = - SpecConfigLoader.loadConfig(configFile.toString(), true, __ -> {}).specConfig(); + SpecConfigLoader.loadConfig(configFile.toString(), false, true, __ -> {}).specConfig(); assertAllBellatrixFieldsSet(config); assertAllBellatrixFieldsSet(config); } @@ -96,7 +126,8 @@ public void shouldFailToReadConfigWithoutDefaulting(@TempDir final Path tempDir) throws Exception { final Path configFile = tempDir.resolve("config.yaml"); Files.writeString(configFile, "PRESET_BASE: 'mainnet'\n"); - assertThatThrownBy(() -> SpecConfigLoader.loadConfig(configFile.toString(), true, __ -> {})) + assertThatThrownBy( + () -> SpecConfigLoader.loadConfig(configFile.toString(), true, true, __ -> {})) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining( "The specified network configuration had missing or invalid values for constants"); @@ -112,7 +143,8 @@ public void shouldFailToReadConfigWithMissingConfigName(@TempDir final Path temp CONFIG_NAME: 'missing' PRESET_BASE: 'mainnet' """); - assertThatThrownBy(() -> SpecConfigLoader.loadConfig(configFile.toString(), true, __ -> {})) + assertThatThrownBy( + () -> SpecConfigLoader.loadConfig(configFile.toString(), true, true, __ -> {})) .isInstanceOf(IllegalArgumentException.class) .hasMessageContaining( "The specified network configuration had missing or invalid values for constants"); diff --git a/networking/p2p/src/main/java/tech/pegasys/teku/networking/p2p/network/config/NetworkConfig.java b/networking/p2p/src/main/java/tech/pegasys/teku/networking/p2p/network/config/NetworkConfig.java index 6f3fa94d8f1..e0252596780 100644 --- a/networking/p2p/src/main/java/tech/pegasys/teku/networking/p2p/network/config/NetworkConfig.java +++ b/networking/p2p/src/main/java/tech/pegasys/teku/networking/p2p/network/config/NetworkConfig.java @@ -49,6 +49,7 @@ public class NetworkConfig { public static final int DEFAULT_P2P_PORT = 9000; public static final int DEFAULT_P2P_PORT_IPV6 = 9090; public static final boolean DEFAULT_YAMUX_ENABLED = false; + public static final boolean DEFAULT_STRICT_CONFIG_LOADING_ENABLED = false; private final GossipConfig gossipConfig; private final WireLogsConfig wireLogsConfig; diff --git a/teku/src/main/java/tech/pegasys/teku/cli/options/Eth2NetworkOptions.java b/teku/src/main/java/tech/pegasys/teku/cli/options/Eth2NetworkOptions.java index 9b3a1387145..92d6f5d1be7 100644 --- a/teku/src/main/java/tech/pegasys/teku/cli/options/Eth2NetworkOptions.java +++ b/teku/src/main/java/tech/pegasys/teku/cli/options/Eth2NetworkOptions.java @@ -31,6 +31,7 @@ import tech.pegasys.teku.config.TekuConfiguration; import tech.pegasys.teku.infrastructure.exceptions.InvalidConfigurationException; import tech.pegasys.teku.infrastructure.unsigned.UInt64; +import tech.pegasys.teku.networking.p2p.network.config.NetworkConfig; import tech.pegasys.teku.networks.Eth2NetworkConfiguration; public class Eth2NetworkOptions { @@ -284,6 +285,17 @@ public class Eth2NetworkOptions { arity = "1") private Long eth1DepositContractDeployBlockOverride; + @Option( + names = {"--Xstartup-strict-config-loader-enabled"}, + paramLabel = "", + showDefaultValue = Visibility.ALWAYS, + description = + "Strict config loading will fail if a required parameter is not present in a passed in file, otherwise defaults will be used.", + arity = "0..1", + hidden = true, + fallbackValue = "true") + private boolean strictConfigLoadingEnabled = NetworkConfig.DEFAULT_STRICT_CONFIG_LOADING_ENABLED; + @CommandLine.Option( names = {"--Xepochs-store-blobs"}, hidden = true, @@ -331,6 +343,7 @@ private void configureEth2Network(final Eth2NetworkConfiguration.Builder builder } builder.applyNetworkDefaults(network); + builder.strictConfigLoadingEnabled(strictConfigLoadingEnabled); if (startupTargetPeerCount != null) { builder.startupTargetPeerCount(startupTargetPeerCount); }