From 28866614159d88352117b76e951e4b0ff732b348 Mon Sep 17 00:00:00 2001 From: HaoranYi Date: Mon, 24 Jul 2023 14:42:38 +0000 Subject: [PATCH] fix stake deactivation in the same epoch after redelegation bug add tests refactor common code into fn avoid early return add feature gate for the new stake redelegate behavior move stake tests out of cli add stake-program-test crate reimplemnt stake test with program-test remove stake-program-test crate reviews add setup.rs remove clippy reveiws --- program-test/tests/setup.rs | 89 +++++++++++ program-test/tests/stake.rs | 203 ++++++++++++++++++++++++ program-test/tests/warp.rs | 94 ++--------- programs/stake/src/stake_instruction.rs | 7 +- programs/stake/src/stake_state.rs | 57 ++++++- sdk/program/src/stake/instruction.rs | 1 - sdk/program/src/stake/state.rs | 39 +++++ 7 files changed, 398 insertions(+), 92 deletions(-) create mode 100644 program-test/tests/setup.rs create mode 100644 program-test/tests/stake.rs diff --git a/program-test/tests/setup.rs b/program-test/tests/setup.rs new file mode 100644 index 00000000000000..60c42372b6e692 --- /dev/null +++ b/program-test/tests/setup.rs @@ -0,0 +1,89 @@ +use { + solana_program_test::ProgramTestContext, + solana_sdk::{ + pubkey::Pubkey, + rent::Rent, + signature::{Keypair, Signer}, + stake::{ + instruction as stake_instruction, + state::{Authorized, Lockup}, + }, + system_instruction, system_program, + transaction::Transaction, + }, + solana_vote_program::{ + vote_instruction, + vote_state::{self, VoteInit, VoteState}, + }, +}; + +pub async fn setup_stake( + context: &mut ProgramTestContext, + user: &Keypair, + vote_address: &Pubkey, + stake_lamports: u64, +) -> Pubkey { + let stake_keypair = Keypair::new(); + let transaction = Transaction::new_signed_with_payer( + &stake_instruction::create_account_and_delegate_stake( + &context.payer.pubkey(), + &stake_keypair.pubkey(), + vote_address, + &Authorized::auto(&user.pubkey()), + &Lockup::default(), + stake_lamports, + ), + Some(&context.payer.pubkey()), + &vec![&context.payer, &stake_keypair, user], + context.last_blockhash, + ); + context + .banks_client + .process_transaction(transaction) + .await + .unwrap(); + stake_keypair.pubkey() +} + +pub async fn setup_vote(context: &mut ProgramTestContext) -> Pubkey { + let mut instructions = vec![]; + let validator_keypair = Keypair::new(); + instructions.push(system_instruction::create_account( + &context.payer.pubkey(), + &validator_keypair.pubkey(), + Rent::default().minimum_balance(0), + 0, + &system_program::id(), + )); + let vote_lamports = Rent::default().minimum_balance(VoteState::size_of()); + let vote_keypair = Keypair::new(); + let user_keypair = Keypair::new(); + instructions.append(&mut vote_instruction::create_account_with_config( + &context.payer.pubkey(), + &vote_keypair.pubkey(), + &VoteInit { + node_pubkey: validator_keypair.pubkey(), + authorized_voter: user_keypair.pubkey(), + ..VoteInit::default() + }, + vote_lamports, + vote_instruction::CreateVoteAccountConfig { + space: vote_state::VoteStateVersions::vote_state_size_of(true) as u64, + ..vote_instruction::CreateVoteAccountConfig::default() + }, + )); + + let transaction = Transaction::new_signed_with_payer( + &instructions, + Some(&context.payer.pubkey()), + &vec![&context.payer, &validator_keypair, &vote_keypair], + context.last_blockhash, + ); + context + .banks_client + .process_transaction(transaction) + .await + .unwrap(); + + vote_keypair.pubkey() +} diff --git a/program-test/tests/stake.rs b/program-test/tests/stake.rs new file mode 100644 index 00000000000000..70bbef3ed135e1 --- /dev/null +++ b/program-test/tests/stake.rs @@ -0,0 +1,203 @@ +#![allow(clippy::arithmetic_side_effects)] + +mod setup; + +use { + setup::{setup_stake, setup_vote}, + solana_program_test::ProgramTest, + solana_sdk::{ + instruction::InstructionError, + signature::{Keypair, Signer}, + stake::{instruction as stake_instruction, instruction::StakeError}, + transaction::{Transaction, TransactionError}, + }, +}; + +#[derive(PartialEq)] +enum PendingStakeActivationTestFlag { + MergeActive, + MergeInactive, + NoMerge, +} + +async fn test_stake_redelegation_pending_activation(merge_flag: PendingStakeActivationTestFlag) { + let program_test = ProgramTest::default(); + let mut context = program_test.start_with_context().await; + + // 1. create first vote accounts + context.warp_to_slot(100).unwrap(); + let vote_address = setup_vote(&mut context).await; + + // 1.1 advance to normal epoch + let first_normal_slot = context.genesis_config().epoch_schedule.first_normal_slot; + let slots_per_epoch = context.genesis_config().epoch_schedule.slots_per_epoch; + let mut current_slot = first_normal_slot + slots_per_epoch; + context.warp_to_slot(current_slot).unwrap(); + context.warp_forward_force_reward_interval_end().unwrap(); + + // 2. create first stake account and delegate to first vote_address + let stake_lamports = 50_000_000_000; + let user_keypair = Keypair::new(); + let stake_address = + setup_stake(&mut context, &user_keypair, &vote_address, stake_lamports).await; + + // 2.1 advance to new epoch so that the stake is activated. + current_slot += slots_per_epoch; + context.warp_to_slot(current_slot).unwrap(); + context.warp_forward_force_reward_interval_end().unwrap(); + + // 2.2 stake is now activated and can't withdrawal directly + let transaction = Transaction::new_signed_with_payer( + &[stake_instruction::withdraw( + &stake_address, + &user_keypair.pubkey(), + &solana_sdk::pubkey::new_rand(), + 1, + None, + )], + Some(&context.payer.pubkey()), + &vec![&context.payer, &user_keypair], + context.last_blockhash, + ); + let r = context.banks_client.process_transaction(transaction).await; + assert_eq!( + r.unwrap_err().unwrap(), + TransactionError::InstructionError(0, InstructionError::InsufficientFunds) + ); + + // 3. create 2nd vote account + let vote_address2 = setup_vote(&mut context).await; + + // 3.1 relegate 1st stake account to 2nd stake account. + let stake_keypair2 = Keypair::new(); + let stake_address2 = stake_keypair2.pubkey(); + let transaction = Transaction::new_signed_with_payer( + &stake_instruction::redelegate( + &stake_address, + &user_keypair.pubkey(), + &vote_address2, + &stake_address2, + ), + Some(&context.payer.pubkey()), + &vec![&context.payer, &user_keypair, &stake_keypair2], + context.last_blockhash, + ); + context + .banks_client + .process_transaction(transaction) + .await + .unwrap(); + + if merge_flag != PendingStakeActivationTestFlag::NoMerge { + // 3.2 create 3rd to-merge stake account + let stake_address3 = + setup_stake(&mut context, &user_keypair, &vote_address2, stake_lamports).await; + + // 3.2.1 deactivate merge stake account + if merge_flag == PendingStakeActivationTestFlag::MergeInactive { + let transaction = Transaction::new_signed_with_payer( + &[stake_instruction::deactivate_stake( + &stake_address3, + &user_keypair.pubkey(), + )], + Some(&context.payer.pubkey()), + &vec![&context.payer, &user_keypair], + context.last_blockhash, + ); + context + .banks_client + .process_transaction(transaction) + .await + .unwrap(); + } + + // 3.2.2 merge 3rd stake account to 2nd stake account. However, it should not clear the pending stake activation flags on stake_account2. + let transaction = Transaction::new_signed_with_payer( + &stake_instruction::merge(&stake_address2, &stake_address3, &user_keypair.pubkey()), + Some(&context.payer.pubkey()), + &vec![&context.payer, &user_keypair], + context.last_blockhash, + ); + context + .banks_client + .process_transaction(transaction) + .await + .unwrap(); + } + + // 3.3 deactivate 2nd stake account should fail because of pending stake activation. + let transaction = Transaction::new_signed_with_payer( + &[stake_instruction::deactivate_stake( + &stake_address2, + &user_keypair.pubkey(), + )], + Some(&context.payer.pubkey()), + &vec![&context.payer, &user_keypair], + context.last_blockhash, + ); + let r = context.banks_client.process_transaction(transaction).await; + assert_eq!( + r.unwrap_err().unwrap(), + TransactionError::InstructionError( + 0, + InstructionError::Custom( + StakeError::RedelegatedStakeMustFullyActivateBeforeDeactivationIsPermitted as u32 + ) + ) + ); + + // 3.4 withdraw from 2nd stake account should also fail because of pending stake activation. + let transaction = Transaction::new_signed_with_payer( + &[stake_instruction::withdraw( + &stake_address2, + &user_keypair.pubkey(), + &solana_sdk::pubkey::new_rand(), + 1, + None, + )], + Some(&context.payer.pubkey()), + &vec![&context.payer, &user_keypair], + context.last_blockhash, + ); + let r = context.banks_client.process_transaction(transaction).await; + assert_eq!( + r.unwrap_err().unwrap(), + TransactionError::InstructionError(0, InstructionError::InsufficientFunds) + ); + + // 4. advance to new epoch so that the 2nd stake account is fully activated + current_slot += slots_per_epoch; + context.warp_to_slot(current_slot).unwrap(); + context.warp_forward_force_reward_interval_end().unwrap(); + + // 4.1 Now deactivate 2nd stake account should succeed because there is no pending stake activation. + let transaction = Transaction::new_signed_with_payer( + &[stake_instruction::deactivate_stake( + &stake_address2, + &user_keypair.pubkey(), + )], + Some(&context.payer.pubkey()), + &vec![&context.payer, &user_keypair], + context.last_blockhash, + ); + context + .banks_client + .process_transaction(transaction) + .await + .unwrap(); +} + +#[tokio::test] +async fn test_stake_redelegation_then_deactivation_withdraw_not_permitted() { + test_stake_redelegation_pending_activation(PendingStakeActivationTestFlag::NoMerge).await +} + +#[tokio::test] +async fn test_stake_redelegation_then_merge_active_then_deactivation_withdraw_not_permitted() { + test_stake_redelegation_pending_activation(PendingStakeActivationTestFlag::MergeActive).await +} + +#[tokio::test] +async fn test_stake_redelegation_then_merge_inactive_then_deactivation_withdraw_not_permitted() { + test_stake_redelegation_pending_activation(PendingStakeActivationTestFlag::MergeInactive).await +} diff --git a/program-test/tests/warp.rs b/program-test/tests/warp.rs index 94f497a98f4a76..3d7be785bf7b12 100644 --- a/program-test/tests/warp.rs +++ b/program-test/tests/warp.rs @@ -1,11 +1,13 @@ #![allow(clippy::arithmetic_side_effects)] + +mod setup; + use { bincode::deserialize, log::debug, + setup::{setup_stake, setup_vote}, solana_banks_client::BanksClient, - solana_program_test::{ - processor, ProgramTest, ProgramTestBanksClientExt, ProgramTestContext, ProgramTestError, - }, + solana_program_test::{processor, ProgramTest, ProgramTestBanksClientExt, ProgramTestError}, solana_sdk::{ account::Account, account_info::{next_account_info, AccountInfo}, @@ -18,9 +20,8 @@ use { signature::{Keypair, Signer}, stake::{ instruction as stake_instruction, - state::{Authorized, Lockup, StakeActivationStatus, StakeStateV2}, + state::{StakeActivationStatus, StakeStateV2}, }, - system_instruction, system_program, sysvar::{ clock, stake_history::{self, StakeHistory}, @@ -29,89 +30,13 @@ use { transaction::{Transaction, TransactionError}, }, solana_stake_program::stake_state, - solana_vote_program::{ - vote_instruction, - vote_state::{self, VoteInit, VoteState}, - }, + solana_vote_program::vote_state::{self}, std::convert::TryInto, }; // Use a big number to be sure that we get the right error const WRONG_SLOT_ERROR: u32 = 123456; -async fn setup_stake( - context: &mut ProgramTestContext, - user: &Keypair, - vote_address: &Pubkey, - stake_lamports: u64, -) -> Pubkey { - let stake_keypair = Keypair::new(); - let transaction = Transaction::new_signed_with_payer( - &stake_instruction::create_account_and_delegate_stake( - &context.payer.pubkey(), - &stake_keypair.pubkey(), - vote_address, - &Authorized::auto(&user.pubkey()), - &Lockup::default(), - stake_lamports, - ), - Some(&context.payer.pubkey()), - &vec![&context.payer, &stake_keypair, user], - context.last_blockhash, - ); - context - .banks_client - .process_transaction(transaction) - .await - .unwrap(); - stake_keypair.pubkey() -} - -async fn setup_vote(context: &mut ProgramTestContext) -> Pubkey { - // warp once to make sure stake config doesn't get rent-collected - context.warp_to_slot(100).unwrap(); - let mut instructions = vec![]; - let validator_keypair = Keypair::new(); - instructions.push(system_instruction::create_account( - &context.payer.pubkey(), - &validator_keypair.pubkey(), - Rent::default().minimum_balance(0), - 0, - &system_program::id(), - )); - let vote_lamports = Rent::default().minimum_balance(VoteState::size_of()); - let vote_keypair = Keypair::new(); - let user_keypair = Keypair::new(); - instructions.append(&mut vote_instruction::create_account_with_config( - &context.payer.pubkey(), - &vote_keypair.pubkey(), - &VoteInit { - node_pubkey: validator_keypair.pubkey(), - authorized_voter: user_keypair.pubkey(), - ..VoteInit::default() - }, - vote_lamports, - vote_instruction::CreateVoteAccountConfig { - space: vote_state::VoteStateVersions::vote_state_size_of(true) as u64, - ..vote_instruction::CreateVoteAccountConfig::default() - }, - )); - - let transaction = Transaction::new_signed_with_payer( - &instructions, - Some(&context.payer.pubkey()), - &vec![&context.payer, &validator_keypair, &vote_keypair], - context.last_blockhash, - ); - context - .banks_client - .process_transaction(transaction) - .await - .unwrap(); - - vote_keypair.pubkey() -} - fn process_instruction( _program_id: &Pubkey, accounts: &[AccountInfo], @@ -214,6 +139,8 @@ async fn stake_rewards_from_warp() { // Initialize and start the test network let program_test = ProgramTest::default(); let mut context = program_test.start_with_context().await; + + context.warp_to_slot(100).unwrap(); let vote_address = setup_vote(&mut context).await; let user_keypair = Keypair::new(); @@ -415,11 +342,12 @@ async fn check_credits_observed( expected_credits ); } - #[tokio::test] async fn stake_merge_immediately_after_activation() { let program_test = ProgramTest::default(); let mut context = program_test.start_with_context().await; + + context.warp_to_slot(100).unwrap(); let vote_address = setup_vote(&mut context).await; context.increment_vote_account_credits(&vote_address, 100); diff --git a/programs/stake/src/stake_instruction.rs b/programs/stake/src/stake_instruction.rs index 6cf5f000745883..20b6c9e0ebe3c4 100644 --- a/programs/stake/src/stake_instruction.rs +++ b/programs/stake/src/stake_instruction.rs @@ -267,7 +267,7 @@ declare_process_instruction!( let mut me = get_stake_account()?; let clock = get_sysvar_with_account_check::clock(invoke_context, instruction_context, 1)?; - deactivate(&mut me, &clock, &signers) + deactivate(invoke_context, &mut me, &clock, &signers) } Ok(StakeInstruction::SetLockup(lockup)) => { let mut me = get_stake_account()?; @@ -413,6 +413,7 @@ declare_process_instruction!( let clock = invoke_context.get_sysvar_cache().get_clock()?; deactivate_delinquent( + invoke_context, transaction_context, instruction_context, &mut me, @@ -7198,6 +7199,10 @@ mod tests { ..Clock::default() }), ), + ( + stake_history::id(), + create_account_shared_data_for_test(&StakeHistory::default()), + ), ], vec![ AccountMeta { diff --git a/programs/stake/src/stake_state.rs b/programs/stake/src/stake_state.rs index 964d2d6ffc5d78..0f913cb13eb5bd 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -633,15 +633,57 @@ pub fn delegate( } } +fn deactivate_stake( + invoke_context: &InvokeContext, + stake: &mut Stake, + stake_flags: &mut StakeFlags, + epoch: Epoch, +) -> Result<(), InstructionError> { + let stake_history = invoke_context.get_sysvar_cache().get_stake_history()?; + + if invoke_context + .feature_set + .is_active(&feature_set::stake_redelegate_instruction::id()) + { + if stake_flags.contains(StakeFlags::MUST_FULLY_ACTIVATE_BEFORE_DEACTIVATION_IS_PERMITTED) { + // when MUST_FULLY_ACTIVATE_BEFORE_DEACTIVATION_IS_PERMITTED flag is set on stake_flags, + // deactivation is only permitted when the stake delegation activating amount is zero. + let status = stake.delegation.stake_activating_and_deactivating( + epoch, + Some(&stake_history), + new_warmup_cooldown_rate_epoch(invoke_context), + ); + if status.activating != 0 { + Err(InstructionError::from( + StakeError::RedelegatedStakeMustFullyActivateBeforeDeactivationIsPermitted, + )) + } else { + stake.deactivate(epoch)?; + // After deactivation, need to clear `MustFullyActivateBeforeDeactivationIsPermitted` flag if any. + // So that future activation and deactivation are not subject to that restriction. + stake_flags + .remove(StakeFlags::MUST_FULLY_ACTIVATE_BEFORE_DEACTIVATION_IS_PERMITTED); + Ok(()) + } + } else { + stake.deactivate(epoch)?; + Ok(()) + } + } else { + stake.deactivate(epoch)?; + Ok(()) + } +} + pub fn deactivate( + invoke_context: &InvokeContext, stake_account: &mut BorrowedAccount, clock: &Clock, signers: &HashSet, ) -> Result<(), InstructionError> { - if let StakeStateV2::Stake(meta, mut stake, stake_flags) = stake_account.get_state()? { + if let StakeStateV2::Stake(meta, mut stake, mut stake_flags) = stake_account.get_state()? { meta.authorized.check(signers, StakeAuthorize::Staker)?; - stake.deactivate(clock.epoch)?; - + deactivate_stake(invoke_context, &mut stake, &mut stake_flags, clock.epoch)?; stake_account.set_state(&StakeStateV2::Stake(meta, stake, stake_flags)) } else { Err(InstructionError::InvalidAccountData) @@ -975,7 +1017,7 @@ pub fn redelegate( // deactivate `stake_account` // // Note: This function also ensures `signers` contains the `StakeAuthorize::Staker` - deactivate(stake_account, &clock, signers)?; + deactivate(invoke_context, stake_account, &clock, signers)?; // transfer the effective stake to the uninitialized stake account stake_account.checked_sub_lamports(effective_stake)?; @@ -1001,7 +1043,7 @@ pub fn redelegate( &vote_state.convert_to_current(), clock.epoch, ), - StakeFlags::empty(), + StakeFlags::MUST_FULLY_ACTIVATE_BEFORE_DEACTIVATION_IS_PERMITTED, ))?; Ok(()) @@ -1115,6 +1157,7 @@ pub fn withdraw( } pub(crate) fn deactivate_delinquent( + invoke_context: &InvokeContext, transaction_context: &TransactionContext, instruction_context: &InstructionContext, stake_account: &mut BorrowedAccount, @@ -1148,7 +1191,7 @@ pub(crate) fn deactivate_delinquent( return Err(StakeError::InsufficientReferenceVotes.into()); } - if let StakeStateV2::Stake(meta, mut stake, stake_flags) = stake_account.get_state()? { + if let StakeStateV2::Stake(meta, mut stake, mut stake_flags) = stake_account.get_state()? { if stake.delegation.voter_pubkey != *delinquent_vote_account_pubkey { return Err(StakeError::VoteAddressMismatch.into()); } @@ -1156,7 +1199,7 @@ pub(crate) fn deactivate_delinquent( // Deactivate the stake account if its delegated vote account has never voted or has not // voted in the last `MINIMUM_DELINQUENT_EPOCHS_FOR_DEACTIVATION` if eligible_for_deactivate_delinquent(&delinquent_vote_state.epoch_credits, current_epoch) { - stake.deactivate(current_epoch)?; + deactivate_stake(invoke_context, &mut stake, &mut stake_flags, current_epoch)?; stake_account.set_state(&StakeStateV2::Stake(meta, stake, stake_flags)) } else { Err(StakeError::MinimumDelinquentEpochsForDeactivationNotMet.into()) diff --git a/sdk/program/src/stake/instruction.rs b/sdk/program/src/stake/instruction.rs index 89db8e96be076e..9b964f0eee5f07 100644 --- a/sdk/program/src/stake/instruction.rs +++ b/sdk/program/src/stake/instruction.rs @@ -68,7 +68,6 @@ pub enum StakeError { #[error("stake redelegation to the same vote account is not permitted")] RedelegateToSameVoteAccount, - #[allow(dead_code)] #[error("redelegated stake must be fully activated before deactivation")] RedelegatedStakeMustFullyActivateBeforeDeactivationIsPermitted, } diff --git a/sdk/program/src/stake/state.rs b/sdk/program/src/stake/state.rs index 969e5f2ceffd29..4f94f73b3f2dd5 100644 --- a/sdk/program/src/stake/state.rs +++ b/sdk/program/src/stake/state.rs @@ -823,6 +823,45 @@ mod test { ); } + #[test] + fn stake_flag_member_offset() { + const FLAG_OFFSET: usize = 196; + let check_flag = |flag, expected| { + let stake = StakeStateV2::Stake( + Meta { + rent_exempt_reserve: 1, + authorized: Authorized { + staker: Pubkey::new_unique(), + withdrawer: Pubkey::new_unique(), + }, + lockup: Lockup::default(), + }, + Stake { + delegation: Delegation { + voter_pubkey: Pubkey::new_unique(), + stake: u64::MAX, + activation_epoch: Epoch::MAX, + deactivation_epoch: Epoch::MAX, + warmup_cooldown_rate: f64::MAX, + }, + credits_observed: 1, + }, + flag, + ); + + let bincode_serialized = serialize(&stake).unwrap(); + let borsh_serialized = StakeStateV2::try_to_vec(&stake).unwrap(); + + assert_eq!(bincode_serialized[FLAG_OFFSET], expected); + assert_eq!(borsh_serialized[FLAG_OFFSET], expected); + }; + check_flag( + StakeFlags::MUST_FULLY_ACTIVATE_BEFORE_DEACTIVATION_IS_PERMITTED, + 1, + ); + check_flag(StakeFlags::empty(), 0); + } + mod deprecated { use super::*; fn check_borsh_deserialization(stake: StakeState) {