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.

Sign up for GitHub

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

Jump to bottom

allow test feature to skip rewrites #33851

Merged
merged 13 commits into from
Oct 27, 2023
Merged

allow test feature to skip rewrites #33851

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c2e406b
allow test feature to skip rewrites
jeffwashington Oct 24, 2023
d2868a5
hook up cli arg for test skip rewrites, update tests
Oct 25, 2023
fa36141
fix sanity checker
Oct 25, 2023
b6ebeb7
add account hash to abi to fix a test
Oct 25, 2023
238dda6
reviews
Oct 25, 2023
d596446
use hashmap to collect skip_rewrites. exclude skip_rewrites from dirty
Oct 25, 2023
1ae5737
accumulate skipped_rewrite in reduce
Oct 25, 2023
561b9a4
mutex
Oct 26, 2023
d986234
fmt
Oct 26, 2023
41c5fd6
skip hash verify for this test flag
Oct 26, 2023
b6bf790
add skipped rewrites num stat
Oct 26, 2023
70c1c01
skip bank hash verify not account hash verify
Oct 26, 2023
48c875d
reviews
Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
150 changes: 90 additions & 60 deletions accounts-db/src/accounts_db.rs
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion ledger-tool/src/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ pub fn get_accounts_db_config(
exhaustively_verify_refcounts: arg_matches.is_present("accounts_db_verify_refcounts"),
skip_initial_hash_calc: arg_matches.is_present("accounts_db_skip_initial_hash_calculation"),
test_partitioned_epoch_rewards,
..AccountsDbConfig::default()
test_skip_rewrites_but_include_in_bank_hash: arg_matches.is_present("accounts_db_test_skip_rewrites_but_include_in_bank_hash"),
..AccountsDbConfig::default(),
}
}

Expand Down
6 changes: 6 additions & 0 deletions ledger-tool/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,12 @@ fn main() {
"Debug option to scan all AppendVecs and verify account index refcounts prior to clean",
)
.hidden(hidden_unless_forced());
let accountsdb_test_skip_rewrites_but_include_in_bank_hash = Arg::with_name("accounts_db_test_skip_rewrites_but_include_in_bank_hash")
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved
.long("accounts-db-test-skip-rewrites-but-include-in-bank-hash")
.help(
"Debug option to skip rewrites for rent exempted accounts but still added them in bank delta hash calculation",
)
.hidden(hidden_unless_forced());
let accounts_filler_count = Arg::with_name("accounts_filler_count")
.long("accounts-filler-count")
.value_name("COUNT")
Expand Down
4 changes: 3 additions & 1 deletion runtime/benches/accounts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,9 @@ fn test_accounts_delta_hash(bencher: &mut Bencher) {
let mut pubkeys: Vec<Pubkey> = vec![];
create_test_accounts(&accounts, &mut pubkeys, 100_000, 0);
bencher.iter(|| {
accounts.accounts_db.calculate_accounts_delta_hash(0);
accounts
.accounts_db
.calculate_accounts_delta_hash(0, Vec::default());
});
}

Expand Down
41 changes: 39 additions & 2 deletions runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,13 @@ use {
AccountAddressFilter, Accounts, LoadedTransaction, PubkeyAccountSlot, RewardInterval,
TransactionLoadResult,
},
accounts_db::AccountsDb,
accounts_db::{
AccountShrinkThreshold, AccountStorageEntry, AccountsDbConfig,
CalcAccountsHashDataSource, VerifyAccountsHashAndLamportsConfig,
ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS, ACCOUNTS_DB_CONFIG_FOR_TESTING,
},
accounts_hash::AccountHash,
accounts_hash::{AccountsHash, CalcAccountsHashConfig, HashStats, IncrementalAccountsHash},
accounts_index::{AccountSecondaryIndexes, IndexKey, ScanConfig, ScanResult, ZeroLamport},
accounts_partition::{self, Partition, PartitionIndex},
Expand Down Expand Up @@ -508,6 +510,7 @@ impl PartialEq for Bank {
return true;
}
let Self {
skipped_rewrites: _,
rc: _,
status_cache: _,
blockhash_queue,
Expand Down Expand Up @@ -816,6 +819,10 @@ pub struct Bank {
/// The change to accounts data size in this Bank, due to off-chain events (i.e. rent collection)
accounts_data_size_delta_off_chain: AtomicI64,

/// until the skipped rewrites feature is activated, it is possible to skip rewrites and still include
/// the account hash of the accounts that would have been rewritten as bank hash expects.
skipped_rewrites: RwLock<Vec<(Pubkey, AccountHash)>>,
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved

/// Transaction fee structure
pub fee_structure: FeeStructure,

Expand Down Expand Up @@ -1014,6 +1021,7 @@ impl Bank {

fn default_with_accounts(accounts: Accounts) -> Self {
let mut bank = Self {
skipped_rewrites: RwLock::default(),
incremental_snapshot_persistence: None,
rc: BankRc::new(accounts, Slot::default()),
status_cache: Arc::<RwLock<BankStatusCache>>::default(),
Expand Down Expand Up @@ -1345,6 +1353,7 @@ impl Bank {

let accounts_data_size_initial = parent.load_accounts_data_size();
let mut new = Self {
skipped_rewrites: RwLock::default(),
incremental_snapshot_persistence: None,
rc,
status_cache,
Expand Down Expand Up @@ -1799,6 +1808,7 @@ impl Bank {
);
let stakes_accounts_load_duration = now.elapsed();
let mut bank = Self {
skipped_rewrites: RwLock::default(),
incremental_snapshot_persistence: fields.incremental_snapshot_persistence,
rc: bank_rc,
status_cache: Arc::<RwLock<BankStatusCache>>::default(),
Expand Down Expand Up @@ -6012,9 +6022,16 @@ impl Bank {
let mut time_collecting_rent_us = 0;
let mut time_storing_accounts_us = 0;
let can_skip_rewrites = self.bank_hash_skips_rent_rewrites();
let test_skip_rewrites_but_include_hash_in_bank_hash = !can_skip_rewrites
&& self
.rc
.accounts
.accounts_db
.test_skip_rewrites_but_include_in_bank_hash;
let set_exempt_rent_epoch_max: bool = self
.feature_set
.is_active(&solana_sdk::feature_set::set_exempt_rent_epoch_max::id());
let mut skipped_rewrites = Vec::default();
for (pubkey, account, _loaded_slot) in accounts.iter_mut() {
let (rent_collected_info, measure) =
measure!(self.rent_collector.collect_from_existing_account(
Expand All @@ -6030,7 +6047,9 @@ impl Bank {
// Also, there's another subtle side-effect from rewrites: this
// ensures we verify the whole on-chain state (= all accounts)
// via the bank delta hash slowly once per an epoch.
if !can_skip_rewrites || !Self::skip_rewrite(rent_collected_info.rent_amount, account) {
if (!can_skip_rewrites && !test_skip_rewrites_but_include_hash_in_bank_hash)
|| !Self::skip_rewrite(rent_collected_info.rent_amount, account)
{
if rent_collected_info.rent_amount > 0 {
if let Some(rent_paying_pubkeys) = rent_paying_pubkeys {
if !rent_paying_pubkeys.contains(pubkey) {
Expand Down Expand Up @@ -6060,9 +6079,23 @@ impl Bank {
}
total_rent_collected_info += rent_collected_info;
accounts_to_store.push((pubkey, account));
} else if test_skip_rewrites_but_include_hash_in_bank_hash {
// include rewrites that we skipped in the accounts delta hash.
// This is what consensus requires prior to activation of bank_hash_skips_rent_rewrites.
// This code path exists to allow us to test the long term effects on validators when the skipped rewrites
// feature is enabled.
let hash = AccountsDb::hash_account(account, &pubkey);
skipped_rewrites.push((*pubkey, hash));
}
rent_debits.insert(pubkey, rent_collected_info.rent_amount, account.lamports());
}
if test_skip_rewrites_but_include_hash_in_bank_hash {
// these account pubkeys will have to be added to the bank hash's accounts delta hash
self.skipped_rewrites
.write()
.unwrap()
.append(&mut skipped_rewrites);
brooksprumo marked this conversation as resolved.
Show resolved Hide resolved
}

if !accounts_to_store.is_empty() {
// TODO: Maybe do not call `store_accounts()` here. Instead return `accounts_to_store`
Expand Down Expand Up @@ -7068,7 +7101,11 @@ impl Bank {
.rc
.accounts
.accounts_db
.calculate_accounts_delta_hash_internal(slot, ignore);
.calculate_accounts_delta_hash_internal(
slot,
ignore,
std::mem::take(&mut self.skipped_rewrites.write().unwrap()),
);

let mut signature_count_buf = [0u8; 8];
LittleEndian::write_u64(&mut signature_count_buf[..], self.signature_count());
Expand Down
4 changes: 2 additions & 2 deletions runtime/src/bank/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1957,7 +1957,7 @@ fn test_collect_rent_from_accounts() {
.rc
.accounts
.accounts_db
.get_pubkey_hash_for_slot(later_slot)
.get_pubkey_hash_for_slot(later_slot, Vec::default())
.0;
assert_eq!(
!deltas
Expand Down Expand Up @@ -14074,7 +14074,7 @@ fn test_last_restart_slot() {
.rc
.accounts
.accounts_db
.get_pubkey_hash_for_slot(bank.slot());
.get_pubkey_hash_for_slot(bank.slot(), Vec::default());
let dirty_accounts: HashSet<_> = dirty_accounts
.into_iter()
.map(|(pubkey, _hash)| pubkey)
Expand Down
38 changes: 21 additions & 17 deletions runtime/src/serde_snapshot/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,9 @@ mod serde_snapshot_tests {
create_test_accounts(&accounts, &mut pubkeys, 100, slot);
check_accounts_local(&accounts, &pubkeys, 100);
accounts.add_root(slot);
let accounts_delta_hash = accounts.accounts_db.calculate_accounts_delta_hash(slot);
let accounts_delta_hash = accounts
.accounts_db
.calculate_accounts_delta_hash(slot, Vec::default());
let accounts_hash = AccountsHash(Hash::new_unique());
accounts
.accounts_db
Expand Down Expand Up @@ -270,7 +272,9 @@ mod serde_snapshot_tests {
.unwrap(),
);
check_accounts_local(&daccounts, &pubkeys, 100);
let daccounts_delta_hash = daccounts.accounts_db.calculate_accounts_delta_hash(slot);
let daccounts_delta_hash = daccounts
.accounts_db
.calculate_accounts_delta_hash(slot, Vec::default());
assert_eq!(accounts_delta_hash, daccounts_delta_hash);
let daccounts_hash = daccounts.accounts_db.get_accounts_hash(slot).unwrap().0;
assert_eq!(accounts_hash, daccounts_hash);
Expand Down Expand Up @@ -300,7 +304,7 @@ mod serde_snapshot_tests {
db.store_for_tests(new_root, &[(&key2, &account0)]);
db.add_root_and_flush_write_cache(new_root);

db.calculate_accounts_delta_hash(new_root);
db.calculate_accounts_delta_hash(new_root, Vec::default());
db.update_accounts_hash_for_tests(new_root, &linear_ancestors(new_root), false, false);

// Simulate reconstruction from snapshot
Expand Down Expand Up @@ -339,7 +343,7 @@ mod serde_snapshot_tests {
accounts.add_root_and_flush_write_cache(0);
accounts.check_storage(0, 100);
accounts.check_accounts(&pubkeys, 0, 100, 2);
accounts.calculate_accounts_delta_hash(0);
accounts.calculate_accounts_delta_hash(0, Vec::default());

let mut pubkeys1: Vec<Pubkey> = vec![];

Expand All @@ -357,7 +361,7 @@ mod serde_snapshot_tests {
// accounts
accounts.create_account(&mut pubkeys1, latest_slot, 10, 0, 0);

accounts.calculate_accounts_delta_hash(latest_slot);
accounts.calculate_accounts_delta_hash(latest_slot, Vec::default());
accounts.add_root_and_flush_write_cache(latest_slot);
accounts.check_storage(1, 21);

Expand All @@ -377,7 +381,7 @@ mod serde_snapshot_tests {
// 21 + 10 = 31 accounts
accounts.create_account(&mut pubkeys2, latest_slot, 10, 0, 0);

accounts.calculate_accounts_delta_hash(latest_slot);
accounts.calculate_accounts_delta_hash(latest_slot, Vec::default());
accounts.add_root_and_flush_write_cache(latest_slot);
accounts.check_storage(2, 31);

Expand Down Expand Up @@ -475,7 +479,7 @@ mod serde_snapshot_tests {

accounts.print_accounts_stats("accounts_post_purge");

accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.update_accounts_hash_for_tests(
current_slot,
&linear_ancestors(current_slot),
Expand Down Expand Up @@ -532,7 +536,7 @@ mod serde_snapshot_tests {
accounts.add_root_and_flush_write_cache(current_slot);

accounts.print_accounts_stats("pre_f");
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.update_accounts_hash_for_tests(4, &Ancestors::default(), false, false);

let accounts = f(accounts, current_slot);
Expand Down Expand Up @@ -629,7 +633,7 @@ mod serde_snapshot_tests {
accounts.add_root_and_flush_write_cache(current_slot);

accounts.print_count_and_status("before reconstruct");
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.update_accounts_hash_for_tests(
current_slot,
&linear_ancestors(current_slot),
Expand Down Expand Up @@ -672,7 +676,7 @@ mod serde_snapshot_tests {
current_slot += 1;
accounts.store_for_tests(current_slot, &[(&pubkey1, &account)]);
accounts.store_for_tests(current_slot, &[(&pubkey2, &account)]);
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.add_root(current_slot);

// B: Test multiple updates to pubkey1 in a single slot/storage
Expand All @@ -687,15 +691,15 @@ mod serde_snapshot_tests {
// Stores to same pubkey, same slot only count once towards the
// ref count
assert_eq!(2, accounts.ref_count_for_pubkey(&pubkey1));
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());

// C: Yet more update to trigger lazy clean of step A
current_slot += 1;
assert_eq!(2, accounts.ref_count_for_pubkey(&pubkey1));
accounts.store_for_tests(current_slot, &[(&pubkey1, &account3)]);
accounts.add_root_and_flush_write_cache(current_slot);
assert_eq!(3, accounts.ref_count_for_pubkey(&pubkey1));
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.add_root_and_flush_write_cache(current_slot);

// D: Make pubkey1 0-lamport; also triggers clean of step B
Expand Down Expand Up @@ -727,13 +731,13 @@ mod serde_snapshot_tests {
3, /* == 3 - 1 + 1 */
accounts.ref_count_for_pubkey(&pubkey1)
);
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.add_root(current_slot);

// E: Avoid missing bank hash error
current_slot += 1;
accounts.store_for_tests(current_slot, &[(&dummy_pubkey, &dummy_account)]);
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.add_root(current_slot);

accounts.assert_load_account(current_slot, pubkey1, zero_lamport);
Expand Down Expand Up @@ -764,7 +768,7 @@ mod serde_snapshot_tests {
// F: Finally, make Step A cleanable
current_slot += 1;
accounts.store_for_tests(current_slot, &[(&pubkey2, &account)]);
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like we should probably make a calculate_accounts_delta_hash_with_skips fn that takes the Vec and leave the signature of calculate_accounts_delta_hash the same.
I should've done that initially. That would remove all this test noise. Once all features are activated, we'll be removing this arg and all these tests would just have to go back.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah. updated.

accounts.add_root(current_slot);

// Do clean
Expand Down Expand Up @@ -805,7 +809,7 @@ mod serde_snapshot_tests {
accounts.store_for_tests(current_slot, &[(pubkey, &account)]);
}
let shrink_slot = current_slot;
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.add_root_and_flush_write_cache(current_slot);

current_slot += 1;
Expand All @@ -815,7 +819,7 @@ mod serde_snapshot_tests {
for pubkey in updated_pubkeys {
accounts.store_for_tests(current_slot, &[(pubkey, &account)]);
}
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.add_root_and_flush_write_cache(current_slot);

accounts.clean_accounts_for_tests();
Expand Down
2 changes: 1 addition & 1 deletion runtime/src/snapshot_minimizer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ mod tests {
minimized_account_set.insert(*pubkey);
}
}
accounts.calculate_accounts_delta_hash(current_slot);
accounts.calculate_accounts_delta_hash(current_slot, Vec::default());
accounts.add_root_and_flush_write_cache(current_slot);
}

Expand Down
6 changes: 6 additions & 0 deletions validator/src/cli.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1194,6 +1194,12 @@ pub fn app<'a>(version: &'a str, default_args: &'a DefaultArgs) -> App<'a, 'a> {
.help("Debug option to scan all append vecs and verify account index refcounts prior to clean")
.hidden(hidden_unless_forced())
)
.arg(
Arg::with_name("accounts_db_test_skip_rewrites_but_include_in_bank_hash")
.long("accounts-db-test-skip-rewrites-but-include-in-bank-hash")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

my words here are so verbose. @brooksprumo or @HaoranYi do you have any better ideas?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about just accounts-db-test-skip-rewrites? Since it's testing-only and hidden, it really only needs to make sense to us.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes. updated.

.help("Debug option to skip rewrites for rent exempted accounts but still added them in bank delta hash calculation")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rent exempt. Remove ed
add them. Remove ed

Copy link
Contributor

Choose a reason for hiding this comment

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

yes updated.

.hidden(hidden_unless_forced())
)
.arg(
Arg::with_name("no_skip_initial_accounts_db_clean")
.long("no-skip-initial-accounts-db-clean")
Expand Down
2 changes: 2 additions & 0 deletions validator/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1206,6 +1206,8 @@ pub fn main() {
.then_some(CreateAncientStorage::Pack)
.unwrap_or_default(),
test_partitioned_epoch_rewards,
test_skip_rewrites_but_include_in_bank_hash: matches
.is_present("accounts_db_test_skip_rewrites_but_include_in_bank_hash"),
..AccountsDbConfig::default()
};

Expand Down