fix duplicate confirmed rollup detection for descendants #34014

AshwinSekar · 2023-11-10T05:40:59Z

Problem

State machine is only notified of duplicate confirmed rollup when optimistically confirmed

Summary of Changes

Make the check granular, notify when >52%
dedup requests against duplicate confirmed slots already sent to the fsm

Fixes #

AshwinSekar · 2023-11-10T05:53:08Z

core/src/replay_stage.rs

+                        bank.hash(),
+                    ));
+                } else if bank.is_frozen()
+                    && tower.is_slot_duplicate_confirmed(*slot, voted_stakes, total_stake)


previously we were only calling mark_slots_confirmed if we got tower.is_slot_confirmed . this change allows us to notify the fsm as soon as we have tower.is_slot_duplicate_confirmed

codecov · 2023-11-10T06:54:53Z

Codecov Report

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

Comparison is base (1db76cf) 81.8% compared to head (be77751) 81.8%.
Report is 66 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #34014   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         822      822           
  Lines      221573   221623   +50     
=======================================
+ Hits       181341   181416   +75     
+ Misses      40232    40207   -25

carllin · 2023-11-15T14:28:29Z

core/src/replay_stage.rs

@@ -3877,7 +3925,25 @@ impl ReplayStage {
                if bank.is_frozen() && tower.is_slot_confirmed(*slot, voted_stakes, total_stake) {
                    info!("validator fork confirmed {} {}ms", *slot, duration);
                    datapoint_info!("validator-confirmation", ("duration_ms", duration, i64));
-                    confirmed_forks.push((*slot, bank.hash()));
+                    confirmed_forks.push(ConfirmedSlot::new_optimistic_confirmed_slot(


don't know if we should call this optimistically confirmed because optimistic confirmation doesn't roll up.

open to ideas about naming, but also I think we should just fix the plumbing and make it roll up while we're looking at this.

The plumbing rn seems to be a mess:

cluster_info_vote_listener tracks gossip votes but also receives replay votes through crossbeam from replay_stage

cluster_info_vote_listener tracks gossip votes for propagated check in vote_tracker

replay_stage also tracks replay votes in vote_tracker and rolls up in fork_stats in compute_bank_stats

cluster_info_vote_listener also separately sends verified gossip votes to replay_stage through crossbeam

replay_stage tracks DuplicateConfirmed through fork_stats but also gets notified through crossbeam from cluster_info_vote_listener

There's a lot of back and forth propagation of votes and stake info in multiple places which seems inefficient. My vote would be to just share 1 vote tracker/fork stats between cluster_info_vote_listener and replay_stage to avoid the extra work. This would also let us rollup replay votes and include gossip votes that would get us over the key threshold.

If we don't want to do a big refactor I think there's a simpler solution for rollup:

solana/ledger/src/blockstore_processor.rs

Lines 171 to 175 in d58db6e

bank_utils::find_and_send_votes(

batch.sanitized_transactions(),

&tx_results,

replay_vote_sender,

);

When sending replayed votes from replay_stage to cluster_info_vote_listener, send the slot hashes as well.

Then

solana/core/src/cluster_info_vote_listener.rs

Lines 677 to 680 in d58db6e

// We cannot count any other slots in this vote toward optimistic confirmation because:

// 1) There may have been a switch between the earlier vote and the last vote

// 2) We do not know the hash of the earlier slot

if slot == last_vote_slot {

If we have !is_gossip we don't have to filter only on the last slot here, we can track the whole tower since we have the accompanying hash in slot hashes.

to clarify this^ is out of scope for this pr and should not be a blocker for merging. we can discuss in #34279.

I agree that optimistically confirmed is an overloaded name and am happy to rename if you have a suggestion.

optimistically confirmed actually can't be safely rolled up, it has to be all votes on the same slot

I think just called it supermajority_voted?

carllin · 2023-11-15T14:35:28Z

core/src/replay_stage.rs

+            if *slot <= root_slot {
+                continue;
+            } else if let Some(prev_hash) = duplicate_confirmed_slots.insert(*slot, *frozen_hash) {


Would be good here to add a check for the ConfirmationType::DuplicateConfirmed in case we add any other cases that accidentally fall into this branch

we want ConfirmationType::OptimisticallyConfirmed to fall through here, but if you prefer I can make it explicit.

yeah I think explicit check would be good

carllin · 2023-11-15T14:40:36Z

core/src/replay_stage.rs

+            if *slot <= root_slot {
+                continue;


This is a deviation from current behavior i.e. some supermajority confirmed slots less than the root may no longer be added as duplicate confirmed and passed to the state machine. Seems ok since whatever the latest root slot was should have notified the state machine that all the ancestors are confirmed?

yeah exactly, since we have the threshold check we must have already processed this signal. But also probably not a huge deal if we remove this check.

mergify · 2023-11-17T17:40:26Z

Backports to the stable 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.

mergify · 2023-11-17T17:40:27Z

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.

carllin

just some nits

AshwinSekar requested review from carllin and wen-coding November 10, 2023 05:51

AshwinSekar commented Nov 10, 2023

View reviewed changes

AshwinSekar added v1.17 PRs that should be backported to v1.17 v1.16 PRs that should be backported to v1.16 labels Nov 10, 2023

AshwinSekar force-pushed the fix-dup-conf-rollup branch from 69254ba to 1b4a697 Compare November 10, 2023 19:51

carllin reviewed Nov 15, 2023

View reviewed changes

carllin previously approved these changes Nov 30, 2023

View reviewed changes

AshwinSekar dismissed carllin’s stale review via 2fab75a November 30, 2023 05:46

AshwinSekar mentioned this pull request Nov 30, 2023

Allow mixed replay descendant + gossip tracking for optimistic confirmation #34279

Closed

AshwinSekar removed v1.16 PRs that should be backported to v1.16 v1.17 PRs that should be backported to v1.17 labels Nov 30, 2023

github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Dec 14, 2023

AshwinSekar requested a review from carllin December 21, 2023 01:13

AshwinSekar added 2 commits December 20, 2023 20:13

fix duplicate confirmed rollup detection for descendants

0211512

pr feedback: optimistic rename -> guard new enum

be77751

AshwinSekar force-pushed the fix-dup-conf-rollup branch from 2fab75a to be77751 Compare December 21, 2023 01:13

github-actions bot added stale [bot only] Added to stale content; results in auto-close after a week. and removed stale [bot only] Added to stale content; results in auto-close after a week. labels Jan 4, 2024

carllin approved these changes Jan 10, 2024

View reviewed changes

AshwinSekar merged commit fb97e93 into solana-labs:master Jan 10, 2024
34 checks passed

willhickey mentioned this pull request Mar 28, 2024

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

Closed

AshwinSekar mentioned this pull request May 7, 2024

fix spammy duplicate confirmation log anza-xyz/agave#1236

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix duplicate confirmed rollup detection for descendants #34014

fix duplicate confirmed rollup detection for descendants #34014

AshwinSekar commented Nov 10, 2023

AshwinSekar Nov 10, 2023

codecov bot commented Nov 10, 2023 •

edited

Loading

carllin Nov 15, 2023

AshwinSekar Nov 15, 2023

AshwinSekar Nov 30, 2023

carllin Nov 30, 2023

carllin Nov 30, 2023 •

edited

Loading

carllin Nov 15, 2023

AshwinSekar Nov 15, 2023

carllin Nov 30, 2023

carllin Nov 15, 2023

AshwinSekar Nov 15, 2023

mergify bot commented Nov 17, 2023

mergify bot commented Nov 17, 2023

carllin left a comment

	bank_utils::find_and_send_votes(
	batch.sanitized_transactions(),
	&tx_results,
	replay_vote_sender,
	);

	// We cannot count any other slots in this vote toward optimistic confirmation because:
	// 1) There may have been a switch between the earlier vote and the last vote
	// 2) We do not know the hash of the earlier slot
	if slot == last_vote_slot {

fix duplicate confirmed rollup detection for descendants #34014

fix duplicate confirmed rollup detection for descendants #34014

Conversation

AshwinSekar commented Nov 10, 2023

Problem

Summary of Changes

Choose a reason for hiding this comment

codecov bot commented Nov 10, 2023 • edited Loading

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

carllin Nov 30, 2023 • edited Loading

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

mergify bot commented Nov 17, 2023

mergify bot commented Nov 17, 2023

carllin left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 10, 2023 •

edited

Loading

carllin Nov 30, 2023 •

edited

Loading