Skip to content
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

Enforce accounts data size limit per block in ReplayStage #25524

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented May 24, 2022

Problem

The per-block accounts data size limit is only part of BankingStage, and is not checked in ReplayStage.

Summary of Changes

Add checks to ReplayStage that will fail blocks that exceed the accounts data size per-block limit.

Feature Gate Issue: #25517

@brooksprumo brooksprumo added work in progress This isn't quite right yet feature-gate Pull Request adds or modifies a runtime feature gate v1.10 labels May 24, 2022
@brooksprumo brooksprumo force-pushed the replay-stage-accounts-data-cap-per-block branch from 7a46f0e to 224ac7b Compare May 25, 2022 11:15
@brooksprumo brooksprumo removed the work in progress This isn't quite right yet label May 25, 2022
@brooksprumo brooksprumo marked this pull request as ready for review May 25, 2022 11:29
@codecov
Copy link

codecov bot commented May 25, 2022

Codecov Report

Merging #25524 (2e246e9) into master (8caced6) will decrease coverage by 0.0%.
The diff coverage is 82.4%.

@@            Coverage Diff            @@
##           master   #25524     +/-   ##
=========================================
- Coverage    82.1%    82.1%   -0.1%     
=========================================
  Files         628      631      +3     
  Lines      171471   173848   +2377     
=========================================
+ Hits       140878   142804   +1926     
- Misses      30593    31044    +451     

tao-stones
tao-stones previously approved these changes May 25, 2022
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

}

debug_assert!(MAX_ACCOUNT_DATA_BLOCK_LEN <= i64::MAX as u64);
if bank.load_accounts_data_size_delta_on_chain() > MAX_ACCOUNT_DATA_BLOCK_LEN as i64 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not familiar with what bank.load_accounts_data_size_delta_on_chain(), but the logic looks good. Will deep dive to that function late.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change in accounts data size from the bank processing transactions is tracked in Bank::accounts_data_size_delta_on_chain, an AtomicI64. So ReplyStage here will check to ensure the block hasn't allocated more accounts data this block that the max (MAX_ACCOUNT_DATA_BLOCK_LEN), and only due to transactions.

This purposely ignores any other changes to the accounts data size, like reclaimed accounts due to rent collection (which are tracked in Bank::accounts_data_size_delta_off_chain).

@brooksprumo brooksprumo requested a review from sakridge May 25, 2022 21:30
@brooksprumo
Copy link
Contributor Author

brooksprumo commented May 25, 2022

@sakridge Can you take a look too to make sure this is as-expected?

@brooksprumo brooksprumo force-pushed the replay-stage-accounts-data-cap-per-block branch from 224ac7b to 58697db Compare May 26, 2022 10:33
@mergify mergify bot dismissed tao-stones’s stale review May 26, 2022 10:33

Pull request has been modified.

@brooksprumo brooksprumo force-pushed the replay-stage-accounts-data-cap-per-block branch from 58697db to f79078f Compare June 1, 2022 11:42
@brooksprumo brooksprumo force-pushed the replay-stage-accounts-data-cap-per-block branch from 4888ef6 to 978f312 Compare June 7, 2022 16:38
@brooksprumo brooksprumo requested a review from sakridge June 7, 2022 18:11
@brooksprumo brooksprumo force-pushed the replay-stage-accounts-data-cap-per-block branch from 978f312 to 49d4357 Compare June 7, 2022 18:33
@brooksprumo
Copy link
Contributor Author

@sakridge This PR is ready for another review.

@brooksprumo brooksprumo force-pushed the replay-stage-accounts-data-cap-per-block branch 2 times, most recently from aca39a5 to 27ae751 Compare June 9, 2022 14:36
@brooksprumo
Copy link
Contributor Author

Rebased and force-pushed to handle feature set conflicts.

@brooksprumo brooksprumo force-pushed the replay-stage-accounts-data-cap-per-block branch from 27ae751 to f397e33 Compare June 10, 2022 15:14
@brooksprumo brooksprumo force-pushed the replay-stage-accounts-data-cap-per-block branch from f397e33 to 2e246e9 Compare June 15, 2022 18:32
@brooksprumo
Copy link
Contributor Author

Rebased and force-pushed to handle feature set conflicts.

@brooksprumo brooksprumo merged commit b4b191e into solana-labs:master Jun 16, 2022
@brooksprumo brooksprumo deleted the replay-stage-accounts-data-cap-per-block branch June 16, 2022 01:35
mergify bot pushed a commit that referenced this pull request Jun 16, 2022
(cherry picked from commit b4b191e)

# Conflicts:
#	ledger/src/blockstore_processor.rs
mergify bot added a commit that referenced this pull request Jun 16, 2022
…25524) (#25998)

* Enforce accounts data size limit per block in ReplayStage (#25524)

(cherry picked from commit b4b191e)

# Conflicts:
#	ledger/src/blockstore_processor.rs

* fixup! resolve merge conflicts

Co-authored-by: Brooks Prumo <brooks@solana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants