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

chains Merkle shreds in fail-entry-verification broadcast #35060

Conversation

behzadnouri
Copy link
Contributor

@behzadnouri behzadnouri commented Feb 2, 2024

Problem

Chain Merkle shreds in turbine/src/broadcast_stage/fail_entry_verification_broadcast_run.rs.

Summary of Changes

Chained Merkle shreds in fail-entry-verification broadcast.

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

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

Comparison is base (adc9da5) 81.6% compared to head (8bc59cd) 81.6%.
Report is 1 commits behind head on master.

❗ Current head 8bc59cd differs from pull request most recent head 6dc8ecb. Consider uploading reports for the commit 6dc8ecb to get more accurate results

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #35060     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         830      830             
  Lines      224952   225053    +101     
=========================================
+ Hits       183698   183743     +45     
- Misses      41254    41310     +56     

@behzadnouri behzadnouri force-pushed the chained-merkle-root-faile-entry-verification branch 4 times, most recently from 96536a9 to 8bc59cd Compare February 5, 2024 16:57
carllin
carllin previously approved these changes Feb 6, 2024
Comment on lines +109 to +112
debug_assert_eq!(slot, 0u64);
return Ok(Hash::default());
}
debug_assert!(parent < slot, "parent: {parent} >= slot: {slot}");
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 only make these debug?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that if the this does not fail in tests then we should be good.
So the extra panic code in release builds is unnecessary.

@@ -96,6 +99,34 @@ pub(super) fn recv_slot_entries(receiver: &Receiver<WorkingBankEntry>) -> Result
})
}

// Returns the Merkle root of the last erasure batch of the parent slot.
pub(super) fn get_chained_merkle_root(
Copy link
Contributor

Choose a reason for hiding this comment

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

this function might be more intuitive if it was named get_chained_merkle_root_from_parent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

bw-solana
bw-solana previously approved these changes Feb 6, 2024
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.

Similar feedback as #35061 . LGTM, but could probably have some more helpful error messages for failed unwraps

@behzadnouri behzadnouri dismissed stale reviews from bw-solana and carllin via 6dc8ecb February 6, 2024 17:56
@behzadnouri behzadnouri force-pushed the chained-merkle-root-faile-entry-verification branch from 8bc59cd to 6dc8ecb Compare February 6, 2024 17:56
@behzadnouri behzadnouri merged commit 8fb389f into solana-labs:master Feb 6, 2024
35 checks passed
@behzadnouri behzadnouri deleted the chained-merkle-root-faile-entry-verification branch February 6, 2024 20:04
@behzadnouri behzadnouri added the v1.18 PRs that should be backported to v1.18 label Feb 21, 2024
Copy link
Contributor

mergify bot commented Feb 21, 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 21, 2024
The commit migrates
    turbine/src/broadcast_stage/fail_entry_verification_broadcast_run.rs
to use chained Merkle shreds variant.

(cherry picked from commit 8fb389f)
mergify bot added a commit that referenced this pull request Feb 22, 2024
…kport of #35060) (#35270)

chains Merkle shreds in fail-entry-verification broadcast (#35060)

The commit migrates
    turbine/src/broadcast_stage/fail_entry_verification_broadcast_run.rs
to use chained Merkle shreds variant.

(cherry picked from commit 8fb389f)

Co-authored-by: behzad nouri <[email protected]>
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