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

feat: disable partitioned rent collection #4572

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

jstarry
Copy link

@jstarry jstarry commented Jan 22, 2025

Problem

Implement SIMD-0175 (Amended in solana-foundation/solana-improvement-documents#230)

Summary of Changes

Adds new feature gate to conditionally not run partitioned rent collection

Feature Gate Issue: #4562

@jstarry jstarry added the feature-gate Pull Request adds or modifies a runtime feature gate label Jan 22, 2025
@jstarry jstarry force-pushed the feat/disable-partitioned-rent branch 2 times, most recently from d993cd1 to d7dd58f Compare January 22, 2025 03:57
Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Here in the SVM code, if partitioned rent collection is disabled, should we skip updating the rent_epoch?

/// Collect rent from an account if rent is still enabled and regardless of
/// whether rent is enabled, set the rent epoch to u64::MAX if the account is
/// rent exempt.
pub fn collect_rent_from_account(
feature_set: &FeatureSet,
rent_collector: &dyn SVMRentCollector,
address: &Pubkey,
account: &mut AccountSharedData,
) -> CollectedInfo {
if !feature_set.is_active(&feature_set::disable_rent_fees_collection::id()) {
rent_collector.collect_rent(address, account)
} else {
// When rent fee collection is disabled, we won't collect rent for any account. If there
// are any rent paying accounts, their `rent_epoch` won't change either. However, if the
// account itself is rent-exempted but its `rent_epoch` is not u64::MAX, we will set its
// `rent_epoch` to u64::MAX. In such case, the behavior stays the same as before.
if account.rent_epoch() != RENT_EXEMPT_RENT_EPOCH
&& rent_collector.get_rent_due(
account.lamports(),
account.data().len(),
account.rent_epoch(),
) == RentDue::Exempt
{
account.set_rent_epoch(RENT_EXEMPT_RENT_EPOCH);
}
CollectedInfo::default()
}
}

My understanding is when the feature is enabled that we should stop modifying any account's rent_epoch field.

@jstarry
Copy link
Author

jstarry commented Jan 23, 2025

My understanding is when the feature is enabled that we should stop modifying any account's rent_epoch field.

We could do that but I didn't mention this in the SIMD amendment at all. We still set the rent_epoch to a non default value for new accounts as well. How about we amend the SIMD again to be explicitly about not updating the rent_epoch anymore (including for new accounts)? By doing that, we can still remove partitioned rent collection completely. Seems like a better way to clean things up.

@brooksprumo
Copy link

We could do that but I didn't mention this in the SIMD amendment at all.

Yes, you're right. I went back and reread the SIMD and see only rent collection during bank freeze is called out.

We still set the rent_epoch to a non default value for new accounts as well. How about we amend the SIMD again to be explicitly about not updating the rent_epoch anymore (including for new accounts)? By doing that, we can still remove partitioned rent collection completely. Seems like a better way to clean things up.

Works for me! I found it unexpected that we still did rent collection for accounts loaded during transaction processing. I do like the idea of removing rent collection everywhere.

@HaoranYi
Copy link

HaoranYi commented Jan 23, 2025

Just to be clear. There are two kinds of rent collections. (a) partitioned
rent collection and (2) individual rent collection.

This SIMD only deals with partitioned rent collection, which is fine, because
most of the gain from disabling rent collection is in (a).

We can leave 'individual rent collection' as it is for future works.

@jstarry jstarry force-pushed the feat/disable-partitioned-rent branch from 20f4ced to 34e5195 Compare January 23, 2025 15:34
@brooksprumo
Copy link

We can leave 'individual rent collection' as it is for future works.

Yeah, we could do it separate, but then we'd need another SIMD and another feature gate.

@HaoranYi
Copy link

HaoranYi commented Jan 23, 2025

Yeah, we could do it separate, but then we'd need another SIMD and another feature gate.

Then it become a question of how complex it is to disable individual accounts rent collection? Is it complex enough to justify the cost to write a new SIMD and new feature gate?

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.

3 participants