accounts: Don't collect rent on newly created accounts

[joncinque]

Problem

This is a doozy for a one-line change.

With #26479 activated, the rent epoch of accounts is no longer updated. If you start a new cluster (or test validator) with this enabled, there may be some accounts with rent_epoch = 0.

When one of those accounts is closed, the runtime thinks it's a new account since its rent epoch is 0, tries to collect rent on it, and cleans up the account in the process.

This is normally OK, but if the account in question is a stake account, the stakes cache will go out of sync and eventually crash the bank during epoch rollover on this assertion:

solana/runtime/src/bank.rs

Line 2704 in 422cff6

assert_eq!(

Summary of Changes

Very simply, don't collect rent on newly created accounts if the feature flag from #26479 is enabled. Since newly created accounts cannot be rent-paying, this rent collection never happened anyway. The rent collector code can be cleaned up once the feature is removed.

Fixes #

[CriesofCarrots]

Looks right to me. Might not hurt to have @jeffwashington eyeball this as well

[jeffwashington]

solana/runtime/src/accounts.rs

Lines 1303 to 1307 in 9ef684f

	loaded_transaction.rent_debits.insert(
	address,
	rent,
	account.lamports(),
	);

I don't understand the ramifications. Is it this line that is the problem? rent should be 0.

[joncinque]

@jeffwashington The issue is actually here

solana/runtime/src/rent_collector.rs

Line 142 in 9ef684f

let account = std::mem::take(account);

-- we collect rent on any newly created accounts, and delete any of them that don't have enough lamports for rent. Since accounts created at epoch 0 never have their rent epoch advanced, when closing these accounts, they'll get re-initialized too early.

[jeffwashington]

when closing these accounts, they'll get re-initialized too early.

Closing means setting lamports to 0?
How would we be re-initializing them (setting AccountSharedData to default?) 'too' early?

[joncinque]

Closing means setting lamports to 0?

Correct

How would we be re-initializing them (setting AccountSharedData to default?) 'too' early?

The account is re-initialized before updating the stakes cache. If it's a stake account, we lose any delegation information, causing the stakes cache to go out of sync.

[behzadnouri]

When do we still need to take this branch?

We no longer create rent-paying accounts; so in what scenario do we still need to take this branch and collect rent on a new account?

[joncinque]

This branch is still taken when an account with rent epoch 0 is referenced in a transaction. On every transaction, there's a rent collection that takes place for new accounts, and we check if an account is new based on rent_epoch == 0.

However, if an account has been closed (emptied of lamports), we still try to do this rent collection afterwards, and since that account now owes rent, the runtime reinitializes the account to Account:;default(). This is mostly OK, but for stake accounts, this means that the cache falls out of sync, since the cache update happens after the reinitialization.

[behzadnouri]

On every transaction, there's a rent collection that takes place for new accounts,

ok, my question is why do we still need this rent collection for new accounts if we no longer allow one to create new rent-paying accounts?

[joncinque]

No reason that I can see, it was simply just never removed. Do you prefer just removing this whole branch rather than tying it to your feature?

[behzadnouri]

yeah, if that is the case that the branch is never taken anyways, then lets remove it entirely.
cc @CriesofCarrots to confirm that the branch is redundant.

[CriesofCarrots]

Okay, I have confirmed that when we reach this line, all transactions that leave any account in a newly rent-paying state will:
a. have execution_status.is_err() (TransactionError::InvalidRentPayingAccount or TransactionError::InsufficientFundsForRent)
b. not feature a rent-paying nonce account, because SystemProgram enforces this
c. not feature a rent-paying fee_payer account, because such a transaction will have result TransactionExecutionResult::NotExecuted and not reach this line

Thus any tx with a rent-paying account will not follow this branch.

Transactions that create rent-exempt accounts still take the branch, and call collect_from_created_account which includes

solana/runtime/src/rent_collector.rs

Line 203 in e74ad90

account.set_rent_epoch(self.epoch);

So I think the feature gating in this PR is still necessary in order to ensure consistency of the rent_epoch field on account creation when nodes update to the change set.

Handling of RentPaying cases inside collect_from_created_account (ie calling collect_from_existing_account) should not be necessary at all, though.

[joncinque]

Thanks so much for looking into all this! And great catch on setting the rent epoch. @behzadnouri are you ok with using your existing feature flag? Since we're no longer advancing rent epoch with your change, it makes sense to also not set rent epoch at creation with the same feature, but I can be convinced otherwise.

[jeffwashington]

Since we're no longer advancing rent epoch with your change, it makes sense to also not set rent epoch at creation with the same feature, but I can be convinced otherwise.

This sounds correct to me.

[CriesofCarrots]

Then we can remove the block entirely on feature cleanup :)

[behzadnouri]

@joncinque yeah, sounds good.

[joncinque]

@jeffwashington do this get your explicit approval too? I want to be sure before merging.

[jeffwashington]

Yes Sent from my phone.

…

On Aug 2, 2022, at 9:22 AM, Jon Cinque ***@***.***> wrote:  @jeffwashington do this get your explicit approval too? I want to be sure before merging. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.

[joncinque]

Thank you for all the gracious reviews!