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

Removes write version from StorableAccountsWithHashesAndWriteVersions #561

Conversation

brooksprumo
Copy link

Problem

Now that we no longer use the write version when storing accounts (see #476), and StorableAccounts no longer contains anything about write version either (see #542), StorableAccountsWithHashesAndWriteVersions does not need to care either.

Summary of Changes

Remove write version from StorableAccountsWithHashesAndWriteVersions.

@brooksprumo brooksprumo self-assigned this Apr 3, 2024
@brooksprumo brooksprumo marked this pull request as ready for review April 3, 2024 15:24
Copy link
Author

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

There's a lot of noise in the diff due to renaming the type and removing a parameter. I've tried to call out the important parts.

@@ -747,7 +747,7 @@ impl AppendVec {
data_len: account
.map(|account| account.data().len())
.unwrap_or_default() as u64,
write_version_obsolete,
write_version_obsolete: 0,
Copy link
Author

Choose a reason for hiding this comment

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

And finally, we explicitly always store a zero for write version!

Comment on lines +906 to +907
fn test_storable_accounts_with_hashes_new2() {
test_mismatch(false);
Copy link
Author

Choose a reason for hiding this comment

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

The test_mismatch() stuff can be refactored. I purposely didn't do that here in this PR to keep the diff small. I'll do the cleanup in a follow up PR.

)
} else {
let write_versions = vec![0; accounts.len()];
Copy link
Author

Choose a reason for hiding this comment

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

And now we don't need to create this vector of zeroes either!

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (0168e0a) to head (ac1858f).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master     #561     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         847      847             
  Lines      229180   229144     -36     
=========================================
- Hits       187600   187502     -98     
- Misses      41580    41642     +62     

Copy link

@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

Copy link

@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.

:shipit:

@brooksprumo brooksprumo merged commit afa65c6 into anza-xyz:master Apr 3, 2024
38 checks passed
@brooksprumo brooksprumo deleted the write-version/storable-accounts-with-hashes branch April 3, 2024 16:41
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.

4 participants