Skip to content

Refactor Accounts Index and AccountsIndexIterator#6923

Merged
jeffwashington merged 4 commits intoanza-xyz:masterfrom
jeffwashington:5ju1
Jul 11, 2025
Merged

Refactor Accounts Index and AccountsIndexIterator#6923
jeffwashington merged 4 commits intoanza-xyz:masterfrom
jeffwashington:5ju1

Conversation

@jeffwashington
Copy link
Copy Markdown

@jeffwashington jeffwashington commented Jul 10, 2025

Problem

With the removal of rent scanning, we can cleanup, remove, and then simplify flushing and holding ranges. To facilitate that, we can refactor how scan iterates throug the accounts. Scanning accounts are deprecated behavior, but still possible to call.

From API's perspective, we should not provide APIs which holds Arc<AccountMapEntry> for an entry that is in the accounts-index. That could lead to race conditions with other accounts-db task, such as flush, clean, shrink etc. To mitigate this, we refactor the account index iterator to avoid returning internal data structures directly and instead return lightweight identifiers (i.e., pubkeys).

Summary of Changes

  1. remove items api
  2. change account index iterator to return just pubkeys
  3. update print_index and scan to take pubkey and lookup the accounts again.

Fixes #

Comment thread accounts-db/src/accounts_db.rs Outdated
Comment thread accounts-db/src/accounts_index.rs Outdated
Comment thread accounts-db/src/accounts_db.rs Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 11, 2025

Codecov Report

Attention: Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.2%. Comparing base (553fc1d) to head (44b9138).
Report is 17 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6923   +/-   ##
=======================================
  Coverage    83.2%    83.2%           
=======================================
  Files         853      853           
  Lines      377587   377566   -21     
=======================================
- Hits       314488   314476   -12     
+ Misses      63099    63090    -9     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

" slots: {:?}",
*account_entry.slot_list.read().unwrap()
);
for pubkey in map.keys() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown

@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.

This looks good to me!

Comment thread accounts-db/src/append_vec.rs
HaoranYi
HaoranYi previously approved these changes Jul 11, 2025
Copy link
Copy Markdown

@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.

lgtm.

We can remove AccountsIndexIterator too. Just use keys() and then filter on the range.
This way we don't need to reimplement iter without hold_range (#6917).
Just #6919 is enough to remove hold_range.

@HaoranYi
Copy link
Copy Markdown

HaoranYi commented Jul 11, 2025

Never mind. We still need AccountsIndexIterator, which iter pubkeys from all maps.
But since AccountsIndexIterator now just use fn keys() on the map, we don't need #6917 once this Pr is merged.
All we need is #6919 to remove hold_range. 😄

start_bin: usize,
end_bin_inclusive: usize,
items: Vec<(Pubkey, Arc<AccountMapEntry<T>>)>,
items: Vec<Pubkey>,
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: rename AccountsIndexIterator -> AccountsIndexPubkeyIterator?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

IMO let's rename in a separate PR.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

That's fine.

Comment thread accounts-db/src/accounts_index.rs Outdated
@@ -730,21 +730,27 @@ impl<T: IndexValue, U: DiskIndexValue + From<T> + Into<T>> AccountsIndex<T, U> {
for pubkey_list in self.iter(range.as_ref(), returns_items) {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: rename pubkey_list -> pubkeys?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

done

@HaoranYi HaoranYi changed the title accounts index iter returns copy Refactor Accounts Index and AccountsIndexIterator Jul 11, 2025
@HaoranYi
Copy link
Copy Markdown

I updated the PR title and description to reflect the new changes.

@jeffwashington jeffwashington marked this pull request as ready for review July 11, 2025 17:14
Copy link
Copy Markdown

@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 jeffwashington merged commit a056aac into anza-xyz:master Jul 11, 2025
41 checks passed
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.

4 participants