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

Skip EL part of graffiti when EL info not available #8175

Merged
merged 3 commits into from
Apr 9, 2024

Conversation

zilm13
Copy link
Contributor

@zilm13 zilm13 commented Apr 5, 2024

PR Description

We are currently filling it with something like NA0000, which is not beautiful, so we decided to skip it at all in this case.
Also behavior was updated to push UNKNOWN version in channel when it's not known due to failure or whatever, but do it only once per client start. So we could have normal log line with expected graffiti on start when execution client is not supporting the new API and it will be not noisy if there are issues with EL.

PS: I don't try to put more CL info in graffiti when EL version is unknown to make it simpler.

Fixed Issue(s)

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.

@StefanBratanov
Copy link
Contributor

Isn't it better if the length requirements change if EL information is not available because less bytes are needed?

@StefanBratanov
Copy link
Contributor

Also I think better to use null and remove UNKNOWN in some improbable case where some EL sends us same as UNKNOWN 😁

Copy link
Contributor

@StefanBratanov StefanBratanov left a comment

Choose a reason for hiding this comment

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

LGTM

@zilm13 zilm13 merged commit 78911c1 into Consensys:master Apr 9, 2024
16 checks passed
@zilm13 zilm13 deleted the el-na-version-graffiti-skip branch April 9, 2024 08:24
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.

2 participants