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

restricts rent-paying accounts lifetime extension #26606

Merged

Conversation

behzadnouri
Copy link
Contributor

@behzadnouri behzadnouri commented Jul 13, 2022

Problem

#22292 prevents rent paying accounts creation going forward. However a rent
paying account can linger on for ever if it is continually topped up but
stays below the rent-exempt minimum.
This can prevent eliminating accounts-rewrites and the problematic
rent_epoch field in accounts.

Link to discord discussion:
https://discord.com/channels/428295358100013066/943609352068145162/995202300001927219

Summary of Changes

This commit restricts rent-paying accounts lifetime extension by preventing
increasing lamports on the account if the account stays below the
rent-exempt minimum.

Feature Gate Issue: #26607

@behzadnouri behzadnouri added the feature-gate Pull Request adds or modifies a runtime feature gate label Jul 13, 2022
@behzadnouri behzadnouri force-pushed the restrict-rent-paying-ttl-extension branch from 33da4c9 to 5fde79a Compare July 13, 2022 15:19
@behzadnouri behzadnouri force-pushed the restrict-rent-paying-ttl-extension branch from 5fde79a to d8d37bc Compare July 13, 2022 19:31
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Thanks, @behzadnouri . Logic looks fine, some nits.

@@ -456,6 +456,10 @@ pub mod enable_bpf_loader_extend_program_data_ix {
solana_sdk::declare_id!("8Zs9W7D9MpSEtUWSQdGniZk2cNmV22y6FLJwCx53asme");
}

pub mod restrict_rent_paying_accounts_ttl_extension {
Copy link
Contributor

Choose a reason for hiding this comment

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

This name is kind of hard to parse at a glance. Can we call it something like: prevent_crediting_rent_paying_accounts? I would prefer we not add a phrase like "TTL extension" that then ought to be defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Comment on lines 57 to 64
post_data_size == pre_data_size && {
!restrict_rent_paying_accounts_ttl_extension
|| post_lamports <= pre_lamports
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did cargo fmt wrap this extended boolean in brackets like this instead of parens? That's weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rust accepts both; I think in this case { gives more readable format:

post_data_size == pre_data_size && {
    !prevent_crediting_rent_paying_accounts || post_lamports <= pre_lamports
}

vs

post_data_size == pre_data_size
    && (!prevent_crediting_rent_paying_accounts
        || post_lamports <= pre_lamports)

runtime/src/account_rent_state.rs Outdated Show resolved Hide resolved
},
restrict_rent_paying_accounts_ttl_extension
));
// No resize, same lamports is always allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the word "transition" is implied here, but if you could find a way to rewrite these comments so they don't say "lamports is" so many times, I would appreciate it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

@behzadnouri behzadnouri force-pushed the restrict-rent-paying-ttl-extension branch from d8d37bc to 7a1e96e Compare July 14, 2022 11:41
@behzadnouri behzadnouri changed the title restricts rent-paying accounts TTL extension restricts rent-paying accounts lifeline extension Jul 14, 2022
jstarry
jstarry previously approved these changes Jul 14, 2022
Copy link
Member

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

lgtm, just some nits on naming and code formatting

Comment on lines 56 to 64
// Cannot be RentPaying if resized
post_data_size == pre_data_size && {
!prevent_crediting_rent_paying_accounts || post_lamports <= pre_lamports
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Cannot be RentPaying if resized
post_data_size == pre_data_size && {
!prevent_crediting_rent_paying_accounts || post_lamports <= pre_lamports
}
// Cannot remain RentPaying if resized
if post_data_size != pre_data_size {
false
} else if prevent_crediting_rent_paying_accounts {
// Cannot remain RentPaying if credited
post_lamports <= pre_lamports
} else {
true
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer this more-explicit suggestion too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -564,6 +568,7 @@ lazy_static! {
(cap_accounts_data_size_per_block::id(), "cap the accounts data size per block #25517"),
(preserve_rent_epoch_for_rent_exempt_accounts::id(), "preserve rent epoch for rent exempt accounts #26479"),
(enable_bpf_loader_extend_program_data_ix::id(), "enable bpf upgradeable loader ExtendProgramData instruction #25234"),
(prevent_crediting_rent_paying_accounts::id(), "prevent crediting rent paying accounts #26606"),
Copy link
Member

Choose a reason for hiding this comment

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

Technically rent paying accounts can be credited if they become rent exempt. Can you rename the feature module name and the description here to match your PR title? I think restrict_rent_paying_accounts_lifetime_extension would be pretty clear

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are doing back and forth here; initially it was: restrict_rent_paying_accounts_ttl_extension.
@CriesofCarrots asked to change it: #26606 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

Yup I saw that comment thread as well. Like I said, I think that @CriesofCarrots's suggestion is a bit misleading so I hope that changing from "ttl" to "lifetime" is a good compromise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have strong preference either way since this is going to be deleted soon anyways.
I will wait for @CriesofCarrots comment. If there is no objection, I can change it to what you suggest.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, I think it’s fine as-is, but to address Justin’s concern about lost nuance, it could be prevent_crediting_accounts_that_end_rent_paying or similar, I suppose.
I am opposed to the “lifetime” suggestion, as rust already has a definition of the word; I don’t think that “accounts lifetime extension” is that clear unless you already understand the problem, and I am disinclined to add new verbiage that ought to be explained, especially for functionality that is being made obsolete.

Copy link
Member

Choose a reason for hiding this comment

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

prevent_crediting_accounts_that_end_rent_paying works for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@behzadnouri behzadnouri changed the title restricts rent-paying accounts lifeline extension restricts rent-paying accounts lifetime extension Jul 14, 2022
@behzadnouri behzadnouri force-pushed the restrict-rent-paying-ttl-extension branch from 7a1e96e to 3299e60 Compare July 14, 2022 14:20
@mergify mergify bot dismissed jstarry’s stale review July 14, 2022 14:21

Pull request has been modified.

CriesofCarrots
CriesofCarrots previously approved these changes Jul 14, 2022
Copy link
Contributor

@CriesofCarrots CriesofCarrots left a comment

Choose a reason for hiding this comment

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

Comment nits. Otherwise, this lgtm

Comment on lines 270 to 278
// Transition is always allowed if there is no account data resize and
// account's lamports is reduced because that will shorten rent paying
// account lifeline.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Transition is always allowed if there is no account data resize and
// account's lamports is reduced because that will shorten rent paying
// account lifeline.
// Transition is always allowed if there is no account data resize and
// account's lamports is reduced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 280 to 287
// Once the feature is activated, transition is not allowed if the
// account is credited with more lamports.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Once the feature is activated, transition is not allowed if the
// account is credited with more lamports.
// Once the feature is activated, transition is not allowed if the
// account is credited with more lamports and remains rent-paying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@behzadnouri behzadnouri force-pushed the restrict-rent-paying-ttl-extension branch from 3299e60 to 5ab36a9 Compare July 14, 2022 17:56
@mergify mergify bot dismissed CriesofCarrots’s stale review July 14, 2022 17:56

Pull request has been modified.

@behzadnouri behzadnouri force-pushed the restrict-rent-paying-ttl-extension branch from 5ab36a9 to 6c826ce Compare July 14, 2022 17:58
behzadnouri added a commit to behzadnouri/solana-program-library that referenced this pull request Jul 14, 2022
solana-labs/solana#26606
prevents crediting a rent paying account if the account stays rent
paying. As a result, test_lamport_transfer fails.

The commit updates destination account lamports so that after transfer
it is no longer rent paying.
behzadnouri added a commit to solana-labs/solana-program-library that referenced this pull request Jul 14, 2022
solana-labs/solana#26606
prevents crediting a rent paying account if the account stays rent
paying. As a result, test_lamport_transfer fails.

The commit updates destination account lamports so that after transfer
it is no longer rent paying.
t-nelson
t-nelson previously approved these changes Jul 15, 2022
solana-labs#22292
prevents rent paying accounts creation going forward. However a rent
paying account can linger on for ever if it is continually topped up but
stays below the rent-exempt minimum.
This can prevent eliminating accounts-rewrites and the problematic
rent_epoch field in accounts.

Link to discord discussion:
https://discord.com/channels/428295358100013066/943609352068145162/995202300001927219

This commit restricts rent-paying accounts lifeline extension by
preventing increasing lamports on the account if the account stays below
the rent-exempt minimum.
@behzadnouri behzadnouri force-pushed the restrict-rent-paying-ttl-extension branch from 6c826ce to ef05348 Compare July 15, 2022 11:30
@mergify mergify bot dismissed t-nelson’s stale review July 15, 2022 11:30

Pull request has been modified.

@behzadnouri behzadnouri merged commit bf225ba into solana-labs:master Jul 15, 2022
@behzadnouri behzadnouri deleted the restrict-rent-paying-ttl-extension branch July 15, 2022 13:23
mergify bot pushed a commit that referenced this pull request Jul 15, 2022
#22292
prevents rent paying accounts creation going forward. However a rent
paying account can linger on for ever if it is continually topped up but
stays below the rent-exempt minimum.
This can prevent eliminating accounts-rewrites and the problematic
rent_epoch field in accounts.

Link to discord discussion:
https://discord.com/channels/428295358100013066/943609352068145162/995202300001927219

This commit restricts rent-paying accounts lifetime extension by
preventing increasing lamports on the account if the account stays below
the rent-exempt minimum.

(cherry picked from commit bf225ba)

# Conflicts:
#	runtime/src/account_rent_state.rs
#	runtime/src/accounts.rs
#	runtime/src/bank/transaction_account_state_info.rs
#	sdk/src/feature_set.rs
mergify bot added a commit that referenced this pull request Jul 15, 2022
…26635)

* restricts rent-paying accounts lifetime extension (#26606)

#22292
prevents rent paying accounts creation going forward. However a rent
paying account can linger on for ever if it is continually topped up but
stays below the rent-exempt minimum.
This can prevent eliminating accounts-rewrites and the problematic
rent_epoch field in accounts.

Link to discord discussion:
https://discord.com/channels/428295358100013066/943609352068145162/995202300001927219

This commit restricts rent-paying accounts lifetime extension by
preventing increasing lamports on the account if the account stays below
the rent-exempt minimum.

(cherry picked from commit bf225ba)

# Conflicts:
#	runtime/src/account_rent_state.rs
#	runtime/src/accounts.rs
#	runtime/src/bank/transaction_account_state_info.rs
#	sdk/src/feature_set.rs

* removes mergify merge conflicts

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
Lcchy pushed a commit to Bonfida/solana that referenced this pull request Jul 22, 2022
solana-labs#22292
prevents rent paying accounts creation going forward. However a rent
paying account can linger on for ever if it is continually topped up but
stays below the rent-exempt minimum.
This can prevent eliminating accounts-rewrites and the problematic
rent_epoch field in accounts.

Link to discord discussion:
https://discord.com/channels/428295358100013066/943609352068145162/995202300001927219

This commit restricts rent-paying accounts lifetime extension by
preventing increasing lamports on the account if the account stays below
the rent-exempt minimum.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants