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

Possible to create new rent-paying accounts #23342

Closed
jstarry opened this issue Feb 25, 2022 · 9 comments · Fixed by #23358
Closed

Possible to create new rent-paying accounts #23342

jstarry opened this issue Feb 25, 2022 · 9 comments · Fixed by #23358
Assignees

Comments

@jstarry
Copy link
Member

jstarry commented Feb 25, 2022

Problem

The new require_rent_exempt_accounts feature introduced by #22292 still allows new rent-paying accounts to be created when a transaction fee payer account is rent exempt but after tx fees becomes rent-paying. This can happen in two ways:

  1. The pre_account_state_info variable captures accounts post fee collection and if a previously rent-exempt fee-payer account became rent-paying after tx fees, it will appear to be a pre-existing rent-paying account and will be allowed to remain rent-paying.
  2. Failed transactions which are committed to a block are not checked for rent-exemption at all. So if a rent-exempt fee payer account becomes rent-paying from the tx fees and the tx also fails, it will be stored as rent-paying.

Proposed Solution

Add a check that previously rent-exempt fee payer accounts do not become rent-paying after tx fees are deducted.

@CriesofCarrots CriesofCarrots self-assigned this Feb 25, 2022
@jackcmay
Copy link
Contributor

Do we allow non-rent-exempt fee payer accounts? Looks like the fee payer either has to contain no data, or be a nonce in which case it is checked that its balance - fee is still exempt.

@CriesofCarrots
Copy link
Contributor

CriesofCarrots commented Feb 25, 2022

Easy repro of case 1:

$ solana-test-validator
$ solana transfer <ACCOUNT> .00089088 --allow-unfunded-recipient
$ solana transfer <RECIPIENT> 1 --fee-payer <ACCOUNT> --allow-unfunded-recipient

$ solana account <ACCOUNT>
Public Key: <PUBKEY>
Balance: 0.000878441 SOL
Owner: 11111111111111111111111111111111
Executable: false
Rent Epoch: 1

$ solana rent 0
Rent per byte-year: 0.00000348 SOL
Rent per epoch: 0.000002439 SOL
Rent-exempt minimum: 0.00089088 SOL

As Justin pointed out, the pre-rent-state considers the balance with fee substracted, so if the fee takes the payer account below rent-exempt minimum, it is grandfathered in to remain rent-paying when it shouldn't be.

@jackcmay
Copy link
Contributor

Does that account hold data?

@CriesofCarrots
Copy link
Contributor

Does that account hold data?

Nope

Do we allow non-rent-exempt fee payer accounts? Looks like the fee payer either has to contain no data

Yeah, if there is no data, a rent-paying fee payer is totally allowed.

@jackcmay
Copy link
Contributor

Ah, got it, keep forgetting that zero-data length accounts can still be rent payers (overhead).

@CriesofCarrots
Copy link
Contributor

CriesofCarrots commented Feb 25, 2022

@jstarry , the fix is mostly straightforward, but there's one case I can't figure out how to handle: when a fee-payer account has a small enough balance that the fee would make the account RentPaying, but the contents of the transaction would empty the account. Do you have any thoughts? Can we just fail those transactions (requiring users to pay the fee with another account, or transfer in enough to cover the fee before closing)?

The reason I'm stuck on this is because if we allow such a transaction to go through to processing in order to find out if it would be emptied by the transaction, there really isn't a way to catch transactions in case 2 above (without some awful fee rollback, which I'd rather not do).

@jstarry
Copy link
Member Author

jstarry commented Feb 26, 2022

@jstarry , the fix is mostly straightforward, but there's one case I can't figure out how to handle: when a fee-payer account has a small enough balance that the fee would make the account RentPaying, but the contents of the transaction would empty the account. Do you have any thoughts? Can we just fail those transactions (requiring users to pay the fee with another account, or transfer in enough to cover the fee before closing)?

I think it's best to reject those transactions and agree that trying to handle them would introduce too much implementation complexity. It wouldn't be great if we had to fully process a transaction to see if it's valid or not. I think this approach is clearer for users as well, that there is no case that a transaction can be committed if the fee payer account can't stay rent exempt after fees.

@CriesofCarrots
Copy link
Contributor

Excellent, that's what I did :)

@github-actions
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants