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

Clones skipped rewrites instead of taking #34311

Merged
merged 2 commits into from
Dec 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion runtime/src/bank.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6909,7 +6909,7 @@ impl Bank {
.calculate_accounts_delta_hash_internal(
slot,
ignore,
std::mem::take(&mut self.skipped_rewrites.lock().unwrap()),
self.skipped_rewrites.lock().unwrap().clone(),
Copy link
Contributor

Choose a reason for hiding this comment

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

another option, with no runtime cost, is to pass &RwLock<HashMap...>>.

Copy link
Contributor

Choose a reason for hiding this comment

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

we could also use something like the OnceLock

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! I originally didn't want to modify calculate_accounts_delta_hash_internal(), which takes the skipped rewrites by value and mutates it.

We can borrow the skipped rewrites and pass a reference into calculate_accounts_delta_hash_internal(). Inside though, we need to only add the rewrites that were not already in the vec of modified accounts in this slot. Currently that iterates the vec of modified accounts and removes any matches in the skipped rewrites.

If we do not modify the skipped rewrites, then I think we'll need to selectively add the skipped rewrites to the vec by searching the vec for each pubkey that's in the skipped rewrites. This feel slow, but I'm not sure if it's faster/slower than the clone.

Wdyt? Or would you like me to get runtime numbers for both versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh. I forgot we modify it. clone is fine.

);

let mut signature_count_buf = [0u8; 8];
Expand Down
61 changes: 61 additions & 0 deletions runtime/src/bank/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13905,3 +13905,64 @@ fn test_filter_executable_program_accounts_invalid_blockhash() {
);
assert_eq!(lock_results[1].0, Err(TransactionError::BlockhashNotFound));
}

/// Test that rehashing works with skipped rewrites
///
/// Since `bank_to_xxx_snapshot_archive()` calls `Bank::rehash()`, we must ensure that rehashing
/// works properly when also using `test_skip_rewrites_but_include_in_bank_hash`.
#[test]
fn test_rehash_with_skipped_rewrites() {
let accounts_db_config = AccountsDbConfig {
test_skip_rewrites_but_include_in_bank_hash: true,
..ACCOUNTS_DB_CONFIG_FOR_TESTING
};
let bank = Arc::new(Bank::new_with_paths(
&GenesisConfig::default(),
Arc::new(RuntimeConfig::default()),
Vec::default(),
None,
None,
AccountSecondaryIndexes::default(),
AccountShrinkThreshold::default(),
false,
Some(accounts_db_config),
None,
Arc::new(AtomicBool::new(false)),
));
// This test is only meaningful while the bank hash contains rewrites.
// Once this feature is enabled, it may be possible to remove this test entirely.
assert!(!bank.bank_hash_skips_rent_rewrites());

// Store an account *in this bank* that will be checked for rent collection *in the next bank*
let pubkey = {
let rent_collection_partition = bank
.variable_cycle_partitions_between_slots(bank.slot(), bank.slot() + 1)
.last()
.copied()
.unwrap();
let pubkey_range =
accounts_partition::pubkey_range_from_partition(rent_collection_partition);
*pubkey_range.end()
};
let mut account = AccountSharedData::new(123_456_789, 0, &Pubkey::default());
// The account's rent epoch must be set to EXEMPT
// in order for its rewrite to be skipped by rent collection.
account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH);
bank.store_account_and_update_capitalization(&pubkey, &account);

// Create a new bank that will do rent collection on the account stored in the previous slot
let bank = Arc::new(Bank::new_from_parent(
bank.clone(),
&Pubkey::new_unique(),
bank.slot() + 1,
));

// Freeze the bank to trigger rent collection and hash calculation
bank.freeze();

// Ensure the bank hash is the same before and after rehashing
let bank_hash = bank.hash();
bank.rehash();
let bank_rehash = bank.hash();
assert_eq!(bank_rehash, bank_hash);
}