gnosis/lighthouse
1
0
Fork 0
mirror of https://github.com/sigp/lighthouse.git synced 2026-06-15 09:48:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix correctness issues in single-block lookup state machine

Browse Source 
- add_peer: replace !=-vs-|= typo so Gloas child-peer additions actually
  propagate back through add_peers_to_lookup_and_ancestors and kick
  continue_requests.
- data_peer_group: return the PeerGroup stored in DataRequestState
  Downloaded/Processing instead of todo!(), so InvalidColumn attribution
  in mod.rs no longer panics on a live error path.
- Restore the original `parent_root != ZERO` guard for the parent-known
  check; the genesis block has no real parent so it must fall through to
  processing rather than panic (was todo!()) or be dropped as Failed.
- Wire envelope_is_known_to_fork_choice as a NoRequestNeeded short-
  circuit at the top of payload_lookup_request.
- Rename gload_child_peers -> gloas_child_peers (typo).
- Drop DataDownloadKind, peek_downloaded_peer_group, DataRequest.slot,
  DownloadedData::Blobs.expected_blobs — all dead per the compiler.
- Update test helpers to send UnknownParentSidecarHeader so the lookup
  test suite compiles and runs under the new manager API.

Tests: phase0 79/79, electra 59/59, fulu 59/59.
This commit is contained in:
dapplion
2026-05-19 03:43:11 -06:00
parent 7739c91a3a
commit 2d2fdf3dce
4 changed files with 63 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
beacon_node/network/src/sync/block_lookups/mod.rs
View File

@@ -1018,7 +1018,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
if let Some(awaiting) = lookup.awaiting_parent() {
let parent_root = awaiting.parent_root();
if let Some((&parent_id, parent_lookup)) = self
if let Some((&parent_id, _)) = self
.single_block_lookups
.iter()
.find(|(_, l)| l.block_root() == parent_root)

104
beacon_node/network/src/sync/block_lookups/single_block_lookup.rs
View File

@@ -238,7 +238,6 @@ impl<E: EthSpec> BlockRequest<E> {
#[derive(Debug)]
struct DataRequest<E: EthSpec> {
peers: PeerSet,
slot: Slot,
state: DataRequestState<E>,
}
@@ -317,19 +316,9 @@ impl<E: EthSpec> DataDownload<E> {
fn take_download_result(&mut self) -> Option<(DownloadedData<E>, PeerGroup)> {
match self {
DataDownload::Blobs {
expected_blobs,
state,
..
} => state.take_download_result().map(|r| {
(
DownloadedData::Blobs {
blobs: r.value,
expected_blobs: *expected_blobs,
},
r.peer_group,
)
}),
DataDownload::Blobs { state, .. } => state
.take_download_result()
.map(|r| (DownloadedData::Blobs(r.value), r.peer_group)),
DataDownload::Columns { state, .. } => state
.take_download_result()
.map(|r| (DownloadedData::Columns(r.value), r.peer_group)),
@@ -342,36 +331,15 @@ impl<E: EthSpec> DataDownload<E> {
DataDownload::Columns { state, .. } => state.is_awaiting_event(),
}
}
fn peek_downloaded_peer_group(&self) -> Option<&PeerGroup> {
match self {
DataDownload::Blobs { state, .. } => state.peek_downloaded_peer_group(),
DataDownload::Columns { state, .. } => state.peek_downloaded_peer_group(),
}
}
}
/// Downloaded data, waiting to be sent for processing
#[derive(Debug)]
enum DownloadedData<E: EthSpec> {
Blobs {
blobs: FixedBlobSidecarList<E>,
expected_blobs: usize,
},
Blobs(FixedBlobSidecarList<E>),
Columns(DataColumnSidecarList<E>),
}
/// Enough info to reconstruct a fresh `DataDownload` when we need to retry data download
/// after a processing failure. We can't call `create_data_request` again from here because
/// we're past the `WaitingForBlock` state and don't have the `SyncNetworkContext` (and
/// therefore no `ChainSpec`) — so the request kind (blobs vs columns, plus the expected
/// blob count) is cached alongside the in-flight request instead.
#[derive(Debug, Clone, Copy)]
enum DataDownloadKind {
Blobs { expected_blobs: usize },
Columns,
}
// === Payload request: WaitingForBlock → Downloading → Downloaded → Processing → Complete ===
#[derive(Debug)]
@@ -501,7 +469,7 @@ pub struct SingleBlockLookup<T: BeaconChainTypes> {
peers: PeerSet,
/// Peers for payload download (0 initially, Gloas only).
#[educe(Debug(method(fmt_peer_map_as_len)))]
gload_child_peers: GloasChildPeers,
gloas_child_peers: GloasChildPeers,
// Parent tracking
awaiting_parent: Option<AwaitingParent>,
@@ -557,7 +525,7 @@ impl<T: BeaconChainTypes> SingleBlockLookup<T> {
data_request: None,
payload_request: None,
peers: block_peers,
gload_child_peers: Arc::new(RwLock::new(gloas_child_peers)),
gloas_child_peers: Arc::new(RwLock::new(gloas_child_peers)),
awaiting_parent,
created: Instant::now(),
failed_processing: 0,
@@ -632,9 +600,14 @@ impl<T: BeaconChainTypes> SingleBlockLookup<T> {
self.block_request.peer()
}
/// Returns custody column peer group if data has been downloaded. Used for peer penalization.
/// Returns the peer group that served the downloaded data (blobs or custody columns) if
/// available, used for peer penalization on data-processing failures.
pub fn data_peer_group(&self) -> Option<&PeerGroup> {
todo!();
match &self.data_request.as_ref()?.state {
DataRequestState::Downloaded { peer_group, .. }
| DataRequestState::Processing { peer_group } => Some(peer_group),
DataRequestState::Downloading(_) | DataRequestState::Complete => None,
}
}
// -- Main state machine driver --
@@ -701,25 +674,24 @@ impl<T: BeaconChainTypes> SingleBlockLookup<T> {
}
let parent_root = block.parent_root();
// Zero hash is the parent of the genesis block — not a real block.
if parent_root == Hash256::ZERO {
todo!();
}
let Some(parent_in_fork_choice) = cx
.chain
.canonical_head
.fork_choice_read_lock()
.get_block(&parent_root)
else {
// Zero hash is the parent of the genesis block — not a real block, so no
// parent-known check is needed. Fall through to send the block for processing.
if parent_root != Hash256::ZERO
&& cx
.chain
.canonical_head
.fork_choice_read_lock()
.get_block(&parent_root)
.is_none()
{
let awaiting_parent = AwaitingParent::from_block(block);
self.awaiting_parent = Some(awaiting_parent.clone());
self.awaiting_parent = Some(awaiting_parent);
return Ok(LookupResult::ParentUnknown {
awaiting_parent,
block_root: self.block_root,
peers: self.all_peers(),
});
};
}
let block = block.clone();
let peer = *peer;
@@ -765,7 +737,6 @@ impl<T: BeaconChainTypes> SingleBlockLookup<T> {
.map_err(LookupRequestError::InternalError)?;
self.data_request = Some(DataRequest {
peers,
slot: block.slot(),
state: DataRequestState::new(
block.slot(),
self.block_root,
@@ -798,7 +769,7 @@ impl<T: BeaconChainTypes> SingleBlockLookup<T> {
}
DataRequestState::Downloaded { data, peer_group } => {
match data {
DownloadedData::Blobs { blobs, .. } => {
DownloadedData::Blobs(blobs) => {
cx.send_blobs_for_processing(
id,
self.block_root,
@@ -935,7 +906,7 @@ impl<T: BeaconChainTypes> SingleBlockLookup<T> {
let Some(execution_hash) = execution_hash else {
return Err("execution_hash is None post gloas".to_string());
};
self.gload_child_peers
self.gloas_child_peers
.write()
.entry(execution_hash)
.or_default()
@@ -1139,16 +1110,15 @@ impl<T: BeaconChainTypes> SingleBlockLookup<T> {
match peer_type {
PeerType::PostGloas(execution_hash) => {
// This peer claims to have imported a child of this block with parent_hash. We
// can't known if the child is full or empty until we know the payload hash of this
// lookup
added
!= self
.gload_child_peers
.write()
.entry(*execution_hash)
.or_default()
.write()
.insert(peer_id);
// can't know whether the child is full or empty until we know the payload hash of
// this lookup.
added |= self
.gloas_child_peers
.write()
.entry(*execution_hash)
.or_default()
.write()
.insert(peer_id);
}
PeerType::PreGloas => {}
}
@@ -1161,7 +1131,7 @@ impl<T: BeaconChainTypes> SingleBlockLookup<T> {
/// Remove peer from available peers.
pub fn remove_peer(&mut self, peer_id: &PeerId) {
self.peers.write().remove(peer_id);
for set in self.gload_child_peers.write().values_mut() {
for set in self.gloas_child_peers.write().values_mut() {
set.write().remove(peer_id);
}
}

8
beacon_node/network/src/sync/network_context.rs
View File

@@ -955,6 +955,14 @@ impl<T: BeaconChainTypes> SyncNetworkContext<T> {
lookup_peers: Arc<RwLock<HashSet<PeerId>>>,
block_root: Hash256,
) -> Result<LookupRequestResult, RpcRequestSendError> {
// Skip the download if fork-choice already saw this envelope (e.g. imported via gossip
// before the lookup got here).
if self.chain.envelope_is_known_to_fork_choice(&block_root) {
return Ok(LookupRequestResult::NoRequestNeeded(
"envelope already known to fork-choice",
));
}
let active_request_count_by_peer = self.active_request_count_by_peer();
let Some(peer_id) = lookup_peers
.read()

19
beacon_node/network/src/sync/tests/lookups.rs
View File

@@ -1522,7 +1522,12 @@ impl TestRig {
}
fn trigger_unknown_parent_blob(&mut self, peer_id: PeerId, blob: Arc<BlobSidecar<E>>) {
self.send_sync_message(SyncMessage::UnknownParentBlob(peer_id, blob));
self.send_sync_message(SyncMessage::UnknownParentSidecarHeader {
peer_id,
block_root: blob.block_root(),
parent_root: blob.block_parent_root(),
slot: blob.slot(),
});
}
fn trigger_unknown_parent_column(
@@ -1530,7 +1535,17 @@ impl TestRig {
peer_id: PeerId,
column: Arc<DataColumnSidecar<E>>,
) {
self.send_sync_message(SyncMessage::UnknownParentDataColumn(peer_id, column));
let DataColumnSidecar::Fulu(col) = column.as_ref() else {
panic!(
"trigger_unknown_parent_column is Fulu-only; Gloas columns use the partial-column path"
);
};
self.send_sync_message(SyncMessage::UnknownParentSidecarHeader {
peer_id,
block_root: col.block_root(),
parent_root: col.block_parent_root(),
slot: col.slot(),
});
}
fn trigger_unknown_block_from_attestation(&mut self, block_root: Hash256, peer_id: PeerId) {