Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not double log uknown lookup #5837

Merged
merged 2 commits into from
May 24, 2024

Conversation

dapplion
Copy link
Collaborator

Issue Addressed

Minor purely log fix. If sync attempts to process an event for an unknown lookup it will log "Dropping child lookup" which is not true. If the lookup is unknown it can't be dropped.

Noticed in #5833

Proposed Changes

Do not double log uknown lookup

@dapplion dapplion added the ready-for-review The code is ready for review label May 24, 2024
@@ -625,7 +625,11 @@ impl<T: BeaconChainTypes> BlockLookups<T> {
/// dropped.
pub fn drop_lookup_and_children(&mut self, dropped_id: SingleLookupId) {
if let Some(dropped_lookup) = self.single_block_lookups.remove(&dropped_id) {
debug!(self.log, "Dropping child lookup"; "id" => ?dropped_id, "block_root" => ?dropped_lookup.block_root());
debug!(self.log, "Dropping lookup";
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of labeling this as "child", log if it's awaiting_parent

@realbigsean realbigsean added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels May 24, 2024
@realbigsean
Copy link
Member

@mergify queue

Copy link

mergify bot commented May 24, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at f1f1250

mergify bot added a commit that referenced this pull request May 24, 2024
@mergify mergify bot merged commit f1f1250 into sigp:unstable May 24, 2024
28 checks passed
@dapplion dapplion deleted the sync-double-log-unknown branch May 24, 2024 21:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants