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

Component search loop #5696

Closed
wants to merge 4 commits into from

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