Removes cap-accounts-data-len bits when constructing a new Bank #34688
Conversation
| cost_tracker: RwLock::new(CostTracker::new_with_account_data_size_limit( | ||
| feature_set | ||
| .is_active(&feature_set::cap_accounts_data_len::id()) | ||
| .then(|| { | ||
| parent | ||
| .accounts_data_size_limit() | ||
| .saturating_sub(accounts_data_size_initial) | ||
| }), | ||
| )), |
There was a problem hiding this comment.
This is within new_from_parent(). By not setting CostTracker::account_data_size_limit, we won't enforce a total accounts data size limit during transaction processing.
| if self | ||
| .feature_set | ||
| .is_active(&feature_set::cap_accounts_data_len::id()) | ||
| { | ||
| self.cost_tracker = RwLock::new(CostTracker::new_with_account_data_size_limit(Some( | ||
| self.accounts_data_size_limit() | ||
| .saturating_sub(self.accounts_data_size_initial), | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Same thing as above, just that this covers the new_from_paths() and new_from_fields() constructors. IOW, starting up from genesis or a snapshot.
| if new_feature_activations.contains(&feature_set::cap_accounts_data_len::id()) { | ||
| const ACCOUNTS_DATA_LEN: u64 = 50_000_000_000; | ||
| self.accounts_data_size_initial = ACCOUNTS_DATA_LEN; | ||
| } |
There was a problem hiding this comment.
This was a hack to set a deterministic value for the initial total accounts data size. It's also quite wrong, value-wise, now.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #34688 +/- ##
=========================================
- Coverage 81.8% 81.8% -0.1%
=========================================
Files 824 824
Lines 222687 222672 -15
=========================================
- Hits 182245 182215 -30
- Misses 40442 40457 +15 |
|
Changes to remove
|
We still want to track the accounts data size in the bank, which transactions update. So these two tests are both still useful. Edit: I'll investigate why these tests use the
Yep! I have PRs already queued up to remove these bits. |
I've created #34701 to cleanup the tests. Both can proceed concurrently, as they do not impact each other. So this PR is ready for another review, @taozhu-chicago. Thanks! |
Problem
Setting a limit for the total accounts data size12 breaks consensus and cannot work3. The same issue exists for setting a limit per block4. Yet, we still have featurization code for both. This is an issue for a few reasons:
Some of the lowest hanging fruit to make developer-experience better is to rip out the code during Bank construction that activates the tracking/limiting.
Summary of Changes
Remove the code during Bank construction that enables/configures the accounts data size limits:
Footnotes
https://github.com/solana-labs/solana/issues/24135 ↩
https://github.com/solana-labs/solana/issues/21604 ↩
https://github.com/solana-labs/solana/issues/27029 ↩
https://github.com/solana-labs/solana/issues/25517 ↩