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

Drop overflow cache #5891

Merged
merged 5 commits into from
Jul 11, 2024
Merged

Drop overflow cache #5891

merged 5 commits into from
Jul 11, 2024

Conversation

dapplion
Copy link
Collaborator

@dapplion dapplion commented Jun 5, 2024

Issue Addressed

Overflow cache is designed to never forget un-finalized not-yet-available blocks.

I believe this is not necessary, and we can drop the complexity to handle that case. Rationale:

Usually a block becomes available on its slot within a second of receiving its first component
over gossip. However, a block may never become available if a malicious proposer does not
publish its data, or there are network issues that prevent us from receiving it. If the block
does not become available after some time we can safely forget about it. Consider this two
cases:

  • Global unavailability: If nobody has received the block components it's likely that the
    proposer never made the block available. So we can safely forget about the block as it will
    never become available.
  • Local unavailability: Some of our peers will attest to a descendant of the dropped block and
    lookup sync will eventually fetch its components

Even in periods of non-finality, the proposer is expected to publish the block's data
immediately. Because this cache only holds fully valid data, its capacity is bounded to 1 block
per slot and fork: before inserting into this cache we check proposer signature and correct
proposer. Having a capacity > 1 is an optimization to prevent sync lookup from having re-fetch
data during moments of unstable network conditions.

Proposed Changes

Allow DataAvailabilityChecker internal LRUCache to drop the least used entry if it exceeds capacity.

This PR does not change the DataAvailabilityChecker state cache. I think its approach is good as is.

Comment on lines 271 to 272
#[strum(serialize = "olc")]
OverflowLRUCache,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the policy to deprecate DBColumns?

Copy link
Member

Choose a reason for hiding this comment

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

We've never done it before

We could keep it around in DBColumn so it can be used with lighthouse db inspect, and then delete it once the release that removes it is buried under a hard fork

Copy link
Member

Choose a reason for hiding this comment

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

added it back here 8366504

@jimmygchen jimmygchen added the v5.3.0 Q3 2024 release with database changes! label Jun 26, 2024
/// immediately. Because this cache only holds fully valid data, its capacity is bounded to 1 block
/// per slot and fork: before inserting into this cache we check proposer signature and correct
/// proposer. Having a capacity > 1 is an optimization to prevent sync lookup from having re-fetch
/// data during moments of unstable network conditions.
Copy link
Member

Choose a reason for hiding this comment

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

great docs!

@jimmygchen jimmygchen added the ready-for-merge This PR is ready to merge. label Jul 11, 2024
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.

LGTM!

@jimmygchen
Copy link
Member

@mergify queue

Copy link

mergify bot commented Jul 11, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 880523d

mergify bot added a commit that referenced this pull request Jul 11, 2024
@mergify mergify bot merged commit 880523d into sigp:unstable Jul 11, 2024
28 checks passed
@dapplion dapplion deleted the drop-overflow-cache branch July 11, 2024 12:26
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. v5.3.0 Q3 2024 release with database changes!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants