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.

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

adds chained_merkle_root to shredder arguments #34952

Merged

Conversation

behzadnouri
Copy link
Contributor

Problem

Working towards chaining Merkle root of erasure batches.

Summary of Changes

Added chained_merkle_root to shredder arguments

@behzadnouri behzadnouri force-pushed the chained-merkle-shreds-lite branch 2 times, most recently from 05743a3 to d3d9cc1 Compare January 25, 2024 22:20
Copy link

codecov bot commented Jan 25, 2024

Codecov Report

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

Comparison is base (5da06c5) 81.6% compared to head (f16bf3e) 81.6%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34952   +/-   ##
=======================================
  Coverage    81.6%    81.6%           
=======================================
  Files         830      830           
  Lines      224746   224797   +51     
=======================================
+ Hits       183512   183560   +48     
- Misses      41234    41237    +3     

Working towards chaining Merkle root of erasure batches, the commit adds
chained_merkle_root to shredder arguments.
@@ -85,6 +85,7 @@ impl StandardBroadcastRun {
keypair,
&[], // entries
true, // is_last_in_slot,
None, // chained_merkle_root
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maintaining last Merkle root here and elsewhere in turbine/src/broadcast_stage will be done separately in the follow up commits.

@@ -7434,7 +7444,7 @@ pub mod tests {
#[test]
fn test_insert_multiple_is_last() {
solana_logger::setup();
let (shreds, _) = make_slot_entries(0, 0, 20, /*merkle_variant:*/ true);
let (shreds, _) = make_slot_entries(0, 0, 19, /*merkle_variant:*/ true);
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to change this from 20 -> 19?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test tries to insert a shred where index > already recorded last_index.
Because of the embedded chained Merkle root, shreds have smaller capacity, so with 20 this line generates the same number of shreds as L7460.
So we won't see any shred with index > last_index when inserting the second set of shreds.

So I reduced this to 19 so that the added assertion in L7461 still holds as before.

Copy link
Contributor

@AshwinSekar AshwinSekar left a comment

Choose a reason for hiding this comment

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

lgtm, adds chained root to entries_to_shreds and updates all the testing callers, broadcast passes None for now, should have no impact.

@behzadnouri behzadnouri merged commit 79bbe43 into solana-labs:master Jan 27, 2024
35 checks passed
@behzadnouri behzadnouri deleted the chained-merkle-shreds-lite branch January 27, 2024 15:04
@behzadnouri behzadnouri added the v1.18 PRs that should be backported to v1.18 label Feb 8, 2024
Copy link
Contributor

mergify bot commented Feb 8, 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 8, 2024
Working towards chaining Merkle root of erasure batches, the commit adds
chained_merkle_root to shredder arguments.

(cherry picked from commit 79bbe43)
mergify bot added a commit that referenced this pull request Feb 9, 2024
…4952) (#35150)

adds chained_merkle_root to shredder arguments (#34952)

Working towards chaining Merkle root of erasure batches, the commit adds
chained_merkle_root to shredder arguments.

(cherry picked from commit 79bbe43)

Co-authored-by: behzad nouri <behzadnouri@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.18 PRs that should be backported to v1.18
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants