Skip to content

Conversation

realbigsean
Copy link
Member

Issue Addressed

Addresses: #5693

  • Suggestion by @dapplion: Adds a completed_lookups time cache, similar to the failed_chains cache as a simple way to track and suppress lookups that have already been completed but are still being processed somewhere

  • I also realized this PR  delete spammy log #5672 would keep us from being able to distinguish current lookups triggered by unknown parents vs attestations easily in the logs, so I added a new enum LookupTrigger solely for the purpose of distinguishing the two

@realbigsean realbigsean added the ready-for-review The code is ready for review label May 2, 2024
@michaelsproul michaelsproul added the v5.2.0 Q2 2024 label May 3, 2024
michaelsproul added a commit that referenced this pull request May 3, 2024
Squashed commit of the following:

commit 3764895
Author: realbigsean <[email protected]>
Date:   Thu May 2 16:33:54 2024 -0400

    return true on lookup completed to ensure child lookup is progressed

commit 910b147
Author: realbigsean <[email protected]>
Date:   Thu May 2 16:27:05 2024 -0400

    lookup trigger logging

commit 56d88fb
Author: realbigsean <[email protected]>
Date:   Thu May 2 16:22:05 2024 -0400

    add cache for completed lookups
@michaelsproul michaelsproul mentioned this pull request May 3, 2024
peers: &[PeerId],
cx: &mut SyncNetworkContext<T>,
) -> bool {
// If the lookup is complete, don't create a new one.
if self.completed_lookups.contains(&block_root) {
return true;
Copy link
Member

Choose a reason for hiding this comment

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

It might be nice to have a debug! log here to know when this is firing

@dapplion
Copy link
Collaborator

dapplion commented May 6, 2024

I would prefer not to go with this solution, instead go for:

@realbigsean
Copy link
Member Author

closing for #5722

@realbigsean realbigsean closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-review The code is ready for review v5.2.0 Q2 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants