-
Notifications
You must be signed in to change notification settings - Fork 172
sdk: Refactor Rentsysvar
#294
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
base: main
Are you sure you want to change the base?
Conversation
812d444 to
37d9cc7
Compare
37d9cc7 to
efaad10
Compare
joncinque
left a comment
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.
Just one question about the target_os = solana part, the rest looks great!
9bb767d to
4146158
Compare
4146158 to
1d823f9
Compare
joncinque
left a comment
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.
Looks great! Just the bit about saturating math, but we can address later
sdk/src/sysvars/rent.rs
Outdated
| F64_SIMD0194_EXEMPTION_THRESHOLD_AS_U64 => { | ||
| match self.exemption_threshold { | ||
| SIMD0194_EXEMPTION_THRESHOLD => { | ||
| (ACCOUNT_STORAGE_OVERHEAD + bytes) * self.lamports_per_byte |
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 know this code has been here for some time, but I'm just realizing that we now have an opportunity to overflow this calculation, which wasn't the case when it used floats. Should we eventually use some saturating math here to be safe?
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.
This is a tricky one. Adding saturated math increased CUs by ~70, which is about 50%. It will probably be cheaper to validate the data_len and lamports_per_byte values so that we can determine that it won't overflow.
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.
Wow, I didn't think it would be so bad! Adding checks makes sense to me, and if it's costly, we could only check data_len and add a warning specifying the range of valid values for lamports_per_byte
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.
Adding a test on both data_len and lamports_per_byte takes 6 extra CUs. The caveat is that currently minimum_exempt is not a fallible function, so either we would need to panic or change the return type.
We could probably have a minimum_exempt and an unsafe minimim_exempt_unchecked (without the validation).
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.
That makes sense to me -- performance is getting huge gains anyway from not using floats, so the extra checks are not such a big deal
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.
Added a fallible try_minimum_balance and unsafe minimum_balance_unchecked; also deprecated the minimum_balance, which now would panic if data_len exceeds the maximum allowed.
It is a bit tricky to have a good check for lamports_per_byte, since it depends on both exempt_threshold and maximum data_len. I was thinking that we can assume that the runtime guarantees the value of lamports_per_byte is valid in relation to the maximum data_len.
| /// Burn percentage | ||
| pub burn_percent: u8, | ||
| /// The concept of rent no longer exists, only rent-exemption. | ||
| exemption_threshold: [u8; 8], |
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.
Should we just store this as a u64 to make things simpler?
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.
We can everything as a [u8; 8] if we use an if statements instead of a match – no difference in CUs (eb6d8da).
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.
Ah interesting, good to know! Do we know if that's a problem with codegen? It seems like the two should be equivalent, no?
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 checked this and the compiler/llvm won't be able to infer all the information to optimize when used in a match statement, so this is the expected behaviour.
0cba916 to
2dcc702
Compare
2dcc702 to
2671ac7
Compare
Problem
The concept of rent no longer exists, only rent-exemption, and upstream BPF does not support
f64values but currently theRentsysvar implementation uses them.Solution
Refactor the sysvar implementation and remove all references to
f64.