Skip to content

Removes featurization for remove_accounts_delta_hash#7006

Merged
brooksprumo merged 1 commit intoanza-xyz:masterfrom
brooksprumo:lthash/223/featurization
Jul 17, 2025
Merged

Removes featurization for remove_accounts_delta_hash#7006
brooksprumo merged 1 commit intoanza-xyz:masterfrom
brooksprumo:lthash/223/featurization

Conversation

@brooksprumo
Copy link
Copy Markdown

@brooksprumo brooksprumo commented Jul 16, 2025

The remove_accounts_delta_hash feature (SIMD-223) has been activated on all public clusters, and its featurization code can be removed.

Note, there's lots of additional cleanup to do after this PR too! This one is meant to be the minimum amount of changes to remove uses of feature_set::remove_accounts_delta_hash.

@brooksprumo brooksprumo self-assigned this Jul 16, 2025
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 16, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.2%. Comparing base (a86cc4b) to head (f00d9af).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master    #7006     +/-   ##
=========================================
- Coverage    83.2%    83.2%   -0.1%     
=========================================
  Files         853      853             
  Lines      375118   375020     -98     
=========================================
- Hits       312256   312135    -121     
- Misses      62862    62885     +23     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread runtime/src/bank.rs
.feature_set
.is_active(&feature_set::remove_accounts_delta_hash::id())
{
self.rc.accounts.accounts_db.start_background_hasher();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we remove this function since it's no longer used now?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yep! There's lots of cleanup to do. I have a big branch where I've been removing everything, then I break that out into smaller PRs that are self contained and easier to review.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Comment thread runtime/src/bank.rs
if new_feature_activations.contains(&feature_set::remove_accounts_delta_hash::id()) {
// If the accounts delta hash has been removed, then we no longer need to compute the
// AccountHash for modified accounts, and can stop the background account hasher.
self.rc.accounts.accounts_db.stop_background_hasher();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks like this can be removed too

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@brooksprumo brooksprumo force-pushed the lthash/223/featurization branch from 0569b95 to f00d9af Compare July 17, 2025 13:33
@brooksprumo brooksprumo marked this pull request as ready for review July 17, 2025 13:42
@brooksprumo brooksprumo requested review from HaoranYi and roryharr July 17, 2025 13:42
@brooksprumo brooksprumo moved this to In Progress in Accounts Lt Hash Jul 17, 2025
Copy link
Copy Markdown

@HaoranYi HaoranYi 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 1f906a9 into anza-xyz:master Jul 17, 2025
41 checks passed
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Accounts Lt Hash Jul 17, 2025
@brooksprumo brooksprumo deleted the lthash/223/featurization branch July 17, 2025 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants