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

Fix ref-count for multiple stores to the same pubkey in a slot, fixes zero lamport purge detection #12462

Merged
merged 4 commits into from
Sep 26, 2020

Conversation

carllin
Copy link
Contributor

@carllin carllin commented Sep 24, 2020

Problem

Storing to the same key multiple times in a slot only keeps one entry alive for that (pubkey, slot) pair in the AccountsIndex, but increments the ref count multiple times. This means the zero-lamport purge logic will not accurately detect if an account can be purged

Summary of Changes

Only increment ref count once per store per pubkey/slot.

Fixes #

@carllin carllin added the v1.3 label Sep 24, 2020
@carllin carllin changed the title Fix ref-count for zero lamport purge Fix ref-count for multiple stores to the same pubkey in a slot, fixes zero lamport purge detection Sep 24, 2020
@carllin carllin force-pushed the FixAccounts2 branch 2 times, most recently from fa8f897 to 92401fe Compare September 25, 2020 02:28
@codecov
Copy link

codecov bot commented Sep 25, 2020

Codecov Report

Merging #12462 into master will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #12462   +/-   ##
=======================================
  Coverage    82.0%    82.0%           
=======================================
  Files         354      354           
  Lines       82719    82732   +13     
=======================================
+ Hits        67896    67913   +17     
+ Misses      14823    14819    -4     

@carllin carllin removed the v1.3 label Sep 25, 2020
.into_iter()
.map(|account| account.meta.pubkey)
.collect::<Vec<Pubkey>>(),
)
})
.collect()
Copy link
Member

Choose a reason for hiding this comment

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

how about reduce()-ing here like this, which will be done in parallel by rayon?:

let pubkeys: HashSet<(Slot, Pubkey)> = 
   ...
   .reduce(|| HashSet<_>::new(), |mut reduced, pubkeys| reduced.extend(pubkeys)

so that we can remove the need to construct cleaned_slot_keys later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated!

ryoqun
ryoqun previously approved these changes Sep 25, 2020
Copy link
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

LGTM with nits, awesome bug fixing. I guess you found this by carefully reading the code to fight with the snapshot bloat issue at #12194 .

I guess this bug doesn't manifest as clear errors. Only this causes is dangling appendvecs, which doesn't reduce the snapshot size as expected.

@mergify mergify bot dismissed ryoqun’s stale review September 25, 2020 08:42

Pull request has been modified.

@carllin
Copy link
Contributor Author

carllin commented Sep 25, 2020

@ryoqun I actually found it while writing those snapshot hash mismatch tests :)

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.

2 participants