feature: set rent_epoch to Epoch::MAX

[jeffwashington]

Problem

rent_epoch is no longer being updated for rent exempt accounts.
But, we have to save the value of rent_epoch per account, since it is used in each account's hash.

Summary of Changes

fix rent_epoch field at a known sentinel value for all rent-exempt accounts. This allows us to avoid storing this field per account. This reduces the cost per-account.

Fixes #
Feature Gate Issue: #28683

[jackcmay]

Looking good. One nit, it would be cleaner if all_features_except came in on a separate pr. No biggie this time, but nice to isolate the functional changes from the infra changes.

[jeffwashington]

Looking good. One nit, it would be cleaner if all_features_except came in on a separate pr. No biggie this time, but nice to isolate the functional changes from the infra changes.

Yes. I'm working on breaking it up already to make the feature alone. This pr just passed the ci for the first time yesterday ;-)

[brooksprumo]

Lgtm

[behzadnouri]

@jeffwashington Can you please confirm that this does not introduce discrepancy between accounts-db and stakes-cache?

[jeffwashington]

@jeffwashington Can you please confirm that this does not introduce discrepancy between accounts-db and stakes-cache?

My guess is there could be discrepancy. As soon as the epoch begins where the feature is activated, every time a tx loads an account or rent is collected, rent_epoch will be set to Epoch::MAX. Certainly the first few slots in the new epoch, if we skip slots (which we will), some of the stake accounts will have rent collected (they are rent exempt), but they will be rewritten with an update to rent_epoch. This will be inconsistent with the stakes cache. I assume stakes cache does a read(from cache)-modify-write?

One possible fix is when the feature is activated, we go through the stakes cache and update all rent_epoch fields. But, this won't match the accounts in accounts db until each account is used in a tx or had rent collected.
So, we could just update the accounts whose rent collection partition has been reached as we retry on each slot to give rewards.
Or, the rewards store code could check the feature flag and write all accounts with the new rent_epoch once the feature is activated.
This is a step function change so currently rent_epoch is frozen in place for rent exempt accounts. As of this change, from feature activation, rent_epoch will be fixed at Epoch::MAX. So if the store code in rewards checked the feature flag and updated rent_epoch, it won't matter(?) if the stakes cache is inconsistent.
When is the stakes cache repopulated from accounts db?
This seems solvable, but there are details I'm missing.

Good job of identifying this issue ahead of time.

[behzadnouri]

When is the stakes cache repopulated from accounts db?

Never afaik.
The stakes cache is presumed to be consistent with accounts-db when the node starts up. Then every write to accounts-db is intercepted to check that if it is writing to a vote or stake account, and if so mirrors the mutation into the stakes-cache as well.
If no accounts-db write is missed or bypassed then stakes-cache should stay consistent with accounts-db.
Otherwise, if some accounts-db mutations bypass that intercepting code, then the cache will become inconsistent with accounts-db.

Also as a reminder, we did #26479 to make and keep the stakes-cache consistent with accounts-db. Seems like this change is reintroducing some discrepancy.

[jeffwashington]

Here's where rent collection will adjust rent_epoch:

solana/runtime/src/bank.rs

Lines 5283 to 5288 in cac52b0

	measure!(self.rent_collector.collect_from_existing_account(
	pubkey,
	account,
	self.rc.accounts.accounts_db.filler_account_suffix.as_ref(),
	set_exempt_rent_epoch_max,
	));

Then we do the store:

solana/runtime/src/bank.rs

Lines 5322 to 5326 in cac52b0

	let (_, measure) = measure!(self.store_accounts((
	self.slot(),
	&accounts_to_store[..],
	self.include_slot_in_hash()
	)));

store updates the stakes cache:

solana/runtime/src/bank.rs

Lines 6180 to 6188 in cac52b0

	pub fn store_accounts<'a, T: ReadableAccount + Sync + ZeroLamport + 'a>(
	&self,
	accounts: impl StorableAccounts<'a, T>,
	) {
	assert!(!self.freeze_started());
	let mut m = Measure::start("stakes_cache.check_and_store");
	(0..accounts.len()).for_each(|i| {
	self.stakes_cache
	.check_and_store(accounts.pubkey(i), accounts.account(i))

Here's where we modify rent_epoch when loading for tx processing:

solana/runtime/src/accounts.rs

Lines 291 to 297 in 1c7662a

	let rent_due = rent_collector
	.collect_from_existing_account(
	key,
	&mut account,
	self.accounts_db.filler_account_suffix.as_ref(),
	set_exempt_rent_epoch_max,
	)

I'm sure the store goes through store_accounts() above.

[jeffwashington]

Since rent collection is calling store_accounts, it looks like the stakes cache may be updated. However, I recall us putting in logging and maybe asserts for verifying the stakes cache was consistent with accounts db. We found that while we were skipping slots due to rewards calculation taking so long, accounts that had passed their rent collection slot in the new epoch had a differing rent_epoch field in accounts db than the stakes cache. I assume the stakes cache is as of the first slot in the new epoch somehow? I'm not sure why rent collection was not updating the stakes cache when we were advancing rent_epoch prior to activation of #26479. Do we still have asserts on cache validity?

[behzadnouri]

accounts that had passed their rent collection slot in the new epoch had a differing rent_epoch field in accounts db than the stakes cache

Wasn't this due to some rent_epoch adjustments during load time?

I assume the stakes cache is as of the first slot in the new epoch somehow?

no, this is not true. The stakes-cache should be in sync with the accounts-db after each every write (so every slot).

I'm not sure why rent collection was not updating the stakes cache when we were advancing rent_epoch prior to activation of #26479.

again, my recollection is that accounts-db load was adjusting rent-epoch at load time.

Do we still have asserts on cache validity?

yes, we do; this is at startup when bank is deserialized:
https://github.com/solana-labs/solana/blob/1c7662a37/runtime/src/stakes.rs#L224-L272

and then there are some at epoch boundary rewards calculation:
https://github.com/solana-labs/solana/blob/1c7662a37/runtime/src/bank.rs#L1131-L1133

[jeffwashington]

Wasn't this due to some rent_epoch adjustments during load time?

That sounds correct. All that load time rent_epoch adjustment code has been removed, so that is good news.

[jeffwashington]

Interestingly, we are still not verifying rent_epoch matches:

solana/runtime/src/stakes.rs

Lines 238 to 244 in 1c7662a

	// Ignoring rent_epoch until the feature for
	// preserve_rent_epoch_for_rent_exempt_accounts is activated.
	let vote_account = vote_account.account();
	if vote_account.lamports() != account.lamports()
	|| vote_account.owner() != account.owner()
	|| vote_account.executable() != account.executable()
	|| vote_account.data() != account.data()

So stakes cache verification will not assert/error EVEN IF there is a mismatch here.
rent_epoch will then be corrected in the stakes cache the next time rent is collected for each account.

[behzadnouri]

Interestingly, we are still not verifying rent_epoch matches:

So stakes cache verification will not assert/error EVEN IF there is a mismatch here. rent_epoch will then be corrected in the stakes cache the next time rent is collected for each account.

That is just some legacy code that I am getting rid of here: #29861

We do actually check rent_epoch at epoch boundary rewards calculation.

[behzadnouri]

We do actually check rent_epoch at epoch boundary rewards calculation.

https://github.com/solana-labs/solana/blob/1c7662a37/runtime/src/bank.rs#L2575-L2581
https://github.com/solana-labs/solana/blob/1c7662a37/runtime/src/bank.rs#L2496-L2497

[jeffwashington]

ok. the reason the cache was out of date previously was because of this:
https://github.com/jeffwashington/solana/blob/d33d0213bb549634873ec8618ca29ce446cd3c11/runtime/src/bank.rs#L6437-L6445
That was required to support some validators skipping rewrites while others didn’t. We decided later not to go with that approach and this code was ripped out. With the code ripped out manually, the stakes cache is correctly kept up to date as of that change.
When rent collection runs, I believe the stakes cache is updated correctly. Therefore, since loading for tx (when we collect rent) and rent collection (from bank freeze) and creation are the places where we set the rent_epoch to max, the stakes cache should be kept up to date with the code in master now.

I'm testing this now.

[jeffwashington]

I have reproduced and confirmed why we had the original discrepancy and why that issue is not present anymore in any form we know of.
I applied that same test case to today's code and had the feature activation occur for this rent_epoch max feature. I confirmed that the stakes cache remains perfectly up to date and should always have the correct rent_epoch value before and after the feature activation.