Skip EL part of graffiti when EL info not available

beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/GraffitiBuilder.java

@@ -128,16 +128,18 @@ protected Bytes32 joinNonEmpty(final String delimiter, final String... parts) {
 
   @VisibleForTesting
   protected String formatClientsInfo(final int length) {
+    final boolean isElInfoAvailable = !ClientVersion.UNKNOWN.equals(executionClientVersion.get());
     final String safeConsensusCode = extractClientCodeSafely(consensusClientVersion);
-    final String safeExecutionCode = extractClientCodeSafely(executionClientVersion.get());
+    final String safeExecutionCode =
+        isElInfoAvailable ? extractClientCodeSafely(executionClientVersion.get()) : "";
     // LH1be52536BU0f91a674
     if (length >= 20) {
       return String.format(
           "%s%s%s%s",
           safeConsensusCode,
           consensusClientVersion.commit().toUnprefixedHexString(),
           safeExecutionCode,
-          executionClientVersion.get().commit().toUnprefixedHexString());
+          isElInfoAvailable ? executionClientVersion.get().commit().toUnprefixedHexString() : "");
     }
     // LH1be5BU0f91
     if (length >= 12) {
@@ -146,7 +148,9 @@ protected String formatClientsInfo(final int length) {
           safeConsensusCode,
           consensusClientVersion.commit().toUnprefixedHexString().substring(0, 4),
           safeExecutionCode,
-          executionClientVersion.get().commit().toUnprefixedHexString().substring(0, 4));
+          isElInfoAvailable
+              ? executionClientVersion.get().commit().toUnprefixedHexString().substring(0, 4)
+              : "");
     }
     // LH1bBU0f
     if (length >= 8) {
@@ -155,11 +159,13 @@ protected String formatClientsInfo(final int length) {
           safeConsensusCode,
           consensusClientVersion.commit().toUnprefixedHexString().substring(0, 2),
           safeExecutionCode,
-          executionClientVersion.get().commit().toUnprefixedHexString().substring(0, 2));
+          isElInfoAvailable
+              ? executionClientVersion.get().commit().toUnprefixedHexString().substring(0, 2)
+              : "");
     }
     // LHBU
     if (length >= 4) {
-      return String.format("%s%s", safeConsensusCode, safeExecutionCode);
+      return String.format("%s%s", safeConsensusCode, isElInfoAvailable ? safeExecutionCode : "");
     }
 
     return "";

beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/GraffitiBuilderTest.java

@@ -101,6 +101,17 @@ protected int calculateGraffitiLength(Optional<Bytes32> graffiti) {
     assertThat(graffitiBuilder.buildGraffiti(Optional.of(graffiti))).isEqualTo(graffiti);
   }
 
+  @Test
+  public void buildGraffiti_shouldPreferCallInput() {
+    final Bytes32 defaultGraffiti = Bytes32Parser.toBytes32(asciiGraffiti32);
+    final Bytes32 userGraffiti = Bytes32Parser.toBytes32(ASCII_GRAFFITI_20);
+    final Bytes32 expectedGraffiti = Bytes32Parser.toBytes32(ASCII_GRAFFITI_20 + " TK");
+    this.graffitiBuilder = new GraffitiBuilder(CLIENT_CODES, Optional.of(defaultGraffiti));
+    graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
+    assertThat(graffitiBuilder.buildGraffiti(Optional.of(userGraffiti)))
+        .isEqualTo(expectedGraffiti);
+  }
+
   @ParameterizedTest(name = "format={0}, userGraffiti={1}")
   @MethodSource("getBuildGraffitiFixtures")
   public void buildGraffiti_shouldProvideCorrectOutput(
@@ -122,6 +133,27 @@ public void buildGraffiti_shouldProvideCorrectOutput(
         .isEqualTo(expectedGraffitiBytes);
   }
 
+  @ParameterizedTest(name = "format={0}, userGraffiti={1}")
+  @MethodSource("getBuildGraffitiFixturesElInfoNa")
+  public void buildGraffiti_shouldProvideCorrectOutput_whenElInfoNa(
+      final ClientGraffitiAppendFormat clientGraffitiAppendFormat,
+      final Optional<String> maybeUserGraffiti,
+      final String expectedGraffiti) {
+    this.graffitiBuilder = new GraffitiBuilder(clientGraffitiAppendFormat, userGraffiti);
+    graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
+    final Bytes32 expectedGraffitiBytes = Bytes32Parser.toBytes32(expectedGraffiti);
+    assertThat(
+            new String(
+                Arrays.copyOfRange(
+                    expectedGraffitiBytes.toArray(),
+                    0,
+                    32 - expectedGraffitiBytes.numberOfTrailingZeroBytes()),
+                StandardCharsets.UTF_8))
+        .isEqualTo(expectedGraffiti);
+    assertThat(graffitiBuilder.buildGraffiti(maybeUserGraffiti.map(Bytes32Parser::toBytes32)))
+        .isEqualTo(expectedGraffitiBytes);
+  }
+
   @Test
   public void extractGraffiti_shouldReturnEmptyString() {
     assertThat(graffitiBuilder.extractGraffiti(Optional.empty(), 0)).isEqualTo("");
@@ -290,7 +322,7 @@ public void formatClientInfo_shouldRenderClientNames() {
   }
 
   @Test
-  public void formatClientInfo_shouldSkipClientsInfoWhenNotEnoughSpace() {
+  public void formatClientInfo_shouldSkipClientsInfo_whenNotEnoughSpace() {
     graffitiBuilder.onExecutionClientVersion(BESU_CLIENT_VERSION);
 
     // Empty
@@ -305,6 +337,84 @@ public void formatClientInfo_shouldSkipClientsInfoWhenNotEnoughSpace() {
         .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isEqualTo(0));
   }
 
+  @Test
+  public void formatClientInfo_shouldRenderClClientNameAndFullCommit_whenElInfoNotAvailable() {
+    graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
+
+    // 20: LH1be52536BU0f91a674
+    assertThat(graffitiBuilder.formatClientsInfo(30))
+        .isEqualTo(
+            TEKU_CLIENT_VERSION.code() + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString())
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(20));
+    assertThat(graffitiBuilder.formatClientsInfo(20))
+        .isEqualTo(
+            TEKU_CLIENT_VERSION.code() + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString())
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(20));
+  }
+
+  @Test
+  public void formatClientInfo_shouldRenderClClientNameAndHalfCommit_whenElInfoNotAvailable() {
+    graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
+
+    // 12: LH1be5BU0f91
+    assertThat(graffitiBuilder.formatClientsInfo(19))
+        .isEqualTo(
+            TEKU_CLIENT_VERSION.code()
+                + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 4))
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(12));
+    assertThat(graffitiBuilder.formatClientsInfo(12))
+        .isEqualTo(
+            TEKU_CLIENT_VERSION.code()
+                + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 4))
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(12));
+  }
+
+  @Test
+  public void formatClientInfo_shouldRenderClClientNameAnd1stCommitByte_whenElInfoNotAvailable() {
+    graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
+
+    // 8: LH1bBU0f
+    assertThat(graffitiBuilder.formatClientsInfo(11))
+        .isEqualTo(
+            TEKU_CLIENT_VERSION.code()
+                + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 2))
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(8));
+    assertThat(graffitiBuilder.formatClientsInfo(8))
+        .isEqualTo(
+            TEKU_CLIENT_VERSION.code()
+                + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 2))
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(8));
+  }
+
+  @Test
+  public void formatClientInfo_shouldRenderClClientName_whenElInfoNotAvailable() {
+    graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
+
+    // 4: LHBU
+    assertThat(graffitiBuilder.formatClientsInfo(7))
+        .isEqualTo(TEKU_CLIENT_VERSION.code())
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(4));
+    assertThat(graffitiBuilder.formatClientsInfo(4))
+        .isEqualTo(TEKU_CLIENT_VERSION.code())
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(4));
+  }
+
+  @Test
+  public void formatClientInfo_shouldSkipClientsInfo_whenNotEnoughSpaceAndElInfoNotAvailable() {
+    graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
+
+    // Empty
+    assertThat(graffitiBuilder.formatClientsInfo(3))
+        .isEqualTo("")
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isEqualTo(0));
+    assertThat(graffitiBuilder.formatClientsInfo(0))
+        .isEqualTo("")
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isEqualTo(0));
+    assertThat(graffitiBuilder.formatClientsInfo(-1))
+        .isEqualTo("")
+        .satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isEqualTo(0));
+  }
+
   @ParameterizedTest(name = "code={0}")
   @MethodSource("getClientCodes")
   public void formatClientInfo_shouldHandleBadCodeOnClientNamesAndFullCommit(
@@ -452,4 +562,46 @@ private static Stream<Arguments> getBuildGraffitiFixtures() {
         Arguments.of(DISABLED, Optional.of(UTF_8_GRAFFITI_4), UTF_8_GRAFFITI_4),
         Arguments.of(DISABLED, Optional.of(ASCII_GRAFFITI_20), ASCII_GRAFFITI_20));
   }
+
+  private static Stream<Arguments> getBuildGraffitiFixturesElInfoNa() {
+    return Stream.of(
+        Arguments.of(
+            AUTO,
+            Optional.empty(),
+            TEKU_CLIENT_VERSION.code() + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString()),
+        Arguments.of(
+            AUTO,
+            Optional.of("small"),
+            "small "
+                + TEKU_CLIENT_VERSION.code()
+                + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString()),
+        Arguments.of(
+            AUTO,
+            Optional.of(UTF_8_GRAFFITI_4),
+            UTF_8_GRAFFITI_4
+                + " "
+                + TEKU_CLIENT_VERSION.code()
+                + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString()),
+        Arguments.of(
+            AUTO,
+            Optional.of(ASCII_GRAFFITI_20),
+            ASCII_GRAFFITI_20
+                + " "
+                + TEKU_CLIENT_VERSION.code()
+                + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 2)),
+        Arguments.of(CLIENT_CODES, Optional.empty(), TEKU_CLIENT_VERSION.code()),
+        Arguments.of(CLIENT_CODES, Optional.of("small"), "small " + TEKU_CLIENT_VERSION.code()),
+        Arguments.of(
+            CLIENT_CODES,
+            Optional.of(UTF_8_GRAFFITI_4),
+            UTF_8_GRAFFITI_4 + " " + TEKU_CLIENT_VERSION.code()),
+        Arguments.of(
+            CLIENT_CODES,
+            Optional.of(ASCII_GRAFFITI_20),
+            ASCII_GRAFFITI_20 + " " + TEKU_CLIENT_VERSION.code()),
+        Arguments.of(DISABLED, Optional.empty(), ""),
+        Arguments.of(DISABLED, Optional.of("small"), "small"),
+        Arguments.of(DISABLED, Optional.of(UTF_8_GRAFFITI_4), UTF_8_GRAFFITI_4),
+        Arguments.of(DISABLED, Optional.of(ASCII_GRAFFITI_20), ASCII_GRAFFITI_20));
+  }
 }

ethereum/executionclient/src/main/java/tech/pegasys/teku/ethereum/executionclient/ExecutionClientVersionProvider.java

@@ -70,16 +70,26 @@ private void updateClientInfo() {
               final ClientVersion executionClientVersion = clientVersions.get(0);
               updateVersionIfNeeded(executionClientVersion);
             })
-        .finish(ex -> LOG.debug("Exception while calling engine_getClientVersion", ex));
+        .finish(
+            ex -> {
+              LOG.debug("Exception while calling engine_getClientVersion", ex);
+              updateVersionIfNeeded(ClientVersion.UNKNOWN);
+            });
   }
 
   private synchronized void updateVersionIfNeeded(final ClientVersion executionClientVersion) {
     if (executionClientVersion.equals(this.executionClientVersion.get())) {
       return;
     }
-    EVENT_LOG.logExecutionClientVersion(
-        executionClientVersion.name(), executionClientVersion.version());
-    this.executionClientVersion.set(executionClientVersion);
-    executionClientVersionChannel.onExecutionClientVersion(executionClientVersion);
+    if (!executionClientVersion.equals(ClientVersion.UNKNOWN)) {
+      EVENT_LOG.logExecutionClientVersion(
+          executionClientVersion.name(), executionClientVersion.version());
+    }
+    // push UNKNOWN forward only when it's set for the first time
+    if (!executionClientVersion.equals(ClientVersion.UNKNOWN)
+        || this.executionClientVersion.get() == null) {
+      this.executionClientVersion.set(executionClientVersion);
+      executionClientVersionChannel.onExecutionClientVersion(executionClientVersion);
+    }
   }
 }

ethereum/executionclient/src/test/java/tech/pegasys/teku/ethereum/executionclient/ExecutionClientVersionProviderTest.java

@@ -16,6 +16,7 @@
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
@@ -43,28 +44,29 @@ public void setUp() {
   }
 
   @Test
-  public void doesNotPublishExecutionClientVersionIfFailed() {
+  public void pushUnknownExecutionClientVersionInChannel_whenFailed() {
     when(executionLayerChannel.engineGetClientVersion(any()))
         .thenReturn(SafeFuture.failedFuture(new IllegalStateException("oopsy")));
 
     new ExecutionClientVersionProvider(
         executionLayerChannel, publishChannel, ClientVersion.UNKNOWN);
-    verify(publishChannel, never()).onExecutionClientVersion(any());
+    verify(publishChannel).onExecutionClientVersion(ClientVersion.UNKNOWN);
   }
 
   @Test
-  public void doesNotTryToUpdateExecutionClientVersionIfElHasNotBeenUnavailable() {
+  public void doesNotTryToUpdateExecutionClientVersion_whenElHasNotBeenUnavailable() {
     final ExecutionClientVersionProvider executionClientVersionProvider =
         new ExecutionClientVersionProvider(
             executionLayerChannel, publishChannel, ClientVersion.UNKNOWN);
 
     executionClientVersionProvider.onAvailabilityUpdated(true);
     // EL called only one time
-    verify(executionLayerChannel, times(1)).engineGetClientVersion(any());
+    verify(executionLayerChannel).engineGetClientVersion(any());
+    verify(publishChannel).onExecutionClientVersion(executionClientVersion);
   }
 
   @Test
-  public void updatesExecutionClientVersionWhenElIsAvailableAfterBeingUnavailable() {
+  public void updatesExecutionClientVersion_whenElIsAvailableAfterBeingUnavailable() {
     final ExecutionClientVersionProvider executionClientVersionProvider =
         new ExecutionClientVersionProvider(
             executionLayerChannel, publishChannel, ClientVersion.UNKNOWN);
@@ -93,4 +95,37 @@ public void updatesExecutionClientVersionWhenElIsAvailableAfterBeingUnavailable(
     // ignoring the same
     verify(publishChannel, times(2)).onExecutionClientVersion(any());
   }
+
+  @Test
+  public void doesNotPushUnknownVersionInChannel_whenELIsDownInTheMiddle() {
+    final ExecutionClientVersionProvider executionClientVersionProvider =
+        new ExecutionClientVersionProvider(
+            executionLayerChannel, publishChannel, ClientVersion.UNKNOWN);
+
+    // Good start
+    verify(publishChannel).onExecutionClientVersion(executionClientVersion);
+    reset(publishChannel);
+
+    // EL is broken
+    when(executionLayerChannel.engineGetClientVersion(any()))
+        .thenReturn(SafeFuture.failedFuture(new IllegalStateException("oopsy")));
+
+    executionClientVersionProvider.onAvailabilityUpdated(false);
+    executionClientVersionProvider.onAvailabilityUpdated(true);
+    // EL called two times
+    verify(executionLayerChannel, times(2)).engineGetClientVersion(any());
+    // UNKNOWN version is not pushed in the channel
+    verify(publishChannel, never()).onExecutionClientVersion(any());
+
+    // EL is back
+    when(executionLayerChannel.engineGetClientVersion(any()))
+        .thenReturn(SafeFuture.completedFuture(List.of(executionClientVersion)));
+
+    executionClientVersionProvider.onAvailabilityUpdated(false);
+    executionClientVersionProvider.onAvailabilityUpdated(true);
+    // EL called 3 times
+    verify(executionLayerChannel, times(3)).engineGetClientVersion(any());
+    // Version is the same, not pushed in the channel
+    verify(publishChannel, never()).onExecutionClientVersion(any());
+  }
 }