No unbonding when signer or next signer #1031

JesseAbram · 2024-08-26T21:11:51Z

Related #942

This is a little controversial as it hurts UX a bit but I'm ok with that. I did wanna bring up that this breaks convention of staking and maybe if we rename this pallet to staking and mock all the calls in that pallet we can use all the frontends that are built around staking, a little more pallet code but a hella a lot less GUI code.

Also @HCastano wanna double check all my entries here, I read up and I believe there are only three ways to unbond from a validator

chill
unbond
withdraw_unbonded

but second eyes on that would be nice.

Closes #942.

ameba23

+1 This seems like something we need.

One question i have is how do we force people to use these wrappers and not call the underlying pallet-staking extrinsics? Are they somehow not exposed by our runtime?

Also, if i understand right, validators can anyway only leave at a session change, which is currently when we do a reshare. So if they call unbond (or chill etc) during a session, could we mark them as being the one who wants to leave the signing committee at the next reshare - or block them from being chosen as a next signer. That way validators can leave whenever they want.

The argument against this would be that it would make it more complicated to change the frequency of reshares, which i think we will want to do, and there could be complications or security issues if several members of the signing comittee want to unbond in the same session.

pallets/staking/src/lib.rs

JesseAbram · 2024-08-28T14:56:08Z

+1 This seems like something we need.

One question i have is how do we force people to use these wrappers and not call the underlying pallet-staking extrinsics? Are they somehow not exposed by our runtime?

That is handled in the base call filter here https://github.com/entropyxyz/entropy-core/pull/1031/files#diff-0ec06ea58bd455f09ce6b3bb4c2c1c0d37bda51c1e1be2151c560c9c973959ecR320-R322

Also, if i understand right, validators can anyway only leave at a session change, which is currently when we do a reshare. So if they call unbond (or chill etc) during a session, could we mark them as being the one who wants to leave the signing committee at the next reshare - or block them from being chosen as a next signer. That way validators can leave whenever they want.

The argument against this would be that it would make it more complicated to change the frequency of reshares, which i think we will want to do, and there could be complications or security issues if several members of the signing comittee want to unbond in the same session.

Yes I had this same thought, however this works if one wants to, but if more than one wants to it gets messy, and it isn't like we are locking them in forever, it isn't that long ( a day or two) I think this is worse UX but way simpler, that being said, id say lets do this for now and we can potentially look at and discuss having them pushed out if they want to unbond earlier

Co-authored-by: peg <peg@magmacollective.org>

HCastano

Okay, so while the direction of this is correct, the implementation breaks the nominator flow since they can also chill(), unbond(), etc.

What we need to do here is check if an account is a validator or nominator and act differently depending on the case. See chill_stash() from the Staking pallet for example.

HCastano · 2024-08-28T16:09:26Z

pallets/staking/src/lib.rs

+        /// Wraps's substrate chill but checks to make targeted validator sure not signer or next signer
+        #[pallet::call_index(3)]
+        #[pallet::weight(<T as Config>::WeightInfo::chill(MAX_SIGNERS as u32))]
+        pub fn chill(origin: OriginFor<T>) -> DispatchResultWithPostInfo {


Call it fate, but I was listening to this while reviewing 😆

pallets/staking/src/lib.rs

pallets/staking/src/tests.rs

HCastano · 2024-08-28T16:26:58Z

pallets/staking/src/benchmarking.rs

@@ -124,11 +136,76 @@ benchmarks! {
    assert_last_event::<T>(Event::<T>::ThresholdAccountChanged(bonder, server_info).into());
  }

+  unbond {


Can you take the body of this (and all the other benches added in this PR) and manually RustFmt it using the Rust Playground? This isn't done by the fmt script since it's since a decl macro.

Bumping this

pallets/staking/src/benchmarking.rs

HCastano · 2024-08-28T16:32:59Z

I did wanna bring up that this breaks convention of staking and maybe if we rename this pallet to staking and mock all the calls in that pallet we can use all the frontends that are built around staking, a little more pallet code but a hella a lot less GUI code.

Oh also, can you elaborate on this? Not sure how renaming and mocking ties into using staking frontends

JesseAbram · 2024-08-28T16:55:15Z

I did wanna bring up that this breaks convention of staking and maybe if we rename this pallet to staking and mock all the calls in that pallet we can use all the frontends that are built around staking, a little more pallet code but a hella a lot less GUI code.

Oh also, can you elaborate on this? Not sure how renaming and mocking ties into using staking frontends

mocking may not be the write word, wrapping is probably better. Pretty much all staking GUIs would call staking.chill or staking.function. Our code blocks that for the specific calls (chill, unbond........etc) so we can add some extra logic, it would be stakingExtention.function, if we rename staking_extention to staking and staking to idk frame_staking, and wrap all the rest of the calls in that pallet in staking extention, (and keep the same interface) then the GUIs for staking like polkadot js frontend or any other staking wallets should just work, instead of us having to roll our own

JesseAbram · 2024-08-28T16:58:26Z

Okay, so while the direction of this is correct, the implementation breaks the nominator flow since they can also chill(), unbond(), etc.

What we need to do here is check if an account is a validator or nominator and act differently depending on the case. See chill_stash() from the Staking pallet for example.

I think I get it, i think I broke the nominator flow, just to be sure we are on the same page, I still wanna apply the same logic to both however for nominator I need to get who they are nominating and for a validator same flow just get the related stash to the controller

HCastano · 2024-08-28T18:35:52Z

mocking may not be the write word, wrapping is probably better. Pretty much all staking GUIs would call staking.chill or staking.function. Our code blocks that for the specific calls (chill, unbond........etc) so we can add some extra logic, it would be stakingExtention.function, if we rename staking_extention to staking and staking to idk frame_staking, and wrap all the rest of the calls in that pallet in staking extention, (and keep the same interface) then the GUIs for staking like polkadot js frontend or any other staking wallets should just work, instead of us having to roll our own

Umm, not sure if it'll be that simple. UIs will probably want to do validation on their
end anyways, and we're emitting different events than the Staking pallet (so the UI might
say something succeeded, but in reality it failed). Something worth following up on though.

I think I get it, i think I broke the nominator flow, just to be sure we are on the same page, I still wanna apply the same logic to both however for nominator I need to get who they are nominating and for a validator same flow just get the related stash to the controller

For the nominator flow we need to allow a nominator to call chill(), unbond(), etc.
That's not possible right now since the nominator account would fail the checks that the
controller is a signer.

So here we have a couple of options:

Skip some of these checks in that case that an an account is a nominator and not a validator.
If a controller is a nominator, check to see who they're delegaging to. If that person
is a signer then we don't allow them to perform an action.

I can see (2) being pretty annoying for nominators, but this would help prevent the case
where a validator only puts up a bond of say, 1 UNIT themselves and then uses a
different nomination account to get reach the required stake.

JesseAbram · 2024-08-28T21:11:01Z

mocking may not be the write word, wrapping is probably better. Pretty much all staking GUIs would call staking.chill or staking.function. Our code blocks that for the specific calls (chill, unbond........etc) so we can add some extra logic, it would be stakingExtention.function, if we rename staking_extention to staking and staking to idk frame_staking, and wrap all the rest of the calls in that pallet in staking extention, (and keep the same interface) then the GUIs for staking like polkadot js frontend or any other staking wallets should just work, instead of us having to roll our own

Umm, not sure if it'll be that simple. UIs will probably want to do validation on their end anyways, and we're emitting different events than the Staking pallet (so the UI might say something succeeded, but in reality it failed). Something worth following up on though.

I think I get it, i think I broke the nominator flow, just to be sure we are on the same page, I still wanna apply the same logic to both however for nominator I need to get who they are nominating and for a validator same flow just get the related stash to the controller

For the nominator flow we need to allow a nominator to call chill(), unbond(), etc. That's not possible right now since the nominator account would fail the checks that the controller is a signer.

on reading into it, it actually wouldn't but the check won't apply to a nominator which also needs to be fixed. pretty much stash would not apply to who the nominator is validating but their own stash and would fail, however this info is kept in the Nominators storage struct

So here we have a couple of options:

Skip some of these checks in that case that an an account is a nominator and not a validator.

If a controller is a nominator, check to see who they're delegaging to. If that person
is a signer then we don't allow them to perform an action.

I can see (2) being pretty annoying for nominators, but this would help prevent the case where a validator only puts up a bond of say, 1 UNIT themselves and then uses a different nomination account to get reach the required stake.

yes 2 is where I was landing mentally too, you don't need to bond from your validator and it is needed

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

…nbonding-when-signer-or-next-signer

HCastano

I still need to look through the benches, but it does look like the nominator flow is working better now

HCastano · 2024-09-13T17:26:47Z

pallets/staking/src/tests.rs

+        // test nominating flow
+        assert_ok!(FrameStaking::bond(
+            RuntimeOrigin::signed(101),
+            100u64,
+            pallet_staking::RewardDestination::Account(1),
+        ));
+        assert_ok!(FrameStaking::nominate(RuntimeOrigin::signed(101), vec![7]));
+        assert_noop!(
+            Staking::withdraw_unbonded(RuntimeOrigin::signed(101), 0),
+            Error::<Test>::NoUnbondingWhenSigner
+        );
+
+        assert_ok!(FrameStaking::bond(
+            RuntimeOrigin::signed(8),
+            100u64,
+            pallet_staking::RewardDestination::Account(1),
+        ));
+
+        NextSigners::<Test>::put(NextSignerInfo { next_signers: vec![8], confirmations: vec![] });
+
+        assert_noop!(
+            Staking::withdraw_unbonded(RuntimeOrigin::signed(8), 0),
+            Error::<Test>::NoUnbondingWhenNextSigner
+        );
+
+        // test nominating flow
+        assert_ok!(FrameStaking::nominate(RuntimeOrigin::signed(101), vec![8]));
+        assert_noop!(
+            Staking::withdraw_unbonded(RuntimeOrigin::signed(101), 0),
+            Error::<Test>::NoUnbondingWhenNextSigner
+        );


Would you be able to move the nominator flow to standalone tests? This will make it easier to reason about the nominator flow and expected behaviour.

(This also applies to the two other tests below)

pallets/staking/src/lib.rs

pallets/staking/src/mock.rs

pallets/staking/src/lib.rs

HCastano

Main thing here is that I think we need to account for nominators in benches now.

Once that and the other review comments are addressed we should be good to go.

HCastano · 2024-09-16T16:43:13Z

pallets/staking/src/benchmarking.rs

@@ -124,11 +136,76 @@ benchmarks! {
    assert_last_event::<T>(Event::<T>::ThresholdAccountChanged(bonder, server_info).into());
  }

+  unbond {


Bumping this

pallets/staking/src/lib.rs

pallets/staking/src/mock.rs

pallets/staking/src/benchmarking.rs

CHANGELOG.md

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

…nbonding-when-signer-or-next-signer

* master: Bump clap from 4.5.18 to 4.5.19 in the patch-dependencies group (#1091) Avoid panic by checking that we have a non-signing validator before selecting one (#1083) Fix master build (#1088) Small fixes to `test-cli` (#1084) Pregenerate keyshares sets for all possible initial signer comittees (#1073) Fix master build (#1079) Bump reqwest from 0.12.7 to 0.12.8 in the patch-dependencies group (#1082) Block tss chain when signer (#1078) Run multiple test validator (#1074) Bump tempfile from 3.12.0 to 3.13.0 (#1076) Bump axum from 0.7.6 to 0.7.7 in the patch-dependencies group (#1075) Unignore register and sign integration test, and do a non-mock jumpstart (#1070) No unbonding when signer or next signer (#1031) Add `/relay_tx` endpoint (#1050) Fix how pre-generated keyshares are added for tests (#1061) Handle Provisioning Certification Keys (PCKs) (#1051) Bump async-trait from 0.1.82 to 0.1.83 in the patch-dependencies group (#1067)

JesseAbram added 5 commits August 26, 2024 17:10

No unbonding when signer or next signer

a3f6c12

test for chill and unbond

cef400f

fix bench

612efcf

add unbond and chill benches

8af8e2e

update benchmarks

4c8e089

JesseAbram requested review from HCastano and ameba23 August 27, 2024 23:13

JesseAbram marked this pull request as ready for review August 27, 2024 23:13

ameba23 approved these changes Aug 28, 2024

View reviewed changes

pallets/staking/src/lib.rs Outdated Show resolved Hide resolved

pallets/staking/src/lib.rs Outdated Show resolved Hide resolved

pallets/staking/src/lib.rs Outdated Show resolved Hide resolved

JesseAbram and others added 4 commits August 28, 2024 07:56

Apply suggestions from code review

8edf952

Co-authored-by: peg <peg@magmacollective.org>

clean

1730efb

lint

ae86a75

changelog

e83597d

HCastano requested changes Aug 28, 2024

View reviewed changes

clean

13af928

JesseAbram and others added 8 commits August 29, 2024 15:01

add in nominating check

f185d77

Update pallets/staking/src/benchmarking.rs

b3d4f2c

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

add tests for nominators

135632f

clean function

f689ca7

fmt

468418a

fmt

35d1aab

Merge branch 'master' of github.com:entropyxyz/entropy-core into no-u…

316b6c8

…nbonding-when-signer-or-next-signer

fmt

cfa6e3f

JesseAbram requested a review from HCastano August 30, 2024 22:20

HCastano reviewed Sep 13, 2024

View reviewed changes

HCastano reviewed Sep 16, 2024

View reviewed changes

JesseAbram and others added 5 commits September 16, 2024 12:33

Apply suggestions from code review

c83c99b

Co-authored-by: Hernando Castano <HCastano@users.noreply.github.com>

clean

069c9ce

Merge branch 'master' of github.com:entropyxyz/entropy-core into no-u…

2f9aac0

…nbonding-when-signer-or-next-signer

add nominator to benchmarks

82c3314

fmt

fc381d8

JesseAbram requested a review from HCastano September 23, 2024 22:58

HCastano approved these changes Sep 24, 2024

View reviewed changes

Merge branch 'master' of github.com:entropyxyz/entropy-core into no-u…

ba76d4b

…nbonding-when-signer-or-next-signer

JesseAbram merged commit e1eaf12 into master Sep 27, 2024
8 checks passed

JesseAbram deleted the no-unbonding-when-signer-or-next-signer branch September 27, 2024 02:05

HCastano added Bump `spec_version` A change which affects the current runtime behaviour Bump `transaction_version` A change which affects how existing extrinsics are created (e.g parameter changes) labels Oct 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

No unbonding when signer or next signer #1031

No unbonding when signer or next signer #1031

JesseAbram commented Aug 26, 2024 •

edited by HCastano

Loading

ameba23 left a comment •

edited

Loading

JesseAbram commented Aug 28, 2024

HCastano left a comment

HCastano Aug 28, 2024

HCastano Aug 28, 2024

HCastano Sep 16, 2024

HCastano commented Aug 28, 2024

JesseAbram commented Aug 28, 2024

JesseAbram commented Aug 28, 2024

HCastano commented Aug 28, 2024

JesseAbram commented Aug 28, 2024

HCastano left a comment

HCastano Sep 13, 2024

HCastano left a comment

HCastano Sep 16, 2024

No unbonding when signer or next signer #1031

No unbonding when signer or next signer #1031

Conversation

JesseAbram commented Aug 26, 2024 • edited by HCastano Loading

ameba23 left a comment • edited Loading

Choose a reason for hiding this comment

JesseAbram commented Aug 28, 2024

HCastano left a comment

Choose a reason for hiding this comment

HCastano Aug 28, 2024

Choose a reason for hiding this comment

HCastano Aug 28, 2024

Choose a reason for hiding this comment

HCastano Sep 16, 2024

Choose a reason for hiding this comment

HCastano commented Aug 28, 2024

JesseAbram commented Aug 28, 2024

JesseAbram commented Aug 28, 2024

HCastano commented Aug 28, 2024

JesseAbram commented Aug 28, 2024

HCastano left a comment

Choose a reason for hiding this comment

HCastano Sep 13, 2024

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

HCastano Sep 16, 2024

Choose a reason for hiding this comment

JesseAbram commented Aug 26, 2024 •

edited by HCastano

Loading

ameba23 left a comment •

edited

Loading