From cbb5381a98dbe91103e753f68501bb1832ab0d28 Mon Sep 17 00:00:00 2001 From: Ashwin Sekar Date: Wed, 28 Jun 2023 21:26:58 -0700 Subject: [PATCH] feature flag cleanup: vote_state_update_root_fix (#32321) --- programs/vote/src/vote_state/mod.rs | 128 ++++++++-------------------- 1 file changed, 34 insertions(+), 94 deletions(-) diff --git a/programs/vote/src/vote_state/mod.rs b/programs/vote/src/vote_state/mod.rs index ce60715364..5aa0f95409 100644 --- a/programs/vote/src/vote_state/mod.rs +++ b/programs/vote/src/vote_state/mod.rs @@ -179,7 +179,6 @@ fn check_update_vote_state_slots_are_valid( vote_state: &VoteState, vote_state_update: &mut VoteStateUpdate, slot_hashes: &[(Slot, Hash)], - feature_set: Option<&FeatureSet>, ) -> Result<(), VoteError> { if vote_state_update.lockouts.is_empty() { return Err(VoteError::EmptySlots); @@ -211,10 +210,6 @@ fn check_update_vote_state_slots_are_valid( } // Check if the proposed root is too old - let is_root_fix_enabled = feature_set - .map(|feature_set| feature_set.is_active(&feature_set::vote_state_update_root_fix::id())) - .unwrap_or(false); - let original_proposed_root = vote_state_update.root; if let Some(new_proposed_root) = original_proposed_root { // If the new proposed root `R` is less than the earliest slot hash in the history @@ -222,22 +217,20 @@ fn check_update_vote_state_slots_are_valid( // the root to the latest vote in the current vote that's less than R. if earliest_slot_hash_in_history > new_proposed_root { vote_state_update.root = vote_state.root_slot; - if is_root_fix_enabled { - let mut prev_slot = Slot::MAX; - let current_root = vote_state_update.root; - for vote in vote_state.votes.iter().rev() { - let is_slot_bigger_than_root = current_root - .map(|current_root| vote.slot() > current_root) - .unwrap_or(true); - // Ensure we're iterating from biggest to smallest vote in the - // current vote state - assert!(vote.slot() < prev_slot && is_slot_bigger_than_root); - if vote.slot() <= new_proposed_root { - vote_state_update.root = Some(vote.slot()); - break; - } - prev_slot = vote.slot(); + let mut prev_slot = Slot::MAX; + let current_root = vote_state_update.root; + for vote in vote_state.votes.iter().rev() { + let is_slot_bigger_than_root = current_root + .map(|current_root| vote.slot() > current_root) + .unwrap_or(true); + // Ensure we're iterating from biggest to smallest vote in the + // current vote state + assert!(vote.slot() < prev_slot && is_slot_bigger_than_root); + if vote.slot() <= new_proposed_root { + vote_state_update.root = Some(vote.slot()); + break; } + prev_slot = vote.slot(); } } } @@ -304,21 +297,14 @@ fn check_update_vote_state_slots_are_valid( vote_state_update_indexes_to_filter.push(vote_state_update_index); } if let Some(new_proposed_root) = root_to_check { - if is_root_fix_enabled { - // 1. Because `root_to_check.is_some()`, then we know that - // we haven't checked the root yet in this loop, so - // `proposed_vote_slot` == `new_proposed_root` == `vote_state_update.root`. - assert_eq!(new_proposed_root, proposed_vote_slot); - // 2. We know from the assert earlier in the function that - // `proposed_vote_slot < earliest_slot_hash_in_history`, - // so from 1. we know that `new_proposed_root < earliest_slot_hash_in_history`. - assert!(new_proposed_root < earliest_slot_hash_in_history); - } else { - // If the vote state update has a root < earliest_slot_hash_in_history - // then we use the current root. The only case where this can happen - // is if the current root itself is not in slot hashes. - assert!(vote_state.root_slot.unwrap() < earliest_slot_hash_in_history); - } + // 1. Because `root_to_check.is_some()`, then we know that + // we haven't checked the root yet in this loop, so + // `proposed_vote_slot` == `new_proposed_root` == `vote_state_update.root`. + assert_eq!(new_proposed_root, proposed_vote_slot); + // 2. We know from the assert earlier in the function that + // `proposed_vote_slot < earliest_slot_hash_in_history`, + // so from 1. we know that `new_proposed_root < earliest_slot_hash_in_history`. + assert!(new_proposed_root < earliest_slot_hash_in_history); root_to_check = None; } else { vote_state_update_index = vote_state_update_index.checked_add(1).expect( @@ -1054,12 +1040,7 @@ pub fn do_process_vote_state_update( mut vote_state_update: VoteStateUpdate, feature_set: Option<&FeatureSet>, ) -> Result<(), VoteError> { - check_update_vote_state_slots_are_valid( - vote_state, - &mut vote_state_update, - slot_hashes, - feature_set, - )?; + check_update_vote_state_slots_are_valid(vote_state, &mut vote_state_update, slot_hashes)?; process_new_vote_state( vote_state, vote_state_update.lockouts, @@ -2337,7 +2318,6 @@ mod tests { &empty_vote_state, &mut vote_state_update, &empty_slot_hashes, - Some(&FeatureSet::all_enabled()) ), Err(VoteError::EmptySlots), ); @@ -2349,7 +2329,6 @@ mod tests { &empty_vote_state, &mut vote_state_update, &empty_slot_hashes, - Some(&FeatureSet::all_enabled()) ), Err(VoteError::SlotsMismatch), ); @@ -2369,7 +2348,6 @@ mod tests { &vote_state, &mut vote_state_update, &slot_hashes, - Some(&FeatureSet::all_enabled()) ), Err(VoteError::VoteTooOld), ); @@ -2385,7 +2363,6 @@ mod tests { &vote_state, &mut vote_state_update, &slot_hashes, - Some(&FeatureSet::all_enabled()), ), Err(VoteError::VoteTooOld), ); @@ -2436,13 +2413,8 @@ mod tests { let mut vote_state_update = VoteStateUpdate::from(vote_state_update_slots_and_lockouts); vote_state_update.hash = vote_state_update_hash; vote_state_update.root = Some(vote_state_update_root); - check_update_vote_state_slots_are_valid( - &vote_state, - &mut vote_state_update, - &slot_hashes, - Some(&FeatureSet::all_enabled()), - ) - .unwrap(); + check_update_vote_state_slots_are_valid(&vote_state, &mut vote_state_update, &slot_hashes) + .unwrap(); assert_eq!(vote_state_update.root, expected_root); // The proposed root slot should become the biggest slot in the current vote state less than @@ -2610,7 +2582,6 @@ mod tests { &vote_state, &mut vote_state_update, &slot_hashes, - Some(&FeatureSet::all_enabled()) ), Err(VoteError::SlotsNotOrdered), ); @@ -2623,7 +2594,6 @@ mod tests { &vote_state, &mut vote_state_update, &slot_hashes, - Some(&FeatureSet::all_enabled()), ), Err(VoteError::SlotsNotOrdered), ); @@ -2653,13 +2623,8 @@ mod tests { (vote_slot, 3), ]); vote_state_update.hash = vote_slot_hash; - check_update_vote_state_slots_are_valid( - &vote_state, - &mut vote_state_update, - &slot_hashes, - Some(&FeatureSet::all_enabled()), - ) - .unwrap(); + check_update_vote_state_slots_are_valid(&vote_state, &mut vote_state_update, &slot_hashes) + .unwrap(); // Check the earlier slot was filtered out assert_eq!( @@ -2704,13 +2669,8 @@ mod tests { let mut vote_state_update = VoteStateUpdate::from(vec![(existing_older_than_history_slot, 3), (vote_slot, 2)]); vote_state_update.hash = vote_slot_hash; - check_update_vote_state_slots_are_valid( - &vote_state, - &mut vote_state_update, - &slot_hashes, - Some(&FeatureSet::all_enabled()), - ) - .unwrap(); + check_update_vote_state_slots_are_valid(&vote_state, &mut vote_state_update, &slot_hashes) + .unwrap(); // Check the earlier slot was *NOT* filtered out assert_eq!(vote_state_update.lockouts.len(), 2); assert_eq!( @@ -2768,13 +2728,8 @@ mod tests { (vote_slot, 1), ]); vote_state_update.hash = vote_slot_hash; - check_update_vote_state_slots_are_valid( - &vote_state, - &mut vote_state_update, - &slot_hashes, - Some(&FeatureSet::all_enabled()), - ) - .unwrap(); + check_update_vote_state_slots_are_valid(&vote_state, &mut vote_state_update, &slot_hashes) + .unwrap(); assert_eq!(vote_state_update.lockouts.len(), 3); assert_eq!( vote_state_update @@ -2826,7 +2781,6 @@ mod tests { &vote_state, &mut vote_state_update, &slot_hashes, - Some(&FeatureSet::all_enabled()) ), Err(VoteError::SlotsMismatch), ); @@ -2846,7 +2800,6 @@ mod tests { &vote_state, &mut vote_state_update, &slot_hashes, - Some(&FeatureSet::all_enabled()) ), Err(VoteError::SlotsMismatch), ); @@ -2881,7 +2834,6 @@ mod tests { &vote_state, &mut vote_state_update, &slot_hashes, - Some(&FeatureSet::all_enabled()) ), Err(VoteError::RootOnDifferentFork), ); @@ -2906,7 +2858,6 @@ mod tests { &vote_state, &mut vote_state_update, &slot_hashes, - Some(&FeatureSet::all_enabled()) ), Err(VoteError::SlotsMismatch), ); @@ -2931,13 +2882,8 @@ mod tests { let mut vote_state_update = VoteStateUpdate::from(vec![(2, 4), (4, 3), (6, 2), (vote_slot, 1)]); vote_state_update.hash = vote_slot_hash; - check_update_vote_state_slots_are_valid( - &vote_state, - &mut vote_state_update, - &slot_hashes, - Some(&FeatureSet::all_enabled()), - ) - .unwrap(); + check_update_vote_state_slots_are_valid(&vote_state, &mut vote_state_update, &slot_hashes) + .unwrap(); // Nothing in the update should have been filtered out assert_eq!( @@ -2982,13 +2928,8 @@ mod tests { .1; let mut vote_state_update = VoteStateUpdate::from(vec![(4, 2), (vote_slot, 1)]); vote_state_update.hash = vote_slot_hash; - check_update_vote_state_slots_are_valid( - &vote_state, - &mut vote_state_update, - &slot_hashes, - Some(&FeatureSet::all_enabled()), - ) - .unwrap(); + check_update_vote_state_slots_are_valid(&vote_state, &mut vote_state_update, &slot_hashes) + .unwrap(); // Nothing in the update should have been filtered out assert_eq!( @@ -3037,7 +2978,6 @@ mod tests { &vote_state, &mut vote_state_update, &slot_hashes, - Some(&FeatureSet::all_enabled()) ), Err(VoteError::SlotHashMismatch), );