-
Notifications
You must be signed in to change notification settings - Fork 462
engine_getBlobsV2: Don't prohibit partial responses #671
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
engine_getBlobsV2: Don't prohibit partial responses #671
Conversation
Reverts to previous behavior of supporting partial responses. This enables optimizations that require these partial responses.
|
@MarcoPolo Is the intention of returning partial responses to test the cell level deltas approach? I would prefer not to merge spec changes to facilitate optimisations until we can validate that they actually give us the savings we expect. |
|
I think a better title to this PR is "engine_getBlobsV2: Don't prohibit partial responses."
Less about testing it, and more about leaving the door open for this optimization after Fusaka launches.
Understood. Note that it is perfectly valid for an EL implementation with this proposed change to retain the current behavior. From:
The implementation may just decide to return null if any blobs are missing. There is no guarantee that the EL side has any blobs. The proposed change is to not prohibit partial responses, in case it's useful. An implementation may want to use an environment variable or compile flag to return partial responses, and leave that flag off until the optimization exists. In this case, there would be no extra cause and it would be at worst a net zero change. If the optimization works, using today's data, it would reduce data column message size by (blob_count-1)/blob_count. We'd only need to transmit the one missing cell. This would buy us more scaling room without a network upgrade.
Ideally we'd have this optimization implemented already. We don't yet, but we know it is possible if we allow partial responses (or equivalently don't prohibit partial response). Removing the prohibition doesn't cost us anything (as mentioned above), but leaves the door open to reusing work across the CL <-> EL. Keeping the prohibition forces the CL to start from scratch even if 99% of the work has already been done by the EL. I'm happy to dive deeper into the proposed optimization in case it would be helpful. |
|
Strictly returning null in case of partially available data was added because partial response figured out to be completely useless specifically in case of Osaka fork. Which can change in the next fork. If you are saying partial responses can be still useful in Osaka - it's a great reason to return partial responses. Otherwise we can still enable it as an optional alternative response, at least because some clients do not want to care. In Nethermind we want to return null just because partial is useless and it saves time to return null. In the next fork we will change this behavior if partial responses will be useful and it will not require a flag. We also still return partial responses in Cancun |
Yes, this is what I'm saying. |
Co-authored-by: fradamt <[email protected]>
mkalinin
left a comment
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.
Looks good to me. Happy to merge once there are approvals from CL and EL devs
|
I fully support partial messages. |
tbenr
left a comment
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.
it makes sense to me. EL can decide what to do (and even change behavior via a CLI param) remaining compatible with the API.
nflaig
left a comment
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 prefer this over adding a v3 later, there is not much downside to allowing partial responses
pawanjay176
left a comment
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.
LGTM.
Reverts to previous behavior of supporting partial responses. This enables optimizations that require these partial responses.
Replaces #669 (as raul is OOO, and it would be good to do this sooner rather than later)
Discussion from ACDT 41 showed consensus around supporting partial responses and going back to the old text.
Context from that issue still applies:
Background
engine_getBlobsV1currently achieves a 70-80% hit rate, expected to remain high even as blob volume increases.Motivation
Remarks