svm: Remove SVMRentCollector, use Rent directly #6782
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6782 +/- ##
=========================================
Coverage ? 82.8%
=========================================
Files ? 801
Lines ? 363391
Branches ? 0
=========================================
Hits ? 300971
Misses ? 62420
Partials ? 0 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
All of these free functions were originally on the SVMRentCollector trait, which enabled consumers of SVM to override them and implement custom rent behavior. We should make sure nobody depends on this capability before removing it.
I wonder if it's better to just phase out the concept of "transitioning" and ultimately the RentState::RentPaying variant as well, since SVM no longer needs rent-paying evaluation logic, as per #6622.
There was a problem hiding this comment.
GitHub doesn't seem to report anything: https://github.com/search?q=%22impl+SVMRentCollector%22&type=code
But I can do a PR first to deprecate the trait, backport it to v2.3, and then put this PR back in. What do you think?
As for the transition function, it makes sense to remove it since it should always return true.
For the other free functions, I can move them into solana-svm, since they're only used by solana-runtime and solana-svm. How does that sound?
There was a problem hiding this comment.
Actually, what do you think about just folding the whole crate into a rent_calculator module within SVM? I only see the functions and RentState used in runtime & SVM.
There was a problem hiding this comment.
You got it, done! There were a lot of functions removed since the creation of this PR, so I had to remove changed functions during the rebase
There was a problem hiding this comment.
GitHub doesn't seem to report anything: https://github.com/search?q=%22impl+SVMRentCollector%22&type=code
But I can do a PR first to deprecate the trait, backport it to v2.3, and then put this PR back in. What do you think?
As for the transition function, it makes sense to remove it since it should always return true.
For the other free functions, I can move them into solana-svm, since they're only used by solana-runtime and solana-svm. How does that sound?
There was a problem hiding this comment.
Changes look good to me, but let's consider just collapsing the entire crate into solana-svm, as per discussion.
#### Problem Now that rent collection is a thing of the past, we should remove the concepts of rent collection and rent epoch. Unfortunately, there's a lot of code around rent collection in the SVM. #### Summary of changes A few parts: * Stop using RentCollector in SVM, use Rent instead * Rename svm-rent-collector to svm-rent-calculator * Remove the SVMRentCollector trait * Remove the rent collector wrapper in runtime, which does the same thing, except submits metrics, which shouldn't be needed
* rent-collector: Inline struct and const, remove dep #### Problem As outlined in anza-xyz/solana-sdk#226, the RentCollector is no longer needed, and with #6782, it's needed even less. #### Summary of changes Inline the `RentCollector` struct into `solana-runtime` with the few functions needed. This changes the frozen-abi hash because the type now comes from a different crate. The only other usage was the `RENT_EXEMPT_RENT_EPOCH` constant, which is used in svm and accounts-db. Since there's no good common crate that both of these use, I inlined the const in both places, along with a test to make sure they don't diverge. Finally, remove solana-rent-collector usage everywhere. * Make `RentCollector` crate-private, expose rent() directly * Revert "Make `RentCollector` crate-private, expose rent() directly" This reverts commit 101c5bf. * Put accidentally removed comment back in post-rebase * Also remove it from snapshot_package.rs
Problem
Now that rent collection is a thing of the past, we should remove the concepts of rent collection and rent epoch. Unfortunately, there's a lot of code around rent collection in the SVM.
Summary of changes
A few parts: