Add clients information to user's graffiti when producing a block #8107

zilm13 · 2024-03-19T20:40:41Z

PR Description

Adds --validators-graffiti-client-append-format to append clients information to user graffiti
Start from ValidatorOptions or ClientGraffitiAppendFormat.

I'm not sure about ValidatorOptions. Feature is used only on BN side and being able to set on VC too is a bit confusing.
I'm not sure where to add info, in the end or in the beginning. In the end looks better from my personal view for user, he gets configured graffiti first, but in the discussion it was proposed to add to the beginning (without much support), so I've added both options

Docs to be added.

Fixed Issue(s)

Fixes #8074

Documentation

I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

I thought about adding a changelog entry, and added one if I deemed necessary.

zilm13 · 2024-03-19T20:45:18Z

Found illegal dependencies in project ':infrastructure:version'
:ethereum:spec

ohh I will fix it.
just want to reuse createTekuVersion

rolfyone · 2024-03-19T20:46:52Z

...ptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/TekuBeaconNode.java

+        5,
+        MINUTES);


5 minutes is a long timeout for a default...

copy-paste
will fix

rolfyone · 2024-03-19T20:52:52Z

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

+  public void formatClientInfo_shouldFitInDesiredLength() {
+    graffitiBuilder.onExecutionClientVersion(BESU_CLIENT_VERSION);
+
+    // 20: LH1be52536BU0f91a674


we're really bundling multiple tests here, so if it ever breaks we'll only see the first failure

ok, will split it

rolfyone · 2024-03-19T20:53:32Z

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

+    assertThat(
+            graffitiBuilder.extractGraffiti(
+                Optional.of(Bytes32Parser.toBytes32(asciiGraffiti0)), 0))
+        .isEqualTo("");


same with most of this suite, we're putting a ton of tests into 1 test case, so breakage will mean that we only see first fail.

ok, will split it

rolfyone · 2024-03-19T21:02:42Z

teku/src/main/java/tech/pegasys/teku/cli/options/ValidatorOptions.java

+                            AUTO_END: (default) Clients info is appended after user's graffiti if any.
+                            AUTO_START: Clients info is added before user's graffiti.


i think the original ticket had a definite idea about where it should be, so allowing users to choose it somewhere else is probably bad....
https://hackmd.io/@wmoBhF17RAOH2NZ5bNXJVg/BJX2c9gja

its a long confusing thread, we may want to just verify what people are thinking on that front

Yeah, I saw this one, mentioned it. It looks just like one of the options/opinions.
It's not a standard and not a big deal to parse it from the both sides.
User must have 100% right to put whatever he wants there

Anyway I can modify it to have only START options (removing START) or mention in thread discussion that we want to give user an option to put it on any side. My personal view that END option looks better for user as graffiti belongs to him in the first place. But as I said I'm ok for any resolution.

rolfyone · 2024-03-19T21:17:04Z

teku/src/main/java/tech/pegasys/teku/cli/options/ValidatorOptions.java

+                            NAME_END: Only clients names are appended after user's graffiti. Good when you don't want to disclose clients versions.
+                            NAME_START: Clients names are added before user's graffiti.
+                            NONE: Clients information is not added to the graffiti.


i'd basically only offer
AUTO - which applies client name and version tag to graffiti, and is default
CLIENT_NAME - allows us to apply client name only to the graffiti. This would be greatly appreciated so that client information can be more accurate kind of description
DISABLED - would be to turn this feature off

worth checking other implementation teams to see their plans if we haven't...
also i think we can actually use enum in cli args which may make this whole thing a little cleaner rather than making it string...

StefanBratanov · 2024-03-20T17:01:33Z

I think I would simplify it as well. From aesthetic point of view, I don't think many users (if any) will prefer _START. We can have only:

AUTO
ONLY_CLIENT_CODES (make it consistent with ClientVersion object
NONE (I prefer it to disabled, since it is a type of format)

I prefer to keep it simple unless we start to get specific user requests that they want to append at start. And then basically append them to end with a space like how it is in this PR. Opinions?

zilm13 · 2024-03-20T17:40:08Z

@rolfyone ready for review

I've checked other clients:

Lighthouse - sigp/lighthouse#5290 implementation does not merge user's graffiti with client information, either user's graffiti or client information
Lodestar - issue created ChainSafe/lodestar#6463, non-default, optional merge in discussion
Prysm - issue created prysmaticlabs/prysm#13558, change default graffiti, no merge from my understanding
Nimbus - no issue

Only Lodestar considering to implement merge optionally, so there will be few merged graffitis in network. It sounds for me like we could freely choose how to merge, add clients info to the beginning or to the end of the string. I suggest to add clients info to the end like: "Happy block TKBU". But your decision on this is final.

Also I'm thinking now that we are not covering this case:
BN could have VC calls with different proposers and may want not to merge anything, but is ok to put clients information in graffiti on proposers without graffiti. So WHEN_EMPTY and CLIENT_NAMES_WHEN_EMPTY may have sense.

And maybe CLIENT_CODES instead of CLIENT_NAMES? Codes is more correct.

zilm13 · 2024-03-25T19:59:24Z

Doc draft

\```bash
validators-graffiti-client-append-format: WHEN_EMPTY
\```

  </TabItem>
</Tabs>

Appends CL and EL clients information with a space to user's graffiti. On
a separate BN/VC setup should be set on a Beacon Node. This feature helps
developers and community to analyze client diversity and block anomalies.
  
Default value is `AUTO`.

Following options are available:
- `AUTO`: If user's graffiti is empty sets graffiti to reveal
  consensus and execution clients information using its codes and build commits 
  like "TK508459f2BUbb9ba13c", where "TK" is Teku, next its build commit, 
  execution layer client 2 characters code, "BU" for Besu in example, and it 
  ends with EL build 4 bytes commit in hexadecimal representation.
  
  If user's graffiti is set, this option will calculate space left 
  (graffiti size is 32 bytes) and if it's more than 5 characters left, appends 
  either full CL/EL version information or one of its compact forms right up to 
  4 characters (client codes only). So, if your graffiti is 
  "It's my first block!", block will be proposed with graffiti 
  "It's my first block TK50BUbb".
- `CLIENT_CODES`: Appends only CL/EL client codes like "TKBU". It's useful if
  you are not sure that version information cannot be used to exploit
  vulnerabilities or don't want to share any extra information.
- `WHEN_EMPTY`: Adds full client information in graffiti only if it's empty.
  Use it if you have different proposers on your node with some of them using 
  graffiti while others not, and you don't want to change user's graffiti if 
  it's set.
- `CLIENT_CODES_WHEN_EMPTY`: Adds client codes information in graffiti only if
  It's empty. Useful if you have different proposers on node and don't want to
  reveal client versions information.
- `DISABLED`: Client information is not appended. If user's graffiti is set it 
  goes as is in block, otherwise empty graffiti is used.

rolfyone · 2024-03-26T02:31:35Z

I still think we should keep it simple...

AUTO
CLIENT_CODES
DISABLED

thats all we need, if people change graffiti they can change this too

zilm13 · 2024-03-26T06:24:48Z

@rolfyone it's all we have in this PR. I've just renamed CLIENT_NAMES -> CLIENT_CODES, codes looks better for me too.
Ready for review and merge

rolfyone · 2024-03-26T08:59:31Z

@rolfyone it's all we have in this PR. I've just renamed CLIENT_NAMES -> CLIENT_CODES, codes looks better for me too.
Ready for review and merge

previous PR has another 2 options... i'll check code, just trying to be clear about how many options we really want

zilm13 · 2024-03-26T09:02:56Z

@rolfyone I've removed it. Now just the list you posted.

rolfyone

LGTM

Add clients information to user's graffiti when producing a block

28bc1c8

zilm13 marked this pull request as draft March 19, 2024 20:40

rolfyone reviewed Mar 19, 2024

View reviewed changes

zilm13 added 6 commits March 20, 2024 16:27

Fix createTekuVersion import dependency issues

58ec4e7

Decrease waiting time for correct block in test

799d59c

Remove extra ClientGraffitiAppendFormat options

c5241da

Fix the test

71b7763

Split big tests, to have less assertions in every test case

a622b5f

Merge branch 'master' into graffitti-append

4e4ce61

Polishing per feedback

8774ac3

zilm13 marked this pull request as ready for review March 20, 2024 17:39

zilm13 added 2 commits March 26, 2024 10:22

Rename CLIENT_NAMES -> CLIENT_CODES

b11aad9

Merge branch 'master' into graffitti-append

dcf1e78

rolfyone approved these changes Mar 26, 2024

View reviewed changes

Merge branch 'master' into graffitti-append

b3543c3

zilm13 merged commit 02110f1 into Consensys:master Mar 26, 2024
16 checks passed

zilm13 deleted the graffitti-append branch March 26, 2024 15:56

zilm13 mentioned this pull request May 1, 2024

Append graffiti should be able to add clients info when only 4 bytes are available #8275

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clients information to user's graffiti when producing a block #8107

Add clients information to user's graffiti when producing a block #8107

zilm13 commented Mar 19, 2024

zilm13 commented Mar 19, 2024

rolfyone Mar 19, 2024

zilm13 Mar 19, 2024

rolfyone Mar 19, 2024

zilm13 Mar 19, 2024

rolfyone Mar 19, 2024

zilm13 Mar 19, 2024

rolfyone Mar 19, 2024

rolfyone Mar 19, 2024

zilm13 Mar 19, 2024 •

edited

Loading

zilm13 Mar 19, 2024

rolfyone Mar 19, 2024

StefanBratanov commented Mar 20, 2024 •

edited

Loading

zilm13 commented Mar 20, 2024

zilm13 commented Mar 25, 2024 •

edited

Loading

rolfyone commented Mar 26, 2024

zilm13 commented Mar 26, 2024

rolfyone commented Mar 26, 2024

zilm13 commented Mar 26, 2024

rolfyone left a comment

		AUTO_END: (default) Clients info is appended after user's graffiti if any.
		AUTO_START: Clients info is added before user's graffiti.

Add clients information to user's graffiti when producing a block #8107

Add clients information to user's graffiti when producing a block #8107

Conversation

zilm13 commented Mar 19, 2024

PR Description

Fixed Issue(s)

Documentation

Changelog

zilm13 commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zilm13 Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanBratanov commented Mar 20, 2024 • edited Loading

zilm13 commented Mar 20, 2024

zilm13 commented Mar 25, 2024 • edited Loading

rolfyone commented Mar 26, 2024

zilm13 commented Mar 26, 2024

rolfyone commented Mar 26, 2024

zilm13 commented Mar 26, 2024

rolfyone left a comment

Choose a reason for hiding this comment

zilm13 Mar 19, 2024 •

edited

Loading

StefanBratanov commented Mar 20, 2024 •

edited

Loading

zilm13 commented Mar 25, 2024 •

edited

Loading