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

Use current root slot to disambiguate pruning of old programs #31548

Merged
merged 3 commits into from
May 9, 2023

Conversation

pgarg66
Copy link
Contributor

@pgarg66 pgarg66 commented May 8, 2023

Problem

ForkGraph returns BlockRelation::Unknown for programs deployed before current root slot. That causes pruning of such programs from the cache.

Summary of Changes

The bank_forks retains slot relationship history till the current root. The pruning has the following logic.

  1. BankForks::set_root() gets called when a new root slot is identified.
  2. set_root() calls cache's prune() method before actually storing the new root slot (i.e. cache's view of ForkGraph still has the old root slot).
  3. prune() calls ForkGraph::relationship() trait method on the BankForks object with new root, and program's deployment slot.
    • If the deployment slot is descendant of the new root, it's retained in the cache.
    • If the deployment slot is an ancestor of the new root, it's retained in the cache (and first ancestor found flag is set).
    • Otherwise, it's assumed that the cache entry is on a fork that doesn't connect to the new root, and the entry is pruned.

This logic doesn't work if the deployment slot is older than the current root. Ideally it should be an ancestor of the new root. But the BankForks doesn't have enough history to calculate this relationship, and returns Unknown.

  1. prune() calls ForkGraph::relationship() trait method on the BankForks object with new root, and program's deployment slot.
    • If the deployment slot is descendant of the new root, it's retained in the cache.
    • If the deployment slot is an ancestor of the new root,
      or, the deployment slot is older than the previous root,
      it's retained in the cache (and first ancestor found flag is set).
    • Otherwise, it's assumed that the cache entry is on a fork that doesn't connect to the new root, and the entry is pruned.

Fixes #

@pgarg66 pgarg66 requested a review from Lichtso May 8, 2023 23:43
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #31548 (eae13f0) into master (b20024c) will decrease coverage by 0.1%.
The diff coverage is 100.0%.

@@            Coverage Diff            @@
##           master   #31548     +/-   ##
=========================================
- Coverage    81.3%    81.3%   -0.1%     
=========================================
  Files         734      734             
  Lines      205284   205287      +3     
=========================================
- Hits       167077   167062     -15     
- Misses      38207    38225     +18     

@pgarg66
Copy link
Contributor Author

pgarg66 commented May 9, 2023

This PR is causing consensus issues when applied on top of #31462
But, the issue is valid. So we need a solution to this.

1 similar comment
@pgarg66
Copy link
Contributor Author

pgarg66 commented May 9, 2023

This PR is causing consensus issues when applied on top of #31462
But, the issue is valid. So we need a solution to this.

@pgarg66 pgarg66 marked this pull request as draft May 9, 2023 12:12
@pgarg66 pgarg66 marked this pull request as ready for review May 9, 2023 17:01
@pgarg66 pgarg66 merged commit 3fd3e6d into solana-labs:master May 9, 2023
4 checks passed
@pgarg66 pgarg66 deleted the fix-fork-graph-usage branch May 9, 2023 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants