diff --git a/programs/stake/src/stake_instruction.rs b/programs/stake/src/stake_instruction.rs index b7fa8e3fbf..27b383b094 100644 --- a/programs/stake/src/stake_instruction.rs +++ b/programs/stake/src/stake_instruction.rs @@ -591,12 +591,15 @@ pub fn process_instruction( } StakeInstruction::Merge => { let source_stake = &next_keyed_account(keyed_accounts)?; + let can_merge_expired_lockups = + invoke_context.is_feature_active(&feature_set::stake_program_v4::id()); me.merge( invoke_context, source_stake, &from_keyed_account::(next_keyed_account(keyed_accounts)?)?, &from_keyed_account::(next_keyed_account(keyed_accounts)?)?, &signers, + can_merge_expired_lockups, ) } diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index 06dc74905b..12deae4018 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -888,6 +888,7 @@ pub trait StakeAccount { clock: &Clock, stake_history: &StakeHistory, signers: &HashSet, + can_merge_expired_lockups: bool, ) -> Result<(), InstructionError>; fn withdraw( &self, @@ -1189,6 +1190,7 @@ impl<'a> StakeAccount for KeyedAccount<'a> { clock: &Clock, stake_history: &StakeHistory, signers: &HashSet, + can_merge_expired_lockups: bool, ) -> Result<(), InstructionError> { // Ensure source isn't spoofed if source_account.owner()? != id() { @@ -1211,8 +1213,16 @@ impl<'a> StakeAccount for KeyedAccount<'a> { let source_merge_kind = MergeKind::get_if_mergeable(invoke_context, source_account, clock, stake_history)?; + let clock = if can_merge_expired_lockups { + Some(clock) + } else { + None + }; + ic_msg!(invoke_context, "Merging stake accounts"); - if let Some(merged_state) = stake_merge_kind.merge(invoke_context, source_merge_kind)? { + if let Some(merged_state) = + stake_merge_kind.merge(invoke_context, source_merge_kind, clock)? + { self.set_state(&merged_state)?; } @@ -1376,13 +1386,24 @@ impl MergeKind { invoke_context: &dyn InvokeContext, stake: &Meta, source: &Meta, + clock: Option<&Clock>, ) -> Result<(), InstructionError> { + let can_merge_lockups = match clock { + // pre-v4 behavior. lockups must match, even when expired + None => stake.lockup == source.lockup, + // v4 behavior. lockups may mismatch so long as both have expired + Some(clock) => { + stake.lockup == source.lockup + || (!stake.lockup.is_in_force(clock, None) + && !source.lockup.is_in_force(clock, None)) + } + }; // `rent_exempt_reserve` has no bearing on the mergeability of accounts, // as the source account will be culled by runtime once the operation // succeeds. Considering it here would needlessly prevent merging stake // accounts with differing data lengths, which already exist in the wild // due to an SDK bug - if stake.authorized == source.authorized && stake.lockup == source.lockup { + if stake.authorized == source.authorized && can_merge_lockups { Ok(()) } else { ic_msg!(invoke_context, "Unable to merge due to metadata mismatch"); @@ -1435,8 +1456,9 @@ impl MergeKind { self, invoke_context: &dyn InvokeContext, source: Self, + clock: Option<&Clock>, ) -> Result, InstructionError> { - Self::metas_can_merge(invoke_context, self.meta(), source.meta())?; + Self::metas_can_merge(invoke_context, self.meta(), source.meta(), clock)?; self.active_stake() .zip(source.active_stake()) .map(|(stake, source)| Self::active_stakes_can_merge(invoke_context, stake, source)) @@ -5362,7 +5384,8 @@ mod tests { &source_stake_keyed_account, &Clock::default(), &StakeHistory::default(), - &HashSet::new() + &HashSet::new(), + false, ), Err(InstructionError::MissingRequiredSignature) ); @@ -5373,7 +5396,8 @@ mod tests { &source_stake_keyed_account, &Clock::default(), &StakeHistory::default(), - &signers + &signers, + false, ), Ok(()) ); @@ -5465,6 +5489,7 @@ mod tests { &Clock::default(), &StakeHistory::default(), &signers, + false, ), Err(InstructionError::InvalidArgument), ); @@ -5522,6 +5547,7 @@ mod tests { &Clock::default(), &StakeHistory::default(), &wrong_signers, + false, ), Err(InstructionError::MissingRequiredSignature) ); @@ -5533,6 +5559,7 @@ mod tests { &Clock::default(), &StakeHistory::default(), &signers, + false, ), Err(StakeError::MergeMismatch.into()) ); @@ -5585,6 +5612,7 @@ mod tests { &Clock::default(), &StakeHistory::default(), &signers, + false, ), Err(InstructionError::InvalidAccountData) ); @@ -5633,7 +5661,8 @@ mod tests { &source_stake_keyed_account, &Clock::default(), &StakeHistory::default(), - &signers + &signers, + false, ), Err(InstructionError::IncorrectProgramId) ); @@ -5730,6 +5759,7 @@ mod tests { clock, stake_history, signers, + false, ); if result.is_ok() { assert_eq!(test_source_keyed.state(), Ok(StakeState::Uninitialized),); @@ -6461,11 +6491,19 @@ mod tests { &good_delegation ) .is_err()); + } + #[test] + fn test_metas_can_merge_pre_v4() { + let invoke_context = MockInvokeContext::default(); // Identical Metas can merge - assert!( - MergeKind::metas_can_merge(&invoke_context, &Meta::default(), &Meta::default()).is_ok() - ); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &Meta::default(), + &Meta::default(), + None, + ) + .is_ok()); let mismatched_rent_exempt_reserve_ok = Meta { rent_exempt_reserve: 42, @@ -6478,13 +6516,15 @@ mod tests { assert!(MergeKind::metas_can_merge( &invoke_context, &Meta::default(), - &mismatched_rent_exempt_reserve_ok + &mismatched_rent_exempt_reserve_ok, + None, ) .is_ok()); assert!(MergeKind::metas_can_merge( &invoke_context, &mismatched_rent_exempt_reserve_ok, - &Meta::default() + &Meta::default(), + None, ) .is_ok()); @@ -6502,13 +6542,15 @@ mod tests { assert!(MergeKind::metas_can_merge( &invoke_context, &Meta::default(), - &mismatched_authorized_fails + &mismatched_authorized_fails, + None, ) .is_err()); assert!(MergeKind::metas_can_merge( &invoke_context, &mismatched_authorized_fails, - &Meta::default() + &Meta::default(), + None, ) .is_err()); @@ -6524,17 +6566,165 @@ mod tests { assert!(MergeKind::metas_can_merge( &invoke_context, &Meta::default(), - &mismatched_lockup_fails + &mismatched_lockup_fails, + None, ) .is_err()); assert!(MergeKind::metas_can_merge( &invoke_context, &mismatched_lockup_fails, - &Meta::default() + &Meta::default(), + None, ) .is_err()); } + #[test] + fn test_metas_can_merge_v4() { + let invoke_context = MockInvokeContext::default(); + // Identical Metas can merge + assert!(MergeKind::metas_can_merge( + &invoke_context, + &Meta::default(), + &Meta::default(), + Some(&Clock::default()) + ) + .is_ok()); + + let mismatched_rent_exempt_reserve_ok = Meta { + rent_exempt_reserve: 42, + ..Meta::default() + }; + assert_ne!( + mismatched_rent_exempt_reserve_ok.rent_exempt_reserve, + Meta::default().rent_exempt_reserve, + ); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &Meta::default(), + &mismatched_rent_exempt_reserve_ok, + Some(&Clock::default()) + ) + .is_ok()); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &mismatched_rent_exempt_reserve_ok, + &Meta::default(), + Some(&Clock::default()) + ) + .is_ok()); + + let mismatched_authorized_fails = Meta { + authorized: Authorized { + staker: Pubkey::new_unique(), + withdrawer: Pubkey::new_unique(), + }, + ..Meta::default() + }; + assert_ne!( + mismatched_authorized_fails.authorized, + Meta::default().authorized, + ); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &Meta::default(), + &mismatched_authorized_fails, + Some(&Clock::default()) + ) + .is_err()); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &mismatched_authorized_fails, + &Meta::default(), + Some(&Clock::default()) + ) + .is_err()); + + let lockup1_timestamp = 42; + let lockup2_timestamp = 4242; + let lockup1_epoch = 4; + let lockup2_epoch = 42; + let metas_with_lockup1 = Meta { + lockup: Lockup { + unix_timestamp: lockup1_timestamp, + epoch: lockup1_epoch, + custodian: Pubkey::new_unique(), + }, + ..Meta::default() + }; + let metas_with_lockup2 = Meta { + lockup: Lockup { + unix_timestamp: lockup2_timestamp, + epoch: lockup2_epoch, + custodian: Pubkey::new_unique(), + }, + ..Meta::default() + }; + + // Mismatched lockups fail when both in force + assert_ne!(metas_with_lockup1.lockup, Meta::default().lockup); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &metas_with_lockup1, + &metas_with_lockup2, + Some(&Clock::default()) + ) + .is_err()); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &metas_with_lockup2, + &metas_with_lockup1, + Some(&Clock::default()) + ) + .is_err()); + + let clock = Clock { + epoch: lockup1_epoch + 1, + unix_timestamp: lockup1_timestamp + 1, + ..Clock::default() + }; + + // Mismatched lockups fail when either in force + assert_ne!(metas_with_lockup1.lockup, Meta::default().lockup); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &metas_with_lockup1, + &metas_with_lockup2, + Some(&clock) + ) + .is_err()); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &metas_with_lockup2, + &metas_with_lockup1, + Some(&clock) + ) + .is_err()); + + let clock = Clock { + epoch: lockup2_epoch + 1, + unix_timestamp: lockup2_timestamp + 1, + ..Clock::default() + }; + + // Mismatched lockups succeed when both expired + assert_ne!(metas_with_lockup1.lockup, Meta::default().lockup); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &metas_with_lockup1, + &metas_with_lockup2, + Some(&clock) + ) + .is_ok()); + assert!(MergeKind::metas_can_merge( + &invoke_context, + &metas_with_lockup2, + &metas_with_lockup1, + Some(&clock) + ) + .is_ok()); + } + #[test] fn test_merge_kind_get_if_mergeable() { let invoke_context = MockInvokeContext::default(); @@ -6790,37 +6980,37 @@ mod tests { assert_eq!( inactive .clone() - .merge(&invoke_context, inactive.clone()) + .merge(&invoke_context, inactive.clone(), None) .unwrap(), None ); assert_eq!( inactive .clone() - .merge(&invoke_context, activation_epoch.clone()) + .merge(&invoke_context, activation_epoch.clone(), None) .unwrap(), None ); assert!(inactive .clone() - .merge(&invoke_context, fully_active.clone()) + .merge(&invoke_context, fully_active.clone(), None) .is_err()); assert!(activation_epoch .clone() - .merge(&invoke_context, fully_active.clone()) + .merge(&invoke_context, fully_active.clone(), None) .is_err()); assert!(fully_active .clone() - .merge(&invoke_context, inactive.clone()) + .merge(&invoke_context, inactive.clone(), None) .is_err()); assert!(fully_active .clone() - .merge(&invoke_context, activation_epoch.clone()) + .merge(&invoke_context, activation_epoch.clone(), None) .is_err()); let new_state = activation_epoch .clone() - .merge(&invoke_context, inactive) + .merge(&invoke_context, inactive, None) .unwrap() .unwrap(); let delegation = new_state.delegation().unwrap(); @@ -6828,7 +7018,7 @@ mod tests { let new_state = activation_epoch .clone() - .merge(&invoke_context, activation_epoch) + .merge(&invoke_context, activation_epoch, None) .unwrap() .unwrap(); let delegation = new_state.delegation().unwrap(); @@ -6839,7 +7029,7 @@ mod tests { let new_state = fully_active .clone() - .merge(&invoke_context, fully_active) + .merge(&invoke_context, fully_active, None) .unwrap() .unwrap(); let delegation = new_state.delegation().unwrap();