Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Handle accounts data size changes due to rent-collected accounts#22412

Merged
brooksprumo merged 1 commit intosolana-labs:masterfrom
brooksprumo:accounts-data-len/rent-collection
Jan 11, 2022
Merged

Handle accounts data size changes due to rent-collected accounts#22412
brooksprumo merged 1 commit intosolana-labs:masterfrom
brooksprumo:accounts-data-len/rent-collection

Conversation

@brooksprumo
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo commented Jan 10, 2022

Problem

Accounts that are reclaimed during rent collection (i.e. lamports go to zero) are not factored into the Bank's accounts_data_len.

Summary of Changes

Track reclaimed accounts data len during rent collection, then return that information to the bank so it can update its accounts data len.

  • Return CollectedInfo from collect_from_existing_account() to track both the rent amount and account data len
  • In collect_rent_in_partition(), sum all the collected-infos then use that final value to update the bank's accounts data len

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 10, 2022

Codecov Report

Merging #22412 (6395e4d) into master (637e366) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

❗ Current head 6395e4d differs from pull request most recent head 94b11b7. Consider uploading reports for the commit 94b11b7 to get more accurate results

@@            Coverage Diff            @@
##           master   #22412     +/-   ##
=========================================
- Coverage    81.1%    81.0%   -0.1%     
=========================================
  Files         553      551      -2     
  Lines      150439   150249    -190     
=========================================
- Hits       122075   121848    -227     
- Misses      28364    28401     +37     

@brooksprumo brooksprumo marked this pull request as ready for review January 10, 2022 22:28
Comment thread runtime/src/bank.rs Outdated
.unwrap()
.extend(rent_debits.into_unordered_rewards_iter());
if total_collected.account_data_len > 0 {
self.update_accounts_data_len(-(total_collected.account_data_len as i64));
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Originally I wanted to use atomic fetch_sub() here, but that doesn't have a 'saturating' version (more details below). I need the update_accounts_data_len() version for a future PR to handle where the accounts data len can either shrink or grow, so pulled that in to use here.

So fetch_sub() should work in all real/production cases†, but causes quite a few tests to fail. This is because the Bank that's created for tests has an accounts_data_len of 0. The test adds accounts, but not in a way that updates accounts data len. Thus, the tests that check for rent cleaning up accounts cause the accounts_data_len to wrap back to a very large positive number.

† The bank's accounts_data_len is not 100% the true size, and I'm not sure it will be. So I'm also concerned of a production scenario that causes the wrap around to happen and takes down the cluster.

To solve, I could've:

  1. Update all the test by manually setting a non zero accounts_data_len
  2. Have a test and not-test (#[cfg(test)] and #[cfg(not(test))] block right here at this line
  3. My current impl

I went with (3) because (1) seems fragile, and (2) seems a bit dangerous to not have the same logic for tests and not tests on such a core component.

Let me know if you'd like a different solution!

Comment thread runtime/src/rent_collector.rs Outdated
Comment thread runtime/src/rent_collector.rs Outdated
jeffwashington
jeffwashington previously approved these changes Jan 10, 2022
Copy link
Copy Markdown
Contributor

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

Comment thread runtime/src/bank.rs
Comment thread runtime/src/bank.rs
Comment thread runtime/src/bank.rs
Comment thread runtime/src/bank.rs
@mergify mergify Bot dismissed jeffwashington’s stale review January 11, 2022 16:30

Pull request has been modified.

@brooksprumo brooksprumo force-pushed the accounts-data-len/rent-collection branch from 6395e4d to 94b11b7 Compare January 11, 2022 20:39
@brooksprumo brooksprumo requested a review from jstarry January 11, 2022 21:27
Copy link
Copy Markdown
Contributor

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for adding the tests!

@brooksprumo brooksprumo merged commit c45dde6 into solana-labs:master Jan 11, 2022
@brooksprumo brooksprumo deleted the accounts-data-len/rent-collection branch January 11, 2022 23:20
mergify Bot pushed a commit that referenced this pull request Feb 3, 2022
)

(cherry picked from commit c45dde6)

# Conflicts:
#	runtime/src/bank.rs
#	runtime/src/rent_collector.rs
mergify Bot added a commit that referenced this pull request Feb 4, 2022
) (#22919)

Co-authored-by: Brooks Prumo <brooks@solana.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants