Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Domains staking: Epoch transition and Pending unlocks #1657

Merged
merged 13 commits into from
Jul 18, 2023
Merged

Domains staking: Epoch transition and Pending unlocks #1657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a631690
finalize domain operator switch during the old domain epoch transition
vedhavyas Jul 7, 2023
2838994
ability to unlock operator at specific domain block number
vedhavyas Jul 10, 2023
226070e
finalize the operators deregistration
vedhavyas Jul 10, 2023
cdccaf8
define error type for epoch transitions
vedhavyas Jul 10, 2023
ab875d8
add operator withdrawal finalization at epoch and adjust the Operator…
vedhavyas Jul 11, 2023
0b4aed5
operator deposit finalization at the end od epoch
vedhavyas Jul 11, 2023
15138e5
add domain epoch finalization and begins next epoch
vedhavyas Jul 11, 2023
0c07426
unlock all the pending operators and nomination withdrawals
vedhavyas Jul 11, 2023
e45668e
track operator balances within staking summary for easier proof verif…
vedhavyas Jul 14, 2023
5be738f
process all operator related info and then remove the operator once u…
vedhavyas Jul 14, 2023
beb4f72
Merge branch 'main' into domains/epoch_transition
vedhavyas Jul 14, 2023
3d7e79a
Merge branch 'main' into domains/epoch_transition
vedhavyas Jul 18, 2023
648b9bb
Merge branch 'main' into domains/epoch_transition
vedhavyas Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions crates/pallet-domains/src/domain_registry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ use scale_info::TypeInfo;
use sp_core::Get;
use sp_domains::{DomainId, GenesisDomain, RuntimeId};
use sp_runtime::traits::{CheckedAdd, Zero};
use sp_std::vec;
use sp_std::collections::btree_map::BTreeMap;
use sp_std::collections::btree_set::BTreeSet;
use sp_std::vec::Vec;

/// Domain registry specific errors
Expand Down Expand Up @@ -149,8 +150,8 @@ pub(crate) fn do_instantiate_domain<T: Config>(
StakingSummary {
current_epoch_index: 0,
current_total_stake: Zero::zero(),
current_operators: vec![],
next_operators: vec![],
current_operators: BTreeMap::new(),
next_operators: BTreeSet::new(),
},
);

Expand Down
93 changes: 64 additions & 29 deletions crates/pallet-domains/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ mod tests;
pub mod domain_registry;
pub mod runtime_registry;
mod staking;
mod staking_epoch;
pub mod weights;

use frame_support::traits::fungible::{Inspect, InspectFreeze};
Expand Down Expand Up @@ -74,11 +75,14 @@ mod pallet {
do_switch_operator_domain, do_withdraw_stake, Error as StakingError, Nominator, Operator,
OperatorConfig, StakingSummary, Withdraw,
};
use crate::staking_epoch::{
do_finalize_domain_current_epoch, do_unlock_pending_withdrawals, PendingNominatorUnlock,
};
use crate::weights::WeightInfo;
use crate::{BalanceOf, FreezeIdentifier, NominatorId};
use codec::FullCodec;
use frame_support::pallet_prelude::{StorageMap, *};
use frame_support::traits::fungible::{InspectFreeze, MutateFreeze};
use frame_support::traits::fungible::{InspectFreeze, Mutate, MutateFreeze};
use frame_support::weights::Weight;
use frame_support::{Identity, PalletError};
use frame_system::pallet_prelude::*;
Expand All @@ -91,6 +95,7 @@ mod pallet {
Zero,
};
use sp_runtime::SaturatedConversion;
use sp_std::collections::btree_set::BTreeSet;
use sp_std::fmt::Debug;
use sp_std::vec;
use sp_std::vec::Vec;
Expand Down Expand Up @@ -139,7 +144,9 @@ mod pallet {
type DomainRuntimeUpgradeDelay: Get<Self::BlockNumber>;

/// Currency type used by the domains for staking and other currency related stuff.
type Currency: MutateFreeze<Self::AccountId> + InspectFreeze<Self::AccountId>;
type Currency: Mutate<Self::AccountId>
+ MutateFreeze<Self::AccountId>
+ InspectFreeze<Self::AccountId>;

/// Type representing the shares in the staking protocol.
type Share: Parameter
Expand Down Expand Up @@ -188,6 +195,12 @@ mod pallet {

/// Minimum operator stake required to become operator of a domain.
type MinOperatorStake: Get<BalanceOf<Self>>;

/// Minimum number of blocks after which any finalized withdrawls are released to nominators.
type StakeWithdrawalLockingPeriod: Get<Self::BlockNumber>;
Comment on lines +199 to +200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why use the consensus block for this period? We need to use the domain block instead.

IMO, this locking period of withdrawal is introduced in case there are slashes after the user issue withdrawal, such that we can still deduct from the locking withdrawal. The slash happens if there is fraud proof challenging an invalid ER, and the challenge period of ER is measured by domain block (i.e. BlockTreePruningDepth).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is per spec where once its moves to unlocking state, it is tracked with consensus chain block

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it takes only StakeEpochDuration domain blocks (currently set to 5) to move into the unlock state, which is much smaller than BlockTreePruningDepth (currently set to 256).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the parameterization will be revisited once the block tree PR lands, should be a non-blocker for this PR.

Copy link
Member

@NingLin-P NingLin-P Jul 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is not about the parameterization, StakeEpochDuration is expected to be small while BlockTreePruningDepth is expected to be large.

My concern is in the current implementation a malicious operator can submit a fraudulent ER and then deregister immediately, so they can successfully get away from slashing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to see concrete example TBH.

In either scenario of domain stall, network delay, and too many bundles to fit in one consensus block, can cause no domain bundle included in a consensus block thus no domain block will be driven from that consensus block, so the consensus chain will progress faster than the domain chain, which means the unlocking period (using consensus block) will progress faster than the challenging period (using domain block).

The following example uses BlockTreePruningDepth = 256, StakeWithdrawalLockingPeriod = 100 and StakeEpochDuration = 5 as the current implementation:

  • In consensus chain #10 and domain chain #10, a malicious operator submits a fraudulent ER and a deregister request.
  • After consensus chain progress #10 -> #12 and domain chain progress #10 -> #12, the domain epoch transited the deregistration and reached the unlocking state.
  • The domain chain stalled at #12, but the consensus chain progressed as usual #12 -> #112, and the deregistration was unlock
  • The consensus chain receives a fraud proof (which is acceptable until domain block #266) but can not slash the malicious operator as it is already deregistered.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My issue with using domain blocks is that we would need to track every domains block height in the unlocks list then. Is that a valid concern?

I think it won't bring extra cost compared to the current approach, we just need to change the index of the unlock list from consensus_block_number to (domain_id, domain_block_number) unless I miss something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it won't bring extra cost compared to the current approach, we just need to change the index of the unlock list from consensus_block_number to (domain_id, domain_block_number) unless I miss something.

Not just that, but we also need to adjust the other Storage items like PendingWithdrawals. This will be also be scoped such that each with withdrawal is on FIFO order. May not be such a trivial change since the underlying assumption has changed.

StakeEpochDuration = 5

This is not the final number and has a TODO to revisit this number.

Looking at the example there, I dont see that practically but this was the known concern during our offline discussion.
I do not have a strong opinion here but we need to update spec accordingly once we make the decision since calling it Global pending unlocks is confusing as well.

The change will be handled as either a separate PR or part of the epoch PR that uses block tree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be also be scoped such that each with withdrawal is on FIFO order.

The domain block number is also monotonically increasing as the consensus block, thus this is preserved.

StakeEpochDuration = 5

This is not the final number and has a TODO to revisit this number.

No matter how large this number can be the malicious operator can always choose to attack at the last few blocks of the current epoch, that is why I use 12 in the above example instead of 15.

The change will be handled as either a separate PR or part of the epoch PR that uses block tree.

I do think it is an issue we need to address but I'm okay with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it is an issue we need to address

Aboslutely, please start the discussion on the spec v2 regarding or open existing one. Thanks!


/// Domain epoch transition interval
type StakeEpochDuration: Get<Self::DomainNumber>;
}

#[pallet::pallet]
Expand Down Expand Up @@ -228,6 +241,7 @@ mod pallet {
pub(super) type DomainStakingSummary<T: Config> =
StorageMap<_, Identity, DomainId, StakingSummary<OperatorId, BalanceOf<T>>, OptionQuery>;

/// List of all registered operators and their configuration.
#[pallet::storage]
pub(super) type Operators<T: Config> =
StorageMap<_, Identity, OperatorId, Operator<BalanceOf<T>, T::Share>, OptionQuery>;
Expand All @@ -238,6 +252,7 @@ mod pallet {
pub(super) type PendingOperatorSwitches<T: Config> =
StorageMap<_, Identity, DomainId, Vec<OperatorId>, OptionQuery>;

/// List of all current epoch's nominators and their shares under a given operator,
#[pallet::storage]
pub(super) type Nominators<T: Config> = StorageDoubleMap<
_,
Expand All @@ -249,6 +264,9 @@ mod pallet {
OptionQuery,
>;

/// Deposits initiated a nominator under this operator.
/// Will be stored temporarily until the current epoch is complete.
/// Once, epoch is complete, these deposits are staked beginning next epoch.
#[pallet::storage]
pub(super) type PendingDeposits<T: Config> = StorageDoubleMap<
_,
Expand All @@ -260,6 +278,9 @@ mod pallet {
OptionQuery,
>;

/// Withdrawals initiated a nominator under this operator.
/// Will be stored temporarily until the current epoch is complete.
/// Once, epoch is complete, these will be moved to PendingNominatorUnlocks.
#[pallet::storage]
pub(super) type PendingWithdrawals<T: Config> = StorageDoubleMap<
_,
Expand All @@ -275,7 +296,33 @@ mod pallet {
/// Stored here temporarily until domain epoch is complete.
#[pallet::storage]
pub(super) type PendingOperatorDeregistrations<T: Config> =
StorageValue<_, Vec<OperatorId>, OptionQuery>;
StorageMap<_, Identity, DomainId, Vec<OperatorId>, OptionQuery>;

/// Stores a list of operators who are unlocking in the coming blocks.
/// The operator will be removed when the wait period is over
/// or when the operator is slashed.
#[pallet::storage]
pub(super) type PendingOperatorUnlocks<T: Config> =
StorageValue<_, BTreeSet<OperatorId>, ValueQuery>;

/// All the pending unlocks for the nominators.
/// We use this storage to fetch all the pending unlocks under a operator pool at the time of slashing.
#[pallet::storage]
pub(super) type PendingNominatorUnlocks<T: Config> = StorageDoubleMap<
_,
Identity,
OperatorId,
Identity,
T::BlockNumber,
Vec<PendingNominatorUnlock<NominatorId<T>, BalanceOf<T>>>,
OptionQuery,
>;

/// A list of operators that are either unregistering or one more of the nominators
/// are withdrawing some staked funds.
#[pallet::storage]
pub(super) type PendingUnlocks<T: Config> =
StorageMap<_, Identity, T::BlockNumber, BTreeSet<OperatorId>, OptionQuery>;

/// Stores the next domain id.
#[pallet::storage]
Expand Down Expand Up @@ -698,30 +745,11 @@ mod pallet {
nomination_tax: genesis_domain.nomination_tax,
};
let operator_stake = T::MinOperatorStake::get();
let operator_id = do_register_operator::<T>(
domain_owner,
domain_id,
operator_stake,
operator_config,
)
.expect("Genesis operator registration must succeed");

// TODO: Enact the epoch transition logic properly.
Operators::<T>::mutate(operator_id, |maybe_operator| {
let operator = maybe_operator
.as_mut()
.expect("Genesis operator must exist");
operator.current_total_stake = operator_stake;
});
DomainStakingSummary::<T>::insert(
domain_id,
StakingSummary {
current_epoch_index: 0,
current_total_stake: operator_stake,
current_operators: vec![operator_id],
next_operators: vec![],
},
);
do_register_operator::<T>(domain_owner, domain_id, operator_stake, operator_config)
.expect("Genesis operator registration must succeed");

do_finalize_domain_current_epoch::<T>(domain_id, Zero::zero(), Zero::zero())
.expect("Genesis epoch must succeed");
}
}
}
Expand All @@ -734,6 +762,9 @@ mod pallet {

do_upgrade_runtimes::<T>(block_number);

do_unlock_pending_withdrawals::<T>(block_number)
.expect("Pending unlocks should not fail due to checks at epoch");

Weight::zero()
}

Expand Down Expand Up @@ -834,7 +865,11 @@ impl<T: Config> Pallet<T> {
DomainStakingSummary::<T>::get(domain_id),
) {
(Some(domain_object), Some(stake_summary)) => Some(BundleProducerElectionParams {
current_operators: stake_summary.current_operators,
current_operators: stake_summary
.current_operators
.keys()
.cloned()
.collect::<Vec<OperatorId>>(),
total_domain_stake: stake_summary.current_total_stake,
bundle_slot_probability: domain_object.domain_config.bundle_slot_probability,
}),
Expand Down Expand Up @@ -921,7 +956,7 @@ impl<T: Config> Pallet<T> {

if !domain_stake_summary
.current_operators
.contains(&operator_id)
.contains_key(&operator_id)
{
return Err(BundleError::BadOperator);
}
Expand Down
Loading
Loading