modify geyser account iter at snapshot load #960
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #960 +/- ##
=========================================
- Coverage 81.9% 81.8% -0.1%
=========================================
Files 853 853
Lines 231797 231813 +16
=========================================
- Hits 189845 189813 -32
- Misses 41952 42000 +48 |
| if let Some(highest_i) = accounts_duplicate.get(account.pubkey()) { | ||
| if highest_i != &i { | ||
| // this pubkey is in this storage twice and the current instance is not the last one, so we skip it. | ||
| // We only send unique accounts in this slot to `notify_filtered_accounts` | ||
| return; | ||
| } |
There was a problem hiding this comment.
Do we need to do move where account_len is incremented? Or maybe before we call return we also decrement? Otherwise I think we'll over count the number of accounts if there are duplicates.
There was a problem hiding this comment.
i wondered this, too. in the previous impl, we incremented account_len for each account in accounts.for_each(|account| {. This would have included duplicates already. The impl in this pr should be exactly the same behavior as the previous impl.
There was a problem hiding this comment.
Gotcha. Yeah, that's fair. Won't be an issue anywhere until we have ancient append vecs. And if we make pack the default too, then by then we'll never hit this issue (and could remove this duplicates handling entirely).
There was a problem hiding this comment.
even still, ancient append vecs should in practice never have duplicates. but it is possible I think.
| if let Some(highest_i) = accounts_duplicate.get(account.pubkey()) { | ||
| if highest_i != &i { | ||
| // this pubkey is in this storage twice and the current instance is not the last one, so we skip it. | ||
| // We only send unique accounts in this slot to `notify_filtered_accounts` | ||
| return; | ||
| } |
There was a problem hiding this comment.
Gotcha. Yeah, that's fair. Won't be an issue anywhere until we have ancient append vecs. And if we make pack the default too, then by then we'll never hit this issue (and could remove this duplicates handling entirely).
Problem
stop mmapping storage files.
storages will no longer be able to return refs to many accounts at a time.
Summary of Changes
populate geyser at snapshot load using scans, which can handle the refs appropriately. This requires 2 passes to deal with duplicates effectively.
Fixes #