Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Remove unnecessary usage of RentCollector#35121

Merged
pgarg66 merged 1 commit intosolana-labs:masterfrom
pgarg66:rent-collector-cleanup
Feb 7, 2024
Merged

Remove unnecessary usage of RentCollector#35121
pgarg66 merged 1 commit intosolana-labs:masterfrom
pgarg66:rent-collector-cleanup

Conversation

@pgarg66
Copy link
Copy Markdown
Contributor

@pgarg66 pgarg66 commented Feb 7, 2024

Problem

Accounts methods are being passed instance of RentCollector, but it's never used. Maybe it can be removed.

Summary of Changes

Update methods to not take RentCollector as an argument.

Fixes #

@pgarg66 pgarg66 marked this pull request as ready for review February 7, 2024 01:41
@pgarg66 pgarg66 requested a review from brooksprumo February 7, 2024 01:42
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (46b9586) 81.6% compared to head (6cc96aa) 81.6%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #35121   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         831      831           
  Lines      225032   225029    -3     
=======================================
+ Hits       183708   183741   +33     
+ Misses      41324    41288   -36     

Copy link
Copy Markdown
Contributor

@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.

lgtm, thanks for the cleanup!

I had to do some digging to understand the context behind where this code came from, and what it was originally intended to do.

In the olden days, we used to allow creating rent-paying accounts. When a new account was created, we'd collect rent from it. This required the rent_collector when storing accounts.

In #22292, it prevented creating rent-paying accounts. As an optimization, #26479 made it so that we don't change the rent epoch for rent-exempt accounts when they are checked for rent collection.

Once the feature gate for #26479 was activated, an issue was discovered that impacted tests and new clusters due to the interaction with epoch being 0. In #26851, it fixed the issue by no longer doing rent collection on newly created accounts (there's a lot of context in this PR, which was interesting).

With #26479 activated and #26851 in place to fix the issue, the feature gate code could be removed. That was done in #28507. This PR removed the code for rent collection when storing accounts. The rent collector was no longer used, yet the parameter to the function was kept (and ignored, I assume to minimize changes in the PR).

And here we are today, finally removing the rent collector parameter!

@pgarg66
Copy link
Copy Markdown
Contributor Author

pgarg66 commented Feb 7, 2024

lgtm, thanks for the cleanup!

I had to do some digging to understand the context behind where this code came from, and what it was originally intended to do.

In the olden days, we used to allow creating rent-paying accounts. When a new account was created, we'd collect rent from it. This required the rent_collector when storing accounts.

In #22292, it prevented creating rent-paying accounts. As an optimization, #26479 made it so that we don't change the rent epoch for rent-exempt accounts when they are checked for rent collection.

Once the feature gate for #26479 was activated, an issue was discovered that impacted tests and new clusters due to the interaction with epoch being 0. In #26851, it fixed the issue by no longer doing rent collection on newly created accounts (there's a lot of context in this PR, which was interesting).

With #26479 activated and #26851 in place to fix the issue, the feature gate code could be removed. That was done in #28507. This PR removed the code for rent collection when storing accounts. The rent collector was no longer used, yet the parameter to the function was kept (and ignored, I assume to minimize changes in the PR).

And here we are today, finally removing the rent collector parameter!

Wow, lots of history with it. Thanks for digging through it!

@pgarg66 pgarg66 merged commit 56391f6 into solana-labs:master Feb 7, 2024
@pgarg66 pgarg66 deleted the rent-collector-cleanup branch February 7, 2024 15:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants