Lookup sync with blocks_by_head#81
Closed
dapplion wants to merge 26 commits into
Closed
Conversation
Block lookups now prefer peers that advertise beacon_blocks_by_head and fetch the block (plus a run of its ancestors) via that protocol, falling back to beacon_blocks_by_root otherwise. on_block_download_response now receives a Vec of blocks; the ancestors beyond the requested root are not yet consumed (header-backfill step).
Implement Serialize for the rpc Protocol enum (as its canonical name) so PeerInfo.supported_protocols is surfaced in the peers API, matching how subnet fields are serialized, rather than skipped.
Move the identify protocol-id parsing into a standalone rpc_protocol_from_id function and add a unit test covering ReqResp, non-ReqResp, and unknown protocol ids.
- AncestorBlocks::from_vec consumes an iterator (no remove(0)) - BlocksByHeadRequestItems::add uses if-let instead of match - reduce rpc_protocol_from_id test to two cases
- Split block_lookup_request send paths into send_block_by_root_request
and send_blocks_by_head, both taking the selected PeerId
- on_{single_block,blocks_by_head}_response return AncestorBlocks so the
sync manager does no transformation
- query peer_supports_blocks_by_head after selection instead of carrying it
Add the beacon_blocks_by_head support flag as the first tuple element and keep unstable's .min()/peer-in-tuple shape instead of restructuring.
Strictly de-prioritize peers already at MAX_CONCURRENT_REQUESTS for the protocol we would use (by_head if supported, else by_root), counting only that protocol's in-flight requests per the spec's per-protocol-ID limit. Expose MAX_CONCURRENT_REQUESTS and add ActiveRequests::active_request_count_of_peer.
Build per-peer blocks_by_root / blocks_by_head count maps once instead of scanning per candidate peer; gate selection on the relevant protocol's count >= MAX_CONCURRENT_REQUESTS.
…ctors - send_block_by_root_request/send_blocks_by_head take lookup_id, allocate their own req_id, and return it - replace the two per-peer count helpers with an ActiveRequestsPerPeer struct exposing at_concurrency_limit(peer) - give AncestorBlocks from_single/from_vec constructors and inline into_ancestor_blocks into the response handlers
…elper get_parent_import_status now reports an already-imported block (genesis, or a finalized block whose parent was pruned) as having an imported parent, so a lookup for such an anchor is processed rather than awaiting a missing parent. Block lookups use cx.conflicts_with_finality (returning a reason String) to drop blocks at/below finalization that are off the finalized chain.
Pass known_blocks down search_parent_of_child -> new_current_lookup -> on_lookup_result so every ancestor seeded from a single beacon_blocks_by_head response is served from the cache, rather than only the first parent level.
- size PARENT_DEPTH_TOLERANCE to BLOCKS_BY_HEAD_REQUEST_COUNT + 1 so one by-head batch can seed a full chain without tripping the depth limit - rig: serve BlocksByHead (walk parent chain) and a peerdb test setter + rig helper to mark a peer as advertising beacon_blocks_by_head - add a test asserting a 32-block chain resolves with 1 by-head request and 0 blocks-by-root, all blocks cached
is_parent_imported now also returns true when the block is itself in fork choice (contains_block), covering genesis and finalized blocks whose anchor proto-node isn't queryable via get_block. This lets a lookup for such a block be processed instead of dropped as conflicting with finality.
Serve beacon_blocks_by_head in the test rig (honouring the empty/wrong-block failure strategies), add a PeerSupport enum threaded into TestRig::default / new_after_fulu, and nest the depth macro so every lookup test runs as a by_root and a by_head variant.
… fetch a batch A lookup that nothing is waiting on is a lone island and requests a small count (4); once a child is awaiting it the lookup is part of a chain proven deep and requests a large batch (32). Derived from the lookup graph at creation, no escalation state. Tune the counts against mainnet metrics.
The subject (beacon_blocks_by_head) now lives in the type name, so the variants read clearly on their own instead of the ambiguous DoesNotSupport.
SingleLookupRequestState now records the peers that failed a download for the request, and block_lookup_request de-prioritizes them in peer selection (just below the concurrency-limit check). This stops a faulty by_head peer from being re-picked on every retry in a mixed peer set, letting the lookup fall back to a by_root peer instead of exhausting its attempts and dropping. Found in review.
Add a rig test asserting a faulty by_head peer's failed download forces the retry to fall back to a by_root peer (1 BlocksByHead + 1 BlocksByRoot, lookup succeeds). Also restore the set_peer_supports_by_head helper name, mangled by the earlier ByHeadSupport rename.
# Conflicts: # beacon_node/network/src/sync/block_lookups/single_block_lookup.rs # beacon_node/network/src/sync/manager.rs # beacon_node/network/src/sync/network_context.rs # beacon_node/network/src/sync/tests/lookups.rs # beacon_node/network/src/sync/tests/range.rs
…chor handling into get_parent_import_status - Remove the out-of-scope failed_peers peer-fallback (block lookups no longer de-prioritize peers that failed a download); it can be a separate PR. - Reject a downloaded block more than SLOT_IMPORT_TOLERANCE ahead of the current slot before creating a parent lookup, so a peer can't make us walk ancestors or force range sync to a bogus future head (cx.block_too_far_in_future). - Move the genesis/anchor 'block is itself in fork choice' case into get_parent_import_status via a proto-less ParentImportStatus::AlreadyImported, so is_parent_imported is a plain matches!.
Revert the consensus-side move from the previous commit: get_parent_import_status returns to pure parent-only semantics (no AlreadyImported variant) and block_verification.rs is restored byte-identical to upstream. The genesis/anchor 'block is itself imported' case lives only in the sync-facing is_parent_imported via '|| contains_block', so consensus block-import paths are untouched.
…aker, finality uses Failed, lookup tidy-ups - SyncRequestId::BlocksByHead uses a tuple variant. - Drop the per-peer request-count tiebreaker from lookup peer selection (and the now-unused active_request_count_by_peer helper). - Reuse LookupRequestError::Failed for finality-conflicting blocks; remove the unused ConflictsWithFinality variant. - Collect known ancestors via iterator; tidy two comments.
Owner
Author
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Part of tree sync (sigp#7678).
Wires lookup sync to fetch over
beacon_blocks_by_head(ethereum/consensus-specs#5181; responder landed in sigp#9237). Instead of walking an unknown parent chain one block at a time overblocks_by_root, a peer returns a block plus a run of its ancestors in one response, so a deep chain resolves in a single round-trip.by_head(parsed fromidentify), fall back toby_root, and skip peers already atMAX_CONCURRENT_REQUESTSfor that protocol.by_headresponse is cached and threaded through parent-lookup creation, so the whole chain is seeded with no follow-up requests.by_rootshares the same path.Tests run the whole lookup suite against both
by_rootandby_headpeers, plus one asserting a 32-block chain resolves with a singleby_headrequest and zeroby_root.Built with Claude (Opus 4.8), reviewed line by line.