shred: expose merkle root for use in blockstore #34063

AshwinSekar · 2023-11-14T20:46:01Z

Problem

We need to access the merkle_root for merkle shreds in blockstore.

Summary of Changes

Dispatch to it, return None for legacy shreds.

Split from #33889
Contributes to #33644

behzadnouri · 2023-11-14T21:08:49Z

ledger/src/blockstore_meta.rs

+impl MerkleRootMeta {
+    pub(crate) fn from_shred(shred: &Shred) -> Self {
+        Self {
+            merkle_root: shred.merkle_root().unwrap_or_default(),


Now I am thinking that MerkleRootMeta.merkle_root should have been Option<Hash> to explicitly distinguish legacy shreds.
This unwrap_or_default here is a bit hacky, and it seems like we will have to special case merkle_root_meta.merkle_root == Hash::default() later.

definitely possible, however I was under the impression that legacy shreds are no longer being broadcasted.
I assume at some point you will delete shred/legacy.rs at which point I can remove unwrap_or_default() and any Hash::default() checks, and return Option<Self> for legitimate errors

They are no longer broadcasted but still totally valid to send legacy shreds.
We might also need to keep shred/legacy.rs for backward compatibility.
I am not sure for how long we will need to preserve legacy shreds, so I am leaning to use Option<Hash> to be consistent with current state of the code.

Got it. Is it supported to send a mixture of legacy and merkle shreds in a single FEC set?
If not I think we can just avoid storing the merkle root meta for legacy FEC sets. There's no benefit to inserting the meta into blockstore if the FEC set only has legacy shreds. For the next pr that writes this column, I can filter out FEC sets that are legacy.

no, but if you receive two shreds with the same fec_set_index and one is merkle and the other legacy, then that is a proof of duplicate block.
So I think we need to record that we have received a legacy shred for this fec_set_index, and I guess the easiest way to do that is to set merkle_root to None

makes sense i'll do that first.

behzadnouri · 2023-11-14T21:10:41Z

ledger/src/shred.rs

@@ -337,6 +337,8 @@ impl Shred {
    dispatch!(pub fn payload(&self) -> &Vec<u8>);
    dispatch!(pub fn sanitize(&self) -> Result<(), Error>);

+    dispatch!(pub fn merkle_root(&self) -> Option<Hash>);


Can you please keep lines 336-340 alphabetically sorted?
also, the blank like 339 is not needed.

also, the return type should be Result<Hash, Error> to preserve the error.

behzadnouri · 2023-11-14T21:12:51Z

ledger/src/shred/shred_code.rs

@@ -47,6 +47,13 @@ impl ShredCode {
        }
    }

+    pub(super) fn merkle_root(&self) -> Option<Hash> {


Can we return -> Result<Hash, Error> to preserve the error?
Self::Legacy(_) case would then return Err(Error::InvalidShredType).

behzadnouri · 2023-11-14T21:13:30Z

ledger/src/shred/shred_data.rs

+    pub(super) fn merkle_root(&self) -> Option<Hash> {
+        match self {
+            Self::Legacy(_) => None,


same here, about preserving the Error.

codecov · 2023-11-14T22:35:37Z

Codecov Report

Merging #34063 (28b6626) into master (7ba9fbc) will increase coverage by 0.0%.
Report is 1333 commits behind head on master.
The diff coverage is 91.1%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #34063      +/-   ##
==========================================
  Coverage    81.8%    81.9%              
==========================================
  Files         766      812      +46     
  Lines      209154   219707   +10553     
==========================================
+ Hits       171206   180014    +8808     
- Misses      37948    39693    +1745

behzadnouri

lgtm, but can we first update MerkleRootMeta.merkle_root to an explicit Option<Hash>.
that might need to be done in a separate PR because that needs to be backported.

behzadnouri · 2023-11-15T19:15:30Z

ledger/src/blockstore_meta.rs

+            // legacy or malformed shreds. We should still store
+            // `None` for those cases in blockstore, as a later
+            // shred that contains a proper merkle root would constitute
+            // a valid duplicate shred proof.


duplicate "block" proof, no?
we might not actually observe any duplicate shreds.

not sure about the terminology. We're still calling them duplicate shred proofs in gossip even though it has evolved beyond shreds with the same index. If you prefer I can do a separate refactor PR afterwards renaming all instances of "duplicate shred proof" to "duplicate block proof"?

never mind then,
we can revisit once the code is shipped and stable.

mergify · 2023-11-30T23:30:11Z

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.

* shred: expose merkle root for use in blockstore * pr feedback: sorted, keep Result return type * convert Result<Hash> -> Option<Hash> (cherry picked from commit 6a5b8e8)

…34063) (#34291) shred: expose merkle root for use in blockstore (#34063) * shred: expose merkle root for use in blockstore * pr feedback: sorted, keep Result return type * convert Result<Hash> -> Option<Hash> (cherry picked from commit 6a5b8e8) Co-authored-by: Ashwin Sekar <[email protected]>

AshwinSekar requested a review from behzadnouri November 14, 2023 20:46

AshwinSekar force-pushed the merkle-root-getter branch 2 times, most recently from 0a32372 to b741505 Compare November 14, 2023 20:47

behzadnouri reviewed Nov 14, 2023

View reviewed changes

AshwinSekar requested a review from behzadnouri November 14, 2023 21:57

behzadnouri reviewed Nov 14, 2023

View reviewed changes

AshwinSekar added 3 commits November 15, 2023 19:07

shred: expose merkle root for use in blockstore

a099979

pr feedback: sorted, keep Result return type

d1acab9

convert Result<Hash> -> Option<Hash>

28b6626

AshwinSekar force-pushed the merkle-root-getter branch from cfa7bc1 to 28b6626 Compare November 15, 2023 19:07

behzadnouri approved these changes Nov 15, 2023

View reviewed changes

AshwinSekar added the automerge Merge this Pull Request automatically once CI passes label Nov 15, 2023

mergify bot merged commit 6a5b8e8 into solana-labs:master Nov 15, 2023

AshwinSekar deleted the merkle-root-getter branch November 15, 2023 20:13

AshwinSekar self-assigned this Nov 29, 2023

AshwinSekar requested a review from t-nelson November 30, 2023 23:29

AshwinSekar added the v1.17 PRs that should be backported to v1.17 label Nov 30, 2023

mergify bot mentioned this pull request Nov 30, 2023

v1.17: shred: expose merkle root for use in blockstore (backport of #34063) #34291

Merged

HaoranYi mentioned this pull request Apr 8, 2024

pr634 1.17 new filter anza-xyz/agave#657

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shred: expose merkle root for use in blockstore #34063

shred: expose merkle root for use in blockstore #34063

AshwinSekar commented Nov 14, 2023 •

edited

Loading

behzadnouri Nov 14, 2023

AshwinSekar Nov 14, 2023 •

edited

Loading

behzadnouri Nov 14, 2023

AshwinSekar Nov 15, 2023

behzadnouri Nov 15, 2023

AshwinSekar Nov 15, 2023

behzadnouri Nov 14, 2023

behzadnouri Nov 14, 2023

behzadnouri Nov 14, 2023

codecov bot commented Nov 14, 2023 •

edited

Loading

behzadnouri left a comment

behzadnouri Nov 15, 2023

AshwinSekar Nov 15, 2023

behzadnouri Nov 15, 2023

mergify bot commented Nov 30, 2023

shred: expose merkle root for use in blockstore #34063

shred: expose merkle root for use in blockstore #34063

Conversation

AshwinSekar commented Nov 14, 2023 • edited Loading

Problem

Summary of Changes

Choose a reason for hiding this comment

AshwinSekar Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 14, 2023 • edited Loading

Codecov Report

behzadnouri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Nov 30, 2023

AshwinSekar commented Nov 14, 2023 •

edited

Loading

AshwinSekar Nov 14, 2023 •

edited

Loading

codecov bot commented Nov 14, 2023 •

edited

Loading