Integrate signer changes dynamically #978

JesseAbram · 2024-08-02T16:07:09Z

Have next_signers and signers bounded by N in parameters
Has Jumpstart bounded by N

This also adds bounding to changing N in parameters, by only allowing 1 signer to be added per session, and in session if the signer size is increased it will add one new signer and not remove a signer instead of adding 2 new ones

ameba23

🥇 Looks good.

ameba23 · 2024-08-06T06:25:40Z

crates/shared/src/constants.rs

@@ -64,6 +64,10 @@ pub const NETWORK_PARENT_KEY: &str = "NETWORK_PARENT_KEY_FOR_ENTROPY_";
 /// Total signers on the network with the parent key
 pub const TOTAL_SIGNERS: u8 = 3;

+/// Max signers, for bounding of total signers for benches,
+/// Can be changed but requires a re-run of benches
+pub const MAX_SIGNERS: u8 = 15;


ameba23 · 2024-08-06T06:37:42Z

pallets/parameters/src/lib.rs

+            );
+            let current_session = pallet_session::Pallet::<T>::current_index();
+            ensure!(
+                current_session >= old_signer_info.last_session_change,


Should this be > rather than >=?

ameba23 · 2024-08-06T06:39:25Z

pallets/parameters/src/lib.rs

    }

    #[derive(Clone, Encode, Decode, Eq, PartialEqNoBound, RuntimeDebug, TypeInfo, Default)]
    pub struct SignersSize {
        pub threshold: u8,
        pub total_signers: u8,
+        pub last_session_change: u32,


A doccomment would be nice. If i understood right this is the session index number of the last session in which threshold or total_signers changed.

ameba23 · 2024-08-06T06:45:51Z

pallets/parameters/src/tests.rs

+                signer_info.total_signers,
+                signer_info.threshold
+            ),
+            Error::<Runtime>::OneChangePerSession,


If i understood right this tests that you cannot make a change when the current session is older than last_session_change (which is hopefully an impossible state to get into), but not that you cannot make two changes in the same session.

fixed as per your previous comment, good catch

ameba23 · 2024-08-06T06:53:08Z

pallets/staking/src/lib.rs

+            // removes first signer and pushes new signer to back if total signers not increased
+            if current_signers.len() >= signers_info.total_signers as usize {
+                current_signers.remove(0);
+            }
            current_signers.push(next_signer_up.clone());


What happens if we decrease total_signers? I guess here we have a similar if as above, and only push the next_signer_up if total_signers did not decrease.

ameba23 · 2024-08-06T06:55:58Z

pallets/staking/src/tests.rs

+        });
+        assert_ok!(Staking::new_session_handler(&[6, 5, 3, 4]));
+        // Signer size increased is reflected as 5 is not removed from vec
+        assert_eq!(Staking::next_signers().unwrap().next_signers, vec![5, 6, 3]);


Maybe we also want a test for decreasing signer size

HCastano

Left a few comments and questions, but overall looks good 👍

HCastano · 2024-08-06T14:25:40Z

pallets/parameters/src/lib.rs

    }

    #[derive(Clone, Encode, Decode, Eq, PartialEqNoBound, RuntimeDebug, TypeInfo, Default)]
    pub struct SignersSize {
        pub threshold: u8,
        pub total_signers: u8,
+        pub last_session_change: u32,


We can probably use the SessionIndex type directly here (which is typedef'd to a u32).

ya but then I would have to pull in sp-staking pallet, a whole new crate for one type does not seem worth it

pallets/parameters/src/lib.rs

HCastano · 2024-08-06T14:37:56Z

pallets/parameters/src/lib.rs

@@ -96,12 +100,19 @@ pub mod module {
        ThresholdGreaterThenSigners,
        /// Threhsold has to be more than 0
        ThrehsoldTooLow,
+        /// Signers over max signers, can happen however needs a benchmark rerun


What benchmark are you referring to here?

If it's the change_signers_info() one this isn't required. The extrinsic is of constant time and doesn't change based off the inputs.

any bench that we loop over the signers, a few, like confirm_jump_start, confirm_reshare, anything with confirm mainly because it has to check if the current signer is in the signers struct which is O(n)

@JesseAbram so these actually don't scale with the number of signers. In the case of confirm_jump_start_confirm() the c parameter doesn't influence the final weight. Also reading through the extrinsic we're only doing constant time storage queries, no loops.

Maybe can you point me to where the O(n) operations are happening?

HCastano · 2024-08-06T14:47:35Z

pallets/parameters/src/tests.rs

+        );
+
+        assert_noop!(
+            Parameters::change_signers_info(RuntimeOrigin::root(), 8, signer_info.threshold),


Instead of 8 here I would do something like new_threshold = old_signer_info.threshold + 2 and use that instead

HCastano · 2024-08-06T14:49:35Z

pallets/registry/src/lib.rs

@@ -327,7 +327,8 @@ pub mod pallet {
            // ensure that registration was indeed successful.
            //
            // If it fails we'll need to allow another jumpstart.
-            if jump_start_info.confirmations.len() == (TOTAL_SIGNERS as usize - 1) {
+            let signers_amount = pallet_parameters::Pallet::<T>::signers_info().total_signers;


Suggested change

let signers_amount = pallet_parameters::Pallet::<T>::signers_info().total_signers;

let total_signers = pallet_parameters::Pallet::<T>::signers_info().total_signers;

For consistency

HCastano · 2024-08-06T14:51:19Z

pallets/staking/src/mock.rs

@@ -409,7 +409,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities {
    pallet_parameters::GenesisConfig::<Test> {
        request_limit: 5u32,
        max_instructions_per_programs: 5u64,
-        total_signers: 5u8,
+        total_signers: 2u8,


I would change this to 3 to match the Registry config, and then in the test change the total_signers to 4. This makes testing across the pallets more predictable

HCastano · 2024-08-06T14:53:00Z

pallets/staking/src/tests.rs

+
+        pallet_parameters::SignersInfo::<Test>::put(SignersSize {
+            total_signers: 3,
+            threshold: 2,
+            last_session_change: 0,
+        });
+        assert_ok!(Staking::new_session_handler(&[6, 5, 3, 4]));
+        // Signer size increased is reflected as 5 is not removed from vec
+        assert_eq!(Staking::next_signers().unwrap().next_signers, vec![5, 6, 3]);


Would you be able to throw these (and the few lines of code above) into a separate test? While it does test the session handler behaviour, it's more of specific test related to the management of the signers at the session boundary

HCastano · 2024-08-06T14:56:43Z

crates/shared/src/constants.rs

@@ -64,6 +64,10 @@ pub const NETWORK_PARENT_KEY: &str = "NETWORK_PARENT_KEY_FOR_ENTROPY_";
 /// Total signers on the network with the parent key
 pub const TOTAL_SIGNERS: u8 = 3;

+/// Max signers, for bounding of total signers for benches,
+/// Can be changed but requires a re-run of benches


Suggested change

/// Can be changed but requires a re-run of benches

Similar thing here, if I'm thinking of the right extrinsic it's constant time

Co-authored-by: Hernando Castano <[email protected]>

JesseAbram added 7 commits August 2, 2024 12:06

add checks to signer changes

4f6a1ba

implement signer addition

f96e25a

add test for signer has not changed

b0c6016

add session tests

dba0a95

clean

842d108

fix test

a0299f7

fix benches

d39cb86

JesseAbram marked this pull request as ready for review August 5, 2024 16:56

JesseAbram requested review from ameba23 and HCastano August 5, 2024 16:56

ameba23 approved these changes Aug 6, 2024

View reviewed changes

HCastano approved these changes Aug 6, 2024

View reviewed changes

JesseAbram and others added 5 commits August 6, 2024 11:34

fix signer decrease logic

d7c5c5f

clean

5864614

doc

426bca8

Apply suggestions from code review

fc3b1ed

Co-authored-by: Hernando Castano <[email protected]>

clean

0942869

JesseAbram merged commit 7454094 into master Aug 6, 2024
14 checks passed

JesseAbram deleted the incorporated-max-signers-dynamically branch August 6, 2024 16:55

github-actions bot locked and limited conversation to collaborators Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate signer changes dynamically #978

Integrate signer changes dynamically #978

JesseAbram commented Aug 2, 2024 •

edited

Loading

ameba23 left a comment

ameba23 Aug 6, 2024

ameba23 Aug 6, 2024

ameba23 Aug 6, 2024 •

edited

Loading

ameba23 Aug 6, 2024 •

edited

Loading

JesseAbram Aug 6, 2024

ameba23 Aug 6, 2024

JesseAbram Aug 6, 2024

ameba23 Aug 6, 2024

HCastano left a comment

HCastano Aug 6, 2024

JesseAbram Aug 6, 2024

HCastano Aug 6, 2024

JesseAbram Aug 6, 2024

HCastano Aug 6, 2024

HCastano Aug 6, 2024

HCastano Aug 6, 2024

HCastano Aug 6, 2024

HCastano Aug 6, 2024

HCastano Aug 6, 2024

	let signers_amount = pallet_parameters::Pallet::<T>::signers_info().total_signers;
	let total_signers = pallet_parameters::Pallet::<T>::signers_info().total_signers;

Integrate signer changes dynamically #978

Integrate signer changes dynamically #978

Conversation

JesseAbram commented Aug 2, 2024 • edited Loading

ameba23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameba23 Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

ameba23 Aug 6, 2024 • 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

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

JesseAbram commented Aug 2, 2024 •

edited

Loading

ameba23 Aug 6, 2024 •

edited

Loading

ameba23 Aug 6, 2024 •

edited

Loading