add engine RPC method optionality notion

Consensys · Oct 2, 2024 · caade1d · caade1d
1 parent e5529f4
commit caade1d
Show file tree

Hide file tree

Showing 9 changed files with 84 additions and 14 deletions.
diff --git a/...nt/src/main/java/tech/pegasys/teku/ethereum/executionclient/methods/EngineGetBlobsV1.java b/...nt/src/main/java/tech/pegasys/teku/ethereum/executionclient/methods/EngineGetBlobsV1.java
@@ -83,6 +83,12 @@ public SafeFuture<List<BlobAndProof>> execute(final JsonRpcRequestParams params)
                     "Response {}(blobVersionedHashes={}) -> {}",
                     getVersionedName(),
                     blobVersionedHashes,
-                    blobsAndProofs));
+                    blobsAndProofs))
+        .exceptionally(
+            throwable -> {
+              throw new UnsupportedOperationException(
+                  String.format(
+                      "Call to engineGetBlobsV1 failed. %s", throwable.getCause().getMessage()));
+            });
   }
 }
diff --git a/...src/main/java/tech/pegasys/teku/ethereum/executionclient/methods/EngineJsonRpcMethod.java b/...src/main/java/tech/pegasys/teku/ethereum/executionclient/methods/EngineJsonRpcMethod.java
@@ -28,6 +28,12 @@ default boolean isDeprecated() {
     return false;
   }
 
+  // TODO should be remove once all ELs implement engine_getBlobsV1. It has been added only to
+  // better handle the use case when the method is missing in the EL side
+  default boolean isOptional() {
+    return false;
+  }
+
   default String getVersionedName() {
     return getVersion() == 0 ? getName() : getName() + "V" + getVersion();
   }

diff --git a/...tech/pegasys/teku/ethereum/executionclient/web3j/DefaultExecutionWeb3jClientProvider.java b/...tech/pegasys/teku/ethereum/executionclient/web3j/DefaultExecutionWeb3jClientProvider.java
@@ -58,7 +58,8 @@ private synchronized void buildClient() {
             .jwtConfigOpt(jwtConfig)
             .timeProvider(timeProvider)
             .executionClientEventsPublisher(executionClientEventsPublisher)
-            .nonCriticalMethods("engine_exchangeCapabilities", "engine_getClientVersionV1")
+            .nonCriticalMethods(
+                "engine_exchangeCapabilities", "engine_getClientVersionV1", "engine_getBlobsV1")
             .build();
     this.alreadyBuilt = true;
   }

diff --git a/...rc/test/java/tech/pegasys/teku/ethereum/executionclient/methods/EngineGetBlobsV1Test.java b/...rc/test/java/tech/pegasys/teku/ethereum/executionclient/methods/EngineGetBlobsV1Test.java
@@ -29,7 +29,6 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.ethereum.executionclient.ExecutionEngineClient;
-import tech.pegasys.teku.ethereum.executionclient.response.InvalidRemoteResponseException;
 import tech.pegasys.teku.ethereum.executionclient.schema.BlobAndProofV1;
 import tech.pegasys.teku.ethereum.executionclient.schema.Response;
 import tech.pegasys.teku.infrastructure.async.SafeFuture;
@@ -99,9 +98,10 @@ public void shouldReturnFailedExecutionWhenEngineClientRequestFails() {
     assertThat(jsonRpcMethod.execute(params))
         .failsWithin(1, TimeUnit.SECONDS)
         .withThrowableOfType(ExecutionException.class)
-        .withRootCauseInstanceOf(InvalidRemoteResponseException.class)
+        .withRootCauseInstanceOf(UnsupportedOperationException.class)
         .withMessageContaining(
-            "Invalid remote response from the execution client: %s", errorResponseFromClient);
+            "Call to engineGetBlobsV1 failed. Invalid remote response from the execution client: %s",
+            errorResponseFromClient);
   }
 
   @Test

diff --git a/...er/src/main/java/tech/pegasys/teku/ethereum/executionlayer/EngineCapabilitiesMonitor.java b/...er/src/main/java/tech/pegasys/teku/ethereum/executionlayer/EngineCapabilitiesMonitor.java
@@ -18,6 +18,7 @@
 import java.util.List;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Supplier;
+import java.util.stream.Stream;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
 import tech.pegasys.teku.ethereum.events.SlotEventsChannel;
@@ -40,6 +41,7 @@ public class EngineCapabilitiesMonitor implements SlotEventsChannel {
   private final Spec spec;
   private final EventLogger eventLogger;
   private final Supplier<List<String>> capabilitiesSupplier;
+  private final Supplier<List<String>> optionalCapabilitiesSupplier;
   private final ExecutionEngineClient executionEngineClient;
 
   public EngineCapabilitiesMonitor(
@@ -51,6 +53,8 @@ public EngineCapabilitiesMonitor(
     this.eventLogger = eventLogger;
     this.capabilitiesSupplier =
         Suppliers.memoize(() -> new ArrayList<>(engineMethodsResolver.getCapabilities()));
+    this.optionalCapabilitiesSupplier =
+        Suppliers.memoize(() -> new ArrayList<>(engineMethodsResolver.getOptionalCapabilities()));
     this.executionEngineClient = executionEngineClient;
   }
 
@@ -79,18 +83,35 @@ private boolean slotIsApplicable(final UInt64 slot) {
 
   private SafeFuture<Void> monitor() {
     final List<String> capabilities = capabilitiesSupplier.get();
+    final List<String> optionalCapabilities = optionalCapabilitiesSupplier.get();
     return executionEngineClient
-        .exchangeCapabilities(capabilities)
+        .exchangeCapabilities(
+            Stream.concat(capabilities.stream(), optionalCapabilities.stream()).toList())
         .thenApply(ResponseUnwrapper::unwrapExecutionClientResponseOrThrow)
         .thenAccept(
             engineCapabilities -> {
               LOG.debug("Engine API capabilities response: " + engineCapabilities);
+
               final List<String> missingEngineCapabilities =
                   capabilities.stream()
-                      .filter(capability -> !engineCapabilities.contains(capability))
+                      .filter(
+                          capability ->
+                              !engineCapabilities.contains(capability)
+                                  && !optionalCapabilities.contains(capability))
+                      .toList();
+
+              final List<String> missingOptionalCapabilities =
+                  optionalCapabilities.stream()
+                      .filter(
+                          optionalCapability -> !engineCapabilities.contains(optionalCapability))
                       .toList();
+
               if (!missingEngineCapabilities.isEmpty()) {
-                eventLogger.missingEngineApiCapabilities(missingEngineCapabilities);
+                eventLogger.missingEngineApiCapabilities(missingEngineCapabilities, false);
+              }
+
+              if (!missingOptionalCapabilities.isEmpty()) {
+                eventLogger.missingEngineApiCapabilities(missingOptionalCapabilities, true);
               }
             });
   }

diff --git a/...src/main/java/tech/pegasys/teku/ethereum/executionlayer/EngineJsonRpcMethodsResolver.java b/...src/main/java/tech/pegasys/teku/ethereum/executionlayer/EngineJsonRpcMethodsResolver.java
@@ -34,4 +34,10 @@ <T> EngineJsonRpcMethod<List<T>> getListMethod(
    * request
    */
   Set<String> getCapabilities();
+
+  /**
+   * TODO this optionality notion should be removed once all ELs implement the engine_getBlobsV1 RPC
+   * method. It has been added to ensure a softer and better logging when the method is missing only
+   */
+  Set<String> getOptionalCapabilities();
 }
diff --git a/...tech/pegasys/teku/ethereum/executionlayer/MilestoneBasedEngineJsonRpcMethodsResolver.java b/...tech/pegasys/teku/ethereum/executionlayer/MilestoneBasedEngineJsonRpcMethodsResolver.java
@@ -165,6 +165,17 @@ public <T> EngineJsonRpcMethod<List<T>> getListMethod(
   public Set<String> getCapabilities() {
     return methodsByMilestone.values().stream()
         .flatMap(methods -> methods.values().stream())
+        .filter(method -> !method.isOptional())
+        .filter(method -> !method.isDeprecated())
+        .map(EngineJsonRpcMethod::getVersionedName)
+        .collect(Collectors.toSet());
+  }
+
+  @Override
+  public Set<String> getOptionalCapabilities() {
+    return methodsByMilestone.values().stream()
+        .flatMap(methods -> methods.values().stream())
+        .filter(EngineJsonRpcMethod::isOptional)
         .filter(method -> !method.isDeprecated())
         .map(EngineJsonRpcMethod::getVersionedName)
         .collect(Collectors.toSet());

diff --git a/...rc/test/java/tech/pegasys/teku/ethereum/executionlayer/EngineCapabilitiesMonitorTest.java b/...rc/test/java/tech/pegasys/teku/ethereum/executionlayer/EngineCapabilitiesMonitorTest.java
@@ -25,6 +25,7 @@
 
 import java.util.HashSet;
 import java.util.List;
+import java.util.stream.Stream;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import tech.pegasys.teku.ethereum.executionclient.ExecutionEngineClient;
@@ -43,14 +44,18 @@ public class EngineCapabilitiesMonitorTest {
       mock(EngineJsonRpcMethodsResolver.class);
   private final ExecutionEngineClient executionEngineClient = mock(ExecutionEngineClient.class);
 
+  private final List<String> engineCapabilities = List.of("method1", "method2", "method3");
   private final List<String> capabilities = List.of("method1", "method2");
+  private final List<String> optionalCapabilities = List.of("method3");
 
   private EngineCapabilitiesMonitor engineCapabilitiesMonitor;
 
   @BeforeEach
   public void setUp() {
     when(engineMethodsResolver.getCapabilities()).thenReturn(new HashSet<>(capabilities));
-    mockEngineCapabilitiesResponse(capabilities);
+    when(engineMethodsResolver.getOptionalCapabilities())
+        .thenReturn(new HashSet<>(optionalCapabilities));
+    mockEngineCapabilitiesResponse(engineCapabilities);
     engineCapabilitiesMonitor =
         new EngineCapabilitiesMonitor(
             spec, eventLogger, engineMethodsResolver, executionEngineClient);
@@ -64,7 +69,18 @@ public void logsWarningIfEngineDoesNotSupportCapabilities() {
     // 3rd slot in epoch
     engineCapabilitiesMonitor.onSlot(UInt64.valueOf(2));
 
-    verify(eventLogger).missingEngineApiCapabilities(List.of("method2"));
+    verify(eventLogger).missingEngineApiCapabilities(List.of("method2"), false);
+  }
+
+  @Test
+  public void logsWarningIfEngineDoesNotSupportOptionalCapabilities() {
+    // engine only supports one of the methods
+    mockEngineCapabilitiesResponse(List.of("method1", "method2"));
+
+    // 3rd slot in epoch
+    engineCapabilitiesMonitor.onSlot(UInt64.valueOf(2));
+
+    verify(eventLogger).missingEngineApiCapabilities(List.of("method3"), true);
   }
 
   @Test
@@ -129,7 +145,8 @@ public void doesNotRunMonitoringIfNotAtRequiredSlot() {
   }
 
   private void mockEngineCapabilitiesResponse(final List<String> engineCapabilities) {
-    when(executionEngineClient.exchangeCapabilities(capabilities))
+    when(executionEngineClient.exchangeCapabilities(
+            Stream.concat(capabilities.stream(), optionalCapabilities.stream()).toList()))
         .thenReturn(SafeFuture.completedFuture(new Response<>(engineCapabilities)));
   }
 

diff --git a/...structure/logging/src/main/java/tech/pegasys/teku/infrastructure/logging/EventLogger.java b/...structure/logging/src/main/java/tech/pegasys/teku/infrastructure/logging/EventLogger.java
@@ -162,11 +162,13 @@ public void executionClientRecovered() {
     info("Execution Client is responding to requests again after a previous failure", Color.GREEN);
   }
 
-  public void missingEngineApiCapabilities(final List<String> missingCapabilities) {
+  // TODO remove the isOptional param when all ELs implement the engine_getBlob
+  public void missingEngineApiCapabilities(
+      final List<String> missingCapabilities, final boolean isOptional) {
     warn(
         String.format(
-            "Execution Client does not support required Engine API methods: %s. Make sure it is upgraded to a compatible version.",
-            missingCapabilities),
+            "Execution Client does not support %s Engine API methods: %s. Make sure it is upgraded to a compatible version.",
+            isOptional ? "optional" : "required", missingCapabilities),
         Color.YELLOW);
   }