-
Notifications
You must be signed in to change notification settings - Fork 974
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
Add BeaconBlocksByRange v3 #3845
base: dev
Are you sure you want to change the base?
Conversation
5ac7052
to
9b8556f
Compare
Request Content: | ||
``` | ||
( | ||
block_root: Root |
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.
Is this intended to be the last block in the segment (start_slot + count)? Or just a block anywhere in the segment
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'm guessing this is intended to be the target block to sync to?
which could be either greater, equal or less than (start_slot + count)
but greater than or equal to the start_slot
e.g. target block_root 0xabcde
(at slot 50)
start_slot: 0, count: 32
: valid and returns block for slots 0-31start_slot: 32, count: 32
: valid and returns block for slots 32-50start_slot: 64, count: 32
: invalid request
In the 2nd case above, the block 0xabcde
is the last block in the segment.
If the above is correct, perhaps this can be renamed to target_block_root
?
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 have updated the definition for block_root
to refer to a block that's at the tip or descendant of the request slot range 06fb7a0 for the block range to be uniquely identifiable block_root
must not be less than start_slot + count
, that was an oversight on my end.
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.
@jimmygchen I like the suggestion to use a more descriptive name than block_root
. However target
is overloaded with the CasperFFG checkpoint meaning.
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'm thinking that maybe we should just start from the end slot and go back to the ancestors rather than starting from the start slot and go down to the descendants. How about changing the entire request content to the following?
(
block_root: Root
end_slot: Slot
count: uint64
)
The request will be valid only if block_root
is in the slot end_slot
.
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.
The request will be valid only if block_root is in the slot end_slot.
I feel it's an unnecessary departure to how by_range requests are done today
I'm a bit surprised that this didn't exist in the beginning. Do we have any reason why we didn't do it in phase0? or we just didn't do it. |
Should we do the same with |
On Nimbus, there is branch One issue on Goerli was that when there is heavy branching, that there are also longer gaps, so range requests are very inefficient due to sometimes long gaps between blocks. |
If the branch is long you can easily get DOS-ed, you can't validate any block until you root the branch on a known block |
Co-authored-by: Pop Chunhapanya <[email protected]>
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 looks good to me
BeaconBlocksByRange v2 allows to sync blocks exclusively from the peer's canonical chain.
Consider an unstable network with a competing forks. One can group peers by their head with status Req Resp messages. However, there's asynchrony between receiving that status message and requesting blocks by range, as the head of the remote peer can change at any time.
Consider a case where you syncing a head chain with BeaconBlocksByRange v2 and peer changes its head before you issue a by range request. It changes into a fork that has all missed slots in the requested range. You should downscore the peer since its giving incorrect blocks for the segment you are intending to sync. However, the root of the issue is the risk of mixing ranges of blocks from different forks without being aware of it.
BeaconBlocksByRange v3 allows requesters to specify which fork they intend to sync. An exemplary consumer would:
If other clients believe this protocol to be unnecessary I'm keen to know how you handle these situations with the existing protocols.