Rename SnapshotHashes to LegacySnapshotHashes #31086
Conversation
Codecov Report
@@ Coverage Diff @@
## master #31086 +/- ##
=======================================
Coverage 81.5% 81.5%
=======================================
Files 728 728
Lines 205783 205790 +7
=======================================
+ Hits 167835 167889 +54
+ Misses 37948 37901 -47 |
core/src/accounts_hash_verifier.rs
Outdated
| @@ -479,7 +479,7 @@ impl AccountsHashVerifier { | |||
| hashes.push((accounts_package.slot, *accounts_hash.as_hash())); | |||
| } | |||
|
|
|||
| retain_max_n_elements(hashes, MAX_SNAPSHOT_HASHES); | |||
| retain_max_n_elements(hashes, MAX_LEGACY_SNAPSHOT_HASHES); | |||
There was a problem hiding this comment.
Shouldn't this actually be MAX_ACCOUNT_HASHES?
This whole file does not seem to be about snapshot-hashes, but only account-hashes.
There was a problem hiding this comment.
Yes, you're correct, good catch. I had used my editor's LSP to rename MAX_SNAPSHOT_HASHES but did not audit the changes in this file. I'll change 'em to use MAX_ACCOUNTS_HASHES.
gossip/src/crds_value.rs
Outdated
| pub base: (Slot, Hash), | ||
| pub hashes: Vec<(Slot, Hash)>, |
There was a problem hiding this comment.
Looks like this already includes base which I am guessing is the full snapshot hash;
So why do we need a new SnapshotHashes?
There was a problem hiding this comment.
Good question! We could use IncrementalSnapshotHashes instead; I was under the impression that we would want to remove the Vec for most space savings. Would it be better to repurpose IncrementalSnapshotHashes instead?
There was a problem hiding this comment.
Repurposing IncrementalSnapshotHashes and setting MAX_INCREMENTAL_SNAPSHOT_HASHES = 1 should get us most of the way there, w.r.t. space savings.
Is this a better trade off, to avoid adding more data variants to gossip? There will still be some rollout management to coordinate, but we won't have to worry about turning on/off any new/legacy data types.
There was a problem hiding this comment.
Why we can't just use current IncrementalSnapshotHashes without any changes?
I mean #30759 claims that:
Bootstrap runs into issues when it is discovering snapshots due to full snapshots and incremental snapshots gossiping out separately.
But from IncrementalSnapshotHashes struct, it does look like it is gossip both full and incremental snapshots together!
There was a problem hiding this comment.
Yes, we can use IncrementalSnapshotHashes without any changes.
I would like to make a few naming changes though:
- rename
SnapshotHashesandIncrementalSnapshotHashestoLegacySnapshotHashesandSnapshotHashes, respectively - rename
baseandhashestofullandincremental, respectively
Maybe (1) is not needed? It seems usefully to at least rename SnapshotHashes to LegacySnapshotHashes even if we do not rename IncrementalSnapshotHashes.
I think it'd also be useful to reduce the value for MAX_INCREMENTAL_SNAPSHOT_HASHES, but that can be handled later.
There was a problem hiding this comment.
I would like to make a few naming changes though:
rename
SnapshotHashesandIncrementalSnapshotHashestoLegacySnapshotHashesandSnapshotHashes, respectivelyrename
baseandhashestofullandincremental, respectively
yeah, this sounds fine.
I think it'd also be useful to reduce the value for
MAX_INCREMENTAL_SNAPSHOT_HASHES, but that can be handled later.
Any reason to do this other than saving bytes from bandwidth?
There was a problem hiding this comment.
Any reason to do this other than saving bytes from bandwidth?
Saving bandwidth was the primary reason. If that's not worth it though, I won't pursue.
brooksprumo
force-pushed
the
snapshot-gossip/legacy
branch
from
4134bb9 to
eafdc69
Compare
|
@behzadnouri This PR has been updated to just handle the rename from Let me know if you would like me to combine the two PRs though. |
Problem
To handle bootstrap better, we'll be changing how
IncrementalSnapshotHasheswill be used. It will contain hashes for both full and incremental snapshots. Then, the currentSnapshotHasheswill no longer be used, and should be indicated as such.Summary of Changes
Rename
SnapshotHashestoLegacySnapshotHashes.Also rename the push/pull functions to include "legacy" in the name.
(The next PR will rename
IncrementalSnapshotHashestoSnapshotHashes)