WIP Make ChangeMembers trait fallible

bin/node/runtime/src/lib.rs

@@ -90,7 +90,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	// and set impl_version to 0. If only runtime
 	// implementation changes and behavior does not, then leave spec_version as
 	// is and increment impl_version.
-	spec_version: 247,
+	spec_version: 248,
 	impl_version: 0,
 	apis: RUNTIME_API_VERSIONS,
 	transaction_version: 1,

frame/collective/src/lib.rs

@@ -383,7 +383,7 @@ impl<T: Trait<I>, I: Instance> ChangeMembers<T::AccountId> for Module<T, I> {
 		_incoming: &[T::AccountId],
 		outgoing: &[T::AccountId],
 		new: &[T::AccountId],
-	) {
+	) -> usize {
 		// remove accounts from all current voting in motions.
 		let mut outgoing = outgoing.to_vec();
 		outgoing.sort_unstable();
@@ -402,6 +402,7 @@ impl<T: Trait<I>, I: Instance> ChangeMembers<T::AccountId> for Module<T, I> {
 		}
 		Members::<T, I>::put(new);
 		Prime::<T, I>::kill();
+		new.len()
 	}
 
 	fn set_prime(prime: Option<T::AccountId>) {
@@ -523,7 +524,7 @@ impl<
 #[cfg(test)]
 mod tests {
 	use super::*;
-	use frame_support::{Hashable, assert_ok, assert_noop, parameter_types, weights::Weight};
+	use frame_support::{Hashable, assert_err, assert_ok, assert_noop, parameter_types, weights::Weight};
 	use frame_system::{self as system, EventRecord, Phase};
 	use hex_literal::hex;
 	use sp_core::H256;

frame/elections-phragmen/src/lib.rs

@@ -549,10 +549,13 @@ impl<T: Trait> Module<T> {
 			let members = members_with_stake.into_iter().map(|m| m.0).collect::<Vec<_>>();
 			let result = Ok(maybe_replacement.is_some());
 			let old = [who.clone()];
-			match maybe_replacement {
+			// We don't need to check the actual new members length because it will be the same
+			// as before or one less.
+			// TODO: Is this safe without ensure_can_change_members?
+			let _ = match maybe_replacement {
 				Some(new) => T::ChangeMembers::change_members_sorted(&[new], &old, &members),
 				None => T::ChangeMembers::change_members_sorted(&[], &old, &members),
-			}
+			};
 			result
 		} else {
 			Err(Error::<T>::NotMember)?
@@ -973,7 +976,7 @@ mod tests {
 
 	pub struct TestChangeMembers;
 	impl ChangeMembers<u64> for TestChangeMembers {
-		fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) {
+		fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) -> usize {
 			// new, incoming, outgoing must be sorted.
 			let mut new_sorted = new.to_vec();
 			new_sorted.sort();
@@ -1004,6 +1007,8 @@ mod tests {
 
 			MEMBERS.with(|m| *m.borrow_mut() = new.to_vec());
 			PRIME.with(|p| *p.borrow_mut() = None);
+
+			new.len()
 		}
 
 		fn set_prime(who: Option<u64>) {

frame/elections/src/mock.rs

@@ -111,7 +111,7 @@ impl Get<u32> for DecayRatio {
 
 pub struct TestChangeMembers;
 impl ChangeMembers<u64> for TestChangeMembers {
-	fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) {
+	fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) -> usize {
 		let mut old_plus_incoming = MEMBERS.with(|m| m.borrow().to_vec());
 		old_plus_incoming.extend_from_slice(incoming);
 		old_plus_incoming.sort();
@@ -121,6 +121,7 @@ impl ChangeMembers<u64> for TestChangeMembers {
 		assert_eq!(old_plus_incoming, new_plus_outgoing);
 
 		MEMBERS.with(|m| *m.borrow_mut() = new.to_vec());
+		new.len()
 	}
 }
 

frame/membership/src/lib.rs

@@ -125,10 +125,13 @@ decl_module! {
 
 			let mut members = <Members<T, I>>::get();
 			let location = members.binary_search(&who).err().ok_or(Error::<T, I>::AlreadyMember)?;
+			let old = members.clone();
 			members.insert(location, who.clone());
+			T::MembershipChanged::ensure_can_change_members(&members, &old)?;
 			<Members<T, I>>::put(&members);
 
-			T::MembershipChanged::change_members_sorted(&[who], &[], &members[..]);
+			// Safe to ignore the length of the new members here because of `ensure_can_change_members`.
+			let _ = T::MembershipChanged::change_members_sorted(&[who], &[], &members[..]);
 
 			Self::deposit_event(RawEvent::MemberAdded);
 		}
@@ -340,7 +343,7 @@ mod tests {
 
 	pub struct TestChangeMembers;
 	impl ChangeMembers<u64> for TestChangeMembers {
-		fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) {
+		fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) -> usize {
 			let mut old_plus_incoming = MEMBERS.with(|m| m.borrow().to_vec());
 			old_plus_incoming.extend_from_slice(incoming);
 			old_plus_incoming.sort();
@@ -351,7 +354,9 @@ mod tests {
 
 			MEMBERS.with(|m| *m.borrow_mut() = new.to_vec());
 			PRIME.with(|p| *p.borrow_mut() = None);
+			new.len()
 		}
+
 		fn set_prime(who: Option<u64>) {
 			PRIME.with(|p| *p.borrow_mut() = who);
 		}

frame/scored-pool/src/lib.rs

@@ -390,7 +390,7 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 	/// Fetches the `MemberCount` highest scoring members from
 	/// `Pool` and puts them into `Members`.
 	///
-	/// The `notify` parameter is used to deduct which associated
+	/// The `notify` parameter is used to decide which associated
 	/// type function to invoke at the end of the method.
 	fn refresh_members(
 		pool: PoolT<T, I>,
@@ -407,16 +407,20 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 		new_members.sort();
 
 		let old_members = <Members<T, I>>::get();
+		// TODO: What to do in case of error?
+		let _ = T::MembershipChanged::ensure_can_change_members(&new_members, &old_members);
 		<Members<T, I>>::put(&new_members);
 
 		match notify {
 			ChangeReceiver::MembershipInitialized =>
 				T::MembershipInitialized::initialize_members(&new_members),
-			ChangeReceiver::MembershipChanged =>
-				T::MembershipChanged::set_members_sorted(
+			ChangeReceiver::MembershipChanged => {
+				// TODO: Is this safe?
+				let _ = T::MembershipChanged::set_members_sorted(
 					&new_members[..],
 					&old_members[..],
-				),
+				);
+			},
 		}
 	}
 

frame/scored-pool/src/mock.rs

@@ -92,7 +92,7 @@ thread_local! {
 
 pub struct TestChangeMembers;
 impl ChangeMembers<u64> for TestChangeMembers {
-	fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) {
+	fn change_members_sorted(incoming: &[u64], outgoing: &[u64], new: &[u64]) -> usize {
 		let mut old_plus_incoming = MEMBERS.with(|m| m.borrow().to_vec());
 		old_plus_incoming.extend_from_slice(incoming);
 		old_plus_incoming.sort();
@@ -104,6 +104,7 @@ impl ChangeMembers<u64> for TestChangeMembers {
 		assert_eq!(old_plus_incoming, new_plus_outgoing);
 
 		MEMBERS.with(|m| *m.borrow_mut() = new.to_vec());
+		new.len()
 	}
 }
 

frame/support/src/traits.rs

@@ -934,36 +934,68 @@ impl WithdrawReasons {
 	}
 }
 
-/// Trait for type that can handle incremental changes to a set of account IDs.
-pub trait ChangeMembers<AccountId: Clone + Ord> {
+/// Trait for types that can handle incremental changes to a set of account IDs.
+///
+/// Allows member changes according to the "verify first, write last" maxime by
+/// providing the `ensure_can_change_members` function for checking as well as the
+/// actual writing logic.
+pub trait ChangeMembers<AccountId: Clone + Ord, MembersCount = usize> {
+
+	/// Returns whether changing the members will execute without errors.
+	///
+	/// Defaults to a noop. Implement this if `change_members_sorted` could fail.
+	///
+	/// If this returns `Err`, calls to `change_members`, `change_members_sorted` and 
+	/// `set_members_sorted` are not guaranteed to have the desired effect.
+	fn ensure_can_change_members(_new: &[AccountId], _old: &[AccountId]) -> DispatchResult {
+		Ok(())
+	}
+
 	/// A number of members `incoming` just joined the set and replaced some `outgoing` ones. The
 	/// new set is given by `new`, and need not be sorted.
 	///
 	/// This resets any previous value of prime.
-	fn change_members(incoming: &[AccountId], outgoing: &[AccountId], mut new: Vec<AccountId>) {
+	///
+	/// Returns the actual number of members set. This may differ from `new.len()`
+	/// if there are errors. Use `ensure_can_change_members` to check for those beforehand.
+	fn change_members(
+		incoming: &[AccountId],
+		outgoing: &[AccountId],
+		mut new: Vec<AccountId>
+	) -> MembersCount {
 		new.sort_unstable();
-		Self::change_members_sorted(incoming, outgoing, &new[..]);
+		Self::change_members_sorted(incoming, outgoing, &new[..])
 	}
 
 	/// A number of members `_incoming` just joined the set and replaced some `_outgoing` ones. The
 	/// new set is thus given by `sorted_new` and **must be sorted**.
 	///
-	/// NOTE: This is the only function that needs to be implemented in `ChangeMembers`.
+	/// NOTE: This is the only function that needs to be implemented in `ChangeMembers`, but make
+	///       sure that `ensure_can_change_members` is implemented correctly.
 	///
 	/// This resets any previous value of prime.
+	///
+	/// Returns the actual number of members set. This may differ from `sorted_new.len()`
+	/// if there are errors. Use `ensure_can_change_members` to check for those beforehand.
 	fn change_members_sorted(
 		incoming: &[AccountId],
 		outgoing: &[AccountId],
 		sorted_new: &[AccountId],
-	);
+	) -> MembersCount;
 
 	/// Set the new members; they **must already be sorted**. This will compute the diff and use it to
 	/// call `change_members_sorted`.
 	///
 	/// This resets any previous value of prime.
-	fn set_members_sorted(new_members: &[AccountId], old_members: &[AccountId]) {
+	///
+	/// Returns the actual number of members set. This may differ from `new_members.len()`
+	/// if there are errors. Use `ensure_can_change_members` to check for those beforehand.
+	fn set_members_sorted(
+		new_members: &[AccountId],
+		old_members: &[AccountId]
+	) -> MembersCount {
 		let (incoming, outgoing) = Self::compute_members_diff(new_members, old_members);
-		Self::change_members_sorted(&incoming[..], &outgoing[..], &new_members);
+		Self::change_members_sorted(&incoming[..], &outgoing[..], &new_members)
 	}
 
 	/// Set the new members; they **must already be sorted**. This will compute the diff and use it to
@@ -1003,13 +1035,15 @@ pub trait ChangeMembers<AccountId: Clone + Ord> {
 	}
 
 	/// Set the prime member.
+	///
+	/// Defaults to a noop.
 	fn set_prime(_prime: Option<AccountId>) {}
 }
 
 impl<T: Clone + Ord> ChangeMembers<T> for () {
-	fn change_members(_: &[T], _: &[T], _: Vec<T>) {}
-	fn change_members_sorted(_: &[T], _: &[T], _: &[T]) {}
-	fn set_members_sorted(_: &[T], _: &[T]) {}
+	fn change_members(_: &[T], _: &[T], new: Vec<T>) -> usize { new.len() }
+	fn change_members_sorted(_: &[T], _: &[T], new: &[T]) -> usize { new.len() }
+	fn set_members_sorted(new: &[T], _: &[T]) -> usize { new.len() }
 	fn set_prime(_: Option<T>) {}
 }