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

adds rollout path for chained Merkle shreds #35076

Merged
merged 2 commits into from
Feb 8, 2024
Merged

adds rollout path for chained Merkle shreds #35076

merged 2 commits into from
Feb 8, 2024

Conversation

behzadnouri
Copy link
Contributor

@behzadnouri behzadnouri commented Feb 4, 2024 •
edited
Loading

Problem

Roll out chained Merkle shreds

Summary of Changes

The commit adds should_chain_merkle_shreds to incrementally roll out chained Merkle shreds to clusters.

@behzadnouri behzadnouri force-pushed the chained-merkle-root-standard-broadcast-run branch 6 times, most recently from fd49d2a to bf98394 Compare February 6, 2024 18:15
@behzadnouri behzadnouri changed the title chains Merkle shreds in standard broadcast run adds rollout path to for chained Merkle shreds Feb 6, 2024
@behzadnouri behzadnouri changed the title adds rollout path to for chained Merkle shreds adds rollout path for chained Merkle shreds Feb 6, 2024
@behzadnouri behzadnouri force-pushed the chained-merkle-root-standard-broadcast-run branch 2 times, most recently from 00dcb50 to 8a01eef Compare February 7, 2024 01:01
@codecov Codecov
Copy link

codecov bot commented Feb 7, 2024 •
edited
Loading

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (9b63ac8) 81.6% compared to head (b06ec12) 81.6%.

❗ Current head b06ec12 differs from pull request most recent head e9975a1. Consider uploading reports for the commit e9975a1 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##           master   #35076   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         832      831    -1     
  Lines      224856   224893   +37     
=======================================
+ Hits       183538   183631   +93     
+ Misses      41318    41262   -56

@behzadnouri behzadnouri force-pushed the chained-merkle-root-standard-broadcast-run branch from 8a01eef to b06ec12 Compare February 7, 2024 16:23
behzadnouri
behzadnouri commented Feb 7, 2024
View reviewed changes
turbine/src/broadcast_stage/standard_broadcast_run.rs
Comment on lines +530 to +532
fn should_chain_merkle_shreds(_slot: Slot, _cluster_type: ClusterType) -> bool {
false
}
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 function will be used to incrementally roll out chained Merkle shreds starting with a small % of slots on testnet etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea that receiving nodes won't know if chained merkle shreds are present for this slot until they deserialize shreds and look?

Here's my strawman of the rollout plan - please correct:

  1. Plumbing for generating and including chained merkle shreds
  2. Start including for "non-standard" broadcast methods
  3. This PR to add single function to decide whether to chain merkle shreds for "standard" broadcast method based on slot/cluster type
  4. Logic to include for some small percentage of slots on testnet
  5. Receivers validating chained Merkle shreds when present
  6. Expand rollout to more slots/clusters
  7. Remove conditional logic
  8. Receivers unconditionally verify Merkle shreds (must be present)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is pretty much it.
We did a similar thing for the Merkle shreds rollout:
https://github.com/solana-labs/solana/pulls?q=is%3Apr+author%3Abehzadnouri+rolls+out+Merkle+is%3Aclosed+

bw-solana
bw-solana reviewed Feb 8, 2024
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 but left a couple of questions

@behzadnouri
adds rollout path for chained Merkle shreds
4522084 
The commit adds should_chain_merkle_shreds to incrementally roll out
chained Merkle shreds to clusters.
@behzadnouri
reverts tests modifications
e9975a1
@behzadnouri behzadnouri force-pushed the chained-merkle-root-standard-broadcast-run branch from b06ec12 to e9975a1 Compare February 8, 2024 18:11
@behzadnouri behzadnouri merged commit 0cfb06f into solana-labs:master Feb 8, 2024
35 checks passed
@behzadnouri behzadnouri deleted the chained-merkle-root-standard-broadcast-run branch February 8, 2024 23:06
@behzadnouri behzadnouri added the v1.18 PRs that should be backported to v1.18 label Feb 23, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 23, 2024

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

mergify bot pushed a commit that referenced this pull request Feb 23, 2024
@behzadnouri @mergify
adds rollout path for chained Merkle shreds (#35076)
df26261 
The commit adds should_chain_merkle_shreds to incrementally roll out
chained Merkle shreds to clusters.

(cherry picked from commit 0cfb06f)
@mergify mergify bot mentioned this pull request Feb 23, 2024
Merged
mergify bot added a commit that referenced this pull request Feb 25, 2024
@mergify @behzadnouri
v1.18: adds rollout path for chained Merkle shreds (backport of #35076)…
2561b25 
… (#35305)

adds rollout path for chained Merkle shreds (#35076)

The commit adds should_chain_merkle_shreds to incrementally roll out
chained Merkle shreds to clusters.

(cherry picked from commit 0cfb06f)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
@willhickey willhickey mentioned this pull request Mar 28, 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

@carllin carllin Awaiting requested review from carllin

@AshwinSekar AshwinSekar Awaiting requested review from AshwinSekar

Assignees
No one assigned
Labels
v1.18 PRs that should be backported to v1.18
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@behzadnouri @bw-solana