[Merge] enforce mandatory CLI params (#4746)

* enforce mandatory merge params rename validators-fee-recipient in validators-suggested-fee-recipient
Consensys · Dec 6, 2021 · 0bfcae9 · 0bfcae9
1 parent 8c51458
commit 0bfcae9
Show file tree

Hide file tree

Showing 16 changed files with 189 additions and 26 deletions.
diff --git a/services/executionengine/build.gradle b/services/executionengine/build.gradle
@@ -1,6 +1,7 @@
 dependencies {
   implementation project(':data')
   implementation project(':infrastructure:events')
+  implementation project(':infrastructure:exceptions')
   implementation project(':ethereum:executionlayer')
   implementation project(':ethereum:networks')
   implementation project(':ethereum:spec')

diff --git a/...rc/main/java/tech/pegasys/teku/services/executionengine/ExecutionEngineConfiguration.java b/...rc/main/java/tech/pegasys/teku/services/executionengine/ExecutionEngineConfiguration.java
@@ -16,7 +16,9 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 
 import java.util.Optional;
+import tech.pegasys.teku.infrastructure.exceptions.InvalidConfigurationException;
 import tech.pegasys.teku.spec.Spec;
+import tech.pegasys.teku.spec.SpecMilestone;
 
 public class ExecutionEngineConfiguration {
 
@@ -57,6 +59,10 @@ public ExecutionEngineConfiguration build() {
 
     private void validate() {
       checkNotNull(spec, "Must specify a spec");
+      if (spec.isMilestoneSupported(SpecMilestone.MERGE) && endpoint.isEmpty()) {
+        throw new InvalidConfigurationException(
+            "Invalid configuration. --ee-endpoint parameter is mandatory when Merge milestone is enabled");
+      }
     }
 
     public Builder endpoint(final String endpoint) {

diff --git a/...est/java/tech/pegasys/teku/services/executionengine/ExecutionEngineConfigurationTest.java b/...est/java/tech/pegasys/teku/services/executionengine/ExecutionEngineConfigurationTest.java
@@ -0,0 +1,52 @@
+/*
+ * Copyright 2021 ConsenSys AG.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ */
+
+package tech.pegasys.teku.services.executionengine;
+
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+import tech.pegasys.teku.infrastructure.exceptions.InvalidConfigurationException;
+import tech.pegasys.teku.spec.Spec;
+import tech.pegasys.teku.spec.TestSpecFactory;
+
+public class ExecutionEngineConfigurationTest {
+  private final ExecutionEngineConfiguration.Builder configBuilder =
+      ExecutionEngineConfiguration.builder();
+  private final Spec altairSpec = TestSpecFactory.createMinimalAltair();
+  private final Spec mergeSpec = TestSpecFactory.createMinimalMerge();
+
+  @Test
+  public void altair_noExceptionThrownIfNoEeEndpointSpecified() {
+    final ExecutionEngineConfiguration.Builder builder = configBuilder.specProvider(altairSpec);
+
+    Assertions.assertThatCode(builder::build).doesNotThrowAnyException();
+  }
+
+  @Test
+  public void merge_shouldThrowExceptionIfNoEeEndpointSpecified() {
+    final ExecutionEngineConfiguration.Builder builder = configBuilder.specProvider(mergeSpec);
+
+    Assertions.assertThatExceptionOfType(InvalidConfigurationException.class)
+        .isThrownBy(builder::build)
+        .withMessageContaining(
+            "Invalid configuration. --ee-endpoint parameter is mandatory when Merge milestone is enabled");
+  }
+
+  @Test
+  public void merge_noExceptionThrownIfEeEndpointSpecified() {
+    final ExecutionEngineConfiguration.Builder builder =
+        configBuilder.specProvider(mergeSpec).endpoint("someEndpoint");
+
+    Assertions.assertThatCode(builder::build).doesNotThrowAnyException();
+  }
+}
diff --git a/teku/src/main/java/tech/pegasys/teku/cli/options/ValidatorOptions.java b/teku/src/main/java/tech/pegasys/teku/cli/options/ValidatorOptions.java
@@ -129,13 +129,13 @@ public class ValidatorOptions {
   private List<URI> additionalPublishUrls = new ArrayList<>();
 
   @Option(
-      names = {"--Xvalidators-fee-recipient-address"},
+      names = {"--Xvalidators-suggested-fee-recipient-address"},
       paramLabel = "<ADDRESS>",
       description =
           "Suggested fee recipient sent to the execution engine, which could use it as fee recipient when producing a new execution block.",
       arity = "0..1",
       hidden = true)
-  private String feeRecipient = null;
+  private String suggestedFeeRecipient = null;
 
   public void configure(TekuConfiguration.Builder builder) {
     if (validatorPerformanceTrackingEnabled != null) {
@@ -160,7 +160,7 @@ public void configure(TekuConfiguration.Builder builder) {
                 .generateEarlyAttestations(generateEarlyAttestations)
                 .sendAttestationsAsBatch(sendAttestationsAsBatch)
                 .additionalPublishUrls(additionalPublishUrls)
-                .feeRecipient(feeRecipient));
+                .suggestedFeeRecipient(suggestedFeeRecipient));
     validatorKeysOptions.configure(builder);
   }
 }
diff --git a/teku/src/main/java/tech/pegasys/teku/config/TekuConfiguration.java b/teku/src/main/java/tech/pegasys/teku/config/TekuConfiguration.java
@@ -216,6 +216,7 @@ public TekuConfiguration build() {
       p2pConfigBuilder.specProvider(spec);
       powchainConfigBuilder.specProvider(spec);
       executionEngineConfigBuilder.specProvider(spec);
+      validatorConfigBuilder.specProvider(spec);
 
       Eth1Address depositContractAddress = eth2NetworkConfiguration.getEth1DepositContractAddress();
       Optional<UInt64> depositContractDeployBlock =

diff --git a/teku/src/test/java/tech/pegasys/teku/cli/options/Eth2NetworkOptionsTest.java b/teku/src/test/java/tech/pegasys/teku/cli/options/Eth2NetworkOptionsTest.java
@@ -56,14 +56,16 @@ void shouldUseAltairForkEpochIfSpecified() {
   @Test
   void shouldUseMergeForkEpochIfSpecified() {
     final TekuConfiguration config =
-        getTekuConfigurationFromArguments("--Xnetwork-merge-fork-epoch", "120000");
+        getTekuConfigurationFromArguments(
+            "--Xnetwork-merge-fork-epoch", "120000", "--Xee-endpoint", "someEndpoint");
     final Spec spec = config.eth2NetworkConfiguration().getSpec();
     assertThat(spec.getForkSchedule().getSpecMilestoneAtEpoch(UInt64.valueOf(119999)))
         .isEqualTo(SpecMilestone.ALTAIR);
     assertThat(spec.getForkSchedule().getSpecMilestoneAtEpoch(UInt64.valueOf(120000)))
         .isEqualTo(SpecMilestone.MERGE);
     assertThat(
             createConfigBuilder()
+                .executionEngine(b -> b.endpoint("someEndpoint"))
                 .eth2NetworkConfig(b -> b.mergeForkEpoch(UInt64.valueOf(120000)))
                 .build())
         .usingRecursiveComparison()

diff --git a/teku/src/test/java/tech/pegasys/teku/cli/options/ValidatorOptionsTest.java b/teku/src/test/java/tech/pegasys/teku/cli/options/ValidatorOptionsTest.java
@@ -150,16 +150,16 @@ void shouldDetectInvalidPublishUrl() {
   @Test
   public void ShouldReportEmptyIfFeeRecipientNotSpecified() {
     final TekuConfiguration config = getTekuConfigurationFromArguments();
-    assertThat(config.validatorClient().getValidatorConfig().getFeeRecipient()).isEmpty();
+    assertThat(config.validatorClient().getValidatorConfig().getSuggestedFeeRecipient()).isEmpty();
   }
 
   @Test
   public void ShouldReportAddressIfFeeRecipientSpecified() {
     final String[] args = {
-      "--Xvalidators-fee-recipient-address", "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73"
+      "--Xvalidators-suggested-fee-recipient-address", "0xfe3b557e8fb62b89f4916b721be55ceb828dbd73"
     };
     final TekuConfiguration config = getTekuConfigurationFromArguments(args);
-    assertThat(config.validatorClient().getValidatorConfig().getFeeRecipient())
+    assertThat(config.validatorClient().getValidatorConfig().getSuggestedFeeRecipient())
         .isEqualTo(
             Optional.of(Eth1Address.fromHexString("0xfe3b557e8fb62b89f4916b721be55ceb828dbd73")));
   }

diff --git a/validator/api/build.gradle b/validator/api/build.gradle
@@ -10,6 +10,7 @@ dependencies {
   implementation 'org.apache.tuweni:tuweni-bytes'
 
   testImplementation testFixtures(project(':bls'))
+  testImplementation testFixtures(project(':ethereum:spec'))
 }
 
 publishing {

diff --git a/validator/api/src/main/java/tech/pegasys/teku/validator/api/ValidatorConfig.java b/validator/api/src/main/java/tech/pegasys/teku/validator/api/ValidatorConfig.java
@@ -23,6 +23,8 @@
 import org.apache.commons.lang3.tuple.Pair;
 import org.apache.tuweni.bytes.Bytes32;
 import tech.pegasys.teku.infrastructure.exceptions.InvalidConfigurationException;
+import tech.pegasys.teku.spec.Spec;
+import tech.pegasys.teku.spec.SpecMilestone;
 import tech.pegasys.teku.spec.datastructures.eth1.Eth1Address;
 
 public class ValidatorConfig {
@@ -57,7 +59,7 @@ public class ValidatorConfig {
   private final boolean useDependentRoots;
   private final boolean generateEarlyAttestations;
   private final boolean sendAttestationsAsBatch;
-  private final Optional<Eth1Address> feeRecipient;
+  private final Optional<Eth1Address> suggestedFeeRecipient;
 
   private ValidatorConfig(
       final List<String> validatorKeys,
@@ -78,7 +80,7 @@ private ValidatorConfig(
       final boolean generateEarlyAttestations,
       final boolean sendAttestationsAsBatch,
       final List<URI> additionalPublishUrls,
-      final Optional<Eth1Address> feeRecipient) {
+      final Optional<Eth1Address> suggestedFeeRecipient) {
     this.validatorKeys = validatorKeys;
     this.validatorExternalSignerPublicKeySources = validatorExternalSignerPublicKeySources;
     this.validatorExternalSignerUrl = validatorExternalSignerUrl;
@@ -100,7 +102,7 @@ private ValidatorConfig(
     this.generateEarlyAttestations = generateEarlyAttestations;
     this.sendAttestationsAsBatch = sendAttestationsAsBatch;
     this.additionalPublishUrls = additionalPublishUrls;
-    this.feeRecipient = feeRecipient;
+    this.suggestedFeeRecipient = suggestedFeeRecipient;
   }
 
   public static Builder builder() {
@@ -172,12 +174,12 @@ public List<URI> getAdditionalPublishUrls() {
     return additionalPublishUrls;
   }
 
-  public Optional<Eth1Address> getFeeRecipient() {
-    return feeRecipient;
+  public Optional<Eth1Address> getSuggestedFeeRecipient() {
+    return suggestedFeeRecipient;
   }
 
   public static final class Builder {
-
+    private Spec spec;
     private List<String> validatorKeys = new ArrayList<>();
     private List<URI> additionalPublishUrls = new ArrayList<>();
     private List<String> validatorExternalSignerPublicKeySources = new ArrayList<>();
@@ -200,7 +202,7 @@ public static final class Builder {
     private boolean useDependentRoots = DEFAULT_USE_DEPENDENT_ROOTS;
     private boolean generateEarlyAttestations = DEFAULT_GENERATE_EARLY_ATTESTATIONS;
     private boolean sendAttestationsAsBatch = DEFAULT_SEND_ATTESTATIONS_AS_BATCH;
-    private Optional<Eth1Address> feeRecipient = Optional.empty();
+    private Optional<Eth1Address> suggestedFeeRecipient = Optional.empty();
 
     private Builder() {}
 
@@ -304,25 +306,31 @@ public Builder additionalPublishUrls(final List<URI> publishUrls) {
       return this;
     }
 
-    public Builder feeRecipient(final Eth1Address feeRecipient) {
-      this.feeRecipient = Optional.ofNullable(feeRecipient);
+    public Builder suggestedFeeRecipient(final Eth1Address suggestedFeeRecipient) {
+      this.suggestedFeeRecipient = Optional.ofNullable(suggestedFeeRecipient);
       return this;
     }
 
-    public Builder feeRecipient(final String feeRecipient) {
-      if (feeRecipient == null) {
-        this.feeRecipient = Optional.empty();
+    public Builder suggestedFeeRecipient(final String suggestedFeeRecipient) {
+      if (suggestedFeeRecipient == null) {
+        this.suggestedFeeRecipient = Optional.empty();
       } else {
-        this.feeRecipient = Optional.of(Eth1Address.fromHexString(feeRecipient));
+        this.suggestedFeeRecipient = Optional.of(Eth1Address.fromHexString(suggestedFeeRecipient));
       }
       return this;
     }
 
+    public Builder specProvider(final Spec spec) {
+      this.spec = spec;
+      return this;
+    }
+
     public ValidatorConfig build() {
       validateExternalSignerUrlAndPublicKeys();
       validateExternalSignerKeystoreAndPasswordFileConfig();
       validateExternalSignerTruststoreAndPasswordFileConfig();
       validateExternalSignerURLScheme();
+      validateFeeRecipient();
       return new ValidatorConfig(
           validatorKeys,
           validatorExternalSignerPublicKeySources,
@@ -342,7 +350,7 @@ public ValidatorConfig build() {
           generateEarlyAttestations,
           sendAttestationsAsBatch,
           additionalPublishUrls,
-          feeRecipient);
+          suggestedFeeRecipient);
     }
 
     private void validateExternalSignerUrlAndPublicKeys() {
@@ -391,6 +399,15 @@ private void validateExternalSignerURLScheme() {
       }
     }
 
+    private void validateFeeRecipient() {
+      if (spec.isMilestoneSupported(SpecMilestone.MERGE)
+          && suggestedFeeRecipient.isEmpty()
+          && !(validatorKeys.isEmpty() && externalPublicKeysNotDefined())) {
+        throw new InvalidConfigurationException(
+            "Invalid configuration. --validators-fee-recipient-address must be specified when Merge milestone is active");
+      }
+    }
+
     private boolean externalPublicKeysNotDefined() {
       return validatorExternalSignerPublicKeySources == null
           || validatorExternalSignerPublicKeySources.isEmpty();

diff --git a/validator/api/src/test/java/tech/pegasys/teku/validator/api/ValidatorConfigTest.java b/validator/api/src/test/java/tech/pegasys/teku/validator/api/ValidatorConfigTest.java
@@ -18,13 +18,24 @@
 import java.nio.file.Path;
 import java.util.List;
 import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.bls.BLSTestUtil;
 import tech.pegasys.teku.infrastructure.exceptions.InvalidConfigurationException;
+import tech.pegasys.teku.spec.Spec;
+import tech.pegasys.teku.spec.TestSpecFactory;
+import tech.pegasys.teku.spec.datastructures.eth1.Eth1Address;
 
 class ValidatorConfigTest {
 
   private final ValidatorConfig.Builder configBuilder = ValidatorConfig.builder();
+  private final Spec altairSpec = TestSpecFactory.createMinimalAltair();
+  private final Spec mergeSpec = TestSpecFactory.createMinimalMerge();
+
+  @BeforeEach
+  void setUp() {
+    configBuilder.specProvider(altairSpec);
+  }
 
   @Test
   public void shouldThrowExceptionIfExternalPublicKeysAreSpecifiedWithoutExternalSignerUrl() {
@@ -117,4 +128,57 @@ public void noExceptionThrownIfBothExternalSignerTruststoreAndPasswordFileAreSpe
 
     Assertions.assertThatCode(builder::build).doesNotThrowAnyException();
   }
+
+  @Test
+  public void merge_shouldThrowExceptionIfExternalSignerPublicKeySourcesIsSpecified()
+      throws MalformedURLException {
+    configBuilder.specProvider(mergeSpec);
+    final ValidatorConfig.Builder builder =
+        configBuilder
+            .validatorExternalSignerPublicKeySources(
+                List.of(BLSTestUtil.randomKeyPair(0).getPublicKey().toString()))
+            .validatorExternalSignerUrl(URI.create("http://localhost:9000").toURL());
+
+    Assertions.assertThatExceptionOfType(InvalidConfigurationException.class)
+        .isThrownBy(builder::build)
+        .withMessageContaining(
+            "Invalid configuration. --validators-fee-recipient-address must be specified when Merge milestone is active");
+  }
+
+  @Test
+  public void merge_shouldThrowExceptionIfValidatorKeysAreSpecified() throws MalformedURLException {
+    configBuilder.specProvider(mergeSpec);
+    final ValidatorConfig.Builder builder = configBuilder.validatorKeys(List.of("some string"));
+
+    Assertions.assertThatExceptionOfType(InvalidConfigurationException.class)
+        .isThrownBy(builder::build)
+        .withMessageContaining(
+            "Invalid configuration. --validators-fee-recipient-address must be specified when Merge milestone is active");
+  }
+
+  @Test
+  public void merge_noExceptionThrownIfIfExternalSignerPublicKeySourcesIsSpecified()
+      throws MalformedURLException {
+    configBuilder.specProvider(mergeSpec);
+    final ValidatorConfig.Builder builder =
+        configBuilder
+            .validatorExternalSignerPublicKeySources(
+                List.of(BLSTestUtil.randomKeyPair(0).getPublicKey().toString()))
+            .validatorExternalSignerUrl(URI.create("http://localhost:9000").toURL())
+            .suggestedFeeRecipient("0x0000000000000000000000000000000000000000");
+
+    Assertions.assertThatCode(builder::build).doesNotThrowAnyException();
+  }
+
+  @Test
+  public void merge_noExceptionThrownIfIfValidatorKeysAreSpecified() throws MalformedURLException {
+    configBuilder.specProvider(mergeSpec);
+    final ValidatorConfig.Builder builder =
+        configBuilder
+            .validatorKeys(List.of("some string"))
+            .suggestedFeeRecipient(
+                Eth1Address.fromHexString("0x0000000000000000000000000000000000000000"));
+
+    Assertions.assertThatCode(builder::build).doesNotThrowAnyException();
+  }
 }
diff --git a/...t/java/tech/pegasys/teku/validator/client/signer/ExternalSignerAltairIntegrationTest.java b/...t/java/tech/pegasys/teku/validator/client/signer/ExternalSignerAltairIntegrationTest.java
@@ -93,6 +93,7 @@ void setup(final ClientAndServer client) throws MalformedURLException {
     this.client = client;
     final ValidatorConfig config =
         ValidatorConfig.builder()
+            .specProvider(spec)
             .validatorExternalSignerPublicKeySources(List.of(KEYPAIR.getPublicKey().toString()))
             .validatorExternalSignerUrl(new URL("http://127.0.0.1:" + client.getLocalPort()))
             .validatorExternalSignerTimeout(TIMEOUT)

diff --git a/...on-test/java/tech/pegasys/teku/validator/client/signer/ExternalSignerIntegrationTest.java b/...on-test/java/tech/pegasys/teku/validator/client/signer/ExternalSignerIntegrationTest.java
@@ -79,6 +79,7 @@ void setup(final ClientAndServer client) throws MalformedURLException {
     this.client = client;
     final ValidatorConfig config =
         ValidatorConfig.builder()
+            .specProvider(spec)
             .validatorExternalSignerPublicKeySources(List.of(KEYPAIR.getPublicKey().toString()))
             .validatorExternalSignerUrl(new URL("http://127.0.0.1:" + client.getLocalPort()))
             .validatorExternalSignerTimeout(TIMEOUT)