diff --git a/crates/pallet-domains/src/benchmarking.rs b/crates/pallet-domains/src/benchmarking.rs index 15919203ea..f0afbb6ef4 100644 --- a/crates/pallet-domains/src/benchmarking.rs +++ b/crates/pallet-domains/src/benchmarking.rs @@ -7,8 +7,8 @@ use crate::block_tree::{prune_receipt, BlockTreeNode}; use crate::bundle_storage_fund::refund_storage_fee; use crate::domain_registry::DomainConfig; use crate::staking::{ - do_convert_previous_epoch_deposits, do_mark_operators_as_slashed, do_reward_operators, - OperatorConfig, OperatorStatus, WithdrawStake, + do_convert_previous_epoch_withdrawal, do_mark_operators_as_slashed, do_reward_operators, + Error as StakingError, OperatorConfig, OperatorStatus, WithdrawStake, }; use crate::staking_epoch::{ do_finalize_domain_current_epoch, do_finalize_domain_epoch_staking, do_slash_operator, @@ -725,10 +725,10 @@ mod benchmarks { /// Benchmark `unlock_funds` extrinsic with the worst possible conditions: /// - Unlock a full withdrawal which also remove the deposit storage for the nominator #[benchmark] - fn unlock_funds() { + fn unlock_funds(w: Linear<1, { T::WithdrawalLimit::get() }>) { let nominator = account("nominator", 1, SEED); let minimum_nominator_stake = T::MinNominatorStake::get(); - let staking_amount = T::MinOperatorStake::get(); + let staking_amount = T::MinOperatorStake::get() * 3u32.into(); T::Currency::set_balance(&nominator, staking_amount + T::MinNominatorStake::get()); let domain_id = register_domain::(); @@ -741,22 +741,32 @@ mod benchmarks { do_finalize_domain_epoch_staking::(domain_id) .expect("finalize domain staking should success"); - // Withdraw all deposit - let withdraw_amount = { - let mut deposit = - Deposits::::get(operator_id, nominator.clone()).expect("deposit must exist"); - do_convert_previous_epoch_deposits::(operator_id, &mut deposit) - .expect("convert must success"); - deposit.known.shares - }; + // Request `w` number of withdrawal in different epoch and withdraw all the stake in the last one + for _ in 1..w { + assert_ok!(Domains::::withdraw_stake( + RawOrigin::Signed(nominator.clone()).into(), + operator_id, + WithdrawStake::Stake(T::MinOperatorStake::get() / w.into()), + )); + do_finalize_domain_epoch_staking::(domain_id) + .expect("finalize domain staking should success"); + } assert_ok!(Domains::::withdraw_stake( RawOrigin::Signed(nominator.clone()).into(), operator_id, - WithdrawStake::Share(withdraw_amount), + WithdrawStake::All, )); do_finalize_domain_epoch_staking::(domain_id) .expect("finalize domain staking should success"); + Withdrawals::::try_mutate(operator_id, nominator.clone(), |maybe_withdrawal| { + let withdrawal = maybe_withdrawal.as_mut().unwrap(); + do_convert_previous_epoch_withdrawal::(operator_id, withdrawal)?; + assert_eq!(withdrawal.withdrawals.len() as u32, w); + Ok::<(), StakingError>(()) + }) + .unwrap(); + // Update the `LatestConfirmedDomainExecutionReceipt` so unlock can success let confirmed_domain_block_number = Pallet::::latest_confirmed_domain_block_number(domain_id) diff --git a/crates/pallet-domains/src/lib.rs b/crates/pallet-domains/src/lib.rs index 6a6c327940..a2d149a298 100644 --- a/crates/pallet-domains/src/lib.rs +++ b/crates/pallet-domains/src/lib.rs @@ -431,6 +431,12 @@ mod pallet { /// Hook to handle chain rewards. type OnChainRewards: OnChainRewards>; + + /// The max number of withdrawals per nominator that may exist at any time, + /// once this limit is reached, the nominator need to unlock the withdrawal + /// before requesting new withdrawal. + #[pallet::constant] + type WithdrawalLimit: Get; } #[pallet::pallet] diff --git a/crates/pallet-domains/src/staking.rs b/crates/pallet-domains/src/staking.rs index 701aa8eb2d..d169163e32 100644 --- a/crates/pallet-domains/src/staking.rs +++ b/crates/pallet-domains/src/staking.rs @@ -134,7 +134,6 @@ pub(crate) struct Withdrawal { #[derive(TypeInfo, Debug, Encode, Decode, Clone, PartialEq, Eq)] pub(crate) struct WithdrawalInBalance { - pub(crate) domain_id: DomainId, pub(crate) unlock_at_confirmed_domain_block_number: DomainBlockNumber, pub(crate) amount_to_unlock: Balance, pub(crate) storage_fee_refund: Balance, @@ -300,6 +299,7 @@ pub enum Error { UnconfirmedER, /// Invalid signature from Signing key owner. InvalidSigningKeySignature, + TooManyWithdrawals, } // Increase `PendingStakingOperationCount` by one and check if the `MaxPendingStakingOperation` @@ -544,10 +544,10 @@ pub(crate) fn do_convert_previous_epoch_withdrawal( }; if let Some(WithdrawalInShares { - domain_epoch, unlock_at_confirmed_domain_block_number, shares, storage_fee_refund, + domain_epoch: _, }) = withdrawal.withdrawal_in_shares.take() { let withdrawal_amount = epoch_share_price.shares_to_stake::(shares); @@ -555,10 +555,8 @@ pub(crate) fn do_convert_previous_epoch_withdrawal( .total_withdrawal_amount .checked_add(&withdrawal_amount) .ok_or(Error::BalanceOverflow)?; - let (domain_id, _) = domain_epoch.deconstruct(); let withdraw_in_balance = WithdrawalInBalance { - domain_id, unlock_at_confirmed_domain_block_number, amount_to_unlock: withdrawal_amount, storage_fee_refund, @@ -811,8 +809,10 @@ pub(crate) fn do_withdraw_stake( Withdrawals::::try_mutate(operator_id, nominator_id.clone(), |maybe_withdrawal| { if let Some(withdrawal) = maybe_withdrawal { do_convert_previous_epoch_withdrawal::(operator_id, withdrawal)?; + if withdrawal.withdrawals.len() as u32 >= T::WithdrawalLimit::get() { + return Err(Error::TooManyWithdrawals); + } } - Ok(()) })?; @@ -988,32 +988,58 @@ pub(crate) fn do_unlock_funds( Withdrawals::::try_mutate_exists(operator_id, nominator_id.clone(), |maybe_withdrawal| { let withdrawal = maybe_withdrawal.as_mut().ok_or(Error::MissingWithdrawal)?; do_convert_previous_epoch_withdrawal::(operator_id, withdrawal)?; - let WithdrawalInBalance { - domain_id, - unlock_at_confirmed_domain_block_number, - amount_to_unlock, - storage_fee_refund, - } = withdrawal - .withdrawals - .pop_front() - .ok_or(Error::MissingWithdrawal)?; + + ensure!(!withdrawal.withdrawals.is_empty(), Error::MissingWithdrawal); let latest_confirmed_block_number = - Pallet::::latest_confirmed_domain_block_number(domain_id); + Pallet::::latest_confirmed_domain_block_number(operator.current_domain_id); + + let mut total_unlocked_amount = BalanceOf::::zero(); + let mut total_storage_fee_refund = BalanceOf::::zero(); + loop { + if withdrawal + .withdrawals + .front() + .map(|w| w.unlock_at_confirmed_domain_block_number > latest_confirmed_block_number) + .unwrap_or(true) + { + break; + } + + let WithdrawalInBalance { + amount_to_unlock, + storage_fee_refund, + .. + } = withdrawal + .withdrawals + .pop_front() + .expect("Must not empty as checked above; qed"); + + total_unlocked_amount = total_unlocked_amount + .checked_add(&amount_to_unlock) + .ok_or(Error::BalanceOverflow)?; + + total_storage_fee_refund = total_storage_fee_refund + .checked_add(&storage_fee_refund) + .ok_or(Error::BalanceOverflow)?; + } + + // There is withdrawal but none being processed meaning the first withdrawal's unlock period has + // not completed yet ensure!( - unlock_at_confirmed_domain_block_number <= latest_confirmed_block_number, + !total_unlocked_amount.is_zero() || !total_storage_fee_refund.is_zero(), Error::UnlockPeriodNotComplete ); // deduct the amount unlocked from total withdrawal.total_withdrawal_amount = withdrawal .total_withdrawal_amount - .checked_sub(&amount_to_unlock) + .checked_sub(&total_unlocked_amount) .ok_or(Error::BalanceUnderflow)?; withdrawal.total_storage_fee_withdrawal = withdrawal .total_storage_fee_withdrawal - .checked_sub(&storage_fee_refund) + .checked_sub(&total_storage_fee_refund) .ok_or(Error::BalanceUnderflow)?; // If the amount to release is more than currently locked, @@ -1021,8 +1047,8 @@ pub(crate) fn do_unlock_funds( let (amount_to_mint, amount_to_release) = DepositOnHold::::try_mutate( (operator_id, nominator_id.clone()), |deposit_on_hold| { - let amount_to_release = amount_to_unlock.min(*deposit_on_hold); - let amount_to_mint = amount_to_unlock.saturating_sub(*deposit_on_hold); + let amount_to_release = total_unlocked_amount.min(*deposit_on_hold); + let amount_to_mint = total_unlocked_amount.saturating_sub(*deposit_on_hold); *deposit_on_hold = deposit_on_hold.saturating_sub(amount_to_release); @@ -1049,7 +1075,7 @@ pub(crate) fn do_unlock_funds( Pallet::::deposit_event(Event::NominatedStakedUnlocked { operator_id, nominator_id: nominator_id.clone(), - unlocked_amount: amount_to_unlock, + unlocked_amount: total_unlocked_amount, }); // Release storage fund @@ -1057,7 +1083,7 @@ pub(crate) fn do_unlock_funds( T::Currency::release( &storage_fund_hold_id, &nominator_id, - storage_fee_refund, + total_storage_fee_refund, Precision::Exact, ) .map_err(|_| Error::RemoveLock)?; @@ -1065,7 +1091,7 @@ pub(crate) fn do_unlock_funds( Pallet::::deposit_event(Event::StorageFeeUnlocked { operator_id, nominator_id: nominator_id.clone(), - storage_fee: storage_fee_refund, + storage_fee: total_storage_fee_refund, }); // if there are no withdrawals, then delete the storage as well @@ -1434,8 +1460,8 @@ pub(crate) mod tests { use crate::staking_epoch::{do_finalize_domain_current_epoch, do_slash_operator}; use crate::tests::{new_test_ext, ExistentialDeposit, RuntimeOrigin, Test}; use crate::{ - bundle_storage_fund, BalanceOf, Error, ExecutionReceiptOf, NominatorId, SlashedReason, - MAX_NOMINATORS_TO_SLASH, + bundle_storage_fund, BalanceOf, DomainBlockNumberFor, Error, ExecutionReceiptOf, + NominatorId, SlashedReason, MAX_NOMINATORS_TO_SLASH, }; use codec::Encode; use frame_support::traits::fungible::Mutate; @@ -2625,6 +2651,162 @@ pub(crate) mod tests { }) } + fn dummy_receipt(domain_block_number: DomainBlockNumberFor) -> ExecutionReceiptOf { + ExecutionReceiptOf:: { + domain_block_number, + domain_block_hash: Default::default(), + domain_block_extrinsic_root: Default::default(), + parent_domain_block_receipt_hash: Default::default(), + consensus_block_number: Default::default(), + consensus_block_hash: Default::default(), + inboxed_bundles: vec![], + final_state_root: Default::default(), + execution_trace: vec![], + execution_trace_root: Default::default(), + block_fees: BlockFees::default(), + transfers: Transfers::default(), + } + } + + #[test] + fn unlock_multiple_withdrawals() { + let domain_id = DomainId::new(0); + let operator_account = 1; + let operator_free_balance = 250 * SSC; + let operator_stake = 200 * SSC; + let pair = OperatorPair::from_seed(&U256::from(0u32).into()); + let data = OperatorSigningKeyProofOfOwnershipData { + operator_owner: operator_account, + }; + let signature = pair.sign(&data.encode()); + let nominator_account = 2; + let nominator_free_balance = 150 * SSC; + let nominator_stake = 100 * SSC; + + let nominators = vec![ + (operator_account, (operator_free_balance, operator_stake)), + (nominator_account, (nominator_free_balance, nominator_stake)), + ]; + + let total_deposit = 300 * SSC; + let init_total_stake = STORAGE_FEE_RESERVE.left_from_one() * total_deposit; + let init_total_storage_fund = STORAGE_FEE_RESERVE * total_deposit; + + let mut ext = new_test_ext(); + ext.execute_with(|| { + let (operator_id, _) = register_operator( + domain_id, + operator_account, + operator_free_balance, + operator_stake, + 10 * SSC, + pair.public(), + signature, + BTreeMap::from_iter(nominators), + ); + + do_finalize_domain_current_epoch::(domain_id).unwrap(); + let domain_stake_summary = DomainStakingSummary::::get(domain_id).unwrap(); + assert_eq!(domain_stake_summary.current_total_stake, init_total_stake); + + let operator = Operators::::get(operator_id).unwrap(); + assert_eq!(operator.current_total_stake, init_total_stake); + assert_eq!(operator.total_storage_fee_deposit, init_total_storage_fund); + assert_eq!( + operator.total_storage_fee_deposit, + bundle_storage_fund::total_balance::(operator_id) + ); + + let amount_per_withdraw = init_total_stake / 100; + let latest_confirmed_block_number = + Domains::latest_confirmed_domain_block_number(domain_id); + + // Request `WithdrawalLimit - 1` number of withdrawal + for _ in 1..::WithdrawalLimit::get() { + do_withdraw_stake::( + operator_id, + nominator_account, + WithdrawStake::Stake(amount_per_withdraw), + ) + .unwrap(); + do_finalize_domain_current_epoch::(domain_id).unwrap(); + } + // Increase the latest confirmed domain block by 1 + LatestConfirmedDomainExecutionReceipt::::insert( + domain_id, + dummy_receipt(latest_confirmed_block_number + 1), + ); + + // All withdrawals of a given nominator submitted in the same epoch will merge into one, + // so we submit can submit as many as we want even though the withdrawal limit is met + for _ in 0..5 { + do_withdraw_stake::( + operator_id, + nominator_account, + WithdrawStake::Stake(amount_per_withdraw), + ) + .unwrap(); + } + do_finalize_domain_current_epoch::(domain_id).unwrap(); + + // After the withdrawal limit is met, any new withdraw will be rejected in the next epoch + assert_err!( + do_withdraw_stake::( + operator_id, + nominator_account, + WithdrawStake::Stake(amount_per_withdraw), + ), + StakingError::TooManyWithdrawals + ); + Withdrawals::::try_mutate(operator_id, nominator_account, |maybe_withdrawal| { + let withdrawal = maybe_withdrawal.as_mut().unwrap(); + do_convert_previous_epoch_withdrawal::(operator_id, withdrawal).unwrap(); + assert_eq!( + withdrawal.withdrawals.len() as u32, + ::WithdrawalLimit::get() + ); + Ok::<(), StakingError>(()) + }) + .unwrap(); + + // Make the first set of withdrawals pass the unlock period then unlock fund + LatestConfirmedDomainExecutionReceipt::::insert( + domain_id, + dummy_receipt( + latest_confirmed_block_number + + ::StakeWithdrawalLockingPeriod::get(), + ), + ); + let total_balance = Balances::usable_balance(nominator_account); + assert_ok!(do_unlock_funds::(operator_id, nominator_account)); + assert_eq!( + Balances::usable_balance(nominator_account) + 60246126106, // `60246126106` is a minior rounding dust + total_balance + + (::WithdrawalLimit::get() as u128 - 1) * total_deposit + / 100 + ); + let withdrawal = Withdrawals::::get(operator_id, nominator_account).unwrap(); + assert_eq!(withdrawal.withdrawals.len(), 1); + + // Make the second set of withdrawals pass the unlock period then unlock funds + LatestConfirmedDomainExecutionReceipt::::insert( + domain_id, + dummy_receipt( + latest_confirmed_block_number + + ::StakeWithdrawalLockingPeriod::get() + + 1, + ), + ); + let total_balance = Balances::usable_balance(nominator_account); + assert_ok!(do_unlock_funds::(operator_id, nominator_account)); + assert_eq!( + Balances::usable_balance(nominator_account) + 18473897451, // `18473897451` is a minor rounding dust + total_balance + 5 * total_deposit / 100 + ); + assert!(Withdrawals::::get(operator_id, nominator_account).is_none()); + }); + } + #[test] fn slash_operator() { let domain_id = DomainId::new(0); diff --git a/crates/pallet-domains/src/tests.rs b/crates/pallet-domains/src/tests.rs index 8ca6980c7a..1210a4d50e 100644 --- a/crates/pallet-domains/src/tests.rs +++ b/crates/pallet-domains/src/tests.rs @@ -147,6 +147,7 @@ parameter_types! { pub const MaxInitialDomainAccounts: u32 = 5; pub const MinInitialDomainAccountBalance: Balance = SSC; pub const BundleLongevity: u32 = 5; + pub const WithdrawalLimit: u32 = 10; } pub struct MockRandomness; @@ -277,6 +278,7 @@ impl pallet_domains::Config for Test { type MmrProofVerifier = (); type FraudProofStorageKeyProvider = (); type OnChainRewards = (); + type WithdrawalLimit = WithdrawalLimit; } pub struct ExtrinsicStorageFees; diff --git a/crates/subspace-runtime/src/lib.rs b/crates/subspace-runtime/src/lib.rs index 71e236a9e5..f35eb19e1a 100644 --- a/crates/subspace-runtime/src/lib.rs +++ b/crates/subspace-runtime/src/lib.rs @@ -757,6 +757,7 @@ parameter_types! { pub const MaxInitialDomainAccounts: u32 = 10; pub const MinInitialDomainAccountBalance: Balance = SSC; pub const BundleLongevity: u32 = 5; + pub const WithdrawalLimit: u32 = 32; } // `BlockSlotCount` must at least keep the slot for the current and the parent block, it also need to @@ -849,6 +850,7 @@ impl pallet_domains::Config for Runtime { type MmrProofVerifier = MmrProofVerifier; type FraudProofStorageKeyProvider = StorageKeyProvider; type OnChainRewards = OnChainRewards; + type WithdrawalLimit = WithdrawalLimit; } parameter_types! { diff --git a/test/subspace-test-runtime/src/lib.rs b/test/subspace-test-runtime/src/lib.rs index 0d7d9721b5..156396a1b9 100644 --- a/test/subspace-test-runtime/src/lib.rs +++ b/test/subspace-test-runtime/src/lib.rs @@ -680,6 +680,7 @@ parameter_types! { pub const MaxInitialDomainAccounts: u32 = 20; pub const MinInitialDomainAccountBalance: Balance = SSC; pub const BundleLongevity: u32 = 5; + pub const WithdrawalLimit: u32 = 32; } // `BlockSlotCount` must at least keep the slot for the current and the parent block, it also need to @@ -769,6 +770,7 @@ impl pallet_domains::Config for Runtime { type MmrProofVerifier = MmrProofVerifier; type FraudProofStorageKeyProvider = StorageKeyProvider; type OnChainRewards = OnChainRewards; + type WithdrawalLimit = WithdrawalLimit; } parameter_types! {