Conversation
…switching, peer mgmt
# Conflicts: # beacon_node/network/src/sync/tests/range.rs
eserilev
left a comment
There was a problem hiding this comment.
This looks really nice, adds lots of new test coverage and makes it easier to write future tests. Nice work! I have a nit or two but they aren't blockers
| #[allow(dead_code)] | ||
| pub fn with_custody_type(node_custody_type: NodeCustodyType) -> Self { |
There was a problem hiding this comment.
i assume we'll plan on writing tests for nodes with different custody types in the future?
| /// Set EE offline at start, bring back online after this many BlocksByRange responses | ||
| ee_offline_for_n_range_responses: Option<usize>, | ||
| /// Disconnect all peers after responding to this many BlocksByRange requests | ||
| disconnect_peers_after_range_requests: Option<usize>, |
There was a problem hiding this comment.
nit: name here was slightly confusing to me since I dont think we respond to the Nth request (we disconnect at Some(0)), maybe something like successful_range_responses_before_disconnect is clearer?
and the comment could read
/// Disconnect all peers after this many successful BlocksByRange responses.
Not a blocker at all, feel free to ignore
| if self | ||
| .complete_strategy | ||
| .return_wrong_range_column_indices_n_times | ||
| > 0 |
There was a problem hiding this comment.
just noting in the supernode case, we'd actually end up returning no columns here I think.
Increase coverage for range sync using the style and pattern for lookup tests:
This pattern in agnostic to the implementation, and achieves two goals:
Coverage (range sync, merged across all 7 forks)
chain.rschain_collection.rsrange.rsblocks_by_range.rsdata_columns_by_range.rsblobs_by_range.rs5 old tests → 19 new tests using
simulate()pattern. Tests run across all forks: base, altair, bellatrix, capella, deneb, electra, fulu.Existing Tests Migration
1.
head_chain_removed_while_finalized_syncing(regression #2821)What it does: Add head peer → head chain created → grab head batch request → add finalized peer → finalized chain takes priority → grab finalized batch request → disconnect head peer → assert still in Finalized state.
What it tests: When a head chain exists and a finalized peer arrives, the finalized chain takes priority. Disconnecting the head peer removes the head chain but the finalized chain survives.
Migration: Already covered by
finalized_to_head_transition(finalized takes priority, both complete). But the specific "head peer disconnect during finalized sync" scenario is NOT tested. Add:Needs:
SimulateConfig::with_disconnect_head_peers()— disconnect head peers mid-simulate but keep finalized peers.2.
state_update_while_purging(regression #2827)What it tested: When chain targets become known to fork choice during a state update,
purge_outdated_chainsruns beforeupdate_finalized_chains/update_head_chainswithout crashing.Removed: The bug was a call ordering issue in
ChainCollection::update(). The fix hardcodespurge_outdated_chains(line 223) beforeupdate_finalized_chains(line 227) andupdate_head_chains(line 231) in a single function body. This ordering can't regress without visibly rewritingupdate().3.
pause_and_resume_on_ee_offlineWhat it does: Add head peer → EE goes offline → complete head batch → processor empty (paused) → add finalized peer → complete finalized batch → processor still empty → EE back online → assert 2 chain segments in processor queue.
What it tests: When the execution engine goes offline, completed batches queue up and aren't sent to the processor. When EE comes back online, all queued batches are dispatched.
Migration:
Needs:
SimulateConfig::with_ee_offline_for_n_batches(n)— set EE offline before simulate, toggle back online after N batches complete. The simulate loop would need to callupdate_execution_engine_stateat the right time.4.
finalized_sync_enough_global_custody_peers_few_chain_peersWhat it does: Add 100 fullnode peers + 1 supernode → assert finalized state → drive sync to completion via
complete_and_process_range_sync_until.What it tests: End-to-end finalized sync with sufficient custody column coverage across many peers. Tests that range sync can complete when no single peer has all columns but the swarm collectively covers them.
Migration: ✅ Already covered by
finalized_sync_completes— usessetup_finalized_sync()which adds 100 fullnode peers + 1 supernode, builds a real chain, andsimulate()drives it to completion withassert_range_sync_completed().5.
finalized_sync_not_enough_custody_peers_on_start(PeerDAS-only)What it does: Add single fullnode → assert finalized state → assert no network requests (not enough custody coverage) → add 100 fullnodes + 1 supernode → drive sync to completion.
What it tests: When there aren't enough peers to cover all custody columns, range sync creates the chain but doesn't send requests. Once enough peers arrive, sync proceeds.
Migration:
Needs:
setup_finalized_sync_with_insufficient_peers()— adds only 1 fullnode peer.add_sufficient_peers()— adds 100 fullnodes + 1 supernode. This is PeerDAS-only so needs aif !fulu_enabled() { return; }guard.