Removes setting write version for geyser when restoring from a snapshot

[brooksprumo]

Problem

We are trying to move account storage files away from using mmaps internally. The function StoredAccountMeta::set_meta() updates the underlying mmap, which will not be possible once mmaps are removed. (It's also not supported for Tiered Storage at all.)

agave/accounts-db/src/account_storage/meta.rs

Lines 174 to 181 in 70c4cb0

	pub fn set_meta(&mut self, meta: &'storage StoredMeta) {
	match self {
	Self::AppendVec(av) => av.set_meta(meta),
	// Hot account does not support this API as it does not
	// use the same in-memory layout as StoredMeta.
	Self::Hot(_) => unreachable!(),
	}
	}

The only caller of set_meta() is in geyser, when restoring from a snapshot.

Back when the call to set_meta() was added (in PR solana-labs#29623), we already knew that write version shouldn't be an issue. With the account write cache, we'll never have more than one version of an account written per slot to a storage file. And when restoring from a snapshot, we only will notify geyser with the latest version of each account (i.e. the version in the highest slot number). So we'll never need to disambiguate which account is "newer" for geyser when restoring from a snapshot (because there will only be one, and it will be the newest).

Now also, we always store zero for the write version to the append vecs. Geyser is the last piece that uses write version. Since geyser will still increment write version for its notifications, post-restore notifications will have a higher write version. (And really, geyser apps should be monitoring for notify_end_of_restore_from_snapshot() to know when account notifications from snapshots has ended.)

Summary of Changes

Removes setting write version for geyser when restoring from a snapshot (i.e. stop calling StoredAccountMeta::set_meta()).

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.8%. Comparing base (c0be86d) to head (55180cf).
Report is 20 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master     #702     +/-   ##
=========================================
- Coverage    81.9%    81.8%   -0.1%     
=========================================
  Files         851      851             
  Lines      230475   230495     +20     
=========================================
- Hits       188785   188770     -15     
- Misses      41690    41725     +35     

[jeffwashington]

i guess this is only updating the local StoredAccountMeta<'_>. It has been 15 months since I last visited this code. I'm happy 0 worked. I don't recall what will happen if we pass random data, which is what is going to happen now.

[jeffwashington]

@lijunwangs I'm sorry to have to keep asking you about this.

[brooksprumo]

I don't think the meta will be random; it'll be whatever was in the append vec. Now we're just not replacing the write version.

[jeffwashington]

my recollection is that I passed a constant 0 here to completely decouple what was written in the append vecs from what geyser got. Ugh. Are we always zeroing write verison now in master. I think so. But, we aren't zeroing it on 1.17 or 1.18. That means when master loads a snapshot built by 1.17, we'll be passing incrementing write versions here. Maybe that is ok. Maybe that is what we used to do? We may have to wait to remove this until we are always writing 0?
Or, maybe the fn that creates StoredAccountMeta is already setting this to 0 everytime? I think I just reviewed that?

[brooksprumo]

my recollection is that I passed a constant 0 here to completely decouple what was written in the append vecs from what geyser got.

Ah, well, then we should be safe to merge #721, since it is just moving where we put that 0 constant. That'll then make this issue moot, as we will no longer need to use set_meta() to change the write version anymore.

Are we always zeroing write verison now in master.

Yep, as of #476 (and #561 makes it clearer).

But, we aren't zeroing it on 1.17 or 1.18.

Correct.

That means when master loads a snapshot built by 1.17, we'll be passing incrementing write versions here. Maybe that is ok.

I think it is ok, and I put my reasoning in the PR description. We could also pull in #721 into this PR, and that'll make it obvious we are not changing any behavior; just moving where the constant is set. Wdyt?

[jeffwashington]

We could also pull in #721 into this PR, and that'll make it obvious we are not changing any behavior; just moving where the constant is set. Wdyt?

I think we just merge 721 and then reference it here. Which we just did. We could assert that write_version is 0 here?

[brooksprumo]

We could assert that write_version is 0 here?

Where specifically is "here"?

When loading from a snapshot, it may be from an older version that was storing non-zero write versions still. That means the accounts_to_stream may have non-zero write versions. So we wouldn't want to add an assert here, since we know it'll fail. But that's ok/expected, as #721 will always put a 0 for the write version anyway.

[jeffwashington]

@brooksprumo assert write version is 0 here?

[brooksprumo]

If we no longer call set_meta(), then the account here may still have a non-zero write version.

This will happen when starting from a snapshot generated by a node running an older version that doesn't always store 0 for write version.

[brooksprumo]

Closing. This has been obviated by #979.