From 4ab7d6c23ee3f1ee59113c787a212eae1d0ca103 Mon Sep 17 00:00:00 2001 From: carllin Date: Thu, 13 Jan 2022 19:51:00 -0500 Subject: [PATCH] Filter out outdated slots (#22450) * Filter out outdated slots * Fixup error --- core/src/cluster_info_vote_listener.rs | 3 + core/src/consensus.rs | 6 +- local-cluster/tests/local_cluster.rs | 1 - programs/vote/src/vote_instruction.rs | 12 +- programs/vote/src/vote_state/mod.rs | 169 +++++++++++++++++++------ sdk/src/feature_set.rs | 5 + 6 files changed, 151 insertions(+), 45 deletions(-) diff --git a/core/src/cluster_info_vote_listener.rs b/core/src/cluster_info_vote_listener.rs index eb137ef69c..b546c6a833 100644 --- a/core/src/cluster_info_vote_listener.rs +++ b/core/src/cluster_info_vote_listener.rs @@ -609,6 +609,9 @@ impl ClusterInfoVoteListener { // The last vote slot, which is the greatest slot in the stack // of votes in a vote transaction, qualifies for optimistic confirmation. + // 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 { let vote_accounts = epoch_stakes.stakes().vote_accounts(); let stake = vote_accounts diff --git a/core/src/consensus.rs b/core/src/consensus.rs index 059284f30f..96929b899e 100644 --- a/core/src/consensus.rs +++ b/core/src/consensus.rs @@ -366,7 +366,7 @@ impl Tower { last_voted_slot_in_bank: Option, ) -> Vote { let vote = Vote::new(vec![slot], hash); - local_vote_state.process_vote_unchecked(&vote); + local_vote_state.process_vote_unchecked(vote); let slots = if let Some(last_voted_slot) = last_voted_slot_in_bank { local_vote_state .votes @@ -2276,7 +2276,7 @@ pub mod test { hash: Hash::default(), timestamp: None, }; - local.process_vote_unchecked(&vote); + local.process_vote_unchecked(vote); assert_eq!(local.votes.len(), 1); let vote = Tower::apply_vote_and_generate_vote_diff(&mut local, 1, Hash::default(), Some(0)); @@ -2292,7 +2292,7 @@ pub mod test { hash: Hash::default(), timestamp: None, }; - local.process_vote_unchecked(&vote); + local.process_vote_unchecked(vote); assert_eq!(local.votes.len(), 1); // First vote expired, so should be evicted from tower. Thus even with diff --git a/local-cluster/tests/local_cluster.rs b/local-cluster/tests/local_cluster.rs index d49dcc488f..35e77bb4fb 100644 --- a/local-cluster/tests/local_cluster.rs +++ b/local-cluster/tests/local_cluster.rs @@ -1946,7 +1946,6 @@ fn root_in_tower(tower_path: &Path, node_pubkey: &Pubkey) -> Option { // slot_hash expiry to 64 slots. #[test] -#[ignore] fn test_slot_hash_expiry() { solana_logger::setup_with_default(RUST_LOG_FILTER); solana_sdk::slot_hashes::set_entries_for_tests_only(64); diff --git a/programs/vote/src/vote_instruction.rs b/programs/vote/src/vote_instruction.rs index 1d637bfbff..8bc745e002 100644 --- a/programs/vote/src/vote_instruction.rs +++ b/programs/vote/src/vote_instruction.rs @@ -80,6 +80,9 @@ pub enum VoteError { #[error("New state contained too many votes")] TooManyVotes, + + #[error("every slot in the vote was older than the SlotHashes history")] + VotesTooOldAllFiltered, } impl DecodeError for VoteError { @@ -478,7 +481,14 @@ pub fn process_instruction( keyed_account_at_index(keyed_accounts, first_instruction_account + 2)?, invoke_context, )?; - vote_state::process_vote(me, &slot_hashes, &clock, &vote, &signers) + vote_state::process_vote( + me, + &slot_hashes, + &clock, + &vote, + &signers, + &invoke_context.feature_set, + ) } VoteInstruction::UpdateVoteState(vote_state_update) | VoteInstruction::UpdateVoteStateSwitch(vote_state_update, _) => { diff --git a/programs/vote/src/vote_state/mod.rs b/programs/vote/src/vote_state/mod.rs index 728fe17632..9c0f51f6a1 100644 --- a/programs/vote/src/vote_state/mod.rs +++ b/programs/vote/src/vote_state/mod.rs @@ -10,6 +10,7 @@ use { account_utils::State, clock::{Epoch, Slot, UnixTimestamp}, epoch_schedule::MAX_LEADER_SCHEDULE_EPOCH_OFFSET, + feature_set::{filter_votes_outside_slot_hashes, FeatureSet}, hash::Hash, instruction::InstructionError, keyed_account::KeyedAccount, @@ -384,7 +385,8 @@ impl VoteState { fn check_slots_are_valid( &self, - vote: &Vote, + vote_slots: &[Slot], + vote_hash: &Hash, slot_hashes: &[(Slot, Hash)], ) -> Result<(), VoteError> { // index into the vote's slots, sarting at the newest @@ -397,32 +399,32 @@ impl VoteState { // Note: // - // 1) `vote.slots` is sorted from oldest/smallest vote to newest/largest + // 1) `vote_slots` is sorted from oldest/smallest vote to newest/largest // vote, due to the way votes are applied to the vote state (newest votes // pushed to the back), but `slot_hashes` is sorted smallest to largest. // // 2) Conversely, `slot_hashes` is sorted from newest/largest vote to // the oldest/smallest vote - while i < vote.slots.len() && j > 0 { - // 1) increment `i` to find the smallest slot `s` in `vote.slots` + while i < vote_slots.len() && j > 0 { + // 1) increment `i` to find the smallest slot `s` in `vote_slots` // where `s` >= `last_voted_slot` if self .last_voted_slot() - .map_or(false, |last_voted_slot| vote.slots[i] <= last_voted_slot) + .map_or(false, |last_voted_slot| vote_slots[i] <= last_voted_slot) { i += 1; continue; } // 2) Find the hash for this slot `s`. - if vote.slots[i] != slot_hashes[j - 1].0 { + if vote_slots[i] != slot_hashes[j - 1].0 { // Decrement `j` to find newer slots j -= 1; continue; } // 3) Once the hash for `s` is found, bump `s` to the next slot - // in `vote.slots` and continue. + // in `vote_slots` and continue. i += 1; j -= 1; } @@ -430,30 +432,30 @@ impl VoteState { if j == slot_hashes.len() { // This means we never made it to steps 2) or 3) above, otherwise // `j` would have been decremented at least once. This means - // there are not slots in `vote` greater than `last_voted_slot` + // there are not slots in `vote_slots` greater than `last_voted_slot` debug!( - "{} dropped vote {:?} too old: {:?} ", - self.node_pubkey, vote, slot_hashes + "{} dropped vote slots {:?}, vote hash: {:?} slot hashes:SlotHash {:?}, too old ", + self.node_pubkey, vote_slots, vote_hash, slot_hashes ); return Err(VoteError::VoteTooOld); } - if i != vote.slots.len() { + if i != vote_slots.len() { // This means there existed some slot for which we couldn't find // a matching slot hash in step 2) info!( - "{} dropped vote {:?} failed to match slot: {:?}", - self.node_pubkey, vote, slot_hashes, + "{} dropped vote slots {:?} failed to match slot hashes: {:?}", + self.node_pubkey, vote_slots, slot_hashes, ); inc_new_counter_info!("dropped-vote-slot", 1); return Err(VoteError::SlotsMismatch); } - if slot_hashes[j].1 != vote.hash { - // This means the newest vote in the slot has a match that + if &slot_hashes[j].1 != vote_hash { + // This means the newest slot in the `vote_slots` has a match that // doesn't match the expected hash for that slot on this // fork warn!( - "{} dropped vote {:?} failed to match hash {} {}", - self.node_pubkey, vote, vote.hash, slot_hashes[j].1 + "{} dropped vote slots {:?} failed to match hash {} {}", + self.node_pubkey, vote_slots, vote_hash, slot_hashes[j].1 ); inc_new_counter_info!("dropped-vote-hash", 1); return Err(VoteError::SlotHashMismatch); @@ -636,13 +638,36 @@ impl VoteState { vote: &Vote, slot_hashes: &[SlotHash], epoch: Epoch, + feature_set: Option<&FeatureSet>, ) -> Result<(), VoteError> { if vote.slots.is_empty() { return Err(VoteError::EmptySlots); } - self.check_slots_are_valid(vote, slot_hashes)?; - vote.slots + let filtered_vote_slots = feature_set.and_then(|feature_set| { + if feature_set.is_active(&filter_votes_outside_slot_hashes::id()) { + let earliest_slot_in_history = + slot_hashes.last().map(|(slot, _hash)| *slot).unwrap_or(0); + Some( + vote.slots + .iter() + .filter(|slot| **slot >= earliest_slot_in_history) + .cloned() + .collect::>(), + ) + } else { + None + } + }); + + let vote_slots = filtered_vote_slots.as_ref().unwrap_or(&vote.slots); + if vote_slots.is_empty() { + return Err(VoteError::VotesTooOldAllFiltered); + } + + self.check_slots_are_valid(vote_slots, &vote.hash, slot_hashes)?; + + vote_slots .iter() .for_each(|s| self.process_next_vote_slot(*s, epoch)); Ok(()) @@ -701,9 +726,9 @@ impl VoteState { } /// "unchecked" functions used by tests and Tower - pub fn process_vote_unchecked(&mut self, vote: &Vote) { + pub fn process_vote_unchecked(&mut self, vote: Vote) { let slot_hashes: Vec<_> = vote.slots.iter().rev().map(|x| (*x, vote.hash)).collect(); - let _ignored = self.process_vote(vote, &slot_hashes, self.current_epoch()); + let _ignored = self.process_vote(&vote, &slot_hashes, self.current_epoch(), None); } #[cfg(test)] @@ -714,7 +739,7 @@ impl VoteState { } pub fn process_slot_vote_unchecked(&mut self, slot: Slot) { - self.process_vote_unchecked(&Vote::new(vec![slot], Hash::default())); + self.process_vote_unchecked(Vote::new(vec![slot], Hash::default())); } pub fn nth_recent_vote(&self, position: usize) -> Option<&Lockout> { @@ -1054,10 +1079,11 @@ pub fn process_vote( clock: &Clock, vote: &Vote, signers: &HashSet, + feature_set: &FeatureSet, ) -> Result<(), InstructionError> { let mut vote_state = verify_and_get_vote_state(vote_account, clock, signers)?; - vote_state.process_vote(vote, slot_hashes, clock.epoch)?; + vote_state.process_vote(vote, slot_hashes, clock.epoch, Some(feature_set))?; if let Some(timestamp) = vote.timestamp { vote.slots .iter() @@ -1086,7 +1112,7 @@ pub fn process_vote_state_update( hash: vote_state_update.hash, timestamp: vote_state_update.timestamp, }; - vote_state.check_slots_are_valid(&vote, slot_hashes)?; + vote_state.check_slots_are_valid(&vote.slots, &vote.hash, slot_hashes)?; } vote_state.process_new_vote_state( vote_state_update.lockouts, @@ -1287,8 +1313,9 @@ mod tests { epoch, ..Clock::default() }, - &vote.clone(), + vote, &signers, + &FeatureSet::default(), )?; StateMut::::state(&*vote_account.borrow()) .map(|versioned| versioned.convert_to_current()) @@ -1496,6 +1523,7 @@ mod tests { }, &vote, &signers, + &FeatureSet::default(), ); assert_eq!(res, Err(InstructionError::MissingRequiredSignature)); @@ -1512,6 +1540,7 @@ mod tests { }, &vote, &signers, + &FeatureSet::default(), ); assert_eq!(res, Ok(())); @@ -1636,6 +1665,7 @@ mod tests { }, &vote, &signers, + &FeatureSet::default(), ); assert_eq!(res, Err(InstructionError::MissingRequiredSignature)); @@ -1657,6 +1687,7 @@ mod tests { }, &vote, &signers, + &FeatureSet::default(), ); assert_eq!(res, Ok(())); } @@ -1844,8 +1875,14 @@ mod tests { let vote = Vote::new(slots, Hash::default()); let slot_hashes: Vec<_> = vote.slots.iter().rev().map(|x| (*x, vote.hash)).collect(); - assert_eq!(vote_state_a.process_vote(&vote, &slot_hashes, 0), Ok(())); - assert_eq!(vote_state_b.process_vote(&vote, &slot_hashes, 0), Ok(())); + assert_eq!( + vote_state_a.process_vote(&vote, &slot_hashes, 0, Some(&FeatureSet::default())), + Ok(()) + ); + assert_eq!( + vote_state_b.process_vote(&vote, &slot_hashes, 0, Some(&FeatureSet::default())), + Ok(()) + ); assert_eq!(recent_votes(&vote_state_a), recent_votes(&vote_state_b)); } @@ -1855,10 +1892,13 @@ mod tests { let vote = Vote::new(vec![0], Hash::default()); let slot_hashes: Vec<_> = vec![(0, vote.hash)]; - assert_eq!(vote_state.process_vote(&vote, &slot_hashes, 0), Ok(())); + assert_eq!( + vote_state.process_vote(&vote, &slot_hashes, 0, Some(&FeatureSet::default())), + Ok(()) + ); let recent = recent_votes(&vote_state); assert_eq!( - vote_state.process_vote(&vote, &slot_hashes, 0), + vote_state.process_vote(&vote, &slot_hashes, 0, Some(&FeatureSet::default())), Err(VoteError::VoteTooOld) ); assert_eq!(recent, recent_votes(&vote_state)); @@ -1870,7 +1910,7 @@ mod tests { let vote = Vote::new(vec![0], Hash::default()); assert_eq!( - vote_state.check_slots_are_valid(&vote, &[]), + vote_state.check_slots_are_valid(&vote.slots, &vote.hash, &[]), Err(VoteError::VoteTooOld) ); } @@ -1882,7 +1922,7 @@ mod tests { let vote = Vote::new(vec![0], Hash::default()); let slot_hashes: Vec<_> = vec![(*vote.slots.last().unwrap(), vote.hash)]; assert_eq!( - vote_state.check_slots_are_valid(&vote, &slot_hashes), + vote_state.check_slots_are_valid(&vote.slots, &vote.hash, &slot_hashes), Ok(()) ); } @@ -1894,7 +1934,7 @@ mod tests { let vote = Vote::new(vec![0], Hash::default()); let slot_hashes: Vec<_> = vec![(*vote.slots.last().unwrap(), hash(vote.hash.as_ref()))]; assert_eq!( - vote_state.check_slots_are_valid(&vote, &slot_hashes), + vote_state.check_slots_are_valid(&vote.slots, &vote.hash, &slot_hashes), Err(VoteError::SlotHashMismatch) ); } @@ -1906,7 +1946,7 @@ mod tests { let vote = Vote::new(vec![1], Hash::default()); let slot_hashes: Vec<_> = vec![(0, vote.hash)]; assert_eq!( - vote_state.check_slots_are_valid(&vote, &slot_hashes), + vote_state.check_slots_are_valid(&vote.slots, &vote.hash, &slot_hashes), Err(VoteError::SlotsMismatch) ); } @@ -1917,9 +1957,12 @@ mod tests { let vote = Vote::new(vec![0], Hash::default()); let slot_hashes: Vec<_> = vec![(*vote.slots.last().unwrap(), vote.hash)]; - assert_eq!(vote_state.process_vote(&vote, &slot_hashes, 0), Ok(())); assert_eq!( - vote_state.check_slots_are_valid(&vote, &slot_hashes), + vote_state.process_vote(&vote, &slot_hashes, 0, Some(&FeatureSet::default())), + Ok(()) + ); + assert_eq!( + vote_state.check_slots_are_valid(&vote.slots, &vote.hash, &slot_hashes), Err(VoteError::VoteTooOld) ); } @@ -1930,12 +1973,15 @@ mod tests { let vote = Vote::new(vec![0], Hash::default()); let slot_hashes: Vec<_> = vec![(*vote.slots.last().unwrap(), vote.hash)]; - assert_eq!(vote_state.process_vote(&vote, &slot_hashes, 0), Ok(())); + assert_eq!( + vote_state.process_vote(&vote, &slot_hashes, 0, Some(&FeatureSet::default())), + Ok(()) + ); let vote = Vote::new(vec![0, 1], Hash::default()); let slot_hashes: Vec<_> = vec![(1, vote.hash), (0, vote.hash)]; assert_eq!( - vote_state.check_slots_are_valid(&vote, &slot_hashes), + vote_state.check_slots_are_valid(&vote.slots, &vote.hash, &slot_hashes), Ok(()) ); } @@ -1946,12 +1992,15 @@ mod tests { let vote = Vote::new(vec![0], Hash::default()); let slot_hashes: Vec<_> = vec![(*vote.slots.last().unwrap(), vote.hash)]; - assert_eq!(vote_state.process_vote(&vote, &slot_hashes, 0), Ok(())); + assert_eq!( + vote_state.process_vote(&vote, &slot_hashes, 0, Some(&FeatureSet::default())), + Ok(()) + ); let vote = Vote::new(vec![1], Hash::default()); let slot_hashes: Vec<_> = vec![(1, vote.hash), (0, vote.hash)]; assert_eq!( - vote_state.check_slots_are_valid(&vote, &slot_hashes), + vote_state.check_slots_are_valid(&vote.slots, &vote.hash, &slot_hashes), Ok(()) ); } @@ -1961,7 +2010,7 @@ mod tests { let vote = Vote::new(vec![], Hash::default()); assert_eq!( - vote_state.process_vote(&vote, &[], 0), + vote_state.process_vote(&vote, &[], 0, Some(&FeatureSet::default())), Err(VoteError::EmptySlots) ); } @@ -3129,4 +3178,44 @@ mod tests { .unwrap(); assert_eq!(vote_state1.votes, good_votes); } + + #[test] + fn test_filter_old_votes() { + // Enable feature + let mut feature_set = FeatureSet::default(); + feature_set.activate(&filter_votes_outside_slot_hashes::id(), 0); + + let mut vote_state = VoteState::default(); + let old_vote_slot = 1; + let vote = Vote::new(vec![old_vote_slot], Hash::default()); + + // Vote with all slots that are all older than the SlotHashes history should + // error with `VotesTooOldAllFiltered` + let slot_hashes = vec![(3, Hash::new_unique()), (2, Hash::new_unique())]; + assert_eq!( + vote_state.process_vote(&vote, &slot_hashes, 0, Some(&feature_set),), + Err(VoteError::VotesTooOldAllFiltered) + ); + + // Vote with only some slots older than the SlotHashes history should + // filter out those older slots + let vote_slot = 2; + let vote_slot_hash = slot_hashes + .iter() + .find(|(slot, _hash)| *slot == vote_slot) + .unwrap() + .1; + + let vote = Vote::new(vec![old_vote_slot, vote_slot], vote_slot_hash); + vote_state + .process_vote(&vote, &slot_hashes, 0, Some(&feature_set)) + .unwrap(); + assert_eq!( + vote_state.votes.into_iter().collect::>(), + vec![Lockout { + slot: vote_slot, + confirmation_count: 1, + }] + ); + } } diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index c7c3dddae7..7879084185 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -299,6 +299,10 @@ pub mod require_rent_exempt_accounts { solana_sdk::declare_id!("BkFDxiJQWZXGTZaJQxH7wVEHkAmwCgSEVkrvswFfRJPD"); } +pub mod filter_votes_outside_slot_hashes { + solana_sdk::declare_id!("3gtZPqvPpsbXZVCx6hceMfWxtsmrjMzmg8C7PLKSxS2d"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -368,6 +372,7 @@ lazy_static! { (cap_accounts_data_len::id(), "cap the accounts data len"), (max_tx_account_locks::id(), "enforce max number of locked accounts per transaction"), (require_rent_exempt_accounts::id(), "require all new transaction accounts with data to be rent-exempt"), + (filter_votes_outside_slot_hashes::id(), "filter vote slots older than the slot hashes history"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()