From bd03eb710b5c0cbb5a223d609eb9dca44a632e23 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Fri, 17 Apr 2020 12:42:47 +0200 Subject: [PATCH 01/34] add old_registrar_count as param to estimate weight --- frame/identity/src/lib.rs | 42 ++++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index ddb9bdcce2163..f56e359f06ef7 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -74,7 +74,7 @@ use sp_runtime::traits::{StaticLookup, Zero, AppendZerosInput}; use frame_support::{ decl_module, decl_event, decl_storage, ensure, decl_error, traits::{Currency, ReservableCurrency, OnUnbalanced, Get, BalanceStatus, EnsureOrigin}, - weights::{SimpleDispatchInfo, MINIMUM_WEIGHT}, + weights::{SimpleDispatchInfo, FunctionOf, DispatchClass}, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -474,18 +474,28 @@ decl_module! { /// - One storage mutation (codec `O(R)`). /// - One event. /// # - #[weight = SimpleDispatchInfo::FixedNormal(MINIMUM_WEIGHT)] - fn add_registrar(origin, account: T::AccountId) { + #[weight = FunctionOf( + |(_, &old_count): (&T::AccountId, &u32)| { + T::DbWeight::get().reads_writes(1, 1) + + 500_000 * (old_count as u64) + }, + DispatchClass::Normal, + true + )] + fn add_registrar(origin, account: T::AccountId, old_registrar_count: u32) -> DispatchResult { T::RegistrarOrigin::try_origin(origin) .map(|_| ()) .or_else(ensure_root)?; + let mut registrars = Self::registrars(); + ensure!(registrars.len() as u32 <= old_registrar_count, "invalid count"); - let i = >::mutate(|r| { - r.push(Some(RegistrarInfo { account, fee: Zero::zero(), fields: Default::default() })); - (r.len() - 1) as RegistrarIndex - }); + registrars.push(Some(RegistrarInfo { account, fee: Zero::zero(), fields: Default::default() })); + let i = (registrars.len() - 1) as RegistrarIndex; + >::put(registrars); Self::deposit_event(RawEvent::RegistrarAdded(i)); + + Ok(()) } /// Set an account's identity information and reserve the appropriate deposit. @@ -1015,7 +1025,7 @@ mod tests { #[test] fn adding_registrar_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); let fields = IdentityFields(IdentityField::Display | IdentityField::Legal); assert_ok!(Identity::set_fields(Origin::signed(3), 0, fields)); @@ -1028,7 +1038,7 @@ mod tests { #[test] fn registration_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); let mut three_fields = ten(); three_fields.additional.push(Default::default()); @@ -1055,7 +1065,7 @@ mod tests { Error::::InvalidIndex ); - assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_noop!( Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable), Error::::InvalidTarget @@ -1079,7 +1089,7 @@ mod tests { #[test] fn clearing_judgement_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable)); assert_ok!(Identity::clear_identity(Origin::signed(10))); @@ -1165,7 +1175,7 @@ mod tests { #[test] fn cancelling_requested_judgement_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::NoIdentity); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); @@ -1182,7 +1192,7 @@ mod tests { #[test] fn requesting_judgement_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 9), Error::::FeeChanged); @@ -1200,7 +1210,7 @@ mod tests { assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10), Error::::StickyJudgement); // Requesting from a second registrar still works. - assert_ok!(Identity::add_registrar(Origin::signed(1), 4)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 4, 1)); assert_ok!(Identity::request_judgement(Origin::signed(10), 1, 10)); // Re-requesting after the judgement has been reduced works. @@ -1212,7 +1222,7 @@ mod tests { #[test] fn field_deposit_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_ok!(Identity::set_identity(Origin::signed(10), IdentityInfo { additional: vec![ @@ -1227,7 +1237,7 @@ mod tests { #[test] fn setting_account_id_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); // account 4 cannot change the first registrar's identity since it's owned by 3. assert_noop!(Identity::set_account_id(Origin::signed(4), 0, 3), Error::::InvalidIndex); // account 3 can, because that's the registrar's current account. From eeaf09f2d688548dc105e35b9a1217518ca8d6a2 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Fri, 17 Apr 2020 15:43:13 +0200 Subject: [PATCH 02/34] cast count to Weight Co-Authored-By: Shawn Tabrizi --- frame/identity/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index f56e359f06ef7..eedc7958b1016 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -477,7 +477,7 @@ decl_module! { #[weight = FunctionOf( |(_, &old_count): (&T::AccountId, &u32)| { T::DbWeight::get().reads_writes(1, 1) - + 500_000 * (old_count as u64) + + 500_000 * (old_count as Weight) }, DispatchClass::Normal, true From abf2bf229717c66c6e69d03e99e99e3cb196ddec Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Fri, 17 Apr 2020 16:15:34 +0200 Subject: [PATCH 03/34] add weight calculation for set_identity --- frame/identity/src/lib.rs | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index eedc7958b1016..0859da9386755 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -74,7 +74,7 @@ use sp_runtime::traits::{StaticLookup, Zero, AppendZerosInput}; use frame_support::{ decl_module, decl_event, decl_storage, ensure, decl_error, traits::{Currency, ReservableCurrency, OnUnbalanced, Get, BalanceStatus, EnsureOrigin}, - weights::{SimpleDispatchInfo, FunctionOf, DispatchClass}, + weights::{SimpleDispatchInfo, FunctionOf, DispatchClass, Weight}, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -83,6 +83,9 @@ mod benchmarking; type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; type NegativeImbalanceOf = <::Currency as Currency<::AccountId>>::NegativeImbalance; +/// Upper bound for the amount of registrars expected. +const MAX_REGISTRARS: u32 = 20; + pub trait Trait: frame_system::Trait { /// The overarching event type. type Event: From> + Into<::Event>; @@ -477,7 +480,7 @@ decl_module! { #[weight = FunctionOf( |(_, &old_count): (&T::AccountId, &u32)| { T::DbWeight::get().reads_writes(1, 1) - + 500_000 * (old_count as Weight) + + 500_000 * (old_count as Weight) // R }, DispatchClass::Normal, true @@ -503,20 +506,30 @@ decl_module! { /// If the account already has identity information, the deposit is taken as part payment /// for the new deposit. /// - /// The dispatch origin for this call must be _Signed_ and the sender must have a registered - /// identity. + /// The dispatch origin for this call must be _Signed_. /// /// - `info`: The identity information. /// /// Emits `IdentitySet` if successful. /// /// # - /// - `O(X + X' + R)` where `X` additional-field-count (deposit-bounded and code-bounded). + /// - `O(X + X' + J)` + /// - where `X` additional-field-count (deposit-bounded and code-bounded) + /// - where `J` judgements-count (registrar-count-bounded) /// - At most two balance operations. - /// - One storage mutation (codec-read `O(X' + R)`, codec-write `O(X + R)`). + /// - One storage mutation (codec-read `O(X' + J)`, codec-write `O(X + J)`). /// - One event. /// # - #[weight = SimpleDispatchInfo::FixedNormal(50_000_000)] + // #[weight = + SimpleDispatchInfo::FixedNormal(50_000_000)] + #[weight = FunctionOf( + |(info,): (&IdentityInfo,)| { + T::DbWeight::get().reads_writes(2, 2) + + 500_000 * (info.additional.len() as Weight) // X + + 500_000 * (MAX_REGISTRARS as Weight) // J + }, + DispatchClass::Normal, + true + )] fn set_identity(origin, info: IdentityInfo) { let sender = ensure_signed(origin)?; let extra_fields = info.additional.len() as u32; From b4cb9defe6f100eef109339f6333fafbd5a0d5af Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Fri, 17 Apr 2020 16:36:09 +0200 Subject: [PATCH 04/34] remove superfluous weight comment --- frame/identity/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 0859da9386755..537f90b1f3027 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -520,7 +520,6 @@ decl_module! { /// - One storage mutation (codec-read `O(X' + J)`, codec-write `O(X + J)`). /// - One event. /// # - // #[weight = + SimpleDispatchInfo::FixedNormal(50_000_000)] #[weight = FunctionOf( |(info,): (&IdentityInfo,)| { T::DbWeight::get().reads_writes(2, 2) From 77e45684ae77ffd239cdacd9e15ff960fabf9560 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Mon, 20 Apr 2020 11:01:32 +0200 Subject: [PATCH 05/34] add detailed weight estimation for set_subs --- frame/identity/src/lib.rs | 45 +++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 537f90b1f3027..8a31bf2a12a5b 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -522,7 +522,8 @@ decl_module! { /// # #[weight = FunctionOf( |(info,): (&IdentityInfo,)| { - T::DbWeight::get().reads_writes(2, 2) + T::DbWeight::get().reads_writes(1, 1) + + T::DbWeight::get().reads_writes(1, 1) // balance ops + 500_000 * (info.additional.len() as Weight) // X + 500_000 * (MAX_REGISTRARS as Weight) // J }, @@ -569,18 +570,32 @@ decl_module! { /// - `subs`: The identity's sub-accounts. /// /// # - /// - `O(S)` where `S` subs-count (hard- and deposit-bounded). - /// - At most two balance operations. - /// - At most O(2 * S + 1) storage mutations; codec complexity `O(1 * S + S * 1)`); + /// - `O(P + S)` + /// - where `P` old-subs-count (hard- and deposit-bounded). + /// - where `S` subs-count (hard- and deposit-bounded). + /// - At most one balance operations. + /// - At most O(P + S + 1) storage mutations; codec complexity `O(S)`); /// one storage-exists. /// # - #[weight = SimpleDispatchInfo::FixedNormal(50_000_000)] - fn set_subs(origin, subs: Vec<(T::AccountId, Data)>) { + #[weight = FunctionOf( + |(subs, &old_subs_count): (&Vec<(T::AccountId, Data)>, &u32)| { + let db = T::DbWeight::get(); + db.reads(1) // storage-exists + + db.reads_writes(1, 1) // balance ops + + db.writes(old_subs_count.into()) // P old DB deletions + + db.writes(subs.len() as u64 + 1) // S + 1 new DB writes + + 500_000 * (subs.len() + 1 + old_subs_count as usize) as Weight // P + S + 1 + }, + DispatchClass::Normal, + true + )] + fn set_subs(origin, subs: Vec<(T::AccountId, Data)>, old_subs_count: u32) { let sender = ensure_signed(origin)?; ensure!(>::contains_key(&sender), Error::::NotFound); ensure!(subs.len() <= T::MaxSubAccounts::get() as usize, Error::::TooManySubAccounts); let (old_deposit, old_ids) = >::get(&sender); + ensure!(old_ids.len() <= old_subs_count as usize, "invalid count"); let new_deposit = T::SubAccountDeposit::get() * >::from(subs.len() as u32); if old_deposit < new_deposit { @@ -1125,25 +1140,27 @@ mod tests { fn setting_subaccounts_should_work() { new_test_ext().execute_with(|| { let mut subs = vec![(20, Data::Raw(vec![40; 1]))]; - assert_noop!(Identity::set_subs(Origin::signed(10), subs.clone()), Error::::NotFound); + assert_noop!(Identity::set_subs(Origin::signed(10), subs.clone(), 0), Error::::NotFound); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone())); + assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone(), 0)); assert_eq!(Balances::free_balance(10), 80); assert_eq!(Identity::subs_of(10), (10, vec![20])); assert_eq!(Identity::super_of(20), Some((10, Data::Raw(vec![40; 1])))); // push another item and re-set it. + let prev_length = subs.len(); subs.push((30, Data::Raw(vec![50; 1]))); - assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone())); + assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone(), prev_length as u32)); assert_eq!(Balances::free_balance(10), 70); assert_eq!(Identity::subs_of(10), (20, vec![20, 30])); assert_eq!(Identity::super_of(20), Some((10, Data::Raw(vec![40; 1])))); assert_eq!(Identity::super_of(30), Some((10, Data::Raw(vec![50; 1])))); // switch out one of the items and re-set. + let prev_length = subs.len(); subs[0] = (40, Data::Raw(vec![60; 1])); - assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone())); + assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone(), prev_length as u32)); assert_eq!(Balances::free_balance(10), 70); // no change in the balance assert_eq!(Identity::subs_of(10), (20, vec![40, 30])); assert_eq!(Identity::super_of(20), None); @@ -1151,14 +1168,14 @@ mod tests { assert_eq!(Identity::super_of(40), Some((10, Data::Raw(vec![60; 1])))); // clear - assert_ok!(Identity::set_subs(Origin::signed(10), vec![])); + assert_ok!(Identity::set_subs(Origin::signed(10), vec![], prev_length as u32)); assert_eq!(Balances::free_balance(10), 90); assert_eq!(Identity::subs_of(10), (0, vec![])); assert_eq!(Identity::super_of(30), None); assert_eq!(Identity::super_of(40), None); subs.push((20, Data::Raw(vec![40; 1]))); - assert_noop!(Identity::set_subs(Origin::signed(10), subs.clone()), Error::::TooManySubAccounts); + assert_noop!(Identity::set_subs(Origin::signed(10), subs.clone(), 0), Error::::TooManySubAccounts); }); } @@ -1166,7 +1183,7 @@ mod tests { fn clearing_account_should_remove_subaccounts_and_refund() { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))])); + assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))], 0)); assert_ok!(Identity::clear_identity(Origin::signed(10))); assert_eq!(Balances::free_balance(10), 100); assert!(Identity::super_of(20).is_none()); @@ -1177,7 +1194,7 @@ mod tests { fn killing_account_should_remove_subaccounts_and_not_refund() { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))])); + assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))], 0)); assert_ok!(Identity::kill_identity(Origin::ROOT, 10)); assert_eq!(Balances::free_balance(10), 80); assert!(Identity::super_of(20).is_none()); From 3c6bb7700250d6fd3edd20062bc1e08b7d8c6842 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Mon, 20 Apr 2020 15:39:45 +0200 Subject: [PATCH 06/34] adjust benchmarking code to the new API --- frame/identity/src/benchmarking.rs | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index fe99cd990720c..255980533200b 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -40,7 +40,7 @@ fn account(name: &'static str, index: u32) -> T::AccountId { fn add_registrars(r: u32) -> Result<(), &'static str> { for i in 0..r { let _ = T::Currency::make_free_balance_be(&account::("registrar", i), BalanceOf::::max_value()); - Identity::::add_registrar(RawOrigin::Root.into(), account::("registrar", i))?; + Identity::::add_registrar(RawOrigin::Root.into(), account::("registrar", i), i)?; Identity::::set_fee(RawOrigin::Signed(account::("registrar", i)).into(), i.into(), 10.into())?; let fields = IdentityFields( IdentityField::Display | IdentityField::Legal | IdentityField::Web | IdentityField::Riot @@ -54,8 +54,8 @@ fn add_registrars(r: u32) -> Result<(), &'static str> { } // Adds `s` sub-accounts to the identity of `who`. Each wil have 32 bytes of raw data added to it. -// This additionally returns the vector of sub-accounts to it can be modified if needed. -fn add_sub_accounts(who: &T::AccountId, s: u32) -> Result, &'static str> { +// This additionally returns the vector of sub-accounts so it can be modified if needed. +fn add_sub_accounts(who: &T::AccountId, s: u32, old_subs_count: u32) -> Result, &'static str> { let mut subs = Vec::new(); let who_origin = RawOrigin::Signed(who.clone()); let data = Data::Raw(vec![0; 32]); @@ -70,7 +70,7 @@ fn add_sub_accounts(who: &T::AccountId, s: u32) -> Result(1); Identity::::set_identity(who_origin.clone().into(), info)?; - Identity::::set_subs(who_origin.into(), subs.clone())?; + Identity::::set_subs(who_origin.into(), subs.clone(), old_subs_count)?; return Ok(subs) } @@ -102,7 +102,7 @@ benchmarks! { let s in 1 .. T::MaxSubAccounts::get() => { // Give them s many sub accounts let caller = account::("caller", 0); - let _ = add_sub_accounts::(&caller, s)?; + let _ = add_sub_accounts::(&caller, s, s - 1)?; }; let x in 1 .. T::MaxAdditionalFields::get() => { // Create their main identity with x additional fields @@ -115,7 +115,7 @@ benchmarks! { add_registrar { let r in ...; - }: _(RawOrigin::Root, account::("registrar", r + 1)) + }: _(RawOrigin::Root, account::("registrar", r + 1), r) set_identity { let r in ...; @@ -147,7 +147,7 @@ benchmarks! { }; }: _( RawOrigin::Signed(caller), - create_identity_info::(x) + create_identity_info::(x), ) set_subs { @@ -155,7 +155,7 @@ benchmarks! { // Give them s many sub accounts. let s in 1 .. T::MaxSubAccounts::get() - 1 => { - let _ = add_sub_accounts::(&caller, s)?; + let _ = add_sub_accounts::(&caller, s, s - 1)?; }; let mut subs = Module::::subs(&caller); @@ -164,7 +164,7 @@ benchmarks! { let data = Data::Raw(vec![0; 32]); subs.push((account::("sub", s + 1), data)); - }: _(RawOrigin::Signed(caller), subs) + }: _(RawOrigin::Signed(caller), subs, subs.len() as u32 - 1) clear_identity { let caller = account::("caller", 0); @@ -212,7 +212,7 @@ benchmarks! { let r in ...; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), 0)?; }: _(RawOrigin::Signed(caller), r, 10.into()) set_account_id { @@ -221,7 +221,7 @@ benchmarks! { let r in ...; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), 0)?; }: _(RawOrigin::Signed(caller), r, account::("new", 0)) set_fields { @@ -230,7 +230,7 @@ benchmarks! { let r in ...; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), 0)?; let fields = IdentityFields( IdentityField::Display | IdentityField::Legal | IdentityField::Web | IdentityField::Riot | IdentityField::Email | IdentityField::PgpFingerprint | IdentityField::Image | IdentityField::Twitter @@ -254,7 +254,7 @@ benchmarks! { Identity::::set_identity(user_origin.clone(), info)?; }; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), 0)?; Identity::::request_judgement(user_origin.clone(), r, 10.into())?; }: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable) From 6a64c7e5e1803a09340e254b7a19fedd8a609b04 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Mon, 20 Apr 2020 18:02:37 +0200 Subject: [PATCH 07/34] add second parameter to set_subs benchmark --- frame/identity/src/benchmarking.rs | 38 ++++++++++++++++++------------ 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 255980533200b..2857e4847d297 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -53,9 +53,9 @@ fn add_registrars(r: u32) -> Result<(), &'static str> { Ok(()) } -// Adds `s` sub-accounts to the identity of `who`. Each wil have 32 bytes of raw data added to it. -// This additionally returns the vector of sub-accounts so it can be modified if needed. -fn add_sub_accounts(who: &T::AccountId, s: u32, old_subs_count: u32) -> Result, &'static str> { +// Create `s` sub-accounts for the identity of `who` and return them. +// Each will have 32 bytes of raw data added to it. +fn create_sub_accounts(who: &T::AccountId, s: u32) -> Result, &'static str> { let mut subs = Vec::new(); let who_origin = RawOrigin::Signed(who.clone()); let data = Data::Raw(vec![0; 32]); @@ -70,9 +70,18 @@ fn add_sub_accounts(who: &T::AccountId, s: u32, old_subs_count: u32) - let info = create_identity_info::(1); Identity::::set_identity(who_origin.clone().into(), info)?; + Ok(subs) +} + +// Adds `s` sub-accounts to the identity of `who`. Each will have 32 bytes of raw data added to it. +// This additionally returns the vector of sub-accounts so it can be modified if needed. +fn add_sub_accounts(who: &T::AccountId, s: u32, old_subs_count: u32) -> Result, &'static str> { + let who_origin = RawOrigin::Signed(who.clone()); + let subs = create_sub_accounts::(who, s)?; + Identity::::set_subs(who_origin.into(), subs.clone(), old_subs_count)?; - return Ok(subs) + Ok(subs) } // This creates an `IdentityInfo` object with `num_fields` extra fields. @@ -99,6 +108,8 @@ benchmarks! { // These are the common parameters along with their instancing. _ { let r in 1 .. MAX_REGISTRARS => add_registrars::(r)?; + // extra parameter for the set_subs bench + let o in 1 .. T::MaxSubAccounts::get() => (); let s in 1 .. T::MaxSubAccounts::get() => { // Give them s many sub accounts let caller = account::("caller", 0); @@ -147,24 +158,21 @@ benchmarks! { }; }: _( RawOrigin::Signed(caller), - create_identity_info::(x), + create_identity_info::(x) ) set_subs { let caller = account::("caller", 0); - // Give them s many sub accounts. - let s in 1 .. T::MaxSubAccounts::get() - 1 => { - let _ = add_sub_accounts::(&caller, s, s - 1)?; + // Give them o many previous sub accounts. + let o in 1 .. T::MaxSubAccounts::get() => { + let _ = add_sub_accounts::(&caller, o, 0)?; }; + // Create a new subs vec with s sub accounts + let s in 1 .. T::MaxSubAccounts::get() => (); + let subs = create_sub_accounts::(&caller, s)?; - let mut subs = Module::::subs(&caller); - - // Create an s + 1 sub account. - let data = Data::Raw(vec![0; 32]); - subs.push((account::("sub", s + 1), data)); - - }: _(RawOrigin::Signed(caller), subs, subs.len() as u32 - 1) + }: _(RawOrigin::Signed(caller), subs, o) clear_identity { let caller = account::("caller", 0); From 31ba72f85a709d10be3dda0e9e9e18bfec113d16 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Mon, 20 Apr 2020 18:23:19 +0200 Subject: [PATCH 08/34] rename o to p --- frame/identity/src/benchmarking.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 2857e4847d297..fba9fdbdf4823 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -108,8 +108,8 @@ benchmarks! { // These are the common parameters along with their instancing. _ { let r in 1 .. MAX_REGISTRARS => add_registrars::(r)?; - // extra parameter for the set_subs bench - let o in 1 .. T::MaxSubAccounts::get() => (); + // extra parameter for the set_subs bench for previous sub accounts + let p in 1 .. T::MaxSubAccounts::get() => (); let s in 1 .. T::MaxSubAccounts::get() => { // Give them s many sub accounts let caller = account::("caller", 0); @@ -165,14 +165,14 @@ benchmarks! { let caller = account::("caller", 0); // Give them o many previous sub accounts. - let o in 1 .. T::MaxSubAccounts::get() => { - let _ = add_sub_accounts::(&caller, o, 0)?; + let p in 1 .. T::MaxSubAccounts::get() => { + let _ = add_sub_accounts::(&caller, p, 0)?; }; // Create a new subs vec with s sub accounts let s in 1 .. T::MaxSubAccounts::get() => (); let subs = create_sub_accounts::(&caller, s)?; - }: _(RawOrigin::Signed(caller), subs, o) + }: _(RawOrigin::Signed(caller), subs, p) clear_identity { let caller = account::("caller", 0); From d4feebed1fdd4f847f8be318dcec836a9f6d3b06 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Mon, 20 Apr 2020 18:23:37 +0200 Subject: [PATCH 09/34] calculate weight based on benchmarks --- frame/identity/src/lib.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 8a31bf2a12a5b..eb217934b6df7 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -476,11 +476,13 @@ decl_module! { /// - `O(R)` where `R` registrar-count (governance-bounded). /// - One storage mutation (codec `O(R)`). /// - One event. + /// - Benchmark: 78.71 + R * 0.965 µs (min square analysis) /// # #[weight = FunctionOf( |(_, &old_count): (&T::AccountId, &u32)| { T::DbWeight::get().reads_writes(1, 1) - + 500_000 * (old_count as Weight) // R + + 78_710_00 // constant + + 965_000 * (old_count as Weight) // R }, DispatchClass::Normal, true @@ -519,13 +521,15 @@ decl_module! { /// - At most two balance operations. /// - One storage mutation (codec-read `O(X' + J)`, codec-write `O(X + J)`). /// - One event. + /// - Benchmark: 136.6 + J * 0.62 + X * 2.62 µs (min square analysis) /// # #[weight = FunctionOf( |(info,): (&IdentityInfo,)| { T::DbWeight::get().reads_writes(1, 1) + T::DbWeight::get().reads_writes(1, 1) // balance ops - + 500_000 * (info.additional.len() as Weight) // X - + 500_000 * (MAX_REGISTRARS as Weight) // J + + 136_600_000 // constant + + 2_622_000 * (info.additional.len() as Weight) // X + + 624_000 * (MAX_REGISTRARS as Weight) // J }, DispatchClass::Normal, true @@ -576,6 +580,7 @@ decl_module! { /// - At most one balance operations. /// - At most O(P + S + 1) storage mutations; codec complexity `O(S)`); /// one storage-exists. + /// - Benchmark: 24.03 + P * 3.37 + S * 4.76 µs (min square analysis) /// # #[weight = FunctionOf( |(subs, &old_subs_count): (&Vec<(T::AccountId, Data)>, &u32)| { @@ -584,7 +589,9 @@ decl_module! { + db.reads_writes(1, 1) // balance ops + db.writes(old_subs_count.into()) // P old DB deletions + db.writes(subs.len() as u64 + 1) // S + 1 new DB writes - + 500_000 * (subs.len() + 1 + old_subs_count as usize) as Weight // P + S + 1 + + 24_030_000 // constant + + 3_374_000 * old_subs_count as Weight // P + + 4_762_000 * subs.len() as Weight // S }, DispatchClass::Normal, true From 6d08bc19f39f778557a646a2e355f4435daa9104 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Mon, 20 Apr 2020 18:32:56 +0200 Subject: [PATCH 10/34] use try_mutate for registrars --- frame/identity/src/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index eb217934b6df7..e036dc535f32f 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -69,7 +69,7 @@ use sp_std::prelude::*; use sp_std::{fmt::Debug, ops::Add, iter::once}; use enumflags2::BitFlags; use codec::{Encode, Decode}; -use sp_runtime::{DispatchResult, RuntimeDebug}; +use sp_runtime::{DispatchError, DispatchResult, RuntimeDebug}; use sp_runtime::traits::{StaticLookup, Zero, AppendZerosInput}; use frame_support::{ decl_module, decl_event, decl_storage, ensure, decl_error, @@ -491,12 +491,12 @@ decl_module! { T::RegistrarOrigin::try_origin(origin) .map(|_| ()) .or_else(ensure_root)?; - let mut registrars = Self::registrars(); - ensure!(registrars.len() as u32 <= old_registrar_count, "invalid count"); - registrars.push(Some(RegistrarInfo { account, fee: Zero::zero(), fields: Default::default() })); - let i = (registrars.len() - 1) as RegistrarIndex; - >::put(registrars); + let i = >::try_mutate(|registrars| -> Result { + ensure!(registrars.len() as u32 <= old_registrar_count, "invalid count"); + registrars.push(Some(RegistrarInfo { account, fee: Zero::zero(), fields: Default::default() })); + Ok((registrars.len() - 1) as RegistrarIndex) + })?; Self::deposit_event(RawEvent::RegistrarAdded(i)); From fa04aab2de506970aa61a9c5506f46d4e2ea23fb Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Mon, 20 Apr 2020 18:33:07 +0200 Subject: [PATCH 11/34] fix weight number typo --- frame/identity/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index e036dc535f32f..b2cf85e979f4c 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -481,7 +481,7 @@ decl_module! { #[weight = FunctionOf( |(_, &old_count): (&T::AccountId, &u32)| { T::DbWeight::get().reads_writes(1, 1) - + 78_710_00 // constant + + 78_710_000 // constant + 965_000 * (old_count as Weight) // R }, DispatchClass::Normal, From 3fb97899a942af5127a21af6438cfe4a2ffb8233 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 12:09:09 +0200 Subject: [PATCH 12/34] update weights for set_subs + add weights for clear_identity and request_judgement --- frame/identity/src/benchmarking.rs | 14 +++--- frame/identity/src/lib.rs | 74 ++++++++++++++++++++---------- 2 files changed, 58 insertions(+), 30 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index fba9fdbdf4823..0997cb5d597e0 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -146,7 +146,7 @@ benchmarks! { // User requests judgement from all the registrars, and they approve for i in 0..r { - Identity::::request_judgement(caller_origin.clone(), i, 10.into())?; + Identity::::request_judgement(caller_origin.clone(), i, 10.into(), x)?; Identity::::provide_judgement( RawOrigin::Signed(account::("registrar", i)).into(), i, @@ -186,7 +186,7 @@ benchmarks! { // User requests judgement from all the registrars, and they approve for i in 0..r { - Identity::::request_judgement(caller_origin.clone(), i, 10.into())?; + Identity::::request_judgement(caller_origin.clone(), i, 10.into(), x)?; Identity::::provide_judgement( RawOrigin::Signed(account::("registrar", i)).into(), i, @@ -194,7 +194,7 @@ benchmarks! { Judgement::Reasonable )?; } - }: _(RawOrigin::Signed(caller)) + }: _(RawOrigin::Signed(caller), s) request_judgement { let caller = account::("caller", 0); @@ -202,7 +202,7 @@ benchmarks! { let r in ...; let x in ...; - }: _(RawOrigin::Signed(caller), r - 1, 10.into()) + }: _(RawOrigin::Signed(caller), r - 1, 10.into(), x) cancel_request { let caller = account::("caller", 0); @@ -212,7 +212,7 @@ benchmarks! { let r in ...; let x in ...; - Identity::::request_judgement(caller_origin, r - 1, 10.into())?; + Identity::::request_judgement(caller_origin, r - 1, 10.into(), x)?; }: _(RawOrigin::Signed(caller), r - 1) set_fee { @@ -263,7 +263,7 @@ benchmarks! { }; Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), 0)?; - Identity::::request_judgement(user_origin.clone(), r, 10.into())?; + Identity::::request_judgement(user_origin.clone(), r, 10.into(), x)?; }: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable) kill_identity { @@ -278,7 +278,7 @@ benchmarks! { // User requests judgement from all the registrars, and they approve for i in 0..r { - Identity::::request_judgement(caller_origin.clone(), i, 10.into())?; + Identity::::request_judgement(caller_origin.clone(), i, 10.into(), x)?; Identity::::provide_judgement( RawOrigin::Signed(account::("registrar", i)).into(), i, diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index b2cf85e979f4c..1eeb752dded3c 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -84,7 +84,7 @@ type BalanceOf = <::Currency as Currency< = <::Currency as Currency<::AccountId>>::NegativeImbalance; /// Upper bound for the amount of registrars expected. -const MAX_REGISTRARS: u32 = 20; +const ASSUMED_MAX_REGISTRARS: u32 = 20; pub trait Trait: frame_system::Trait { /// The overarching event type. @@ -529,7 +529,7 @@ decl_module! { + T::DbWeight::get().reads_writes(1, 1) // balance ops + 136_600_000 // constant + 2_622_000 * (info.additional.len() as Weight) // X - + 624_000 * (MAX_REGISTRARS as Weight) // J + + 624_000 * (ASSUMED_MAX_REGISTRARS as Weight) // J }, DispatchClass::Normal, true @@ -580,7 +580,7 @@ decl_module! { /// - At most one balance operations. /// - At most O(P + S + 1) storage mutations; codec complexity `O(S)`); /// one storage-exists. - /// - Benchmark: 24.03 + P * 3.37 + S * 4.76 µs (min square analysis) + /// - Benchmark: 115.2 + P * 5.11 + S * 6.67 µs (min square analysis) /// # #[weight = FunctionOf( |(subs, &old_subs_count): (&Vec<(T::AccountId, Data)>, &u32)| { @@ -589,9 +589,9 @@ decl_module! { + db.reads_writes(1, 1) // balance ops + db.writes(old_subs_count.into()) // P old DB deletions + db.writes(subs.len() as u64 + 1) // S + 1 new DB writes - + 24_030_000 // constant - + 3_374_000 * old_subs_count as Weight // P - + 4_762_000 * subs.len() as Weight // S + + 115_200_000 // constant + + 5_118_000 * old_subs_count as Weight // P + + 6_676_000 * subs.len() as Weight // S }, DispatchClass::Normal, true @@ -628,7 +628,7 @@ decl_module! { } } - /// Clear an account's identity info and all sub-account and return all deposits. + /// Clear an account's identity info and all sub-accounts and return all deposits. /// /// Payment: All reserved balances on the account are returned. /// @@ -638,16 +638,31 @@ decl_module! { /// Emits `IdentityCleared` if successful. /// /// # - /// - `O(R + S + X)`. - /// - One balance-reserve operation. + /// - `O(R + S + X)` + /// - where `R` registrar-count (governance-bounded). + /// - where `S` subs-count (hard- and deposit-bounded). + /// - where `X` additional-field-count (deposit-bounded and code-bounded). + /// - One balance-unreserve operation. /// - `S + 2` storage deletions. /// - One event. + /// - Benchmark: 152.3 + R * 0.306 + S * 4.967 + X * 1.697 µs (min square analysis) /// # - #[weight = SimpleDispatchInfo::FixedNormal(50_000_000)] - fn clear_identity(origin) { + #[weight = FunctionOf( + |(&subs_count,): (&u32,)| { + T::DbWeight::get().writes(subs_count as Weight + 2) // S + 2 deletions + + 152_300_000 // constant + + 306_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 4_967_000 * subs_count as Weight // S + + 1_697_000 * T::MaxAdditionalFields::get() as Weight // X + }, + DispatchClass::Normal, + true + )] + fn clear_identity(origin, subs_count: u32) { let sender = ensure_signed(origin)?; let (subs_deposit, sub_ids) = >::take(&sender); + ensure!(sub_ids.len() <= subs_count as usize, "invalid count"); let deposit = >::take(&sender).ok_or(Error::::NotNamed)?.total_deposit() + subs_deposit; for sub in sub_ids.iter() { @@ -681,11 +696,23 @@ decl_module! { /// - One balance-reserve operation. /// - Storage: 1 read `O(R)`, 1 mutate `O(X + R)`. /// - One event. + /// - Benchmark: 154 + R * 0.932 + X * 3.302 µs (min square analysis) /// # - #[weight = SimpleDispatchInfo::FixedNormal(50_000_000)] + #[weight = FunctionOf( + |(_, _, &fields_count): (&RegistrarIndex, &BalanceOf, &u32)| { + T::DbWeight::get().reads_writes(2, 1) + + T::DbWeight::get().reads_writes(1, 1) // balance ops + + 154_000_000 // constant + + 932_000 * ASSUMED_MAX_REGISTRARS as Weight// R + + 3_302_000 * fields_count as Weight // X + }, + DispatchClass::Normal, + true + )] fn request_judgement(origin, #[compact] reg_index: RegistrarIndex, #[compact] max_fee: BalanceOf, + fields_count: u32, ) { let sender = ensure_signed(origin)?; let registrars = >::get(); @@ -693,6 +720,7 @@ decl_module! { .ok_or(Error::::EmptyIndex)?; ensure!(max_fee >= registrar.fee, Error::::FeeChanged); let mut id = >::get(&sender).ok_or(Error::::NoIdentity)?; + ensure!(id.info.additional.len() <= fields_count as usize, "invalid count"); let item = (reg_index, Judgement::FeePaid(registrar.fee)); match id.judgements.binary_search_by_key(®_index, |x| x.0) { @@ -1085,9 +1113,9 @@ mod tests { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_eq!(Identity::identity(10).unwrap().info, ten()); assert_eq!(Balances::free_balance(10), 90); - assert_ok!(Identity::clear_identity(Origin::signed(10))); + assert_ok!(Identity::clear_identity(Origin::signed(10), 0)); assert_eq!(Balances::free_balance(10), 100); - assert_noop!(Identity::clear_identity(Origin::signed(10)), Error::::NotNamed); + assert_noop!(Identity::clear_identity(Origin::signed(10), 0), Error::::NotNamed); }); } @@ -1126,7 +1154,7 @@ mod tests { assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable)); - assert_ok!(Identity::clear_identity(Origin::signed(10))); + assert_ok!(Identity::clear_identity(Origin::signed(10), 0)); assert_eq!(Identity::identity(10), None); }); } @@ -1191,7 +1219,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))], 0)); - assert_ok!(Identity::clear_identity(Origin::signed(10))); + assert_ok!(Identity::clear_identity(Origin::signed(10), 1)); assert_eq!(Balances::free_balance(10), 100); assert!(Identity::super_of(20).is_none()); }); @@ -1215,7 +1243,7 @@ mod tests { assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::NoIdentity); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10)); + assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10, 0)); assert_ok!(Identity::cancel_request(Origin::signed(10), 0)); assert_eq!(Balances::free_balance(10), 90); assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::NotFound); @@ -1231,27 +1259,27 @@ mod tests { assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 9), Error::::FeeChanged); - assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10)); + assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 9, 0), Error::::FeeChanged); + assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10, 0)); // 10 for the judgement request, 10 for the identity. assert_eq!(Balances::free_balance(10), 80); // Re-requesting won't work as we already paid. - assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10), Error::::StickyJudgement); + assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10, 0), Error::::StickyJudgement); assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Erroneous)); // Registrar got their payment now. assert_eq!(Balances::free_balance(3), 20); // Re-requesting still won't work as it's erroneous. - assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10), Error::::StickyJudgement); + assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10, 0), Error::::StickyJudgement); // Requesting from a second registrar still works. assert_ok!(Identity::add_registrar(Origin::signed(1), 4, 1)); - assert_ok!(Identity::request_judgement(Origin::signed(10), 1, 10)); + assert_ok!(Identity::request_judgement(Origin::signed(10), 1, 10, 0)); // Re-requesting after the judgement has been reduced works. assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::OutOfDate)); - assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10)); + assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10, 0)); }); } From 9d027babdce3fa3c7a40fdfa38f772e9dcb9f68e Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 12:18:15 +0200 Subject: [PATCH 13/34] improve naming and docs --- frame/identity/src/lib.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 1eeb752dded3c..d7bae50bd4d89 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -572,6 +572,7 @@ decl_module! { /// identity. /// /// - `subs`: The identity's sub-accounts. + /// - `old_subs_count`: This is the number of previous sub accounts associated with the identity. /// /// # /// - `O(P + S)` @@ -634,6 +635,8 @@ decl_module! { /// /// The dispatch origin for this call must be _Signed_ and the sender must have a registered /// identity. + /// + /// - `subs_count`: This is the number of sub accounts associated with the identity to clear. /// /// Emits `IdentityCleared` if successful. /// @@ -686,7 +689,13 @@ decl_module! { /// - `max_fee`: The maximum fee that may be paid. This should just be auto-populated as: /// /// ```nocompile - /// Self::registrars(reg_index).unwrap().fee + /// Self::registrars().get(reg_index).unwrap().fee + /// ``` + /// - `additional_fields_count`: The number of additional fields on the identity info. This + /// should be auto-populated as: + /// + /// ```nocompile + /// Self::identity(account_id).unwrap().info.additional.len() /// ``` /// /// Emits `JudgementRequested` if successful. @@ -703,7 +712,7 @@ decl_module! { T::DbWeight::get().reads_writes(2, 1) + T::DbWeight::get().reads_writes(1, 1) // balance ops + 154_000_000 // constant - + 932_000 * ASSUMED_MAX_REGISTRARS as Weight// R + + 932_000 * ASSUMED_MAX_REGISTRARS as Weight // R + 3_302_000 * fields_count as Weight // X }, DispatchClass::Normal, @@ -712,7 +721,7 @@ decl_module! { fn request_judgement(origin, #[compact] reg_index: RegistrarIndex, #[compact] max_fee: BalanceOf, - fields_count: u32, + additional_fields_count: u32, ) { let sender = ensure_signed(origin)?; let registrars = >::get(); @@ -720,7 +729,7 @@ decl_module! { .ok_or(Error::::EmptyIndex)?; ensure!(max_fee >= registrar.fee, Error::::FeeChanged); let mut id = >::get(&sender).ok_or(Error::::NoIdentity)?; - ensure!(id.info.additional.len() <= fields_count as usize, "invalid count"); + ensure!(id.info.additional.len() <= additional_fields_count as usize, "invalid count"); let item = (reg_index, Judgement::FeePaid(registrar.fee)); match id.judgements.binary_search_by_key(®_index, |x| x.0) { From 6c0dfa97d332232e73e50eab3d52011ccf4d74d1 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 14:27:40 +0200 Subject: [PATCH 14/34] add weight calculation for cancel_request --- frame/identity/src/lib.rs | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index d7bae50bd4d89..fb66da978cf82 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -691,6 +691,7 @@ decl_module! { /// ```nocompile /// Self::registrars().get(reg_index).unwrap().fee /// ``` + /// /// - `additional_fields_count`: The number of additional fields on the identity info. This /// should be auto-populated as: /// @@ -756,6 +757,12 @@ decl_module! { /// registered identity. /// /// - `reg_index`: The index of the registrar whose judgement is no longer requested. + /// - `additional_fields_count`: The number of additional fields on the identity info. This + /// should be auto-populated as: + /// + /// ```nocompile + /// Self::identity(account_id).unwrap().info.additional.len() + /// ``` /// /// Emits `JudgementUnrequested` if successful. /// @@ -764,11 +771,23 @@ decl_module! { /// - One balance-reserve operation. /// - One storage mutation `O(R + X)`. /// - One event. + /// - Benchmark: 135.3 + R * 0.574 + X * 3.394 µs (min square analysis) /// # - #[weight = SimpleDispatchInfo::FixedNormal(50_000_000)] - fn cancel_request(origin, reg_index: RegistrarIndex) { + #[weight = FunctionOf( + |(_, &fields_count): (&RegistrarIndex, &u32)| { + T::DbWeight::get().reads_writes(1, 1) + + T::DbWeight::get().reads_writes(1, 1) // balance ops + + 135_300_000 // constant + + 574_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 3_394_000 * fields_count as Weight // X + }, + DispatchClass::Normal, + true + )] + fn cancel_request(origin, reg_index: RegistrarIndex, additional_fields_count: u32) { let sender = ensure_signed(origin)?; let mut id = >::get(&sender).ok_or(Error::::NoIdentity)?; + ensure!(id.info.additional.len() <= additional_fields_count as usize, "invalid count"); let pos = id.judgements.binary_search_by_key(®_index, |x| x.0) .map_err(|_| Error::::NotFound)?; From d662f5a044f33a970f8b7e657ad72ada17d28278 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 15:08:47 +0200 Subject: [PATCH 15/34] fix benchmark --- frame/identity/src/benchmarking.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 0997cb5d597e0..7741e0a6a623d 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -213,14 +213,14 @@ benchmarks! { let x in ...; Identity::::request_judgement(caller_origin, r - 1, 10.into(), x)?; - }: _(RawOrigin::Signed(caller), r - 1) + }: _(RawOrigin::Signed(caller), r - 1, x) set_fee { let caller = account::("caller", 0); let r in ...; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), 0)?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; }: _(RawOrigin::Signed(caller), r, 10.into()) set_account_id { @@ -229,7 +229,7 @@ benchmarks! { let r in ...; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), 0)?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; }: _(RawOrigin::Signed(caller), r, account::("new", 0)) set_fields { @@ -238,7 +238,7 @@ benchmarks! { let r in ...; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), 0)?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; let fields = IdentityFields( IdentityField::Display | IdentityField::Legal | IdentityField::Web | IdentityField::Riot | IdentityField::Email | IdentityField::PgpFingerprint | IdentityField::Image | IdentityField::Twitter @@ -262,7 +262,7 @@ benchmarks! { Identity::::set_identity(user_origin.clone(), info)?; }; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), 0)?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; Identity::::request_judgement(user_origin.clone(), r, 10.into(), x)?; }: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable) From 13163454b7f5360d7654af5d9ae329b0c11732df Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 15:08:59 +0200 Subject: [PATCH 16/34] fix tests --- frame/identity/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index fb66da978cf82..e68649d0d6684 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -1269,15 +1269,15 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); - assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::NoIdentity); + assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::NoIdentity); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10, 0)); - assert_ok!(Identity::cancel_request(Origin::signed(10), 0)); + assert_ok!(Identity::cancel_request(Origin::signed(10), 0, 0)); assert_eq!(Balances::free_balance(10), 90); - assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::NotFound); + assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::NotFound); assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable)); - assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::JudgementGiven); + assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::JudgementGiven); }); } From c699699a8200d49f6678cf66afe68c850611b81f Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 15:09:23 +0200 Subject: [PATCH 17/34] fix arithmetic overflow in balances triggered by tests --- frame/balances/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/balances/src/lib.rs b/frame/balances/src/lib.rs index 98d6a93738147..5f86bbcec0350 100644 --- a/frame/balances/src/lib.rs +++ b/frame/balances/src/lib.rs @@ -1099,7 +1099,7 @@ impl, I: Instance> Currency for Module where // equal and opposite cause (returned as an Imbalance), then in the // instance that there's no other accounts on the system at all, we might // underflow the issuance and our arithmetic will be off. - ensure!(value + account.reserved >= ed || !account.total().is_zero(), ()); + ensure!(value.saturating_add(account.reserved) >= ed || !account.total().is_zero(), ()); let imbalance = if account.free <= value { SignedImbalance::Positive(PositiveImbalance::new(value - account.free)) From 269f50bbfc628eed1736d1863ae58f1f98ebd2f0 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 15:47:03 +0200 Subject: [PATCH 18/34] add weight calcluations for more dispatchables --- frame/identity/src/lib.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index e68649d0d6684..38e78dbf0117c 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -815,7 +815,10 @@ decl_module! { /// - `O(R)`. /// - One storage mutation `O(R)`. /// # - #[weight = SimpleDispatchInfo::FixedNormal(50_000_000)] + #[weight = T::DbWeight::get().reads_writes(1, 1) + + 30_000_000 // constant + + 700_000 * ASSUMED_MAX_REGISTRARS as Weight // R + ] fn set_fee(origin, #[compact] index: RegistrarIndex, #[compact] fee: BalanceOf, @@ -842,7 +845,10 @@ decl_module! { /// - `O(R)`. /// - One storage mutation `O(R)`. /// # - #[weight = SimpleDispatchInfo::FixedNormal(50_000_000)] + #[weight = T::DbWeight::get().reads_writes(1, 1) + + 30_000_000 // constant + + 700_000 * ASSUMED_MAX_REGISTRARS as Weight // R + ] fn set_account_id(origin, #[compact] index: RegistrarIndex, new: T::AccountId, @@ -869,7 +875,10 @@ decl_module! { /// - `O(R)`. /// - One storage mutation `O(R)`. /// # - #[weight = SimpleDispatchInfo::FixedNormal(50_000_000)] + #[weight = T::DbWeight::get().reads_writes(1, 1) + + 30_000_000 // constant + + 700_000 * ASSUMED_MAX_REGISTRARS as Weight // R + ] fn set_fields(origin, #[compact] index: RegistrarIndex, fields: IdentityFields, From 8dec341b064aa9e8e42fab91f307bd38e4b87f56 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 16:01:13 +0200 Subject: [PATCH 19/34] add weight calculation for provide_judgement --- frame/identity/src/benchmarking.rs | 11 ++++++---- frame/identity/src/lib.rs | 32 ++++++++++++++++++++---------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 7741e0a6a623d..f80f04c4281d7 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -151,7 +151,8 @@ benchmarks! { RawOrigin::Signed(account::("registrar", i)).into(), i, caller_lookup.clone(), - Judgement::Reasonable + Judgement::Reasonable, + x )?; } caller @@ -191,7 +192,8 @@ benchmarks! { RawOrigin::Signed(account::("registrar", i)).into(), i, caller_lookup.clone(), - Judgement::Reasonable + Judgement::Reasonable, + x )?; } }: _(RawOrigin::Signed(caller), s) @@ -264,7 +266,7 @@ benchmarks! { Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; Identity::::request_judgement(user_origin.clone(), r, 10.into(), x)?; - }: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable) + }: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable, x) kill_identity { let caller = account::("caller", 0); @@ -283,7 +285,8 @@ benchmarks! { RawOrigin::Signed(account::("registrar", i)).into(), i, caller_lookup.clone(), - Judgement::Reasonable + Judgement::Reasonable, + x )?; } }: _(RawOrigin::Root, caller_lookup) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 38e78dbf0117c..de6b3b6f2159b 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -912,11 +912,22 @@ decl_module! { /// - Storage: 1 read `O(R)`, 1 mutate `O(R + X)`. /// - One event. /// # - #[weight = SimpleDispatchInfo::FixedNormal(50_000_000)] + #[weight = FunctionOf( + |(_, _, _, &fields_count): (&RegistrarIndex, &::Source, &Judgement>, &u32)| { + T::DbWeight::get().reads_writes(2, 1) + + T::DbWeight::get().reads_writes(1, 1) // balance ops + + 154_000_000 // constant + + 932_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 3_302_000 * fields_count as Weight // X + }, + DispatchClass::Normal, + true + )] fn provide_judgement(origin, #[compact] reg_index: RegistrarIndex, target: ::Source, judgement: Judgement>, + additional_fields_count: u32, ) { let sender = ensure_signed(origin)?; let target = T::Lookup::lookup(target)?; @@ -927,6 +938,7 @@ decl_module! { .and_then(|r| if r.account == sender { Some(r) } else { None }) .ok_or(Error::::InvalidIndex)?; let mut id = >::get(&target).ok_or(Error::::InvalidTarget)?; + ensure!(id.info.additional.len() <= additional_fields_count as usize, "invalid count"); let item = (reg_index, judgement); match id.judgements.binary_search_by_key(®_index, |x| x.0) { @@ -1160,27 +1172,27 @@ mod tests { fn uninvited_judgement_should_work() { new_test_ext().execute_with(|| { assert_noop!( - Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable), + Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0), Error::::InvalidIndex ); assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_noop!( - Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable), + Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0), Error::::InvalidTarget ); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_noop!( - Identity::provide_judgement(Origin::signed(10), 0, 10, Judgement::Reasonable), + Identity::provide_judgement(Origin::signed(10), 0, 10, Judgement::Reasonable, 0), Error::::InvalidIndex ); assert_noop!( - Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::FeePaid(1)), + Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::FeePaid(1), 0), Error::::InvalidJudgement ); - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0)); assert_eq!(Identity::identity(10).unwrap().judgements, vec![(0, Judgement::Reasonable)]); }); } @@ -1190,7 +1202,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0)); assert_ok!(Identity::clear_identity(Origin::signed(10), 0)); assert_eq!(Identity::identity(10), None); }); @@ -1285,7 +1297,7 @@ mod tests { assert_eq!(Balances::free_balance(10), 90); assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::NotFound); - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0)); assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::JudgementGiven); }); } @@ -1303,7 +1315,7 @@ mod tests { // Re-requesting won't work as we already paid. assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10, 0), Error::::StickyJudgement); - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Erroneous)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Erroneous, 0)); // Registrar got their payment now. assert_eq!(Balances::free_balance(3), 20); @@ -1315,7 +1327,7 @@ mod tests { assert_ok!(Identity::request_judgement(Origin::signed(10), 1, 10, 0)); // Re-requesting after the judgement has been reduced works. - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::OutOfDate)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::OutOfDate, 0)); assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10, 0)); }); } From 155f1932faa411a15cf6054150baca2e8a66f0e2 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 16:52:42 +0200 Subject: [PATCH 20/34] mark param as unused --- frame/support/src/traits.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 35e1231698a46..dfc2ddf4de129 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -208,7 +208,7 @@ pub trait Contains { /// /// **Should be used for benchmarking only!!!** #[cfg(feature = "runtime-benchmarks")] - fn add(t: &T) { unimplemented!() } + fn add(_t: &T) { unimplemented!() } } /// Determiner to say whether a given account is unused. From d6f68ff38f42e84d58006acff4182b80f34e3cc9 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 16:53:23 +0200 Subject: [PATCH 21/34] add MaxRegistrars associated type used for weight estimation --- bin/node/runtime/src/lib.rs | 2 ++ frame/identity/src/benchmarking.rs | 5 +---- frame/identity/src/lib.rs | 25 ++++++++++++++----------- 3 files changed, 17 insertions(+), 15 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 337242f884ddc..e961fbf4570cd 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -588,6 +588,7 @@ parameter_types! { pub const SubAccountDeposit: Balance = 2 * DOLLARS; // 53 bytes on-chain pub const MaxSubAccounts: u32 = 100; pub const MaxAdditionalFields: u32 = 100; + pub const MaxRegistrars: u32 = 20; } impl pallet_identity::Trait for Runtime { @@ -598,6 +599,7 @@ impl pallet_identity::Trait for Runtime { type SubAccountDeposit = SubAccountDeposit; type MaxSubAccounts = MaxSubAccounts; type MaxAdditionalFields = MaxAdditionalFields; + type MaxRegistrars = MaxRegistrars; type Slashed = Treasury; type ForceOrigin = pallet_collective::EnsureProportionMoreThan<_1, _2, AccountId, CouncilCollective>; type RegistrarOrigin = pallet_collective::EnsureProportionMoreThan<_1, _2, AccountId, CouncilCollective>; diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index f80f04c4281d7..f891f5abf9ed9 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -27,9 +27,6 @@ use sp_runtime::traits::Bounded; use crate::Module as Identity; -// The maximum number of identity registrars we will test. -const MAX_REGISTRARS: u32 = 50; - // Support Functions fn account(name: &'static str, index: u32) -> T::AccountId { let entropy = (name, index).using_encoded(blake2_256); @@ -107,7 +104,7 @@ fn create_identity_info(num_fields: u32) -> IdentityInfo { benchmarks! { // These are the common parameters along with their instancing. _ { - let r in 1 .. MAX_REGISTRARS => add_registrars::(r)?; + let r in 1 .. T::MaxRegistrars::get() => add_registrars::(r)?; // extra parameter for the set_subs bench for previous sub accounts let p in 1 .. T::MaxSubAccounts::get() => (); let s in 1 .. T::MaxSubAccounts::get() => { diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index de6b3b6f2159b..39d5325a5a4b0 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -83,9 +83,6 @@ mod benchmarking; type BalanceOf = <::Currency as Currency<::AccountId>>::Balance; type NegativeImbalanceOf = <::Currency as Currency<::AccountId>>::NegativeImbalance; -/// Upper bound for the amount of registrars expected. -const ASSUMED_MAX_REGISTRARS: u32 = 20; - pub trait Trait: frame_system::Trait { /// The overarching event type. type Event: From> + Into<::Event>; @@ -111,6 +108,10 @@ pub trait Trait: frame_system::Trait { /// required to access an identity, but can be pretty high. type MaxAdditionalFields: Get; + /// Maxmimum number of registrars allowed in the system. Needed to bound the complexity + /// of, e.g., updating judgements. + type MaxRegistrars: Get; + /// What to do with slashed funds. type Slashed: OnUnbalanced>; @@ -529,7 +530,7 @@ decl_module! { + T::DbWeight::get().reads_writes(1, 1) // balance ops + 136_600_000 // constant + 2_622_000 * (info.additional.len() as Weight) // X - + 624_000 * (ASSUMED_MAX_REGISTRARS as Weight) // J + + 624_000 * (T::MaxRegistrars::get() as Weight) // J }, DispatchClass::Normal, true @@ -654,7 +655,7 @@ decl_module! { |(&subs_count,): (&u32,)| { T::DbWeight::get().writes(subs_count as Weight + 2) // S + 2 deletions + 152_300_000 // constant - + 306_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 306_000 * T::MaxRegistrars::get() as Weight // R + 4_967_000 * subs_count as Weight // S + 1_697_000 * T::MaxAdditionalFields::get() as Weight // X }, @@ -713,7 +714,7 @@ decl_module! { T::DbWeight::get().reads_writes(2, 1) + T::DbWeight::get().reads_writes(1, 1) // balance ops + 154_000_000 // constant - + 932_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 932_000 * T::MaxRegistrars::get() as Weight // R + 3_302_000 * fields_count as Weight // X }, DispatchClass::Normal, @@ -778,7 +779,7 @@ decl_module! { T::DbWeight::get().reads_writes(1, 1) + T::DbWeight::get().reads_writes(1, 1) // balance ops + 135_300_000 // constant - + 574_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 574_000 * T::MaxRegistrars::get() as Weight // R + 3_394_000 * fields_count as Weight // X }, DispatchClass::Normal, @@ -817,7 +818,7 @@ decl_module! { /// # #[weight = T::DbWeight::get().reads_writes(1, 1) + 30_000_000 // constant - + 700_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 700_000 * T::MaxRegistrars::get() as Weight // R ] fn set_fee(origin, #[compact] index: RegistrarIndex, @@ -847,7 +848,7 @@ decl_module! { /// # #[weight = T::DbWeight::get().reads_writes(1, 1) + 30_000_000 // constant - + 700_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 700_000 * T::MaxRegistrars::get() as Weight // R ] fn set_account_id(origin, #[compact] index: RegistrarIndex, @@ -877,7 +878,7 @@ decl_module! { /// # #[weight = T::DbWeight::get().reads_writes(1, 1) + 30_000_000 // constant - + 700_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 700_000 * T::MaxRegistrars::get() as Weight // R ] fn set_fields(origin, #[compact] index: RegistrarIndex, @@ -917,7 +918,7 @@ decl_module! { T::DbWeight::get().reads_writes(2, 1) + T::DbWeight::get().reads_writes(1, 1) // balance ops + 154_000_000 // constant - + 932_000 * ASSUMED_MAX_REGISTRARS as Weight // R + + 932_000 * T::MaxRegistrars::get() as Weight // R + 3_302_000 * fields_count as Weight // X }, DispatchClass::Normal, @@ -1076,6 +1077,7 @@ mod tests { pub const SubAccountDeposit: u64 = 10; pub const MaxSubAccounts: u32 = 2; pub const MaxAdditionalFields: u32 = 2; + pub const MaxRegistrars: u32 = 20; } ord_parameter_types! { pub const One: u64 = 1; @@ -1090,6 +1092,7 @@ mod tests { type SubAccountDeposit = SubAccountDeposit; type MaxSubAccounts = MaxSubAccounts; type MaxAdditionalFields = MaxAdditionalFields; + type MaxRegistrars = MaxRegistrars; type RegistrarOrigin = EnsureSignedBy; type ForceOrigin = EnsureSignedBy; } From ba839d6401d794f05c2595363d254c3fa59b04aa Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 17:30:53 +0200 Subject: [PATCH 22/34] check that MaxRegistrars is not exceeded --- frame/identity/src/benchmarking.rs | 10 +++++----- frame/identity/src/lib.rs | 17 +++++++++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index f891f5abf9ed9..c31c9ac5fe215 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -122,7 +122,7 @@ benchmarks! { } add_registrar { - let r in ...; + let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; }: _(RawOrigin::Root, account::("registrar", r + 1), r) set_identity { @@ -217,7 +217,7 @@ benchmarks! { set_fee { let caller = account::("caller", 0); - let r in ...; + let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; }: _(RawOrigin::Signed(caller), r, 10.into()) @@ -226,7 +226,7 @@ benchmarks! { let caller = account::("caller", 0); let _ = T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); - let r in ...; + let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; }: _(RawOrigin::Signed(caller), r, account::("new", 0)) @@ -235,7 +235,7 @@ benchmarks! { let caller = account::("caller", 0); let _ = T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); - let r in ...; + let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; let fields = IdentityFields( @@ -254,7 +254,7 @@ benchmarks! { let caller = account::("caller", 0); let _ = T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); - let r in ...; + let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; // For this x, it's the user identity that gts the fields, not the caller. let x in _ .. _ => { let info = create_identity_info::(x); diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 39d5325a5a4b0..01e5b22f445f2 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -455,6 +455,8 @@ decl_error! { InvalidTarget, /// Too many additional fields. TooManyFields, + /// Maximum amount of registrars reached. Cannot add any more. + TooManyRegistrars, } } @@ -495,6 +497,7 @@ decl_module! { let i = >::try_mutate(|registrars| -> Result { ensure!(registrars.len() as u32 <= old_registrar_count, "invalid count"); + ensure!((registrars.len() as u32) < T::MaxRegistrars::get(), Error::::TooManyRegistrars); registrars.push(Some(RegistrarInfo { account, fee: Zero::zero(), fields: Default::default() })); Ok((registrars.len() - 1) as RegistrarIndex) })?; @@ -1149,6 +1152,20 @@ mod tests { }); } + #[test] + fn amount_of_registrars_is_limited() { + new_test_ext().execute_with(|| { + for i in 1..MaxRegistrars::get() + 1 { + assert_ok!(Identity::add_registrar(Origin::signed(1), i as u64, i - 1)); + } + let last_registrar = MaxRegistrars::get() as u64 + 1; + assert_noop!( + Identity::add_registrar(Origin::signed(1), last_registrar, MaxRegistrars::get()), + Error::::TooManyRegistrars + ); + }); + } + #[test] fn registration_should_work() { new_test_ext().execute_with(|| { From 4719cc5fac3ff65c1b76319efd075ac605b361b2 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Tue, 21 Apr 2020 17:55:42 +0200 Subject: [PATCH 23/34] add remaining weight calculations --- frame/identity/src/benchmarking.rs | 2 +- frame/identity/src/lib.rs | 47 ++++++++++++++++++++---------- 2 files changed, 33 insertions(+), 16 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index c31c9ac5fe215..75966fd0c41e7 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -286,7 +286,7 @@ benchmarks! { x )?; } - }: _(RawOrigin::Root, caller_lookup) + }: _(RawOrigin::Root, caller_lookup, s) } #[cfg(test)] diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 01e5b22f445f2..8c2cc6c2c19a4 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -818,10 +818,11 @@ decl_module! { /// # /// - `O(R)`. /// - One storage mutation `O(R)`. + /// - Benchmark: 23.81 + R * 0.774 µs (min square analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) - + 30_000_000 // constant - + 700_000 * T::MaxRegistrars::get() as Weight // R + + 23_810_000 // constant + + 774_000 * T::MaxRegistrars::get() as Weight // R ] fn set_fee(origin, #[compact] index: RegistrarIndex, @@ -848,10 +849,11 @@ decl_module! { /// # /// - `O(R)`. /// - One storage mutation `O(R)`. + /// - Benchmark: 24.59 + R * 0.832 µs (min square analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) - + 30_000_000 // constant - + 700_000 * T::MaxRegistrars::get() as Weight // R + + 24_590_000 // constant + + 832_000 * T::MaxRegistrars::get() as Weight // R ] fn set_account_id(origin, #[compact] index: RegistrarIndex, @@ -878,10 +880,11 @@ decl_module! { /// # /// - `O(R)`. /// - One storage mutation `O(R)`. + /// - Benchmark: 22.85 + R * 0.853 µs (min square analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) - + 30_000_000 // constant - + 700_000 * T::MaxRegistrars::get() as Weight // R + + 22_850_000 // constant + + 853_000 * T::MaxRegistrars::get() as Weight // R ] fn set_fields(origin, #[compact] index: RegistrarIndex, @@ -915,14 +918,15 @@ decl_module! { /// - Up to one account-lookup operation. /// - Storage: 1 read `O(R)`, 1 mutate `O(R + X)`. /// - One event. + /// - Benchmark: 110.7 + R * 1.066 + X * 3.402 µs (min square analysis) /// # #[weight = FunctionOf( |(_, _, _, &fields_count): (&RegistrarIndex, &::Source, &Judgement>, &u32)| { T::DbWeight::get().reads_writes(2, 1) + T::DbWeight::get().reads_writes(1, 1) // balance ops - + 154_000_000 // constant - + 932_000 * T::MaxRegistrars::get() as Weight // R - + 3_302_000 * fields_count as Weight // X + + 110_700_000 // constant + + 1_066_000 * T::MaxRegistrars::get() as Weight // R + + 3_402_000 * fields_count as Weight // X }, DispatchClass::Normal, true @@ -976,9 +980,21 @@ decl_module! { /// - One balance-reserve operation. /// - `S + 2` storage mutations. /// - One event. + /// - Benchmark: 167.4 + R * 1.107 + S * 5.343 + X * 2.294 µs (min square analysis) /// # - #[weight = SimpleDispatchInfo::FixedNormal(100_000_000)] - fn kill_identity(origin, target: ::Source) { + #[weight = FunctionOf( + |(_, &subs_count): (&::Source, &u32)| { + T::DbWeight::get().reads_writes(subs_count as Weight + 1, subs_count as Weight + 1) + + T::DbWeight::get().reads_writes(1, 1) // balance ops + + 167_400_000 // constant + + 1_107_000 * T::MaxRegistrars::get() as Weight // R + + 5_343_000 * subs_count as Weight // S + + 2_294_000 * T::MaxAdditionalFields::get() as Weight // X + }, + DispatchClass::Normal, + true + )] + fn kill_identity(origin, target: ::Source, subs_count: u32) { T::ForceOrigin::try_origin(origin) .map(|_| ()) .or_else(ensure_root)?; @@ -987,6 +1003,7 @@ decl_module! { let target = T::Lookup::lookup(target)?; // Grab their deposit (and check that they have one). let (subs_deposit, sub_ids) = >::take(&target); + ensure!(sub_ids.len() <= subs_count as usize, "invalid count"); let deposit = >::take(&target).ok_or(Error::::NotNamed)?.total_deposit() + subs_deposit; for sub in sub_ids.iter() { @@ -1232,11 +1249,11 @@ mod tests { fn killing_slashing_should_work() { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_noop!(Identity::kill_identity(Origin::signed(1), 10), BadOrigin); - assert_ok!(Identity::kill_identity(Origin::signed(2), 10)); + assert_noop!(Identity::kill_identity(Origin::signed(1), 10, 0), BadOrigin); + assert_ok!(Identity::kill_identity(Origin::signed(2), 10, 0)); assert_eq!(Identity::identity(10), None); assert_eq!(Balances::free_balance(10), 90); - assert_noop!(Identity::kill_identity(Origin::signed(2), 10), Error::::NotNamed); + assert_noop!(Identity::kill_identity(Origin::signed(2), 10, 0), Error::::NotNamed); }); } @@ -1299,7 +1316,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))], 0)); - assert_ok!(Identity::kill_identity(Origin::ROOT, 10)); + assert_ok!(Identity::kill_identity(Origin::ROOT, 10, 1)); assert_eq!(Balances::free_balance(10), 80); assert!(Identity::super_of(20).is_none()); }); From 6dce9f10db49c205baccde3a59813e3156f45646 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Wed, 22 Apr 2020 17:41:50 +0200 Subject: [PATCH 24/34] use weight refunds to use more constants in weight estimation --- frame/identity/src/benchmarking.rs | 12 +- frame/identity/src/lib.rs | 190 ++++++++++++++++------------- 2 files changed, 111 insertions(+), 91 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 75966fd0c41e7..f7b028d2d3d00 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -37,7 +37,7 @@ fn account(name: &'static str, index: u32) -> T::AccountId { fn add_registrars(r: u32) -> Result<(), &'static str> { for i in 0..r { let _ = T::Currency::make_free_balance_be(&account::("registrar", i), BalanceOf::::max_value()); - Identity::::add_registrar(RawOrigin::Root.into(), account::("registrar", i), i)?; + Identity::::add_registrar(RawOrigin::Root.into(), account::("registrar", i))?; Identity::::set_fee(RawOrigin::Signed(account::("registrar", i)).into(), i.into(), 10.into())?; let fields = IdentityFields( IdentityField::Display | IdentityField::Legal | IdentityField::Web | IdentityField::Riot @@ -123,7 +123,7 @@ benchmarks! { add_registrar { let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; - }: _(RawOrigin::Root, account::("registrar", r + 1), r) + }: _(RawOrigin::Root, account::("registrar", r + 1)) set_identity { let r in ...; @@ -219,7 +219,7 @@ benchmarks! { let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; }: _(RawOrigin::Signed(caller), r, 10.into()) set_account_id { @@ -228,7 +228,7 @@ benchmarks! { let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; }: _(RawOrigin::Signed(caller), r, account::("new", 0)) set_fields { @@ -237,7 +237,7 @@ benchmarks! { let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::(r)?; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; let fields = IdentityFields( IdentityField::Display | IdentityField::Legal | IdentityField::Web | IdentityField::Riot | IdentityField::Email | IdentityField::PgpFingerprint | IdentityField::Image | IdentityField::Twitter @@ -261,7 +261,7 @@ benchmarks! { Identity::::set_identity(user_origin.clone(), info)?; }; - Identity::::add_registrar(RawOrigin::Root.into(), caller.clone(), r)?; + Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; Identity::::request_judgement(user_origin.clone(), r, 10.into(), x)?; }: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable, x) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 8c2cc6c2c19a4..2106763b2549a 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -73,8 +73,9 @@ use sp_runtime::{DispatchError, DispatchResult, RuntimeDebug}; use sp_runtime::traits::{StaticLookup, Zero, AppendZerosInput}; use frame_support::{ decl_module, decl_event, decl_storage, ensure, decl_error, + dispatch::DispatchResultWithPostInfo, traits::{Currency, ReservableCurrency, OnUnbalanced, Get, BalanceStatus, EnsureOrigin}, - weights::{SimpleDispatchInfo, FunctionOf, DispatchClass, Weight}, + weights::{DispatchClass, FunctionOf, Weight}, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -460,6 +461,26 @@ decl_error! { } } +/// Functions for calcuating the weight of dispatchables. +mod weight_for { + use frame_support::weights::{RuntimeDbWeight, Weight}; + + /// Weight calculation for `clear_identity`. + pub(crate) fn clear_identity( + db: RuntimeDbWeight, + subs: impl Into + Copy, + judgements: impl Into, + extra_fields: impl Into) -> Weight + { + db.reads_writes(2, subs.into() + 2) // S + 2 deletions + + db.reads_writes(1, 1) // balance ops + + 160_000_000 // constant + + 500_000 * judgements.into() // R + + 5_400_000 * subs.into() // S + + 2_000_000 * extra_fields.into() // X + } +} + decl_module! { // Simple declaration of the `Module` type. Lets the macro know what it's working on. pub struct Module for enum Call where origin: T::Origin { @@ -476,35 +497,31 @@ decl_module! { /// Emits `RegistrarAdded` if successful. /// /// # - /// - `O(R)` where `R` registrar-count (governance-bounded). + /// - `O(R)` where `R` registrar-count (governance-bounded and code-bounded). /// - One storage mutation (codec `O(R)`). /// - One event. - /// - Benchmark: 78.71 + R * 0.965 µs (min square analysis) + /// - Benchmarks: + /// - 78.71 + R * 0.965 µs (min squares analysis) + /// - 94.28 + R * 0.991 µs (min squares analysis) /// # - #[weight = FunctionOf( - |(_, &old_count): (&T::AccountId, &u32)| { - T::DbWeight::get().reads_writes(1, 1) - + 78_710_000 // constant - + 965_000 * (old_count as Weight) // R - }, - DispatchClass::Normal, - true - )] - fn add_registrar(origin, account: T::AccountId, old_registrar_count: u32) -> DispatchResult { + #[weight = T::DbWeight::get().reads_writes(1, 1) + + 100_000_000 // constant + + 1_000_000 * T::MaxRegistrars::get() as Weight // R + ] + fn add_registrar(origin, account: T::AccountId) -> DispatchResultWithPostInfo { T::RegistrarOrigin::try_origin(origin) .map(|_| ()) .or_else(ensure_root)?; - let i = >::try_mutate(|registrars| -> Result { - ensure!(registrars.len() as u32 <= old_registrar_count, "invalid count"); + let (i, registrar_count) = >::try_mutate(|registrars| -> Result<(RegistrarIndex, usize), DispatchError> { ensure!((registrars.len() as u32) < T::MaxRegistrars::get(), Error::::TooManyRegistrars); registrars.push(Some(RegistrarInfo { account, fee: Zero::zero(), fields: Default::default() })); - Ok((registrars.len() - 1) as RegistrarIndex) + Ok(((registrars.len() - 1) as RegistrarIndex, registrars.len())) })?; Self::deposit_event(RawEvent::RegistrarAdded(i)); - Ok(()) + Ok(Some(100_000_000 + 1_000_000 * registrar_count as Weight).into()) } /// Set an account's identity information and reserve the appropriate deposit. @@ -519,32 +536,29 @@ decl_module! { /// Emits `IdentitySet` if successful. /// /// # - /// - `O(X + X' + J)` + /// - `O(X + X' + R)` /// - where `X` additional-field-count (deposit-bounded and code-bounded) - /// - where `J` judgements-count (registrar-count-bounded) - /// - At most two balance operations. - /// - One storage mutation (codec-read `O(X' + J)`, codec-write `O(X + J)`). + /// - where `R` judgements-count (registrar-count-bounded) + /// - One balance reserve operation. + /// - One storage mutation (codec-read `O(X' + R)`, codec-write `O(X + R)`). /// - One event. - /// - Benchmark: 136.6 + J * 0.62 + X * 2.62 µs (min square analysis) + /// - Benchmark: + /// - 136.6 + R * 0.62 + X * 2.62 µs (min squares analysis) + /// - 146.2 + R * 0.372 + X * 2.98 µs (min squares analysis) /// # - #[weight = FunctionOf( - |(info,): (&IdentityInfo,)| { - T::DbWeight::get().reads_writes(1, 1) - + T::DbWeight::get().reads_writes(1, 1) // balance ops - + 136_600_000 // constant - + 2_622_000 * (info.additional.len() as Weight) // X - + 624_000 * (T::MaxRegistrars::get() as Weight) // J - }, - DispatchClass::Normal, - true - )] - fn set_identity(origin, info: IdentityInfo) { + #[weight = T::DbWeight::get().reads_writes(1, 1) + + T::DbWeight::get().reads_writes(1, 1) // balance ops + + 150_000_000 // constant + + 3_000_000 * (info.additional.len() as Weight) // X + + 700_000 * (T::MaxRegistrars::get() as Weight) // R + ] + fn set_identity(origin, info: IdentityInfo) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; let extra_fields = info.additional.len() as u32; ensure!(extra_fields <= T::MaxAdditionalFields::get(), Error::::TooManyFields); let fd = >::from(extra_fields) * T::FieldDeposit::get(); - let mut id = match >::get(&sender) { + let mut id = match >::get(&sender) { Some(mut id) => { // Only keep non-positive judgements. id.judgements.retain(|j| j.1.is_sticky()); @@ -563,8 +577,11 @@ decl_module! { let _ = T::Currency::unreserve(&sender, old_deposit - id.deposit); } + let judgements = id.judgements.len(); >::insert(&sender, id); Self::deposit_event(RawEvent::IdentitySet(sender)); + + Ok(Some(150_000_000 + 3_000_000 * extra_fields as Weight + 700_000 * judgements as Weight).into()) } /// Set the sub-accounts of the sender. @@ -583,24 +600,23 @@ decl_module! { /// - where `P` old-subs-count (hard- and deposit-bounded). /// - where `S` subs-count (hard- and deposit-bounded). /// - At most one balance operations. - /// - At most O(P + S + 1) storage mutations; codec complexity `O(S)`); - /// one storage-exists. - /// - Benchmark: 115.2 + P * 5.11 + S * 6.67 µs (min square analysis) + /// - DB: + /// - `P + S` storage mutations (codec complexity `O(1)`) + /// - One storage read (codec complexity `O(P)`). + /// - One storage write (codec complexity `O(S)`). + /// - One storage-exists (`IdentityOf::contains_key`). + /// - Benchmark: + /// - 115.2 + P * 5.11 + S * 6.67 µs (min squares analysis) + /// - 121 + P * 4.852 + S * 7.111 µs (min squares analysis) /// # - #[weight = FunctionOf( - |(subs, &old_subs_count): (&Vec<(T::AccountId, Data)>, &u32)| { - let db = T::DbWeight::get(); - db.reads(1) // storage-exists - + db.reads_writes(1, 1) // balance ops - + db.writes(old_subs_count.into()) // P old DB deletions - + db.writes(subs.len() as u64 + 1) // S + 1 new DB writes - + 115_200_000 // constant - + 5_118_000 * old_subs_count as Weight // P - + 6_676_000 * subs.len() as Weight // S - }, - DispatchClass::Normal, - true - )] + #[weight = T::DbWeight::get().reads(1) // storage-exists + + T::DbWeight::get().reads_writes(1, 1) // balance ops + + T::DbWeight::get().reads_writes(1, (*old_subs_count).into()) // P old DB deletions + + T::DbWeight::get().writes(subs.len() as u64 + 1) // S + 1 new DB writes + + 130_000_000 // constant + + 5_200_000 * (*old_subs_count) as Weight // P + + 7_300_000 * subs.len() as Weight // S + ] fn set_subs(origin, subs: Vec<(T::AccountId, Data)>, old_subs_count: u32) { let sender = ensure_signed(origin)?; ensure!(>::contains_key(&sender), Error::::NotFound); @@ -650,27 +666,24 @@ decl_module! { /// - where `S` subs-count (hard- and deposit-bounded). /// - where `X` additional-field-count (deposit-bounded and code-bounded). /// - One balance-unreserve operation. - /// - `S + 2` storage deletions. + /// - `2` storage reads and `S + 2` storage deletions. /// - One event. - /// - Benchmark: 152.3 + R * 0.306 + S * 4.967 + X * 1.697 µs (min square analysis) + /// - Benchmarks: + /// - 152.3 + R * 0.306 + S * 4.967 + X * 1.697 µs (min squares analysis) + /// - 139.5 + R * 0.466 + S * 5.304 + X * 1.895 µs (min squares analysis) /// # - #[weight = FunctionOf( - |(&subs_count,): (&u32,)| { - T::DbWeight::get().writes(subs_count as Weight + 2) // S + 2 deletions - + 152_300_000 // constant - + 306_000 * T::MaxRegistrars::get() as Weight // R - + 4_967_000 * subs_count as Weight // S - + 1_697_000 * T::MaxAdditionalFields::get() as Weight // X - }, - DispatchClass::Normal, - true + #[weight = weight_for::clear_identity( + T::DbWeight::get(), + T::MaxSubAccounts::get(), // S + T::MaxRegistrars::get(), // R + T::MaxAdditionalFields::get() // X )] - fn clear_identity(origin, subs_count: u32) { + fn clear_identity(origin) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; let (subs_deposit, sub_ids) = >::take(&sender); - ensure!(sub_ids.len() <= subs_count as usize, "invalid count"); - let deposit = >::take(&sender).ok_or(Error::::NotNamed)?.total_deposit() + let id = >::take(&sender).ok_or(Error::::NotNamed)?; + let deposit = id.total_deposit() + subs_deposit; for sub in sub_ids.iter() { >::remove(sub); @@ -679,6 +692,13 @@ decl_module! { let _ = T::Currency::unreserve(&sender, deposit.clone()); Self::deposit_event(RawEvent::IdentityCleared(sender, deposit)); + + Ok(Some(weight_for::clear_identity( + T::DbWeight::get(), + sub_ids.len() as Weight, + id.judgements.len() as Weight, + id.info.additional.len() as Weight + )).into()) } /// Request a judgement from a registrar. @@ -710,7 +730,7 @@ decl_module! { /// - One balance-reserve operation. /// - Storage: 1 read `O(R)`, 1 mutate `O(X + R)`. /// - One event. - /// - Benchmark: 154 + R * 0.932 + X * 3.302 µs (min square analysis) + /// - Benchmark: 154 + R * 0.932 + X * 3.302 µs (min squares analysis) /// # #[weight = FunctionOf( |(_, _, &fields_count): (&RegistrarIndex, &BalanceOf, &u32)| { @@ -775,7 +795,7 @@ decl_module! { /// - One balance-reserve operation. /// - One storage mutation `O(R + X)`. /// - One event. - /// - Benchmark: 135.3 + R * 0.574 + X * 3.394 µs (min square analysis) + /// - Benchmark: 135.3 + R * 0.574 + X * 3.394 µs (min squares analysis) /// # #[weight = FunctionOf( |(_, &fields_count): (&RegistrarIndex, &u32)| { @@ -818,7 +838,7 @@ decl_module! { /// # /// - `O(R)`. /// - One storage mutation `O(R)`. - /// - Benchmark: 23.81 + R * 0.774 µs (min square analysis) + /// - Benchmark: 23.81 + R * 0.774 µs (min squares analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) + 23_810_000 // constant @@ -849,7 +869,7 @@ decl_module! { /// # /// - `O(R)`. /// - One storage mutation `O(R)`. - /// - Benchmark: 24.59 + R * 0.832 µs (min square analysis) + /// - Benchmark: 24.59 + R * 0.832 µs (min squares analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) + 24_590_000 // constant @@ -880,7 +900,7 @@ decl_module! { /// # /// - `O(R)`. /// - One storage mutation `O(R)`. - /// - Benchmark: 22.85 + R * 0.853 µs (min square analysis) + /// - Benchmark: 22.85 + R * 0.853 µs (min squares analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) + 22_850_000 // constant @@ -918,7 +938,7 @@ decl_module! { /// - Up to one account-lookup operation. /// - Storage: 1 read `O(R)`, 1 mutate `O(R + X)`. /// - One event. - /// - Benchmark: 110.7 + R * 1.066 + X * 3.402 µs (min square analysis) + /// - Benchmark: 110.7 + R * 1.066 + X * 3.402 µs (min squares analysis) /// # #[weight = FunctionOf( |(_, _, _, &fields_count): (&RegistrarIndex, &::Source, &Judgement>, &u32)| { @@ -980,7 +1000,7 @@ decl_module! { /// - One balance-reserve operation. /// - `S + 2` storage mutations. /// - One event. - /// - Benchmark: 167.4 + R * 1.107 + S * 5.343 + X * 2.294 µs (min square analysis) + /// - Benchmark: 167.4 + R * 1.107 + S * 5.343 + X * 2.294 µs (min squares analysis) /// # #[weight = FunctionOf( |(_, &subs_count): (&::Source, &u32)| { @@ -1159,7 +1179,7 @@ mod tests { #[test] fn adding_registrar_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); let fields = IdentityFields(IdentityField::Display | IdentityField::Legal); assert_ok!(Identity::set_fields(Origin::signed(3), 0, fields)); @@ -1173,11 +1193,11 @@ mod tests { fn amount_of_registrars_is_limited() { new_test_ext().execute_with(|| { for i in 1..MaxRegistrars::get() + 1 { - assert_ok!(Identity::add_registrar(Origin::signed(1), i as u64, i - 1)); + assert_ok!(Identity::add_registrar(Origin::signed(1), i as u64)); } let last_registrar = MaxRegistrars::get() as u64 + 1; assert_noop!( - Identity::add_registrar(Origin::signed(1), last_registrar, MaxRegistrars::get()), + Identity::add_registrar(Origin::signed(1), last_registrar), Error::::TooManyRegistrars ); }); @@ -1186,7 +1206,7 @@ mod tests { #[test] fn registration_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); let mut three_fields = ten(); three_fields.additional.push(Default::default()); @@ -1213,7 +1233,7 @@ mod tests { Error::::InvalidIndex ); - assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_noop!( Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0), Error::::InvalidTarget @@ -1237,7 +1257,7 @@ mod tests { #[test] fn clearing_judgement_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0)); assert_ok!(Identity::clear_identity(Origin::signed(10), 0)); @@ -1325,7 +1345,7 @@ mod tests { #[test] fn cancelling_requested_judgement_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::NoIdentity); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); @@ -1342,7 +1362,7 @@ mod tests { #[test] fn requesting_judgement_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 9, 0), Error::::FeeChanged); @@ -1360,7 +1380,7 @@ mod tests { assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10, 0), Error::::StickyJudgement); // Requesting from a second registrar still works. - assert_ok!(Identity::add_registrar(Origin::signed(1), 4, 1)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 4)); assert_ok!(Identity::request_judgement(Origin::signed(10), 1, 10, 0)); // Re-requesting after the judgement has been reduced works. @@ -1372,7 +1392,7 @@ mod tests { #[test] fn field_deposit_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_ok!(Identity::set_identity(Origin::signed(10), IdentityInfo { additional: vec![ @@ -1387,7 +1407,7 @@ mod tests { #[test] fn setting_account_id_should_work() { new_test_ext().execute_with(|| { - assert_ok!(Identity::add_registrar(Origin::signed(1), 3, 0)); + assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); // account 4 cannot change the first registrar's identity since it's owned by 3. assert_noop!(Identity::set_account_id(Origin::signed(4), 0, 3), Error::::InvalidIndex); // account 3 can, because that's the registrar's current account. From 21ee4131ad031c3d6f44fef2f79c790684ef7724 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Apr 2020 09:26:05 +0200 Subject: [PATCH 25/34] adjust usage of clear_identity --- frame/identity/src/benchmarking.rs | 2 +- frame/identity/src/lib.rs | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index f7b028d2d3d00..0cd6d3f6f3234 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -193,7 +193,7 @@ benchmarks! { x )?; } - }: _(RawOrigin::Signed(caller), s) + }: _(RawOrigin::Signed(caller)) request_judgement { let caller = account::("caller", 0); diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 2106763b2549a..37d79ddbf5b26 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -1219,9 +1219,9 @@ mod tests { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_eq!(Identity::identity(10).unwrap().info, ten()); assert_eq!(Balances::free_balance(10), 90); - assert_ok!(Identity::clear_identity(Origin::signed(10), 0)); + assert_ok!(Identity::clear_identity(Origin::signed(10))); assert_eq!(Balances::free_balance(10), 100); - assert_noop!(Identity::clear_identity(Origin::signed(10), 0), Error::::NotNamed); + assert_noop!(Identity::clear_identity(Origin::signed(10)), Error::::NotNamed); }); } @@ -1260,7 +1260,7 @@ mod tests { assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0)); - assert_ok!(Identity::clear_identity(Origin::signed(10), 0)); + assert_ok!(Identity::clear_identity(Origin::signed(10))); assert_eq!(Identity::identity(10), None); }); } @@ -1325,7 +1325,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))], 0)); - assert_ok!(Identity::clear_identity(Origin::signed(10), 1)); + assert_ok!(Identity::clear_identity(Origin::signed(10))); assert_eq!(Balances::free_balance(10), 100); assert!(Identity::super_of(20).is_none()); }); From 9316e8e967b54b78e0f1027ed80b35c5eeeeef13 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Apr 2020 09:40:54 +0200 Subject: [PATCH 26/34] refund request_judgement weights and remove param --- frame/identity/src/benchmarking.rs | 12 +++--- frame/identity/src/lib.rs | 59 ++++++++++++++++++------------ 2 files changed, 41 insertions(+), 30 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 0cd6d3f6f3234..5d3e2c80677af 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -143,7 +143,7 @@ benchmarks! { // User requests judgement from all the registrars, and they approve for i in 0..r { - Identity::::request_judgement(caller_origin.clone(), i, 10.into(), x)?; + Identity::::request_judgement(caller_origin.clone(), i, 10.into())?; Identity::::provide_judgement( RawOrigin::Signed(account::("registrar", i)).into(), i, @@ -184,7 +184,7 @@ benchmarks! { // User requests judgement from all the registrars, and they approve for i in 0..r { - Identity::::request_judgement(caller_origin.clone(), i, 10.into(), x)?; + Identity::::request_judgement(caller_origin.clone(), i, 10.into())?; Identity::::provide_judgement( RawOrigin::Signed(account::("registrar", i)).into(), i, @@ -201,7 +201,7 @@ benchmarks! { let r in ...; let x in ...; - }: _(RawOrigin::Signed(caller), r - 1, 10.into(), x) + }: _(RawOrigin::Signed(caller), r - 1, 10.into()) cancel_request { let caller = account::("caller", 0); @@ -211,7 +211,7 @@ benchmarks! { let r in ...; let x in ...; - Identity::::request_judgement(caller_origin, r - 1, 10.into(), x)?; + Identity::::request_judgement(caller_origin, r - 1, 10.into())?; }: _(RawOrigin::Signed(caller), r - 1, x) set_fee { @@ -262,7 +262,7 @@ benchmarks! { }; Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; - Identity::::request_judgement(user_origin.clone(), r, 10.into(), x)?; + Identity::::request_judgement(user_origin.clone(), r, 10.into())?; }: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable, x) kill_identity { @@ -277,7 +277,7 @@ benchmarks! { // User requests judgement from all the registrars, and they approve for i in 0..r { - Identity::::request_judgement(caller_origin.clone(), i, 10.into(), x)?; + Identity::::request_judgement(caller_origin.clone(), i, 10.into())?; Identity::::provide_judgement( RawOrigin::Signed(account::("registrar", i)).into(), i, diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 37d79ddbf5b26..8f82db0174ffc 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -479,6 +479,19 @@ mod weight_for { + 5_400_000 * subs.into() // S + 2_000_000 * extra_fields.into() // X } + + /// Weight calculation for `request_judgement`. + pub(crate) fn request_judgement( + db: RuntimeDbWeight, + judgements: impl Into, + extra_fields: impl Into) -> Weight + { + db.reads_writes(2, 1) + + db.reads_writes(1, 1) // balance ops + + 180_000_000 // constant + + 950_000 * judgements.into() // R + + 3_400_000 * extra_fields.into() // X + } } decl_module! { @@ -695,9 +708,9 @@ decl_module! { Ok(Some(weight_for::clear_identity( T::DbWeight::get(), - sub_ids.len() as Weight, - id.judgements.len() as Weight, - id.info.additional.len() as Weight + sub_ids.len() as Weight, // S + id.judgements.len() as Weight, // R + id.info.additional.len() as Weight // X )).into()) } @@ -730,31 +743,25 @@ decl_module! { /// - One balance-reserve operation. /// - Storage: 1 read `O(R)`, 1 mutate `O(X + R)`. /// - One event. - /// - Benchmark: 154 + R * 0.932 + X * 3.302 µs (min squares analysis) + /// - Benchmarks: + /// - 154 + R * 0.932 + X * 3.302 µs (min squares analysis) + /// - 172.9 + R * 0.69 + X * 3.304 µs (min squares analysis) /// # - #[weight = FunctionOf( - |(_, _, &fields_count): (&RegistrarIndex, &BalanceOf, &u32)| { - T::DbWeight::get().reads_writes(2, 1) - + T::DbWeight::get().reads_writes(1, 1) // balance ops - + 154_000_000 // constant - + 932_000 * T::MaxRegistrars::get() as Weight // R - + 3_302_000 * fields_count as Weight // X - }, - DispatchClass::Normal, - true + #[weight = weight_for::request_judgement( + T::DbWeight::get(), + T::MaxRegistrars::get(), // R + T::MaxAdditionalFields::get() // X )] fn request_judgement(origin, #[compact] reg_index: RegistrarIndex, #[compact] max_fee: BalanceOf, - additional_fields_count: u32, - ) { + ) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; let registrars = >::get(); let registrar = registrars.get(reg_index as usize).and_then(Option::as_ref) .ok_or(Error::::EmptyIndex)?; ensure!(max_fee >= registrar.fee, Error::::FeeChanged); let mut id = >::get(&sender).ok_or(Error::::NoIdentity)?; - ensure!(id.info.additional.len() <= additional_fields_count as usize, "invalid count"); let item = (reg_index, Judgement::FeePaid(registrar.fee)); match id.judgements.binary_search_by_key(®_index, |x| x.0) { @@ -768,9 +775,13 @@ decl_module! { T::Currency::reserve(&sender, registrar.fee)?; + let judgements = id.judgements.len() as Weight; + let extra_fields = id.info.additional.len() as Weight; >::insert(&sender, id); Self::deposit_event(RawEvent::JudgementRequested(sender, reg_index)); + + Ok(Some(weight_for::request_judgement(T::DbWeight::get(), judgements, extra_fields)).into()) } /// Cancel a previous request. @@ -1349,7 +1360,7 @@ mod tests { assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::NoIdentity); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10, 0)); + assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10)); assert_ok!(Identity::cancel_request(Origin::signed(10), 0, 0)); assert_eq!(Balances::free_balance(10), 90); assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::NotFound); @@ -1365,27 +1376,27 @@ mod tests { assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 9, 0), Error::::FeeChanged); - assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10, 0)); + assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 9), Error::::FeeChanged); + assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10)); // 10 for the judgement request, 10 for the identity. assert_eq!(Balances::free_balance(10), 80); // Re-requesting won't work as we already paid. - assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10, 0), Error::::StickyJudgement); + assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10), Error::::StickyJudgement); assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Erroneous, 0)); // Registrar got their payment now. assert_eq!(Balances::free_balance(3), 20); // Re-requesting still won't work as it's erroneous. - assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10, 0), Error::::StickyJudgement); + assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10), Error::::StickyJudgement); // Requesting from a second registrar still works. assert_ok!(Identity::add_registrar(Origin::signed(1), 4)); - assert_ok!(Identity::request_judgement(Origin::signed(10), 1, 10, 0)); + assert_ok!(Identity::request_judgement(Origin::signed(10), 1, 10)); // Re-requesting after the judgement has been reduced works. assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::OutOfDate, 0)); - assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10, 0)); + assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10)); }); } From 94624ba1470c80530ff49a7b4db0458380a53d55 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Apr 2020 09:50:02 +0200 Subject: [PATCH 27/34] refund weights for cancel_request and remove param --- frame/identity/src/benchmarking.rs | 2 +- frame/identity/src/lib.rs | 46 +++++++++++++++++++----------- 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 5d3e2c80677af..68268f23bea39 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -212,7 +212,7 @@ benchmarks! { let x in ...; Identity::::request_judgement(caller_origin, r - 1, 10.into())?; - }: _(RawOrigin::Signed(caller), r - 1, x) + }: _(RawOrigin::Signed(caller), r - 1) set_fee { let caller = account::("caller", 0); diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 8f82db0174ffc..4228c889039e0 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -492,6 +492,19 @@ mod weight_for { + 950_000 * judgements.into() // R + 3_400_000 * extra_fields.into() // X } + + /// Weight calculation for `cancel_request`. + pub(crate) fn cancel_request( + db: RuntimeDbWeight, + judgements: impl Into, + extra_fields: impl Into) -> Weight + { + db.reads_writes(1, 1) + + db.reads_writes(1, 1) // balance ops + + 150_000_000 // constant + + 600_000 * judgements.into() // R + + 3_600_000 * extra_fields.into() // X + } } decl_module! { @@ -806,23 +819,18 @@ decl_module! { /// - One balance-reserve operation. /// - One storage mutation `O(R + X)`. /// - One event. - /// - Benchmark: 135.3 + R * 0.574 + X * 3.394 µs (min squares analysis) + /// - Benchmarks: + /// - 135.3 + R * 0.574 + X * 3.394 µs (min squares analysis) + /// - 144.3 + R * 0.316 + X * 3.53 µs (min squares analysis) /// # - #[weight = FunctionOf( - |(_, &fields_count): (&RegistrarIndex, &u32)| { - T::DbWeight::get().reads_writes(1, 1) - + T::DbWeight::get().reads_writes(1, 1) // balance ops - + 135_300_000 // constant - + 574_000 * T::MaxRegistrars::get() as Weight // R - + 3_394_000 * fields_count as Weight // X - }, - DispatchClass::Normal, - true + #[weight = weight_for::cancel_request( + T::DbWeight::get(), + T::MaxRegistrars::get(), // R + T::MaxAdditionalFields::get() // X )] - fn cancel_request(origin, reg_index: RegistrarIndex, additional_fields_count: u32) { + fn cancel_request(origin, reg_index: RegistrarIndex) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; let mut id = >::get(&sender).ok_or(Error::::NoIdentity)?; - ensure!(id.info.additional.len() <= additional_fields_count as usize, "invalid count"); let pos = id.judgements.binary_search_by_key(®_index, |x| x.0) .map_err(|_| Error::::NotFound)?; @@ -833,9 +841,13 @@ decl_module! { }; let _ = T::Currency::unreserve(&sender, fee); + let judgements = id.judgements.len() as Weight; + let extra_fields = id.info.additional.len() as Weight; >::insert(&sender, id); Self::deposit_event(RawEvent::JudgementUnrequested(sender, reg_index)); + + Ok(Some(weight_for::request_judgement(T::DbWeight::get(), judgements, extra_fields)).into()) } /// Set the fee required for a judgement to be requested from a registrar. @@ -1358,15 +1370,15 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_fee(Origin::signed(3), 0, 10)); - assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::NoIdentity); + assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::NoIdentity); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10)); - assert_ok!(Identity::cancel_request(Origin::signed(10), 0, 0)); + assert_ok!(Identity::cancel_request(Origin::signed(10), 0)); assert_eq!(Balances::free_balance(10), 90); - assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::NotFound); + assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::NotFound); assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0)); - assert_noop!(Identity::cancel_request(Origin::signed(10), 0, 0), Error::::JudgementGiven); + assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::JudgementGiven); }); } From 8574053f1d1998a64ab71616bf271e51b59ebaf8 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Apr 2020 10:29:15 +0200 Subject: [PATCH 28/34] add remaining refunds and remove params --- frame/identity/src/benchmarking.rs | 13 +- frame/identity/src/lib.rs | 185 ++++++++++++++++------------- 2 files changed, 109 insertions(+), 89 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 68268f23bea39..1b3ef5ba4209c 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -148,8 +148,7 @@ benchmarks! { RawOrigin::Signed(account::("registrar", i)).into(), i, caller_lookup.clone(), - Judgement::Reasonable, - x + Judgement::Reasonable )?; } caller @@ -189,8 +188,7 @@ benchmarks! { RawOrigin::Signed(account::("registrar", i)).into(), i, caller_lookup.clone(), - Judgement::Reasonable, - x + Judgement::Reasonable )?; } }: _(RawOrigin::Signed(caller)) @@ -263,7 +261,7 @@ benchmarks! { Identity::::add_registrar(RawOrigin::Root.into(), caller.clone())?; Identity::::request_judgement(user_origin.clone(), r, 10.into())?; - }: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable, x) + }: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable) kill_identity { let caller = account::("caller", 0); @@ -282,11 +280,10 @@ benchmarks! { RawOrigin::Signed(account::("registrar", i)).into(), i, caller_lookup.clone(), - Judgement::Reasonable, - x + Judgement::Reasonable )?; } - }: _(RawOrigin::Root, caller_lookup, s) + }: _(RawOrigin::Root, caller_lookup) } #[cfg(test)] diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 4228c889039e0..28d65c77ac0bc 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -75,7 +75,7 @@ use frame_support::{ decl_module, decl_event, decl_storage, ensure, decl_error, dispatch::DispatchResultWithPostInfo, traits::{Currency, ReservableCurrency, OnUnbalanced, Get, BalanceStatus, EnsureOrigin}, - weights::{DispatchClass, FunctionOf, Weight}, + weights::Weight, }; use frame_system::{self as system, ensure_signed, ensure_root}; @@ -465,13 +465,26 @@ decl_error! { mod weight_for { use frame_support::weights::{RuntimeDbWeight, Weight}; + /// Weight calculation for `set_identity`. + pub(crate) fn set_identity( + db: RuntimeDbWeight, + judgements: impl Into, + extra_fields: impl Into + ) -> Weight { + db.reads_writes(1, 1) + + db.reads_writes(1, 1) // balance ops + + 150_000_000 // constant + + 700_000 * judgements.into() // R + + 3_000_000 * extra_fields.into() // X + } + /// Weight calculation for `clear_identity`. pub(crate) fn clear_identity( db: RuntimeDbWeight, - subs: impl Into + Copy, judgements: impl Into, - extra_fields: impl Into) -> Weight - { + subs: impl Into + Copy, + extra_fields: impl Into + ) -> Weight { db.reads_writes(2, subs.into() + 2) // S + 2 deletions + db.reads_writes(1, 1) // balance ops + 160_000_000 // constant @@ -484,8 +497,8 @@ mod weight_for { pub(crate) fn request_judgement( db: RuntimeDbWeight, judgements: impl Into, - extra_fields: impl Into) -> Weight - { + extra_fields: impl Into + ) -> Weight { db.reads_writes(2, 1) + db.reads_writes(1, 1) // balance ops + 180_000_000 // constant @@ -497,14 +510,39 @@ mod weight_for { pub(crate) fn cancel_request( db: RuntimeDbWeight, judgements: impl Into, - extra_fields: impl Into) -> Weight - { + extra_fields: impl Into + ) -> Weight { db.reads_writes(1, 1) + db.reads_writes(1, 1) // balance ops + 150_000_000 // constant + 600_000 * judgements.into() // R + 3_600_000 * extra_fields.into() // X } + + pub(crate) fn provide_judgement( + db: RuntimeDbWeight, + judgements: impl Into, + extra_fields: impl Into + ) -> Weight { + db.reads_writes(2, 1) + + 120_000_000 // constant + + 1_100_000 * judgements.into() // R + + 3_500_000 * extra_fields.into()// X + } + + pub(crate) fn kill_identity( + db: RuntimeDbWeight, + judgements: impl Into, + subs: impl Into + Copy, + extra_fields: impl Into + ) -> Weight { + db.reads_writes(3, subs.into() + 3) // 2 `take`s + S deletions + + db.reads_writes(1, 1) // balance ops + + 170_000_000 // constant + + 1_200_000 * judgements.into() // R + + 5_400_000 * subs.into() // S + + 2_300_000 * extra_fields.into() // X + } } decl_module! { @@ -531,7 +569,7 @@ decl_module! { /// - 94.28 + R * 0.991 µs (min squares analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) - + 100_000_000 // constant + + 95_000_000 // constant + 1_000_000 * T::MaxRegistrars::get() as Weight // R ] fn add_registrar(origin, account: T::AccountId) -> DispatchResultWithPostInfo { @@ -547,7 +585,8 @@ decl_module! { Self::deposit_event(RawEvent::RegistrarAdded(i)); - Ok(Some(100_000_000 + 1_000_000 * registrar_count as Weight).into()) + Ok(Some(T::DbWeight::get().reads_writes(1, 1) + + 100_000_000 + 1_000_000 * registrar_count as Weight).into()) } /// Set an account's identity information and reserve the appropriate deposit. @@ -572,12 +611,11 @@ decl_module! { /// - 136.6 + R * 0.62 + X * 2.62 µs (min squares analysis) /// - 146.2 + R * 0.372 + X * 2.98 µs (min squares analysis) /// # - #[weight = T::DbWeight::get().reads_writes(1, 1) - + T::DbWeight::get().reads_writes(1, 1) // balance ops - + 150_000_000 // constant - + 3_000_000 * (info.additional.len() as Weight) // X - + 700_000 * (T::MaxRegistrars::get() as Weight) // R - ] + #[weight = weight_for::set_identity( + T::DbWeight::get(), + T::MaxRegistrars::get(), // R + T::MaxAdditionalFields::get() // X + )] fn set_identity(origin, info: IdentityInfo) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; let extra_fields = info.additional.len() as u32; @@ -603,11 +641,11 @@ decl_module! { let _ = T::Currency::unreserve(&sender, old_deposit - id.deposit); } - let judgements = id.judgements.len(); + let judgements = id.judgements.len() as Weight; >::insert(&sender, id); Self::deposit_event(RawEvent::IdentitySet(sender)); - Ok(Some(150_000_000 + 3_000_000 * extra_fields as Weight + 700_000 * judgements as Weight).into()) + Ok(Some(weight_for::set_identity(T::DbWeight::get(), extra_fields as Weight, judgements)).into()) } /// Set the sub-accounts of the sender. @@ -700,8 +738,8 @@ decl_module! { /// # #[weight = weight_for::clear_identity( T::DbWeight::get(), - T::MaxSubAccounts::get(), // S T::MaxRegistrars::get(), // R + T::MaxSubAccounts::get(), // S T::MaxAdditionalFields::get() // X )] fn clear_identity(origin) -> DispatchResultWithPostInfo { @@ -721,8 +759,8 @@ decl_module! { Ok(Some(weight_for::clear_identity( T::DbWeight::get(), - sub_ids.len() as Weight, // S id.judgements.len() as Weight, // R + sub_ids.len() as Weight, // S id.info.additional.len() as Weight // X )).into()) } @@ -741,13 +779,6 @@ decl_module! { /// ```nocompile /// Self::registrars().get(reg_index).unwrap().fee /// ``` - /// - /// - `additional_fields_count`: The number of additional fields on the identity info. This - /// should be auto-populated as: - /// - /// ```nocompile - /// Self::identity(account_id).unwrap().info.additional.len() - /// ``` /// /// Emits `JudgementRequested` if successful. /// @@ -805,12 +836,6 @@ decl_module! { /// registered identity. /// /// - `reg_index`: The index of the registrar whose judgement is no longer requested. - /// - `additional_fields_count`: The number of additional fields on the identity info. This - /// should be auto-populated as: - /// - /// ```nocompile - /// Self::identity(account_id).unwrap().info.additional.len() - /// ``` /// /// Emits `JudgementUnrequested` if successful. /// @@ -861,11 +886,12 @@ decl_module! { /// # /// - `O(R)`. /// - One storage mutation `O(R)`. - /// - Benchmark: 23.81 + R * 0.774 µs (min squares analysis) + /// - Benchmarks: + /// - 23.81 + R * 0.774 µs (min squares analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) - + 23_810_000 // constant - + 774_000 * T::MaxRegistrars::get() as Weight // R + + 24_000_000 // constant + + 780_000 * T::MaxRegistrars::get() as Weight // R ] fn set_fee(origin, #[compact] index: RegistrarIndex, @@ -895,8 +921,8 @@ decl_module! { /// - Benchmark: 24.59 + R * 0.832 µs (min squares analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) - + 24_590_000 // constant - + 832_000 * T::MaxRegistrars::get() as Weight // R + + 25_000_000 // constant + + 850_000 * T::MaxRegistrars::get() as Weight // R ] fn set_account_id(origin, #[compact] index: RegistrarIndex, @@ -926,8 +952,8 @@ decl_module! { /// - Benchmark: 22.85 + R * 0.853 µs (min squares analysis) /// # #[weight = T::DbWeight::get().reads_writes(1, 1) - + 22_850_000 // constant - + 853_000 * T::MaxRegistrars::get() as Weight // R + + 23_000_000 // constant + + 860_000 * T::MaxRegistrars::get() as Weight // R ] fn set_fields(origin, #[compact] index: RegistrarIndex, @@ -963,23 +989,16 @@ decl_module! { /// - One event. /// - Benchmark: 110.7 + R * 1.066 + X * 3.402 µs (min squares analysis) /// # - #[weight = FunctionOf( - |(_, _, _, &fields_count): (&RegistrarIndex, &::Source, &Judgement>, &u32)| { - T::DbWeight::get().reads_writes(2, 1) - + T::DbWeight::get().reads_writes(1, 1) // balance ops - + 110_700_000 // constant - + 1_066_000 * T::MaxRegistrars::get() as Weight // R - + 3_402_000 * fields_count as Weight // X - }, - DispatchClass::Normal, - true + #[weight = weight_for::provide_judgement( + T::DbWeight::get(), + T::MaxRegistrars::get(), // R + T::MaxAdditionalFields::get() // X )] fn provide_judgement(origin, #[compact] reg_index: RegistrarIndex, target: ::Source, judgement: Judgement>, - additional_fields_count: u32, - ) { + ) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; let target = T::Lookup::lookup(target)?; ensure!(!judgement.has_deposit(), Error::::InvalidJudgement); @@ -989,7 +1008,6 @@ decl_module! { .and_then(|r| if r.account == sender { Some(r) } else { None }) .ok_or(Error::::InvalidIndex)?; let mut id = >::get(&target).ok_or(Error::::InvalidTarget)?; - ensure!(id.info.additional.len() <= additional_fields_count as usize, "invalid count"); let item = (reg_index, judgement); match id.judgements.binary_search_by_key(®_index, |x| x.0) { @@ -1001,8 +1019,13 @@ decl_module! { } Err(position) => id.judgements.insert(position, item), } + + let judgements = id.judgements.len() as Weight; + let extra_fields = id.info.additional.len() as Weight; >::insert(&target, id); Self::deposit_event(RawEvent::JudgementGiven(target, reg_index)); + + Ok(Some(weight_for::provide_judgement(T::DbWeight::get(), judgements, extra_fields)).into()) } /// Remove an account's identity and sub-account information and slash the deposits. @@ -1025,19 +1048,13 @@ decl_module! { /// - One event. /// - Benchmark: 167.4 + R * 1.107 + S * 5.343 + X * 2.294 µs (min squares analysis) /// # - #[weight = FunctionOf( - |(_, &subs_count): (&::Source, &u32)| { - T::DbWeight::get().reads_writes(subs_count as Weight + 1, subs_count as Weight + 1) - + T::DbWeight::get().reads_writes(1, 1) // balance ops - + 167_400_000 // constant - + 1_107_000 * T::MaxRegistrars::get() as Weight // R - + 5_343_000 * subs_count as Weight // S - + 2_294_000 * T::MaxAdditionalFields::get() as Weight // X - }, - DispatchClass::Normal, - true + #[weight = weight_for::kill_identity( + T::DbWeight::get(), + T::MaxRegistrars::get(), // R + T::MaxSubAccounts::get(), // S + T::MaxAdditionalFields::get() // X )] - fn kill_identity(origin, target: ::Source, subs_count: u32) { + fn kill_identity(origin, target: ::Source) -> DispatchResultWithPostInfo { T::ForceOrigin::try_origin(origin) .map(|_| ()) .or_else(ensure_root)?; @@ -1046,9 +1063,8 @@ decl_module! { let target = T::Lookup::lookup(target)?; // Grab their deposit (and check that they have one). let (subs_deposit, sub_ids) = >::take(&target); - ensure!(sub_ids.len() <= subs_count as usize, "invalid count"); - let deposit = >::take(&target).ok_or(Error::::NotNamed)?.total_deposit() - + subs_deposit; + let id = >::take(&target).ok_or(Error::::NotNamed)?; + let deposit = id.total_deposit() + subs_deposit; for sub in sub_ids.iter() { >::remove(sub); } @@ -1056,6 +1072,13 @@ decl_module! { T::Slashed::on_unbalanced(T::Currency::slash_reserved(&target, deposit).0); Self::deposit_event(RawEvent::IdentityKilled(target, deposit)); + + Ok(Some(weight_for::kill_identity( + T::DbWeight::get(), + id.judgements.len() as Weight, // R + sub_ids.len() as Weight, // S + id.info.additional.len() as Weight // X + )).into()) } } } @@ -1252,27 +1275,27 @@ mod tests { fn uninvited_judgement_should_work() { new_test_ext().execute_with(|| { assert_noop!( - Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0), + Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable), Error::::InvalidIndex ); assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_noop!( - Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0), + Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable), Error::::InvalidTarget ); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_noop!( - Identity::provide_judgement(Origin::signed(10), 0, 10, Judgement::Reasonable, 0), + Identity::provide_judgement(Origin::signed(10), 0, 10, Judgement::Reasonable), Error::::InvalidIndex ); assert_noop!( - Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::FeePaid(1), 0), + Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::FeePaid(1)), Error::::InvalidJudgement ); - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable)); assert_eq!(Identity::identity(10).unwrap().judgements, vec![(0, Judgement::Reasonable)]); }); } @@ -1282,7 +1305,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Identity::add_registrar(Origin::signed(1), 3)); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable)); assert_ok!(Identity::clear_identity(Origin::signed(10))); assert_eq!(Identity::identity(10), None); }); @@ -1292,11 +1315,11 @@ mod tests { fn killing_slashing_should_work() { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_noop!(Identity::kill_identity(Origin::signed(1), 10, 0), BadOrigin); - assert_ok!(Identity::kill_identity(Origin::signed(2), 10, 0)); + assert_noop!(Identity::kill_identity(Origin::signed(1), 10), BadOrigin); + assert_ok!(Identity::kill_identity(Origin::signed(2), 10)); assert_eq!(Identity::identity(10), None); assert_eq!(Balances::free_balance(10), 90); - assert_noop!(Identity::kill_identity(Origin::signed(2), 10, 0), Error::::NotNamed); + assert_noop!(Identity::kill_identity(Origin::signed(2), 10), Error::::NotNamed); }); } @@ -1359,7 +1382,7 @@ mod tests { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))], 0)); - assert_ok!(Identity::kill_identity(Origin::ROOT, 10, 1)); + assert_ok!(Identity::kill_identity(Origin::ROOT, 10)); assert_eq!(Balances::free_balance(10), 80); assert!(Identity::super_of(20).is_none()); }); @@ -1377,7 +1400,7 @@ mod tests { assert_eq!(Balances::free_balance(10), 90); assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::NotFound); - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable, 0)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Reasonable)); assert_noop!(Identity::cancel_request(Origin::signed(10), 0), Error::::JudgementGiven); }); } @@ -1395,7 +1418,7 @@ mod tests { // Re-requesting won't work as we already paid. assert_noop!(Identity::request_judgement(Origin::signed(10), 0, 10), Error::::StickyJudgement); - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Erroneous, 0)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::Erroneous)); // Registrar got their payment now. assert_eq!(Balances::free_balance(3), 20); @@ -1407,7 +1430,7 @@ mod tests { assert_ok!(Identity::request_judgement(Origin::signed(10), 1, 10)); // Re-requesting after the judgement has been reduced works. - assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::OutOfDate, 0)); + assert_ok!(Identity::provide_judgement(Origin::signed(3), 0, 10, Judgement::OutOfDate)); assert_ok!(Identity::request_judgement(Origin::signed(10), 0, 10)); }); } From cb04804641158425896541cad7097ea8d0fa5918 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Apr 2020 10:47:13 +0200 Subject: [PATCH 29/34] refund weight for set_subs and remove param --- frame/identity/src/benchmarking.rs | 10 ++--- frame/identity/src/lib.rs | 62 +++++++++++++++++++----------- 2 files changed, 45 insertions(+), 27 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index 1b3ef5ba4209c..d40f16b960dda 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -72,11 +72,11 @@ fn create_sub_accounts(who: &T::AccountId, s: u32) -> Result(who: &T::AccountId, s: u32, old_subs_count: u32) -> Result, &'static str> { +fn add_sub_accounts(who: &T::AccountId, s: u32) -> Result, &'static str> { let who_origin = RawOrigin::Signed(who.clone()); let subs = create_sub_accounts::(who, s)?; - Identity::::set_subs(who_origin.into(), subs.clone(), old_subs_count)?; + Identity::::set_subs(who_origin.into(), subs.clone())?; Ok(subs) } @@ -110,7 +110,7 @@ benchmarks! { let s in 1 .. T::MaxSubAccounts::get() => { // Give them s many sub accounts let caller = account::("caller", 0); - let _ = add_sub_accounts::(&caller, s, s - 1)?; + let _ = add_sub_accounts::(&caller, s)?; }; let x in 1 .. T::MaxAdditionalFields::get() => { // Create their main identity with x additional fields @@ -163,13 +163,13 @@ benchmarks! { // Give them o many previous sub accounts. let p in 1 .. T::MaxSubAccounts::get() => { - let _ = add_sub_accounts::(&caller, p, 0)?; + let _ = add_sub_accounts::(&caller, p)?; }; // Create a new subs vec with s sub accounts let s in 1 .. T::MaxSubAccounts::get() => (); let subs = create_sub_accounts::(&caller, s)?; - }: _(RawOrigin::Signed(caller), subs, p) + }: _(RawOrigin::Signed(caller), subs) clear_identity { let caller = account::("caller", 0); diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 28d65c77ac0bc..7b2128c7852d1 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -478,6 +478,21 @@ mod weight_for { + 3_000_000 * extra_fields.into() // X } + /// Weight calculation for `set_subs`. + pub(crate) fn set_subs( + db: RuntimeDbWeight, + old_subs: impl Into + Copy, + subs: impl Into + Copy + ) -> Weight { + db.reads(1) // storage-exists + + db.reads_writes(1, 1) // balance ops + + db.reads_writes(1, old_subs.into()) // P old DB deletions + + db.writes(subs.into() + 1) // S + 1 new DB writes + + 130_000_000 // constant + + 5_200_000 * old_subs.into() // P + + 7_300_000 * subs.into() // S + } + /// Weight calculation for `clear_identity`. pub(crate) fn clear_identity( db: RuntimeDbWeight, @@ -519,6 +534,7 @@ mod weight_for { + 3_600_000 * extra_fields.into() // X } + /// Weight calculation for `provide_judgement`. pub(crate) fn provide_judgement( db: RuntimeDbWeight, judgements: impl Into, @@ -530,6 +546,7 @@ mod weight_for { + 3_500_000 * extra_fields.into()// X } + /// Weight calculation for `kill_identity`. pub(crate) fn kill_identity( db: RuntimeDbWeight, judgements: impl Into, @@ -607,7 +624,7 @@ decl_module! { /// - One balance reserve operation. /// - One storage mutation (codec-read `O(X' + R)`, codec-write `O(X + R)`). /// - One event. - /// - Benchmark: + /// - Benchmarks: /// - 136.6 + R * 0.62 + X * 2.62 µs (min squares analysis) /// - 146.2 + R * 0.372 + X * 2.98 µs (min squares analysis) /// # @@ -669,25 +686,21 @@ decl_module! { /// - One storage read (codec complexity `O(P)`). /// - One storage write (codec complexity `O(S)`). /// - One storage-exists (`IdentityOf::contains_key`). - /// - Benchmark: + /// - Benchmarks: /// - 115.2 + P * 5.11 + S * 6.67 µs (min squares analysis) /// - 121 + P * 4.852 + S * 7.111 µs (min squares analysis) /// # - #[weight = T::DbWeight::get().reads(1) // storage-exists - + T::DbWeight::get().reads_writes(1, 1) // balance ops - + T::DbWeight::get().reads_writes(1, (*old_subs_count).into()) // P old DB deletions - + T::DbWeight::get().writes(subs.len() as u64 + 1) // S + 1 new DB writes - + 130_000_000 // constant - + 5_200_000 * (*old_subs_count) as Weight // P - + 7_300_000 * subs.len() as Weight // S - ] - fn set_subs(origin, subs: Vec<(T::AccountId, Data)>, old_subs_count: u32) { + #[weight = weight_for::set_subs( + T::DbWeight::get(), + T::MaxSubAccounts::get(), // P + subs.len() as Weight // S + )] + fn set_subs(origin, subs: Vec<(T::AccountId, Data)>) -> DispatchResultWithPostInfo { let sender = ensure_signed(origin)?; ensure!(>::contains_key(&sender), Error::::NotFound); ensure!(subs.len() <= T::MaxSubAccounts::get() as usize, Error::::TooManySubAccounts); let (old_deposit, old_ids) = >::get(&sender); - ensure!(old_ids.len() <= old_subs_count as usize, "invalid count"); let new_deposit = T::SubAccountDeposit::get() * >::from(subs.len() as u32); if old_deposit < new_deposit { @@ -705,12 +718,19 @@ decl_module! { >::insert(&id, (sender.clone(), name)); id }).collect::>(); + let new_subs = ids.len() as Weight; if ids.is_empty() { >::remove(&sender); } else { >::insert(&sender, (new_deposit, ids)); } + + Ok(Some(weight_for::set_subs( + T::DbWeight::get(), + old_ids.len() as Weight, // P + new_subs // S + )).into()) } /// Clear an account's identity info and all sub-accounts and return all deposits. @@ -1327,27 +1347,25 @@ mod tests { fn setting_subaccounts_should_work() { new_test_ext().execute_with(|| { let mut subs = vec![(20, Data::Raw(vec![40; 1]))]; - assert_noop!(Identity::set_subs(Origin::signed(10), subs.clone(), 0), Error::::NotFound); + assert_noop!(Identity::set_subs(Origin::signed(10), subs.clone()), Error::::NotFound); assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone(), 0)); + assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone())); assert_eq!(Balances::free_balance(10), 80); assert_eq!(Identity::subs_of(10), (10, vec![20])); assert_eq!(Identity::super_of(20), Some((10, Data::Raw(vec![40; 1])))); // push another item and re-set it. - let prev_length = subs.len(); subs.push((30, Data::Raw(vec![50; 1]))); - assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone(), prev_length as u32)); + assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone())); assert_eq!(Balances::free_balance(10), 70); assert_eq!(Identity::subs_of(10), (20, vec![20, 30])); assert_eq!(Identity::super_of(20), Some((10, Data::Raw(vec![40; 1])))); assert_eq!(Identity::super_of(30), Some((10, Data::Raw(vec![50; 1])))); // switch out one of the items and re-set. - let prev_length = subs.len(); subs[0] = (40, Data::Raw(vec![60; 1])); - assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone(), prev_length as u32)); + assert_ok!(Identity::set_subs(Origin::signed(10), subs.clone())); assert_eq!(Balances::free_balance(10), 70); // no change in the balance assert_eq!(Identity::subs_of(10), (20, vec![40, 30])); assert_eq!(Identity::super_of(20), None); @@ -1355,14 +1373,14 @@ mod tests { assert_eq!(Identity::super_of(40), Some((10, Data::Raw(vec![60; 1])))); // clear - assert_ok!(Identity::set_subs(Origin::signed(10), vec![], prev_length as u32)); + assert_ok!(Identity::set_subs(Origin::signed(10), vec![])); assert_eq!(Balances::free_balance(10), 90); assert_eq!(Identity::subs_of(10), (0, vec![])); assert_eq!(Identity::super_of(30), None); assert_eq!(Identity::super_of(40), None); subs.push((20, Data::Raw(vec![40; 1]))); - assert_noop!(Identity::set_subs(Origin::signed(10), subs.clone(), 0), Error::::TooManySubAccounts); + assert_noop!(Identity::set_subs(Origin::signed(10), subs.clone()), Error::::TooManySubAccounts); }); } @@ -1370,7 +1388,7 @@ mod tests { fn clearing_account_should_remove_subaccounts_and_refund() { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))], 0)); + assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))])); assert_ok!(Identity::clear_identity(Origin::signed(10))); assert_eq!(Balances::free_balance(10), 100); assert!(Identity::super_of(20).is_none()); @@ -1381,7 +1399,7 @@ mod tests { fn killing_account_should_remove_subaccounts_and_not_refund() { new_test_ext().execute_with(|| { assert_ok!(Identity::set_identity(Origin::signed(10), ten())); - assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))], 0)); + assert_ok!(Identity::set_subs(Origin::signed(10), vec![(20, Data::Raw(vec![40; 1]))])); assert_ok!(Identity::kill_identity(Origin::ROOT, 10)); assert_eq!(Balances::free_balance(10), 80); assert!(Identity::super_of(20).is_none()); From e0bbfcce944fdbb7b8e4550e5cb6fae2b794bbca Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Apr 2020 10:48:20 +0200 Subject: [PATCH 30/34] make comment more specific --- frame/identity/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 7b2128c7852d1..134206ff0b433 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -484,7 +484,7 @@ mod weight_for { old_subs: impl Into + Copy, subs: impl Into + Copy ) -> Weight { - db.reads(1) // storage-exists + db.reads(1) // storage-exists (`IdentityOf::contains_key`) + db.reads_writes(1, 1) // balance ops + db.reads_writes(1, old_subs.into()) // P old DB deletions + db.writes(subs.into() + 1) // S + 1 new DB writes From f5ffbb5d1da2ee62ab5d00ea51241e37010e3167 Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Apr 2020 14:59:23 +0200 Subject: [PATCH 31/34] add range note to benchmarking docs --- frame/benchmarking/src/lib.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 6bb10f3d97216..c7f5e005035f3 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -69,6 +69,9 @@ pub use paste; /// (or not) by each arm. Syntax is available to allow for only the range to be drawn upon if /// desired, allowing an alternative instancing expression to be given. /// +/// Note that the ranges are *inclusive* on both sides. This is in contrast to ranges in Rust which +/// are left-inclusive right-exclusive. +/// /// Each arm may also have a block of code which is run prior to any instancing and a block of code /// which is run afterwards. All code blocks may draw upon the specific value of each parameter /// at any time. Local variables are shared between the two pre- and post- code blocks, but do not @@ -81,6 +84,7 @@ pub use paste; /// ```ignore /// benchmarks! { /// // common parameter; just one for this example. +/// // will be `1`, `MAX_LENGTH` or any value inbetween /// _ { /// let l in 1 .. MAX_LENGTH => initialize_l(l); /// } From 1072ff8b6c3862e73ff4f847b07b6fb01c0446da Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Apr 2020 15:10:05 +0200 Subject: [PATCH 32/34] fix inconsistencies before review --- frame/identity/src/benchmarking.rs | 2 +- frame/identity/src/lib.rs | 13 +++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/frame/identity/src/benchmarking.rs b/frame/identity/src/benchmarking.rs index d40f16b960dda..81a9f3e1340cf 100644 --- a/frame/identity/src/benchmarking.rs +++ b/frame/identity/src/benchmarking.rs @@ -161,7 +161,7 @@ benchmarks! { set_subs { let caller = account::("caller", 0); - // Give them o many previous sub accounts. + // Give them p many previous sub accounts. let p in 1 .. T::MaxSubAccounts::get() => { let _ = add_sub_accounts::(&caller, p)?; }; diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 134206ff0b433..23b8c7a049d70 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -639,7 +639,7 @@ decl_module! { ensure!(extra_fields <= T::MaxAdditionalFields::get(), Error::::TooManyFields); let fd = >::from(extra_fields) * T::FieldDeposit::get(); - let mut id = match >::get(&sender) { + let mut id = match >::get(&sender) { Some(mut id) => { // Only keep non-positive judgements. id.judgements.retain(|j| j.1.is_sticky()); @@ -662,7 +662,11 @@ decl_module! { >::insert(&sender, id); Self::deposit_event(RawEvent::IdentitySet(sender)); - Ok(Some(weight_for::set_identity(T::DbWeight::get(), extra_fields as Weight, judgements)).into()) + Ok(Some(weight_for::set_identity( + T::DbWeight::get(), + judgements, // R + extra_fields as Weight // X + )).into()) } /// Set the sub-accounts of the sender. @@ -673,8 +677,7 @@ decl_module! { /// The dispatch origin for this call must be _Signed_ and the sender must have a registered /// identity. /// - /// - `subs`: The identity's sub-accounts. - /// - `old_subs_count`: This is the number of previous sub accounts associated with the identity. + /// - `subs`: The identity's (new) sub-accounts. /// /// # /// - `O(P + S)` @@ -739,8 +742,6 @@ decl_module! { /// /// The dispatch origin for this call must be _Signed_ and the sender must have a registered /// identity. - /// - /// - `subs_count`: This is the number of sub accounts associated with the identity to clear. /// /// Emits `IdentityCleared` if successful. /// From 98718d9e233174cb2c6c834d84601fe83476d6ee Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Thu, 23 Apr 2020 18:30:59 +0200 Subject: [PATCH 33/34] fix actual weight calculation for add_registrar --- frame/identity/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 23b8c7a049d70..4f105a4990434 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -603,7 +603,7 @@ decl_module! { Self::deposit_event(RawEvent::RegistrarAdded(i)); Ok(Some(T::DbWeight::get().reads_writes(1, 1) - + 100_000_000 + 1_000_000 * registrar_count as Weight).into()) + + 95_000_000 + 1_000_000 * registrar_count as Weight).into()) } /// Set an account's identity information and reserve the appropriate deposit. From 02f8cabafcb7204fb1546894fb3ea6c7b189452e Mon Sep 17 00:00:00 2001 From: Alexander Popiak Date: Fri, 24 Apr 2020 11:45:45 +0200 Subject: [PATCH 34/34] remove duplicate balance ops weights + refund on all dispatchables --- frame/identity/src/lib.rs | 45 ++++++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/frame/identity/src/lib.rs b/frame/identity/src/lib.rs index 4f105a4990434..273cf5f71a544 100644 --- a/frame/identity/src/lib.rs +++ b/frame/identity/src/lib.rs @@ -69,7 +69,7 @@ use sp_std::prelude::*; use sp_std::{fmt::Debug, ops::Add, iter::once}; use enumflags2::BitFlags; use codec::{Encode, Decode}; -use sp_runtime::{DispatchError, DispatchResult, RuntimeDebug}; +use sp_runtime::{DispatchError, RuntimeDebug}; use sp_runtime::traits::{StaticLookup, Zero, AppendZerosInput}; use frame_support::{ decl_module, decl_event, decl_storage, ensure, decl_error, @@ -472,7 +472,6 @@ mod weight_for { extra_fields: impl Into ) -> Weight { db.reads_writes(1, 1) - + db.reads_writes(1, 1) // balance ops + 150_000_000 // constant + 700_000 * judgements.into() // R + 3_000_000 * extra_fields.into() // X @@ -485,8 +484,7 @@ mod weight_for { subs: impl Into + Copy ) -> Weight { db.reads(1) // storage-exists (`IdentityOf::contains_key`) - + db.reads_writes(1, 1) // balance ops - + db.reads_writes(1, old_subs.into()) // P old DB deletions + + db.reads_writes(1, old_subs.into()) // `SubsOf::get` read + P old DB deletions + db.writes(subs.into() + 1) // S + 1 new DB writes + 130_000_000 // constant + 5_200_000 * old_subs.into() // P @@ -501,7 +499,6 @@ mod weight_for { extra_fields: impl Into ) -> Weight { db.reads_writes(2, subs.into() + 2) // S + 2 deletions - + db.reads_writes(1, 1) // balance ops + 160_000_000 // constant + 500_000 * judgements.into() // R + 5_400_000 * subs.into() // S @@ -515,7 +512,6 @@ mod weight_for { extra_fields: impl Into ) -> Weight { db.reads_writes(2, 1) - + db.reads_writes(1, 1) // balance ops + 180_000_000 // constant + 950_000 * judgements.into() // R + 3_400_000 * extra_fields.into() // X @@ -528,7 +524,6 @@ mod weight_for { extra_fields: impl Into ) -> Weight { db.reads_writes(1, 1) - + db.reads_writes(1, 1) // balance ops + 150_000_000 // constant + 600_000 * judgements.into() // R + 3_600_000 * extra_fields.into() // X @@ -917,15 +912,19 @@ decl_module! { fn set_fee(origin, #[compact] index: RegistrarIndex, #[compact] fee: BalanceOf, - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - >::mutate(|rs| + let registrars = >::mutate(|rs| -> Result { rs.get_mut(index as usize) .and_then(|x| x.as_mut()) .and_then(|r| if r.account == who { r.fee = fee; Some(()) } else { None }) - .ok_or_else(|| Error::::InvalidIndex.into()) - ) + .ok_or_else(|| DispatchError::from(Error::::InvalidIndex))?; + Ok(rs.len()) + })?; + Ok(Some(T::DbWeight::get().reads_writes(1, 1) + + 24_000_000 + 780_000 * registrars as Weight // R + ).into()) } /// Change the account associated with a registrar. @@ -948,15 +947,19 @@ decl_module! { fn set_account_id(origin, #[compact] index: RegistrarIndex, new: T::AccountId, - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - >::mutate(|rs| + let registrars = >::mutate(|rs| -> Result { rs.get_mut(index as usize) .and_then(|x| x.as_mut()) .and_then(|r| if r.account == who { r.account = new; Some(()) } else { None }) - .ok_or_else(|| Error::::InvalidIndex.into()) - ) + .ok_or_else(|| DispatchError::from(Error::::InvalidIndex))?; + Ok(rs.len()) + })?; + Ok(Some(T::DbWeight::get().reads_writes(1, 1) + + 25_000_000 + 850_000 * registrars as Weight // R + ).into()) } /// Set the field information for a registrar. @@ -979,15 +982,19 @@ decl_module! { fn set_fields(origin, #[compact] index: RegistrarIndex, fields: IdentityFields, - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { let who = ensure_signed(origin)?; - >::mutate(|rs| + let registrars = >::mutate(|rs| -> Result { rs.get_mut(index as usize) .and_then(|x| x.as_mut()) .and_then(|r| if r.account == who { r.fields = fields; Some(()) } else { None }) - .ok_or_else(|| Error::::InvalidIndex.into()) - ) + .ok_or_else(|| DispatchError::from(Error::::InvalidIndex))?; + Ok(rs.len()) + })?; + Ok(Some(T::DbWeight::get().reads_writes(1, 1) + + 23_000_000 + 860_000 * registrars as Weight // R + ).into()) } /// Provide a judgement for an account's identity.