Skip to content

Commit 21cdc64

Browse files
realbigseanrealbigsean
authored
Improve parent lookup logging (#5451)
* upgrade parent lookup result processing logs to debug, use display instead of debug for BlockError in case a blob parent unknown error is hit, add block root to BlockIsAlreadyKnown * fix compile * fix compile * fix compile
1 parent 5ce1619 commit 21cdc64

File tree

8 files changed

+30
-22
lines changed

8 files changed

+30
-22
lines changed

‎beacon_node/beacon_chain/src/beacon_chain.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2651,7 +2651,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
26512651
// If the block is relevant, add it to the filtered chain segment.
26522652
Ok(_) => filtered_chain_segment.push((block_root, block)),
26532653
// If the block is already known, simply ignore this block.
2654-
Err(BlockError::BlockIsAlreadyKnown) => continue,
2654+
Err(BlockError::BlockIsAlreadyKnown(_)) => continue,
26552655
// If the block is the genesis block, simply ignore this block.
26562656
Err(BlockError::GenesisBlock) => continue,
26572657
// If the block is is for a finalized slot, simply ignore this block.
@@ -2879,7 +2879,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
28792879
.fork_choice_read_lock()
28802880
.contains_block(&block_root)
28812881
{
2882-
return Err(BlockError::BlockIsAlreadyKnown);
2882+
return Err(BlockError::BlockIsAlreadyKnown(blob.block_root()));
28832883
}
28842884

28852885
if let Some(event_handler) = self.event_handler.as_ref() {
@@ -2911,7 +2911,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
29112911
.fork_choice_read_lock()
29122912
.contains_block(&block_root)
29132913
{
2914-
return Err(BlockError::BlockIsAlreadyKnown);
2914+
return Err(BlockError::BlockIsAlreadyKnown(block_root));
29152915
}
29162916

29172917
if let Some(event_handler) = self.event_handler.as_ref() {

‎beacon_node/beacon_chain/src/block_verification.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -190,7 +190,7 @@ pub enum BlockError<T: EthSpec> {
190190
/// ## Peer scoring
191191
///
192192
/// The block is valid and we have already imported a block with this hash.
193-
BlockIsAlreadyKnown,
193+
BlockIsAlreadyKnown(Hash256),
194194
/// The block slot exceeds the MAXIMUM_BLOCK_SLOT_NUMBER.
195195
///
196196
/// ## Peer scoring
@@ -832,7 +832,7 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
832832
// already know this block.
833833
let fork_choice_read_lock = chain.canonical_head.fork_choice_read_lock();
834834
if fork_choice_read_lock.contains_block(&block_root) {
835-
return Err(BlockError::BlockIsAlreadyKnown);
835+
return Err(BlockError::BlockIsAlreadyKnown(block_root));
836836
}
837837

838838
// Do not process a block that doesn't descend from the finalized root.
@@ -966,7 +966,7 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
966966
SeenBlock::Slashable => {
967967
return Err(BlockError::Slashable);
968968
}
969-
SeenBlock::Duplicate => return Err(BlockError::BlockIsAlreadyKnown),
969+
SeenBlock::Duplicate => return Err(BlockError::BlockIsAlreadyKnown(block_root)),
970970
SeenBlock::UniqueNonSlashable => {}
971971
};
972972

@@ -1784,7 +1784,7 @@ pub fn check_block_relevancy<T: BeaconChainTypes>(
17841784
.fork_choice_read_lock()
17851785
.contains_block(&block_root)
17861786
{
1787-
return Err(BlockError::BlockIsAlreadyKnown);
1787+
return Err(BlockError::BlockIsAlreadyKnown(block_root));
17881788
}
17891789

17901790
Ok(block_root)

‎beacon_node/beacon_chain/tests/block_verification.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1087,7 +1087,7 @@ async fn block_gossip_verification() {
10871087
assert!(
10881088
matches!(
10891089
unwrap_err(harness.chain.verify_block_for_gossip(Arc::new(block.clone())).await),
1090-
BlockError::BlockIsAlreadyKnown,
1090+
BlockError::BlockIsAlreadyKnown(_),
10911091
),
10921092
"should register any valid signature against the proposer, even if the block failed later verification"
10931093
);
@@ -1115,7 +1115,7 @@ async fn block_gossip_verification() {
11151115
.verify_block_for_gossip(block.clone())
11161116
.await
11171117
.expect_err("should error when processing known block"),
1118-
BlockError::BlockIsAlreadyKnown
1118+
BlockError::BlockIsAlreadyKnown(_)
11191119
),
11201120
"the second proposal by this validator should be rejected"
11211121
);

‎beacon_node/http_api/src/publish_blocks.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlockConten
113113
let (gossip_verified_block, gossip_verified_blobs) =
114114
match block_contents.into_gossip_verified_block(&chain) {
115115
Ok(b) => b,
116-
Err(BlockContentsError::BlockError(BlockError::BlockIsAlreadyKnown))
116+
Err(BlockContentsError::BlockError(BlockError::BlockIsAlreadyKnown(_)))
117117
| Err(BlockContentsError::BlobError(
118118
beacon_chain::blob_verification::GossipBlobError::RepeatBlob { .. },
119119
)) => {
@@ -133,7 +133,7 @@ pub async fn publish_block<T: BeaconChainTypes, B: IntoGossipVerifiedBlockConten
133133
log,
134134
"Not publishing block - not gossip verified";
135135
"slot" => slot,
136-
"error" => ?e
136+
"error" => %e
137137
);
138138
return Err(warp_utils::reject::custom_bad_request(e.to_string()));
139139
}

‎beacon_node/network/src/network_beacon_processor/gossip_methods.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -963,7 +963,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
963963
self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Ignore);
964964
return None;
965965
}
966-
Err(BlockError::BlockIsAlreadyKnown) => {
966+
Err(BlockError::BlockIsAlreadyKnown(_)) => {
967967
debug!(
968968
self.log,
969969
"Gossip block is already known";

‎beacon_node/network/src/network_beacon_processor/sync_methods.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
276276
"slot" => %slot,
277277
);
278278
}
279-
Err(BlockError::BlockIsAlreadyKnown) => {
279+
Err(BlockError::BlockIsAlreadyKnown(_)) => {
280280
debug!(
281281
self.log,
282282
"Blobs have already been imported";
@@ -639,7 +639,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
639639
peer_action: Some(PeerAction::LowToleranceError),
640640
})
641641
}
642-
BlockError::BlockIsAlreadyKnown => {
642+
BlockError::BlockIsAlreadyKnown(_) => {
643643
// This can happen for many reasons. Head sync's can download multiples and parent
644644
// lookups can download blocks before range sync
645645
Ok(())

‎beacon_node/network/src/sync/block_lookups/mod.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -811,7 +811,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
811811
let root = lookup.block_root();
812812
trace!(self.log, "Single block processing failed"; "block" => %root, "error" => %e);
813813
match e {
814-
BlockError::BlockIsAlreadyKnown => {
814+
BlockError::BlockIsAlreadyKnown(_) => {
815815
// No error here
816816
return Ok(None);
817817
}
@@ -898,17 +898,17 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
898898
match &result {
899899
BlockProcessingResult::Ok(status) => match status {
900900
AvailabilityProcessingStatus::Imported(block_root) => {
901-
trace!(self.log, "Parent block processing succeeded"; &parent_lookup, "block_root" => ?block_root)
901+
debug!(self.log, "Parent block processing succeeded"; &parent_lookup, "block_root" => ?block_root)
902902
}
903903
AvailabilityProcessingStatus::MissingComponents(_, block_root) => {
904-
trace!(self.log, "Parent missing parts, triggering single block lookup "; &parent_lookup,"block_root" => ?block_root)
904+
debug!(self.log, "Parent missing parts, triggering single block lookup "; &parent_lookup,"block_root" => ?block_root)
905905
}
906906
},
907907
BlockProcessingResult::Err(e) => {
908-
trace!(self.log, "Parent block processing failed"; &parent_lookup, "error" => %e)
908+
debug!(self.log, "Parent block processing failed"; &parent_lookup, "error" => %e)
909909
}
910910
BlockProcessingResult::Ignored => {
911-
trace!(
911+
debug!(
912912
self.log,
913913
"Parent block processing job was ignored";
914914
"action" => "re-requesting block",
@@ -954,7 +954,7 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
954954
self.request_parent(parent_lookup, cx);
955955
}
956956
BlockProcessingResult::Ok(AvailabilityProcessingStatus::Imported(_))
957-
| BlockProcessingResult::Err(BlockError::BlockIsAlreadyKnown { .. }) => {
957+
| BlockProcessingResult::Err(BlockError::BlockIsAlreadyKnown(_)) => {
958958
// Check if the beacon processor is available
959959
let Some(beacon_processor) = cx.beacon_processor_if_enabled() else {
960960
return trace!(

‎beacon_node/network/src/sync/block_lookups/tests.rs‎

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -458,7 +458,11 @@ fn test_parent_lookup_happy_path() {
458458
rig.expect_empty_network();
459459

460460
// Processing succeeds, now the rest of the chain should be sent for processing.
461-
bl.parent_block_processed(chain_hash, BlockError::BlockIsAlreadyKnown.into(), &mut cx);
461+
bl.parent_block_processed(
462+
chain_hash,
463+
BlockError::BlockIsAlreadyKnown(block_root).into(),
464+
&mut cx,
465+
);
462466
rig.expect_parent_chain_process();
463467
let process_result = BatchProcessResult::Success {
464468
was_non_empty: true,
@@ -1117,7 +1121,11 @@ fn test_same_chain_race_condition() {
11171121
// the processing result
11181122
if i + 2 == depth {
11191123
// one block was removed
1120-
bl.parent_block_processed(chain_hash, BlockError::BlockIsAlreadyKnown.into(), &mut cx)
1124+
bl.parent_block_processed(
1125+
chain_hash,
1126+
BlockError::BlockIsAlreadyKnown(block.canonical_root()).into(),
1127+
&mut cx,
1128+
)
11211129
} else {
11221130
bl.parent_block_processed(
11231131
chain_hash,

0 commit comments

Comments
 (0)