Skip to content

Commit

Permalink
Defaulted config attributes that were moved from constants
Browse files Browse the repository at this point in the history 
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 Consensys#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 Consensys#7555

Signed-off-by: Paul Harris <paul.harris@consensys.net>
  • Loading branch information
@rolfyone
rolfyone committed Oct 2, 2023
1 parent 98fd1d5 commit 79ca86d
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 36 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
15 changes: 0 additions & 15 deletions ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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, __ -> {});
}
Expand All @@ -62,17 +58,6 @@ public static SpecConfig loadConfig(
public static SpecConfig loadRemoteConfig(
final Map<String, String> config, final Consumer<SpecConfigBuilder> 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));
Expand Down
2 changes: 1 addition & 1 deletion ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/SpecConfigReader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> KEYS_TO_IGNORE =
ImmutableSet.of(
PRESET_KEY,
Expand Down
48 changes: 33 additions & 15 deletions ethereum/spec/src/main/java/tech/pegasys/teku/spec/config/builder/SpecConfigBuilder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -28,7 +30,7 @@

@SuppressWarnings({"UnusedReturnValue", "unused"})
public class SpecConfigBuilder {

private static final Logger LOG = LogManager.getLogger();
private final Map<String, Object> rawConfig = new HashMap<>();

// Misc
Expand Down Expand Up @@ -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<SpecConfig, SpecConfigDeneb> builderChain =
BuilderChain.create(new AltairBuilder())
Expand Down Expand Up @@ -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;
}
Expand Down
11 changes: 6 additions & 5 deletions teku/src/test/java/tech/pegasys/teku/cli/subcommand/RemoteSpecLoaderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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"),
Expand Down

0 comments on commit 79ca86d

Please sign in to comment.