Rent timing is not consistant for writable/read-only access

[ryoqun]

Problem

Maybe to avoid write lock to accounts, rent collection is currently only done at post transaction. This causes some confusion and odd behavior, specifically #7413 (comment) and #7413 (comment).

Proposed Solution

Always, apply rent collection on load from AccountsDB to Bank. There is no exception. This solves varying view of program execution and cli/rpc, as well.
So, rent is collected at both at pre-transaction and post-transaction.

Come to think of it, I think there is no problem for mutating read-only accounts by maintaining the list of the actually rent collected read-only accounts and writable accounts touched by failed transaction in a bank and flush those account updates immeditely before at Bank::freeze() like with the eager rent collection account updates and by consistently applying rent collection on every access accounts before passing them into programs because rent collection is deterministic and idempotent.

Also, after post-transaction rent collection, if the balance goes below 0, abort the transaction itself with new TransactionError like InsufficientFundsForRent (renamed from NotWritableByRent) or similar, instead of silently writing Account::default() into the AccountDB.

related #7413, #10088.

found via #10348

[ryoqun]

FYIs: @jackcmay for NotWritableByRent idea, @sakridge for performance concerns, if any, @jstarry for smart-contract developer perspective.

[jackcmay]

NotWritableByRent does that imply that the account persists even if the balance is zoro and can be stil read?

[ryoqun]

@jackcmay No, it does not persist and cannot be read. Ideally, we want to save NotWritableByRent in the StatusCache and flag the touched account as being perged. Then, bank does the actual purging at the end of the slot along with eager rent collection. That way we can provide more intuitive user experience, outlined like this:

case (*)	old(current) semantics	new semantics	resulting balance	purge at end of slot
transfer 1 lamport to not-existing address	Ok	InsufficientFundsForRent	N/A	no
transfer 1 lamport to existing address with 1 lamport	Ok	InsufficientFundsForRent	N/A	yes
transfer 1 SOL to existing address with 1 SOL	Ok (metadata & data IS preserved)	Ok (metadata & data IS preserved)	1_SOL + 1_SOL - 1_epoch_rent	no
transfer 1 SOL to existing address with 1 lamport	Ok (metadata & data IS preserved?)	Ok (metadata & data ISNOT preserved)	1_SOL - 1_epoch_rent	no
read an account with 1 lamport	Ok	AccountNotFound	N/A	yes
modify an account with 1 lamport	AccountNotFound? or Ok?(I'm not sure..)	AccountNotFound	N/A	yes
transfer 1 lamport from not-existing address	AccountNotFound	AccountNotFound	N/A	no
transfer 1 lamport from existing address with 1 lamport	AccountNotFound? or Ok? (I'm not sure..)	AccountNotFound	N/A	yes
transfer 1 lamport from/to the same existing address with 1 lamport #10339/#10337	Ok?	AccountNotFound	N/A	yes

*: Assume all existing accounts are NOT yet rent-collected for this epoch.
*: Assume 1_rent_epoch < 1 SOL + 1 lamport.

Also, NotWritableByRent should be predictable by definition and should issue an warning at cli/web3.js?

Also, I think we can realize this consistent behavior if we enforce to apply rent collection for every loaded accounts.

[jackcmay]

@ryoqun

The 1 SOL to 1 lamport case above is only valid if 1_rent_epoch < 1 SOL + 1 lamport)?

Why not allow a read of an account with 1 lamport? Is that because it's considered already delinquent?

[ryoqun]

The 1 SOL to 1 lamport case above is only valid if 1_rent_epoch < 1 SOL + 1 lamport)?

Yeah, that's true. That's implied.

Why not allow a read of an account with 1 lamport? Is that because it's considered already delinquent?

Yeah, that's correct as well. Because it's delinquent, it should behave so consistently for easier reasoning for development and hiding our implementation details. Otherwise, our developers can't be sure when/how exactly their expiring accounts actually go disappear.

Eventually, the account will be corrected by the eager-rent collection cycle for the current epoch. But, it could go away anytime when someone sends a no-op transaction on it.

Morever, while the account can be viewed safely by cli/explorer/transactions, the account cannot be modified and that action itself causes irrecoverable deletion of the accout itself. However, currently there is a workaround to prevent it from being purged: send some rent-enough lamports to the account. Then, the account can be recovered to be readable/writable.

I think we could simplify these inconsistencies across various actions on the account for more consistent behavior: Once an account goes delinquent, it's gone in every way. Also note that, if we leave the current behavior as-is, we must document this in detail. Our docs (even after updated by #10348) assumes we provide the ideal consistent behavior.

@jstarry Any thoughts from app development side? Btw, I've prepared a fancy table for rent-related edge cases.

[jstarry]

Once an account goes delinquent, it's gone in every way

This sounds great! And accounts then go delinquent immediately when the new epoch starts?

I've prepared a fancy table for rent-related edge cases.

Very helpful!

Also, NotWritableByRent should be predictable by definition and should issue an warning at cli/web3.js?

Yeah, it would also be caught by the transaction simulation check!

Questions:

1. Should we pro-rate rent? Seems unfortunate to have full rent charged twice in two slots if you create an account in the last slot of an epoch.

2. "modify an account with 1 lamport" - For this case, why would the error be NotWritableRent? I would expect it to be AccountNotFound.

Maybe my expectation comes from thinking that NotWritableByRent means that you tried to transfer tokens to an account which ended up rent delinquent. Also, may I suggest InsufficientFundsForRent as an alternative name for this type of error?

App Development Notes

It seems that if any instruction in a transaction creates an account which cannot cover rent, the whole transaction will fail. This will require some documentation for program developers but shouldn't be a problem. I think we could add a convenience method to the Rent sysvar for checking if an account can cover rent cost for some number of epochs.

Other than that, nothing to add. This change would be a big improvement to the app development experience IMO.

[ryoqun]

@jstarry

Questions:

1. Should we pro-rate rent? Seems unfortunate to have full rent charged twice in two slots if you create an account in the last slot of an epoch.

Yeah, that near-end-of-epoch case is unfortunate..

Pro-rate might makes things complicated... Instead, we could use slot index in an epoch as the different timings for each accounts? In that case, I think this change is significant yet relatively easy if we clean-up now rather deprecated account.rent_epoch with this in mind?

2. "modify an account with 1 lamport" - For this case, why would the error be `NotWritableRent`? I would expect it to be `AccountNotFound`.

Yeah, your reasoning makes sense for me. Just updated. Maybe just my typo..

Also, may I suggest InsufficientFundsForRent as an alternative name for this type of error?

I like this name! Updated!

[ryoqun]

App Development Notes

It seems that if any instruction in a transaction creates an account which cannot cover rent, the whole transaction will fail. This will require some documentation for program developers but shouldn't be a problem. I think we could add a convenience method to the Rent sysvar for checking if an account can cover rent cost for some number of epochs.

Good catch! Hmm, I think we need more than this. Othwerwise, every transaction must be guarded against rent checks when they're dealing with arbitrary account pubkeys? This might turn out to be a footgun too easily... Here basically we're considering the design choice of {dict/map/hash}.get(not_existing_key) => {error? or empty vaue?} and not-guaranteed success of {dict/map/hash}.set(any_key, any_value) (a very primitive operation).

Other than that, nothing to add. This change would be a big improvement to the app development experience IMO.

Thanks!!

[jstarry]

Pro-rate might makes things complicated

True, are you referring to the determinism of rent fees here or something else?

Instead, we could use slot index in an epoch as the different timings for each accounts?

Yeah this sounds better. I'm not sure if the best way is to use a slot index yet. Couldn't epoch slot length change in the future?

[jstarry]

I'm not so sure about this case now:

transfer 1 SOL to existing address with 1 lamport

I don't think it should preserve data because data preservation implies that the account was readable despite being rent delinquent. Thoughts?

[ryoqun]

Pro-rate might makes things complicated

True, are you referring to the determinism of rent fees here or something else?

Well, the determinism can be assured. I'm mainly concerned with the complication in implementation-wise.

@jstarry In my opinion, extra rent fee for an epoch should be pretty cheap. I guess pro-rate isn't desired so much? Or is there some clever use case with micro-payment and short-lived accounts?

Instead, we could use slot index in an epoch as the different timings for each accounts?

Yeah this sounds better. I'm not sure if the best way is to use a slot index yet. Couldn't epoch slot length change in the future?

Hmm, I'm not sure but epoch slot length can change from the current 2 days to longer or shorter? Then, this idea might be difficult... This also complicates pro-rate rent fee. (if implemented)

I'm not so sure about this case now:

transfer 1 SOL to existing address with 1 lamport

I don't think it should preserve data because data preservation implies that the account was readable despite being rent delinquent. Thoughts?

Oh, good call! I've deliberately made the case to preserve because this allows last-minute restoration of account data this way. Otherwise, it'll be impossible rent-delinquent account to be restored after the new epoch begins. Come to think of it, users should act early. Well, maybe I was caring too much in the edge cases. I've aligned the behavior as you suggested like you suggested for the other case! Prefer overall logical consistency. :)

[jstarry]

@jstarry In my opinion, extra rent fee for an epoch should be pretty cheap. I guess pro-rate isn't desired so much? Or is there some clever use case with micro-payment and short-lived accounts?

Yeah, seems complicated and I don't think this was the right place to bring up the conversation. Sorry for detracting the conversation!

Come to think of it, users should act early.

Sounds good!

So now the only way that InsufficientFundsForRent can happen during a transaction instruction is when adding funds to a new account. I think that's reasonable and not much of a footgun, thoughts?

[ryoqun]

Yeah, seems complicated and I don't think this was the right place to bring up the conversation. Sorry for detracting the conversation!

No problem! Thanks for helping discussions to go in deep! :)

So now the only way that InsufficientFundsForRent can happen during a transaction instruction is when adding funds to a new account. I think that's reasonable and not much of a footgun, thoughts?

Yeah, I agree. :)

@jackcmay FYI, I've changed some cases. How does it look now for you?

If there is no remaining topics to discuss, I'll leave this issue open as-is to be implemented in mid-term (I hope..). Thanks for inputs!

[ryoqun]

todo: #10468 (comment)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[stale]

This stale issue has been automatically closed. Thank you for your contributions.

[github-actions]

This issue has been automatically locked since there has not been any activity in past 7 days after it was closed. Please open a new issue for related bugs.