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

allow test feature to skip rewrites #33851

Merged
merged 13 commits into from
Oct 27, 2023
+120 −15
Merged

allow test feature to skip rewrites #33851

Show file tree
Hide file tree
Changes from 1 commit
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
117 changes: 58 additions & 59 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
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