Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Commit

Permalink
Account for duplicate before a bank is frozen or replayed (#17866) (#…
Browse files Browse the repository at this point in the history 
…17883)

(cherry picked from commit afafa62)

Co-authored-by: carllin <carl@solana.com>
  • Loading branch information
@mergify @carllin
mergify[bot] and carllin authored Jun 11, 2021
1 parent af2a610 commit 10507f0
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 23 deletions.
117 changes: 102 additions & 15 deletions core/src/cluster_slot_state_verifier.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ use crate::{
progress_map::ProgressMap,
};
use solana_sdk::{clock::Slot, hash::Hash};
use std::collections::{BTreeMap, HashMap, HashSet};
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};

pub(crate) type DuplicateSlotsTracker = BTreeSet<Slot>;
pub(crate) type GossipDuplicateConfirmedSlots = BTreeMap<Slot, Hash>;
type SlotStateHandler = fn(Slot, &Hash, Option<&Hash>, bool, bool) -> Vec<ResultingStateChange>;

Expand Down Expand Up @@ -234,10 +235,12 @@ fn apply_state_changes(
}
}

#[allow(clippy::too_many_arguments)]
pub(crate) fn check_slot_agrees_with_cluster(
slot: Slot,
root: Slot,
frozen_hash: Option<Hash>,
duplicate_slots_tracker: &mut DuplicateSlotsTracker,
gossip_duplicate_confirmed_slots: &GossipDuplicateConfirmedSlots,
ancestors: &HashMap<Slot, HashSet<Slot>>,
descendants: &HashMap<Slot, HashSet<Slot>>,
Expand All @@ -258,6 +261,15 @@ pub(crate) fn check_slot_agrees_with_cluster(
return;
}

// Needs to happen before the frozen_hash.is_none() check below to account for duplicate
// signals arriving before the bank is constructed in replay.
if matches!(slot_state_update, SlotStateUpdate::Duplicate) {
// If this slot has already been processed before, return
if !duplicate_slots_tracker.insert(slot) {
return;
}
}

if frozen_hash.is_none() {
// If the bank doesn't even exist in BankForks yet,
// then there's nothing to do as replay of the slot
Expand All @@ -275,18 +287,7 @@ pub(crate) fn check_slot_agrees_with_cluster(
&frozen_hash,
is_local_replay_duplicate_confirmed,
);
let mut is_slot_duplicate =
progress.is_unconfirmed_duplicate(slot).expect("If the frozen hash exists, then the slot must exist in bank forks and thus in progress map");
if matches!(slot_state_update, SlotStateUpdate::Duplicate) {
if is_slot_duplicate {
// Already processed duplicate signal for this slot, no need to continue
return;
} else {
// Otherwise, mark the slot as duplicate so the appropriate state changes
// will trigger
is_slot_duplicate = true;
}
}
let is_slot_duplicate = duplicate_slots_tracker.contains(&slot);
let is_dead = progress.is_dead(slot).expect("If the frozen hash exists, then the slot must exist in bank forks and thus in progress map");

info!(
Expand Down Expand Up @@ -687,6 +688,84 @@ mod test {
.unwrap());
}

fn run_test_state_duplicate_then_bank_frozen(initial_bank_hash: Option<Hash>) {
// Common state
let InitialState {
mut heaviest_subtree_fork_choice,
mut progress,
ancestors,
descendants,
bank_forks,
..
} = setup();

// Setup a duplicate slot state transition with the initial bank state of the duplicate slot
// determined by `initial_bank_hash`, which can be:
// 1) A default hash (unfrozen bank),
// 2) None (a slot that hasn't even started replay yet).
let root = 0;
let mut duplicate_slots_tracker = DuplicateSlotsTracker::default();
let gossip_duplicate_confirmed_slots = GossipDuplicateConfirmedSlots::default();
let duplicate_slot = 2;
check_slot_agrees_with_cluster(
duplicate_slot,
root,
initial_bank_hash,
&mut duplicate_slots_tracker,
&gossip_duplicate_confirmed_slots,
&ancestors,
&descendants,
&mut progress,
&mut heaviest_subtree_fork_choice,
SlotStateUpdate::Duplicate,
);

// Now freeze the bank
let frozen_duplicate_slot_hash = bank_forks
.read()
.unwrap()
.get(duplicate_slot)
.unwrap()
.hash();
check_slot_agrees_with_cluster(
duplicate_slot,
root,
Some(frozen_duplicate_slot_hash),
&mut duplicate_slots_tracker,
&gossip_duplicate_confirmed_slots,
&ancestors,
&descendants,
&mut progress,
&mut heaviest_subtree_fork_choice,
SlotStateUpdate::Frozen,
);

// Progress map should have the correct updates, fork choice should mark duplicate
// as unvotable
assert!(progress.is_unconfirmed_duplicate(duplicate_slot).unwrap());

// The ancestor of the duplicate slot should be the best slot now
let (duplicate_ancestor, duplicate_parent_hash) = {
let r_bank_forks = bank_forks.read().unwrap();
let parent_bank = r_bank_forks.get(duplicate_slot).unwrap().parent().unwrap();
(parent_bank.slot(), parent_bank.hash())
};
assert_eq!(
heaviest_subtree_fork_choice.best_overall_slot(),
(duplicate_ancestor, duplicate_parent_hash)
);
}

#[test]
fn test_state_unfrozen_bank_duplicate_then_bank_frozen() {
run_test_state_duplicate_then_bank_frozen(Some(Hash::default()));
}

#[test]
fn test_state_unreplayed_bank_duplicate_then_bank_frozen() {
run_test_state_duplicate_then_bank_frozen(None);
}

#[test]
fn test_state_ancestor_confirmed_descendant_duplicate() {
// Common state
Expand All @@ -705,6 +784,7 @@ mod test {
(3, slot3_hash)
);
let root = 0;
let mut duplicate_slots_tracker = DuplicateSlotsTracker::default();
let mut gossip_duplicate_confirmed_slots = GossipDuplicateConfirmedSlots::default();

// Mark slot 2 as duplicate confirmed
Expand All @@ -714,6 +794,7 @@ mod test {
2,
root,
Some(slot2_hash),
&mut duplicate_slots_tracker,
&gossip_duplicate_confirmed_slots,
&ancestors,
&descendants,
Expand All @@ -727,11 +808,13 @@ mod test {
(3, slot3_hash)
);

// Mark 3 as duplicate, should not remove slot 2 from fork choice
// Mark 3 as duplicate, should not remove the duplicate confirmed slot 2 from
// fork choice
check_slot_agrees_with_cluster(
3,
root,
Some(slot3_hash),
&mut duplicate_slots_tracker,
&gossip_duplicate_confirmed_slots,
&ancestors,
&descendants,
Expand Down Expand Up @@ -764,12 +847,15 @@ mod test {
(3, slot3_hash)
);
let root = 0;
let mut duplicate_slots_tracker = DuplicateSlotsTracker::default();
let mut gossip_duplicate_confirmed_slots = GossipDuplicateConfirmedSlots::default();
// Mark 2 as duplicate confirmed

// Mark 2 as duplicate
check_slot_agrees_with_cluster(
2,
root,
Some(bank_forks.read().unwrap().get(2).unwrap().hash()),
&mut duplicate_slots_tracker,
&gossip_duplicate_confirmed_slots,
&ancestors,
&descendants,
Expand All @@ -790,6 +876,7 @@ mod test {
3,
root,
Some(slot3_hash),
&mut duplicate_slots_tracker,
&gossip_duplicate_confirmed_slots,
&ancestors,
&descendants,
Expand Down
3 changes: 2 additions & 1 deletion core/src/consensus.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1358,7 +1358,7 @@ pub mod test {
use super::*;
use crate::{
cluster_info_vote_listener::VoteTracker,
cluster_slot_state_verifier::GossipDuplicateConfirmedSlots,
cluster_slot_state_verifier::{DuplicateSlotsTracker, GossipDuplicateConfirmedSlots},
cluster_slots::ClusterSlots,
fork_choice::SelectVoteAndResetForkResult,
heaviest_subtree_fork_choice::{HeaviestSubtreeForkChoice, SlotHashKey},
Expand Down Expand Up @@ -1554,6 +1554,7 @@ pub mod test {
&AbsRequestSender::default(),
None,
&mut self.heaviest_subtree_fork_choice,
&mut DuplicateSlotsTracker::default(),
&mut GossipDuplicateConfirmedSlots::default(),
&mut UnfrozenGossipVerifiedVoteHashes::default(),
&mut true,
Expand Down
1 change: 1 addition & 0 deletions core/src/progress_map.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,7 @@ impl ProgressMap {
}
}

#[cfg(test)]
pub fn is_unconfirmed_duplicate(&self, slot: Slot) -> Option<bool> {
self.get(&slot).map(|p| {
p.duplicate_stats
Expand Down
Loading

0 comments on commit 10507f0

Please sign in to comment.