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

unref accounts in shink and pack when we're committed #2806

Merged
merged 14 commits into from
Sep 4, 2024

Conversation

jeffwashington
Copy link

Problem

there is a race with shrink and packing (and ancient appending) where we unref a dead account while preparing but then decide to not act on the slot. This addrefs the accounts. In the meantime, a zero lamport single ref account may have been prepared for marking dead. This race leads to refcount issues.

Summary of Changes

Only unref when we are committed to making the account dead. This is a much cleaner architecture. The index lookup performance we were worried about will be handled by the new scanning model that is rolling out soon.

Fixes #

@jeffwashington
Copy link
Author

@brooksprumo go ahead and turn your eyes on this when you can, please. I believe we have a race condition in master today that this fixes.

@HaoranYi
Copy link

HaoranYi commented Sep 4, 2024

Rebase to master. No functional change.

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 go through the tests. And likely do another pass.

accounts-db/src/accounts_db.rs Outdated Show resolved Hide resolved
accounts-db/src/accounts_db.rs Show resolved Hide resolved
@brooksprumo brooksprumo self-requested a review September 4, 2024 14:22
@jeffwashington jeffwashington marked this pull request as ready for review September 4, 2024 14:29
@brooksprumo brooksprumo self-requested a review September 4, 2024 14:34
@@ -4159,7 +4158,7 @@ impl AccountsDb {
.for_each(|stored_accounts| {
let LoadAccountsIndexForShrink {
alive_accounts,
mut unrefed_pubkeys,
pubkeys_to_unref: mut unrefed_pubkeys,

Choose a reason for hiding this comment

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

nit: Here's another unrefed_pubkeys that can be renamed. (And the others in this function.)

It looks like ShrinkCollect has an unrefed_pubkeys field too. That can be renamed. IMO a subsequent PR is fine too.

accounts-db/src/accounts_db.rs Show resolved Hide resolved
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.

Code looks good to me. The rename nits can be handled here in this PR, or in a follow up.

@jeffwashington jeffwashington merged commit 820644f into anza-xyz:master Sep 4, 2024
40 checks passed
ray-kast pushed a commit to abklabs/agave that referenced this pull request Nov 27, 2024
* unref accounts in shink and pack when we're committed

* remove bad comments

* rewrite shrink_ancient_fail_ref test

* fix comments

* fix a test

* fix another test

* fmt

* del invalid comments

* reviews: move log to new PR.

* Revert "reviews: move log to new PR."

This reverts commit f8aefe0.

* fix comments

* revert log content

* pr: rename

* pr: more rename

---------

Co-authored-by: HaoranYi <[email protected]>
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