From 457430a5dcf3627660cf4bc0c1bb04675468d4a2 Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Wed, 14 Feb 2024 13:24:28 +0100 Subject: [PATCH 1/4] Membership weights --- substrate/frame/membership/src/lib.rs | 61 ++++++++++++------- .../frame/membership/src/migrations/v4.rs | 6 +- substrate/frame/membership/src/weights.rs | 18 ++---- 3 files changed, 47 insertions(+), 38 deletions(-) diff --git a/substrate/frame/membership/src/lib.rs b/substrate/frame/membership/src/lib.rs index 0e03c097d123..6422fb1d1e0e 100644 --- a/substrate/frame/membership/src/lib.rs +++ b/substrate/frame/membership/src/lib.rs @@ -27,7 +27,7 @@ use frame_support::{ traits::{ChangeMembers, Contains, Get, InitializeMembers, SortedMembers}, BoundedVec, }; -use sp_runtime::traits::StaticLookup; +use sp_runtime::traits::{StaticLookup, UniqueSaturatedInto}; use sp_std::prelude::*; pub mod migrations; @@ -163,12 +163,16 @@ pub mod pallet { /// /// May only be called from `T::AddOrigin`. #[pallet::call_index(0)] - #[pallet::weight({50_000_000})] - pub fn add_member(origin: OriginFor, who: AccountIdLookupOf) -> DispatchResult { + #[pallet::weight(T::WeightInfo::add_member(T::MaxMembers::get()))] + pub fn add_member( + origin: OriginFor, + who: AccountIdLookupOf, + ) -> DispatchResultWithPostInfo { T::AddOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; let mut members = >::get(); + let init_length = members.len(); let location = members.binary_search(&who).err().ok_or(Error::::AlreadyMember)?; members .try_insert(location, who.clone()) @@ -179,19 +183,24 @@ pub mod pallet { T::MembershipChanged::change_members_sorted(&[who], &[], &members[..]); Self::deposit_event(Event::MemberAdded); - Ok(()) + + Ok(Some(T::WeightInfo::add_member(init_length as u32)).into()) } /// Remove a member `who` from the set. /// /// May only be called from `T::RemoveOrigin`. #[pallet::call_index(1)] - #[pallet::weight({50_000_000})] - pub fn remove_member(origin: OriginFor, who: AccountIdLookupOf) -> DispatchResult { + #[pallet::weight(T::WeightInfo::remove_member(T::MaxMembers::get()))] + pub fn remove_member( + origin: OriginFor, + who: AccountIdLookupOf, + ) -> DispatchResultWithPostInfo { T::RemoveOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; let mut members = >::get(); + let init_length = members.len(); let location = members.binary_search(&who).ok().ok_or(Error::::NotMember)?; members.remove(location); @@ -201,7 +210,7 @@ pub mod pallet { Self::rejig_prime(&members); Self::deposit_event(Event::MemberRemoved); - Ok(()) + Ok(Some(T::WeightInfo::remove_member(init_length as u32)).into()) } /// Swap out one member `remove` for another `add`. @@ -210,18 +219,18 @@ pub mod pallet { /// /// Prime membership is *not* passed from `remove` to `add`, if extant. #[pallet::call_index(2)] - #[pallet::weight({50_000_000})] + #[pallet::weight(T::WeightInfo::swap_member(T::MaxMembers::get()))] pub fn swap_member( origin: OriginFor, remove: AccountIdLookupOf, add: AccountIdLookupOf, - ) -> DispatchResult { + ) -> DispatchResultWithPostInfo { T::SwapOrigin::ensure_origin(origin)?; let remove = T::Lookup::lookup(remove)?; let add = T::Lookup::lookup(add)?; if remove == add { - return Ok(()) + return Ok(().into()); } let mut members = >::get(); @@ -236,7 +245,7 @@ pub mod pallet { Self::rejig_prime(&members); Self::deposit_event(Event::MembersSwapped); - Ok(()) + Ok(Some(T::WeightInfo::swap_member(members.len() as u32)).into()) } /// Change the membership to a new set, disregarding the existing membership. Be nice and @@ -244,7 +253,7 @@ pub mod pallet { /// /// May only be called from `T::ResetOrigin`. #[pallet::call_index(3)] - #[pallet::weight({50_000_000})] + #[pallet::weight(T::WeightInfo::reset_members(members.len().unique_saturated_into()))] pub fn reset_members(origin: OriginFor, members: Vec) -> DispatchResult { T::ResetOrigin::ensure_origin(origin)?; @@ -267,13 +276,19 @@ pub mod pallet { /// /// Prime membership is passed from the origin account to `new`, if extant. #[pallet::call_index(4)] - #[pallet::weight({50_000_000})] - pub fn change_key(origin: OriginFor, new: AccountIdLookupOf) -> DispatchResult { + #[pallet::weight(T::WeightInfo::change_key(T::MaxMembers::get()))] + pub fn change_key( + origin: OriginFor, + new: AccountIdLookupOf, + ) -> DispatchResultWithPostInfo { let remove = ensure_signed(origin)?; let new = T::Lookup::lookup(new)?; + let mut members_length = T::MaxMembers::get(); + if remove != new { let mut members = >::get(); + members_length = members.len() as u32; let location = members.binary_search(&remove).ok().ok_or(Error::::NotMember)?; let _ = members.binary_search(&new).err().ok_or(Error::::AlreadyMember)?; @@ -295,28 +310,29 @@ pub mod pallet { } Self::deposit_event(Event::KeyChanged); - Ok(()) + Ok(Some(T::WeightInfo::change_key(members_length)).into()) } /// Set the prime member. Must be a current member. /// /// May only be called from `T::PrimeOrigin`. #[pallet::call_index(5)] - #[pallet::weight({50_000_000})] - pub fn set_prime(origin: OriginFor, who: AccountIdLookupOf) -> DispatchResult { + #[pallet::weight(T::WeightInfo::set_prime(T::MaxMembers::get()))] + pub fn set_prime(origin: OriginFor, who: AccountIdLookupOf) -> DispatchResultWithPostInfo { T::PrimeOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; - Self::members().binary_search(&who).ok().ok_or(Error::::NotMember)?; + let members = Self::members(); + members.binary_search(&who).ok().ok_or(Error::::NotMember)?; Prime::::put(&who); T::MembershipChanged::set_prime(Some(who)); - Ok(()) + Ok(Some(T::WeightInfo::set_prime(members.len() as u32)).into()) } /// Remove the prime member if it exists. /// /// May only be called from `T::PrimeOrigin`. #[pallet::call_index(6)] - #[pallet::weight({50_000_000})] + #[pallet::weight(T::WeightInfo::clear_prime())] pub fn clear_prime(origin: OriginFor) -> DispatchResult { T::PrimeOrigin::ensure_origin(origin)?; Prime::::kill(); @@ -442,7 +458,7 @@ mod benchmark { } // er keep the prime common between incoming and outgoing to make sure it is rejigged. - reset_member { + reset_members { let m in 1 .. T::MaxMembers::get(); let members = (1..m+1).map(|i| account("member", i, SEED)).collect::>(); @@ -500,8 +516,7 @@ mod benchmark { } clear_prime { - let m in 1 .. T::MaxMembers::get(); - let members = (0..m).map(|i| account("member", i, SEED)).collect::>(); + let members = (0..T::MaxMembers::get()).map(|i| account("member", i, SEED)).collect::>(); let prime = members.last().cloned().unwrap(); set_members::(members, None); }: { diff --git a/substrate/frame/membership/src/migrations/v4.rs b/substrate/frame/membership/src/migrations/v4.rs index 9b80aca86847..5982fcef0a6c 100644 --- a/substrate/frame/membership/src/migrations/v4.rs +++ b/substrate/frame/membership/src/migrations/v4.rs @@ -47,7 +47,7 @@ pub fn migrate::on_chain_storage_version(); @@ -86,7 +86,7 @@ pub fn pre_migrate>(old_pallet_name: N, new_ log_migration("pre-migration", old_pallet_name, new_pallet_name); if new_pallet_name == old_pallet_name { - return + return; } let new_pallet_prefix = twox_128(new_pallet_name.as_bytes()); @@ -114,7 +114,7 @@ pub fn post_migrate>(old_pallet_name: N, new log_migration("post-migration", old_pallet_name, new_pallet_name); if new_pallet_name == old_pallet_name { - return + return; } // Assert that nothing remains at the old prefix. diff --git a/substrate/frame/membership/src/weights.rs b/substrate/frame/membership/src/weights.rs index 18ea7fcb315a..2d18848b89ab 100644 --- a/substrate/frame/membership/src/weights.rs +++ b/substrate/frame/membership/src/weights.rs @@ -55,10 +55,10 @@ pub trait WeightInfo { fn add_member(m: u32, ) -> Weight; fn remove_member(m: u32, ) -> Weight; fn swap_member(m: u32, ) -> Weight; - fn reset_member(m: u32, ) -> Weight; + fn reset_members(m: u32, ) -> Weight; fn change_key(m: u32, ) -> Weight; fn set_prime(m: u32, ) -> Weight; - fn clear_prime(m: u32, ) -> Weight; + fn clear_prime() -> Weight; } /// Weights for pallet_membership using the Substrate node and recommended hardware. @@ -142,7 +142,7 @@ impl WeightInfo for SubstrateWeight { /// Storage: TechnicalCommittee Prime (r:0 w:1) /// Proof Skipped: TechnicalCommittee Prime (max_values: Some(1), max_size: None, mode: Measured) /// The range of component `m` is `[1, 100]`. - fn reset_member(m: u32, ) -> Weight { + fn reset_members(m: u32, ) -> Weight { // Proof Size summary in bytes: // Measured: `312 + m * (64 ±0)` // Estimated: `4687 + m * (64 ±0)` @@ -200,15 +200,12 @@ impl WeightInfo for SubstrateWeight { /// Proof: TechnicalMembership Prime (max_values: Some(1), max_size: Some(32), added: 527, mode: MaxEncodedLen) /// Storage: TechnicalCommittee Prime (r:0 w:1) /// Proof Skipped: TechnicalCommittee Prime (max_values: Some(1), max_size: None, mode: Measured) - /// The range of component `m` is `[1, 100]`. - fn clear_prime(m: u32, ) -> Weight { + fn clear_prime() -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` // Minimum execution time: 3_373_000 picoseconds. Weight::from_parts(3_750_452, 0) - // Standard Error: 142 - .saturating_add(Weight::from_parts(505, 0).saturating_mul(m.into())) .saturating_add(T::DbWeight::get().writes(2_u64)) } } @@ -293,7 +290,7 @@ impl WeightInfo for () { /// Storage: TechnicalCommittee Prime (r:0 w:1) /// Proof Skipped: TechnicalCommittee Prime (max_values: Some(1), max_size: None, mode: Measured) /// The range of component `m` is `[1, 100]`. - fn reset_member(m: u32, ) -> Weight { + fn reset_members(m: u32, ) -> Weight { // Proof Size summary in bytes: // Measured: `312 + m * (64 ±0)` // Estimated: `4687 + m * (64 ±0)` @@ -351,15 +348,12 @@ impl WeightInfo for () { /// Proof: TechnicalMembership Prime (max_values: Some(1), max_size: Some(32), added: 527, mode: MaxEncodedLen) /// Storage: TechnicalCommittee Prime (r:0 w:1) /// Proof Skipped: TechnicalCommittee Prime (max_values: Some(1), max_size: None, mode: Measured) - /// The range of component `m` is `[1, 100]`. - fn clear_prime(m: u32, ) -> Weight { + fn clear_prime() -> Weight { // Proof Size summary in bytes: // Measured: `0` // Estimated: `0` // Minimum execution time: 3_373_000 picoseconds. Weight::from_parts(3_750_452, 0) - // Standard Error: 142 - .saturating_add(Weight::from_parts(505, 0).saturating_mul(m.into())) .saturating_add(RocksDbWeight::get().writes(2_u64)) } } From a437c953bd51aae7b0dfc8c809f1dc8ab073e560 Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Wed, 14 Feb 2024 15:54:46 +0100 Subject: [PATCH 2/4] Revert changes in the migration file --- substrate/frame/membership/src/migrations/v4.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/membership/src/migrations/v4.rs b/substrate/frame/membership/src/migrations/v4.rs index 5982fcef0a6c..9b80aca86847 100644 --- a/substrate/frame/membership/src/migrations/v4.rs +++ b/substrate/frame/membership/src/migrations/v4.rs @@ -47,7 +47,7 @@ pub fn migrate::on_chain_storage_version(); @@ -86,7 +86,7 @@ pub fn pre_migrate>(old_pallet_name: N, new_ log_migration("pre-migration", old_pallet_name, new_pallet_name); if new_pallet_name == old_pallet_name { - return; + return } let new_pallet_prefix = twox_128(new_pallet_name.as_bytes()); @@ -114,7 +114,7 @@ pub fn post_migrate>(old_pallet_name: N, new log_migration("post-migration", old_pallet_name, new_pallet_name); if new_pallet_name == old_pallet_name { - return; + return } // Assert that nothing remains at the old prefix. From 0f7399f8d5f0c7f85c6539c1da367c18734e6648 Mon Sep 17 00:00:00 2001 From: Dino Pacandi Date: Thu, 15 Feb 2024 13:39:28 +0100 Subject: [PATCH 3/4] early-exit --- substrate/frame/membership/src/lib.rs | 71 +++++++++++++++++---------- 1 file changed, 46 insertions(+), 25 deletions(-) diff --git a/substrate/frame/membership/src/lib.rs b/substrate/frame/membership/src/lib.rs index 6422fb1d1e0e..fca25c45e02a 100644 --- a/substrate/frame/membership/src/lib.rs +++ b/substrate/frame/membership/src/lib.rs @@ -284,29 +284,28 @@ pub mod pallet { let remove = ensure_signed(origin)?; let new = T::Lookup::lookup(new)?; - let mut members_length = T::MaxMembers::get(); - - if remove != new { - let mut members = >::get(); - members_length = members.len() as u32; - let location = - members.binary_search(&remove).ok().ok_or(Error::::NotMember)?; - let _ = members.binary_search(&new).err().ok_or(Error::::AlreadyMember)?; - members[location] = new.clone(); - members.sort(); - - >::put(&members); - - T::MembershipChanged::change_members_sorted( - &[new.clone()], - &[remove.clone()], - &members[..], - ); - - if Prime::::get() == Some(remove) { - Prime::::put(&new); - T::MembershipChanged::set_prime(Some(new)); - } + if remove == new { + return Ok(().into()); + } + + let mut members = >::get(); + let members_length = members.len() as u32; + let location = members.binary_search(&remove).ok().ok_or(Error::::NotMember)?; + let _ = members.binary_search(&new).err().ok_or(Error::::AlreadyMember)?; + members[location] = new.clone(); + members.sort(); + + >::put(&members); + + T::MembershipChanged::change_members_sorted( + &[new.clone()], + &[remove.clone()], + &members[..], + ); + + if Prime::::get() == Some(remove) { + Prime::::put(&new); + T::MembershipChanged::set_prime(Some(new)); } Self::deposit_event(Event::KeyChanged); @@ -318,7 +317,10 @@ pub mod pallet { /// May only be called from `T::PrimeOrigin`. #[pallet::call_index(5)] #[pallet::weight(T::WeightInfo::set_prime(T::MaxMembers::get()))] - pub fn set_prime(origin: OriginFor, who: AccountIdLookupOf) -> DispatchResultWithPostInfo { + pub fn set_prime( + origin: OriginFor, + who: AccountIdLookupOf, + ) -> DispatchResultWithPostInfo { T::PrimeOrigin::ensure_origin(origin)?; let who = T::Lookup::lookup(who)?; let members = Self::members(); @@ -546,7 +548,8 @@ mod tests { }; use frame_support::{ - assert_noop, assert_ok, derive_impl, ord_parameter_types, parameter_types, + assert_noop, assert_ok, assert_storage_noop, derive_impl, ord_parameter_types, + parameter_types, traits::{ConstU32, ConstU64, StorageVersion}, }; use frame_system::EnsureSignedBy; @@ -758,6 +761,17 @@ mod tests { }); } + #[test] + fn swap_member_with_identical_arguments_changes_nothing() { + new_test_ext().execute_with(|| { + assert_storage_noop!(assert_ok!(Membership::swap_member( + RuntimeOrigin::signed(3), + 10, + 10 + ))); + }); + } + #[test] fn change_key_works() { new_test_ext().execute_with(|| { @@ -787,6 +801,13 @@ mod tests { }); } + #[test] + fn change_key_with_same_caller_as_argument_changes_nothing() { + new_test_ext().execute_with(|| { + assert_storage_noop!(assert_ok!(Membership::change_key(RuntimeOrigin::signed(10), 10))); + }); + } + #[test] fn reset_members_works() { new_test_ext().execute_with(|| { From 5f4a9de949c036096d6e566a60379c9045388f30 Mon Sep 17 00:00:00 2001 From: Oliver Tale-Yazdi Date: Mon, 19 Feb 2024 19:00:08 +0100 Subject: [PATCH 4/4] Add prdoc Signed-off-by: Oliver Tale-Yazdi --- prdoc/pr_3324.prdoc | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 prdoc/pr_3324.prdoc diff --git a/prdoc/pr_3324.prdoc b/prdoc/pr_3324.prdoc new file mode 100644 index 000000000000..0425fbf317c8 --- /dev/null +++ b/prdoc/pr_3324.prdoc @@ -0,0 +1,13 @@ +# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 +# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json + +title: "Fix weight calculation and event emission in pallet-membership" + +doc: + - audience: Runtime Dev + description: | + Bug fix for the membership pallet to use correct weights. Also no event will be emitted + anymore when `change_key` is called with identical accounts. + +crates: + - name: pallet-membership