From 968b15849444d8621c90f5713a049ba9a61d9b62 Mon Sep 17 00:00:00 2001 From: Jon Cinque Date: Fri, 23 Dec 2022 12:24:39 +0100 Subject: [PATCH] vote: Prevent commission update in the second half of epochs (#29362) * vote: Prevent commission update in the second half of epochs * Address feedback * Fix tests * Make the feature enabled by single-contributor * Use a cooler pubkey --- Cargo.lock | 1 + programs/vote/Cargo.toml | 1 + programs/vote/src/vote_processor.rs | 31 +++++++++++++- programs/vote/src/vote_state/mod.rs | 64 ++++++++++++++++++++++++++++- sdk/program/src/vote/error.rs | 3 ++ sdk/src/feature_set.rs | 5 +++ 6 files changed, 102 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index be6ffbdd7b..554b41fa13 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6899,6 +6899,7 @@ dependencies = [ "solana-program 1.15.0", "solana-program-runtime", "solana-sdk 1.15.0", + "test-case", "thiserror", ] diff --git a/programs/vote/Cargo.toml b/programs/vote/Cargo.toml index 3f9720640d..481af6e26c 100644 --- a/programs/vote/Cargo.toml +++ b/programs/vote/Cargo.toml @@ -26,6 +26,7 @@ thiserror = "1.0" [dev-dependencies] solana-logger = { path = "../../logger", version = "=1.15.0" } +test-case = "2.2.2" [build-dependencies] rustc_version = "0.4" diff --git a/programs/vote/src/vote_processor.rs b/programs/vote/src/vote_processor.rs index a8b0f8e0a2..f09d6bf724 100644 --- a/programs/vote/src/vote_processor.rs +++ b/programs/vote/src/vote_processor.rs @@ -1,7 +1,7 @@ //! Vote program processor use { - crate::vote_state, + crate::{vote_error::VoteError, vote_state}, log::*, solana_program::vote::{instruction::VoteInstruction, program::id, state::VoteAuthorize}, solana_program_runtime::{ @@ -135,6 +135,16 @@ pub fn process_instruction( vote_state::update_validator_identity(&mut me, node_pubkey, &signers) } VoteInstruction::UpdateCommission(commission) => { + if invoke_context.feature_set.is_active( + &feature_set::commission_updates_only_allowed_in_first_half_of_epoch::id(), + ) { + let sysvar_cache = invoke_context.get_sysvar_cache(); + let epoch_schedule = sysvar_cache.get_epoch_schedule()?; + let clock = sysvar_cache.get_clock()?; + if !vote_state::is_commission_update_allowed(clock.slot, &epoch_schedule) { + return Err(VoteError::CommissionUpdateTooLate.into()); + } + } vote_state::update_commission(&mut me, commission, &signers) } VoteInstruction::Vote(vote) | VoteInstruction::VoteSwitch(vote, _) => { @@ -276,7 +286,10 @@ mod tests { hash::Hash, instruction::{AccountMeta, Instruction}, pubkey::Pubkey, - sysvar::{self, clock::Clock, rent::Rent, slot_hashes::SlotHashes}, + sysvar::{ + self, clock::Clock, epoch_schedule::EpochSchedule, rent::Rent, + slot_hashes::SlotHashes, + }, }, std::{collections::HashSet, str::FromStr}, }; @@ -344,6 +357,7 @@ mod tests { .map(|meta| meta.pubkey) .collect(); pubkeys.insert(sysvar::clock::id()); + pubkeys.insert(sysvar::epoch_schedule::id()); pubkeys.insert(sysvar::rent::id()); pubkeys.insert(sysvar::slot_hashes::id()); let transaction_accounts: Vec<_> = pubkeys @@ -353,6 +367,10 @@ mod tests { *pubkey, if sysvar::clock::check_id(pubkey) { account::create_account_shared_data_for_test(&Clock::default()) + } else if sysvar::epoch_schedule::check_id(pubkey) { + account::create_account_shared_data_for_test( + &EpochSchedule::without_warmup(), + ) } else if sysvar::slot_hashes::check_id(pubkey) { account::create_account_shared_data_for_test(&SlotHashes::default()) } else if sysvar::rent::check_id(pubkey) { @@ -667,6 +685,15 @@ mod tests { let transaction_accounts = vec![ (vote_pubkey, vote_account), (authorized_withdrawer, AccountSharedData::default()), + // Add the sysvar accounts so they're in the cache for mock processing + ( + sysvar::clock::id(), + account::create_account_shared_data_for_test(&Clock::default()), + ), + ( + sysvar::epoch_schedule::id(), + account::create_account_shared_data_for_test(&EpochSchedule::without_warmup()), + ), ]; let mut instruction_accounts = vec![ AccountMeta { diff --git a/programs/vote/src/vote_state/mod.rs b/programs/vote/src/vote_state/mod.rs index 634943e3ff..b695d67382 100644 --- a/programs/vote/src/vote_state/mod.rs +++ b/programs/vote/src/vote_state/mod.rs @@ -9,6 +9,7 @@ use { solana_sdk::{ account::{AccountSharedData, ReadableAccount, WritableAccount}, clock::{Epoch, Slot, UnixTimestamp}, + epoch_schedule::EpochSchedule, feature_set::{self, filter_votes_outside_slot_hashes, FeatureSet}, hash::Hash, instruction::InstructionError, @@ -797,6 +798,22 @@ pub fn update_commission( vote_account.set_state(&VoteStateVersions::new_current(vote_state)) } +/// Given the current slot and epoch schedule, determine if a commission change +/// is allowed +pub fn is_commission_update_allowed(slot: Slot, epoch_schedule: &EpochSchedule) -> bool { + // always allowed during warmup epochs + if let Some(relative_slot) = slot + .saturating_sub(epoch_schedule.first_normal_slot) + .checked_rem(epoch_schedule.slots_per_epoch) + { + // allowed up to the midpoint of the epoch + relative_slot.saturating_mul(2) <= epoch_schedule.slots_per_epoch + } else { + // no slots per epoch, just allow it, even though this should never happen + true + } +} + fn verify_authorized_signer( authorized: &Pubkey, signers: &HashSet, @@ -1020,8 +1037,12 @@ mod tests { use { super::*, crate::vote_state, - solana_sdk::{account::AccountSharedData, account_utils::StateMut, hash::hash}, + solana_sdk::{ + account::AccountSharedData, account_utils::StateMut, clock::DEFAULT_SLOTS_PER_EPOCH, + hash::hash, + }, std::cell::RefCell, + test_case::test_case, }; const MAX_RECENT_VOTES: usize = 16; @@ -2955,4 +2976,45 @@ mod tests { Err(VoteError::SlotHashMismatch), ); } + + #[test_case(0, true; "first slot")] + #[test_case(DEFAULT_SLOTS_PER_EPOCH / 2, true; "halfway through epoch")] + #[test_case(DEFAULT_SLOTS_PER_EPOCH / 2 + 1, false; "halfway through epoch plus one")] + #[test_case(DEFAULT_SLOTS_PER_EPOCH - 1, false; "last slot in epoch")] + #[test_case(DEFAULT_SLOTS_PER_EPOCH, true; "first slot in second epoch")] + fn test_epoch_half_check(slot: Slot, expected_allowed: bool) { + let epoch_schedule = EpochSchedule::without_warmup(); + assert_eq!( + is_commission_update_allowed(slot, &epoch_schedule), + expected_allowed + ); + } + + #[test] + fn test_warmup_epoch_half_check_with_warmup() { + let epoch_schedule = EpochSchedule::default(); + let first_normal_slot = epoch_schedule.first_normal_slot; + // first slot works + assert!(is_commission_update_allowed(0, &epoch_schedule)); + // right before first normal slot works, since all warmup slots allow + // commission updates + assert!(is_commission_update_allowed( + first_normal_slot - 1, + &epoch_schedule + )); + } + + #[test_case(0, true; "first slot")] + #[test_case(DEFAULT_SLOTS_PER_EPOCH / 2, true; "halfway through epoch")] + #[test_case(DEFAULT_SLOTS_PER_EPOCH / 2 + 1, false; "halfway through epoch plus one")] + #[test_case(DEFAULT_SLOTS_PER_EPOCH - 1, false; "last slot in epoch")] + #[test_case(DEFAULT_SLOTS_PER_EPOCH, true; "first slot in second epoch")] + fn test_epoch_half_check_with_warmup(slot: Slot, expected_allowed: bool) { + let epoch_schedule = EpochSchedule::default(); + let first_normal_slot = epoch_schedule.first_normal_slot; + assert_eq!( + is_commission_update_allowed(first_normal_slot + slot, &epoch_schedule), + expected_allowed + ); + } } diff --git a/sdk/program/src/vote/error.rs b/sdk/program/src/vote/error.rs index 568cfc9678..d5924302f1 100644 --- a/sdk/program/src/vote/error.rs +++ b/sdk/program/src/vote/error.rs @@ -66,6 +66,9 @@ pub enum VoteError { #[error("Cannot close vote account unless it stopped voting at least one full epoch ago")] ActiveVoteAccountClose, + + #[error("Cannot update commission at this point in the epoch")] + CommissionUpdateTooLate, } impl DecodeError for VoteError { diff --git a/sdk/src/feature_set.rs b/sdk/src/feature_set.rs index 560db47032..8e30dd3d6c 100644 --- a/sdk/src/feature_set.rs +++ b/sdk/src/feature_set.rs @@ -554,6 +554,10 @@ pub mod enable_program_redeployment_cooldown { solana_sdk::declare_id!("J4HFT8usBxpcF63y46t1upYobJgChmKyZPm5uTBRg25Z"); } +pub mod commission_updates_only_allowed_in_first_half_of_epoch { + solana_sdk::declare_id!("noRuG2kzACwgaY7TVmLRnUNPLKNVQE1fb7X55YWBehp"); +} + lazy_static! { /// Map of feature identifiers to user-visible description pub static ref FEATURE_NAMES: HashMap = [ @@ -687,6 +691,7 @@ lazy_static! { (cap_transaction_accounts_data_size::id(), "cap transaction accounts data size up to its compute unit limits #27839"), (enable_alt_bn128_syscall::id(), "add alt_bn128 syscalls #27961"), (enable_program_redeployment_cooldown::id(), "enable program redeployment cooldown #29135"), + (commission_updates_only_allowed_in_first_half_of_epoch::id(), "validator commission updates are only allowed in the first half of an epoch #29362"), /*************** ADD NEW FEATURES HERE ***************/ ] .iter()