diff --git a/CLAUDE.md b/CLAUDE.md index 74d3544b..22276733 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -339,9 +339,10 @@ actual_slot = finalized_slot + 1 + relative_index - Beacon adds `beacon_blocks_by_{range,root}/2` alongside its Status/Ping/MetaData/Goodbye set. Both serve from the checkpoint-anchored store, and `build_status` advertises it (head, finalized checkpoint, and the anchor's slot as `earliest_available_slot`), so peers - have a reason to ask. Asking runs too: a peer's Status starts a range session paced by - `P2PServer::beacon_fetched_through`, the highest slot handed to the chain actor, rather - than the store's head, which trails a delivered batch by the whole actor mailbox. + have a reason to ask. Asking runs too: a peer's Status starts a range session from the + higher of `P2PServer::beacon_fetched_through` (the highest slot a range answer handed to + the chain actor, since the store's head trails a delivered batch by the whole actor + mailbox) and the store's head (which gossip imports move and the watermark never sees). Fetched blocks reach the actor as `BlockSource::Sync`. Their chunks carry four `` (the block's own epoch's fork digest), which is why `BeaconWire` and the codec carry `genesis_validators_root`. See [`docs/beacon_wire.md`](docs/beacon_wire.md) diff --git a/crates/net/p2p/src/lib.rs b/crates/net/p2p/src/lib.rs index 190b6d82..04994cf6 100644 --- a/crates/net/p2p/src/lib.rs +++ b/crates/net/p2p/src/lib.rs @@ -1169,8 +1169,9 @@ pub struct P2PServer { pub(crate) outbound_requests: HashMap, pub(crate) range_sync_state: Option, - /// Highest beacon slot handed to the chain actor, whether or not it has - /// been imported yet. + /// Highest beacon slot a range answer has handed to the chain actor, + /// whether or not it has been imported yet. Gossip never advances it, so + /// range sync reads it beside the store's head; see `beacon_sync_target`. pub(crate) beacon_fetched_through: u64, bootnode_addrs: HashMap>, node_names: HashMap, diff --git a/crates/net/p2p/src/req_resp/handlers.rs b/crates/net/p2p/src/req_resp/handlers.rs index d358c16a..0cc3e2e2 100644 --- a/crates/net/p2p/src/req_resp/handlers.rs +++ b/crates/net/p2p/src/req_resp/handlers.rs @@ -1627,24 +1627,39 @@ fn handle_goodbye(peer: PeerId, goodbye: Goodbye) { } /// The `[start_slot, end_exclusive)` a beacon range sync should now cover, -/// given how far this node has fetched and a peer's advertised head. `None` -/// when the peer is not ahead of `fetched_through`. +/// given how far this node has the chain and a peer's advertised head. `None` +/// when the peer is not ahead of it. /// -/// Takes `fetched_through` rather than a `Store`, deliberately: this is -/// `server.beacon_fetched_through`, not `server.store.head_slot()`. Delivery -/// to the chain actor is a message and import is work, so the store's own -/// head lags a delivered batch by the whole actor mailbox. Driven off the -/// store's head, the live follower kept re-requesting the part of the range -/// still draining and pulled 11,213 blocks off the wire to import 100; see -/// `beacon_fetched_through`'s own doc comment on `P2PServer`. Bounded by -/// [`MAX_SYNC_RANGE`], the same ceiling `handle_lean_status_response` bounds -/// its own request span by. -fn beacon_sync_target(fetched_through: u64, peer_head_slot: u64) -> Option> { - if peer_head_slot <= fetched_through { +/// How far this node has the chain is the higher of two floors, because each +/// one misses blocks the other sees: +/// +/// - `fetched_through` is `server.beacon_fetched_through`, the highest slot a +/// range answer has handed to the chain actor. Delivery is a message and +/// import is work, so the store's head lags a delivered batch by the whole +/// actor mailbox. Driven off the store's head alone, the live follower kept +/// re-requesting the part of the range still draining and pulled 11,213 +/// blocks off the wire to import 100. +/// - `head_slot` is the store's head, which gossip imports move and nothing on +/// the range path sees. Driven off `fetched_through` alone, a follower that +/// had caught up kept it at its last range batch, so every peer that +/// connected later was asked again for every block gossip had delivered +/// since: 748 blocks in 66 bursts on one follower, a single burst of +/// which held a gossip block 4.2 s in the chain actor's queue. The head is +/// a safe floor, since a head block's whole ancestry is already imported. +/// +/// Bounded by [`MAX_SYNC_RANGE`], the same ceiling +/// `handle_lean_status_response` bounds its own request span by. +fn beacon_sync_target( + fetched_through: u64, + head_slot: u64, + peer_head_slot: u64, +) -> Option> { + let synced_through = fetched_through.max(head_slot); + if peer_head_slot <= synced_through { return None; } - let gap = peer_head_slot - fetched_through; - let start_slot = fetched_through.saturating_add(1); + let gap = peer_head_slot - synced_through; + let start_slot = synced_through.saturating_add(1); let end_exclusive = start_slot.saturating_add(gap.min(MAX_SYNC_RANGE)); Some(start_slot..end_exclusive) } @@ -1686,7 +1701,11 @@ async fn handle_status_response( "Beacon handshake complete" ); - let Some(target_range) = beacon_sync_target(server.beacon_fetched_through, peer_head_slot) + // A whole-block decode per handshake, the same one `build_status` already + // pays to send ours. + let head_slot = server.store.beacon_head().map_or(0, |(slot, _)| slot); + let Some(target_range) = + beacon_sync_target(server.beacon_fetched_through, head_slot, peer_head_slot) else { return; }; @@ -1695,6 +1714,7 @@ async fn handle_status_response( %peer, peer_head_slot, fetched_through = server.beacon_fetched_through, + head_slot, start_slot = target_range.start, end_exclusive = target_range.end, "Beacon peer status head is ahead of what has been fetched" @@ -2409,8 +2429,8 @@ async fn handle_beacon_blocks_by_range_response( debug!(%peer, received, accepted, "Beacon blocks received"); // Highest slot *handed to* the actor, not the highest imported: see - // `beacon_fetched_through`'s own doc comment on `P2PServer` for why range - // sync must be driven off this rather than the store's head. + // [`beacon_sync_target`] for why range sync reads this beside the store's + // head rather than the head alone. if let Some(highest) = highest_forwarded_slot { server.beacon_fetched_through = server.beacon_fetched_through.max(highest); } @@ -2862,24 +2882,36 @@ mod tests { } #[test] - fn beacon_sync_target_keys_off_fetched_through_not_store_head() { + fn beacon_sync_target_keys_off_fetched_through_while_the_head_lags() { // A batch already handed to the chain actor but not yet imported // leaves the store's own head behind `fetched_through`; the sync // target must still be computed from `fetched_through`. See - // `beacon_sync_target`'s own doc comment for why the store's head is - // the wrong signal to drive this off: it is what pulled 11,213 - // blocks off the wire to import 100 on the live follower. - assert_eq!(beacon_sync_target(100, 150), Some(101..151)); + // `beacon_sync_target`'s own doc comment for why the store's head + // alone is the wrong signal to drive this off: it is what pulled + // 11,213 blocks off the wire to import 100 on the live follower. + assert_eq!(beacon_sync_target(100, 40, 150), Some(101..151)); // A peer at or behind what has already been fetched has nothing to // offer, regardless of what the store's own (possibly much lower) // head happens to be. - assert_eq!(beacon_sync_target(150, 150), None); - assert_eq!(beacon_sync_target(150, 100), None); + assert_eq!(beacon_sync_target(150, 40, 150), None); + assert_eq!(beacon_sync_target(150, 40, 100), None); + } + + #[test] + fn beacon_sync_target_starts_after_a_head_gossip_moved_past_fetched_through() { + // A caught-up follower imports from gossip, which leaves + // `fetched_through` at its last range batch. A peer one block ahead + // is owed that one block, not every block since that batch. + assert_eq!(beacon_sync_target(100, 149, 150), Some(150..151)); + // A peer at or behind the head has nothing to offer, however far + // behind `fetched_through` is. + assert_eq!(beacon_sync_target(100, 150, 150), None); + assert_eq!(beacon_sync_target(100, 160, 150), None); } #[test] fn beacon_sync_target_is_bounded_by_max_sync_range() { - let target = beacon_sync_target(0, u64::MAX).expect("peer is far ahead"); + let target = beacon_sync_target(0, 0, u64::MAX).expect("peer is far ahead"); assert_eq!(target, 1..(1 + MAX_SYNC_RANGE)); } diff --git a/docs/beacon_wire.md b/docs/beacon_wire.md index 59912834..c07cba64 100644 --- a/docs/beacon_wire.md +++ b/docs/beacon_wire.md @@ -494,6 +494,14 @@ which on the live follower meant 11,213 blocks off the wire to import 100, each duplicate paying a `hash_tree_root` before the store could reject it. Paced off the watermark, the ratio was 1.7:1. +The watermark alone misses gossip, though. Only a range answer advances it, so a +follower that had caught up and then imported from gossip kept it at its last +range batch, and every peer that connected later was asked again for every block +since: 748 blocks in 66 bursts on one follower. A `Status` answer is +therefore measured against the higher of the watermark and the store's head +(`beacon_sync_target`). The head is a safe floor, since a head block's whole +ancestry is already imported. + `Status` is store-derived: head from `Store::beacon_head`, the finalized checkpoint from `Store::beacon_finalized_checkpoint`, and `earliest_available_slot` from the anchor's own slot rather than