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] Make exits manual, patch lack of delay for {increasing, decreasing} bonds #810

Merged
merged 95 commits into from
Nov 19, 2021

Conversation

4meta5
Copy link
Contributor

@4meta5 4meta5 commented Sep 14, 2021

Changes how exits and bond changes are scheduled, executed, and cancelled. It also changed the terminology from NPoS to DPoS i.e. nominator -> delegator, nomination -> delegation.

Logic changes include:

  1. The ExitQueue is replaced by manual exits. This requires more interaction from users to request {exits, bond changes} and execute them after a delay.
  2. Adds 2 round delay to bond_{more, less} requests by collator candidates and delegators. The execution is not automatic and requires an execute_ call by any signed account.

Please review the precompiles/parachain-staking/StakingInterface.sol to see the changes to the precompile interface.

Automatic -> Manual Execution

Before execution of delayed exits was automatic, but now an execute extrinsic is added for all exit and bond actions for candidates and delegators.

The execute extrinsics can be called by any signed origin but only executes if there is a pending request and it has waited the 2 round delay (at least 2 rounds after the round in which the request was made).

There is no explicit reward for executing someone else's request, but this design provides some disincentive for queueing up requests without the intention to execute (because anyone else can execute them and some users may have economic incentive to do so i.e. if it increases their relative share of staking rewards).

New Exit Rules

The only constraint is that delegators cannot schedule_leave_delegators and then delegate (assuming not calling cancel_leave_delegators in between). So delegators cannot delegate in a leaving state.

Everything else is allowed during candidate or delegator leaving now because lots of constraints were relaxed.

Relaxed Constraints

When the candidate is scheduled to leave,

  1. the candidate can requests to bond_{more, less} and can execute pending requests.
  2. delegators can make requests to {revoke, bond_{more, less}} and can execute these requests.
  3. delegators can make request to leave (revoke all delegations) and can execute the exit request.
  4. delegators can make new delegations to the candidate.

The candidate can make at most 1 request to adjust their self bond (bond_{more, less}). If a second request is made, it returns an error and the storage state before the call is preserved.

When the delegator is scheduled to leave, the delegator can make requests to {revoke, bond_{more, less}} and execute pending requests.

The delegator can make at most 1 pending request to {revoke, bond_{more, less}} per delegation. When the delegator is making a pending request to {revoke, bond_{more, less}} for a specific candidate, the delegator cannot make any additional requests to change the given delegation (unless they cancel the first request).

Follow Up Tasks

  • remove deprecated storage item code (ExitQueue, CollatorState2, NominatorState2)
  • add back TS tests for bond and exits (is there run_to_block in TS?)

@4meta5 4meta5 added A3-inprogress Pull request is in progress. No review needed at this stage. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes labels Sep 15, 2021
@4meta5 4meta5 marked this pull request as ready for review September 28, 2021 15:41
pallets/parachain-staking/src/tests.rs Outdated Show resolved Hide resolved
// execute delegator migration
for (delegator_id, nominator_state) in <NominatorState2<T>>::drain() {
let mut delegator_state =
migrate_nominator_to_delegator_state::<T>(delegator_id.clone(), nominator_state);
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be pretty heavy CPU-wise, I wonder if your 10% estimate might be insufficient here...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is a custom From conversion from struct Nominator2 to struct Delegator which also takes 1 parameter. So I don't think the conversion itself is expensive.

I'm more worried about iterating all the items in the NominatorState2 map, maybe that's what you were referring to?

Either way, we can increase the margin of safety.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

increased to 50%

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it looks like we can expect around 3k of them. I think we'll be ok, but (as I mentioned privately) I'll help you test this with try-runtime.

/// Delegator account
pub id: AccountId,
/// All current delegations
pub delegations: OrderedSet<Bond<AccountId, Balance>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is maybe more of a note to myself to verify later, but does this OrderedSet need to be re-sorted upon migration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I don't think this question makes sense, but I still want to revisit it

Copy link
Contributor Author

@4meta5 4meta5 Nov 19, 2021

Choose a reason for hiding this comment

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

No need to resort. I expect the order to be preserved, but maybe worth verifying.

Moreover, the ordering isn't actually used anywhere, the OrderedSet is used to enforce the set constraint that there can be no two delegations to the same candidate. A better data structure could be used, definitely, but not in scope for this PR.

It'd be great to remove OrderedSet altogether, but it would require migrating a few storage items.

);
let delegator_id: T::AccountId = self.id.clone().into();
ensure!(
T::Currency::can_reserve(&delegator_id, more.into()),
Copy link
Contributor

Choose a reason for hiding this comment

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

should we be reserving here instead of waiting?

Copy link
Contributor Author

@4meta5 4meta5 Nov 19, 2021

Choose a reason for hiding this comment

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

No because this is scheduling the increase, not executing it. We could remove this check altogether, but I think we shouldn't allow someone to schedule the request without having the liquid balance to execute it. They could transfer the balance out in between, but the reserve in the execution logic would still require they transfer it back in before executing.

Copy link
Contributor

Choose a reason for hiding this comment

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

(if not, a test case to cover issues like "scheduled to bond more and then spent those funds before executing" could be useful)

@crystalin crystalin added A8-mergeoncegreen Pull request is reviewed well. and removed A0-pleasereview Pull request needs code review. labels Nov 19, 2021
@4meta5 4meta5 merged commit 76f748a into master Nov 19, 2021
@4meta5 4meta5 deleted the amar-manual-exit branch November 19, 2021 18:13
@4meta5
Copy link
Contributor Author

4meta5 commented Nov 19, 2021

Any further review can still be addressed in follow ups!

@JoshOrndorff JoshOrndorff added D10-breaksdocs Changes to code that require changes in documentation A9-buythat(wo)manabeer Pull request is reviewed well and worth buying the author a beer. labels Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A8-mergeoncegreen Pull request is reviewed well. A9-buythat(wo)manabeer Pull request is reviewed well and worth buying the author a beer. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D1-runtime-migration PR introduces code that might require downstream chains to run a runtime upgrade. D10-breaksdocs Changes to code that require changes in documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants