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

Move cap_accounts_data_len feature gate only around new error #23048

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Feb 10, 2022

Problem

We'd like to track accounts data size independent of the cap_accounts_data_len feature. The accounts data size is already a datapoint in bank_forks::SetRootMetrics, but it currently never updates since the feature is disabled.

Summary of Changes

  • Make it so InvokeContext always consumes from AccountsDataMeter. If feature is disabled, do not throw any errors.
  • Always update accounts data len when handling rent exempt reclaims

@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #23048 (2fa685f) into master (67f6787) will decrease coverage by 0.0%.
The diff coverage is 92.3%.

@@            Coverage Diff            @@
##           master   #23048     +/-   ##
=========================================
- Coverage    81.2%    81.2%   -0.1%     
=========================================
  Files         564      564             
  Lines      153419   153422      +3     
=========================================
- Hits       124667   124655     -12     
- Misses      28752    28767     +15     

@brooksprumo brooksprumo marked this pull request as ready for review February 10, 2022 02:09
Comment on lines -4612 to -4616
if self
.feature_set
.is_active(&feature_set::cap_accounts_data_len::id())
&& total_collected.account_data_len_reclaimed > 0
{
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 just made this change yesterday, now to undo it... #22918

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly you are not checking the feature gate here because update_accounts_data_len() won't throw an error / change the outcome and you want the calculation to happen for statistical purposes, right?

Copy link
Contributor Author

@brooksprumo brooksprumo Feb 10, 2022

Choose a reason for hiding this comment

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

Correct!

Yesterday when I added the feature gate here, it was to keep everything consistent w.r.t. accounts_data_len; either nothing changes it, or both rent and transaction processing changes it. So similarly now, to track the changes with consistency for stats, I need to remove the feature gate both here (rent) and transaction processing.

@brooksprumo brooksprumo merged commit 0a1ab94 into solana-labs:master Feb 10, 2022
@brooksprumo brooksprumo deleted the accounts-data-len/only-feature-gate-error branch February 10, 2022 17:57
mergify bot pushed a commit that referenced this pull request Feb 10, 2022
mergify bot added a commit that referenced this pull request Feb 10, 2022
#23057)

(cherry picked from commit 0a1ab94)

Co-authored-by: Brooks Prumo <brooks@solana.com>
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Feb 24, 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.

2 participants