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

allow test feature to skip rewrites #33851

Merged
merged 13 commits into from
Oct 27, 2023
150 changes: 90 additions & 60 deletions accounts-db/src/accounts_db.rs

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion accounts-db/src/accounts_hash.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1243,7 +1243,7 @@ pub enum ZeroLamportAccounts {

/// Hash of an account
#[repr(transparent)]
#[derive(Debug, Copy, Clone, Eq, PartialEq, Pod, Zeroable)]
#[derive(Debug, Copy, Clone, Eq, PartialEq, Pod, Zeroable, AbiExample)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this is caused by the addition of the rewrites on Bank?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah.

pub struct AccountHash(pub Hash);

// Ensure the newtype wrapper never changes size from the underlying Hash
Expand Down
2 changes: 2 additions & 0 deletions ledger-tool/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +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,
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
13 changes: 13 additions & 0 deletions ledger-tool/src/main.rs
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 Expand Up @@ -1556,6 +1562,7 @@ fn main() {
.arg(&disable_disk_index)
.arg(&accountsdb_verify_refcounts)
.arg(&accounts_db_skip_initial_hash_calc_arg)
.arg(&accountsdb_test_skip_rewrites_but_include_in_bank_hash)
)
.subcommand(
SubCommand::with_name("shred-meta")
Expand All @@ -1573,6 +1580,7 @@ fn main() {
.arg(&disable_disk_index)
.arg(&accountsdb_verify_refcounts)
.arg(&accounts_db_skip_initial_hash_calc_arg)
.arg(&accountsdb_test_skip_rewrites_but_include_in_bank_hash)
)
.subcommand(
SubCommand::with_name("bounds")
Expand Down Expand Up @@ -1608,6 +1616,7 @@ fn main() {
.arg(&disable_disk_index)
.arg(&accountsdb_skip_shrink)
.arg(&accountsdb_verify_refcounts)
.arg(&accountsdb_test_skip_rewrites_but_include_in_bank_hash)
.arg(&accounts_filler_count)
.arg(&accounts_filler_size)
.arg(&verify_index_arg)
Expand Down Expand Up @@ -1688,6 +1697,7 @@ fn main() {
.arg(&accounts_index_limit)
.arg(&disable_disk_index)
.arg(&accountsdb_verify_refcounts)
.arg(&accountsdb_test_skip_rewrites_but_include_in_bank_hash)
.arg(&accounts_db_skip_initial_hash_calc_arg)
.arg(&halt_at_slot_arg)
.arg(&hard_forks_arg)
Expand Down Expand Up @@ -1724,6 +1734,7 @@ fn main() {
.arg(&accounts_index_limit)
.arg(&disable_disk_index)
.arg(&accountsdb_verify_refcounts)
.arg(&accountsdb_test_skip_rewrites_but_include_in_bank_hash)
.arg(&accounts_db_skip_initial_hash_calc_arg)
.arg(&accountsdb_skip_shrink)
.arg(&ancient_append_vecs)
Expand Down Expand Up @@ -1918,6 +1929,7 @@ fn main() {
.arg(&accounts_index_limit)
.arg(&disable_disk_index)
.arg(&accountsdb_verify_refcounts)
.arg(&accountsdb_test_skip_rewrites_but_include_in_bank_hash)
.arg(&accounts_db_skip_initial_hash_calc_arg)
.arg(&halt_at_slot_arg)
.arg(&hard_forks_arg)
Expand Down Expand Up @@ -1952,6 +1964,7 @@ fn main() {
.arg(&accounts_index_limit)
.arg(&disable_disk_index)
.arg(&accountsdb_verify_refcounts)
.arg(&accountsdb_test_skip_rewrites_but_include_in_bank_hash)
.arg(&accounts_db_skip_initial_hash_calc_arg)
.arg(&halt_at_slot_arg)
.arg(&hard_forks_arg)
Expand Down
4 changes: 3 additions & 1 deletion runtime/benches/accounts.rs
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
45 changes: 41 additions & 4 deletions runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,13 @@ use {
TransactionLoadResult,
},
accounts_db::{
AccountShrinkThreshold, AccountStorageEntry, AccountsDbConfig,
AccountShrinkThreshold, AccountStorageEntry, AccountsDb, AccountsDbConfig,
CalcAccountsHashDataSource, VerifyAccountsHashAndLamportsConfig,
ACCOUNTS_DB_CONFIG_FOR_BENCHMARKS, ACCOUNTS_DB_CONFIG_FOR_TESTING,
},
accounts_hash::{AccountsHash, CalcAccountsHashConfig, HashStats, IncrementalAccountsHash},
accounts_hash::{
AccountHash, AccountsHash, CalcAccountsHashConfig, HashStats, IncrementalAccountsHash,
},
accounts_index::{AccountSecondaryIndexes, IndexKey, ScanConfig, ScanResult, ZeroLamport},
accounts_partition::{self, Partition, PartitionIndex},
accounts_update_notifier_interface::AccountsUpdateNotifier,
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
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
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
Loading