-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
add filler accounts to bloat validator and predict failure #20491
Conversation
@carllin - I just got this working fully. Maybe some cosmetic things wrong with it. Please take a look so I can get pass 1 of your feedback. One variation: I considered making these dummy accounts 'executable' so that we don't collect rent from them and I don't have to plumb through pubkey checking. |
runtime/src/accounts_db.rs
Outdated
@@ -6645,7 +6792,9 @@ impl AccountsDb { | |||
for slot_stores in self.storage.0.iter() { | |||
for (id, store) in slot_stores.value().read().unwrap().iter() { | |||
// Should be default at this point | |||
assert_eq!(store.alive_bytes(), 0); | |||
if self.filler_account_count == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might also be good to sanity check in generate_index
that none of the accounts in the snapshot match the filler_account_suffix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this if was left over from previous attempts and has been removed.
I added an assert
runtime/src/accounts_db.rs
Outdated
@@ -1972,6 +1992,10 @@ impl AccountsDb { | |||
let mut purges_zero_lamports = HashMap::new(); | |||
let mut purges_old_accounts = Vec::new(); | |||
for pubkey in pubkeys { | |||
if self.is_filler_account(pubkey) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm when rent runs and updates these accounts, won't we need to run clean in order to remove the old update from the old storage entries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I think you're right. Not sure why I left this in there.
if self.is_filler_account(loaded_account.pubkey()) { | ||
None | ||
} else { | ||
// Cache only has one version per key, don't need to worry about versioning | ||
Some((*loaded_account.pubkey(), loaded_account.loaded_hash())) | ||
} | ||
}, | ||
|accum: &DashMap<Pubkey, (u64, Hash)>, loaded_account: LoadedAccount| { | ||
let loaded_write_version = loaded_account.write_version(); | ||
let loaded_hash = loaded_account.loaded_hash(); | ||
if self.is_filler_account(loaded_account.pubkey()) { | ||
return; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, glad to see this worked for getting around the updates caused by rent.
Codecov Report
@@ Coverage Diff @@
## master #20491 +/- ##
=========================================
- Coverage 82.0% 82.0% -0.1%
=========================================
Files 494 494
Lines 137478 137611 +133
=========================================
+ Hits 112786 112871 +85
- Misses 24692 24740 +48 |
dc69f7c
to
60015b7
Compare
60015b7
to
c51b88b
Compare
@@ -6426,7 +6427,9 @@ impl AccountsDb { | |||
let accounts = storage.all_accounts(); | |||
accounts.into_iter().for_each(|stored_account| { | |||
let this_version = stored_account.meta.write_version; | |||
match accounts_map.entry(stored_account.meta.pubkey) { | |||
let pubkey = stored_account.meta.pubkey; | |||
assert!(!self.is_filler_account(&pubkey)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also sanity checked that this check only runs when the filler accounts are passed in, so shouldn't trigger during normal validator operation.
…bs#20491) * add filler accounts to bloat validator and predict failure * assert no accounts match filler * cleanup magic numbers * panic if can't load from snapshot with filler accounts specified * some renames * renames * into_par_iter * clean filler accts, too
…olana-labs#20491)" This reverts commit 9f04d7f.
Problem
For ledger-tool verify or a validator, you can specify --accounts-filler-count 10000000 and we will add 10M dummy accounts in the append vecs and acct idx to clog things up. These accounts will not be used to calculate hashes and they won't have rent collected from them. But, they will bloat the appendvecs, acct idx, rent collection and all other algorithms while running against mnb or whatever. The idea is that if we keep a validator like this running with extra accounts, then when it ooms or otherwise fails, it gives us an indication that other validators may be nearing failure.
Note this currently only works when starting from a snapshot.
Summary of Changes
Fixes #