Merge branch 'peerdas-devnet-7' into peerdas-rangesync

This commit is contained in:
Jimmy Chen
2025-06-05 23:39:03 +10:00
committed by GitHub
46 changed files with 940 additions and 242 deletions

View File

@@ -106,7 +106,7 @@ pub type SingleLookupId = u32;
enum Action {
Retry,
ParentUnknown { parent_root: Hash256 },
Drop,
Drop(/* reason: */ String),
Continue,
}
@@ -194,19 +194,22 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
/// Creates a parent lookup for the block with the given `block_root` and immediately triggers it.
/// If a parent lookup exists or is triggered, a current lookup will be created.
///
/// Returns true if the lookup is created or already exists
#[instrument(parent = None,
level = "info",
fields(service = "lookup_sync"),
name = "lookup_sync",
skip_all
)]
#[must_use = "only reference the new lookup if returns true"]
pub fn search_child_and_parent(
&mut self,
block_root: Hash256,
block_component: BlockComponent<T::EthSpec>,
peer_id: PeerId,
cx: &mut SyncNetworkContext<T>,
) {
) -> bool {
let parent_root = block_component.parent_root();
let parent_lookup_exists =
@@ -223,11 +226,14 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
// the lookup with zero peers to house the block components.
&[],
cx,
);
)
} else {
false
}
}
/// Seach a block whose parent root is unknown.
///
/// Returns true if the lookup is created or already exists
#[instrument(parent = None,
level = "info",
@@ -235,13 +241,14 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
name = "lookup_sync",
skip_all
)]
#[must_use = "only reference the new lookup if returns true"]
pub fn search_unknown_block(
&mut self,
block_root: Hash256,
peer_source: &[PeerId],
cx: &mut SyncNetworkContext<T>,
) {
self.new_current_lookup(block_root, None, None, peer_source, cx);
) -> bool {
self.new_current_lookup(block_root, None, None, peer_source, cx)
}
/// A block or blob triggers the search of a parent.
@@ -256,6 +263,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
name = "lookup_sync",
skip_all
)]
#[must_use = "only reference the new lookup if returns true"]
pub fn search_parent_of_child(
&mut self,
block_root_to_search: Hash256,
@@ -363,6 +371,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
name = "lookup_sync",
skip_all
)]
#[must_use = "only reference the new lookup if returns true"]
fn new_current_lookup(
&mut self,
block_root: Hash256,
@@ -656,7 +665,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
// This is unreachable because RPC blocks do not undergo gossip verification, and
// this error can *only* come from gossip verification.
error!(?block_root, "Single block lookup hit unreachable condition");
Action::Drop
Action::Drop("DuplicateImportStatusUnknown".to_owned())
}
BlockProcessingResult::Ignored => {
// Beacon processor signalled to ignore the block processing result.
@@ -665,14 +674,14 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
component = ?R::response_type(),
"Lookup component processing ignored, cpu might be overloaded"
);
Action::Drop
Action::Drop("Block processing ignored".to_owned())
}
BlockProcessingResult::Err(e) => {
match e {
BlockError::BeaconChainError(e) => {
// Internal error
error!(%block_root, error = ?e, "Beacon chain error processing lookup component");
Action::Drop
Action::Drop(format!("{e:?}"))
}
BlockError::ParentUnknown { parent_root, .. } => {
// Reverts the status of this request to `AwaitingProcessing` holding the
@@ -691,7 +700,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
error = ?e,
"Single block lookup failed. Execution layer is offline / unsynced / misconfigured"
);
Action::Drop
Action::Drop(format!("{e:?}"))
}
BlockError::AvailabilityCheck(e)
if e.category() == AvailabilityCheckErrorCategory::Internal =>
@@ -703,7 +712,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
// lookup state transition. This error invalidates both blob and block requests, and we don't know the
// state of both requests. Blobs may have already successfullly processed for example.
// We opt to drop the lookup instead.
Action::Drop
Action::Drop(format!("{e:?}"))
}
other => {
debug!(
@@ -757,19 +766,32 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
}
Action::ParentUnknown { parent_root } => {
let peers = lookup.all_peers();
// Mark lookup as awaiting **before** creating the parent lookup. At this point the
// lookup maybe inconsistent.
lookup.set_awaiting_parent(parent_root);
debug!(
id = lookup.id,
?block_root,
?parent_root,
"Marking lookup as awaiting parent"
);
self.search_parent_of_child(parent_root, block_root, &peers, cx);
Ok(LookupResult::Pending)
let parent_lookup_exists =
self.search_parent_of_child(parent_root, block_root, &peers, cx);
if parent_lookup_exists {
// The parent lookup exist or has been created. It's safe for `lookup` to
// reference the parent as awaiting.
debug!(
id = lookup_id,
?block_root,
?parent_root,
"Marking lookup as awaiting parent"
);
Ok(LookupResult::Pending)
} else {
// The parent lookup is faulty and was not created, we must drop the `lookup` as
// it's in an inconsistent state. We must drop all of its children too.
Err(LookupRequestError::Failed(format!(
"Parent lookup is faulty {parent_root:?}"
)))
}
}
Action::Drop => {
Action::Drop(reason) => {
// Drop with noop
Err(LookupRequestError::Failed)
Err(LookupRequestError::Failed(reason))
}
Action::Continue => {
// Drop this completed lookup only

View File

@@ -40,7 +40,7 @@ pub enum LookupRequestError {
/// Inconsistent lookup request state
BadState(String),
/// Lookup failed for some other reason and should be dropped
Failed,
Failed(/* reason: */ String),
/// Received MissingComponents when all components have been processed. This should never
/// happen, and indicates some internal bug
MissingComponentsAfterAllProcessed,

View File

@@ -938,12 +938,20 @@ impl<T: BeaconChainTypes> SyncManager<T> {
) {
match self.should_search_for_block(Some(slot), &peer_id) {
Ok(_) => {
self.block_lookups.search_child_and_parent(
if self.block_lookups.search_child_and_parent(
block_root,
block_component,
peer_id,
&mut self.network,
);
) {
// Lookup created. No need to log here it's logged in `new_current_lookup`
} else {
debug!(
?block_root,
?parent_root,
"No lookup created for child and parent"
);
}
}
Err(reason) => {
debug!(%block_root, %parent_root, reason, "Ignoring unknown parent request");
@@ -954,8 +962,15 @@ impl<T: BeaconChainTypes> SyncManager<T> {
fn handle_unknown_block_root(&mut self, peer_id: PeerId, block_root: Hash256) {
match self.should_search_for_block(None, &peer_id) {
Ok(_) => {
self.block_lookups
.search_unknown_block(block_root, &[peer_id], &mut self.network);
if self.block_lookups.search_unknown_block(
block_root,
&[peer_id],
&mut self.network,
) {
// Lookup created. No need to log here it's logged in `new_current_lookup`
} else {
debug!(?block_root, "No lookup created for unknown block");
}
}
Err(reason) => {
debug!(%block_root, reason, "Ignoring unknown block request");

View File

@@ -1833,6 +1833,63 @@ fn test_parent_lookup_too_deep_grow_ancestor() {
rig.assert_failed_chain(chain_hash);
}
// Regression test for https://github.com/sigp/lighthouse/pull/7118
#[test]
fn test_child_lookup_not_created_for_failed_chain_parent_after_processing() {
// GIVEN: A parent chain longer than PARENT_DEPTH_TOLERANCE.
let mut rig = TestRig::test_setup();
let mut blocks = rig.rand_blockchain(PARENT_DEPTH_TOLERANCE + 1);
let peer_id = rig.new_connected_peer();
// The child of the trigger block to be used to extend the chain.
let trigger_block_child = blocks.pop().unwrap();
// The trigger block that starts the lookup.
let trigger_block = blocks.pop().unwrap();
let tip_root = trigger_block.canonical_root();
// Trigger the initial unknown parent block for the tip.
rig.trigger_unknown_parent_block(peer_id, trigger_block.clone());
// Simulate the lookup chain building up via `ParentUnknown` errors.
for block in blocks.into_iter().rev() {
let id = rig.expect_block_parent_request(block.canonical_root());
rig.parent_lookup_block_response(id, peer_id, Some(block.clone()));
rig.parent_lookup_block_response(id, peer_id, None);
rig.expect_block_process(ResponseType::Block);
rig.parent_block_processed(
tip_root,
BlockProcessingResult::Err(BlockError::ParentUnknown {
parent_root: block.parent_root(),
}),
);
}
// At this point, the chain should have been deemed too deep and pruned.
// The tip root should have been inserted into failed chains.
rig.assert_failed_chain(tip_root);
rig.expect_no_penalty_for(peer_id);
// WHEN: Trigger the extending block that points to the tip.
let trigger_block_child_root = trigger_block_child.canonical_root();
rig.trigger_unknown_block_from_attestation(trigger_block_child_root, peer_id);
let id = rig.expect_block_lookup_request(trigger_block_child_root);
rig.single_lookup_block_response(id, peer_id, Some(trigger_block_child.clone()));
rig.single_lookup_block_response(id, peer_id, None);
rig.expect_block_process(ResponseType::Block);
rig.single_block_component_processed(
id.lookup_id,
BlockProcessingResult::Err(BlockError::ParentUnknown {
parent_root: tip_root,
}),
);
// THEN: The extending block should not create a lookup because the tip was inserted into failed chains.
rig.expect_no_active_lookups();
// AND: The peer should be penalized for extending a failed chain.
rig.expect_single_penalty(peer_id, "failed_chain");
rig.expect_empty_network();
}
#[test]
fn test_parent_lookup_too_deep_grow_tip() {
let mut rig = TestRig::test_setup();