-
Notifications
You must be signed in to change notification settings - Fork 97
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-0194: Deprecate rent exemption threshold #194
Open
deanmlittle
wants to merge
16
commits into
solana-foundation:main
Choose a base branch
from
deanmlittle:deprecate-rent-exemption-threshold
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
0e95331
Create XXXX-deprecate-rent-exemption-threshold.md
deanmlittle 7e7bd88
Update XXXX-deprecate-rent-exemption-threshold.md
deanmlittle f485623
Update XXXX-deprecate-rent-exemption-threshold.md
deanmlittle 32216f5
Update XXXX-deprecate-rent-exemption-threshold.md
deanmlittle 45110fe
Update XXXX-deprecate-rent-exemption-threshold.md
deanmlittle c21074f
Update XXXX-deprecate-rent-exemption-threshold.md
deanmlittle c6c60f4
Update XXXX-deprecate-rent-exemption-threshold.md
deanmlittle a1b1e03
Update XXXX-deprecate-rent-exemption-threshold.md
deanmlittle 2183690
Minor edits
febo 4cbf8cf
Wrap lines
febo 15f85fa
Typos
febo fc7adb0
Merge pull request #1 from febo/patch-1
deanmlittle b979b31
Update XXXX-deprecate-rent-exemption-threshold.md
deanmlittle 48235b3
Fix lint
febo 5555a94
Merge pull request #2 from febo/patch-2
deanmlittle e36946d
Update and rename XXXX-deprecate-rent-exemption-threshold.md to 0194-…
deanmlittle File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
--- | ||
simd: '0194' | ||
title: Deprecate Rent Exemption Threshold | ||
authors: | ||
- Dean Little (@deanmlittle) | ||
- Leonardo Donatacci (@L0STE) | ||
- febo (Anza) | ||
category: Standard/Meta | ||
type: Core | ||
status: Draft | ||
created: 2024-11-13 | ||
feature: (fill in with feature tracking issues once accepted) | ||
--- | ||
|
||
## Summary | ||
|
||
This proposal aims to eliminate the use of floating-point operations to | ||
determine whether an account is rent exempt or not in programs by deprecating | ||
the use of the `exempt_threshold` (`f64`) value. | ||
|
||
More specifically, rename `lamports_per_byte_year` to `lamports_per_byte`, | ||
change default value from `3480` to `6960`, change `exemption_threshold` to | ||
`1.0` and deprecate it from the protocol. This will enable us to remove `f64` | ||
math from all Rent-related SDKs and onchain programs moving forward. | ||
|
||
## Motivation | ||
|
||
Emulating floating point math is expensive inside of SVM due to the lack of | ||
native floating point number support. This makes calculating the rent exempt | ||
threshold cost `~248` more CUs than it would if we were to simply use a `u64`. | ||
This is due to the `exemption_threshold` (`f64`), which is currently set to | ||
`2.0` years. Since rent exemption is no longer time-based, we have the | ||
opportunity to make this commonly used calculation much more performant and | ||
simplify our tooling without affecting the existing API. It also simplifies any | ||
further changes we may want to make to Rent down the line. | ||
|
||
## New Terminology | ||
|
||
`lamports_per_byte` - the number of lamports required to pay for 1 byte of | ||
account storage. | ||
|
||
## Detailed Design | ||
|
||
Set the value of `DEFAULT_EXEMPTION_THRESHOLD` from `2.0` to `1.0`, and | ||
deprecate it from the protocol. | ||
|
||
```rs | ||
pub const DEFAULT_EXEMPTION_THRESHOLD: f64 = 1.0; | ||
``` | ||
|
||
Set `DEFAULT_LAMPORTS_PER_BYTE` from its current value of `3480` (the `u64` | ||
value of `1_000_000_000 / 100 * 365 / (1024 * 1024)`), to `6960`; double its | ||
current `u64` value, to counteract reducing the exemption threshold above. | ||
|
||
```rs | ||
pub const DEFAULT_LAMPORTS_PER_BYTE: u64 = 6960; | ||
``` | ||
|
||
Rename `lamports_per_byte_year` in the Rent account to `lamports_per_byte` to | ||
reflect that Rent is no longer time-based and officially deprecate | ||
`exemption_threshold`. | ||
|
||
```rs | ||
pub struct Rent { | ||
pub lamports_per_byte: u64, | ||
#[deprecated(since = "2.X.X", note = "Use only `lamports_per_byte` | ||
instead")] | ||
pub exemption_threshold: f64, | ||
pub burn_percent: u8, | ||
} | ||
``` | ||
|
||
Remove all `f64` math from Rent calculations in all SDKs and onchain programs | ||
moving forwards, replacing it with simple `u64` math, ie: | ||
|
||
```rs | ||
/// Minimum balance due for rent-exemption of a given account data size. | ||
pub fn minimum_balance(&self, data_len: usize) -> u64 { | ||
(ACCOUNT_STORAGE_OVERHEAD + data_len as u64) * self.lamports_per_byte | ||
} | ||
``` | ||
|
||
Validator implementations should stop using `exemption_threshold` and only use | ||
the `lamports_per_byte` value. Any existing program using the current | ||
implementation will be unaffected. | ||
|
||
## Alternatives Considered | ||
deanmlittle marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- Leave things as they are. | ||
- Calculating rent exemption on a program remains an | ||
expensive operation. | ||
|
||
- Allow users to make the assumption that `2.0` will remain stable and do `u64` | ||
math themselves. | ||
- Risk of the protocol changing on them. | ||
|
||
- Perform the "conversion" ouside the VM and change the type of | ||
`exemption_threshold` to a `u64` using a new `RentV2` struct. | ||
- Requires a new syscall and sysvar. | ||
|
||
## Impact | ||
|
||
This change will significantly improve the compute unit (CU) consumption of | ||
new onchain programs using updated SDKs programs that calculate the minimum | ||
lamports balance required for rent exemption. Currently, the calculation of the | ||
minimum balance for rent exemption (`Rent::minimum_balance`) consumes | ||
approximately `256` CUs; with the proposed change, this will be reduced to | ||
just `8` CUs. Existing programs will not be impacted. | ||
|
||
## Security Considerations | ||
|
||
None. | ||
|
||
## Drawbacks | ||
|
||
None. | ||
|
||
## Backwards Compatibility | ||
|
||
This feature is backwards compatible. It does not change the absolute value | ||
required for an account to be rent exempt and the current way of calculating | ||
the threshold will continue to evaluate to the same value. Any deployed program | ||
will be unaffected. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this calculation faster is great! I'm just wondering if renaming
lamports_per_byte_year
tolamports_per_byte
and changing the values might be confusing for people grepping forlamports_per_byte_year
.If we were going to keep the current concept of rent long-term I would agree, but as rent is going away anyway I wonder if this would add more short-term confusion?
If the goal of this SIMD is to remove the floating point operations from this calculation, I wonder if it would be simpler to mark
exemption_threshold
as deprecated and make the following change in the SDK to remove the floating-point calculation:Thoughts? That said, I am ok with the proposed change, just wanted to throw a simpler alternative out there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was one of the discussed alternatives above. If we simply decide "This number is now 2 forever, we promise not to change it", it has the least moving parts. It doesn't require a feature gate either. We can simply change all our libraries to ignore the f64 value stored in the Rent account/sysvar and substitute with our own u64. The main downsides are that we waste 1 extra CU multiplying by 2 every time unnecessarily, and instead of removing existing protocol crust, we introduce a new piece where we have this random number 2 thrown in there for no reason. I prefer changing the value to 1 to remove it from the equation, but both are fine changes imo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another idea. What if we were to just hardcode the value of rent in our libraries too? Then it should be possible to determine rent calculation statically, avoiding needing a syscall (another 100 CUs saved) or rent account (15 or so CUs saved if you use pinocchio) at all. If the idea is that we want a stop gap before eventually removing it entirely, I would think that's an even better solution than hardcoding the 2. If we have to support rent long-term, changing the value still makes the most sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer this option, but I am happy with either.
Do you mean making the minimum balance independent of the size of the account? I think that we should still take the size of the account into consideration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I mean making the lamports per bytes for rent exemption static. Right now, the reason we have a syscall and/or Rent account at all is to tell us:
lamports_per_byte_year
we are paying, which is multiplied byexemption_threshold
of2.0f64
yearsIf these values will not change between now and when Rent is removed altogether, we could simply hardcode both values into our libraries enabling you to statically derive the correct amount of rent to pay without invoking a syscall at all.