[Merged by Bors] - Set graffiti per validator #2044

pawanjay176 · 2020-12-03T12:16:43Z

Issue Addressed

Resolves #1944

Proposed Changes

Adds a "graffiti" key to the validator_definitions.yml. Setting the key will override anything passed through the validator --graffiti flag.
Returns an error if the value for the graffiti key is > 32 bytes instead of silently truncating.

realbigsean

Just a few feature suggestions, looks good to me otherwise

We could add a graffiti field to this PATCH: https://github.com/sigp/lighthouse/blob/stable/validator_client/src/http_api/mod.rs#L417 Which would resolve Remote graffiti provider #2031
could add a reference to this in: https://lighthouse-book.sigmaprime.io/validator-management.html

realbigsean · 2020-12-15T19:05:46Z

book/src/api-vc-endpoints.md

@@ -278,7 +278,8 @@ Typical Responses | 200
    {
        "enable": true,
        "description": "validator_one",
-        "deposit_gwei": "32000000000"
+        "deposit_gwei": "32000000000",
+        "graffiti": "Mr F was here"


validator_client/src/http_api/mod.rs

AgeManning

This looks good to me. It should be backwards compatible, have you tested this?

Also, I guess there's no real way for the user to set this other than manually editing the file? I guess in the VC API we can make it more user friendly.

This is failing an old test that should be fixed in latest code.

pawanjay176 · 2020-12-16T06:30:26Z

I have tested this but I think the requirements of #2074 supersedes this PR now.

Signed-off-by: pawan <[email protected]>

realbigsean

tiny requests and I'm a little unsure about whether we should load graffiti from file in the block service. I think I would lean towards it's ok because of the defaulting logic but wondering what @paulhauner thinks

validator_client/src/graffiti_file.rs

realbigsean · 2021-01-20T16:36:38Z

validator_client/src/block_service.rs

@@ -226,14 +238,22 @@ impl<T: SlotClock + 'static, E: EthSpec> BlockService<T, E> {
            .ok_or("Unable to produce randao reveal")?
            .into();

+        let graffiti = self


wondering if we should load graffiti here or as a separate service

validator_client/src/graffiti_file.rs

pawanjay176 · 2021-01-22T14:31:58Z

I think I would lean towards it's ok because of the defaulting logic

Yeah the defaulting logic should work well enough. One potential issue is where the graffiti file itself is malicious. Since we try reading from the graffiti file before every proposal, if an attacker manages to change the file in some malicious way(e.g. file causes oom or takes too long to read leading to delayed proposal), they could crash the vc or delay the proposal enough for the block to get orphaned.

paulhauner

Cool man! I'm sure people will enjoy drawing ~~body parts~~ pictures on the graffiti walls with this!

I just had a few comments, nothing major :)

book/src/SUMMARY.md

book/src/graffiti.md

consensus/types/src/graffiti.rs

validator_client/src/graffiti_file.rs

book/src/graffiti.md

validator_client/src/initialized_validators.rs

validator_client/src/block_service.rs

fireduck64 · 2021-02-15T20:14:02Z

For whatever it is worth, I just tried this from the PR branch and couldn't get it to work.

The graffiti file had just this single line:
default: graffitiwall:720:641:#ffff00

Command line option to vc was:
--graffiti-file /vol/data/graffiti.txt

Feb 15 20:06:32.119 INFO Lighthouse started                      version: Lighthouse/v1.1.0-c7cc99e+
Feb 15 20:06:32.119 INFO Configured for network                  name: mainnet
Unable to initialize validator config: Error reading graffiti file: InvalidLine

I later removed the file just to make sure it was getting the right file path and got:

Feb 15 20:07:22.989 INFO Lighthouse started                      version: Lighthouse/v1.1.0-c7cc99e+
Feb 15 20:07:22.989 INFO Configured for network                  name: mainnet
Unable to initialize validator config: Error reading graffiti file: InvalidFile(Os { code: 2, kind: NotFound, message: "No such file or directory" })

Full command was:
docker run -d --restart no --name lighthouse-vc --network host
--mount 'type=volume,src=lighthouse_vc_vol,dst=/root/.lighthouse'
-v ~/data:/vol/data
$DOCKER_LIGHTHOUSE lighthouse vc --network mainnet --http --graffiti-file /vol/data/graffiti.txt --metrics

fireduck64 · 2021-02-15T21:05:55Z

After commit 529725f my graffiti file seems to be accepted.

I am excited about this feature.

pawanjay176 · 2021-02-15T21:09:47Z

@fireduck64 Thank you so much. There was a bug in the file reading logic. Should be fixed now.

paulhauner

This is working nicely for me!

I just had a couple of minor suggestions for docs, detail in errors and a nit about some parsing. We're almost there!

book/src/graffiti.md

validator_client/src/graffiti_file.rs

Co-authored-by: Paul Hauner <[email protected]>

paulhauner

Looks good! I'll merge this tomorrow once we've solved a new cargo audit issue that showed up :)

validator_client/src/graffiti_file.rs

michaelsproul · 2021-03-02T22:35:30Z

bors r+

## Issue Addressed Resolves #1944 ## Proposed Changes Adds a "graffiti" key to the `validator_definitions.yml`. Setting the key will override anything passed through the validator `--graffiti` flag. Returns an error if the value for the graffiti key is > 32 bytes instead of silently truncating.

bors · 2021-03-02T23:31:12Z

Pull request successfully merged into unstable.

Build succeeded:

## Issue Addressed Resolves #1944 ## Proposed Changes Adds a "graffiti" key to the `validator_definitions.yml`. Setting the key will override anything passed through the validator `--graffiti` flag. Returns an error if the value for the graffiti key is > 32 bytes instead of silently truncating.

This reverts commit cc7a309.

pawanjay176 marked this pull request as ready for review December 4, 2020 16:25

pawanjay176 added the ready-for-review label Dec 4, 2020

michaelsproul mentioned this pull request Dec 10, 2020

Read validator graffiti from file #2074

Closed

realbigsean reviewed Dec 15, 2020

View reviewed changes

AgeManning reviewed Dec 16, 2020

View reviewed changes

AgeManning added waiting-on-author and removed ready-for-review labels Dec 16, 2020

pawanjay176 added 7 commits December 21, 2020 15:23

Add graffiti to validator_definitions.yml

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

926b89f

Add graffiti to validator http api

d23054d

Fix tests

6aee77f

Fix serialize/deserialize

91c843a

Add a GraffitiString newtype

f383ee7

Signed-off-by: pawan <[email protected]>

Add graffiti to vc api

5ecd058

Signed-off-by: pawan <[email protected]>

Add graffiti-file cli option

f9daecb

pawanjay176 force-pushed the separate-graffiti branch from 7bdec09 to f9daecb Compare December 21, 2020 17:26

pawanjay176 added 3 commits December 22, 2020 18:23

Tidy

de18540

Add test

698780a

Add graffiti to keystore api

afa1cd0

pawanjay176 added ready-for-review and removed waiting-on-author labels Dec 23, 2020

gak mentioned this pull request Jan 5, 2021

Remote graffiti provider #2031

Closed

realbigsean requested changes Jan 20, 2021

View reviewed changes

pawanjay176 added 2 commits January 22, 2021 16:37

Merge branch 'unstable' into separate-graffiti

15bdd10

Log failures

1adcf88

pawanjay176 added 2 commits January 22, 2021 20:51

Docs v1

94573f9

Improve docs

758cb85

paulhauner requested changes Feb 11, 2021

View reviewed changes

paulhauner removed the ready-for-review label Feb 11, 2021

pawanjay176 added 2 commits February 15, 2021 20:50

Merge branch 'unstable' into separate-graffiti

55832f1

Fix doc

58670c4

pawanjay176 added ready-for-review and removed waiting-on-author labels Feb 15, 2021

Lint

c7cc99e

Fix graffiti file reading

529725f

pawanjay176 added 3 commits February 16, 2021 03:25

Add check; additional logging

b3e241d

Merge branch 'unstable' into separate-graffiti

e385698

Appease clippy

5e444eb

paulhauner requested changes Feb 28, 2021

View reviewed changes

paulhauner added waiting-on-author and removed ready-for-review labels Mar 1, 2021

pawanjay176 and others added 3 commits March 1, 2021 12:14

Apply suggestions from code review

7f010c5

Co-authored-by: Paul Hauner <[email protected]>

Show better errors

59a7574

Co-authored-by: Paul Hauner <[email protected]>

Add test for empty graffiti

b871898

pawanjay176 added ready-for-review and removed waiting-on-author labels Mar 1, 2021

paulhauner approved these changes Mar 2, 2021

View reviewed changes

validator_client/src/graffiti_file.rs Outdated Show resolved Hide resolved

paulhauner added ready-for-merge and removed ready-for-review labels Mar 2, 2021

bors bot changed the title ~~Set graffiti per validator~~ [Merged by Bors] - Set graffiti per validator Mar 2, 2021

bors bot closed this Mar 2, 2021

michaelsproul added a commit that referenced this pull request Mar 10, 2021

Revert "Set graffiti per validator (#2044)"

ab41b6c

This reverts commit cc7a309.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Set graffiti per validator #2044

[Merged by Bors] - Set graffiti per validator #2044

pawanjay176 commented Dec 3, 2020

realbigsean left a comment

realbigsean Dec 15, 2020

AgeManning left a comment

pawanjay176 commented Dec 16, 2020

realbigsean left a comment

realbigsean Jan 20, 2021

pawanjay176 commented Jan 22, 2021 •

edited

Loading

paulhauner left a comment

fireduck64 commented Feb 15, 2021

fireduck64 commented Feb 15, 2021

pawanjay176 commented Feb 15, 2021

paulhauner left a comment •

edited

Loading

paulhauner left a comment

michaelsproul commented Mar 2, 2021

bors bot commented Mar 2, 2021

[Merged by Bors] - Set graffiti per validator #2044

[Merged by Bors] - Set graffiti per validator #2044

Conversation

pawanjay176 commented Dec 3, 2020

Issue Addressed

Proposed Changes

realbigsean left a comment

Choose a reason for hiding this comment

realbigsean Dec 15, 2020

Choose a reason for hiding this comment

AgeManning left a comment

Choose a reason for hiding this comment

pawanjay176 commented Dec 16, 2020

realbigsean left a comment

Choose a reason for hiding this comment

realbigsean Jan 20, 2021

Choose a reason for hiding this comment

pawanjay176 commented Jan 22, 2021 • edited Loading

paulhauner left a comment

Choose a reason for hiding this comment

fireduck64 commented Feb 15, 2021

fireduck64 commented Feb 15, 2021

pawanjay176 commented Feb 15, 2021

paulhauner left a comment • edited Loading

Choose a reason for hiding this comment

paulhauner left a comment

Choose a reason for hiding this comment

michaelsproul commented Mar 2, 2021

bors bot commented Mar 2, 2021

pawanjay176 commented Jan 22, 2021 •

edited

Loading

paulhauner left a comment •

edited

Loading