From ca150b4e64f9588a4e488e6a9c662f1f361ab8db 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..371cce9d9181fe --- /dev/null +++ b/program-test/tests/stake.rs @@ -0,0 +1,203 @@ +#![allow(clippy::integer_arithmetic)] + +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 05e60deef3df2a..74e8d02cd7282f 100644 --- a/program-test/tests/warp.rs +++ b/program-test/tests/warp.rs @@ -1,11 +1,13 @@ #![allow(clippy::integer_arithmetic)] + +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 2a17c481b37ba9..e8802a34b7cde4 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, @@ -6643,6 +6644,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 9557544e8142af..5e4cfe63fa18cf 100644 --- a/programs/stake/src/stake_state.rs +++ b/programs/stake/src/stake_state.rs @@ -623,15 +623,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) @@ -958,7 +1000,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)?; @@ -984,7 +1026,7 @@ pub fn redelegate( &vote_state.convert_to_current(), clock.epoch, ), - StakeFlags::empty(), + StakeFlags::MUST_FULLY_ACTIVATE_BEFORE_DEACTIVATION_IS_PERMITTED, ))?; Ok(()) @@ -1098,6 +1140,7 @@ pub fn withdraw( } pub(crate) fn deactivate_delinquent( + invoke_context: &InvokeContext, transaction_context: &TransactionContext, instruction_context: &InstructionContext, stake_account: &mut BorrowedAccount, @@ -1131,7 +1174,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()); } @@ -1139,7 +1182,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 b8f2e3be0140f4..4d06b66da2eb3d 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) {