Add Signer groups and rotation #938

JesseAbram · 2024-07-16T16:10:29Z

Set parameters for size of signer group and T of N
Add Signer groups to chain
Get randomness to select new signer from validators on session refresh
Rotate Signers

JesseAbram · 2024-07-16T20:58:35Z

runtime/src/lib.rs

@@ -714,6 +714,7 @@ parameter_types! {
 impl pallet_staking_extension::Config for Runtime {
    type Currency = Balances;
    type MaxEndpointLength = MaxEndpointLength;
+    type Randomness = pallet_babe::RandomnessFromOneEpochAgo<Runtime>;


HCastano

Don't think this needs an impl_version bump since we're already bumping the spec_version. I also don't think the transaction_version needs to be bumped since only new stuff is being added, as opposed to existing extrinsics being changed

HCastano · 2024-07-16T21:18:56Z

pallets/staking/src/lib.rs

+    #[pallet::storage]
+    #[pallet::getter(fn signers)]
+    pub type Signers<T: Config> = StorageValue<_, Vec<T::ValidatorId>, ValueQuery>;
+
+    #[pallet::storage]
+    #[pallet::getter(fn next_signers)]
+    pub type NextSigners<T: Config> = StorageValue<_, Vec<T::ValidatorId>, ValueQuery>;


Can you add docs to these?

HCastano · 2024-07-16T21:23:06Z

pallets/staking/Cargo.toml

  'sp-runtime/std',
-
+  "rand_chacha/std",


Why use chaha20 here instead of something else?

saw it used in the society pallet, when I have a choice like this to make I default to auditted production code, open to other ideas

HCastano · 2024-07-16T21:24:10Z

pallets/staking/src/lib.rs

+            // TODO: Is randomness freshness an issue here
+            // https://github.com/paritytech/substrate/issues/8312
+            let (seed, _) = T::Randomness::random(phrase);


So does this mean that we should also be checking how fresh the randomness is here before committing to use it ?

ummmmmmm no I don't believe so this is like more of a commit reveal thing where this can be useful, as in like idk a lottery, bets are placed, there is a clear end time then you grab randomness, but the way we use randomness is in an ever flowing system where validators are swapped in and out, potentially we can grab the randomness from a previous session, some more investitgation on how validators are ordered may help here, but good randomness is probably enough

In any case this should be hashed out in #944

HCastano · 2024-07-16T21:25:42Z

pallets/staking/src/lib.rs

+            let mut current_signers = Self::signers();
+            // Since not enough validators do not allow rotation
+            // TODO: https://github.com/entropyxyz/entropy-core/issues/943
+            if validators.len() <= current_signers.len() {


In the case that validators.len() == current_signers.len() why not allow rotation?

do have #943 open for discussion about this, but if we do not let signers leave validators while in the signer group (which is also a discussion lol) then we can assume all validators are signers here and we can not let them leave in fear of losing the parent key

If validators.len() == current_signers.len() then we still want to run the last bit of this function, which is currently just a comment - as in we still do a reshare with the same signer set.

validators.len() < current_signers.len() i guess should be a state which is impossible to get into. Not sure how to handle that.

ya probably not a reshare but a refresh instead, can you add this comment to #943

HCastano · 2024-07-16T21:31:29Z

pallets/staking/src/lib.rs

+            let mut next_signer_up = &current_signers[0].clone();
+            let mut index;
+            // loops to find signer in validator that is not already signer
+            while current_signers.contains(next_signer_up) {


This is a pretty expensive lookup to do on a Vec, esp with random indexes, can we get away with making Signers a map?

mmmmm ok so we do know signers is relatively small and this would cause other operations (coming in future PRs) to increase complexity (happy to explain them but honestly may be better to see in a few days) that being said Im open to making this an issue and going back over to optimise this, but for now I feel strongly about leaving this as is to push through the flow

pallets/parameters/src/lib.rs

node/cli/src/chain_spec/testnet.rs

pallets/parameters/src/lib.rs

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

ameba23

Looks great 💯 (pending CI)

Just to confirm the scope of this PR is to add a signing committee concept to the staking pallet, but not yet actually use them.

I haven't looked into the issue around 'freshness' relating to randomness, and obviously having a good source of randomness is pretty critical here, but since there is a comment and #944 i think we should move ahead with this.

As discussed elsewhere, as well as randomness there are other factors we could use in this selection process eg: stake, age of validator, and number of blocks since a validator was last in the signing committee.

As for the frequency of rotation (moving one validator in/out per session), i think this is too frequent, and should probably also be configurable as a parameter, or even depend on this size of the validator set (the more validators, the more frequent the rotation). As i mentioned before, my main concern is that the more frequent the rotation, the higher the chance that a malicious actor who controls n nodes will control the complete signing committee. But for simplicity i think we should leave this as it is for now and merge this.

As for having t and n configurable as a parameter. I totally agree with this, and i can't see a reason why changing them with a running network would cause issues. But of course issues with this could well pop up.

ameba23 · 2024-07-17T08:19:05Z

pallets/staking/src/lib.rs

+            }
+
+            // removes first signer and pushes new signer to back
+            current_signers.remove(0);


i dont think we need to worry too much about optimising for performance here as this only runs once per session and we know that current_signers is small, but i agree that VecDeque is better suited to this.

i am totally down with removing the first current signer and pushing the new one to the end. 👍

ameba23 · 2024-07-17T08:32:02Z

pallets/staking/src/lib.rs

+            let mut current_signers = Self::signers();
+            // Since not enough validators do not allow rotation
+            // TODO: https://github.com/entropyxyz/entropy-core/issues/943
+            if validators.len() <= current_signers.len() {


If validators.len() == current_signers.len() then we still want to run the last bit of this function, which is currently just a comment - as in we still do a reshare with the same signer set.

validators.len() < current_signers.len() i guess should be a state which is impossible to get into. Not sure how to handle that.

ameba23 · 2024-07-17T08:48:22Z

pallets/parameters/src/lib.rs

+        ) -> DispatchResult {
+            T::UpdateOrigin::ensure_origin(origin)?;
+            let signer_info = SignersSize { total_signers, threshold };
+            // TODO: add checks to make sure threshold is not bigger then signature size


and that threshold is greater than 1

Why are these checks being left as a TODO? For some quick ones, e.g Peg's suggestion or threshold <= total_signers we should be able to do now

ya imma add them now

pallets/parameters/src/benchmarking.rs

pallets/parameters/src/lib.rs

HCastano · 2024-07-17T13:54:01Z

pallets/parameters/src/lib.rs

+        ) -> DispatchResult {
+            T::UpdateOrigin::ensure_origin(origin)?;
+            let signer_info = SignersSize { total_signers, threshold };
+            // TODO: add checks to make sure threshold is not bigger then signature size


Why are these checks being left as a TODO? For some quick ones, e.g Peg's suggestion or threshold <= total_signers we should be able to do now

HCastano · 2024-07-17T14:04:00Z

pallets/staking/src/tests.rs

+        // no next signers at start
+        assert_eq!(Staking::next_signers().len(), 0);
+
+        Staking::new_session_handler(&vec![1u64, 2u64, 3u64]);


You shouldn't need to explicitly specify the u64 type in this whole test

pallets/staking/src/tests.rs

HCastano · 2024-07-17T14:07:03Z

node/cli/src/chain_spec/testnet.rs

+            inital_signers: initial_authorities.iter().map(|auth| {
+                auth.0.clone()
+            })
+            .collect::<Vec<_>>(),


For the testnet config we may want it to be empty actually, and instead have it be populated through a different jumpstart related mechanism.

maybe, let's open an issue here, too much discussion to be had and let's not slow down the rest of the flow

actually covered in #941

CHANGELOG.md

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

JesseAbram added 3 commits July 16, 2024 12:06

Add Signer groups and randmoness

731e205

add parameters

d464a99

add signer roation

354fa14

JesseAbram mentioned this pull request Jul 16, 2024

Validator joining flow #941

Closed

19 tasks

JesseAbram added 2 commits July 16, 2024 16:40

changelog

8017fea

docs

f633109

JesseAbram marked this pull request as ready for review July 16, 2024 20:48

lint

09bcd5a

JesseAbram requested review from HCastano and ameba23 July 16, 2024 20:53

JesseAbram commented Jul 16, 2024

View reviewed changes

HCastano reviewed Jul 16, 2024

View reviewed changes

JesseAbram and others added 2 commits July 16, 2024 15:32

Apply suggestions from code review

c1e24fa

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

fixes

038dba3

JesseAbram requested a review from HCastano July 16, 2024 22:48

ameba23 approved these changes Jul 17, 2024

View reviewed changes

fix compile

4b47d88

HCastano approved these changes Jul 17, 2024

View reviewed changes

HCastano removed Bump `impl_version` A change which only affect the implementation details of the runtime Bump `transaction_version` A change which affects how existing extrinsics are created (e.g parameter changes) labels Jul 17, 2024

JesseAbram and others added 5 commits July 17, 2024 10:18

add threshold and signer checks

c312673

remove todo

7c09edf

Apply suggestions from code review

5432748

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

update metadata

a757d27

fix

c2f5436

JesseAbram merged commit 9549194 into master Jul 17, 2024
13 checks passed

JesseAbram deleted the signer-rotation-groups branch July 17, 2024 16:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Signer groups and rotation #938

Add Signer groups and rotation #938

JesseAbram commented Jul 16, 2024 •

edited

Loading

JesseAbram Jul 16, 2024

HCastano left a comment

HCastano Jul 16, 2024

HCastano Jul 16, 2024

JesseAbram Jul 16, 2024

HCastano Jul 16, 2024

JesseAbram Jul 16, 2024

JesseAbram Jul 16, 2024

HCastano Jul 16, 2024

JesseAbram Jul 16, 2024

ameba23 Jul 17, 2024

JesseAbram Jul 17, 2024

HCastano Jul 16, 2024

JesseAbram Jul 16, 2024

ameba23 left a comment •

edited

Loading

ameba23 Jul 17, 2024

ameba23 Jul 17, 2024

ameba23 Jul 17, 2024

HCastano Jul 17, 2024

JesseAbram Jul 17, 2024

HCastano Jul 17, 2024

HCastano Jul 17, 2024

HCastano Jul 17, 2024

JesseAbram Jul 17, 2024

JesseAbram Jul 17, 2024

Add Signer groups and rotation #938

Add Signer groups and rotation #938

Conversation

JesseAbram commented Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameba23 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram commented Jul 16, 2024 •

edited

Loading

ameba23 left a comment •

edited

Loading