Skip to content

Commit

Permalink
address initial feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@2501babe
2501babe committed Jun 19, 2024
1 parent 7b34da2 commit adfeb11
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 81 deletions.
13 changes: 9 additions & 4 deletions programs/stake-tests/tests/test_move_stake_and_lamports.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}
}
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 0 additions & 2 deletions programs/stake/src/stake_instruction.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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(
Expand Down
154 changes: 79 additions & 75 deletions programs/stake/src/stake_state.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(())
Expand Down

0 comments on commit adfeb11

Please sign in to comment.