Add feature flag for LastIndex and Erasure duplicate proofs #34360

AshwinSekar · 2023-12-07T19:53:20Z

Problem

These cases introduced in 1.17 #32965 need to be feature flagged as

Lines 172 to 173 in 74c54a7

    
           // Notify duplicate consensus state machine 
        
           duplicate_slots_sender.send(shred_slot)?;

will update fork choice.

Summary of Changes

Add feature flag to ensure that fork choice is consistent along the cluster when upgrading to v1.17.

codecov · 2023-12-07T20:57:39Z

Codecov Report

Merging #34360 (c83719d) into master (65e10ae) will increase coverage by 0.0%.
Report is 9 commits behind head on master.
The diff coverage is 93.3%.

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #34360    +/-   ##
========================================
  Coverage    81.8%    81.8%            
========================================
  Files         820      820            
  Lines      220869   221087   +218     
========================================
+ Hits       180791   180987   +196     
- Misses      40078    40100    +22

mergify · 2023-12-07T21:22:40Z

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.

behzadnouri · 2023-12-08T00:01:43Z

core/src/window_service.rs

+    feature_set: &FeatureSet,
+    epoch_schedule: &EpochSchedule,


Better to send root_bank: &Bank here instead of these 2 args.
The reason I was using feature_set and epoch_schedule in the other code was because I didn't want to keep a reference to bank in order to avoid reintroducing: #33105

behzadnouri

How about LastIndex and Erasure duplicate proofs received from other nodes from gossip?

e.g. 50% of the cluster have upgraded to v1.17, and 50% have not; and then one node which does not have this patch starts sending LastIndex and Erasure duplicate proofs over gossip. Wouldn't that cause an issue because 50% of the cluster recognize that as valid duplicate proof and the other 50% don't?!

behzadnouri · 2023-12-08T00:02:35Z

core/src/window_service.rs

 fn run_check_duplicate(
    cluster_info: &ClusterInfo,
    blockstore: &Blockstore,
    shred_receiver: &Receiver<PossibleDuplicateShred>,
    duplicate_slots_sender: &DuplicateSlotSender,
+    bank_forks: &Arc<RwLock<BankForks>>,


&Arc is redundant. Either just & or if you really need Arc then just Arc.

AshwinSekar · 2023-12-08T00:12:03Z

How about LastIndex and Erasure duplicate proofs received from other nodes from gossip?

e.g. 50% of the cluster have upgraded to v1.17, and 50% have not; and then one node which does not have this patch starts sending LastIndex and Erasure duplicate proofs over gossip. Wouldn't that cause an issue because 50% of the cluster recognize that as valid duplicate proof and the other 50% don't?!

As stated here #34292 (comment) we don't consume duplicate proofs from gossip in fork choice. #32963 is the change which accomplishes that, and is planned to be enabled alongside the new vote tx change.

behzadnouri

This seems like feature gating the consensus part, but I am wondering if we should feature gate earlier in the pipeline in the ingestion/blockstore code which identifies these duplicates.

Like the new code wouldn't store the shreds. Wouldn't that cause issues if the duplicate is confirmed and the node has go with it anyways?

behzadnouri · 2023-12-10T03:09:13Z

core/src/window_service.rs

@@ -137,17 +141,50 @@ impl WindowServiceMetrics {
    }
 }

+fn should_send_index_and_erasure_conflicts(shred_slot: Slot, root_bank: &Arc<Bank>) -> bool {


This should be just &Bank. not &Arc<Bank>.

behzadnouri · 2023-12-10T03:12:19Z

core/src/window_service.rs

+    match root_bank
+        .feature_set
+        .activated_slot(&feature_set::index_erasure_conflict_duplicate_proofs::id())
+    {
+        None => false,
+        Some(feature_slot) => {
+            let epoch_schedule = root_bank.epoch_schedule();
+            let feature_epoch = epoch_schedule.get_epoch(feature_slot);
+            let shred_epoch = epoch_schedule.get_epoch(shred_slot);
+            // Has a 1 epoch delay, as we don't have enough information
+            // on the epoch boundary of the feature activation
+            feature_epoch < shred_epoch
+        }
+    }
+}


You can make this function:
https://github.com/solana-labs/solana/blob/2971e84ec/turbine/src/cluster_nodes.rs#L514-L526
and reuse it here.

behzadnouri · 2023-12-10T03:14:06Z

core/src/window_service.rs

 ) -> Result<()> {
+    let mut root_bank = bank_forks.read().unwrap().root_bank().clone();


The clone is redundant:
https://github.com/solana-labs/solana/blob/2971e84ec/runtime/src/bank_forks.rs#L166

behzadnouri · 2023-12-10T03:16:17Z

core/src/window_service.rs

+        if last_updated.elapsed().as_millis() as u64 > DEFAULT_MS_PER_SLOT {
+            // Grabs bank forks lock once a slot
+            last_updated = Instant::now();
+            root_bank = bank_forks.read().unwrap().root_bank().clone();


clone is redundant

behzadnouri · 2023-12-10T03:22:31Z

core/src/window_service.rs

+                if send_index_and_erasure_conflicts {
+                    (shred, conflict)
+                } else {
+                    return Ok(());
+                }


Shouldn't we actually feature-gate earlier in the blockstore code which identifies these conflicts?

behzadnouri · 2023-12-10T03:42:54Z

This seems like feature gating the consensus part, but I am wondering if we should feature gate earlier in the pipeline in the ingestion/blockstore code which identifies these duplicates.

Like the new code wouldn't store the shreds. Wouldn't that cause issues if the duplicate is confirmed and the node has go with it anyways?

One other issue is that I believe if blockstore already has a duplicate/conflict for a slot, future duplicates/conflicts are not processed the same ways as before (like they are not inserted into blockstore anymore, not sure if they are still channeled to replay or not).
So seems like if the blockstore code is not feature gated there is already some change of behavior. Not sure if it impacts consensus or not.

AshwinSekar · 2023-12-11T16:26:06Z

from a consensus perspective it doesn't matter which scenario a duplicate block proof is constructed for. they all result in the slot being marked as invalid in fork choice.

One other issue is that I believe if blockstore already has a duplicate/conflict for a slot, future duplicates/conflicts are not processed the same ways as before (like they are not inserted into blockstore anymore, not sure if they are still channeled to replay or not).

this is correct, if there already is a duplicate proof no further action will be taken. replay/gossip will not be notified.
however the code for detecting last index and erasure conflict in blockstore has been around for a long time #13992, so I don't believe it needs to be feature gated.

AshwinSekar · 2023-12-11T17:44:38Z

I believe #29227 should have been feature gated for the scenario you're describing.

Receive duplicate proof from gossip -> store in blockstore -> no notification to replay.
Later we receive shreds for the slot and try to construct the duplicate proof by ourselves but there is already a duplicate proof in blockstore, so no action is taken.

It seems #29227 was already in 1.16, so I believe it is too late to feature gate it.

behzadnouri · 2023-12-13T07:07:22Z

this is correct, if there already is a duplicate proof no further action will be taken. replay/gossip will not be notified.
however the code for detecting last index and erasure conflict in blockstore has been around for a long time #13992, so I don't believe it needs to be feature gated.

does this mean in current v1.16 code if it sees a last index or erasure conflict it will not mark the slot as dead but also will no longer recognize any kind of duplicates in the block?

separately, didn't you submit a change which would extend erasure conflicts detection?

AshwinSekar · 2023-12-13T16:30:58Z

does this mean in current v1.16 code if it sees a last index or erasure conflict it will not mark the slot as dead but also will no longer recognize any kind of duplicates in the block?

yes that's correct. We will add the proof to the blockstore column but not invoke handle_duplicate, so the results of the column are unused:

solana/ledger/src/blockstore.rs

Lines 1212 to 1214 in 85e3058

    
           // TODO: handle_duplicate is not invoked and so duplicate shreds are 
        
           // not gossiped to the rest of cluster. 
        
           if !erasure_meta.check_coding_shred(&shred) {

however because we have stored it in the column

solana/ledger/src/blockstore.rs

Lines 1537 to 1540 in 85e3058

    
           if self 
        
               .store_duplicate_if_not_existing( 
        
                   slot, 
        
                   ending_shred.into_owned(),

for any future simple duplicate shred case (2 shreds with the same index), we will no longer be able to take any action, as there is already a proof in the column:

solana/core/src/window_service.rs

Lines 142 to 157 in 85e3058

    
           let check_duplicate = |shred: Shred| -> Result<()> { 
        
               let shred_slot = shred.slot(); 
        
               if !blockstore.has_duplicate_shreds_in_slot(shred_slot) { 
        
                   if let Some(existing_shred_payload) = 
        
                       blockstore.is_shred_duplicate(shred.id(), shred.payload().clone()) 
        
                   { 
        
                       cluster_info.push_duplicate_shred(&shred, &existing_shred_payload)?; 
        
                       blockstore.store_duplicate_slot( 
        
                           shred_slot, 
        
                           existing_shred_payload, 
        
                           shred.into_payload(), 
        
                       )?; 
        
                       duplicate_slots_sender.send(shred_slot)?; 
        
                   } 
        
               }

AshwinSekar · 2023-12-13T16:33:47Z

separately, didn't you submit a change which would extend erasure conflicts detection?

If you're referring to #33037, it is still in development.

* Add feature flag for LastIndex and Erasure duplicate proofs * pr feedback: use root bank instead of 2 params * pr feedback: & instead of &Arc * pr feedback: reuse fn, remove redundant clones * rebase: fix feature set conflict (cherry picked from commit def3bc4) # Conflicts: # sdk/src/feature_set.rs

…ackport of #34360) (#34541) * Add feature flag for LastIndex and Erasure duplicate proofs (#34360) * Add feature flag for LastIndex and Erasure duplicate proofs * pr feedback: use root bank instead of 2 params * pr feedback: & instead of &Arc * pr feedback: reuse fn, remove redundant clones * rebase: fix feature set conflict (cherry picked from commit def3bc4) # Conflicts: # sdk/src/feature_set.rs * fix feature set conflict --------- Co-authored-by: Ashwin Sekar <[email protected]>

AshwinSekar force-pushed the dup-proof-ff branch from 1000898 to 412385b Compare December 7, 2023 19:54

AshwinSekar marked this pull request as ready for review December 7, 2023 20:15

AshwinSekar requested a review from behzadnouri December 7, 2023 20:15

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

behzadnouri reviewed Dec 8, 2023

View reviewed changes

AshwinSekar force-pushed the dup-proof-ff branch 2 times, most recently from 6a76edc to 2798583 Compare December 8, 2023 00:09

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

AshwinSekar force-pushed the dup-proof-ff branch from 2798583 to ae703c9 Compare December 8, 2023 19:57

AshwinSekar requested a review from carllin December 8, 2023 19:57

AshwinSekar force-pushed the dup-proof-ff branch from ae703c9 to e6215cf Compare December 8, 2023 22:36

behzadnouri reviewed Dec 10, 2023

View reviewed changes

AshwinSekar force-pushed the dup-proof-ff branch from e6215cf to 8314fa2 Compare December 11, 2023 16:03

AshwinSekar requested a review from behzadnouri December 11, 2023 19:04

carllin previously approved these changes Dec 13, 2023

View reviewed changes

AshwinSekar added 4 commits December 19, 2023 00:02

Add feature flag for LastIndex and Erasure duplicate proofs

8491030

pr feedback: use root bank instead of 2 params

b428be0

pr feedback: & instead of &Arc

2262c6d

pr feedback: reuse fn, remove redundant clones

d7ffab5

AshwinSekar dismissed carllin’s stale review via d7ffab5 December 19, 2023 00:02

AshwinSekar force-pushed the dup-proof-ff branch from 8314fa2 to d7ffab5 Compare December 19, 2023 00:02

AshwinSekar requested a review from carllin December 19, 2023 00:03

rebase: fix feature set conflict

c83719d

carllin approved these changes Dec 20, 2023

View reviewed changes

AshwinSekar merged commit def3bc4 into solana-labs:master Dec 20, 2023
44 checks passed

mergify bot mentioned this pull request Dec 20, 2023

v1.17: Add feature flag for LastIndex and Erasure duplicate proofs (backport of #34360) #34541

Merged

AshwinSekar mentioned this pull request Dec 20, 2023

Feature Gate: LastIndex and Erasure duplicate proofs #34543

Closed

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

Add feature flag for LastIndex and Erasure duplicate proofs #34360

Add feature flag for LastIndex and Erasure duplicate proofs #34360

AshwinSekar commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading

mergify bot commented Dec 7, 2023

behzadnouri Dec 8, 2023

behzadnouri left a comment

behzadnouri Dec 8, 2023

AshwinSekar commented Dec 8, 2023 •

edited

Loading

behzadnouri left a comment

behzadnouri Dec 10, 2023

behzadnouri Dec 10, 2023

behzadnouri Dec 10, 2023

behzadnouri Dec 10, 2023

behzadnouri Dec 10, 2023

behzadnouri commented Dec 10, 2023

AshwinSekar commented Dec 11, 2023 •

edited

Loading

AshwinSekar commented Dec 11, 2023

behzadnouri commented Dec 13, 2023 •

edited

Loading

AshwinSekar commented Dec 13, 2023

AshwinSekar commented Dec 13, 2023

	// Notify duplicate consensus state machine
	duplicate_slots_sender.send(shred_slot)?;

		) -> Result<()> {
		let mut root_bank = bank_forks.read().unwrap().root_bank().clone();

Add feature flag for LastIndex and Erasure duplicate proofs #34360

Add feature flag for LastIndex and Erasure duplicate proofs #34360

Conversation

AshwinSekar commented Dec 7, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Dec 7, 2023 • edited Loading

Codecov Report

mergify bot commented Dec 7, 2023

behzadnouri Dec 8, 2023

Choose a reason for hiding this comment

behzadnouri left a comment

Choose a reason for hiding this comment

behzadnouri Dec 8, 2023

Choose a reason for hiding this comment

AshwinSekar commented Dec 8, 2023 • edited Loading

behzadnouri left a comment

Choose a reason for hiding this comment

behzadnouri Dec 10, 2023

Choose a reason for hiding this comment

behzadnouri Dec 10, 2023

Choose a reason for hiding this comment

behzadnouri Dec 10, 2023

Choose a reason for hiding this comment

behzadnouri Dec 10, 2023

Choose a reason for hiding this comment

behzadnouri Dec 10, 2023

Choose a reason for hiding this comment

behzadnouri commented Dec 10, 2023

AshwinSekar commented Dec 11, 2023 • edited Loading

AshwinSekar commented Dec 11, 2023

behzadnouri commented Dec 13, 2023 • edited Loading

AshwinSekar commented Dec 13, 2023

AshwinSekar commented Dec 13, 2023

AshwinSekar commented Dec 7, 2023 •

edited

Loading

codecov bot commented Dec 7, 2023 •

edited

Loading

AshwinSekar commented Dec 8, 2023 •

edited

Loading

AshwinSekar commented Dec 11, 2023 •

edited

Loading

behzadnouri commented Dec 13, 2023 •

edited

Loading