[Merged by Bors] - Reconstruct Payloads using Payload Bodies Methods #4028
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
ethDreamer
changed the title
There was a problem hiding this comment.
Looking good overall. I'm still in the process of understanding the block streamer and will come back for a more thorough review next week.
Some thoughts on complexity:
- The current impl handles a graceful conversion from BlocksByRoot requests to PayloadsByRange. I initially thought this might not be necessary and we could always convert BlocksByRoot to PayloadsByRoot, but the non-finality requirement for using PayloadsByRoot makes that impossible.
- I'd suggested we could avoid ever using PayloadsByRoot because all unfinalized payloads should be present in the hot database. This works OK in the case where the EL's view of finality is up-to-date or ahead of the CL, but not in the case where the EL's view is behind. In practice I think handling the case where the EL is behind might be important, because the CL can sync ahead using optimistic sync. In this case however the CL's view of the finalized slot is irrelevant to whether or not we should use PayloadsByRoot, so perhaps we should refactor to always use PayloadsByRange and treat PayloadsByRoot as a fallback (like getBlockByHash).
paulhauner
added
v4.0.0
ethDreamer
force-pushed
the
payload_bodies
branch
from
0c41a7e
to
6409839
Compare
Co-authored-by: Michael Sproul <micsproul@gmail.com>
ethDreamer
force-pushed
the
payload_bodies
branch
from
6409839
to
0dac21b
Compare
ethDreamer
added
ready-for-review
|
I've added some more descriptive logging to the streamer (perhaps we should add metrics?). Looks like most requests are for ~64 blocks. I monitored the logs for ~5 hours. In that time, every single block requested was returned. The overwhelming majority of requests require either here's a random section of continuous logs over the course of ~5 minutes: |
|
In my previous review I wrote:
Having thought about it more I'm quite certain we should action this. The current approach of trying to use lighthouse/beacon_node/beacon_chain/src/beacon_block_streamer.rs Lines 643 to 652 in 9a7d32f The fundamental reason for this is that we always store full payloads for all blocks in the hot part of the database (with I think there are two approaches we could take to remedy this:
I started trying to implement (2) with fallback logic at this point but switched to (1) when I encountered the
One option would be to take approach (1) for now, and come back for (2) at a later date. |
|
Looks good to me! I've merged your changes in :) |
michaelsproul
added
ready-for-merge
michaelsproul
approved these changes
Mar 17, 2023
|
I've seen no issues running this on our testnet infra, merging! 🎉 |
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Mar 19, 2023
## Issue Addressed * #3895 Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> Co-authored-by: Michael Sproul <michael@sigmaprime.io>
bors
bot
changed the title
bors bot
pushed a commit
that referenced
this pull request
Apr 18, 2023
## Proposed Changes Builds on #4028 to use the new payload bodies methods in the HTTP API as well. ## Caveats The payloads by range method only works for the finalized chain, so it can't be used in the execution engine integration tests because we try to reconstruct unfinalized payloads there.
xenowits
pushed a commit
to xenowits/lighthouse
that referenced
this pull request
May 16, 2023
## Proposed Changes Builds on sigp#4028 to use the new payload bodies methods in the HTTP API as well. ## Caveats The payloads by range method only works for the finalized chain, so it can't be used in the execution engine integration tests because we try to reconstruct unfinalized payloads there.
ghost
pushed a commit
to oone-world/lighthouse
that referenced
this pull request
Jul 13, 2023
## Proposed Changes Builds on sigp#4028 to use the new payload bodies methods in the HTTP API as well. ## Caveats The payloads by range method only works for the finalized chain, so it can't be used in the execution engine integration tests because we try to reconstruct unfinalized payloads there.
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
* sigp#3895 Co-authored-by: ethDreamer <37123614+ethDreamer@users.noreply.github.com> Co-authored-by: Michael Sproul <michael@sigmaprime.io>
Woodpile37
pushed a commit
to Woodpile37/lighthouse
that referenced
this pull request
Jan 6, 2024
Builds on sigp#4028 to use the new payload bodies methods in the HTTP API as well. The payloads by range method only works for the finalized chain, so it can't be used in the execution engine integration tests because we try to reconstruct unfinalized payloads there.
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.
Issue Addressed