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

shrink/ancient pack purge zero lamport accounts #2312

Merged
merged 5 commits into from
Aug 6, 2024

Conversation

jeffwashington
Copy link

@jeffwashington jeffwashington commented Jul 26, 2024

Problem

when an account has 1 slot list entry and ref_count=1, it can be removed from the index (marked dead) and removed from storages. Currently, shrink and ancient pack operations carry along zero lamport accounts with a single refcount forever. This is not ideal.

Summary of Changes

Shrink and ancient pack do not copy over zero lamport accounts with slot_list=1 and ref_count=1. Since there are no other instances of the account, the account can be removed from the index and the storage. If it is reincarnated after loading a snapshot, clean will delete it again from the index.

Fixes #

@jeffwashington jeffwashington force-pushed the 4jul40 branch 18 times, most recently from c37b35e to 6c0f2aa Compare August 2, 2024 19:19
@jeffwashington jeffwashington marked this pull request as ready for review August 2, 2024 20:08
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

I still need to review the actual content of the pr... saw these two small nits first

accounts-db/src/accounts_db.rs Show resolved Hide resolved
accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
@brooksprumo brooksprumo self-requested a review August 2, 2024 20:46
@jeffwashington jeffwashington requested a review from HaoranYi August 5, 2024 14:32
accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
accounts-db/src/accounts_db.rs Show resolved Hide resolved
accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
brooksprumo
brooksprumo previously approved these changes Aug 5, 2024
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

:shipit:

@jeffwashington
Copy link
Author

sorry @brooksprumo found a comment typo

Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

PR looks correct to me.

My understanding is that the current code in master is still correct but not
optimal.

In master, these single-ref zero lamports accounts will be copied into new
storage during shrink, but when next clean starts, it will drop these accounts
nevertheless.

With this PR, we are dropping these accounts earlier and don't wait for the
next round of clean.

Is that understanding correct?

@jeffwashington jeffwashington merged commit c84900d into anza-xyz:master Aug 6, 2024
42 checks passed
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* shrink/ancient pack purge zero lamport accounts

* pr feedback

* add =

* comment

* fix comment typo
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.

3 participants