-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 broadcast fake shreds #35061
chains Merkle shreds in broadcast fake shreds #35061
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #35061 +/- ##
========================================
Coverage 81.6% 81.6%
========================================
Files 830 830
Lines 224947 225053 +106
========================================
+ Hits 183676 183763 +87
- Misses 41271 41290 +19 |
06bee56
to
075dedd
Compare
Some(index) => { | ||
let shred = blockstore | ||
.get_data_shred(bank.slot(), u64::from(index)) | ||
.unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use expect here?
.expect("Blockstore Error")
.expect("Shred not present")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 1st unwrap will print the BlockstoreError
if it fails.
The 2nd unwrap will print called Option::unwrap() on a None value
, indicating there was no shred.
Given that this code is only invoked in tests I think that is already sufficient here.
bank.parent_slot(), | ||
blockstore, | ||
) | ||
.unwrap(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have some nice error handling in get_chained_merkle_root
but attempt to just unwrap it here. Maybe add an expect or something? Is there any way to fail gracefully in the event we can't get the chained merkle root?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add an expect or something?
The unwrap
will actually print the error. That is why I am adding those new error types, which are descriptive of what went wrong. So not sure if expect
will add much more here. This is code is also invoked only in tests.
Is there any way to fail gracefully in the event we can't get the chained merkle root?
This code is only invoked in tests and if it fails we want to get a loud panic so to debug root cause.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me. Curious if we could do more to handle any potential error cases
075dedd
to
b7d8cee
Compare
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. |
The commit migrates turbine/src/broadcast_stage/broadcast_fake_shreds_run.rs to use chained Merkle shreds variant. (cherry picked from commit 8d0ca9d)
) (#35253) chains Merkle shreds in broadcast fake shreds (#35061) The commit migrates turbine/src/broadcast_stage/broadcast_fake_shreds_run.rs to use chained Merkle shreds variant. (cherry picked from commit 8d0ca9d) Co-authored-by: behzad nouri <[email protected]>
Problem
Use chained Merkle shreds in
turbine/src/broadcast_stage/broadcast_fake_shreds_run.rs
.Summary of Changes
Chains Merkle shreds in broadcast fake shreds.