From 79ca86dad253960d88e75318e068a419c1f63006 Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Tue, 3 Oct 2023 08:27:52 +1000 Subject: [PATCH 1/7] Defaulted config attributes that were moved from constants The constants that got moved have caused issues in remote config, and now in local config. This change defaults the values, which means if they're not supplied it'll silently succeed assuming the values from old constant values. I've reverted the initial remote config change, as it didn't really address the local loading issue mentioned in #7555, and it was potentially also applying in the incorrect order. `MIN_EPOCHS_FOR_BLOCK_REQUESTS` is different on mainnet and minimal, so that will be using the mainnet value by default. fixes #7555 Signed-off-by: Paul Harris --- CHANGELOG.md | 1 + .../teku/spec/config/SpecConfigLoader.java | 15 ------ .../teku/spec/config/SpecConfigReader.java | 2 +- .../config/builder/SpecConfigBuilder.java | 48 +++++++++++++------ .../cli/subcommand/RemoteSpecLoaderTest.java | 11 +++-- 5 files changed, 41 insertions(+), 36 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e817c6260b..2144132ffb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,3 +15,4 @@ For information on changes in released versions of Teku, see the [releases page] - Solve an unintended breaking change in `23.9.1` release which was including an updated leveldb native library not compatible with relative old Linux distributions. Leveldb native library has been updated to support older GLIBC versions (ie Ubuntu 20.04 and Debian 11). ### Bug Fixes +- Fixed an issue in remote config loading where preset values were being loaded over custom network values. 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 57e2ed00073..4ea9710c7e8 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 @@ -22,8 +22,6 @@ import java.util.Map; import java.util.Optional; import java.util.function.Consumer; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import tech.pegasys.teku.infrastructure.io.resource.ResourceLoader; import tech.pegasys.teku.spec.config.builder.SpecConfigBuilder; import tech.pegasys.teku.spec.networks.Eth2Network; @@ -35,8 +33,6 @@ public class SpecConfigLoader { private static final String CONFIG_PATH = "configs/"; private static final String PRESET_PATH = "presets/"; - private static final Logger LOG = LogManager.getLogger(); - public static SpecConfig loadConfigStrict(final String configName) { return loadConfig(configName, false, __ -> {}); } @@ -62,17 +58,6 @@ public static SpecConfig loadConfig( public static SpecConfig loadRemoteConfig( final Map config, final Consumer modifier) { final SpecConfigReader reader = new SpecConfigReader(); - if (config.containsKey(SpecConfigReader.CONFIG_NAME_KEY)) { - final String configNameKey = config.get(SpecConfigReader.CONFIG_NAME_KEY); - try { - processConfig(configNameKey, reader, true); - } catch (IllegalArgumentException exception) { - LOG.debug( - "Failed to load base configuration from {}, {}", - () -> configNameKey, - exception::getMessage); - } - } if (config.containsKey(SpecConfigReader.PRESET_KEY)) { try { applyPreset("remote", reader, true, config.get(SpecConfigReader.PRESET_KEY)); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java index ccab382ddbb..595220657c6 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java @@ -53,7 +53,7 @@ public class SpecConfigReader { private static final Logger LOG = LogManager.getLogger(); public static final String PRESET_KEY = "PRESET_BASE"; - public static final String CONFIG_NAME_KEY = "CONFIG_NAME"; + private static final String CONFIG_NAME_KEY = "CONFIG_NAME"; private static final ImmutableSet KEYS_TO_IGNORE = ImmutableSet.of( PRESET_KEY, diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java index b45dfc4dd43..49d85a8223d 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java @@ -19,6 +19,8 @@ import java.util.HashMap; import java.util.Map; import java.util.function.Consumer; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import tech.pegasys.teku.ethereum.execution.types.Eth1Address; import tech.pegasys.teku.infrastructure.bytes.Bytes4; import tech.pegasys.teku.infrastructure.unsigned.UInt64; @@ -28,7 +30,7 @@ @SuppressWarnings({"UnusedReturnValue", "unused"}) public class SpecConfigBuilder { - + private static final Logger LOG = LogManager.getLogger(); private final Map rawConfig = new HashMap<>(); // Misc @@ -102,21 +104,21 @@ public class SpecConfigBuilder { private Eth1Address depositContractAddress; // Networking - private Integer gossipMaxSize; - private Integer maxChunkSize; - private Integer maxRequestBlocks; - private Integer epochsPerSubnetSubscription; - private Integer ttfbTimeout; - private Integer respTimeout; - private Integer attestationPropagationSlotRange; - private Integer maximumGossipClockDisparity; - private Bytes4 messageDomainInvalidSnappy; - private Bytes4 messageDomainValidSnappy; + private Integer gossipMaxSize = 10485760; + private Integer maxChunkSize = 10485760; + private Integer maxRequestBlocks = 1024; + private Integer epochsPerSubnetSubscription = 256; + private Integer ttfbTimeout = 5; + private Integer respTimeout = 10; + private Integer attestationPropagationSlotRange = 32; + private Integer maximumGossipClockDisparity = 500; + private Bytes4 messageDomainInvalidSnappy = Bytes4.fromHexString("0x00000000"); + private Bytes4 messageDomainValidSnappy = Bytes4.fromHexString("0x01000000"); private Integer subnetsPerNode; - private Integer minEpochsForBlockRequests; - private Integer attestationSubnetCount; - private Integer attestationSubnetExtraBits; - private Integer attestationSubnetPrefixBits; + private Integer minEpochsForBlockRequests = 33024; + private Integer attestationSubnetCount = 64; + private Integer attestationSubnetExtraBits = 0; + private Integer attestationSubnetPrefixBits = 6; private final BuilderChain builderChain = BuilderChain.create(new AltairBuilder()) @@ -582,77 +584,93 @@ public SpecConfigBuilder depositContractAddress(final Eth1Address depositContrac } public SpecConfigBuilder gossipMaxSize(final Integer gossipMaxSize) { + LOG.debug("Updating GOSSIP_MAX_SIZE to {}", () -> gossipMaxSize); this.gossipMaxSize = gossipMaxSize; return this; } public SpecConfigBuilder maxChunkSize(final Integer maxChunkSize) { + LOG.debug("Updating MAX_CHUNK_SIZE to {}", () -> maxChunkSize); this.maxChunkSize = maxChunkSize; return this; } public SpecConfigBuilder maxRequestBlocks(final Integer maxRequestBlocks) { + LOG.debug("Updating MAX_REQUEST_BLOCKS to {}", () -> maxRequestBlocks); this.maxRequestBlocks = maxRequestBlocks; return this; } public SpecConfigBuilder epochsPerSubnetSubscription(final Integer epochsPerSubnetSubscription) { + LOG.debug("Updating EPOCHS_PER_SUBNET_SUBSCRIPTION to {}", () -> epochsPerSubnetSubscription); this.epochsPerSubnetSubscription = epochsPerSubnetSubscription; return this; } public SpecConfigBuilder minEpochsForBlockRequests(final Integer minEpochsForBlockRequests) { + LOG.debug("Updating MIN_EPOCHS_FOR_BLOCK_REQUESTS to {}", () -> minEpochsForBlockRequests); this.minEpochsForBlockRequests = minEpochsForBlockRequests; return this; } public SpecConfigBuilder ttfbTimeout(final Integer ttfbTimeout) { + LOG.debug("Updating TTFB_TIMEOUT to {}", () -> ttfbTimeout); this.ttfbTimeout = ttfbTimeout; return this; } public SpecConfigBuilder respTimeout(final Integer respTimeout) { + LOG.debug("Updating RESP_TIMEOUT to {}", () -> respTimeout); this.respTimeout = respTimeout; return this; } public SpecConfigBuilder attestationPropagationSlotRange( final Integer attestationPropagationSlotRange) { + LOG.debug( + "Updating ATTESTATION_PROPAGATION_SLOT_RANGE to {}", () -> attestationPropagationSlotRange); this.attestationPropagationSlotRange = attestationPropagationSlotRange; return this; } public SpecConfigBuilder maximumGossipClockDisparity(final Integer maximumGossipClockDisparity) { + LOG.debug("Updating MAXIMUM_CLOCK_DISPARITY to {}", () -> maximumGossipClockDisparity); this.maximumGossipClockDisparity = maximumGossipClockDisparity; return this; } public SpecConfigBuilder messageDomainInvalidSnappy(final Bytes4 messageDomainInvalidSnappy) { + LOG.debug("Updating MESSAGE_DOMAIN_INVALID_SNAPPY to {}", () -> messageDomainInvalidSnappy); this.messageDomainInvalidSnappy = messageDomainInvalidSnappy; return this; } public SpecConfigBuilder messageDomainValidSnappy(final Bytes4 messageDomainValidSnappy) { + LOG.debug("Updating MESSAGE_DOMAIN_VALID_SNAPPY to {}", () -> messageDomainValidSnappy); this.messageDomainValidSnappy = messageDomainValidSnappy; return this; } public SpecConfigBuilder subnetsPerNode(final Integer subnetsPerNode) { + LOG.debug("Updating SUBNETS_PER_NODE to {}", () -> subnetsPerNode); this.subnetsPerNode = subnetsPerNode; return this; } public SpecConfigBuilder attestationSubnetCount(final Integer attestationSubnetCount) { + LOG.debug("Updating ATTESTATION_SUBNET_COUNT to {}", () -> attestationSubnetCount); this.attestationSubnetCount = attestationSubnetCount; return this; } public SpecConfigBuilder attestationSubnetExtraBits(final Integer attestationSubnetExtraBits) { + LOG.debug("Updating ATTESTATION_SUBNET_EXTRA_BITS to {}", () -> attestationSubnetExtraBits); this.attestationSubnetExtraBits = attestationSubnetExtraBits; return this; } public SpecConfigBuilder attestationSubnetPrefixBits(final Integer attestationSubnetPrefixBits) { + LOG.debug("Updating ATTESTATION_SUBNET_PREFIX_BITS to {}", () -> attestationSubnetPrefixBits); this.attestationSubnetPrefixBits = attestationSubnetPrefixBits; return this; } diff --git a/teku/src/test/java/tech/pegasys/teku/cli/subcommand/RemoteSpecLoaderTest.java b/teku/src/test/java/tech/pegasys/teku/cli/subcommand/RemoteSpecLoaderTest.java index 777dd0b4d55..94b8caa2073 100644 --- a/teku/src/test/java/tech/pegasys/teku/cli/subcommand/RemoteSpecLoaderTest.java +++ b/teku/src/test/java/tech/pegasys/teku/cli/subcommand/RemoteSpecLoaderTest.java @@ -14,6 +14,7 @@ package tech.pegasys.teku.cli.subcommand; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static tech.pegasys.teku.cli.subcommand.ValidatorClientCommand.DENEB_KZG_NOOP; @@ -28,7 +29,7 @@ import org.junit.jupiter.api.Test; import tech.pegasys.teku.api.ConfigProvider; import tech.pegasys.teku.api.response.v1.config.GetSpecResponse; -import tech.pegasys.teku.infrastructure.bytes.Bytes4; +import tech.pegasys.teku.infrastructure.exceptions.InvalidConfigurationException; import tech.pegasys.teku.infrastructure.unsigned.UInt64; import tech.pegasys.teku.spec.Spec; import tech.pegasys.teku.spec.TestSpecFactory; @@ -59,13 +60,13 @@ void shouldFillWhenRequiredItemsAreMissing() { when(apiClient.getConfigSpec()).thenReturn(Optional.of(new GetSpecResponse(rawConfig))); - final SpecConfig config = - RemoteSpecLoader.getSpec(apiClient, modifier -> {}).getSpecConfig(UInt64.ONE); - assertThat(config.getGenesisForkVersion()).isEqualTo(Bytes4.fromHexString("0x00000001")); + assertThatThrownBy(() -> RemoteSpecLoader.getSpec(apiClient, modifier -> {})) + .isInstanceOf(InvalidConfigurationException.class) + .hasMessageContaining("GENESIS_FORK_VERSION"); } @Test - void shouldProvideValidSpecConfigWithIncompleteRemoteConfig() throws IOException { + void shouldDefaultNetworkConfigThatMovedFromConstants() throws IOException { final String jsonConfig = Resources.toString( Resources.getResource(RemoteSpecLoaderTest.class, "config_missing_network_fields.json"), From 1e05bfbca0817623022bb5045625edbb2124115b Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Tue, 3 Oct 2023 08:30:27 +1000 Subject: [PATCH 2/7] updated changelog. Signed-off-by: Paul Harris --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2144132ffb3..29755730523 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,4 +15,4 @@ For information on changes in released versions of Teku, see the [releases page] - Solve an unintended breaking change in `23.9.1` release which was including an updated leveldb native library not compatible with relative old Linux distributions. Leveldb native library has been updated to support older GLIBC versions (ie Ubuntu 20.04 and Debian 11). ### Bug Fixes -- Fixed an issue in remote config loading where preset values were being loaded over custom network values. +- Fixed an issue in configuration loading, where network attributes moved from constants were stopping old configuration from loading. From 3918564d72646d7ff9d3a086d77462328979036c Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Tue, 3 Oct 2023 10:54:21 +1000 Subject: [PATCH 3/7] change update messages down to trace Signed-off-by: Paul Harris --- .../config/builder/SpecConfigBuilder.java | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java index 49d85a8223d..773c869b9ba 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java @@ -584,93 +584,93 @@ public SpecConfigBuilder depositContractAddress(final Eth1Address depositContrac } public SpecConfigBuilder gossipMaxSize(final Integer gossipMaxSize) { - LOG.debug("Updating GOSSIP_MAX_SIZE to {}", () -> gossipMaxSize); + LOG.trace("Updating GOSSIP_MAX_SIZE to {}", () -> gossipMaxSize); this.gossipMaxSize = gossipMaxSize; return this; } public SpecConfigBuilder maxChunkSize(final Integer maxChunkSize) { - LOG.debug("Updating MAX_CHUNK_SIZE to {}", () -> maxChunkSize); + LOG.trace("Updating MAX_CHUNK_SIZE to {}", () -> maxChunkSize); this.maxChunkSize = maxChunkSize; return this; } public SpecConfigBuilder maxRequestBlocks(final Integer maxRequestBlocks) { - LOG.debug("Updating MAX_REQUEST_BLOCKS to {}", () -> maxRequestBlocks); + LOG.trace("Updating MAX_REQUEST_BLOCKS to {}", () -> maxRequestBlocks); this.maxRequestBlocks = maxRequestBlocks; return this; } public SpecConfigBuilder epochsPerSubnetSubscription(final Integer epochsPerSubnetSubscription) { - LOG.debug("Updating EPOCHS_PER_SUBNET_SUBSCRIPTION to {}", () -> epochsPerSubnetSubscription); + LOG.trace("Updating EPOCHS_PER_SUBNET_SUBSCRIPTION to {}", () -> epochsPerSubnetSubscription); this.epochsPerSubnetSubscription = epochsPerSubnetSubscription; return this; } public SpecConfigBuilder minEpochsForBlockRequests(final Integer minEpochsForBlockRequests) { - LOG.debug("Updating MIN_EPOCHS_FOR_BLOCK_REQUESTS to {}", () -> minEpochsForBlockRequests); + LOG.trace("Updating MIN_EPOCHS_FOR_BLOCK_REQUESTS to {}", () -> minEpochsForBlockRequests); this.minEpochsForBlockRequests = minEpochsForBlockRequests; return this; } public SpecConfigBuilder ttfbTimeout(final Integer ttfbTimeout) { - LOG.debug("Updating TTFB_TIMEOUT to {}", () -> ttfbTimeout); + LOG.trace("Updating TTFB_TIMEOUT to {}", () -> ttfbTimeout); this.ttfbTimeout = ttfbTimeout; return this; } public SpecConfigBuilder respTimeout(final Integer respTimeout) { - LOG.debug("Updating RESP_TIMEOUT to {}", () -> respTimeout); + LOG.trace("Updating RESP_TIMEOUT to {}", () -> respTimeout); this.respTimeout = respTimeout; return this; } public SpecConfigBuilder attestationPropagationSlotRange( final Integer attestationPropagationSlotRange) { - LOG.debug( + LOG.trace( "Updating ATTESTATION_PROPAGATION_SLOT_RANGE to {}", () -> attestationPropagationSlotRange); this.attestationPropagationSlotRange = attestationPropagationSlotRange; return this; } public SpecConfigBuilder maximumGossipClockDisparity(final Integer maximumGossipClockDisparity) { - LOG.debug("Updating MAXIMUM_CLOCK_DISPARITY to {}", () -> maximumGossipClockDisparity); + LOG.trace("Updating MAXIMUM_CLOCK_DISPARITY to {}", () -> maximumGossipClockDisparity); this.maximumGossipClockDisparity = maximumGossipClockDisparity; return this; } public SpecConfigBuilder messageDomainInvalidSnappy(final Bytes4 messageDomainInvalidSnappy) { - LOG.debug("Updating MESSAGE_DOMAIN_INVALID_SNAPPY to {}", () -> messageDomainInvalidSnappy); + LOG.trace("Updating MESSAGE_DOMAIN_INVALID_SNAPPY to {}", () -> messageDomainInvalidSnappy); this.messageDomainInvalidSnappy = messageDomainInvalidSnappy; return this; } public SpecConfigBuilder messageDomainValidSnappy(final Bytes4 messageDomainValidSnappy) { - LOG.debug("Updating MESSAGE_DOMAIN_VALID_SNAPPY to {}", () -> messageDomainValidSnappy); + LOG.trace("Updating MESSAGE_DOMAIN_VALID_SNAPPY to {}", () -> messageDomainValidSnappy); this.messageDomainValidSnappy = messageDomainValidSnappy; return this; } public SpecConfigBuilder subnetsPerNode(final Integer subnetsPerNode) { - LOG.debug("Updating SUBNETS_PER_NODE to {}", () -> subnetsPerNode); + LOG.trace("Updating SUBNETS_PER_NODE to {}", () -> subnetsPerNode); this.subnetsPerNode = subnetsPerNode; return this; } public SpecConfigBuilder attestationSubnetCount(final Integer attestationSubnetCount) { - LOG.debug("Updating ATTESTATION_SUBNET_COUNT to {}", () -> attestationSubnetCount); + LOG.trace("Updating ATTESTATION_SUBNET_COUNT to {}", () -> attestationSubnetCount); this.attestationSubnetCount = attestationSubnetCount; return this; } public SpecConfigBuilder attestationSubnetExtraBits(final Integer attestationSubnetExtraBits) { - LOG.debug("Updating ATTESTATION_SUBNET_EXTRA_BITS to {}", () -> attestationSubnetExtraBits); + LOG.trace("Updating ATTESTATION_SUBNET_EXTRA_BITS to {}", () -> attestationSubnetExtraBits); this.attestationSubnetExtraBits = attestationSubnetExtraBits; return this; } public SpecConfigBuilder attestationSubnetPrefixBits(final Integer attestationSubnetPrefixBits) { - LOG.debug("Updating ATTESTATION_SUBNET_PREFIX_BITS to {}", () -> attestationSubnetPrefixBits); + LOG.trace("Updating ATTESTATION_SUBNET_PREFIX_BITS to {}", () -> attestationSubnetPrefixBits); this.attestationSubnetPrefixBits = attestationSubnetPrefixBits; return this; } From d6257999895a6dcdfabc7396b642490c24a9bfcb Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Wed, 4 Oct 2023 12:48:22 +1000 Subject: [PATCH 4/7] refactored to put defaults in the reader rather than the builder. Signed-off-by: Paul Harris --- .../teku/spec/config/SpecConfigReader.java | 23 ++++++++- .../config/builder/SpecConfigBuilder.java | 47 ++++++------------- 2 files changed, 36 insertions(+), 34 deletions(-) diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java index 595220657c6..0c507ec3fcd 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java @@ -107,7 +107,28 @@ public class SpecConfigReader { .put(Eth1Address.class, fromString(Eth1Address::fromHexString)) .build(); - final SpecConfigBuilder configBuilder = SpecConfig.builder(); + final SpecConfigBuilder configBuilder; + + public SpecConfigReader() { + // Values moved from Constants that should be defaulted to reduce compatibility issues + configBuilder = + SpecConfig.builder() + .gossipMaxSize(10485760) + .maxChunkSize(10485760) + .maxRequestBlocks(1024) + .epochsPerSubnetSubscription(256) + .ttfbTimeout(5) + .respTimeout(10) + .attestationPropagationSlotRange(32) + .maximumGossipClockDisparity(500) + .messageDomainInvalidSnappy(Bytes4.fromHexString("0x00000000")) + .messageDomainValidSnappy(Bytes4.fromHexString("0x01000000")) + .subnetsPerNode(2) + .minEpochsForBlockRequests(33024) + .attestationSubnetCount(64) + .attestationSubnetExtraBits(0) + .attestationSubnetPrefixBits(6); + } public SpecConfig build() { return configBuilder.build(); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java index 773c869b9ba..73367f5ca31 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java @@ -19,8 +19,6 @@ import java.util.HashMap; import java.util.Map; import java.util.function.Consumer; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import tech.pegasys.teku.ethereum.execution.types.Eth1Address; import tech.pegasys.teku.infrastructure.bytes.Bytes4; import tech.pegasys.teku.infrastructure.unsigned.UInt64; @@ -30,7 +28,6 @@ @SuppressWarnings({"UnusedReturnValue", "unused"}) public class SpecConfigBuilder { - private static final Logger LOG = LogManager.getLogger(); private final Map rawConfig = new HashMap<>(); // Misc @@ -104,21 +101,21 @@ public class SpecConfigBuilder { private Eth1Address depositContractAddress; // Networking - private Integer gossipMaxSize = 10485760; - private Integer maxChunkSize = 10485760; - private Integer maxRequestBlocks = 1024; - private Integer epochsPerSubnetSubscription = 256; - private Integer ttfbTimeout = 5; - private Integer respTimeout = 10; - private Integer attestationPropagationSlotRange = 32; - private Integer maximumGossipClockDisparity = 500; - private Bytes4 messageDomainInvalidSnappy = Bytes4.fromHexString("0x00000000"); - private Bytes4 messageDomainValidSnappy = Bytes4.fromHexString("0x01000000"); + private Integer gossipMaxSize; + private Integer maxChunkSize; + private Integer maxRequestBlocks; + private Integer epochsPerSubnetSubscription; + private Integer ttfbTimeout; + private Integer respTimeout; + private Integer attestationPropagationSlotRange; + private Integer maximumGossipClockDisparity; + private Bytes4 messageDomainInvalidSnappy; + private Bytes4 messageDomainValidSnappy; private Integer subnetsPerNode; - private Integer minEpochsForBlockRequests = 33024; - private Integer attestationSubnetCount = 64; - private Integer attestationSubnetExtraBits = 0; - private Integer attestationSubnetPrefixBits = 6; + private Integer minEpochsForBlockRequests; + private Integer attestationSubnetCount; + private Integer attestationSubnetExtraBits; + private Integer attestationSubnetPrefixBits; private final BuilderChain builderChain = BuilderChain.create(new AltairBuilder()) @@ -584,93 +581,77 @@ public SpecConfigBuilder depositContractAddress(final Eth1Address depositContrac } public SpecConfigBuilder gossipMaxSize(final Integer gossipMaxSize) { - LOG.trace("Updating GOSSIP_MAX_SIZE to {}", () -> gossipMaxSize); this.gossipMaxSize = gossipMaxSize; return this; } public SpecConfigBuilder maxChunkSize(final Integer maxChunkSize) { - LOG.trace("Updating MAX_CHUNK_SIZE to {}", () -> maxChunkSize); this.maxChunkSize = maxChunkSize; return this; } public SpecConfigBuilder maxRequestBlocks(final Integer maxRequestBlocks) { - LOG.trace("Updating MAX_REQUEST_BLOCKS to {}", () -> maxRequestBlocks); this.maxRequestBlocks = maxRequestBlocks; return this; } public SpecConfigBuilder epochsPerSubnetSubscription(final Integer epochsPerSubnetSubscription) { - LOG.trace("Updating EPOCHS_PER_SUBNET_SUBSCRIPTION to {}", () -> epochsPerSubnetSubscription); this.epochsPerSubnetSubscription = epochsPerSubnetSubscription; return this; } public SpecConfigBuilder minEpochsForBlockRequests(final Integer minEpochsForBlockRequests) { - LOG.trace("Updating MIN_EPOCHS_FOR_BLOCK_REQUESTS to {}", () -> minEpochsForBlockRequests); this.minEpochsForBlockRequests = minEpochsForBlockRequests; return this; } public SpecConfigBuilder ttfbTimeout(final Integer ttfbTimeout) { - LOG.trace("Updating TTFB_TIMEOUT to {}", () -> ttfbTimeout); this.ttfbTimeout = ttfbTimeout; return this; } public SpecConfigBuilder respTimeout(final Integer respTimeout) { - LOG.trace("Updating RESP_TIMEOUT to {}", () -> respTimeout); this.respTimeout = respTimeout; return this; } public SpecConfigBuilder attestationPropagationSlotRange( final Integer attestationPropagationSlotRange) { - LOG.trace( - "Updating ATTESTATION_PROPAGATION_SLOT_RANGE to {}", () -> attestationPropagationSlotRange); this.attestationPropagationSlotRange = attestationPropagationSlotRange; return this; } public SpecConfigBuilder maximumGossipClockDisparity(final Integer maximumGossipClockDisparity) { - LOG.trace("Updating MAXIMUM_CLOCK_DISPARITY to {}", () -> maximumGossipClockDisparity); this.maximumGossipClockDisparity = maximumGossipClockDisparity; return this; } public SpecConfigBuilder messageDomainInvalidSnappy(final Bytes4 messageDomainInvalidSnappy) { - LOG.trace("Updating MESSAGE_DOMAIN_INVALID_SNAPPY to {}", () -> messageDomainInvalidSnappy); this.messageDomainInvalidSnappy = messageDomainInvalidSnappy; return this; } public SpecConfigBuilder messageDomainValidSnappy(final Bytes4 messageDomainValidSnappy) { - LOG.trace("Updating MESSAGE_DOMAIN_VALID_SNAPPY to {}", () -> messageDomainValidSnappy); this.messageDomainValidSnappy = messageDomainValidSnappy; return this; } public SpecConfigBuilder subnetsPerNode(final Integer subnetsPerNode) { - LOG.trace("Updating SUBNETS_PER_NODE to {}", () -> subnetsPerNode); this.subnetsPerNode = subnetsPerNode; return this; } public SpecConfigBuilder attestationSubnetCount(final Integer attestationSubnetCount) { - LOG.trace("Updating ATTESTATION_SUBNET_COUNT to {}", () -> attestationSubnetCount); this.attestationSubnetCount = attestationSubnetCount; return this; } public SpecConfigBuilder attestationSubnetExtraBits(final Integer attestationSubnetExtraBits) { - LOG.trace("Updating ATTESTATION_SUBNET_EXTRA_BITS to {}", () -> attestationSubnetExtraBits); this.attestationSubnetExtraBits = attestationSubnetExtraBits; return this; } public SpecConfigBuilder attestationSubnetPrefixBits(final Integer attestationSubnetPrefixBits) { - LOG.trace("Updating ATTESTATION_SUBNET_PREFIX_BITS to {}", () -> attestationSubnetPrefixBits); this.attestationSubnetPrefixBits = attestationSubnetPrefixBits; return this; } From 4bb80f0b06f8f178f6d6b4e38e13600ac33c5b63 Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Thu, 5 Oct 2023 11:03:12 +1000 Subject: [PATCH 5/7] Now back to automatically loading the defaults for remote configuration For local configuration, will still fail to load, but will provide all of the fields failing validation, not just the first. Arguably we could have a CLI arg that loads default config for local resources also, but Im not sure that it should be the default, as we'd be hiding actual configuration issues and never be made aware. Error will be ``` java.lang.IllegalArgumentException: The specified network configuration had missing or invalid values for constants GOSSIP_MAX_SIZE, ATTESTATION_SUBNET_PREFIX_BITS, RESP_TIMEOUT, ATTESTATION_SUBNET_EXTRA_BITS, MAXIMUM_GOSSIP_CLOCK_DISPARITY, MAX_CHUNK_SIZE, ATTESTATION_PROPAGATION_SLOT_RANGE, MESSAGE_DOMAIN_INVALID_SNAPPY, TTFB_TIMEOUT, EPOCHS_PER_SUBNET_SUBSCRIPTION, MESSAGE_DOMAIN_VALID_SNAPPY, ATTESTATION_SUBNET_COUNT, MIN_EPOCHS_FOR_BLOCK_REQUESTS, MAX_REQUEST_BLOCKS ``` which is arguably harder to read, but doesnt force you to fix one, rinse repeat until you've got a valid configuration. Signed-off-by: Paul Harris --- .../teku/spec/config/SpecConfigLoader.java | 14 ++ .../teku/spec/config/SpecConfigReader.java | 25 +-- .../spec/config/builder/AltairBuilder.java | 47 ++++-- .../spec/config/builder/BellatrixBuilder.java | 35 ++++ .../spec/config/builder/CapellaBuilder.java | 36 +++- .../spec/config/builder/DenebBuilder.java | 47 ++++-- .../spec/config/builder/SpecBuilderUtil.java | 43 +++-- .../config/builder/SpecConfigBuilder.java | 159 ++++++++++-------- .../spec/config/SpecConfigReaderTest.java | 6 +- .../cli/subcommand/RemoteSpecLoaderTest.java | 9 +- 10 files changed, 275 insertions(+), 146 deletions(-) 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 4ea9710c7e8..8df3491ab14 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 @@ -22,12 +22,15 @@ import java.util.Map; import java.util.Optional; import java.util.function.Consumer; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import tech.pegasys.teku.infrastructure.io.resource.ResourceLoader; import tech.pegasys.teku.spec.config.builder.SpecConfigBuilder; import tech.pegasys.teku.spec.networks.Eth2Network; import tech.pegasys.teku.spec.networks.Eth2Presets; public class SpecConfigLoader { + private static final Logger LOG = LogManager.getLogger(); private static final List AVAILABLE_PRESETS = List.of("phase0", "altair", "bellatrix", "capella", "deneb"); private static final String CONFIG_PATH = "configs/"; @@ -65,6 +68,17 @@ public static SpecConfig loadRemoteConfig( throw new UncheckedIOException(e); } } + if (config.containsKey(SpecConfigReader.CONFIG_NAME_KEY)) { + final String configNameKey = config.get(SpecConfigReader.CONFIG_NAME_KEY); + try { + processConfig(configNameKey, reader, true); + } catch (IllegalArgumentException exception) { + LOG.debug( + "Failed to load base configuration from {}, {}", + () -> configNameKey, + exception::getMessage); + } + } reader.loadFromMap(config, true); return reader.build(modifier); } diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java index 0c507ec3fcd..ccab382ddbb 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java @@ -53,7 +53,7 @@ public class SpecConfigReader { private static final Logger LOG = LogManager.getLogger(); public static final String PRESET_KEY = "PRESET_BASE"; - private static final String CONFIG_NAME_KEY = "CONFIG_NAME"; + public static final String CONFIG_NAME_KEY = "CONFIG_NAME"; private static final ImmutableSet KEYS_TO_IGNORE = ImmutableSet.of( PRESET_KEY, @@ -107,28 +107,7 @@ public class SpecConfigReader { .put(Eth1Address.class, fromString(Eth1Address::fromHexString)) .build(); - final SpecConfigBuilder configBuilder; - - public SpecConfigReader() { - // Values moved from Constants that should be defaulted to reduce compatibility issues - configBuilder = - SpecConfig.builder() - .gossipMaxSize(10485760) - .maxChunkSize(10485760) - .maxRequestBlocks(1024) - .epochsPerSubnetSubscription(256) - .ttfbTimeout(5) - .respTimeout(10) - .attestationPropagationSlotRange(32) - .maximumGossipClockDisparity(500) - .messageDomainInvalidSnappy(Bytes4.fromHexString("0x00000000")) - .messageDomainValidSnappy(Bytes4.fromHexString("0x01000000")) - .subnetsPerNode(2) - .minEpochsForBlockRequests(33024) - .attestationSubnetCount(64) - .attestationSubnetExtraBits(0) - .attestationSubnetPrefixBits(6); - } + final SpecConfigBuilder configBuilder = SpecConfig.builder(); public SpecConfig build() { return configBuilder.build(); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/AltairBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/AltairBuilder.java index 7d347a8fb93..f306864a393 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/AltairBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/AltairBuilder.java @@ -16,6 +16,11 @@ import static com.google.common.base.Preconditions.checkNotNull; import static tech.pegasys.teku.spec.config.SpecConfig.FAR_FUTURE_EPOCH; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; import java.util.function.BiConsumer; import tech.pegasys.teku.infrastructure.bytes.Bytes4; import tech.pegasys.teku.infrastructure.unsigned.UInt64; @@ -95,19 +100,20 @@ public void validate() { SpecBuilderUtil.fillMissingValuesWithZeros(this); } - SpecBuilderUtil.validateConstant( - "inactivityPenaltyQuotientAltair", inactivityPenaltyQuotientAltair); - SpecBuilderUtil.validateConstant( - "minSlashingPenaltyQuotientAltair", minSlashingPenaltyQuotientAltair); - SpecBuilderUtil.validateConstant( - "proportionalSlashingMultiplierAltair", proportionalSlashingMultiplierAltair); - SpecBuilderUtil.validateConstant("syncCommitteeSize", syncCommitteeSize); - SpecBuilderUtil.validateConstant("inactivityScoreBias", inactivityScoreBias); - SpecBuilderUtil.validateConstant("inactivityScoreRecoveryRate", inactivityScoreRecoveryRate); - SpecBuilderUtil.validateConstant("epochsPerSyncCommitteePeriod", epochsPerSyncCommitteePeriod); - SpecBuilderUtil.validateConstant("altairForkVersion", altairForkVersion); - SpecBuilderUtil.validateConstant("altairForkEpoch", altairForkEpoch); - SpecBuilderUtil.validateConstant("minSyncCommitteeParticipants", minSyncCommitteeParticipants); + final List> maybeErrors = new ArrayList<>(); + final Map constants = getValidationMap(); + + constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); + + final List fieldsFailingValidation = + maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); + + if (!fieldsFailingValidation.isEmpty()) { + throw new IllegalArgumentException( + String.format( + "Errors were found validating configuration, missing or invalid values for constants %s", + String.join(", ", fieldsFailingValidation))); + } // Config items were added after launch so provide defaults to preserve compatibility if (updateTimeout == null) { @@ -115,6 +121,21 @@ public void validate() { } } + private Map getValidationMap() { + final Map constants = new HashMap<>(); + constants.put("inactivityPenaltyQuotientAltair", inactivityPenaltyQuotientAltair); + constants.put("minSlashingPenaltyQuotientAltair", minSlashingPenaltyQuotientAltair); + constants.put("proportionalSlashingMultiplierAltair", proportionalSlashingMultiplierAltair); + constants.put("syncCommitteeSize", syncCommitteeSize); + constants.put("inactivityScoreBias", inactivityScoreBias); + constants.put("inactivityScoreRecoveryRate", inactivityScoreRecoveryRate); + constants.put("epochsPerSyncCommitteePeriod", epochsPerSyncCommitteePeriod); + constants.put("altairForkVersion", altairForkVersion); + constants.put("altairForkEpoch", altairForkEpoch); + constants.put("minSyncCommitteeParticipants", minSyncCommitteeParticipants); + return constants; + } + @Override public void addOverridableItemsToRawConfig(final BiConsumer rawConfig) { rawConfig.accept("ALTAIR_FORK_EPOCH", altairForkEpoch); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BellatrixBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BellatrixBuilder.java index 5bba70d7f0d..5d0cf0b8a1c 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BellatrixBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BellatrixBuilder.java @@ -18,6 +18,11 @@ import static tech.pegasys.teku.spec.constants.NetworkConstants.DEFAULT_SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY; import java.math.BigInteger; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; import java.util.function.BiConsumer; import org.apache.tuweni.bytes.Bytes32; import org.apache.tuweni.units.bigints.UInt256; @@ -108,6 +113,36 @@ public void validate() { SpecBuilderUtil.validateConstant("maxTransactionsPerPayload", maxTransactionsPerPayload); SpecBuilderUtil.validateConstant("bytesPerLogsBloom", bytesPerLogsBloom); SpecBuilderUtil.validateConstant("maxExtraDataBytes", maxExtraDataBytes); + + final List> maybeErrors = new ArrayList<>(); + final Map constants = getValidationMap(); + + constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); + + final List fieldsFailingValidation = + maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); + + if (!fieldsFailingValidation.isEmpty()) { + throw new IllegalArgumentException( + String.format( + "The specified network configuration had missing or invalid values for constants %s", + String.join(", ", fieldsFailingValidation))); + } + } + + private Map getValidationMap() { + final Map constants = new HashMap<>(); + constants.put("bellatrixForkVersion", bellatrixForkVersion); + constants.put("bellatrixForkEpoch", bellatrixForkEpoch); + constants.put("inactivityPenaltyQuotientBellatrix", inactivityPenaltyQuotientBellatrix); + constants.put("minSlashingPenaltyQuotientBellatrix", minSlashingPenaltyQuotientBellatrix); + constants.put( + "proportionalSlashingMultiplierBellatrix", proportionalSlashingMultiplierBellatrix); + constants.put("maxBytesPerTransaction", maxBytesPerTransaction); + constants.put("maxTransactionsPerPayload", maxTransactionsPerPayload); + constants.put("bytesPerLogsBloom", bytesPerLogsBloom); + constants.put("maxExtraDataBytes", maxExtraDataBytes); + return constants; } @Override diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/CapellaBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/CapellaBuilder.java index 942a5e28dfa..88eabc0ed02 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/CapellaBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/CapellaBuilder.java @@ -16,6 +16,11 @@ import static com.google.common.base.Preconditions.checkNotNull; import static tech.pegasys.teku.spec.config.SpecConfig.FAR_FUTURE_EPOCH; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Optional; import java.util.function.BiConsumer; import tech.pegasys.teku.infrastructure.bytes.Bytes4; import tech.pegasys.teku.infrastructure.unsigned.UInt64; @@ -93,12 +98,31 @@ public void validate() { SpecBuilderUtil.fillMissingValuesWithZeros(this); } - SpecBuilderUtil.validateConstant("capellaForkVersion", capellaForkVersion); - SpecBuilderUtil.validateConstant("capellaForkEpoch", capellaForkEpoch); - SpecBuilderUtil.validateConstant("maxBlsToExecutionChanges", maxBlsToExecutionChanges); - SpecBuilderUtil.validateConstant("maxWithdrawalsPerPayload", maxWithdrawalsPerPayload); - SpecBuilderUtil.validateConstant( - "maxValidatorsPerWithdrawalSweep", maxValidatorsPerWithdrawalSweep); + final List> maybeErrors = new ArrayList<>(); + final Map constants = getValidationMap(); + + constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); + + final List fieldsFailingValidation = + maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); + + if (!fieldsFailingValidation.isEmpty()) { + throw new IllegalArgumentException( + String.format( + "The specified network configuration had missing or invalid values for constants %s", + String.join(", ", fieldsFailingValidation))); + } + } + + private Map getValidationMap() { + final Map constants = new HashMap<>(); + constants.put("capellaForkVersion", capellaForkVersion); + constants.put("capellaForkEpoch", capellaForkEpoch); + constants.put("maxBlsToExecutionChanges", maxBlsToExecutionChanges); + constants.put("maxWithdrawalsPerPayload", maxWithdrawalsPerPayload); + constants.put("maxValidatorsPerWithdrawalSweep", maxValidatorsPerWithdrawalSweep); + + return constants; } @Override diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/DenebBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/DenebBuilder.java index 6ac9efc48e9..f6c81d9acd6 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/DenebBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/DenebBuilder.java @@ -16,6 +16,10 @@ import static com.google.common.base.Preconditions.checkNotNull; import static tech.pegasys.teku.spec.config.SpecConfig.FAR_FUTURE_EPOCH; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.function.BiConsumer; import tech.pegasys.teku.infrastructure.bytes.Bytes4; @@ -147,23 +151,42 @@ public void validate() { SpecBuilderUtil.fillMissingValuesWithZeros(this); } - SpecBuilderUtil.validateConstant("denebForkEpoch", denebForkEpoch); - SpecBuilderUtil.validateConstant("denebForkVersion", denebForkVersion); - SpecBuilderUtil.validateConstant( - "maxPerEpochActivationChurnLimit", maxPerEpochActivationChurnLimit); - SpecBuilderUtil.validateConstant("fieldElementsPerBlob", fieldElementsPerBlob); - SpecBuilderUtil.validateConstant("maxBlobCommitmentsPerBlock", maxBlobCommitmentsPerBlock); - SpecBuilderUtil.validateConstant("maxBlobsPerBlock", maxBlobsPerBlock); - SpecBuilderUtil.validateConstant("maxRequestBlocksDeneb", maxRequestBlocksDeneb); - SpecBuilderUtil.validateConstant("maxRequestBlobSidecars", maxRequestBlobSidecars); - SpecBuilderUtil.validateConstant( - "minEpochsForBlobSidecarsRequests", minEpochsForBlobSidecarsRequests); - SpecBuilderUtil.validateConstant("blobSidecarSubnetCount", blobSidecarSubnetCount); + final List> maybeErrors = new ArrayList<>(); + final Map constants = getValidationMap(); + + constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); + + final List fieldsFailingValidation = + maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); + + if (!fieldsFailingValidation.isEmpty()) { + throw new IllegalArgumentException( + String.format( + "The specified network configuration had missing or invalid values for constants %s", + String.join(", ", fieldsFailingValidation))); + } if (!denebForkEpoch.equals(SpecConfig.FAR_FUTURE_EPOCH) && !kzgNoop) { SpecBuilderUtil.validateRequiredOptional("trustedSetupPath", trustedSetupPath); } } + private Map getValidationMap() { + final Map constants = new HashMap<>(); + + constants.put("denebForkEpoch", denebForkEpoch); + constants.put("denebForkVersion", denebForkVersion); + constants.put("maxPerEpochActivationChurnLimit", maxPerEpochActivationChurnLimit); + constants.put("fieldElementsPerBlob", fieldElementsPerBlob); + constants.put("maxBlobCommitmentsPerBlock", maxBlobCommitmentsPerBlock); + constants.put("maxBlobsPerBlock", maxBlobsPerBlock); + constants.put("maxRequestBlocksDeneb", maxRequestBlocksDeneb); + constants.put("maxRequestBlobSidecars", maxRequestBlobSidecars); + constants.put("minEpochsForBlobSidecarsRequests", minEpochsForBlobSidecarsRequests); + constants.put("blobSidecarSubnetCount", blobSidecarSubnetCount); + + return constants; + } + @Override public void addOverridableItemsToRawConfig(final BiConsumer rawConfig) { rawConfig.accept("DENEB_FORK_EPOCH", denebForkEpoch); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecBuilderUtil.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecBuilderUtil.java index 0fe90ad45d3..a47f21294df 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecBuilderUtil.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecBuilderUtil.java @@ -13,13 +13,14 @@ package tech.pegasys.teku.spec.config.builder; -import static com.google.common.base.Preconditions.checkArgument; import static tech.pegasys.teku.spec.config.SpecConfigFormatter.camelToSnakeCase; import java.lang.reflect.Modifier; import java.util.Arrays; import java.util.Map; import java.util.Optional; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.tuweni.bytes.Bytes; import org.apache.tuweni.bytes.Bytes32; import org.apache.tuweni.units.bigints.UInt256; @@ -27,6 +28,7 @@ import tech.pegasys.teku.infrastructure.unsigned.UInt64; public class SpecBuilderUtil { + private static final Logger LOG = LogManager.getLogger(); private static final Map, Object> DEFAULT_ZERO_VALUES = Map.of( UInt64.class, @@ -45,26 +47,37 @@ public class SpecBuilderUtil { // Placeholder version explicitly doesn't match MainNet (or any other known testnet) static final Bytes4 PLACEHOLDER_FORK_VERSION = Bytes4.fromHexString("0x99999999"); - static void validateConstant(final String name, final Object value) { - validateNotNull(name, value); + static Optional validateConstant(final String name, final Object value) { + return validateNotNull(name, value); } - static void validateConstant(final String name, final Long value) { - validateNotNull(name, value); - checkArgument(value >= 0, "Long values must be positive"); + static Optional validateConstant(final String name, final Integer value) { + final Optional maybeError = validateNotNull(name, value); + if (maybeError.isPresent()) { + return maybeError; + } + if (value < 0) { + LOG.error( + "Value for constant '{}' ({}) failed to validate - Integer values must be positive", + name, + value); + return Optional.of(name); + } + return Optional.empty(); } - static void validateConstant(final String name, final Integer value) { - validateNotNull(name, value); - checkArgument(value >= 0, "Integer values must be positive"); + private static Optional validateNotNull(final String name, final Object value) { + if (value == null) { + return Optional.of(camelToSnakeCase(name)); + } + return Optional.empty(); } - static void validateNotNull(final String name, final Object value) { - checkArgument(value != null, "Missing value for spec constant '%s'", camelToSnakeCase(name)); - } - - static void validateRequiredOptional(final String name, final Optional value) { - checkArgument(value.isPresent(), "Missing value for required '%s'", name); + static Optional validateRequiredOptional(final String name, final Optional value) { + if (value.isEmpty()) { + return Optional.of(name); + } + return Optional.empty(); } static void fillMissingValuesWithZeros(final ForkConfigBuilder builder) { diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java index 73367f5ca31..b141fc8cf7e 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java @@ -16,9 +16,14 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.function.Consumer; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import tech.pegasys.teku.ethereum.execution.types.Eth1Address; import tech.pegasys.teku.infrastructure.bytes.Bytes4; import tech.pegasys.teku.infrastructure.unsigned.UInt64; @@ -28,6 +33,7 @@ @SuppressWarnings({"UnusedReturnValue", "unused"}) public class SpecConfigBuilder { + private static final Logger LOG = LogManager.getLogger(); private final Map rawConfig = new HashMap<>(); // Misc @@ -202,76 +208,91 @@ public SpecConfig build() { return builderChain.build(config); } + private Map getValidationMap() { + final Map constants = new HashMap<>(); + constants.put("eth1FollowDistance", eth1FollowDistance); + constants.put("maxCommitteesPerSlot", maxCommitteesPerSlot); + constants.put("targetCommitteeSize", targetCommitteeSize); + constants.put("maxValidatorsPerCommittee", maxValidatorsPerCommittee); + constants.put("minPerEpochChurnLimit", minPerEpochChurnLimit); + constants.put("churnLimitQuotient", churnLimitQuotient); + constants.put("shuffleRoundCount", shuffleRoundCount); + constants.put("minGenesisActiveValidatorCount", minGenesisActiveValidatorCount); + constants.put("minGenesisTime", minGenesisTime); + constants.put("hysteresisQuotient", hysteresisQuotient); + constants.put("hysteresisDownwardMultiplier", hysteresisDownwardMultiplier); + constants.put("hysteresisUpwardMultiplier", hysteresisUpwardMultiplier); + constants.put("proportionalSlashingMultiplier", proportionalSlashingMultiplier); + constants.put("minDepositAmount", minDepositAmount); + constants.put("maxEffectiveBalance", maxEffectiveBalance); + constants.put("ejectionBalance", ejectionBalance); + constants.put("effectiveBalanceIncrement", effectiveBalanceIncrement); + constants.put("genesisForkVersion", genesisForkVersion); + constants.put("genesisDelay", genesisDelay); + constants.put("secondsPerSlot", secondsPerSlot); + constants.put("minAttestationInclusionDelay", minAttestationInclusionDelay); + constants.put("slotsPerEpoch", slotsPerEpoch); + constants.put("minSeedLookahead", minSeedLookahead); + constants.put("maxSeedLookahead", maxSeedLookahead); + constants.put("minEpochsToInactivityPenalty", minEpochsToInactivityPenalty); + constants.put("epochsPerEth1VotingPeriod", epochsPerEth1VotingPeriod); + constants.put("slotsPerHistoricalRoot", slotsPerHistoricalRoot); + constants.put("minValidatorWithdrawabilityDelay", minValidatorWithdrawabilityDelay); + constants.put("shardCommitteePeriod", shardCommitteePeriod); + constants.put("epochsPerHistoricalVector", epochsPerHistoricalVector); + constants.put("epochsPerSlashingsVector", epochsPerSlashingsVector); + constants.put("historicalRootsLimit", historicalRootsLimit); + constants.put("validatorRegistryLimit", validatorRegistryLimit); + constants.put("baseRewardFactor", baseRewardFactor); + constants.put("whistleblowerRewardQuotient", whistleblowerRewardQuotient); + constants.put("proposerRewardQuotient", proposerRewardQuotient); + constants.put("inactivityPenaltyQuotient", inactivityPenaltyQuotient); + constants.put("minSlashingPenaltyQuotient", minSlashingPenaltyQuotient); + constants.put("maxProposerSlashings", maxProposerSlashings); + constants.put("maxAttesterSlashings", maxAttesterSlashings); + constants.put("maxAttestations", maxAttestations); + constants.put("maxDeposits", maxDeposits); + constants.put("maxVoluntaryExits", maxVoluntaryExits); + constants.put("secondsPerEth1Block", secondsPerEth1Block); + constants.put("safeSlotsToUpdateJustified", safeSlotsToUpdateJustified); + constants.put("depositChainId", depositChainId); + constants.put("depositNetworkId", depositNetworkId); + constants.put("depositContractAddress", depositContractAddress); + + constants.put("gossipMaxSize", gossipMaxSize); + constants.put("maxChunkSize", maxChunkSize); + constants.put("maxRequestBlocks", maxRequestBlocks); + constants.put("epochsPerSubnetSubscription", epochsPerSubnetSubscription); + constants.put("minEpochsForBlockRequests", minEpochsForBlockRequests); + constants.put("ttfbTimeout", ttfbTimeout); + constants.put("respTimeout", respTimeout); + constants.put("attestationPropagationSlotRange", attestationPropagationSlotRange); + constants.put("maximumGossipClockDisparity", maximumGossipClockDisparity); + constants.put("messageDomainInvalidSnappy", messageDomainInvalidSnappy); + constants.put("messageDomainValidSnappy", messageDomainValidSnappy); + constants.put("subnetsPerNode", subnetsPerNode); + constants.put("attestationSubnetCount", attestationSubnetCount); + constants.put("attestationSubnetExtraBits", attestationSubnetExtraBits); + constants.put("attestationSubnetPrefixBits", attestationSubnetPrefixBits); + return constants; + } + private void validate() { - checkArgument(rawConfig.size() > 0, "Raw spec config must be provided"); - SpecBuilderUtil.validateConstant("eth1FollowDistance", eth1FollowDistance); - SpecBuilderUtil.validateConstant("maxCommitteesPerSlot", maxCommitteesPerSlot); - SpecBuilderUtil.validateConstant("targetCommitteeSize", targetCommitteeSize); - SpecBuilderUtil.validateConstant("maxValidatorsPerCommittee", maxValidatorsPerCommittee); - SpecBuilderUtil.validateConstant("minPerEpochChurnLimit", minPerEpochChurnLimit); - SpecBuilderUtil.validateConstant("churnLimitQuotient", churnLimitQuotient); - SpecBuilderUtil.validateConstant("shuffleRoundCount", shuffleRoundCount); - SpecBuilderUtil.validateConstant( - "minGenesisActiveValidatorCount", minGenesisActiveValidatorCount); - SpecBuilderUtil.validateConstant("minGenesisTime", minGenesisTime); - SpecBuilderUtil.validateConstant("hysteresisQuotient", hysteresisQuotient); - SpecBuilderUtil.validateConstant("hysteresisDownwardMultiplier", hysteresisDownwardMultiplier); - SpecBuilderUtil.validateConstant("hysteresisUpwardMultiplier", hysteresisUpwardMultiplier); - SpecBuilderUtil.validateConstant( - "proportionalSlashingMultiplier", proportionalSlashingMultiplier); - SpecBuilderUtil.validateConstant("minDepositAmount", minDepositAmount); - SpecBuilderUtil.validateConstant("maxEffectiveBalance", maxEffectiveBalance); - SpecBuilderUtil.validateConstant("ejectionBalance", ejectionBalance); - SpecBuilderUtil.validateConstant("effectiveBalanceIncrement", effectiveBalanceIncrement); - SpecBuilderUtil.validateConstant("genesisForkVersion", genesisForkVersion); - SpecBuilderUtil.validateConstant("genesisDelay", genesisDelay); - SpecBuilderUtil.validateConstant("secondsPerSlot", secondsPerSlot); - SpecBuilderUtil.validateConstant("minAttestationInclusionDelay", minAttestationInclusionDelay); - SpecBuilderUtil.validateConstant("slotsPerEpoch", slotsPerEpoch); - SpecBuilderUtil.validateConstant("minSeedLookahead", minSeedLookahead); - SpecBuilderUtil.validateConstant("maxSeedLookahead", maxSeedLookahead); - SpecBuilderUtil.validateConstant("minEpochsToInactivityPenalty", minEpochsToInactivityPenalty); - SpecBuilderUtil.validateConstant("epochsPerEth1VotingPeriod", epochsPerEth1VotingPeriod); - SpecBuilderUtil.validateConstant("slotsPerHistoricalRoot", slotsPerHistoricalRoot); - SpecBuilderUtil.validateConstant( - "minValidatorWithdrawabilityDelay", minValidatorWithdrawabilityDelay); - SpecBuilderUtil.validateConstant("shardCommitteePeriod", shardCommitteePeriod); - SpecBuilderUtil.validateConstant("epochsPerHistoricalVector", epochsPerHistoricalVector); - SpecBuilderUtil.validateConstant("epochsPerSlashingsVector", epochsPerSlashingsVector); - SpecBuilderUtil.validateConstant("historicalRootsLimit", historicalRootsLimit); - SpecBuilderUtil.validateConstant("validatorRegistryLimit", validatorRegistryLimit); - SpecBuilderUtil.validateConstant("baseRewardFactor", baseRewardFactor); - SpecBuilderUtil.validateConstant("whistleblowerRewardQuotient", whistleblowerRewardQuotient); - SpecBuilderUtil.validateConstant("proposerRewardQuotient", proposerRewardQuotient); - SpecBuilderUtil.validateConstant("inactivityPenaltyQuotient", inactivityPenaltyQuotient); - SpecBuilderUtil.validateConstant("minSlashingPenaltyQuotient", minSlashingPenaltyQuotient); - SpecBuilderUtil.validateConstant("maxProposerSlashings", maxProposerSlashings); - SpecBuilderUtil.validateConstant("maxAttesterSlashings", maxAttesterSlashings); - SpecBuilderUtil.validateConstant("maxAttestations", maxAttestations); - SpecBuilderUtil.validateConstant("maxDeposits", maxDeposits); - SpecBuilderUtil.validateConstant("maxVoluntaryExits", maxVoluntaryExits); - SpecBuilderUtil.validateConstant("secondsPerEth1Block", secondsPerEth1Block); - SpecBuilderUtil.validateConstant("safeSlotsToUpdateJustified", safeSlotsToUpdateJustified); - SpecBuilderUtil.validateConstant("depositChainId", depositChainId); - SpecBuilderUtil.validateConstant("depositNetworkId", depositNetworkId); - SpecBuilderUtil.validateConstant("depositContractAddress", depositContractAddress); - - SpecBuilderUtil.validateConstant("gossipMaxSize", gossipMaxSize); - SpecBuilderUtil.validateConstant("maxChunkSize", maxChunkSize); - SpecBuilderUtil.validateConstant("maxRequestBlocks", maxRequestBlocks); - SpecBuilderUtil.validateConstant("epochsPerSubnetSubscription", epochsPerSubnetSubscription); - SpecBuilderUtil.validateConstant("minEpochsForBlockRequests", minEpochsForBlockRequests); - SpecBuilderUtil.validateConstant("ttfbTimeout", ttfbTimeout); - SpecBuilderUtil.validateConstant("respTimeout", respTimeout); - SpecBuilderUtil.validateConstant( - "attestationPropagationSlotRange", attestationPropagationSlotRange); - SpecBuilderUtil.validateConstant("maximumGossipClockDisparity", maximumGossipClockDisparity); - SpecBuilderUtil.validateConstant("messageDomainInvalidSnappy", messageDomainInvalidSnappy); - SpecBuilderUtil.validateConstant("messageDomainValidSnappy", messageDomainValidSnappy); - SpecBuilderUtil.validateConstant("subnetsPerNode", subnetsPerNode); - SpecBuilderUtil.validateConstant("attestationSubnetCount", attestationSubnetCount); - SpecBuilderUtil.validateConstant("attestationSubnetExtraBits", attestationSubnetExtraBits); - SpecBuilderUtil.validateConstant("attestationSubnetPrefixBits", attestationSubnetPrefixBits); + checkArgument(!rawConfig.isEmpty(), "Raw spec config must be provided"); + final List> maybeErrors = new ArrayList<>(); + final Map constants = getValidationMap(); + + constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); + + final List fieldsFailingValidation = + maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); + + if (!fieldsFailingValidation.isEmpty()) { + throw new IllegalArgumentException( + String.format( + "The specified network configuration had missing or invalid values for constants %s", + String.join(", ", fieldsFailingValidation))); + } builderChain.validate(); } diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigReaderTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigReaderTest.java index 256f5090c00..668969c5d94 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigReaderTest.java +++ b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigReaderTest.java @@ -31,7 +31,7 @@ public void read_missingConfig() { assertThatThrownBy(reader::build) .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Missing value for spec constant 'MIN_PER_EPOCH_CHURN_LIMIT'"); + .hasMessageContaining("MIN_PER_EPOCH_CHURN_LIMIT"); } @Test @@ -49,7 +49,7 @@ public void read_missingAltairConstant() { bellatrixBuilder -> bellatrixBuilder.bellatrixForkEpoch(UInt64.ZERO)))) .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Missing value for spec constant 'EPOCHS_PER_SYNC_COMMITTEE_PERIOD'"); + .hasMessageContaining("EPOCHS_PER_SYNC_COMMITTEE_PERIOD"); } @Test @@ -88,7 +88,7 @@ public void read_almostEmptyFile() { assertThatThrownBy(reader::build) .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("Missing value for spec constant"); + .hasMessageContaining("missing or invalid values for constants"); } @Test diff --git a/teku/src/test/java/tech/pegasys/teku/cli/subcommand/RemoteSpecLoaderTest.java b/teku/src/test/java/tech/pegasys/teku/cli/subcommand/RemoteSpecLoaderTest.java index 94b8caa2073..5e3839708c1 100644 --- a/teku/src/test/java/tech/pegasys/teku/cli/subcommand/RemoteSpecLoaderTest.java +++ b/teku/src/test/java/tech/pegasys/teku/cli/subcommand/RemoteSpecLoaderTest.java @@ -14,7 +14,6 @@ package tech.pegasys.teku.cli.subcommand; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import static tech.pegasys.teku.cli.subcommand.ValidatorClientCommand.DENEB_KZG_NOOP; @@ -29,7 +28,7 @@ import org.junit.jupiter.api.Test; import tech.pegasys.teku.api.ConfigProvider; import tech.pegasys.teku.api.response.v1.config.GetSpecResponse; -import tech.pegasys.teku.infrastructure.exceptions.InvalidConfigurationException; +import tech.pegasys.teku.infrastructure.bytes.Bytes4; import tech.pegasys.teku.infrastructure.unsigned.UInt64; import tech.pegasys.teku.spec.Spec; import tech.pegasys.teku.spec.TestSpecFactory; @@ -60,9 +59,9 @@ void shouldFillWhenRequiredItemsAreMissing() { when(apiClient.getConfigSpec()).thenReturn(Optional.of(new GetSpecResponse(rawConfig))); - assertThatThrownBy(() -> RemoteSpecLoader.getSpec(apiClient, modifier -> {})) - .isInstanceOf(InvalidConfigurationException.class) - .hasMessageContaining("GENESIS_FORK_VERSION"); + final SpecConfig config = + RemoteSpecLoader.getSpec(apiClient, modifier -> {}).getSpecConfig(UInt64.ONE); + assertThat(config.getGenesisForkVersion()).isEqualTo(Bytes4.fromHexString("0x00000001")); } @Test From 84e09c33ea2d0505ab0dfcb101edfed0cf732177 Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Thu, 5 Oct 2023 11:07:20 +1000 Subject: [PATCH 6/7] changelog. Signed-off-by: Paul Harris --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c5842c5a32..4a6f0977b0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,4 +23,4 @@ the [releases page](https://github.com/Consensys/teku/releases). been updated to support older GLIBC versions (ie Ubuntu 20.04 and Debian 11). ### Bug Fixes -- Fixed an issue in configuration loading, where network attributes moved from constants were stopping old configuration from loading. +- During network configuration load, all missing fields will now be reported, rather than just the first missing field causing failure. From 9da3e572810ad333179fa35d23cce2536ddf50c9 Mon Sep 17 00:00:00 2001 From: Paul Harris Date: Mon, 9 Oct 2023 11:33:31 +1000 Subject: [PATCH 7/7] review feedback. Also improved a test case to allow us to see multiple fields returned (without listing the actual fields) Signed-off-by: Paul Harris --- .../spec/config/builder/AltairBuilder.java | 21 ++---------- .../spec/config/builder/BellatrixBuilder.java | 34 ++----------------- .../spec/config/builder/BuilderChain.java | 11 ++++++ .../spec/config/builder/CapellaBuilder.java | 21 ++---------- .../spec/config/builder/DenebBuilder.java | 19 ++--------- .../config/builder/ForkConfigBuilder.java | 23 +++++++++++++ .../spec/config/SpecConfigReaderTest.java | 15 ++++++-- 7 files changed, 58 insertions(+), 86 deletions(-) diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/AltairBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/AltairBuilder.java index f306864a393..bd30f3552b9 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/AltairBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/AltairBuilder.java @@ -16,11 +16,8 @@ import static com.google.common.base.Preconditions.checkNotNull; import static tech.pegasys.teku.spec.config.SpecConfig.FAR_FUTURE_EPOCH; -import java.util.ArrayList; import java.util.HashMap; -import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.function.BiConsumer; import tech.pegasys.teku.infrastructure.bytes.Bytes4; import tech.pegasys.teku.infrastructure.unsigned.UInt64; @@ -100,20 +97,7 @@ public void validate() { SpecBuilderUtil.fillMissingValuesWithZeros(this); } - final List> maybeErrors = new ArrayList<>(); - final Map constants = getValidationMap(); - - constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); - - final List fieldsFailingValidation = - maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); - - if (!fieldsFailingValidation.isEmpty()) { - throw new IllegalArgumentException( - String.format( - "Errors were found validating configuration, missing or invalid values for constants %s", - String.join(", ", fieldsFailingValidation))); - } + validateConstants(); // Config items were added after launch so provide defaults to preserve compatibility if (updateTimeout == null) { @@ -121,7 +105,8 @@ public void validate() { } } - private Map getValidationMap() { + @Override + public Map getValidationMap() { final Map constants = new HashMap<>(); constants.put("inactivityPenaltyQuotientAltair", inactivityPenaltyQuotientAltair); constants.put("minSlashingPenaltyQuotientAltair", minSlashingPenaltyQuotientAltair); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BellatrixBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BellatrixBuilder.java index 5d0cf0b8a1c..e6d6bc53c07 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BellatrixBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BellatrixBuilder.java @@ -18,11 +18,8 @@ import static tech.pegasys.teku.spec.constants.NetworkConstants.DEFAULT_SAFE_SLOTS_TO_IMPORT_OPTIMISTICALLY; import java.math.BigInteger; -import java.util.ArrayList; import java.util.HashMap; -import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.function.BiConsumer; import org.apache.tuweni.bytes.Bytes32; import org.apache.tuweni.units.bigints.UInt256; @@ -101,36 +98,11 @@ public void validate() { SpecBuilderUtil.fillMissingValuesWithZeros(this); } - SpecBuilderUtil.validateConstant("bellatrixForkVersion", bellatrixForkVersion); - SpecBuilderUtil.validateConstant("bellatrixForkEpoch", bellatrixForkEpoch); - SpecBuilderUtil.validateConstant( - "inactivityPenaltyQuotientBellatrix", inactivityPenaltyQuotientBellatrix); - SpecBuilderUtil.validateConstant( - "minSlashingPenaltyQuotientBellatrix", minSlashingPenaltyQuotientBellatrix); - SpecBuilderUtil.validateConstant( - "proportionalSlashingMultiplierBellatrix", proportionalSlashingMultiplierBellatrix); - SpecBuilderUtil.validateConstant("maxBytesPerTransaction", maxBytesPerTransaction); - SpecBuilderUtil.validateConstant("maxTransactionsPerPayload", maxTransactionsPerPayload); - SpecBuilderUtil.validateConstant("bytesPerLogsBloom", bytesPerLogsBloom); - SpecBuilderUtil.validateConstant("maxExtraDataBytes", maxExtraDataBytes); - - final List> maybeErrors = new ArrayList<>(); - final Map constants = getValidationMap(); - - constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); - - final List fieldsFailingValidation = - maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); - - if (!fieldsFailingValidation.isEmpty()) { - throw new IllegalArgumentException( - String.format( - "The specified network configuration had missing or invalid values for constants %s", - String.join(", ", fieldsFailingValidation))); - } + validateConstants(); } - private Map getValidationMap() { + @Override + public Map getValidationMap() { final Map constants = new HashMap<>(); constants.put("bellatrixForkVersion", bellatrixForkVersion); constants.put("bellatrixForkEpoch", bellatrixForkEpoch); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BuilderChain.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BuilderChain.java index e705e453337..7e2d99a6817 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BuilderChain.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/BuilderChain.java @@ -13,6 +13,7 @@ package tech.pegasys.teku.spec.config.builder; +import java.util.Map; import java.util.function.BiConsumer; import java.util.function.Consumer; import tech.pegasys.teku.spec.config.SpecConfig; @@ -86,6 +87,11 @@ public void validate() { tail.validate(); } + @Override + public Map getValidationMap() { + return Map.of(); + } + private static class NoOpForkBuilder implements ForkConfigBuilder { @Override @@ -96,6 +102,11 @@ public T build(final T specConfig) { @Override public void validate() {} + @Override + public Map getValidationMap() { + return Map.of(); + } + @Override public void addOverridableItemsToRawConfig(final BiConsumer rawConfig) {} } diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/CapellaBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/CapellaBuilder.java index 88eabc0ed02..5d8736dd9f2 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/CapellaBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/CapellaBuilder.java @@ -16,11 +16,8 @@ import static com.google.common.base.Preconditions.checkNotNull; import static tech.pegasys.teku.spec.config.SpecConfig.FAR_FUTURE_EPOCH; -import java.util.ArrayList; import java.util.HashMap; -import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.function.BiConsumer; import tech.pegasys.teku.infrastructure.bytes.Bytes4; import tech.pegasys.teku.infrastructure.unsigned.UInt64; @@ -98,23 +95,11 @@ public void validate() { SpecBuilderUtil.fillMissingValuesWithZeros(this); } - final List> maybeErrors = new ArrayList<>(); - final Map constants = getValidationMap(); - - constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); - - final List fieldsFailingValidation = - maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); - - if (!fieldsFailingValidation.isEmpty()) { - throw new IllegalArgumentException( - String.format( - "The specified network configuration had missing or invalid values for constants %s", - String.join(", ", fieldsFailingValidation))); - } + validateConstants(); } - private Map getValidationMap() { + @Override + public Map getValidationMap() { final Map constants = new HashMap<>(); constants.put("capellaForkVersion", capellaForkVersion); constants.put("capellaForkEpoch", capellaForkEpoch); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/DenebBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/DenebBuilder.java index f6c81d9acd6..058d1649fe0 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/DenebBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/DenebBuilder.java @@ -16,9 +16,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static tech.pegasys.teku.spec.config.SpecConfig.FAR_FUTURE_EPOCH; -import java.util.ArrayList; import java.util.HashMap; -import java.util.List; import java.util.Map; import java.util.Optional; import java.util.function.BiConsumer; @@ -151,26 +149,15 @@ public void validate() { SpecBuilderUtil.fillMissingValuesWithZeros(this); } - final List> maybeErrors = new ArrayList<>(); - final Map constants = getValidationMap(); + validateConstants(); - constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); - - final List fieldsFailingValidation = - maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); - - if (!fieldsFailingValidation.isEmpty()) { - throw new IllegalArgumentException( - String.format( - "The specified network configuration had missing or invalid values for constants %s", - String.join(", ", fieldsFailingValidation))); - } if (!denebForkEpoch.equals(SpecConfig.FAR_FUTURE_EPOCH) && !kzgNoop) { SpecBuilderUtil.validateRequiredOptional("trustedSetupPath", trustedSetupPath); } } - private Map getValidationMap() { + @Override + public Map getValidationMap() { final Map constants = new HashMap<>(); constants.put("denebForkEpoch", denebForkEpoch); diff --git a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/ForkConfigBuilder.java b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/ForkConfigBuilder.java index e04ecadfc1a..d811492d378 100644 --- a/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/ForkConfigBuilder.java +++ b/ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/ForkConfigBuilder.java @@ -13,6 +13,10 @@ package tech.pegasys.teku.spec.config.builder; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Optional; import java.util.function.BiConsumer; import tech.pegasys.teku.spec.config.SpecConfig; @@ -22,5 +26,24 @@ interface ForkConfigBuilder getValidationMap(); + + default void validateConstants() { + final List> maybeErrors = new ArrayList<>(); + final Map constants = getValidationMap(); + + constants.forEach((k, v) -> maybeErrors.add(SpecBuilderUtil.validateConstant(k, v))); + + final List fieldsFailingValidation = + maybeErrors.stream().filter(Optional::isPresent).map(Optional::get).toList(); + + if (!fieldsFailingValidation.isEmpty()) { + throw new IllegalArgumentException( + String.format( + "The specified network configuration had missing or invalid values for constants %s", + String.join(", ", fieldsFailingValidation))); + } + } + void addOverridableItemsToRawConfig(BiConsumer rawConfig); } diff --git a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigReaderTest.java b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigReaderTest.java index 668969c5d94..a2c0c58aeca 100644 --- a/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigReaderTest.java +++ b/ethereum/spec/src/test/java/tech/pegasys/teku/spec/config/SpecConfigReaderTest.java @@ -14,6 +14,7 @@ package tech.pegasys.teku.spec.config; import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy; +import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat; import static tech.pegasys.teku.spec.config.SpecConfigAssertions.assertAllAltairFieldsSet; import java.io.IOException; @@ -86,9 +87,17 @@ public void read_emptyFile() { public void read_almostEmptyFile() { processFileAsInputStream(getInvalidConfigPath("almostEmpty"), this::readConfig); - assertThatThrownBy(reader::build) - .isInstanceOf(IllegalArgumentException.class) - .hasMessageContaining("missing or invalid values for constants"); + try { + reader.build(); + Assertions.fail("Should have received an exception"); + } catch (IllegalArgumentException e) { + final String message = e.getMessage(); + assertThat(message).contains("missing or invalid values for constants"); + // this message contains a number of items separated by ", ". + // If there's only one field, then this test will return 1 element, but it should return all + // missing fields. + assertThat(message.split(", ")).hasSizeGreaterThan(1); + } } @Test