fix: handle 0x prefix from execution client commit #7273
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #7273 +/- ##
============================================
- Coverage 48.51% 48.50% -0.01%
============================================
Files 600 600
Lines 40138 40138
Branches 2058 2057 -1
============================================
- Hits 19471 19470 -1
- Misses 20629 20630 +1
Partials 38 38 |
Performance Report✔️ no performance regression detected Full benchmark results
|
nflaig
reviewed
Dec 3, 2024
| const {code: executionCode, commit: executionCommit} = executionClientVersion; | ||
| let {code: executionCode, commit: executionCommit} = executionClientVersion; | ||
|
|
||
| executionCommit = executionCommit.startsWith("0x") ? executionCommit.slice(2, 6) : executionCommit.slice(0, 4); |
There was a problem hiding this comment.
why is there are 0x prefix in the first place? I think our deserialization is not correct, we also don't serialize it correctly according to #7217
There was a problem hiding this comment.
commit has the type DATA as per https://github.com/ethereum/execution-apis/blob/main/src/engine/identification.md#clientversionv1
DATA type must be prepended by 0x as per https://github.com/ethereum/execution-apis/blob/main/src/engine/common.md#encoding
There was a problem hiding this comment.
we also don't serialize it correctly according to #7217
Yea will address it in a separate PR
There was a problem hiding this comment.
DATA type must be prepended by 0x as per https://github.com/ethereum/execution-apis/blob/main/src/engine/common.md#encoding
but isn't that the encoding in transit, e.g. see parentHash for example, it's bytes internally but will be hex encoded in transit
see data conversion functions
lodestar/packages/beacon-node/src/eth1/provider/utils.ts
Lines 101 to 119 in 84e0699
|
Closing in favour of #7282 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Lodestar currently sets default graffiti containing execution client commit with 0x prefix eg.
GE0x2bLSae1f.We should not include the prefix in the graffiti.