adds chained merkle shreds variant #34787

behzadnouri · 2024-01-15T22:18:15Z

Problem

Harden shreds spec.

Summary of Changes

With the new variants, each Merkle shred will also include the Merkle
root of the previous erasure batch.

- For easier review, I have removed all the plumbing from this commit.
- The commit only includes the changes to shred struct and the code to generate
- and recover shreds.
- The rest would be done in separate commits.

codecov · 2024-01-16T15:04:38Z

Codecov Report

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

Comparison is base (acef7ba) 0.0% compared to head (66d69c9) 81.7%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #34787       +/-   ##
===========================================
+ Coverage        0    81.7%    +81.7%     
===========================================
  Files           0      826      +826     
  Lines           0   223324   +223324     
===========================================
+ Hits            0   182461   +182461     
- Misses          0    40863    +40863

behzadnouri · 2024-01-17T23:27:28Z

ledger/src/shredder.rs

@@ -93,6 +93,7 @@ impl Shredder {
                self.version,
                self.reference_tick,
                is_last_in_slot,
+                None, // chained_merkle_root


Following commits will do the plumbing here.

for the first FEC set will we chain the merkle root from the last FEC set of the parent block?

@AshwinSekar no real objection to do so (aside from slightly more code to look that up from blockstore),
but can you clarify the motivation to do so?

essentially it gives us a unique identifier for a fork. this is useful for comparison if we add the merkle root to votes.

imagine we have some unplayed slot S in our blockstore and S gets confirmed through votes. we can easily compare S to the merkle root in the votes to verify that we have the correct version of S.

if we also chain the merkle root to the parent block, the same comparison will tell us if we have the correct version of S - 1, S -2, and such ancestors.

in the event that we have parent block P and child block C where the chaining of the first FEC set of C does not match the last FEC set of P, we can conclude that we have incompatible versions of P and C. Depending on which merkle root the votes for C or its descendants contain, we can reconcile the situation by dumping and repairing either P or C appropriately.

ledger/src/shred.rs

ledger/src/shred/merkle.rs

bw-solana

Looks pretty good to me. Lots of eyeballing that size/offset calculations are functioning as intended, so we'll have to lean on testing

steviez · 2024-01-18T17:21:09Z

ci/do-audit.sh

@@ -30,6 +30,7 @@ cargo_audit_ignores=(
  --ignore RUSTSEC-2023-0001

  --ignore RUSTSEC-2022-0093
+  --ignore RUSTSEC-2024-0003


nit: This has been addressed upstream so you can rebase and remove this

bw-solana

LGTM. Should probably wait for others to weigh in as well

carllin

Looks good!

ledger/src/shred.rs

carllin · 2024-01-19T18:20:58Z

ledger/src/shred/merkle.rs

+        let ShredVariant::MerkleCode(proof_size, chained) = self.common_header.shred_variant else {
+            return Err(Error::InvalidShredVariant);
+        };
+        Self::get_proof_offset(proof_size, chained)
+    }
+
+    fn get_proof_offset(proof_size: u8, chained: bool) -> Result<usize, Error> {
+        Ok(Self::SIZE_OF_HEADERS
+            + Self::capacity(proof_size, chained)?
+            + if chained { SIZE_OF_MERKLE_ROOT } else { 0 })
+    }
+
+    fn chained_merkle_root_offset(&self) -> Result<usize, Error> {
+        let ShredVariant::MerkleCode(proof_size, /*chained:*/ true) =
+            self.common_header.shred_variant
+        else {
            return Err(Error::InvalidShredVariant);
        };
-        Self::get_proof_offset(proof_size)
+        Ok(Self::SIZE_OF_HEADERS + Self::capacity(proof_size, /*chained:*/ true)?)
+    }
+
+    fn chained_merkle_root(&self) -> Result<Hash, Error> {
+        let offset = self.chained_merkle_root_offset()?;
+        self.payload
+            .get(offset..offset + SIZE_OF_MERKLE_ROOT)
+            .map(Hash::new)
+            .ok_or(Error::InvalidPayloadSize(self.payload.len()))
    }

-    fn get_proof_offset(proof_size: u8) -> Result<usize, Error> {
-        Ok(Self::SIZE_OF_HEADERS + Self::capacity(proof_size)?)
+    fn set_chained_merkle_root(&mut self, chained_merkle_root: &Hash) -> Result<(), Error> {
+        let offset = self.chained_merkle_root_offset()?;
+        let Some(buffer) = self.payload.get_mut(offset..offset + SIZE_OF_MERKLE_ROOT) else {
+            return Err(Error::InvalidPayloadSize(self.payload.len()));
+        };
+        buffer.copy_from_slice(chained_merkle_root.as_ref());
+        Ok(())


The worst type of code to write + review, but seems right 😃

In what sense?

the tedium of having to remember where all the offsets are haha

carllin · 2024-01-19T18:55:18Z

ledger/src/shred/merkle.rs

+        let ShredVariant::MerkleData(proof_size, chained) = self.common_header.shred_variant else {
+            return Err(Error::InvalidShredVariant);
+        };
+        let offset = Self::SIZE_OF_HEADERS + Self::capacity(proof_size, chained)?;


Is this calculation still equivalent to calling self.proof_offset()?

no. the chained merkle root is not part of the erasure coding.
so if the shred is chained, then proof_offset would be 32 bytes past the offset calculated here.

no. the chained merkle root is not part of the erasure coding

Ah yeah, thats the critical part here, thanks.

ledger/src/shred/merkle.rs

With the new variants, each Merkle shred will also include the Merkle root of the previous erasure batch.

behzadnouri force-pushed the chained-merkle-shreds-lite branch 2 times, most recently from 9c3587f to 836ede2 Compare January 16, 2024 13:35

behzadnouri force-pushed the chained-merkle-shreds-lite branch 3 times, most recently from b5d567f to 5f05e9f Compare January 17, 2024 23:21

behzadnouri marked this pull request as ready for review January 17, 2024 23:21

behzadnouri force-pushed the chained-merkle-shreds-lite branch 3 times, most recently from e15abe6 to 3b31d6c Compare January 17, 2024 23:25

behzadnouri commented Jan 17, 2024

View reviewed changes

behzadnouri requested review from steviez, AshwinSekar and bw-solana January 17, 2024 23:32

behzadnouri force-pushed the chained-merkle-shreds-lite branch from 3b31d6c to df841cc Compare January 17, 2024 23:44

behzadnouri requested a review from carllin January 18, 2024 15:29

bw-solana reviewed Jan 18, 2024

View reviewed changes

ledger/src/shred.rs Outdated Show resolved Hide resolved

bw-solana reviewed Jan 18, 2024

View reviewed changes

ledger/src/shred.rs Show resolved Hide resolved

bw-solana reviewed Jan 18, 2024

View reviewed changes

ledger/src/shred/merkle.rs Show resolved Hide resolved

bw-solana reviewed Jan 18, 2024

View reviewed changes

steviez reviewed Jan 18, 2024

View reviewed changes

behzadnouri force-pushed the chained-merkle-shreds-lite branch from df841cc to 9f44a29 Compare January 18, 2024 19:53

behzadnouri requested review from steviez and bw-solana January 18, 2024 19:53

bw-solana previously approved these changes Jan 18, 2024

View reviewed changes

carllin previously approved these changes Jan 19, 2024

View reviewed changes

adds chained merkle shreds variant

66d69c9

With the new variants, each Merkle shred will also include the Merkle root of the previous erasure batch.

behzadnouri dismissed stale reviews from carllin and bw-solana via 66d69c9 January 19, 2024 20:08

behzadnouri force-pushed the chained-merkle-shreds-lite branch from 9f44a29 to 66d69c9 Compare January 19, 2024 20:08

behzadnouri requested a review from carllin January 19, 2024 20:08

bw-solana approved these changes Jan 19, 2024

View reviewed changes

carllin approved these changes Jan 20, 2024

View reviewed changes

behzadnouri merged commit 9a520fd into solana-labs:master Jan 20, 2024
35 checks passed

behzadnouri deleted the chained-merkle-shreds-lite branch January 20, 2024 16:08

AshwinSekar mentioned this pull request Jan 23, 2024

Generate duplicate proofs for blocks with incorrectly chained merkle roots #34897

Open

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds chained merkle shreds variant #34787

adds chained merkle shreds variant #34787

behzadnouri commented Jan 15, 2024 •

edited

codecov bot commented Jan 16, 2024 •

edited

behzadnouri Jan 17, 2024

AshwinSekar Jan 23, 2024

behzadnouri Jan 27, 2024

AshwinSekar Jan 29, 2024 •

edited

bw-solana left a comment

steviez Jan 18, 2024

behzadnouri Jan 18, 2024

bw-solana left a comment

carllin left a comment

carllin Jan 19, 2024

behzadnouri Jan 19, 2024

carllin Jan 20, 2024

carllin Jan 19, 2024

behzadnouri Jan 19, 2024

carllin Jan 20, 2024 •

edited

adds chained merkle shreds variant #34787

adds chained merkle shreds variant #34787

Conversation

behzadnouri commented Jan 15, 2024 • edited

Problem

Summary of Changes

codecov bot commented Jan 16, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshwinSekar Jan 29, 2024 • edited

Choose a reason for hiding this comment

bw-solana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bw-solana left a comment

Choose a reason for hiding this comment

carllin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carllin Jan 20, 2024 • edited

Choose a reason for hiding this comment

behzadnouri commented Jan 15, 2024 •

edited

codecov bot commented Jan 16, 2024 •

edited

AshwinSekar Jan 29, 2024 •

edited

carllin Jan 20, 2024 •

edited