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

send duplicate shred proofs for merkle root conflicts #33889

Closed

Conversation

AshwinSekar
Copy link
Contributor

Problem

Data shred + coding shred that are part of the same FEC set with different merkle roots are not marked duplicate.

Summary of Changes

Detect this scenario, store the proof in blockstore and notify the cluster.

Fixes #33644

@AshwinSekar AshwinSekar force-pushed the merkle-root-dup-proof branch 5 times, most recently from c82b3a2 to d0b2c9f Compare November 7, 2023 23:15
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #33889 (196b7e1) into master (0e91e96) will increase coverage by 0.0%.
Report is 2 commits behind head on master.
The diff coverage is 96.8%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master   #33889    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         811      811            
  Lines      219583   219972   +389     
========================================
+ Hits       179935   180295   +360     
- Misses      39648    39677    +29     

@AshwinSekar AshwinSekar marked this pull request as ready for review November 13, 2023 18:47
@@ -30,7 +31,7 @@ pub struct DuplicateShred {
pub(crate) wallclock: u64,
pub(crate) slot: Slot,
_unused: u32,
shred_type: ShredType,
_unused_shred_type: ShredType,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

now that we can have a conflict between data and coding shreds, this field is useless.

just_inserted_shreds,
slot,
last_index.unwrap(),
u32::try_from(last_index.unwrap()).unwrap(),
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 annoying, it seems in order to use u32 indices everywhere, we need to update the slot_meta to use u32 for last_index, consumed, received. then we can also update the shred columns to use u32

@behzadnouri
Copy link
Contributor

This is 800+ LOC change.
Can you please split this into separate PRs for 1) shred code changes, 2) gossip code changes, 3) blockstore changes? (or whatever other order it makes sense)

Also, would be better to separate the change writing merkle roots to blockstore from the change consuming/reading from blockstore.

@AshwinSekar
Copy link
Contributor Author

sure - it ended up being bigger than i originally thought 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Extend duplicate proofs for merkle shreds
2 participants