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.

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

[Staking] Approval Stake tracking #443

Open
ruseinov opened this issue Nov 16, 2022 · 6 comments
Open

[Staking] Approval Stake tracking #443

ruseinov opened this issue Nov 16, 2022 · 6 comments

Comments

@ruseinov
Copy link
Contributor

ruseinov commented Nov 16, 2022

This is basically a redesign and revival of the original PR: paritytech/substrate#11013 .

The first part is done here: paritytech/substrate#12034 and merely introduces a TargetList without any significant logic changes.

The work on the second part is done here: paritytech/substrate#12744

After discussing this with @kianenigma we decided to take a slightly different approach than in the original PR. The approach is different in that instead of keeping all of the potential validators in the TargetList with their score up-to-date, which among other things introduces:

  1. A problem of filtering the electable targets, since they could be a nominator or just chilled.
  2. TargetList bloat, since we have to keep everyone who's ever been nominated in the list and up-to-date with their score.

The solution:

We need to introduce another pallet, let's call it pallet-stake-tracker. The pallet will be passed two instances of bags-list that would serve as a TargetList and VoterList and expose an interface for pallet-staking to interact with.
How this works:

Interface:

/// A generic staking event listener. Current used for implementations involved in stake tracking.
/// Note that the interface is designed in a way that the events are fired post-action, so any
/// pre-action data that is needed needs to be passed to interface methods.
/// The rest of the data can be retrieved by using `StakingInterface`.
pub trait OnStakingUpdate<AccountId, Balance> {
	/// Track ledger updates.
	fn on_update_ledger(who: &AccountId, old_ledger: Stake<AccountId, Balance>);
	/// Track nominators, those reinstated and also new ones.
	fn on_nominator_add(who: &AccountId, old_nominations: Vec<AccountId>);
	/// Track validators, those reinstated and new.
	fn on_validator_add(who: &AccountId);
	/// Track removed validators. Either chilled or those that became nominators instead.
	fn on_validator_remove(who: &AccountId); // only fire this event when this is an actual Validator
	/// Track removed nominators.
	fn on_nominator_remove(who: &AccountId, nominations: Vec<AccountId>); // only fire this if this is an actual Nominator
	/// Track those participants of staking system that are kicked out for whatever reason.
	fn on_reaped(who: &AccountId); // -> basically `kill_stash`
}

Storage Map

Keeps track of participant's approval stake

pub type ApprovalStake<T: Config> =  StorageMap<_, Twox64Concat, T::AccountId, BalanceOf<T>>;

TargetList

One valid approach is to have an instance of bags-list that is being passed to pallet-approval-stake, what we currently call TargetList. Have the pallet-approval-stake be a score provider for it via aforementioned Storage Map.

SortedListProvider

Implement a SortedListProvider interface for pallet-approval-stake to expose the underlying TargetList and VoterList to pallet-staking. The idea behind this is we want to make sure that the TargetList is updated only by pallet-approval-stake and exposed in a read-only format to pallet-staking. This is going to be achieved by making sure that most of the methods exposed by SortedListProvider are going to be a noop in pallet-approval-stake implementation.

Config examples

pallet-approval-stake:

impl pallet_approval_stake::Config for Runtime {
   type TargetList = TargetList; // an instance of bags-list
   type VoterList = VoterList; // an instance of bags-list

}

pallet-staking:

impl pallet_staking::Config for Runtime {
   type TargetList = ApprovalStake::TargetListWrapper; // an instance of `pallet-approval-stake` that exposes a read-only version of `TargetList` via `SortedListProvider` interface
   type VoterList = ApprovalStake::VoterListWrapper; // an instance of `pallet-approval-stake` that exposes a read-only version of `VoterList` via `SortedListProvider` interface
   type ApprovalStake = ApprovalStake; // the same instance of `pallet-approval-stake` exposing an `OnStakerUpdate` interface
}
@ruseinov
Copy link
Contributor Author

ruseinov commented Nov 16, 2022

Discussed below
@kianenigma This seems to be the situation in which we'd like to clear the ApprovalStake of a Validator and remove them from the storage map: https://github.com/paritytech/substrate/blob/845780086e0ecad3fb334ad6d038c599ba53af54/frame/staking/src/pallet/impls.rs#L621-L620

@ruseinov
Copy link
Contributor Author

ruseinov commented Nov 16, 2022

NOT RELEVANT ANYMORE

@kianenigma I'm not sure if we need

    fn chilled(); // a validator is removed from TargetList, but their score is still tracked in a storage map
    fn validating(); // a validator is reinstated/introduced to the TargetList, based on the score from the storage map
    fn nominating();

According to the current logic in the old PR of yours we'll still need to do most of the operations on Staking side, because we need to do things like chill_stash, where we need to know targets for do_remove_nominator, score that's accessible via Self::slashable_balance_of in Staking etc.

My proposal would be as follows - we use active_stake_increased and active_stake_decreased for things like this and keep ApprovalStake tracker stupid:

fn validator(who: &AccountId, active: bool); // covers all the cases validating/chilled/nominating while keeping the score in StorageMap, because really we still do most of the operations in Staking, just calling methods on `OnStakerUpdate` interface instead of `bags-list` instance. 
fn kill(who: &AccountId); // covers the case when we need to remove the account from the staking system
fn active_stake_increased(who: &AccountId, balance: BalanceOf<Self>); // makes necessary modifications to all relevant entries in the storage map and TargetList
fn active_stake_decreased(who: &AccountId, balance: BalanceOf<Self>));  // makes necessary modifications to all relevant entries in the storage map and TargetList

Otherwise if we wanted to take care of all the logic that currently lives in Staking - in ApprovalStakeTracker we'd need to have access to several things:

  1. slashable_balance_of
  2. NominatorsHelper::<T>::get_any(who).map(|n| n.targets)
  3. and some more

So basically my proposal is to use ApprovalStake as a fairly thin wrapper around TargetList bags-list with a friendly interface for convenience.

@ruseinov
Copy link
Contributor Author

@ruseinov
Copy link
Contributor Author

After discussing this, we've decided to go with the approach where the approval stake tracker (TargetList + storage) is an event listener and so is the VoterList tracker.

Most of the tracking logic has to go into the tracker pallet and use StakingInterface to fetch necessary data, like stash balances and Nominations.

@ruseinov
Copy link
Contributor Author

ruseinov commented Nov 19, 2022

Make sure all the events are fired post-action (if pre-data is needed we pass it as an arg)

OnStakingUpdate {
  on_update_ledger(who: &AccountId, old_ledger: sp_staking::Stake...);
  on_nominator_add(who: &AccountId, old_nominations: Vec<AccountId>);
  on_validator_add(who: &AccountId);
  on_validator_remove(who: &AccountId); // only fire this event when this is an actual Validator
  on_nominator_remove(who: &AccountId, nominations: Vec<AccountId>);  // only fire this if this is an actual Nominator
  on_reaped(who: &AccountId); -> basically `kill_stash`
}

@ruseinov
Copy link
Contributor Author

ruseinov commented Nov 19, 2022

As a temp solution we only take care of TargetList which is not actually used, while maintaining the VoterList logic. This will duplicate the logic for now, but that's fine for testing. (Basically we'll have a set of the same rules within the eventListener and in Staking).

So the first iteration:
the tracker has two new bags lists for testing, representing voterList and targetList, maintaining them. (have two structs expose implementind sortedListProvider backed by those lists, allowing read-only access to them).
While maintaining existing logic for actual TargetList and VoterList in Staking for now.

@juangirini juangirini transferred this issue from paritytech/substrate Aug 24, 2023
gpestana added a commit that referenced this issue Oct 15, 2023
This PR refactors the staking ledger logic to encapsulate all reads and
mutations of `Ledger`, `Bonded`, `Payee` and stake locks within the
`StakingLedger` struct implementation.

With these changes, all the reads and mutations to the `Ledger`, `Payee`
and `Bonded` storage map should be done through the methods exposed by
StakingLedger to ensure the data and lock consistency of the operations.
The new introduced methods that mutate and read Ledger are:

- `ledger.update()`: inserts/updates a staking ledger in storage;
updates staking locks accordingly (and ledger.bond(), which is synthatic
sugar for ledger.update())
- `ledger.kill()`: removes all Bonded and StakingLedger related data for
a given ledger; updates staking locks accordingly;
`StakingLedger::get(account)`: queries both the `Bonded` and `Ledger`
storages and returns a `Option<StakingLedger>`. The pallet impl exposes
fn ledger(account) as synthatic sugar for `StakingLedger::get(account)`.

Retrieving a ledger with `StakingLedger::get()` can be done by providing
either a stash or controller account. The input must be wrapped in a
`StakingAccount` variant (Stash or Controller) which is treated
accordingly. This simplifies the caller API but will eventually be
deprecated once we completely get rid of the controller account in
staking. However, this refactor will help with the work necessary when
completely removing the controller.

Other goals:

- No logical changes have been introduced in this PR;
- No breaking changes or updates in wallets required;
- No new storage items or need to perform storage migrations;
- Centralise the changes to bonds and ledger updates to simplify the
OnStakingUpdate updates to the target list (related to
#443)

Note: it would be great to prevent or at least raise a warning if
`Ledger<T>`, `Payee<T>` and `Bonded<T>` storage types are accessed
outside the `StakingLedger` implementation. This PR should not get
blocked by that feature, but there's a tracking issue here
#149

Related and step towards
#443
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this issue Mar 25, 2024
This PR refactors the staking ledger logic to encapsulate all reads and
mutations of `Ledger`, `Bonded`, `Payee` and stake locks within the
`StakingLedger` struct implementation.

With these changes, all the reads and mutations to the `Ledger`, `Payee`
and `Bonded` storage map should be done through the methods exposed by
StakingLedger to ensure the data and lock consistency of the operations.
The new introduced methods that mutate and read Ledger are:

- `ledger.update()`: inserts/updates a staking ledger in storage;
updates staking locks accordingly (and ledger.bond(), which is synthatic
sugar for ledger.update())
- `ledger.kill()`: removes all Bonded and StakingLedger related data for
a given ledger; updates staking locks accordingly;
`StakingLedger::get(account)`: queries both the `Bonded` and `Ledger`
storages and returns a `Option<StakingLedger>`. The pallet impl exposes
fn ledger(account) as synthatic sugar for `StakingLedger::get(account)`.

Retrieving a ledger with `StakingLedger::get()` can be done by providing
either a stash or controller account. The input must be wrapped in a
`StakingAccount` variant (Stash or Controller) which is treated
accordingly. This simplifies the caller API but will eventually be
deprecated once we completely get rid of the controller account in
staking. However, this refactor will help with the work necessary when
completely removing the controller.

Other goals:

- No logical changes have been introduced in this PR;
- No breaking changes or updates in wallets required;
- No new storage items or need to perform storage migrations;
- Centralise the changes to bonds and ledger updates to simplify the
OnStakingUpdate updates to the target list (related to
paritytech#443)

Note: it would be great to prevent or at least raise a warning if
`Ledger<T>`, `Payee<T>` and `Bonded<T>` storage types are accessed
outside the `StakingLedger` implementation. This PR should not get
blocked by that feature, but there's a tracking issue here
paritytech#149

Related and step towards
paritytech#443
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✂️ In progress.
Development

No branches or pull requests

1 participant