Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Graffiti hex support #8894

Merged

Conversation

d00medman
Copy link
Contributor

What type of PR is this?
Feature

What does this PR do? Why is it needed?
When a user provides a string prefixed with "hex:" followed by a hexidecimal string, this change will cause that piece of graffiti to be converted to ascii characters. It was deemed a reasonable feature for the system to have

Which issues(s) does this PR fix?

Fixes #8168

Other notes for review
Performs the conversion to ascii at startup; original value of yaml file captured by the hash field of the graffiti struct. Opted to make conversion errors non-blocking, when this happens the graffiti will be "hex:whatever"

@d00medman d00medman requested a review from a team as a code owner May 14, 2021 19:08
validator/graffiti/parse_graffiti_test.go Outdated Show resolved Hide resolved
validator/graffiti/parse_graffiti_test.go Outdated Show resolved Hide resolved
validator/graffiti/parse_graffiti_test.go Outdated Show resolved Hide resolved
validator/graffiti/parse_graffiti_test.go Outdated Show resolved Hide resolved
validator/graffiti/parse_graffiti.go Outdated Show resolved Hide resolved
@rkapka
Copy link
Contributor

rkapka commented May 17, 2021

Opted to make conversion errors non-blocking, when this happens the graffiti will be "hex:whatever"

This doesn't seem to be covered by any test case. Also I am not that big of a fan of whatever. I would prefer leaving the graffiti empty or providing something more meaningful like [parse error]. I don't have a strong opinion on this one, though.

@d00medman
Copy link
Contributor Author

Opted to make conversion errors non-blocking, when this happens the graffiti will be "hex:whatever"

This doesn't seem to be covered by any test case. Also I am not that big of a fan of whatever. I would prefer leaving the graffiti empty or providing something more meaningful like [parse error]. I don't have a strong opinion on this one, though.

So the "whatever" isn't actually what you'll get as an output, I just used it as a stand in. For example, if you put "hex:test 123", the graffiti used by the validator will be "hex:test 123", apologies for the lack of clarity here

validator/graffiti/parse_graffiti.go Outdated Show resolved Hide resolved
@rkapka
Copy link
Contributor

rkapka commented May 21, 2021

Thanks for addressing my comments @d00medman. You need to run bazel run //:gazelle -- fix to make the build happy. This will update package dependencies.

validator/graffiti/parse_graffiti.go Outdated Show resolved Hide resolved
validator/graffiti/parse_graffiti.go Outdated Show resolved Hide resolved
rauljordan
rauljordan previously approved these changes May 25, 2021
@rauljordan
Copy link
Contributor

PTAL @rkapka

@0xKiwi
Copy link
Contributor

0xKiwi commented May 27, 2021

E2E is currently failing with the following error:

    assertions.go:85: endtoend_test.go:221 Evaluation failed for epoch 1: could not get graffiti from the list: could not get graffiti from the list

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please address e2e failure

validator/graffiti/parse_graffiti_test.go Outdated Show resolved Hide resolved
Copy link

@rndstr rndstr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some drive-by nits, feel free to ignore

validator/graffiti/parse_graffiti.go Outdated Show resolved Hide resolved
validator/graffiti/parse_graffiti.go Outdated Show resolved Hide resolved
validator/graffiti/parse_graffiti.go Outdated Show resolved Hide resolved
@prestonvanloon
Copy link
Member

Looks like some build failures:

validator/graffiti/parse_graffiti.go:64:42: undefined: hexGraffitiPrefixKey

@d00medman
Copy link
Contributor Author

It is unclear to me why the e2e test is failing; eyeball test alone is not doing the trick. I have thus far been unsuccessful in running this suite locally, any help in doing so would be very helpful.

@prestonvanloon
Copy link
Member

Its failing to write the correct graffiti. You also have unit test failures. I imagine that if you fix the issues causing failures in the unit test, then you'll also fix the issue in the e2e tests.

@prestonvanloon
Copy link
Member

E2E is failing for this error

assertions.go:85: endtoend_test.go:221 Evaluation failed for epoch 1: could not get graffiti from the list: could not get graffiti from the list

@prestonvanloon
Copy link
Member

The e2e test is supposed to use a random graffiti, and i see that this block had no graffiti.

time="2021-06-09 16:50:41" level=info msg="Submitted new block" blockRoot=0x5b303b82d7ec graffiti="\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00\00" numAttestations=4 numDeposits=0 prefix=validator pubKey=0x87bbd5574c17 slot=1

@prestonvanloon
Copy link
Member

Without changing the unit tests, if you can make all of TestParseGraffitiFile_* pass, then I believe e2e should pass as well. There are multiple test failures there when parsing a graffiti file.

@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #8894 (d10fe42) into develop (0d8dd8d) will decrease coverage by 0.01%.
The diff coverage is 86.36%.

@@             Coverage Diff             @@
##           develop    #8894      +/-   ##
===========================================
- Coverage    60.78%   60.77%   -0.02%     
===========================================
  Files          531      531              
  Lines        37862    37883      +21     
===========================================
+ Hits         23015    23022       +7     
- Misses       11567    11579      +12     
- Partials      3280     3282       +2     

Copy link
Member

@prestonvanloon prestonvanloon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! Thanks so much

@prylabs-bulldozer prylabs-bulldozer bot merged commit 644d5bb into prysmaticlabs:develop Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Graffiti in hex
6 participants