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

SIMD-0084: Disable rent fees collection #84

Merged
merged 11 commits into from
May 21, 2024

Conversation

HaoranYi
Copy link
Contributor

@HaoranYi HaoranYi commented Nov 4, 2023

Add simd for disable rent fees collection.

@HaoranYi HaoranYi changed the title add simd SIMD-0084: Disable rent fees collection Nov 4, 2023
@HaoranYi HaoranYi self-assigned this Nov 4, 2023
@HaoranYi HaoranYi marked this pull request as ready for review November 16, 2023 18:38
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
@apfitzge
Copy link
Contributor

Without rent payments, can we eliminate the concept & term of "rent" entirely?

  • How confident are we with the current minimum balance calculation(s)? If we remove rent, it seems this would become more difficult to change in the future.
  • If the concept of rent goes away, should the Rent sysvar continue to be available?

@brooksprumo
Copy link
Contributor

Without rent payments, can we eliminate the concept & term of "rent" entirely?

  • How confident are we with the current minimum balance calculation(s)? If we remove rent, it seems this would become more difficult to change in the future.
  • If the concept of rent goes away, should the Rent sysvar continue to be available?

This only eliminates rent collection (and distribution), so the concept of rent still remains. We could rename it to "Minimum Balance", since that's what it effectively will be now.

(But renaming is probably not worth it; I imagine the Rent sysvar address is used in programs.)

@HaoranYi
Copy link
Contributor Author

HaoranYi commented Nov 16, 2023 via email

@apfitzge
Copy link
Contributor

This only eliminates rent collection (and distribution), so the concept of rent still remains. We could rename it to "Minimum Balance", since that's what it effectively will be now.

Right I was essentially asking this. If no one pays "rent", it's not really "rent" but a minimum required balance. I was definitely not suggesting we get rid of the minimum balance.

That still leaves my other question about if we're comfortable with the current minimum balances we have now.
Right now, if we decided the minimum balances were not sufficient to cover storage costs, we could increase these. Some accounts would become rent-paying again. They'd eventually get drained and we'd go back to not having rent-paying accounts.
But if we get rid of rent-paying, then it's more difficult for us to make this increase.
So we should be confident that the current values are correct.

Does that make sense?

@HaoranYi
Copy link
Contributor Author

With disks going cheaper and cheaper in future, I think, most likely, we will only lower the minimum required balance in future.

@brooksprumo
Copy link
Contributor

That still leaves my other question about if we're comfortable with the current minimum balances we have now. Right now, if we decided the minimum balances were not sufficient to cover storage costs, we could increase these. Some accounts would become rent-paying again. They'd eventually get drained and we'd go back to not having rent-paying accounts. But if we get rid of rent-paying, then it's more difficult for us to make this increase. So we should be confident that the current values are correct.

Yep, definitely agree. I wrote up some thoughts adjacent to this last year (#8), but haven't done more work since. Tl;dr is that I'd rather have a fee market for storage and have people pay at allocation time instead of carrying a minimum balance.

proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
HaoranYi and others added 3 commits January 3, 2024 10:08
Co-authored-by: lheeger-jump <126003637+lheeger-jump@users.noreply.github.com>
Co-authored-by: lheeger-jump <126003637+lheeger-jump@users.noreply.github.com>
Comment on lines 35 to 40
When the feature - "disable rent fees collections" is activated, rent will no
longer be collected from accounts nor will it be distributed to validators.

Note that this does **not** change the requirement that existing rent-paying
accounts, which need to be made rent-exempt first before any withdrawals can be
make from them.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth noting the specific protocol changes:

  • Slot boundaries no longer do eager rent collection
  • Slot boundaries no longer do lazy rent collection
  • The rent_epoch value of all accounts in the network is 2^64-1 (which would allow database implementations to repurpose this metadata field)

Copy link
Contributor

Choose a reason for hiding this comment

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

Should any of these points be addressed still?

Copy link
Contributor Author

@HaoranYi HaoranYi May 7, 2024

Choose a reason for hiding this comment

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

These are good observations and notes. But I don't think these points are relevant to the simd design changes. I prefer not to include them in the content of the SIMD.

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me.

The part about rent_epoch is interesting, as we'd like to reuse that field as well. I agree isn't not directly related to this SIMD, since this SIMD purposely does not touch/change accounts. While all the accounts likely have a rent epoch of MAX, we do not guarantee that (yet), and this SIMD is not meant to address that change.

ripatel-fd
ripatel-fd previously approved these changes Jan 8, 2024
Copy link
Contributor

@ripatel-fd ripatel-fd left a comment

Choose a reason for hiding this comment

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

@HaoranYi Looks like there is consensus on the technical changes installed by this SIMD. All the remaining review comments are regarding terminology. We can fix terminology later, I am therefore approving.

@sakridge sakridge added the core Standard SIMD with type Core label Feb 9, 2024
lheeger-jump
lheeger-jump previously approved these changes May 4, 2024
Copy link
Contributor

@lheeger-jump lheeger-jump left a comment

Choose a reason for hiding this comment

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

I think this change looks great. If we can get an approval from Anza, we can get it merged ASAP, as it does not have any new comments.

@HaoranYi
Copy link
Contributor Author

HaoranYi commented May 6, 2024

@brooksprumo @jeffwashington What do you think about this SIMD? Can we merge it?

Copy link
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.

nit: Please standardize on rent paying or rent-paying. Since we already use rent-exempt, I'd lean towards rent-paying.

Comment on lines 35 to 40
When the feature - "disable rent fees collections" is activated, rent will no
longer be collected from accounts nor will it be distributed to validators.

Note that this does **not** change the requirement that existing rent-paying
accounts, which need to be made rent-exempt first before any withdrawals can be
make from them.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should any of these points be addressed still?

proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
Co-authored-by: Brooks <brooks@prumo.org>
@brooksprumo brooksprumo self-requested a review May 7, 2024 15:57
Copy link
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.

Some grammar nits, but overall looks good.

proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
proposals/0084-disable-rent-fees-collection.md Outdated Show resolved Hide resolved
HaoranYi and others added 2 commits May 7, 2024 11:20
Co-authored-by: Brooks <brooks@prumo.org>
Co-authored-by: Brooks <brooks@prumo.org>
@HaoranYi
Copy link
Contributor Author

HaoranYi commented May 8, 2024

@jacobcreech We have received approval for this SIMD from FD and Anza. Do you mind merging it?

@0xSol
Copy link

0xSol commented May 8, 2024

Seem this SIMD reaches consensus. All please take a final check,. Will merge if no objections commented by May 14. Thanks for great collaboration!

@0xSol 0xSol merged commit 718f914 into solana-foundation:main May 21, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Standard SIMD with type Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants