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

Prevent sync lookups from reverting to awaiting block #6443

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

dapplion
Copy link
Collaborator

Issue Addressed

Sync lookups have an "oracle" into the da_checker state. Blob or custody column requests are only progressed when the block is already downloaded. This allows deterministic requests after we are sure the block has data.

However, the current implementation has an interesting flaw. If the "oracle" is buggy, the sync lookup can consider that:

  • t_0: block is not downloaded, halt custody column downloads
  • t_1: block is downloaded, continue column downloads
  • t_2: block is not downloaded, halt custody column downloads (again)

This sequence of states should never happen but the type models allow it to happen.

Proposed Changes

Instead, we can model SingleBlockLookup such that once the block is seen, it can never be unseen. This new enum:

enum ComponentRequests<E: EthSpec> {
    WaitingForBlock,
    ActiveBlobRequest(BlobRequestState<E>, usize /* expected blob count */),
    ActiveCustodyRequest(CustodyRequestState<E>),
    NotNeeded(&'static str),
}

and moving the "oracle" logic from the network context to the lookup logic achieve this effect.

I'm leaving the PR in draft for those familiar with the topic to thumbs up this design. The overall goal is to reduce the chance of lookups getting stuck if we change the da_checker logic.

@dapplion dapplion added the das Data Availability Sampling label Sep 27, 2024
@dapplion dapplion force-pushed the sync-lookup-no-revert-state branch from 68a4cc3 to f6c1027 Compare October 8, 2024 10:21
@dapplion dapplion added the ready-for-review The code is ready for review label Oct 8, 2024
@dapplion dapplion marked this pull request as ready for review October 8, 2024 10:21
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

This looks great to me! I think moving the logic up to SingleBlockLookup is a lot cleaner, and avoids the possibility of the scenario you described.

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 9, 2024
@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Oct 10, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at a0a62ea

mergify bot added a commit that referenced this pull request Oct 10, 2024
@mergify mergify bot merged commit a0a62ea into sigp:unstable Oct 10, 2024
29 checks passed
@dapplion dapplion deleted the sync-lookup-no-revert-state branch October 11, 2024 08:54
chong-he pushed a commit to chong-he/lighthouse that referenced this pull request Nov 26, 2024
* Prevent sync lookups from reverting to awaiting block

* Remove stale comment
@dapplion dapplion mentioned this pull request Dec 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
das Data Availability Sampling ready-for-merge This PR is ready to merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants