Skip to content
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

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Jul 15, 2024

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:

  • Status all its peers
  • Group them by head_root
  • Issue BeaconBlocksByRange v3 requests to each group where block_root = head_root

If other clients believe this protocol to be unnecessary I'm keen to know how you handle these situations with the existing protocols.

Request Content:
```
(
block_root: Root
Copy link
Contributor

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

Copy link
Contributor

@jimmygchen jimmygchen Jul 17, 2024

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-31
  • start_slot: 32, count: 32: valid and returns block for slots 32-50
  • start_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?

Copy link
Collaborator Author

@dapplion dapplion Jul 23, 2024

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.

Copy link
Collaborator Author

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.

Copy link
Member

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.

Copy link
Collaborator Author

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

@ppopth
Copy link
Member

ppopth commented Jul 24, 2024

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.

@ppopth
Copy link
Member

ppopth commented Jul 24, 2024

Should we do the same with BlobSidecarsByRange as well? I feel that there is no reason to do it only with one but not the other.

@etan-status
Copy link
Contributor

On Nimbus, there is branch feat/splitview which pulls in blocks via BeaconBlocksByRoot in a backward motion, from peers specifically advertising unknown branches.

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.

@dapplion
Copy link
Collaborator Author

On Nimbus, there is branch feat/splitview which pulls in blocks via BeaconBlocksByRoot in a backward motion, from peers specifically advertising unknown branches.

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

Copy link
Member

@ppopth ppopth left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants