gossip: notify state machine of duplicate proofs #32963

AshwinSekar · 2023-08-23T22:42:30Z

Problem

We consume duplicate shred proofs from gossip, however we don't let fork choice know.

Summary of Changes

Notify the consensus state machine which will in turn update fork choice.

Split from #32862
Fixes #16508

bw-solana

LGTM

AshwinSekar · 2023-08-25T17:58:11Z

this looks like the commit which will allow Scenario 3, i'll wait to merge this until we have a fix.

behzadnouri · 2023-08-30T19:05:35Z

gossip/src/duplicate_shred.rs

@@ -84,6 +84,8 @@ pub enum Error {
    TryFromIntError(#[from] TryFromIntError),
    #[error("unknown slot leader: {0}")]
    UnknownSlotLeader(Slot),
+    #[error("unable to send duplicate slot to state machine")]
+    DuplicateSlotSenderFailure,


please keep the enum sorted

behzadnouri · 2023-08-30T19:06:57Z

gossip/src/duplicate_shred_handler.rs

@@ -25,6 +26,7 @@ const MAX_NUM_ENTRIES_PER_PUBKEY: usize = 128;
 const BUFFER_CAPACITY: usize = 512 * MAX_NUM_ENTRIES_PER_PUBKEY;

 type BufferEntry = [Option<DuplicateShred>; MAX_NUM_CHUNKS];
+type DuplicateSlotSender = Sender<Slot>;


please remove this typedef.
For someone reading the code, Sender<Slot> is more explicit what actual type we are working with, and does not need to jump elsewhere to find the definition.

AshwinSekar · 2023-11-30T04:04:20Z

reopening this now that Scenario 3 has been merged #34186
understandably some duplicate block local cluster tests are failing now that the duplicate proof is actually functional - will keep this as draft until I can fix up those tests.

codecov · 2023-12-01T10:03:23Z

Codecov Report

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

Comparison is base (b04765f) 81.6% compared to head (87bf3e5) 81.6%.
Report is 7 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #32963     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         828      830      +2     
  Lines      224339   224743    +404     
=========================================
+ Hits       183274   183569    +295     
- Misses      41065    41174    +109

behzadnouri · 2024-01-25T15:33:40Z

gossip/src/duplicate_shred_handler.rs

+        let activated_slot = root_bank
+            .feature_set
+            .activated_slot(&feature_set::enable_gossip_duplicate_proof_ingestion::id())
+            .map(|slot| root_bank.epoch_schedule().get_epoch(slot));
+        activated_slot.is_some_and(|feature_epoch| {
+            root_bank.epoch_schedule().get_epoch(slot) > feature_epoch
+                // feature_epoch could only be 0 in tests and new cluster setup.
+                || feature_epoch == 0
+        })


This is more complicated than it should be. Can you make this more readable with something like:

let Some(activated_slot) = root_bank .feature_set .activated_slot(&feature_set::enable_gossip_duplicate_proof_ingestion::id()) else { return false; }

…stead

mergify · 2024-01-29T17:54:17Z

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.

* gossip: notify state machine of duplicate proofs * Add feature flag for ingesting duplicate proofs from Gossip. * Use the Epoch the shred is in instead of the root bank epoch. * Fix unittest by activating the feature. * Add a test for feature disabled case. * EpochSchedule is now not copyable, clone it explicitly. * pr feedback: read epoch schedule on startup, add guard for ff recache * pr feedback: bank_forks lock, -cached_slots_in_epoch, init ff * pr feedback: bank.forks_try_read() -> read() * pr feedback: fix local-cluster setup * local-cluster: do not expose gossip internals, use retry mechanism instead * local-cluster: split out case 4b into separate test and ignore * pr feedback: avoid taking lock if ff is already found * pr feedback: do not cache ff epoch * pr feedback: bank_forks lock, revert to cached_slots_in_epoch * pr feedback: move local variable into helper function * pr feedback: use let else, remove epoch 0 hack --------- Co-authored-by: Wen <crocoxu@gmail.com> (cherry picked from commit 93271d9)

…32963) (#35006) gossip: notify state machine of duplicate proofs (#32963) * gossip: notify state machine of duplicate proofs * Add feature flag for ingesting duplicate proofs from Gossip. * Use the Epoch the shred is in instead of the root bank epoch. * Fix unittest by activating the feature. * Add a test for feature disabled case. * EpochSchedule is now not copyable, clone it explicitly. * pr feedback: read epoch schedule on startup, add guard for ff recache * pr feedback: bank_forks lock, -cached_slots_in_epoch, init ff * pr feedback: bank.forks_try_read() -> read() * pr feedback: fix local-cluster setup * local-cluster: do not expose gossip internals, use retry mechanism instead * local-cluster: split out case 4b into separate test and ignore * pr feedback: avoid taking lock if ff is already found * pr feedback: do not cache ff epoch * pr feedback: bank_forks lock, revert to cached_slots_in_epoch * pr feedback: move local variable into helper function * pr feedback: use let else, remove epoch 0 hack --------- Co-authored-by: Wen <crocoxu@gmail.com> (cherry picked from commit 93271d9) Co-authored-by: Ashwin Sekar <ashwin@solana.com>

AshwinSekar requested review from wen-coding and carllin August 23, 2023 22:43

AshwinSekar force-pushed the duplicate-shred-proof-from-gossip branch from 2331e41 to d147176 Compare August 24, 2023 00:45

bw-solana previously approved these changes Aug 25, 2023

View reviewed changes

behzadnouri reviewed Aug 30, 2023

View reviewed changes

AshwinSekar dismissed bw-solana’s stale review via b63c9d7 September 5, 2023 09:34

AshwinSekar force-pushed the duplicate-shred-proof-from-gossip branch from d147176 to b63c9d7 Compare September 5, 2023 09:34

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

github-actions bot closed this Sep 27, 2023

AshwinSekar reopened this Nov 29, 2023

AshwinSekar marked this pull request as draft November 29, 2023 18:10

AshwinSekar self-assigned this Nov 29, 2023

AshwinSekar force-pushed the duplicate-shred-proof-from-gossip branch from b63c9d7 to bd931bf Compare November 30, 2023 04:01

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

AshwinSekar force-pushed the duplicate-shred-proof-from-gossip branch from bd931bf to 3a0ba29 Compare December 1, 2023 08:38

AshwinSekar marked this pull request as ready for review December 1, 2023 08:50

AshwinSekar force-pushed the duplicate-shred-proof-from-gossip branch from 3a0ba29 to 048e1a0 Compare December 1, 2023 08:51

This was referenced Dec 7, 2023

v1.17: gossip: process duplicate proofs for merkle root conflicts (backport of #34066) #34292

Closed

Add feature flag for LastIndex and Erasure duplicate proofs #34360

Merged

wen-coding mentioned this pull request Dec 8, 2023

Gossip proofs should be ingested into blockstore's, and used to notify ReplayStage of duplicates #16508

Closed

AshwinSekar marked this pull request as draft December 8, 2023 17:19

AshwinSekar added the feature-gate Pull Request adds or modifies a runtime feature gate label Jan 3, 2024

AshwinSekar force-pushed the duplicate-shred-proof-from-gossip branch from bd3ccc4 to e3dcff6 Compare January 3, 2024 03:35

AshwinSekar marked this pull request as ready for review January 3, 2024 17:12

AshwinSekar force-pushed the duplicate-shred-proof-from-gossip branch 2 times, most recently from 34dc33b to 61ce88a Compare January 4, 2024 02:35

behzadnouri reviewed Jan 25, 2024

View reviewed changes

AshwinSekar force-pushed the duplicate-shred-proof-from-gossip branch from bfa18af to 850dc1e Compare January 25, 2024 17:33

AshwinSekar and others added 17 commits January 25, 2024 11:08

gossip: notify state machine of duplicate proofs

3ce88e7

Add feature flag for ingesting duplicate proofs from Gossip.

1f9591b

Use the Epoch the shred is in instead of the root bank epoch.

653f1d3

Fix unittest by activating the feature.

9af61ed

Add a test for feature disabled case.

32b1d58

EpochSchedule is now not copyable, clone it explicitly.

8705f78

pr feedback: read epoch schedule on startup, add guard for ff recache

43b68f7

pr feedback: bank_forks lock, -cached_slots_in_epoch, init ff

596c163

pr feedback: bank.forks_try_read() -> read()

8872824

pr feedback: fix local-cluster setup

52ca823

local-cluster: do not expose gossip internals, use retry mechanism in…

70677ca

…stead

local-cluster: split out case 4b into separate test and ignore

92c3dc3

pr feedback: avoid taking lock if ff is already found

5c825fb

pr feedback: do not cache ff epoch

8805a63

pr feedback: bank_forks lock, revert to cached_slots_in_epoch

bee6578

pr feedback: move local variable into helper function

8e2e70c

pr feedback: use let else, remove epoch 0 hack

87bf3e5

AshwinSekar force-pushed the duplicate-shred-proof-from-gossip branch from 850dc1e to 87bf3e5 Compare January 25, 2024 19:08

behzadnouri approved these changes Jan 26, 2024

View reviewed changes

AshwinSekar merged commit 93271d9 into solana-labs:master Jan 26, 2024
45 checks passed

wen-coding mentioned this pull request Jan 29, 2024

Feature Gate: notify state machine of duplicate proofs #35005

Open

AshwinSekar added the v1.18 PRs that should be backported to v1.18 label Jan 29, 2024

mergify bot mentioned this pull request Jan 29, 2024

v1.18: gossip: notify state machine of duplicate proofs (backport of #32963) #35006

Merged

AshwinSekar mentioned this pull request Feb 29, 2024

Fix the setup for duplicate block local-cluster tests #34857

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

gossip: notify state machine of duplicate proofs #32963

gossip: notify state machine of duplicate proofs #32963

AshwinSekar commented Aug 23, 2023 •

edited

bw-solana left a comment

AshwinSekar commented Aug 25, 2023

behzadnouri Aug 30, 2023

behzadnouri Aug 30, 2023

AshwinSekar commented Nov 30, 2023

codecov bot commented Dec 1, 2023 •

edited

behzadnouri Jan 25, 2024

mergify bot commented Jan 29, 2024

gossip: notify state machine of duplicate proofs #32963

gossip: notify state machine of duplicate proofs #32963

Conversation

AshwinSekar commented Aug 23, 2023 • edited

Problem

Summary of Changes

bw-solana left a comment

Choose a reason for hiding this comment

AshwinSekar commented Aug 25, 2023

behzadnouri Aug 30, 2023

Choose a reason for hiding this comment

behzadnouri Aug 30, 2023

Choose a reason for hiding this comment

AshwinSekar commented Nov 30, 2023

codecov bot commented Dec 1, 2023 • edited

Codecov Report

behzadnouri Jan 25, 2024

Choose a reason for hiding this comment

mergify bot commented Jan 29, 2024

AshwinSekar commented Aug 23, 2023 •

edited

codecov bot commented Dec 1, 2023 •

edited