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.

Sign up for GitHub

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

Jump to bottom

Stops pushing legacy snapshot hashes to crds #33576

Merged
merged 1 commit into from
Oct 7, 2023
Merged

Stops pushing legacy snapshot hashes to crds #33576

merged 1 commit into from
Oct 7, 2023

Conversation

brooksprumo
Copy link
Contributor

@brooksprumo brooksprumo commented Oct 7, 2023 •
edited

Problem

Now that the clusters are on v1.16 (or later), we no longer query LegacySnapshotHashes during bootstrap to discover snapshots to download (as of #31275). This means we can also stop pushing that information to CRDS.

Summary of Changes

Stop pushing legacy snapshot hashes.

Fixes #31282

Note, this PR purposely does not remove anything from CRDS itself. That will be handled in subsequent PRs.

@brooksprumo brooksprumo added noCI Suppress CI on this Pull Request blocked Unable to proceed labels Oct 7, 2023
@brooksprumo brooksprumo self-assigned this Oct 7, 2023
@brooksprumo brooksprumo added CI Pull Request is ready to enter CI and removed noCI Suppress CI on this Pull Request blocked Unable to proceed labels Oct 7, 2023
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Oct 7, 2023
@brooksprumo brooksprumo added the v1.17 PRs that should be backported to v1.17 label Oct 7, 2023
@brooksprumo brooksprumo marked this pull request as ready for review October 7, 2023 13:48
brooksprumo
brooksprumo commented Oct 7, 2023
View reviewed changes
core/src/snapshot_packager_service/snapshot_gossip_manager.rs
Comment on lines -160 to -163
self.cluster_info
.push_legacy_snapshot_hashes(clone_hashes_for_crds(
self.legacy_full_snapshot_hashes.as_slice(),
));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the main change. Because we remove this call, then all the other code removals fall out from here.

@bw-solana
Copy link
Contributor

What happens if a node running legacy v1.14 tries to query the legacy snapshot hash?

@brooksprumo
Copy link
Contributor Author

What happens if a node running legacy v1.14 tries to query the legacy snapshot hash?

If the majority of nodes on the cluster are running v1.16 or older, the v1.14 node will bootstrap without issue. This is mnb today.

If the cluster has already enabled v1.16 feature gates, then a v1.14 node cannot join the network at all, so its bootstrap process is moot. This is testnet today.

This PR will only be backported to v1.17, and v1.16 is compatible with it. So testnet is fine.

If someone runs v1.17+ on mnb, it shouldn't impact a v1.14 node. This is because (1) the v1.17 node is unlikely to be listed as one of the v1.14 node's known validators, and (2), almost all the nodes on mnb are v1.16, since v1.17 is not recommended for mnb.

(And soon mnb's floor will raise to v1.16 due to feature gate activations, rendering v1.14 moot there as well.)

bw-solana
bw-solana approved these changes Oct 7, 2023
View reviewed changes
Copy link
Contributor

@bw-solana bw-solana 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 merged commit c588f25 into solana-labs:master Oct 7, 2023
31 checks passed
@brooksprumo brooksprumo deleted the bsb/stop-pushing-legacy-snapshot-hashes branch October 7, 2023 18:29
mergify bot pushed a commit that referenced this pull request Oct 7, 2023
@brooksprumo @mergify
Stops pushing legacy snapshot hashes to crds (#33576)
cb668e3 
(cherry picked from commit c588f25)
@mergify mergify bot mentioned this pull request Oct 7, 2023
Merged
brooksprumo added a commit that referenced this pull request Oct 7, 2023
@brooksprumo
Stops pushing legacy snapshot hashes to crds (#33576)
46832bb 
(cherry picked from commit c588f25)
deanmlittle pushed a commit to deanmlittle/solana that referenced this pull request Oct 8, 2023
@brooksprumo @deanmlittle
Stops pushing legacy snapshot hashes to crds (solana-labs#33576)
dce59b9
deanmlittle pushed a commit to deanmlittle/solana that referenced this pull request Oct 8, 2023
@brooksprumo @deanmlittle
Stops pushing legacy snapshot hashes to crds (solana-labs#33576)
19dfaea
behzadnouri added a commit to behzadnouri/solana that referenced this pull request Oct 9, 2023
@behzadnouri
removes unused legacy snapshot hashes api in gossip
823e7cb 
solana-labs#33576
stops broadcasting legacy snapshot hashes over gossip, and the commit
removes unused legacy snapshot hashed code in gossip.
behzadnouri added a commit to behzadnouri/solana that referenced this pull request Oct 9, 2023
@behzadnouri
removes unused legacy-snapshot-hashes api in gossip
bbd2bf4 
solana-labs#33576
stops broadcasting legacy snapshot hashes over gossip, and this commit
removes unused legacy snapshot hashed code in gossip.
mergify bot pushed a commit that referenced this pull request Oct 9, 2023
@behzadnouri
removes unused legacy-snapshot-hashes api in gossip (#33593)
1d91b60 
#33576
stops broadcasting legacy snapshot hashes over gossip, and this commit
removes unused legacy snapshot hashed code in gossip.
brooksprumo added a commit that referenced this pull request Oct 10, 2023
@mergify @brooksprumo
v1.17: Stops pushing legacy snapshot hashes to crds (backport of #33576…
c45c9dc 
…) (#33581)

(cherry picked from commit c588f25)

Co-authored-by: Brooks <brooks@solana.com>
@HaoranYi HaoranYi mentioned this pull request Apr 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bw-solana bw-solana bw-solana approved these changes

Assignees

@brooksprumo brooksprumo

Labels
v1.17 PRs that should be backported to v1.17
Projects
No open projects
Better Snapshot Bootstrap (BSB)
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stop pushing to LegacySnapshotHashes
3 participants
@brooksprumo @bw-solana @solana-grimes