blockstore: populate merkle root metas column #34097

AshwinSekar · 2023-11-15T21:28:59Z

Problem

We don't yet write to this column.

Summary of Changes

When inserting the first shred for each fec set, populate the merkle root metas
Note: we must only create the merkle root meta if the shred was successfully inserted, otherwise there will be a desync on first_received_shred_index and what's actually available in blockstore.

Split from #33889
Contributes to #33644

codecov · 2023-11-15T22:36:38Z

Codecov Report

Merging #34097 (1b6c66e) into master (6a5b8e8) will increase coverage by 0.0%.
Report is 1 commits behind head on master.
The diff coverage is 99.0%.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #34097    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         812      812            
  Lines      219656   219864   +208     
========================================
+ Hits       179989   180238   +249     
+ Misses      39667    39626    -41

behzadnouri · 2023-11-20T23:40:34Z

ledger/src/blockstore.rs

+        for (erasure_set, merkle_root_meta) in merkle_root_metas {
+            write_batch.put::<cf::MerkleRootMeta>(erasure_set.key(), &merkle_root_meta)?;
+        }


doesn't this overwrite existing merkle-root meta in blockstore?
it seems like at least some entries will be re-written with the same value.

you're right, i just c/p the erasure_metas logic, but I think we can be smarter and keep track of which entries are dirty and need to be written.

behzadnouri · 2023-11-20T23:41:34Z

ledger/src/blockstore.rs

+        let erasure_set = shred.erasure_set();
+
+        if let HashMapEntry::Vacant(entry) = merkle_root_metas.entry(erasure_set) {
+            if let Some(meta) = self.merkle_root_meta(erasure_set).unwrap() {
+                entry.insert(meta);
+            }
+        }


I don't understand why merkle-root meta is read from the blockstore.
shouldn't it be obtained from the current shred?

the idea here is to load the merkle entry we already have in blockstore into the in memory map for comparison in the future pr

behzadnouri · 2023-11-20T23:45:10Z

ledger/src/blockstore.rs

+            merkle_root_metas
+                .entry(erasure_set)
+                .or_insert(MerkleRootMeta::from_shred(&shred));


Why do we need the code in L1204 then?

I guess follow up commits will compare if the merkle-roots are the same.
but then I guess we could read the blockstore-db here instead of pre-emptively fetching it but then not using it if the shred is not inserted.

so the idea is:

L1204 - load merkle root meta from blockstore -> in memory map
(future commit) L1196 - compare with the current merkle shred meta for conflict
(here) L1287 - if the in memory map is empty and the insert has succeeded, add the current merkle shred meta to the in memory map
L1024 - commit in memory map to blockstore

but then I guess we could read the blockstore-db here instead of pre-emptively fetching it but then not using it if the shred is not inserted.

this is possible, but it would result in 2 blockstore reads, one for the future comparison, and then one to decide whether to insert the newly received merkle root meta.

behzadnouri · 2023-11-20T23:48:49Z

ledger/src/shred.rs

+    pub(crate) fn key(&self) -> (Slot, /*fec_set_index:*/ u32) {
+        (self.0, self.1)
+    }


oh, this key vs store_key is pretty bad and confusing.
Can you please leave a comment where one should use each?

Maybe we should remove the (_, u64) version, and do the type cast only at the call-sites which use that.

yeah you're right i'll remove/deprecate the old store_key and only use the u32 version.

for posterity this is being fixed here: #34268

AshwinSekar

will update tomorrow to distinguish dirty entries in merkle_root_metas and deprecate the u32 version of store_key

AshwinSekar · 2023-11-21T06:34:44Z

ledger/src/blockstore.rs

+        for (erasure_set, merkle_root_meta) in merkle_root_metas {
+            write_batch.put::<cf::MerkleRootMeta>(erasure_set.key(), &merkle_root_meta)?;
+        }


you're right, i just c/p the erasure_metas logic, but I think we can be smarter and keep track of which entries are dirty and need to be written.

AshwinSekar · 2023-11-21T06:36:32Z

ledger/src/blockstore.rs

+        let erasure_set = shred.erasure_set();
+
+        if let HashMapEntry::Vacant(entry) = merkle_root_metas.entry(erasure_set) {
+            if let Some(meta) = self.merkle_root_meta(erasure_set).unwrap() {
+                entry.insert(meta);
+            }
+        }


the idea here is to load the merkle entry we already have in blockstore into the in memory map for comparison in the future pr

AshwinSekar · 2023-11-21T06:42:25Z

ledger/src/blockstore.rs

+            merkle_root_metas
+                .entry(erasure_set)
+                .or_insert(MerkleRootMeta::from_shred(&shred));


so the idea is:

L1204 - load merkle root meta from blockstore -> in memory map
(future commit) L1196 - compare with the current merkle shred meta for conflict
(here) L1287 - if the in memory map is empty and the insert has succeeded, add the current merkle shred meta to the in memory map
L1024 - commit in memory map to blockstore

AshwinSekar · 2023-11-21T06:43:38Z

ledger/src/shred.rs

+    pub(crate) fn key(&self) -> (Slot, /*fec_set_index:*/ u32) {
+        (self.0, self.1)
+    }


yeah you're right i'll remove/deprecate the old store_key and only use the u32 version.

AshwinSekar · 2023-11-21T06:45:21Z

ledger/src/blockstore.rs

+            merkle_root_metas
+                .entry(erasure_set)
+                .or_insert(MerkleRootMeta::from_shred(&shred));


but then I guess we could read the blockstore-db here instead of pre-emptively fetching it but then not using it if the shred is not inserted.

this is possible, but it would result in 2 blockstore reads, one for the future comparison, and then one to decide whether to insert the newly received merkle root meta.

behzadnouri

lgtm, assuming the leftover issues will be addressed in follow up commits.

mergify · 2023-12-07T16:58:28Z

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.

(cherry picked from commit e1165aa)

blockstore: populate merkle root metas column

1b6c66e

AshwinSekar requested review from behzadnouri and steviez November 15, 2023 21:40

behzadnouri reviewed Nov 20, 2023

View reviewed changes

AshwinSekar commented Nov 21, 2023

View reviewed changes

behzadnouri approved these changes Nov 29, 2023

View reviewed changes

AshwinSekar merged commit e1165aa into solana-labs:master Nov 29, 2023
32 checks passed

AshwinSekar deleted the blockstore-merkle-meta-write branch November 29, 2023 16:39

AshwinSekar self-assigned this Nov 29, 2023

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

AshwinSekar added the v1.17 PRs that should be backported to v1.17 label Dec 7, 2023

mergify bot pushed a commit that referenced this pull request Dec 7, 2023

blockstore: populate merkle root metas column (#34097)

751a7bb

(cherry picked from commit e1165aa)

mergify bot mentioned this pull request Dec 7, 2023

v1.17: blockstore: populate merkle root metas column (backport of #34097) #34358

Closed

willhickey mentioned this pull request Mar 28, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blockstore: populate merkle root metas column #34097

blockstore: populate merkle root metas column #34097

AshwinSekar commented Nov 15, 2023 •

edited

Loading

codecov bot commented Nov 15, 2023

behzadnouri Nov 20, 2023

AshwinSekar Nov 21, 2023

behzadnouri Nov 20, 2023

AshwinSekar Nov 21, 2023

behzadnouri Nov 20, 2023

AshwinSekar Nov 21, 2023

AshwinSekar Nov 21, 2023

behzadnouri Nov 20, 2023

AshwinSekar Nov 21, 2023

AshwinSekar Nov 29, 2023

AshwinSekar left a comment

AshwinSekar Nov 21, 2023

AshwinSekar Nov 21, 2023

AshwinSekar Nov 21, 2023

AshwinSekar Nov 21, 2023

AshwinSekar Nov 21, 2023

behzadnouri left a comment

mergify bot commented Dec 7, 2023

blockstore: populate merkle root metas column #34097

blockstore: populate merkle root metas column #34097

Conversation

AshwinSekar commented Nov 15, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Nov 15, 2023

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AshwinSekar 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

behzadnouri left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 7, 2023

AshwinSekar commented Nov 15, 2023 •

edited

Loading