-
-
Notifications
You must be signed in to change notification settings - Fork 289
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
fix: handle skip_randao_verification query param as per spec #6576
Conversation
if (skipRandaoVerification) { | ||
query["skip_randao_verification"] = ""; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per the current spec
This query parameter is a flag and does not take a value.
So we have to change the type to string
and make it only take empty string
when the value is actually true.
@@ -486,7 +486,7 @@ export type ReqTypes = { | |||
query: { | |||
randao_reveal: string; | |||
graffiti: string; | |||
skip_randao_verification?: boolean; | |||
skip_randao_verification?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nimbus also treated this attribute as string.
I am good with the PR as such leave it to @nflaig and @nazarhussain to hash out some comments |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## unstable #6576 +/- ##
============================================
+ Coverage 61.46% 61.49% +0.02%
============================================
Files 556 556
Lines 58850 58895 +45
Branches 1850 1856 +6
============================================
+ Hits 36171 36216 +45
Misses 22638 22638
Partials 41 41 |
// usage of produce blinded block which should return execution block in blinded format | ||
// but only enable that after testing lighthouse beacon | ||
"builder.selection": "executiononly", | ||
useProduceBlockV3: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we even have to set this explicitly, or do we want to test as it works in production, i.e. pre deneb use v2 and post deneb v3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe we need to explicitly state this anymore now that we're post Deneb and use it by default. Everything on our side should've flipped to v3 automatically at Deneb unless explicitly stated false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but sim tests go through all forks from phase0, just wondering if it makes more sense to test this more closely to production behavior were this only switches to v3 post deneb. On the other hand, there is no public network I know of that is pre deneb anymore, so it just matters for private / testnets or maybe emphemery which does not start genesis at deneb right now iirc
Performance Report✔️ no performance regression detected 🚀🚀 Significant benchmark improvement detected
Full benchmark results
|
@@ -795,3 +795,7 @@ export function getReturnTypes(): ReturnTypes<Api> { | |||
function parseBuilderBoostFactor(builderBoostFactorInput?: string | number | bigint): bigint | undefined { | |||
return builderBoostFactorInput !== undefined ? BigInt(builderBoostFactorInput) : undefined; | |||
} | |||
|
|||
function parseSkipRandaoVerification(skipRandaoVerification?: string): boolean { | |||
return skipRandaoVerification !== undefined && skipRandaoVerification === ""; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This query parameter is a flag and does not take a value.
I think checking the value doesn't even make sense as the spec states it does not take a value, might be best to just checking if the flag exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of does not take a value
implies an empty string in terms a query string parameter for a url. So I believe that's correct to check it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From this Lighthouse implementation and Nimbus it's also clear they check for the empty string value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implies an empty string in terms a query string parameter for a url
Hmm yeah might be, or potentially the difference between setting ?skip_randao_verification
and ?skip_randao_verification=
. If other clients are strictly checking it that way we should be fine as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?skip_randao_verification
is not a right query string parameter format, it query parameters always suffix with =
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 This PR is included in v1.18.0 🎉 |
Motivation
Make sure Lodestar VC works with other clients.
Description
produceBlockV3
more flexible onskip_randao_verification
skip_randao_verification
to string, which is inferred type for query parameters.empty string
is treated astrue
and notfalse
.Steps to test or reproduce