Limit accounts data size #21719
Limit accounts data size #21719
Conversation
brooksprumo
added
noCI
brooksprumo
force-pushed
the
cap-account-data-size
branch
from
eafc4b2
to
5a60296
Compare
| current: usize, | ||
| } | ||
| impl AccountsDataBudget { | ||
| pub fn new(maximum: usize, current: usize) -> Self { |
There was a problem hiding this comment.
Is maximum something that is going to change dynamically? If not could encapsulate that in AccountDataBudget
There was a problem hiding this comment.
I'm not sure if the maximum will change dynamically yet. This is still to make prototyping easiest, and to allow tests to set a different maximum as well. I agree that the max could be in AccountsDataBudget directly.
|
|
||
| /// bprumo TODO: doc, and/or move | ||
| #[derive(Debug, Default, Clone, Copy, Eq, PartialEq)] | ||
| pub struct AccountsDataBudget { |
There was a problem hiding this comment.
Since this is local to a tx, could just store remaining
There was a problem hiding this comment.
Yes, I agree. I'm still prototyping the tracking of the accounts data len, so was trying to keep around more information for now. Once I have a good idea on how this all fits together I was planning to reduce the impl.
| pub fn new(maximum: usize, current: usize) -> Self { | ||
| Self { maximum, current } | ||
| } | ||
| pub fn maximum(&self) -> usize { |
There was a problem hiding this comment.
Nope. Just here for convenience while prototyping.
|
|
||
| /// bprumo TODO: doc, and/or move | ||
| #[derive(Debug, Default, Clone, Copy, Eq, PartialEq)] | ||
| pub struct AccountsDataMeter { |
There was a problem hiding this comment.
Probably just need this, don't need the AccountDataBudget
There was a problem hiding this comment.
Yah. At the moment AccountsDataBudget is not really used for anything.
| @@ -783,6 +851,11 @@ impl<'a> InvokeContext<'a> { | |||
| self.compute_meter.clone() | |||
| } | |||
|
|
|||
| /// bprumo TODO: doc | |||
| pub fn accounts_data_meter(&self) -> Rc<RefCell<AccountsDataMeter>> { | |||
There was a problem hiding this comment.
our convention is get_account_data_meter
| @@ -1518,6 +1518,14 @@ struct IndexAccountMapEntry<'a> { | |||
|
|
|||
| type GenerateIndexAccountsMap<'a> = HashMap<Pubkey, IndexAccountMapEntry<'a>>; | |||
|
|
|||
| #[derive(Debug, Default, Clone, Copy)] | |||
| struct GenerateIndexForSlotResult { | |||
There was a problem hiding this comment.
Maybe a better name here, something more descriptive of what it actually represents?
There was a problem hiding this comment.
Yes, is for getting the accounts data len during deserialization of a snapshot.
| @@ -3514,6 +3545,7 @@ impl Bank { | |||
| let feature_set = self.feature_set.clone(); | |||
| signature_count += u64::from(tx.message().header().num_required_signatures); | |||
|
|
|||
| // bprumo NOTE: Here's where the ComputeBudget is created | |||
| let mut compute_budget = self.compute_budget.unwrap_or_else(ComputeBudget::new); | |||
There was a problem hiding this comment.
Maybe update the compute budget here with the number of bytes that can be allocated
There was a problem hiding this comment.
Yes, I can do that. It felt like the accounts data len wasn't really related to compute, but happy to move the max len into here.
| @@ -3562,11 +3595,22 @@ impl Bank { | |||
| instruction_recorders.as_deref(), | |||
| feature_set, | |||
| compute_budget, | |||
| AccountsDataBudget::new( | |||
There was a problem hiding this comment.
If passed via compute_budget don't need this param
| @@ -34,6 +36,12 @@ impl ::solana_frozen_abi::abi_example::AbiExample for MessageProcessor { | |||
| } | |||
| } | |||
|
|
|||
| /// bprumo TODO: doc | |||
| #[derive(Debug, Eq, PartialEq)] | |||
| pub struct ProcessMessageResult { | |||
There was a problem hiding this comment.
Could this just be a usize/u64?
There was a problem hiding this comment.
Yes! I tend to like types for their self-documenting-ness, and also to make it easier to add/remove fields while prototyping. Additionally, when a function doesn't naturally return the values I want, I find it helps to have a struct with named fields. I.e. this case, I would say it's not obvious that a call to process_message() should return the number of new bytes added to the accounts data. I can remove this too!
There was a problem hiding this comment.
Yeah, I could go either way depending on the case, down the leave it in
|
Closing. This has been implemented by #21994 |
Work in progress!
This PR is aiming to put a cap on the accounts data size—128 GB, in particular—by keeping track of the current accounts data size, and then checking instructions/transactions/messages against the amount of remaining space.
Some questions:
accounts_data_len, and all related variables, be tracked in au64or ausizeBankFieldsToSerialize/Deserialize?accounts_data_len, because otherwise that value needs to be calculated at runtimeAccountsDataBudgetandAccountsDataMetertypes live (i.e. which file(s))?Related to issue #21604