SVM: add new solana-svm-rent-collector crate
#2688
Merged
Conversation
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
buffalojoec
commented
Aug 21, 2024
Comment on lines
+56
to
+77
| /// Check rent state transition for an account directly. | ||
| /// | ||
| /// This method has a default implementation that checks whether the | ||
| /// transition is allowed and returns an error if it is not. It also | ||
| /// verifies that the account is not the incinerator. | ||
| fn check_rent_state_with_account( | ||
| &self, | ||
| pre_rent_state: &RentState, | ||
| post_rent_state: &RentState, | ||
| address: &Pubkey, | ||
| _account_state: &AccountSharedData, | ||
| account_index: IndexOfAccount, | ||
| ) -> Result<()> { | ||
| if !solana_sdk::incinerator::check_id(address) | ||
| && !self.transition_allowed(pre_rent_state, post_rent_state) | ||
| { | ||
| let account_index = account_index as u8; | ||
| Err(TransactionError::InsufficientFundsForRent { account_index }) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Here I've used the same implementation as the original, but I've omitted the debug! (to avoid a dependency on log) and the metrics submission. The former omission is why _account_state is unused here.
There was a problem hiding this comment.
As mentioned in the PR description, Bank's wrapper around RentCollector would then override this method to inject debug! and metrics submission.
buffalojoec
commented
Aug 21, 2024
Comment on lines
+82
to
+101
| /// Determine the rent state of an account. | ||
| /// | ||
| /// This method has a default implementation that treats accounts with zero | ||
| /// lamports as uninitialized and uses the implemented `get_rent` to | ||
| /// determine whether an account is rent-exempt. | ||
| fn get_account_rent_state(&self, account: &AccountSharedData) -> RentState { | ||
| if account.lamports() == 0 { | ||
| RentState::Uninitialized | ||
| } else if self | ||
| .get_rent() | ||
| .is_exempt(account.lamports(), account.data().len()) | ||
| { | ||
| RentState::RentExempt | ||
| } else { | ||
| RentState::RentPaying { | ||
| data_size: account.data().len(), | ||
| lamports: account.lamports(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I've taken the methods from RentState and implemented them on the trait itself to provide more customization.
buffalojoec
commented
Aug 21, 2024
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
All of these tests came from SVM's account_rent_state module, but refactored to use the trait methods implemented on RentCollector.
LucasSte
approved these changes
Aug 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
As part of the ongoing effort to evict
solana-metricsfrom the SVM, we've realized that the only way to evict the metrics submission that occurs deep in account rent state management is to factor out all rent management into a plugin trait, similar toTransactionProcessingCallback.Summary of Changes
As a first step in this process, I've introduced a new crate for SVM rent management -
solana-svm-rent-collector. Within this new crate is a trait -SVMRentCollector- that will be used by the SVM (in future PRs) to perform rent management duties, including evaluating account rent state and collecting rent.Much of the trait is based on the current
account_rent_statemodule in SVM as well as the SDK'sRentCollectorstruct. I've intentionally omitted a few small things (logs and metrics) as well as repurposed method signatures to suit the new trait. I've provided comments for each of these changes.The new crate also provides an implementation of
SVMRentCollectorfor the SDK'sRentCollectorstruct. Downstream users can simply provide aRentCollectorif they do not wish to customize rent behavior, and the SVM can default toRentCollector::default()when noSVMRentCollectorimplementation is provided.The idea within Bank would be to create a wrapper struct around
RentCollectorthat will override the methods that require metrics and logging specific to Agave.This follow-up work will be done in subsequent PRs. Here's a draft of the next one, where I've done the Bank wrapper type: buffalojoec#29.