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

Track more accounts data size changes #26467

Merged

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Jul 7, 2022

Problem

Not all of the off-chain changes to accounts data size are tracked. In addition, calling Bank::get_total_accounts_stats() used to always be greater than Bank::load_accounts_data_size() by a constant amount—no more!

As I investigate why the accounts data size is different when starting from a snapshot vs replay, I found some accounts data size changes that weren't tracked. This PR doesn't fully solve that issue, but that's the context behind this PR.

Summary of Changes

Track 'em! (Also add some tests.)

Mostly audited all calls to store_account() and store_accounts() to see if the account may've changed size.

@brooksprumo brooksprumo added work in progress This isn't quite right yet noCI Suppress CI on this Pull Request labels Jul 7, 2022
@brooksprumo brooksprumo force-pushed the accounts-data-len/genesis branch from 93ca829 to d2d3293 Compare July 7, 2022 05:01
@brooksprumo brooksprumo added CI Pull Request is ready to enter CI and removed work in progress This isn't quite right yet noCI Suppress CI on this Pull Request labels Jul 7, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Jul 7, 2022
@brooksprumo brooksprumo marked this pull request as ready for review July 7, 2022 13:57
self.capitalization
.fetch_add(native_mint_account.lamports(), Relaxed);
true
};

if store {
self.store_account(&inline_spl_token::native_mint::id(), &native_mint_account);
let data_size_delta = (native_mint_account.data().len() as i64)
.saturating_sub(old_account_data_size as i64);
self.update_accounts_data_size_delta_off_chain(data_size_delta);
Copy link
Contributor

Choose a reason for hiding this comment

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

seems useful to have an update function that takes (new_lamports, prev_lamports) and does the casts, saturating subs, delta update, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I was thinking combine both of these:

let data_size_delta =
                    compute_data_size_delta(old_account.data().len(), new_account.data().len());
                self.update_accounts_data_size_delta_off_chain(data_size_delta);
                ```

Copy link
Contributor

Choose a reason for hiding this comment

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

self.update_accounts_data_size_difference_off_chain(old, new);?

Copy link
Contributor

Choose a reason for hiding this comment

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

name is bad. just the idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Done! (I chose calculate_and_update_accounts_data_size_delta_off_chain() 🙃)

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, I am so much happy

&solana_sdk::pubkey::new_rand(),
);
let mut account =
AccountSharedData::new(account_balance, 0, &solana_sdk::pubkey::new_rand());
Copy link
Contributor

Choose a reason for hiding this comment

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

is this still rent exempt? Not sure how test is relying on that, but test name implies that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't add this test originally, but did later modify it to include a non-zero data size. Since then, I have added more tests for the accounts data size and it's no longer required here, so I wanted to return this test back to its original behavior. Since store_account() is called, I wanted to remove as much special-case code around it as possible. Let me know if you'd like me to change something in here.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's fine. just a little confusing and making sure we understand.

Copy link
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

@brooksprumo brooksprumo merged commit 785a7a5 into solana-labs:master Jul 9, 2022
@brooksprumo brooksprumo deleted the accounts-data-len/genesis branch July 9, 2022 00:17
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants