diff --git a/programs/stake-tests/tests/test_move_stake_and_lamports.rs b/programs/stake-tests/tests/test_move_stake_and_lamports.rs index 7c3a66f4e46f2d..7c67db2d5520b9 100644 --- a/programs/stake-tests/tests/test_move_stake_and_lamports.rs +++ b/programs/stake-tests/tests/test_move_stake_and_lamports.rs @@ -74,13 +74,11 @@ impl Accounts { impl Default for Accounts { fn default() -> Self { - let vote_account = Keypair::new(); - Self { validator: Keypair::new(), voter: Keypair::new(), withdrawer: Keypair::new(), - vote_account, + vote_account: Keypair::new(), } } } @@ -943,7 +941,10 @@ async fn test_move_general_fail( move_dest_type: StakeLifecycle, move_lamports: bool, ) { - // clear the states that are only valid for move_lamports + // the test_matrix includes all valid source/dest combinations for MoveLamports + // we dont test invalid combinations because they would fail regardless of the fail cases we test here + // valid source/dest for MoveStake are a strict subset of MoveLamports + // source must be active, and dest must be active or inactive. so we skip the additional invalid MoveStake cases if !move_lamports && (move_source_type != StakeLifecycle::Active || move_dest_type == StakeLifecycle::Activating) @@ -1221,6 +1222,10 @@ async fn test_move_feature_gate_fail( move_dest_type: StakeLifecycle, move_lamports: bool, ) { + // the test_matrix includes all valid source/dest combinations for MoveLamports + // we dont test invalid combinations because they would fail regardless of the fail cases we test here + // valid source/dest for MoveStake are a strict subset of MoveLamports + // source must be active, and dest must be active or inactive. so we skip the additional invalid MoveStake cases if !move_lamports && (move_source_type != StakeLifecycle::Active || move_dest_type == StakeLifecycle::Activating) diff --git a/programs/stake/src/stake_instruction.rs b/programs/stake/src/stake_instruction.rs index e6cf2a2320c22e..1ca36e6fb13d18 100644 --- a/programs/stake/src/stake_instruction.rs +++ b/programs/stake/src/stake_instruction.rs @@ -359,7 +359,6 @@ declare_process_instruction!(Entrypoint, DEFAULT_COMPUTE_UNITS, |invoke_context| .get_feature_set() .is_active(&feature_set::move_stake_and_move_lamports_ixs::id()) { - instruction_context.check_number_of_instruction_accounts(2)?; let clock = get_sysvar_with_account_check::clock(invoke_context, instruction_context, 2)?; let stake_history = get_sysvar_with_account_check::stake_history( @@ -390,7 +389,6 @@ declare_process_instruction!(Entrypoint, DEFAULT_COMPUTE_UNITS, |invoke_context| .get_feature_set() .is_active(&feature_set::move_stake_and_move_lamports_ixs::id()) { - instruction_context.check_number_of_instruction_accounts(2)?; let clock = get_sysvar_with_account_check::clock(invoke_context, instruction_context, 2)?; let stake_history = get_sysvar_with_account_check::stake_history( diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index cf7e0047c09b26..556e80dbd728c6 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -824,95 +824,99 @@ pub fn move_stake( // destination must not be activating // if active, destination must be delegated to the same vote account as source // minimum delegations must be respected for any accounts that become/remain active - match source_merge_kind { - MergeKind::FullyActive(source_meta, mut source_stake) => { - let minimum_delegation = - crate::get_minimum_delegation(invoke_context.get_feature_set()); - - let source_effective_stake = source_stake.delegation.stake; - let source_final_stake = source_effective_stake - .checked_sub(lamports) - .ok_or(InstructionError::InvalidArgument)?; - - if source_final_stake != 0 && source_final_stake < minimum_delegation { - return Err(InstructionError::InvalidArgument); - } + let MergeKind::FullyActive(source_meta, mut source_stake) = source_merge_kind else { + return Err(InstructionError::InvalidAccountData); + }; - let destination_meta = match destination_merge_kind { - MergeKind::FullyActive(destination_meta, mut destination_stake) => { - if source_stake.delegation.voter_pubkey - != destination_stake.delegation.voter_pubkey - { - return Err(StakeError::VoteAddressMismatch.into()); - } + let minimum_delegation = crate::get_minimum_delegation(invoke_context.get_feature_set()); - let destination_effective_stake = destination_stake.delegation.stake; - let destination_final_stake = destination_effective_stake - .checked_add(lamports) - .ok_or(InstructionError::ArithmeticOverflow)?; + let source_effective_stake = source_stake.delegation.stake; + let source_final_stake = source_effective_stake + .checked_sub(lamports) + .ok_or(InstructionError::InvalidArgument)?; - if destination_final_stake < minimum_delegation { - return Err(InstructionError::InvalidArgument); - } + if source_final_stake != 0 && source_final_stake < minimum_delegation { + return Err(InstructionError::InvalidArgument); + } - merge_delegation_stake_and_credits_observed( - &mut destination_stake, - lamports, - source_stake.credits_observed, - )?; + let destination_meta = match destination_merge_kind { + MergeKind::FullyActive(destination_meta, mut destination_stake) => { + if source_stake.delegation.voter_pubkey != destination_stake.delegation.voter_pubkey { + return Err(StakeError::VoteAddressMismatch.into()); + } - destination_account.set_state(&StakeStateV2::Stake( - destination_meta, - destination_stake, - StakeFlags::empty(), - ))?; + let destination_effective_stake = destination_stake.delegation.stake; + let destination_final_stake = destination_effective_stake + .checked_add(lamports) + .ok_or(InstructionError::ArithmeticOverflow)?; - destination_meta - } - MergeKind::Inactive(destination_meta, _, _) => { - if lamports < minimum_delegation { - return Err(InstructionError::InvalidArgument); - } + if destination_final_stake < minimum_delegation { + return Err(InstructionError::InvalidArgument); + } - let mut destination_stake = source_stake; - destination_stake.delegation.stake = lamports; - destination_account.set_state(&StakeStateV2::Stake( - destination_meta, - destination_stake, - StakeFlags::empty(), - ))?; + merge_delegation_stake_and_credits_observed( + &mut destination_stake, + lamports, + source_stake.credits_observed, + )?; - destination_meta - } - _ => return Err(InstructionError::InvalidAccountData), - }; + // StakeFlags::empty() is valid here because the only existing stake flag, + // MUST_FULLY_ACTIVATE_BEFORE_DEACTIVATION_IS_PERMITTED, does not apply to active stakes + destination_account.set_state(&StakeStateV2::Stake( + destination_meta, + destination_stake, + StakeFlags::empty(), + ))?; - if source_final_stake == 0 { - source_account.set_state(&StakeStateV2::Initialized(source_meta))?; - } else { - source_stake.delegation.stake = source_final_stake; - source_account.set_state(&StakeStateV2::Stake( - source_meta, - source_stake, - StakeFlags::empty(), - ))?; + destination_meta + } + MergeKind::Inactive(destination_meta, _, _) => { + if lamports < minimum_delegation { + return Err(InstructionError::InvalidArgument); } - source_account.checked_sub_lamports(lamports)?; - destination_account.checked_add_lamports(lamports)?; + let mut destination_stake = source_stake; + destination_stake.delegation.stake = lamports; - // this should be impossible, but because we do all our math with delegations, best to guard it - if source_account.get_lamports() < source_meta.rent_exempt_reserve - || destination_account.get_lamports() < destination_meta.rent_exempt_reserve - { - ic_msg!( - invoke_context, - "Delegation calculations violated lamport balance assumptions" - ); - return Err(InstructionError::InvalidArgument); - } + // StakeFlags::empty() is valid here because the only existing stake flag, + // MUST_FULLY_ACTIVATE_BEFORE_DEACTIVATION_IS_PERMITTED, is cleared when a stake is activated + destination_account.set_state(&StakeStateV2::Stake( + destination_meta, + destination_stake, + StakeFlags::empty(), + ))?; + + destination_meta } _ => return Err(InstructionError::InvalidAccountData), + }; + + if source_final_stake == 0 { + source_account.set_state(&StakeStateV2::Initialized(source_meta))?; + } else { + source_stake.delegation.stake = source_final_stake; + + // StakeFlags::empty() is valid here because the only existing stake flag, + // MUST_FULLY_ACTIVATE_BEFORE_DEACTIVATION_IS_PERMITTED, does not apply to active stakes + source_account.set_state(&StakeStateV2::Stake( + source_meta, + source_stake, + StakeFlags::empty(), + ))?; + } + + source_account.checked_sub_lamports(lamports)?; + destination_account.checked_add_lamports(lamports)?; + + // this should be impossible, but because we do all our math with delegations, best to guard it + if source_account.get_lamports() < source_meta.rent_exempt_reserve + || destination_account.get_lamports() < destination_meta.rent_exempt_reserve + { + ic_msg!( + invoke_context, + "Delegation calculations violated lamport balance assumptions" + ); + return Err(InstructionError::InvalidArgument); } Ok(())