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 drop lookups without peers while awaiting events #5839

Merged
merged 1 commit into from
May 24, 2024

Conversation

dapplion
Copy link
Collaborator

Issue Addressed

Lookups without peers should be allowed to exist for some time. See this common race condition:

  1. Receive unknown block parent event
  2. Create child lookup with zero peers
  3. Parent is processed, before receiving any attestation for the child block
  4. Child lookup is attempted to make progress but has no peers
  5. We receive an attestion for child block and add a peer to the child block lookup

In step 4 we could drop the lookup because we attempt to issue a request with no peers available. This has two issues:

  • We may drop the lookup while some other block component is processing, triggering an unknown lookup error. This can potentially cause unrelated child lookups to also be dropped when calling drop_lookup_and_children.
  • We lose all progress of the lookup, and have to re-download its components that we may already have there cached.

Instead, there's no negative for keeping lookups with no peers around for some time. If we regularly prune them, it should not be a memory concern (TODO: maybe yes!).


This PR fixes the race condition seen in first issue of:

Proposed Changes

Do not immediately drop a lookup when attempting to do a download and it has no peers. Instead wait for all the following conditions before dropping it:

  • Lookup has no peers
  • Lookup is not awaiting any event (i.e. not processing a block component)
  • Some time has elapsed since its creation

@dapplion dapplion added the ready-for-review The code is ready for review label May 24, 2024
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

Seems like a good approach to fixing this bug

@realbigsean
Copy link
Member

@mergify queue

Copy link

mergify bot commented May 24, 2024

queue

🛑 The pull request has been removed from the queue default

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

@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
mergify bot added a commit that referenced this pull request May 24, 2024
@realbigsean
Copy link
Member

@mergify refresh

Copy link

mergify bot commented May 24, 2024

refresh

✅ Pull request refreshed

@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 f187ad8

mergify bot added a commit that referenced this pull request May 24, 2024
@mergify mergify bot merged commit f187ad8 into sigp:unstable May 24, 2024
28 checks passed
@dapplion dapplion deleted the dont-drop-lookups-mid-state branch May 30, 2024 06:31
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