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

Allow fork choice to support multiple versions of a slot #16266

Merged
merged 8 commits into from
Apr 12, 2021
Merged

Allow fork choice to support multiple versions of a slot #16266

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
657ae72
Allow fork choice to support multiple versions of a slot
carllin Mar 16, 2021
6683a16
test fixes
carllin Apr 1, 2021
554c152
Work in progress for handling dup votes
carllin Apr 1, 2021
1defd1f
Handle duplicates in same add_votes batch
carllin Apr 3, 2021
9846345
Fix tests
carllin Apr 6, 2021
af6a3a3
PR comments
carllin Apr 10, 2021
8c0be19
Just take smaller version with smaller hash of duplicate vote
carllin Apr 10, 2021
1d461b9
More PR comments
carllin Apr 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
97 changes: 65 additions & 32 deletions core/src/cluster_slot_state_verifier.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@ pub enum SlotStateUpdate {

#[derive(PartialEq, Debug)]
pub enum ResultingStateChange {
MarkSlotDuplicate,
// Hash of our current frozen version of the slot
MarkSlotDuplicate(Hash),
// Hash of the cluster confirmed slot that is not equivalent
// to our frozen version of the slot
RepairDuplicateConfirmedVersion(Hash),
DuplicateConfirmedSlotMatchesCluster,
// Hash of our current frozen version of the slot
DuplicateConfirmedSlotMatchesCluster(Hash),
}

impl SlotStateUpdate {
Expand Down Expand Up @@ -56,9 +60,9 @@ fn on_dead_slot(
// No need to check `is_slot_duplicate` and modify fork choice as dead slots
// are never frozen, and thus never added to fork choice. The state change for
// `MarkSlotDuplicate` will try to modify fork choice, but won't find the slot
// in the fork choice tree, so is equivalent to a
// in the fork choice tree, so is equivalent to a no-op
return vec![
ResultingStateChange::MarkSlotDuplicate,
ResultingStateChange::MarkSlotDuplicate(Hash::default()),
ResultingStateChange::RepairDuplicateConfirmedVersion(
*cluster_duplicate_confirmed_hash,
),
Expand Down Expand Up @@ -92,7 +96,7 @@ fn on_frozen_slot(
slot, cluster_duplicate_confirmed_hash, bank_frozen_hash
);
return vec![
ResultingStateChange::MarkSlotDuplicate,
ResultingStateChange::MarkSlotDuplicate(*bank_frozen_hash),
ResultingStateChange::RepairDuplicateConfirmedVersion(
*cluster_duplicate_confirmed_hash,
),
Expand All @@ -101,7 +105,9 @@ fn on_frozen_slot(
// If the versions match, then add the slot to the candidate
// set to account for the case where it was removed earlier
// by the `on_duplicate_slot()` handler
return vec![ResultingStateChange::DuplicateConfirmedSlotMatchesCluster];
return vec![ResultingStateChange::DuplicateConfirmedSlotMatchesCluster(
*bank_frozen_hash,
)];
}
}

Expand All @@ -115,7 +121,7 @@ fn on_frozen_slot(
// 2) A gossip duplicate_confirmed version that didn't match our frozen
// version.
// In both cases, mark the progress map for this slot as duplicate
return vec![ResultingStateChange::MarkSlotDuplicate];
return vec![ResultingStateChange::MarkSlotDuplicate(*bank_frozen_hash)];
}

vec![]
Expand Down Expand Up @@ -202,12 +208,12 @@ fn apply_state_changes(
) {
for state_change in state_changes {
match state_change {
ResultingStateChange::MarkSlotDuplicate => {
ResultingStateChange::MarkSlotDuplicate(bank_frozen_hash) => {
progress.set_unconfirmed_duplicate_slot(
slot,
descendants.get(&slot).unwrap_or(&HashSet::default()),
);
fork_choice.mark_fork_invalid_candidate(slot);
fork_choice.mark_fork_invalid_candidate(&(slot, bank_frozen_hash));
}
ResultingStateChange::RepairDuplicateConfirmedVersion(
cluster_duplicate_confirmed_hash,
Expand All @@ -216,13 +222,13 @@ fn apply_state_changes(
// progress map from ReplayStage::confirm_forks to here.
repair_correct_version(slot, &cluster_duplicate_confirmed_hash);
}
ResultingStateChange::DuplicateConfirmedSlotMatchesCluster => {
ResultingStateChange::DuplicateConfirmedSlotMatchesCluster(bank_frozen_hash) => {
progress.set_confirmed_duplicate_slot(
slot,
ancestors.get(&slot).unwrap_or(&HashSet::default()),
descendants.get(&slot).unwrap_or(&HashSet::default()),
);
fork_choice.mark_fork_valid_candidate(slot);
fork_choice.mark_fork_valid_candidate(&(slot, bank_frozen_hash));
}
}
}
Expand Down Expand Up @@ -388,7 +394,7 @@ mod test {
is_slot_duplicate,
is_dead
),
vec![ResultingStateChange::MarkSlotDuplicate]
vec![ResultingStateChange::MarkSlotDuplicate(bank_frozen_hash)]
);
}

Expand Down Expand Up @@ -426,7 +432,9 @@ mod test {
is_slot_duplicate,
is_dead
),
vec![ResultingStateChange::DuplicateConfirmedSlotMatchesCluster,]
vec![ResultingStateChange::DuplicateConfirmedSlotMatchesCluster(
bank_frozen_hash
),]
);

// If the cluster_duplicate_confirmed_hash does not match, then we
Expand All @@ -443,7 +451,7 @@ mod test {
is_dead
),
vec![
ResultingStateChange::MarkSlotDuplicate,
ResultingStateChange::MarkSlotDuplicate(bank_frozen_hash),
ResultingStateChange::RepairDuplicateConfirmedVersion(mismatched_hash),
]
);
Expand Down Expand Up @@ -483,7 +491,7 @@ mod test {
is_slot_duplicate,
is_dead
),
vec![ResultingStateChange::MarkSlotDuplicate,]
vec![ResultingStateChange::MarkSlotDuplicate(bank_frozen_hash),]
);

// If the cluster_duplicate_confirmed_hash matches, we just confirm
Expand All @@ -497,7 +505,9 @@ mod test {
is_slot_duplicate,
is_dead
),
vec![ResultingStateChange::DuplicateConfirmedSlotMatchesCluster,]
vec![ResultingStateChange::DuplicateConfirmedSlotMatchesCluster(
bank_frozen_hash
),]
);

// If the cluster_duplicate_confirmed_hash does not match, then we
Expand All @@ -514,7 +524,7 @@ mod test {
is_dead
),
vec![
ResultingStateChange::MarkSlotDuplicate,
ResultingStateChange::MarkSlotDuplicate(bank_frozen_hash),
ResultingStateChange::RepairDuplicateConfirmedVersion(mismatched_hash),
]
);
Expand Down Expand Up @@ -589,7 +599,7 @@ mod test {
is_dead
),
vec![
ResultingStateChange::MarkSlotDuplicate,
ResultingStateChange::MarkSlotDuplicate(bank_frozen_hash),
ResultingStateChange::RepairDuplicateConfirmedVersion(correct_hash),
]
);
Expand All @@ -605,7 +615,7 @@ mod test {
is_dead
),
vec![
ResultingStateChange::MarkSlotDuplicate,
ResultingStateChange::MarkSlotDuplicate(bank_frozen_hash),
ResultingStateChange::RepairDuplicateConfirmedVersion(correct_hash),
]
);
Expand All @@ -620,21 +630,22 @@ mod test {
ancestors,
descendants,
slot,
..
bank_forks,
} = setup();

// MarkSlotDuplicate should mark progress map and remove
// the slot from fork choice
let slot_hash = bank_forks.read().unwrap().get(slot).unwrap().hash();
apply_state_changes(
slot,
&mut progress,
&mut heaviest_subtree_fork_choice,
&ancestors,
&descendants,
vec![ResultingStateChange::MarkSlotDuplicate],
vec![ResultingStateChange::MarkSlotDuplicate(slot_hash)],
);
assert!(!heaviest_subtree_fork_choice
.is_candidate_slot(slot)
.is_candidate_slot(&(slot, slot_hash))
.unwrap());
for child_slot in descendants
.get(&slot)
Expand All @@ -657,7 +668,9 @@ mod test {
&mut heaviest_subtree_fork_choice,
&ancestors,
&descendants,
vec![ResultingStateChange::DuplicateConfirmedSlotMatchesCluster],
vec![ResultingStateChange::DuplicateConfirmedSlotMatchesCluster(
slot_hash,
)],
);
for child_slot in descendants
.get(&slot)
Expand All @@ -670,7 +683,7 @@ mod test {
.is_none());
}
assert!(heaviest_subtree_fork_choice
.is_candidate_slot(slot)
.is_candidate_slot(&(slot, slot_hash))
.unwrap());
}

Expand All @@ -686,7 +699,11 @@ mod test {
..
} = setup();

assert_eq!(heaviest_subtree_fork_choice.best_overall_slot(), 3);
let slot3_hash = bank_forks.read().unwrap().get(3).unwrap().hash();
assert_eq!(
heaviest_subtree_fork_choice.best_overall_slot(),
(3, slot3_hash)
);
let root = 0;
let mut gossip_duplicate_confirmed_slots = GossipDuplicateConfirmedSlots::default();

Expand All @@ -705,13 +722,16 @@ mod test {
SlotStateUpdate::DuplicateConfirmed,
);

assert_eq!(heaviest_subtree_fork_choice.best_overall_slot(), 3);
assert_eq!(
heaviest_subtree_fork_choice.best_overall_slot(),
(3, slot3_hash)
);

// Mark 3 as duplicate, should not remove slot 2 from fork choice
check_slot_agrees_with_cluster(
3,
root,
Some(bank_forks.read().unwrap().get(3).unwrap().hash()),
Some(slot3_hash),
&gossip_duplicate_confirmed_slots,
&ancestors,
&descendants,
Expand All @@ -720,7 +740,10 @@ mod test {
SlotStateUpdate::Duplicate,
);

assert_eq!(heaviest_subtree_fork_choice.best_overall_slot(), 2);
assert_eq!(
heaviest_subtree_fork_choice.best_overall_slot(),
(2, slot2_hash)
);
}

#[test]
Expand All @@ -735,7 +758,11 @@ mod test {
..
} = setup();

assert_eq!(heaviest_subtree_fork_choice.best_overall_slot(), 3);
let slot3_hash = bank_forks.read().unwrap().get(3).unwrap().hash();
assert_eq!(
heaviest_subtree_fork_choice.best_overall_slot(),
(3, slot3_hash)
);
let root = 0;
let mut gossip_duplicate_confirmed_slots = GossipDuplicateConfirmedSlots::default();
// Mark 2 as duplicate confirmed
Expand All @@ -751,10 +778,13 @@ mod test {
SlotStateUpdate::Duplicate,
);

assert_eq!(heaviest_subtree_fork_choice.best_overall_slot(), 1);
let slot1_hash = bank_forks.read().unwrap().get(1).unwrap().hash();
assert_eq!(
heaviest_subtree_fork_choice.best_overall_slot(),
(1, slot1_hash)
);

// Mark slot 3 as duplicate confirmed, should mark slot 2 as duplicate confirmed as well
let slot3_hash = bank_forks.read().unwrap().get(3).unwrap().hash();
gossip_duplicate_confirmed_slots.insert(3, slot3_hash);
check_slot_agrees_with_cluster(
3,
Expand All @@ -768,6 +798,9 @@ mod test {
SlotStateUpdate::DuplicateConfirmed,
);

assert_eq!(heaviest_subtree_fork_choice.best_overall_slot(), 3);
assert_eq!(
heaviest_subtree_fork_choice.best_overall_slot(),
(3, slot3_hash)
);
}
}
13 changes: 10 additions & 3 deletions core/src/consensus.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,9 @@ impl Tower {
);
let root = root_bank.slot();

let (best_slot, best_hash) = heaviest_subtree_fork_choice.best_overall_slot();
let heaviest_bank = bank_forks
.get(heaviest_subtree_fork_choice.best_overall_slot())
.get_with_checked_hash((best_slot, best_hash))
.expect(
"The best overall slot must be one of `frozen_banks` which all exist in bank_forks",
)
Expand Down Expand Up @@ -436,6 +437,10 @@ impl Tower {
self.last_vote.last_voted_slot()
}

pub fn last_voted_slot_hash(&self) -> Option<(Slot, Hash)> {
self.last_vote.last_voted_slot_hash()
}

pub fn stray_restored_slot(&self) -> Option<Slot> {
self.stray_restored_slot
}
Expand Down Expand Up @@ -1372,8 +1377,10 @@ pub mod test {
}
}
new_bank.freeze();
self.heaviest_subtree_fork_choice
.add_new_leaf_slot(new_bank.slot(), Some(new_bank.parent_slot()));
self.heaviest_subtree_fork_choice.add_new_leaf_slot(
(new_bank.slot(), new_bank.hash()),
Some((new_bank.parent_slot(), new_bank.parent_hash())),
);
self.bank_forks.write().unwrap().insert(new_bank);
walk.forward();
}
Expand Down
6 changes: 3 additions & 3 deletions core/src/fork_choice.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use crate::{
replay_stage::HeaviestForkFailures,
};
use solana_runtime::{bank::Bank, bank_forks::BankForks};
use solana_sdk::clock::Slot;
use std::{
collections::{HashMap, HashSet},
sync::{Arc, RwLock},
Expand All @@ -17,6 +16,7 @@ pub(crate) struct SelectVoteAndResetForkResult {
}

pub(crate) trait ForkChoice {
type ForkChoiceKey;
fn compute_bank_stats(
&mut self,
bank: &Bank,
Expand All @@ -38,7 +38,7 @@ pub(crate) trait ForkChoice {
bank_forks: &RwLock<BankForks>,
) -> (Arc<Bank>, Option<Arc<Bank>>);

fn mark_fork_invalid_candidate(&mut self, invalid_slot: Slot);
fn mark_fork_invalid_candidate(&mut self, invalid_slot: &Self::ForkChoiceKey);

fn mark_fork_valid_candidate(&mut self, valid_slot: Slot);
fn mark_fork_valid_candidate(&mut self, valid_slot: &Self::ForkChoiceKey);
}
Loading