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

Uses 0 for write version at startup for geyser #721

Closed

Conversation

brooksprumo
Copy link

@brooksprumo brooksprumo commented Apr 10, 2024

Problem

We're trying to remove write version everywhere. The only remaining use of write version is for geyser. As an incremental step, we'd like to remove the write_version() method on StoredAccountMeta. Both to prevent new uses, and also because Tiered Storage does not support getting/setting a write version at all.

At startup, when loading from a snapshot, geyser is notified of all accounts. Currently this uses the write version from each account. The observation is that at startup we know (1) there is only a single version of each account, and (2) there are no concurrent slots/banks getting replayed. This means we'll never have concurrent updates to the same account and would need to disambiguate between them which is newer. This allows us to set the write version to 0.

Summary of Changes

Use 0 for write version at startup.

Also note that we are manually setting the write version to 0 on the StoredAccountMeta just before accountinfo_from_stored_account_meta() is called:

let local_write_version = 0;
for (_, mut account) in accounts_to_stream.drain() {
// We do not need to rely on the specific write_version read from the append vec.
// So, overwrite the write_version with something that works.
// 'accounts_to_stream' is already a hashmap, so there is already only entry per pubkey.
// write_version is only used to order multiple entries with the same pubkey, so it doesn't matter what value it gets here.
// Passing 0 for everyone's write_version is sufficiently correct.
let meta = StoredMeta {
write_version_obsolete: local_write_version,
..*account.meta()
};
account.set_meta(&meta);

So the current behavior is unchanged with this PR.

Related to #702

@brooksprumo brooksprumo self-assigned this Apr 10, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (70c4cb0) to head (e6d24c5).
Report is 11 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #721   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         851      851           
  Lines      230709   230710    +1     
=======================================
+ Hits       189003   189045   +42     
+ Misses      41706    41665   -41     

@brooksprumo brooksprumo marked this pull request as ready for review April 10, 2024 21:56
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

@brooksprumo brooksprumo requested a review from lijunwangs April 11, 2024 14:23
@brooksprumo
Copy link
Author

@lijunwangs Added you as a reviewer here to double check this'll be ok for geyser.

@brooksprumo
Copy link
Author

brooksprumo commented Apr 11, 2024

Ah, just realized that this PR does not change behavior at all. Prior to this function (accountinfo_from_stored_account_meta()) getting called, we're manually setting the write version of the StoredAccountMeta to 0:

let local_write_version = 0;
for (_, mut account) in accounts_to_stream.drain() {
// We do not need to rely on the specific write_version read from the append vec.
// So, overwrite the write_version with something that works.
// 'accounts_to_stream' is already a hashmap, so there is already only entry per pubkey.
// write_version is only used to order multiple entries with the same pubkey, so it doesn't matter what value it gets here.
// Passing 0 for everyone's write_version is sufficiently correct.
let meta = StoredMeta {
write_version_obsolete: local_write_version,
..*account.meta()
};
account.set_meta(&meta);

I've updated the PR description to mention this.

And by setting 0 here in this PR, it'll allow us to remove the call to set_meta() in #702 easier (since we can guarantee no behavior change again).

@brooksprumo
Copy link
Author

Closing. This has been obviated by #979.

@brooksprumo brooksprumo deleted the write-version/geyser branch April 29, 2024 15:16
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