Skip to content

Commit 78911c1

Browse files
zilm13zilm13
authored
Skip EL part of graffiti when EL info not available (Consensys#8175)
* Skip EL part of graffiti when EL info not available (previously was NA0000) * Change EL tracker to push either UNKNOWN (when failed) or valuable version
1 parent 3ae6afa commit 78911c1

File tree

4 files changed

+219
-16
lines changed

4 files changed

+219
-16
lines changed

Diff for: ‎beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/GraffitiBuilder.java

+11-5
Original file line numberDiff line numberDiff line change
@@ -128,16 +128,18 @@ protected Bytes32 joinNonEmpty(final String delimiter, final String... parts) {
128128

129129
@VisibleForTesting
130130
protected String formatClientsInfo(final int length) {
131+
final boolean isElInfoAvailable = !ClientVersion.UNKNOWN.equals(executionClientVersion.get());
131132
final String safeConsensusCode = extractClientCodeSafely(consensusClientVersion);
132-
final String safeExecutionCode = extractClientCodeSafely(executionClientVersion.get());
133+
final String safeExecutionCode =
134+
isElInfoAvailable ? extractClientCodeSafely(executionClientVersion.get()) : "";
133135
// LH1be52536BU0f91a674
134136
if (length >= 20) {
135137
return String.format(
136138
"%s%s%s%s",
137139
safeConsensusCode,
138140
consensusClientVersion.commit().toUnprefixedHexString(),
139141
safeExecutionCode,
140-
executionClientVersion.get().commit().toUnprefixedHexString());
142+
isElInfoAvailable ? executionClientVersion.get().commit().toUnprefixedHexString() : "");
141143
}
142144
// LH1be5BU0f91
143145
if (length >= 12) {
@@ -146,7 +148,9 @@ protected String formatClientsInfo(final int length) {
146148
safeConsensusCode,
147149
consensusClientVersion.commit().toUnprefixedHexString().substring(0, 4),
148150
safeExecutionCode,
149-
executionClientVersion.get().commit().toUnprefixedHexString().substring(0, 4));
151+
isElInfoAvailable
152+
? executionClientVersion.get().commit().toUnprefixedHexString().substring(0, 4)
153+
: "");
150154
}
151155
// LH1bBU0f
152156
if (length >= 8) {
@@ -155,11 +159,13 @@ protected String formatClientsInfo(final int length) {
155159
safeConsensusCode,
156160
consensusClientVersion.commit().toUnprefixedHexString().substring(0, 2),
157161
safeExecutionCode,
158-
executionClientVersion.get().commit().toUnprefixedHexString().substring(0, 2));
162+
isElInfoAvailable
163+
? executionClientVersion.get().commit().toUnprefixedHexString().substring(0, 2)
164+
: "");
159165
}
160166
// LHBU
161167
if (length >= 4) {
162-
return String.format("%s%s", safeConsensusCode, safeExecutionCode);
168+
return String.format("%s%s", safeConsensusCode, isElInfoAvailable ? safeExecutionCode : "");
163169
}
164170

165171
return "";

Diff for: ‎beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/GraffitiBuilderTest.java

+153-1
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,17 @@ protected int calculateGraffitiLength(Optional<Bytes32> graffiti) {
101101
assertThat(graffitiBuilder.buildGraffiti(Optional.of(graffiti))).isEqualTo(graffiti);
102102
}
103103

104+
@Test
105+
public void buildGraffiti_shouldPreferCallInput() {
106+
final Bytes32 defaultGraffiti = Bytes32Parser.toBytes32(asciiGraffiti32);
107+
final Bytes32 userGraffiti = Bytes32Parser.toBytes32(ASCII_GRAFFITI_20);
108+
final Bytes32 expectedGraffiti = Bytes32Parser.toBytes32(ASCII_GRAFFITI_20 + " TK");
109+
this.graffitiBuilder = new GraffitiBuilder(CLIENT_CODES, Optional.of(defaultGraffiti));
110+
graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
111+
assertThat(graffitiBuilder.buildGraffiti(Optional.of(userGraffiti)))
112+
.isEqualTo(expectedGraffiti);
113+
}
114+
104115
@ParameterizedTest(name = "format={0}, userGraffiti={1}")
105116
@MethodSource("getBuildGraffitiFixtures")
106117
public void buildGraffiti_shouldProvideCorrectOutput(
@@ -122,6 +133,27 @@ public void buildGraffiti_shouldProvideCorrectOutput(
122133
.isEqualTo(expectedGraffitiBytes);
123134
}
124135

136+
@ParameterizedTest(name = "format={0}, userGraffiti={1}")
137+
@MethodSource("getBuildGraffitiFixturesElInfoNa")
138+
public void buildGraffiti_shouldProvideCorrectOutput_whenElInfoNa(
139+
final ClientGraffitiAppendFormat clientGraffitiAppendFormat,
140+
final Optional<String> maybeUserGraffiti,
141+
final String expectedGraffiti) {
142+
this.graffitiBuilder = new GraffitiBuilder(clientGraffitiAppendFormat, userGraffiti);
143+
graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
144+
final Bytes32 expectedGraffitiBytes = Bytes32Parser.toBytes32(expectedGraffiti);
145+
assertThat(
146+
new String(
147+
Arrays.copyOfRange(
148+
expectedGraffitiBytes.toArray(),
149+
0,
150+
32 - expectedGraffitiBytes.numberOfTrailingZeroBytes()),
151+
StandardCharsets.UTF_8))
152+
.isEqualTo(expectedGraffiti);
153+
assertThat(graffitiBuilder.buildGraffiti(maybeUserGraffiti.map(Bytes32Parser::toBytes32)))
154+
.isEqualTo(expectedGraffitiBytes);
155+
}
156+
125157
@Test
126158
public void extractGraffiti_shouldReturnEmptyString() {
127159
assertThat(graffitiBuilder.extractGraffiti(Optional.empty(), 0)).isEqualTo("");
@@ -290,7 +322,7 @@ public void formatClientInfo_shouldRenderClientNames() {
290322
}
291323

292324
@Test
293-
public void formatClientInfo_shouldSkipClientsInfoWhenNotEnoughSpace() {
325+
public void formatClientInfo_shouldSkipClientsInfo_whenNotEnoughSpace() {
294326
graffitiBuilder.onExecutionClientVersion(BESU_CLIENT_VERSION);
295327

296328
// Empty
@@ -305,6 +337,84 @@ public void formatClientInfo_shouldSkipClientsInfoWhenNotEnoughSpace() {
305337
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isEqualTo(0));
306338
}
307339

340+
@Test
341+
public void formatClientInfo_shouldRenderClClientNameAndFullCommit_whenElInfoNotAvailable() {
342+
graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
343+
344+
// 20: LH1be52536BU0f91a674
345+
assertThat(graffitiBuilder.formatClientsInfo(30))
346+
.isEqualTo(
347+
TEKU_CLIENT_VERSION.code() + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString())
348+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(20));
349+
assertThat(graffitiBuilder.formatClientsInfo(20))
350+
.isEqualTo(
351+
TEKU_CLIENT_VERSION.code() + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString())
352+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(20));
353+
}
354+
355+
@Test
356+
public void formatClientInfo_shouldRenderClClientNameAndHalfCommit_whenElInfoNotAvailable() {
357+
graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
358+
359+
// 12: LH1be5BU0f91
360+
assertThat(graffitiBuilder.formatClientsInfo(19))
361+
.isEqualTo(
362+
TEKU_CLIENT_VERSION.code()
363+
+ TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 4))
364+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(12));
365+
assertThat(graffitiBuilder.formatClientsInfo(12))
366+
.isEqualTo(
367+
TEKU_CLIENT_VERSION.code()
368+
+ TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 4))
369+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(12));
370+
}
371+
372+
@Test
373+
public void formatClientInfo_shouldRenderClClientNameAnd1stCommitByte_whenElInfoNotAvailable() {
374+
graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
375+
376+
// 8: LH1bBU0f
377+
assertThat(graffitiBuilder.formatClientsInfo(11))
378+
.isEqualTo(
379+
TEKU_CLIENT_VERSION.code()
380+
+ TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 2))
381+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(8));
382+
assertThat(graffitiBuilder.formatClientsInfo(8))
383+
.isEqualTo(
384+
TEKU_CLIENT_VERSION.code()
385+
+ TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 2))
386+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(8));
387+
}
388+
389+
@Test
390+
public void formatClientInfo_shouldRenderClClientName_whenElInfoNotAvailable() {
391+
graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
392+
393+
// 4: LHBU
394+
assertThat(graffitiBuilder.formatClientsInfo(7))
395+
.isEqualTo(TEKU_CLIENT_VERSION.code())
396+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(4));
397+
assertThat(graffitiBuilder.formatClientsInfo(4))
398+
.isEqualTo(TEKU_CLIENT_VERSION.code())
399+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isLessThan(4));
400+
}
401+
402+
@Test
403+
public void formatClientInfo_shouldSkipClientsInfo_whenNotEnoughSpaceAndElInfoNotAvailable() {
404+
graffitiBuilder.onExecutionClientVersion(ClientVersion.UNKNOWN);
405+
406+
// Empty
407+
assertThat(graffitiBuilder.formatClientsInfo(3))
408+
.isEqualTo("")
409+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isEqualTo(0));
410+
assertThat(graffitiBuilder.formatClientsInfo(0))
411+
.isEqualTo("")
412+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isEqualTo(0));
413+
assertThat(graffitiBuilder.formatClientsInfo(-1))
414+
.isEqualTo("")
415+
.satisfies(s -> assertThat(s.getBytes(StandardCharsets.UTF_8).length).isEqualTo(0));
416+
}
417+
308418
@ParameterizedTest(name = "code={0}")
309419
@MethodSource("getClientCodes")
310420
public void formatClientInfo_shouldHandleBadCodeOnClientNamesAndFullCommit(
@@ -452,4 +562,46 @@ private static Stream<Arguments> getBuildGraffitiFixtures() {
452562
Arguments.of(DISABLED, Optional.of(UTF_8_GRAFFITI_4), UTF_8_GRAFFITI_4),
453563
Arguments.of(DISABLED, Optional.of(ASCII_GRAFFITI_20), ASCII_GRAFFITI_20));
454564
}
565+
566+
private static Stream<Arguments> getBuildGraffitiFixturesElInfoNa() {
567+
return Stream.of(
568+
Arguments.of(
569+
AUTO,
570+
Optional.empty(),
571+
TEKU_CLIENT_VERSION.code() + TEKU_CLIENT_VERSION.commit().toUnprefixedHexString()),
572+
Arguments.of(
573+
AUTO,
574+
Optional.of("small"),
575+
"small "
576+
+ TEKU_CLIENT_VERSION.code()
577+
+ TEKU_CLIENT_VERSION.commit().toUnprefixedHexString()),
578+
Arguments.of(
579+
AUTO,
580+
Optional.of(UTF_8_GRAFFITI_4),
581+
UTF_8_GRAFFITI_4
582+
+ " "
583+
+ TEKU_CLIENT_VERSION.code()
584+
+ TEKU_CLIENT_VERSION.commit().toUnprefixedHexString()),
585+
Arguments.of(
586+
AUTO,
587+
Optional.of(ASCII_GRAFFITI_20),
588+
ASCII_GRAFFITI_20
589+
+ " "
590+
+ TEKU_CLIENT_VERSION.code()
591+
+ TEKU_CLIENT_VERSION.commit().toUnprefixedHexString().substring(0, 2)),
592+
Arguments.of(CLIENT_CODES, Optional.empty(), TEKU_CLIENT_VERSION.code()),
593+
Arguments.of(CLIENT_CODES, Optional.of("small"), "small " + TEKU_CLIENT_VERSION.code()),
594+
Arguments.of(
595+
CLIENT_CODES,
596+
Optional.of(UTF_8_GRAFFITI_4),
597+
UTF_8_GRAFFITI_4 + " " + TEKU_CLIENT_VERSION.code()),
598+
Arguments.of(
599+
CLIENT_CODES,
600+
Optional.of(ASCII_GRAFFITI_20),
601+
ASCII_GRAFFITI_20 + " " + TEKU_CLIENT_VERSION.code()),
602+
Arguments.of(DISABLED, Optional.empty(), ""),
603+
Arguments.of(DISABLED, Optional.of("small"), "small"),
604+
Arguments.of(DISABLED, Optional.of(UTF_8_GRAFFITI_4), UTF_8_GRAFFITI_4),
605+
Arguments.of(DISABLED, Optional.of(ASCII_GRAFFITI_20), ASCII_GRAFFITI_20));
606+
}
455607
}

Diff for: ‎ethereum/executionclient/src/main/java/tech/pegasys/teku/ethereum/executionclient/ExecutionClientVersionProvider.java

+15-5
Original file line numberDiff line numberDiff line change
@@ -70,16 +70,26 @@ private void updateClientInfo() {
7070
final ClientVersion executionClientVersion = clientVersions.get(0);
7171
updateVersionIfNeeded(executionClientVersion);
7272
})
73-
.finish(ex -> LOG.debug("Exception while calling engine_getClientVersion", ex));
73+
.finish(
74+
ex -> {
75+
LOG.debug("Exception while calling engine_getClientVersion", ex);
76+
updateVersionIfNeeded(ClientVersion.UNKNOWN);
77+
});
7478
}
7579

7680
private synchronized void updateVersionIfNeeded(final ClientVersion executionClientVersion) {
7781
if (executionClientVersion.equals(this.executionClientVersion.get())) {
7882
return;
7983
}
80-
EVENT_LOG.logExecutionClientVersion(
81-
executionClientVersion.name(), executionClientVersion.version());
82-
this.executionClientVersion.set(executionClientVersion);
83-
executionClientVersionChannel.onExecutionClientVersion(executionClientVersion);
84+
if (!executionClientVersion.equals(ClientVersion.UNKNOWN)) {
85+
EVENT_LOG.logExecutionClientVersion(
86+
executionClientVersion.name(), executionClientVersion.version());
87+
}
88+
// push UNKNOWN forward only when it's set for the first time
89+
if (!executionClientVersion.equals(ClientVersion.UNKNOWN)
90+
|| this.executionClientVersion.get() == null) {
91+
this.executionClientVersion.set(executionClientVersion);
92+
executionClientVersionChannel.onExecutionClientVersion(executionClientVersion);
93+
}
8494
}
8595
}

Diff for: ‎ethereum/executionclient/src/test/java/tech/pegasys/teku/ethereum/executionclient/ExecutionClientVersionProviderTest.java

+40-5
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import static org.mockito.ArgumentMatchers.any;
1717
import static org.mockito.Mockito.mock;
1818
import static org.mockito.Mockito.never;
19+
import static org.mockito.Mockito.reset;
1920
import static org.mockito.Mockito.times;
2021
import static org.mockito.Mockito.verify;
2122
import static org.mockito.Mockito.when;
@@ -43,28 +44,29 @@ public void setUp() {
4344
}
4445

4546
@Test
46-
public void doesNotPublishExecutionClientVersionIfFailed() {
47+
public void pushUnknownExecutionClientVersionInChannel_whenFailed() {
4748
when(executionLayerChannel.engineGetClientVersion(any()))
4849
.thenReturn(SafeFuture.failedFuture(new IllegalStateException("oopsy")));
4950

5051
new ExecutionClientVersionProvider(
5152
executionLayerChannel, publishChannel, ClientVersion.UNKNOWN);
52-
verify(publishChannel, never()).onExecutionClientVersion(any());
53+
verify(publishChannel).onExecutionClientVersion(ClientVersion.UNKNOWN);
5354
}
5455

5556
@Test
56-
public void doesNotTryToUpdateExecutionClientVersionIfElHasNotBeenUnavailable() {
57+
public void doesNotTryToUpdateExecutionClientVersion_whenElHasNotBeenUnavailable() {
5758
final ExecutionClientVersionProvider executionClientVersionProvider =
5859
new ExecutionClientVersionProvider(
5960
executionLayerChannel, publishChannel, ClientVersion.UNKNOWN);
6061

6162
executionClientVersionProvider.onAvailabilityUpdated(true);
6263
// EL called only one time
63-
verify(executionLayerChannel, times(1)).engineGetClientVersion(any());
64+
verify(executionLayerChannel).engineGetClientVersion(any());
65+
verify(publishChannel).onExecutionClientVersion(executionClientVersion);
6466
}
6567

6668
@Test
67-
public void updatesExecutionClientVersionWhenElIsAvailableAfterBeingUnavailable() {
69+
public void updatesExecutionClientVersion_whenElIsAvailableAfterBeingUnavailable() {
6870
final ExecutionClientVersionProvider executionClientVersionProvider =
6971
new ExecutionClientVersionProvider(
7072
executionLayerChannel, publishChannel, ClientVersion.UNKNOWN);
@@ -93,4 +95,37 @@ public void updatesExecutionClientVersionWhenElIsAvailableAfterBeingUnavailable(
9395
// ignoring the same
9496
verify(publishChannel, times(2)).onExecutionClientVersion(any());
9597
}
98+
99+
@Test
100+
public void doesNotPushUnknownVersionInChannel_whenELIsDownInTheMiddle() {
101+
final ExecutionClientVersionProvider executionClientVersionProvider =
102+
new ExecutionClientVersionProvider(
103+
executionLayerChannel, publishChannel, ClientVersion.UNKNOWN);
104+
105+
// Good start
106+
verify(publishChannel).onExecutionClientVersion(executionClientVersion);
107+
reset(publishChannel);
108+
109+
// EL is broken
110+
when(executionLayerChannel.engineGetClientVersion(any()))
111+
.thenReturn(SafeFuture.failedFuture(new IllegalStateException("oopsy")));
112+
113+
executionClientVersionProvider.onAvailabilityUpdated(false);
114+
executionClientVersionProvider.onAvailabilityUpdated(true);
115+
// EL called two times
116+
verify(executionLayerChannel, times(2)).engineGetClientVersion(any());
117+
// UNKNOWN version is not pushed in the channel
118+
verify(publishChannel, never()).onExecutionClientVersion(any());
119+
120+
// EL is back
121+
when(executionLayerChannel.engineGetClientVersion(any()))
122+
.thenReturn(SafeFuture.completedFuture(List.of(executionClientVersion)));
123+
124+
executionClientVersionProvider.onAvailabilityUpdated(false);
125+
executionClientVersionProvider.onAvailabilityUpdated(true);
126+
// EL called 3 times
127+
verify(executionLayerChannel, times(3)).engineGetClientVersion(any());
128+
// Version is the same, not pushed in the channel
129+
verify(publishChannel, never()).onExecutionClientVersion(any());
130+
}
96131
}

0 commit comments

Comments
 (0)