blockstore: populate duplicate shred proofs for merkle root conflicts #34270

AshwinSekar · 2023-11-29T17:28:09Z

Problem

We only construct duplicate proofs for conflicting shreds, LAST_SHRED_IN_SLOT, and erasure meta conflicts.

Summary of Changes

When receiving a new shred check to see if there is a merkle root conflict from an existing merkle root meta. If there is such a conflict, construct a duplicate proof and notify gossip and the duplicate block state machine.

Split from #33889
Fixes #33644

codecov · 2023-11-29T18:52:42Z

Codecov Report

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

Comparison is base (59dd007) 81.8% compared to head (0c1f848) 81.8%.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #34270    +/-   ##
========================================
  Coverage    81.8%    81.8%            
========================================
  Files         824      824            
  Lines      222261   222327    +66     
========================================
+ Hits       181887   182009   +122     
+ Misses      40374    40318    -56

AshwinSekar · 2023-11-30T22:39:26Z

Only last commit is relevant, builds on top of #34268 and #34269

AshwinSekar · 2023-11-30T23:02:19Z

I have removed the refactor portion of this pr. This can be merged independently of #34268 and #34269

ledger/src/blockstore.rs

behzadnouri · 2023-12-01T22:41:21Z

ledger/src/blockstore.rs

        if let Some(shred) = just_inserted_shreds.get(&key) {
            Cow::Borrowed(shred.payload())
-        } else {
+        } else if shred_type == ShredType::Data {


Can we use a match expression here?
This code is not safe if ShredType enum is extended with a new variant. (unlikely that it happens but no reason to leave this foot gun here).

behzadnouri · 2023-12-01T22:44:43Z

ledger/src/blockstore.rs

+                self.get_coding_shred(slot, u64::from(index))
+                    .unwrap()
+                    .unwrap_or_else(|| {
+                        panic!("{} {} {:?} must be present!", slot, index, shred_type)


What ensures that the shred is present here?
This is a pretty risky method that internally relies on certain presumptions where the caller may not be aware of.
I do see the logic was already present before this commit, but the commit is extending the uses of this method.

Nothing. This method is used for cases where a meta structure has already informed you of the shreds existence, where it is preferable to panic if there is an inconsistency.

However if you prefer I can return an option, and have the callers error! and continue instead.

yeah, I think returning an Option is better here. Maybe in a separate pr first if that is changing too much of existing code.
this function as it stands is a pretty risky code.

behzadnouri · 2023-12-01T22:47:18Z

ledger/src/blockstore.rs

        slot: Slot,
-        index: u64,
+        index: u32,
+        shred_type: ShredType,


can we just take a ShredId instead of these 3 arguments?

behzadnouri · 2024-01-02T12:57:07Z

core/src/window_service.rs

+        let send_merkle_root_conflicts = cluster_nodes::check_feature_activation(
+            &feature_set::merkle_conflict_duplicate_proofs::id(),


This is using two different names to refer to the same thing,

send_merkle_root_conflicts merkle_conflict_duplicate_proofs

Can we use the same name for the feature and the flag, so that it is easier to find all the instances?

behzadnouri · 2024-01-02T12:59:12Z

core/src/window_service.rs

+                    // Although this proof can be immediately stored on detection, we wait until
+                    // here in order to check the feature flag, as storage in blockstore can
+                    // preclude the detection of other duplicate proofs in this slot


Why blockstore doesn't check the feature flag before sending this?

behzadnouri · 2024-01-02T13:02:04Z

ledger/src/blockstore.rs

+                // A previous shred has been inserted in this batch or in blockstore
+                // Compare our current shred against the previous shred for potential
+                // conflicts
+                if !self.perform_merkle_check(


perform_merkle_check is a pretty confusing name and error prone.
Like by no means it is clear if false is good or bad.

behzadnouri · 2024-01-02T13:05:58Z

ledger/src/blockstore.rs

+                self.get_coding_shred(slot, u64::from(index))
+                    .unwrap()
+                    .unwrap_or_else(|| {
+                        panic!("{} {} {:?} must be present!", slot, index, shred_type)


yeah, I think returning an Option is better here. Maybe in a separate pr first if that is changing too much of existing code.
this function as it stands is a pretty risky code.

behzadnouri · 2024-01-02T13:07:13Z

ledger/src/blockstore.rs

+    /// Returns true if there is no merkle root conflict between
+    /// the existing `merkle_root_meta` and `shred`
+    ///
+    /// Otherwise return false and if not already present, add duplicate proof to
+    /// `duplicate_shreds`.
+    fn perform_merkle_check(


need a better name to be more clear what true or false means at the call site without having to look up this comment. otherwise the usage will be very error prone.

behzadnouri · 2024-01-02T13:09:05Z

ledger/src/blockstore.rs

+            merkle_root_meta.merkle_root(),
+            merkle_root_meta.first_received_shred_index(),
+            merkle_root_meta.first_received_shred_type(),


can we just {:?} the merkle_root_meta instead of these 3 lines?

behzadnouri · 2024-01-02T13:12:50Z

ledger/src/blockstore.rs

+        shred: &Shred,
+        duplicate_shreds: &mut Vec<PossibleDuplicateShred>,
+    ) -> bool {
+        let new_merkle_root = shred.merkle_root().ok();


Why don't we check for the feature_set::merkle_conflict_duplicate_proofs feature activation here in this function?

I also considered this as an option - since this is code is temporary until the feature flag is turned on I went with this approach to keep the PR small.

There are 183 calls (mostly all testing code) for blockstore.insert_shreds that we would need to be altered and then removed on feature flag cleanup.

I figured it would be easiest to check the feature flag in window_service and once the feature flag has been activated move the blockstore.store_duplicate_slot from window_service to blockstore.insert_shreds

fine, if there is no correctness concern.
e.g. when blockstore returns a duplicate proof it will stop processing there and ignores other variants of duplicate proofs.
But if window-service ignores that because the feature is not activated, then it is possible that the other variants of duplicates proofs were also present but not processed.

since we don't insert into blockstore until window service, other variants of duplicate proofs will not be precluded from being processed if the feature flag is not present.

additionally because they are separate enum variants, multiple signals can be sent to window service if a block violates multiple duplicate conditions.

AshwinSekar self-assigned this Nov 29, 2023

AshwinSekar force-pushed the merkle-root-dup-proof branch from f1d368d to 58802c3 Compare November 30, 2023 22:38

AshwinSekar marked this pull request as ready for review November 30, 2023 22:39

AshwinSekar force-pushed the merkle-root-dup-proof branch from 58802c3 to e9a174a Compare November 30, 2023 23:01

AshwinSekar requested review from wen-coding and behzadnouri November 30, 2023 23:02

wen-coding reviewed Nov 30, 2023

View reviewed changes

ledger/src/blockstore.rs Show resolved Hide resolved

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

ledger/src/blockstore.rs Show resolved Hide resolved

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

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

AshwinSekar force-pushed the merkle-root-dup-proof branch from e9a174a to 5b986db Compare December 1, 2023 02:40

AshwinSekar requested a review from t-nelson December 1, 2023 03:19

AshwinSekar force-pushed the merkle-root-dup-proof branch from 82ac70c to c0e3f85 Compare December 1, 2023 03:38

wen-coding previously approved these changes Dec 1, 2023

View reviewed changes

behzadnouri reviewed Dec 1, 2023

View reviewed changes

AshwinSekar dismissed wen-coding’s stale review via 169f8ce December 6, 2023 17:14

AshwinSekar force-pushed the merkle-root-dup-proof branch 2 times, most recently from 169f8ce to 38a4e8d Compare December 6, 2023 17:28

AshwinSekar requested a review from behzadnouri December 6, 2023 18:00

AshwinSekar marked this pull request as draft December 7, 2023 18:08

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 22, 2023

AshwinSekar force-pushed the merkle-root-dup-proof branch 2 times, most recently from 4e73705 to 0c05052 Compare December 23, 2023 00:23

AshwinSekar removed the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 23, 2023

AshwinSekar force-pushed the merkle-root-dup-proof branch from 0c05052 to ffc9039 Compare December 23, 2023 00:26

AshwinSekar added the feature-gate Pull Request adds or modifies a runtime feature gate label Dec 23, 2023

AshwinSekar marked this pull request as ready for review December 23, 2023 03:17

behzadnouri reviewed Jan 2, 2024

View reviewed changes

AshwinSekar added 7 commits January 3, 2024 03:04

blockstore: populate duplicate shred proofs for merkle root conflicts

aabcb94

pr feedback: check test case

ce7ac2f

pr feedback: comment

10e79c8

pr feedback: match statement, shred_id, comment

69db736

add feature flag

1210a01

pr feedback: rename ff var and perform_merkle_check

2a5a575

pr feedback: move panic to callers in get_shred_from_just_inserted_or_db

bf6fea9

AshwinSekar force-pushed the merkle-root-dup-proof branch from ffc9039 to bf6fea9 Compare January 3, 2024 03:04

avoid unecessary write if proof is already present

0c1f848

AshwinSekar requested a review from behzadnouri January 3, 2024 03:35

behzadnouri approved these changes Jan 3, 2024

View reviewed changes

AshwinSekar merged commit 1908841 into solana-labs:master Jan 3, 2024
45 checks passed

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 duplicate shred proofs for merkle root conflicts #34270

blockstore: populate duplicate shred proofs for merkle root conflicts #34270

AshwinSekar commented Nov 29, 2023

codecov bot commented Nov 29, 2023 •

edited

Loading

AshwinSekar commented Nov 30, 2023

AshwinSekar commented Nov 30, 2023

behzadnouri Dec 1, 2023

behzadnouri Dec 1, 2023

AshwinSekar Dec 6, 2023

behzadnouri Jan 2, 2024

behzadnouri Dec 1, 2023

behzadnouri Jan 2, 2024

behzadnouri Jan 2, 2024

behzadnouri Jan 2, 2024

behzadnouri Jan 2, 2024

behzadnouri Jan 2, 2024

behzadnouri Jan 2, 2024

behzadnouri Jan 2, 2024

AshwinSekar Jan 3, 2024

behzadnouri Jan 3, 2024

AshwinSekar Jan 3, 2024

		let send_merkle_root_conflicts = cluster_nodes::check_feature_activation(
		&feature_set::merkle_conflict_duplicate_proofs::id(),

blockstore: populate duplicate shred proofs for merkle root conflicts #34270

blockstore: populate duplicate shred proofs for merkle root conflicts #34270

Conversation

AshwinSekar commented Nov 29, 2023

Problem

Summary of Changes

codecov bot commented Nov 29, 2023 • edited Loading

Codecov Report

AshwinSekar commented Nov 30, 2023

AshwinSekar commented Nov 30, 2023

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

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 29, 2023 •

edited

Loading