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

Integrate signer changes dynamically #978

Merged
merged 12 commits into from
Aug 6, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Binary file modified crates/client/entropy_metadata.scale
Binary file not shown.
4 changes: 4 additions & 0 deletions crates/shared/src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

pub const MAX_SIGNERS: u8 = 15;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


/// Threshold for those signers
pub const SIGNER_THRESHOLD: u8 = 2;

Expand Down
11 changes: 9 additions & 2 deletions pallets/parameters/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,16 @@ frame-support ={ version="29.0.0", default-features=false }
frame-system ={ version="29.0.0", default-features=false }
sp-runtime ={ version="32.0.0", default-features=false }
sp-std ={ version="14.0.0", default-features=false }
pallet-session ={ version="29.0.0", default-features=false }

entropy-shared={ version="0.2.0", path="../../crates/shared", features=[
"wasm-no-std",
], default-features=false }

[dev-dependencies]
sp-core={ version="29.0.0" }
sp-io ={ version="31.0.0" }
sp-core ={ version="29.0.0" }
sp-io ={ version="31.0.0" }
sp-staking={ version="27.0.0", default-features=false }

[features]
default=["std"]
Expand All @@ -32,6 +38,7 @@ runtime-benchmarks=[
std=[
"frame-support/std",
"frame-system/std",
"pallet-session/std",
"scale-info/std",
"sp-runtime/std",
"sp-std/std",
Expand Down
3 changes: 2 additions & 1 deletion pallets/parameters/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ benchmarks! {

change_signers_info {
let origin = T::UpdateOrigin::try_successful_origin().unwrap();
let signer_info = SignersSize { total_signers: 5, threshold: 3 };
pallet_session::CurrentIndex::<T>::put(1);
let signer_info = SignersSize { total_signers: 5, threshold: 3, last_session_change: 1 };
}: {
assert_ok!(
<Parameters<T>>::change_signers_info(origin, signer_info.total_signers, signer_info.threshold)
Expand Down
44 changes: 37 additions & 7 deletions pallets/parameters/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#![cfg_attr(not(feature = "std"), no_std)]
#![allow(clippy::unused_unit)]

use entropy_shared::MAX_SIGNERS;
use frame_support::pallet_prelude::*;
use frame_system::pallet_prelude::*;
use sp_runtime::DispatchResult;
Expand All @@ -56,7 +57,7 @@ pub mod module {
use super::*;

#[pallet::config]
pub trait Config: frame_system::Config {
pub trait Config: frame_system::Config + pallet_session::Config {
type RuntimeEvent: From<Event<Self>> + IsType<<Self as frame_system::Config>::RuntimeEvent>;

/// The origin which may set filter.
Expand Down Expand Up @@ -84,8 +85,11 @@ pub mod module {
assert!(self.total_signers >= self.threshold, "Threshold is larger then signer");
RequestLimit::<T>::put(self.request_limit);
MaxInstructionsPerPrograms::<T>::put(self.max_instructions_per_programs);
let signer_info =
SignersSize { total_signers: self.total_signers, threshold: self.threshold };
let signer_info = SignersSize {
total_signers: self.total_signers,
threshold: self.threshold,
last_session_change: 0,
};
SignersInfo::<T>::put(signer_info);
}
}
Expand All @@ -96,12 +100,23 @@ pub mod module {
ThresholdGreaterThenSigners,
/// Threhsold has to be more than 0
ThrehsoldTooLow,
/// Signers over max signers, can happen however needs a benchmark rerun
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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?

TooManySigners,
/// Signers can only change by one at a time
SignerDiffTooLarge,
/// Can only do one change per session
OneChangePerSession,
}

/// Signer info for the next reshare
#[derive(Clone, Encode, Decode, Eq, PartialEqNoBound, RuntimeDebug, TypeInfo, Default)]
pub struct SignersSize {
/// Next threshold amount
pub threshold: u8,
/// Total signers in signer party
pub total_signers: u8,
/// Last time it was changed (one change allowed per session)
pub last_session_change: u32,
Copy link
Contributor

@ameba23 ameba23 Aug 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

#[pallet::event]
Expand Down Expand Up @@ -137,7 +152,7 @@ pub mod module {
#[pallet::call]
impl<T: Config> Pallet<T> {
#[pallet::call_index(0)]
#[pallet::weight(T::WeightInfo::change_request_limit())]
#[pallet::weight( <T as Config>::WeightInfo::change_request_limit())]
pub fn change_request_limit(origin: OriginFor<T>, request_limit: u32) -> DispatchResult {
T::UpdateOrigin::ensure_origin(origin)?;
RequestLimit::<T>::put(request_limit);
Expand All @@ -146,7 +161,7 @@ pub mod module {
}

#[pallet::call_index(1)]
#[pallet::weight(T::WeightInfo::max_instructions_per_programs())]
#[pallet::weight( <T as Config>::WeightInfo::max_instructions_per_programs())]
pub fn change_max_instructions_per_programs(
origin: OriginFor<T>,
max_instructions_per_programs: u64,
Expand All @@ -161,7 +176,7 @@ pub mod module {

/// Changes the threshold related parameters for signing.
#[pallet::call_index(2)]
#[pallet::weight(T::WeightInfo::change_signers_info())]
#[pallet::weight( <T as Config>::WeightInfo::change_signers_info())]
pub fn change_signers_info(
origin: OriginFor<T>,
total_signers: u8,
Expand All @@ -170,7 +185,22 @@ pub mod module {
T::UpdateOrigin::ensure_origin(origin)?;
ensure!(total_signers >= threshold, Error::<T>::ThresholdGreaterThenSigners);
ensure!(threshold > 0, Error::<T>::ThrehsoldTooLow);
let signer_info = SignersSize { total_signers, threshold };
ensure!(total_signers <= MAX_SIGNERS, Error::<T>::TooManySigners);

let old_signer_info = Self::signers_info();
ensure!(
old_signer_info.total_signers.abs_diff(total_signers) <= 1,
Error::<T>::SignerDiffTooLarge
);

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

let signer_info =
SignersSize { total_signers, threshold, last_session_change: current_session };
SignersInfo::<T>::put(&signer_info);
Self::deposit_event(Event::SignerInfoChanged { signer_info });
Ok(())
Expand Down
56 changes: 54 additions & 2 deletions pallets/parameters/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,15 @@

use frame_support::{
construct_runtime, derive_impl, ord_parameter_types,
traits::{ConstU64, Everything},
traits::{ConstU64, Everything, OneSessionHandler},
};
use frame_system::EnsureRoot;
use sp_core::H256;
use sp_runtime::{traits::IdentityLookup, BuildStorage};
use sp_runtime::{
testing::UintAuthorityId,
traits::{ConvertInto, IdentityLookup},
BuildStorage,
};

use super::*;

Expand Down Expand Up @@ -58,6 +62,52 @@ impl frame_system::Config for Runtime {
type Version = ();
}

pub struct MockSessionManager;
impl pallet_session::SessionManager<AccountId> for MockSessionManager {
fn end_session(_: sp_staking::SessionIndex) {}
fn start_session(_: sp_staking::SessionIndex) {}
fn new_session(_: sp_staking::SessionIndex) -> Option<Vec<AccountId>> {
None
}
}

pub struct OtherSessionHandler;
impl OneSessionHandler<AccountId> for OtherSessionHandler {
type Key = UintAuthorityId;

fn on_genesis_session<'a, I: 'a>(_: I)
where
I: Iterator<Item = (&'a AccountId, Self::Key)>,
AccountId: 'a,
{
}

fn on_new_session<'a, I: 'a>(_: bool, _: I, _: I)
where
I: Iterator<Item = (&'a AccountId, Self::Key)>,
AccountId: 'a,
{
}

fn on_disabled(_validator_index: u32) {}
}

impl sp_runtime::BoundToRuntimeAppPublic for OtherSessionHandler {
type Public = UintAuthorityId;
}

impl pallet_session::Config for Runtime {
type RuntimeEvent = RuntimeEvent;
type ValidatorId = u128;
type ValidatorIdOf = ConvertInto;
type ShouldEndSession = pallet_session::PeriodicSessions<ConstU64<1>, ConstU64<0>>;
type NextSessionRotation = pallet_session::PeriodicSessions<ConstU64<1>, ConstU64<0>>;
type SessionManager = MockSessionManager;
type SessionHandler = (OtherSessionHandler,);
type Keys = UintAuthorityId;
type WeightInfo = ();
}

ord_parameter_types! {
pub const One: AccountId = 1;
}
Expand All @@ -75,6 +125,8 @@ construct_runtime!(
{
System: frame_system,
Parameters: pallet_parameters,
Session: pallet_session,

}
);

Expand Down
36 changes: 32 additions & 4 deletions pallets/parameters/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,13 @@

#![cfg(test)]

use super::*;
use crate::SignersInfo;
use entropy_shared::MAX_SIGNERS;
use frame_support::{assert_noop, assert_ok};
use mock::*;
use sp_runtime::traits::BadOrigin;

use super::*;

#[test]
fn request_limit_changed() {
new_test_ext().execute_with(|| {
Expand Down Expand Up @@ -65,8 +66,10 @@ fn max_instructions_per_programs_changed() {
#[test]
fn signer_info_changed() {
new_test_ext().execute_with(|| {
let signer_info = SignersSize { total_signers: 5, threshold: 3 };
let new_signer_info = SignersSize { total_signers: 6, threshold: 4 };
pallet_session::CurrentIndex::<Runtime>::put(1);
let signer_info = SignersSize { total_signers: 5, threshold: 3, last_session_change: 0 };
let new_signer_info =
SignersSize { total_signers: 6, threshold: 4, last_session_change: 1 };

assert_eq!(Parameters::signers_info(), signer_info, "Inital signer info set");

Expand Down Expand Up @@ -99,5 +102,30 @@ fn signer_info_changed() {
Parameters::change_signers_info(RuntimeOrigin::root(), 0, 0),
Error::<Runtime>::ThrehsoldTooLow,
);

// Fails too many signers
assert_noop!(
Parameters::change_signers_info(RuntimeOrigin::root(), MAX_SIGNERS + 1, 1),
Error::<Runtime>::TooManySigners,
);

assert_noop!(
Parameters::change_signers_info(
RuntimeOrigin::root(),
new_signer_info.total_signers + 2,
signer_info.threshold
),
Error::<Runtime>::SignerDiffTooLarge,
);
SignersInfo::<Runtime>::put(new_signer_info);

assert_noop!(
Parameters::change_signers_info(
RuntimeOrigin::root(),
signer_info.total_signers,
signer_info.threshold
),
Error::<Runtime>::OneChangePerSession,
Copy link
Contributor

@ameba23 ameba23 Aug 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed as per your previous comment, good catch

);
});
}
16 changes: 8 additions & 8 deletions pallets/registry/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// along with this program. If not, see <https://www.gnu.org/licenses/>.

//! Benchmarking setup for pallet-propgation
use entropy_shared::{TOTAL_SIGNERS, VERIFICATION_KEY_LENGTH};
use entropy_shared::{MAX_SIGNERS, VERIFICATION_KEY_LENGTH};
use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller};
use frame_support::{
traits::{Currency, Get},
Expand Down Expand Up @@ -79,20 +79,20 @@ benchmarks! {
}

confirm_jump_start_done {
let c in 0 .. TOTAL_SIGNERS as u32;
let c in 0 .. MAX_SIGNERS as u32;
let sig_req_account: T::AccountId = whitelisted_caller();
let validator_account: T::AccountId = whitelisted_caller();
let expected_verifying_key = BoundedVec::default();

let mut accounts = vec![];
for i in 0..TOTAL_SIGNERS {
for i in 0..MAX_SIGNERS {
accounts.push(account::<T::AccountId>("ts_account", i as u32, SEED));
}

let validators = add_non_syncing_validators::<T>(TOTAL_SIGNERS as u32, 0);
let validators = add_non_syncing_validators::<T>(MAX_SIGNERS as u32, 0);
<Validators<T>>::set(validators.clone());

for i in 0..TOTAL_SIGNERS {
for i in 0..MAX_SIGNERS {
<ThresholdToStash<T>>::insert(accounts[i as usize].clone(), &validators[i as usize]);
}

Expand All @@ -112,15 +112,15 @@ benchmarks! {
}

confirm_jump_start_confirm {
let c in 0 .. TOTAL_SIGNERS as u32;
let c in 0 .. MAX_SIGNERS as u32;
let sig_req_account: T::AccountId = whitelisted_caller();
let validator_account: T::AccountId = whitelisted_caller();
let threshold_account: T::AccountId = whitelisted_caller();
let expected_verifying_key = BoundedVec::default();

// add validators and a registering user
for i in 0..TOTAL_SIGNERS {
let validators = add_non_syncing_validators::<T>(TOTAL_SIGNERS as u32, 0);
for i in 0..MAX_SIGNERS {
let validators = add_non_syncing_validators::<T>(MAX_SIGNERS as u32, 0);
<Validators<T>>::set(validators.clone());
<ThresholdToStash<T>>::insert(&threshold_account, &validators[i as usize]);
}
Expand Down
9 changes: 5 additions & 4 deletions pallets/registry/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ pub mod weights;

#[frame_support::pallet]
pub mod pallet {
use entropy_shared::{NETWORK_PARENT_KEY, TOTAL_SIGNERS, VERIFICATION_KEY_LENGTH};
use entropy_shared::{MAX_SIGNERS, NETWORK_PARENT_KEY, VERIFICATION_KEY_LENGTH};
use frame_support::{
dispatch::{DispatchResultWithPostInfo, Pays},
pallet_prelude::*,
Expand Down Expand Up @@ -286,8 +286,8 @@ pub mod pallet {
/// Allows validators to signal a successful network jumpstart
#[pallet::call_index(1)]
#[pallet::weight({
<T as Config>::WeightInfo::confirm_jump_start_confirm(TOTAL_SIGNERS as u32)
.max(<T as Config>::WeightInfo::confirm_jump_start_done(TOTAL_SIGNERS as u32))
<T as Config>::WeightInfo::confirm_jump_start_confirm(MAX_SIGNERS as u32)
.max(<T as Config>::WeightInfo::confirm_jump_start_done(MAX_SIGNERS as u32))
})]
pub fn confirm_jump_start(
origin: OriginFor<T>,
Expand Down Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if jump_start_info.confirmations.len() == (signers_amount as usize - 1) {
// registration finished, lock call
jump_start_info.confirmations.push(validator_stash);
let confirmations = jump_start_info.confirmations.len();
Expand Down
Loading
Loading