Skip to content

Commit

Permalink
Fixing commit failed tx. Remove stake must remove total retired stake
Browse files Browse the repository at this point in the history
  • Loading branch information
@ii-cruz
ii-cruz committed Jan 26, 2024
1 parent d585ce8 commit 44615ae
Show file tree
Hide file tree
Showing 3 changed files with 155 additions and 246 deletions.
95 changes: 31 additions & 64 deletions primitives/account/src/account/staking_contract/staker.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,9 @@ use crate::{
/// This action is only possible if:
/// (a) the resulting non-retired funds respect the invariant 1 - minimum stake for non-retired funds.
/// (b) the inactive funds are released (*).
/// 5. RemoveStake: Removes retired balance from a staker to outside the staking contract.
/// The retired balance can always be withdrawn as long as the staker respects the minimum
/// total stake or all balances are removed. The latter case results in the deletion of the staker.
/// This action is only possible if:
/// (a) the resulting total stake respects the invariant 2 - minimum stake for total funds.
/// 5. RemoveStake: Removes the total retired balance from a staker to outside the staking contract.
/// The retired balance can always be withdrawn as long as it removes all retired stake.
/// In case of total sake = 0 results in the deletion of the staker.
///
/// (*) For inactive balance to be released, the maximum of the lock-up period for inactive stake and the validator's
/// potential jail period must have passed.
Expand Down Expand Up @@ -219,20 +217,11 @@ impl StakingContract {
let mut staker = store.expect_staker(staker_address)?;

// Fail if the minimum stake would be violated for the non-retired funds (invariant 1).
if !Self::is_min_stake_enforced(
self.enforce_min_stake(
staker.balance + value,
staker.inactive_balance,
staker.retired_balance,
) {
debug!(
active_balance=?staker.balance,
inactive_balance=?staker.inactive_balance,
retired_balance=?staker.retired_balance,
?value,
"Tried to add stake that would violate the minimum stake"
);
return Err(AccountError::InvalidCoinValue);
}
)?;

// If we are delegating to a validator, we need to update it.
if let Some(validator_address) = &staker.delegation {
Expand Down Expand Up @@ -574,20 +563,11 @@ impl StakingContract {

// Fail if the minimum stake would be violated for non-retired funds (invariant 1).
let new_inactive_balance = staker.inactive_balance - retire_stake;
if !Self::is_min_stake_enforced(
self.enforce_min_stake(
staker.balance,
new_inactive_balance,
staker.retired_balance + retire_stake,
) {
debug!(
active_balance=?staker.balance,
inactive_balance=?staker.inactive_balance,
retired_balance=?staker.retired_balance,
?retire_stake,
"Tried to retire stake that would violate the minimum stake"
);
return Err(AccountError::InvalidCoinValue);
}
)?;

// Fail if the the funds are locked or jailed.
if !self.is_inactive_stake_released(store, &staker, block_number) {
Expand Down Expand Up @@ -663,7 +643,7 @@ impl StakingContract {
}

/// Removes balance from the retired stake of the staker.
/// The stake removal fails if the invariant 2 for minimum stake for total balance is violated.
/// The stake removal enforces a total withdrawal of retired stake.
/// If the staker's total balance is 0 then the staker is deleted.
pub fn remove_stake(
&mut self,
Expand All @@ -675,8 +655,7 @@ impl StakingContract {
// Get the staker.
let mut staker = store.expect_staker(staker_address)?;

// Fails if there are not enough retired funds for the withdrawal or if the
// minimum total stake is violated (invariant 2).
// Fails if we are not withdrawing exactly total retired funds.
self.can_remove_stake(&staker, value)?;

// All checks passed, not allowed to fail from here on!
Expand All @@ -685,7 +664,7 @@ impl StakingContract {
let old_delegation = staker.delegation.clone();

// Update balances.
staker.retired_balance -= value;
staker.retired_balance = Coin::ZERO;
self.balance -= value;

tx_logger.push_log(Log::RemoveStake {
Expand All @@ -698,10 +677,7 @@ impl StakingContract {
// We do not update the validator's stake balance because remove stake
// is only referring to already retired stake. Thus this stake
// has already been removed from the validator.
if staker.balance.is_zero()
&& staker.inactive_balance.is_zero()
&& staker.retired_balance.is_zero()
{
if staker.balance.is_zero() && staker.inactive_balance.is_zero() {
// If staker is to be removed and it had delegation, we update the validator.
if let Some(validator_address) = &staker.delegation {
self.unregister_staker_from_validator(store, validator_address);
Expand Down Expand Up @@ -771,35 +747,14 @@ impl StakingContract {
/* Helpers for checking release block heights and invariants */

/// Checks that the given value can be withdrawn from the retired balance.
/// Fails if there are not enough retired funds for the withdrawal or if the
/// minimum total stake is violated (invariant 2).
/// Fails if we are trying to withdraw less than total retired balance.
pub(crate) fn can_remove_stake(
&self,
staker: &Staker,
value: Coin,
) -> Result<(), AccountError> {
// Fail if the value is too high.
if staker.retired_balance < value {
return Err(AccountError::InsufficientFunds {
needed: value,
balance: staker.retired_balance,
});
}

// Fail if the minimum stake would be violated for the total stake (invariant 2).
let new_retired_balance = staker.retired_balance - value;
if !Self::is_min_stake_enforced(
staker.balance,
staker.inactive_balance,
new_retired_balance,
) {
debug!(
active_balance=?staker.balance,
inactive_balance=?staker.inactive_balance,
retired_balance=?staker.retired_balance,
?value,
"Tried to remove stake that would violate the minimum total stake"
);
// The value to be withdrawn must be equal to the retired balance.
if value != staker.retired_balance {
return Err(AccountError::InvalidCoinValue);
}

Expand All @@ -811,17 +766,23 @@ impl StakingContract {
/// Invariants:
/// (1) active + inactive balances must be == 0 or >= minimum stake
/// (2) active + inactive + retired balances must be == 0 or >= minimum stake
fn is_min_stake_enforced(
pub(crate) fn enforce_min_stake(
&self,
active_balance: Coin,
inactive_balance: Coin,
retired_balance: Coin,
) -> bool {
) -> Result<(), AccountError> {
// Check invariant 1: that total non-retired balances (active+inactive) do not violate the minimum stake.
let non_retired_balances = active_balance + inactive_balance;
if non_retired_balances > Coin::ZERO
&& non_retired_balances < Coin::from_u64_unchecked(Policy::MINIMUM_STAKE)
{
return false;
debug!(
?active_balance,
?inactive_balance,
"Transaction would violate the minimum non-retired stake"
);
return Err(AccountError::InvalidCoinValue);
}

// Check invariant 2: total stake does not violate minimum stake.
Expand All @@ -830,10 +791,16 @@ impl StakingContract {
&& retired_balance > Coin::ZERO
&& retired_balance < Coin::from_u64_unchecked(Policy::MINIMUM_STAKE)
{
return false;
debug!(
?active_balance,
?inactive_balance,
?retired_balance,
"Transaction would violate the minimum total stake"
);
return Err(AccountError::InvalidCoinValue);
}

true
Ok(())
}

/// Returns true if the inactive funds are released both from the lockup period and any potential jail
Expand Down
109 changes: 75 additions & 34 deletions primitives/account/src/account/staking_contract/traits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,8 +439,9 @@ impl AccountTransactionInteraction for StakingContract {

match data {
// In the case of a failed Delete Validator we will:
// 1. Pay the fee from the validator deposit
// 2. If the deposit reaches 0, we delete the validator
// 1. Fail if funds are not released yet.
// 2. Pay the fee from the validator deposit
// 3. If the deposit reaches 0, we delete the validator
OutgoingStakingTransactionData::DeleteValidator => {
let validator_address = proof.compute_signer();
let mut validator = store.expect_validator(&validator_address)?;
Expand Down Expand Up @@ -482,36 +483,56 @@ impl AccountTransactionInteraction for StakingContract {
}
OutgoingStakingTransactionData::RemoveStake => {
// In the case of a failed Remove Stake we will:
// 1. perform a normal remove stake with the fee instead of total transaction value.
// 2. If the fee deduction would violate the minimum stake it fails.
// 3. If the staker has a total balance of 0, it gets removed.
// 1. Fail if fee deduction would violate the minimum stake.
// 2. Pay the fee from the retired stake deposit
// 2. If the staker has a total balance of 0, it gets removed.

// Get the staker address from the proof and fetch the staker.
let staker_address = proof.compute_signer();
let mut staker = store.expect_staker(&staker_address)?;

// We do not want the fee payment to be displayed as a successful remove in the block logs,
// which is why we pass an empty logger.
let receipt = self.remove_stake(
&mut store,
&staker_address,
transaction.fee,
&mut TransactionLog::empty(),
let new_retired_balance = staker.retired_balance.safe_sub(transaction.fee)?;
self.enforce_min_stake(
staker.balance,
staker.inactive_balance,
new_retired_balance,
)?;

// Log a Delete Staker if staker total balance is 0.
if store.get_staker(&staker_address).is_none() {
let receipt = if new_retired_balance.is_zero() {
// Log a Delete Staker if staker total balance is 0.
tx_logger.push_log(Log::DeleteStaker {
staker_address: staker_address.clone(),
validator_address: receipt.delegation.clone(),
validator_address: staker.delegation.clone(),
});

// We do not want the fee payment to be displayed as a successful remove in the block logs,
// which is why we pass an empty logger.
Some(
self.remove_stake(
&mut store,
&staker_address,
transaction.fee,
&mut TransactionLog::empty(),
)?
.into(),
)
} else {
// Update the balances of the validator and staking contract.
staker.retired_balance = new_retired_balance;
self.balance -= transaction.fee;

// Update the validator entry.
store.put_staker(&staker_address, staker);

None
};

tx_logger.push_log(Log::StakerFeeDeduction {
staker_address,
fee: transaction.fee,
});

Ok(Some(receipt.into()))
Ok(receipt)
}
}
}
Expand Down Expand Up @@ -575,25 +596,41 @@ impl AccountTransactionInteraction for StakingContract {
fee: transaction.fee,
});

let receipt: RemoveStakeReceipt =
receipt.ok_or(AccountError::InvalidReceipt)?.try_into()?;
// Get or restore staker.
let mut staker = {
if let Some(staker) = store.get_staker(&staker_address) {
staker
} else if let Some(receipt) = receipt {
let receipt: RemoveStakeReceipt = receipt.try_into()?;
// Similar to the commit failed transaction, we manually log a DeleteStaker if staker was removed.
tx_logger.push_log(Log::DeleteStaker {
staker_address: staker_address.clone(),
validator_address: receipt.delegation.clone(),
});
self.revert_remove_stake(
&mut store,
&staker_address,
Coin::ZERO,
receipt,
&mut TransactionLog::empty(),
)?;

// Similar to the commit failed transaction, we manually log a DeleteStaker if staker was removed.
if store.get_staker(&staker_address).is_none() {
tx_logger.push_log(Log::DeleteStaker {
staker_address: staker_address.clone(),
validator_address: receipt.delegation.clone(),
});
}
store
.get_staker(&staker_address)
.expect("staker should be restored")
} else {
return Err(AccountError::InvalidReceipt);
}
};

// Revert the staker deletion.
self.revert_remove_stake(
&mut store,
&staker_address,
transaction.fee,
receipt,
&mut TransactionLog::empty(),
)?;
// Update the balances of the validator and staking contract.
staker.retired_balance += transaction.fee;
self.balance += transaction.fee;

// Update the validator entry.
store.put_staker(&staker_address, staker);

if store.get_staker(&staker_address).is_none() {}
}
};

Expand Down Expand Up @@ -646,7 +683,11 @@ impl AccountTransactionInteraction for StakingContract {
// Verify that the stake can actually be removed.
self.can_remove_stake(&staker, transaction.total_value())?;
// Verify that the fee by itself can be removed without violating the minimum stake.
self.can_remove_stake(&staker, transaction.fee)?;
self.enforce_min_stake(
staker.balance,
staker.inactive_balance,
staker.retired_balance - transaction.fee,
)?;

reserved_balance.reserve_for(
&staker_address,
Expand Down
Loading

0 comments on commit 44615ae

Please sign in to comment.