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

Set rent_epoch for updated sysvars to RENT_EXEMPT_EPOCH #3398

Closed
wants to merge 6 commits into from
Closed

Set rent_epoch for updated sysvars to RENT_EXEMPT_EPOCH #3398

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d5f7e09
syavar rent_epoch
Oct 31, 2024
df65770
fix test
Oct 31, 2024
6c37fa2
fix one more test
Oct 31, 2024
b956c4d
pr
Nov 5, 2024
12f100a
fmtwq
Nov 5, 2024
7ac4965
update comments
Nov 6, 2024
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
11 changes: 10 additions & 1 deletion runtime/src/bank.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ use {
packet::PACKET_DATA_SIZE,
precompiles::get_precompiles,
pubkey::Pubkey,
rent_collector::{CollectedInfo, RentCollector},
rent_collector::{CollectedInfo, RentCollector, RENT_EXEMPT_RENT_EPOCH},
rent_debits::RentDebits,
reserved_account_keys::ReservedAccountKeys,
reward_info::RewardInfo,
Expand Down Expand Up @@ -1940,6 +1940,15 @@ impl Bank {
// More generally, this code always re-calculates for possible sysvar data size change,
// although there is no such sysvars currently.
self.adjust_sysvar_balance_for_rent(&mut new_account);

// When new sysvar comes into existence, this code ensures that the
// sysvar's rent_epoch is set to RENT_EXEMPT_EPOCH (i.e. U64::MAX). This
// is because sysvars are never deleted, and are always rent exempt.
// Currently, it relies on rent collection code to update rent_epoch to
// RENT_EXEMPT_EPOCH. In the future, we will remove rent collection
// code. Therefore, we should set rent_epoch here, and don't depend on
// rent collection code to update rent_epoch..
new_account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH);
self.store_account_and_update_capitalization(pubkey, &new_account);
}

Expand Down
35 changes: 21 additions & 14 deletions runtime/src/bank/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3951,7 +3951,6 @@ fn test_bank_update_sysvar_account() {
// (since rent collection will update the rent epoch, thus causing the
// subsequent checks to fail spuriously).
let dummy_clock_id = Pubkey::from_str_const("64jsX5hwtsjsKR7eNcNU4yhgwjuXoU9KR2MpnV47iXXz");
let dummy_rent_epoch = 44;
let (mut genesis_config, _mint_keypair) = create_genesis_config(500);

let expected_previous_slot = 3;
Expand All @@ -3974,22 +3973,20 @@ fn test_bank_update_sysvar_account() {
bank1.update_sysvar_account(&dummy_clock_id, |optional_account| {
assert!(optional_account.is_none());

let mut account = create_account(
create_account(
&Clock {
slot: expected_previous_slot,
..Clock::default()
},
bank1.inherit_specially_retained_account_fields(optional_account),
);
account.set_rent_epoch(dummy_rent_epoch);
account
)
});
let current_account = bank1.get_account(&dummy_clock_id).unwrap();
assert_eq!(
expected_previous_slot,
from_account::<Clock, _>(&current_account).unwrap().slot
);
assert_eq!(dummy_rent_epoch, current_account.rent_epoch());
assert_eq!(RENT_EXEMPT_RENT_EPOCH, current_account.rent_epoch());
},
|old, new| {
assert_eq!(
Expand Down Expand Up @@ -4053,7 +4050,7 @@ fn test_bank_update_sysvar_account() {
expected_next_slot,
from_account::<Clock, _>(&current_account).unwrap().slot
);
assert_eq!(dummy_rent_epoch, current_account.rent_epoch());
assert_eq!(RENT_EXEMPT_RENT_EPOCH, current_account.rent_epoch());
},
|old, new| {
// if existing, capitalization shouldn't change
Expand Down Expand Up @@ -6472,30 +6469,29 @@ fn test_bank_hash_consistency() {
assert_eq!(bank.get_slots_in_epoch(0), 32);
loop {
goto_end_of_slot(bank.clone());

if bank.slot == 0 {
assert_eq!(
bank.hash().to_string(),
"Hn2FoJuoFWXVFVnwcQ6peuT24mUPmhDtXHXVjKD7M4yP",
"5twxs7hhHtqsMWjW3CVQdxp5f9FVTYtFs4obs5aPAPeY",
);
}

if bank.slot == 32 {
assert_eq!(
bank.hash().to_string(),
"7FPfwBut4b7bXtKPsobQS1cuFgF47SZHDb4teQcJRomv"
"7bLTgyPMtNYd853kzSgiFjipQFcCGxNCqCEbtymmjRBS"
);
}
if bank.slot == 64 {
assert_eq!(
bank.hash().to_string(),
"28CWiEuA3izdt5xe4LyS4Q1DTALmYgrVctSTazFiPVcW"
"G7yjmpzektRBH6KcQs7UXpueb1U5Xw8eL9dm7qiqjHLz"
);
}
if bank.slot == 128 {
assert_eq!(
bank.hash().to_string(),
"AdCmEvRXWKpvXb9fG6AFQhzGgB5ciAXnDajvaNK7YUg8"
"F5Z5MfY4pgpHKu6HrSc2bMqf3MXyNdHKaWYsponpSzVT"
);
break;
}
Expand Down Expand Up @@ -12376,6 +12372,17 @@ where
let bank = new_from_parent_next_epoch(bank, &bank_forks, 2);
bank.freeze(); // trigger rent collection

// advance to the next slot to create a new bank, which is cleanable. The
// previous bank is at epoch boundary. An epoch boundary bank is not
// cleanable because epoch rewards sysvar is written in this slot. Before
// #3398, it was cleanable because rent collection will rewrite the epoch
// rewards sysvar due to rent_epoch not setting to u64::MAX. With #3398,
// rent_epoch is set to u64::MAX at creating time, so epoch rewards sysvar
// is not rewritten. Therefore, the slot at epoch boundary is now
// uncleanable. This is why we need to advance to the next slot.
Copy link

Choose a reason for hiding this comment

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

I'm not sure I follow. The first slot in an epoch cannot be cleaned or should not be cleaned? If it is "cannot", then are there broader implications in AccountsBackgroundService and snapshots too (since there's nothing that prevents clean from running on the first slot in an epoch)? If it is "should not", is that only for this test, or does that impact other code as well?

Copy link
Author

@HaoranYi HaoranYi Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It is "should not". And it is only because the way the test is written. More details are as follows.

Without the PR, epoch reward sysvar is created with rent_epoch = 0 at the epoch boundary slot X. And in a late slot Y, when rent collection runs, it finds that the rent_epoch is not MAX, so it write epoch reward sysvar at Y. This will make epoch reward sysvar dead in slot X. So clean can remove slot X.

However, with this PR, epoch reward sysvar is created with rent_epoch = MAX at the epoch boundary slot X. In the late slot Y, when rent collection runs, it find that rent_epoch is already at MAX, so no need to write epoch rewards sysvar at Y. Therefore, the sysvar is still alive at slot X. Clean can't remove slot X.

In this test, the main intent is to test clean to remove a dead slot when all of its account are overwritten in a later slots. Unfortunately, we pick the first slot of the epoch to do the test (a bad choice). Because of the epoch reward sysvar was implicitly created at the first slot of the epoch (which could be live or dead depends on rent collection), and the test doesn't have control over this sysvar, we should move our test of clean one slot later, so that we can just test clean with user accounts that we can control without worrying about the sysvar accounts and rent collection ...

Copy link

Choose a reason for hiding this comment

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

I think it might be better not to refer to PR numbers in comments.

Copy link
Author

Choose a reason for hiding this comment

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

yes. updated the comments.

let slot = bank.slot() + 1;
let bank = new_bank_from_parent_with_bank_forks(bank_forks.as_ref(), bank, &collector, slot);

// create zero-lamports account to be cleaned
let account = AccountSharedData::new(0, len1, &program);
let slot = bank.slot() + 1;
Expand Down Expand Up @@ -12423,9 +12430,9 @@ fn test_create_zero_lamport_with_clean() {
bank.squash();
bank.force_flush_accounts_cache();
// do clean and assert that it actually did its job
assert_eq!(5, bank.get_snapshot_storages(None).len());
assert_eq!(6, bank.get_snapshot_storages(None).len());
bank.clean_accounts();
assert_eq!(4, bank.get_snapshot_storages(None).len());
assert_eq!(5, bank.get_snapshot_storages(None).len());
});
}

Expand Down
Loading