SnapshotPackagerService pushes incremental snapshot hashes to CRDS #20442
Conversation
Codecov Report
@@ Coverage Diff @@
## master #20442 +/- ##
=========================================
- Coverage 82.1% 82.1% -0.1%
=========================================
Files 492 493 +1
Lines 137136 137206 +70
=========================================
+ Hits 112676 112690 +14
- Misses 24460 24516 +56 |
gossip/src/crds_value.rs
Outdated
| wallclock: u64, | ||
| pub(crate) from: Pubkey, | ||
| pub(crate) base: (Slot, Hash), | ||
| pub(crate) hashes: Vec<(Slot, Hash)>, |
There was a problem hiding this comment.
It might be better to break hashes struct into different indexes like we do when we have multiple votes, hence why we use an index in the gossip key for those votes:
solana/gossip/src/crds_value.rs
Line 490 in 57592e4
This has the benefits that people can't push unbounded vectors and also if we update one hash, we don't have to rewrite the entire vector in gossip
There was a problem hiding this comment.
Oh interesting, I didn't know that was an option; I was following the precedent from SnapshotHashes.
@behzadnouri What do you think about changing the way IncrementalSnapshotHashes is in CRDS to be what @carllin suggested above?
@carllin Some additional info: this vec is at most 25 entries (but usually only 4, since 4 is the default number of incremental snapshot archives to retain), and it's pushed maximum once per incremental_snapshot_interval, which defaults to 100 slots, so not very often.
If Behzad OKs that change, I would prefer to break that out as a follow-on PR instead of doing that work within this PR. Does that work?
There was a problem hiding this comment.
This has the benefits that people can't push unbounded vectors
They already cannot do it. After some point the value will not fit into a Packet and cannot be sent over push-messages or pull-reponses.
Each added crds value will have an overhead, so I think we are better to keep the current definition. The indexing is only useful when there is no way to use a single crds value.
There was a problem hiding this comment.
@behzadnouri makes sense, thanks!
Now that CRDS supports incremental snapshot hashes, SnapshotPackagerService needs to push 'em! This commit does two main things: 1. SnapshotPackagerService now knows about incremental snapshot hashes, and will push SnapshotPackage::IncrementalSnapshot hashes to CRDS. 2. At startup, when loading from a full + incremental snapshot, the hashes need to be passed all the way to SnapshotPackagerService so it can push these starting hashes to CRDS. Those values have been piped through. Fixes solana-labs#20441 and solana-labs#20423
…olana-labs#20442) Now that CRDS supports incremental snapshot hashes, SnapshotPackagerService needs to push 'em! This commit does two main things: 1. SnapshotPackagerService now knows about incremental snapshot hashes, and will push SnapshotPackage::IncrementalSnapshot hashes to CRDS. 2. At startup, when loading from a full + incremental snapshot, the hashes need to be passed all the way to SnapshotPackagerService so it can push these starting hashes to CRDS. Those values have been piped through. Fixes solana-labs#20441 and solana-labs#20423
… CRDS (solana-labs#20442)" This reverts commit 0b266d2.
Now that CRDS supports incremental snapshot hashes,
SnapshotPackagerService needs to push 'em!
This commit does two main things:
and will push SnapshotPackage::IncrementalSnapshot hashes to CRDS.
hashes need to be passed all the way to SnapshotPackagerService so it
can push these starting hashes to CRDS. Those values have been piped
through.
Fixes #20441 and #20423