Proposal: Defensive trait for infallible frame operations (paritytech…

…#10626)

* add a blueprint of how defensive traits could look like

* add something for arithmetic as well

* add some use cases in different pallets

* some build fixing

* Some new stuff and examples

* Fix deadly bug

* add more doc.

* undo faulty change to assets pallet

* Update frame/support/src/traits/misc.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* some review comments

* remove draft comment

* Fix ident test

* Fix proxy tests as well

* a few new ideas

* Fix build

* Fix doc

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

frame/balances/src/lib.rs

@@ -170,7 +170,7 @@ use frame_support::{
 	pallet_prelude::DispatchResult,
 	traits::{
 		tokens::{fungible, BalanceStatus as Status, DepositConsequence, WithdrawConsequence},
-		Currency, ExistenceRequirement,
+		Currency, DefensiveSaturating, ExistenceRequirement,
 		ExistenceRequirement::{AllowDeath, KeepAlive},
 		Get, Imbalance, LockIdentifier, LockableCurrency, NamedReservableCurrency, OnUnbalanced,
 		ReservableCurrency, SignedImbalance, StoredMap, TryDrop, WithdrawReasons,
@@ -1783,7 +1783,7 @@ where
 			account.reserved -= actual;
 			// defensive only: this can never fail since total issuance which is at least
 			// free+reserved fits into the same data type.
-			account.free = account.free.saturating_add(actual);
+			account.free = account.free.defensive_saturating_add(actual);
 			actual
 		}) {
 			Ok(x) => x,
@@ -1896,7 +1896,7 @@ where
 			match reserves.binary_search_by_key(id, |data| data.id) {
 				Ok(index) => {
 					// this add can't overflow but just to be defensive.
-					reserves[index].amount = reserves[index].amount.saturating_add(value);
+					reserves[index].amount = reserves[index].amount.defensive_saturating_add(value);
 				},
 				Err(index) => {
 					reserves
@@ -1929,8 +1929,8 @@ where
 
 						let remain = <Self as ReservableCurrency<_>>::unreserve(who, to_change);
 
-						// remain should always be zero but just to be defensive here
-						let actual = to_change.saturating_sub(remain);
+						// remain should always be zero but just to be defensive here.
+						let actual = to_change.defensive_saturating_sub(remain);
 
 						// `actual <= to_change` and `to_change <= amount`; qed;
 						reserves[index].amount -= actual;
@@ -1976,8 +1976,8 @@ where
 					let (imb, remain) =
 						<Self as ReservableCurrency<_>>::slash_reserved(who, to_change);
 
-					// remain should always be zero but just to be defensive here
-					let actual = to_change.saturating_sub(remain);
+					// remain should always be zero but just to be defensive here.
+					let actual = to_change.defensive_saturating_sub(remain);
 
 					// `actual <= to_change` and `to_change <= amount`; qed;
 					reserves[index].amount -= actual;
@@ -2036,12 +2036,12 @@ where
 											)?;
 
 										// remain should always be zero but just to be defensive
-										// here
-										let actual = to_change.saturating_sub(remain);
+										// here.
+										let actual = to_change.defensive_saturating_sub(remain);
 
 										// this add can't overflow but just to be defensive.
 										reserves[index].amount =
-											reserves[index].amount.saturating_add(actual);
+											reserves[index].amount.defensive_saturating_add(actual);
 
 										Ok(actual)
 									},
@@ -2056,7 +2056,7 @@ where
 
 										// remain should always be zero but just to be defensive
 										// here
-										let actual = to_change.saturating_sub(remain);
+										let actual = to_change.defensive_saturating_sub(remain);
 
 										reserves
 											.try_insert(
@@ -2079,7 +2079,7 @@ where
 						)?;
 
 						// remain should always be zero but just to be defensive here
-						to_change.saturating_sub(remain)
+						to_change.defensive_saturating_sub(remain)
 					};
 
 					// `actual <= to_change` and `to_change <= amount`; qed;

frame/democracy/src/lib.rs

@@ -156,6 +156,7 @@ use codec::{Decode, Encode, Input};
 use frame_support::{
 	ensure,
 	traits::{
+		defensive_prelude::*,
 		schedule::{DispatchTime, Named as ScheduleNamed},
 		BalanceStatus, Currency, Get, LockIdentifier, LockableCurrency, OnUnbalanced,
 		ReservableCurrency, WithdrawReasons,
@@ -1630,7 +1631,7 @@ impl<T: Config> Pallet<T> {
 		let mut public_props = Self::public_props();
 		if let Some((winner_index, _)) = public_props.iter().enumerate().max_by_key(
 			// defensive only: All current public proposals have an amount locked
-			|x| Self::backing_for((x.1).0).unwrap_or_else(Zero::zero),
+			|x| Self::backing_for((x.1).0).defensive_unwrap_or_else(Zero::zero),
 		) {
 			let (prop_index, proposal, _) = public_props.swap_remove(winner_index);
 			<PublicProps<T>>::put(public_props);

frame/election-provider-multi-phase/src/signed.rs

@@ -25,7 +25,7 @@ use crate::{
 use codec::{Decode, Encode, HasCompact};
 use frame_support::{
 	storage::bounded_btree_map::BoundedBTreeMap,
-	traits::{Currency, Get, OnUnbalanced, ReservableCurrency},
+	traits::{defensive_prelude::*, Currency, Get, OnUnbalanced, ReservableCurrency},
 };
 use sp_arithmetic::traits::SaturatedConversion;
 use sp_npos_elections::{is_score_better, ElectionScore, NposSolution};
@@ -365,7 +365,7 @@ impl<T: Config> Pallet<T> {
 			let active_voters = raw_solution.solution.voter_count() as u32;
 			let feasibility_weight = {
 				// defensive only: at the end of signed phase, snapshot will exits.
-				let desired_targets = Self::desired_targets().unwrap_or_default();
+				let desired_targets = Self::desired_targets().defensive_unwrap_or_default();
 				T::WeightInfo::feasibility_check(voters, targets, active_voters, desired_targets)
 			};
 			// the feasibility check itself has some weight

frame/election-provider-support/src/lib.rs

@@ -343,7 +343,7 @@ impl<AccountId, BlockNumber> ElectionProvider for NoElection<(AccountId, BlockNu
 /// used on the implementing side of [`ElectionDataProvider`].
 pub trait SortedListProvider<AccountId> {
 	/// The list's error type.
-	type Error;
+	type Error: sp_std::fmt::Debug;
 
 	/// An iterator over the list, which can have `take` called on it.
 	fn iter() -> Box<dyn Iterator<Item = AccountId>>;

frame/elections-phragmen/src/lib.rs

@@ -102,9 +102,9 @@ use codec::{Decode, Encode};
 use frame_support::{
 	dispatch::WithPostDispatchInfo,
 	traits::{
-		ChangeMembers, Contains, ContainsLengthBound, Currency, CurrencyToVote, Get,
-		InitializeMembers, LockIdentifier, LockableCurrency, OnUnbalanced, ReservableCurrency,
-		SortedMembers, StorageVersion, WithdrawReasons,
+		defensive_prelude::*, ChangeMembers, Contains, ContainsLengthBound, Currency,
+		CurrencyToVote, Get, InitializeMembers, LockIdentifier, LockableCurrency, OnUnbalanced,
+		ReservableCurrency, SortedMembers, StorageVersion, WithdrawReasons,
 	},
 	weights::Weight,
 };
@@ -1028,7 +1028,7 @@ impl<T: Config> Pallet<T> {
 				candidates_and_deposit
 					.iter()
 					.find_map(|(c, d)| if c == x { Some(*d) } else { None })
-					.unwrap_or_default()
+					.defensive_unwrap_or_default()
 			};
 			// fetch deposits from the one recorded one. This will make sure that a candidate who
 			// submitted candidacy before a change to candidacy deposit will have the correct amount

frame/gilt/src/lib.rs

@@ -79,7 +79,7 @@ pub mod pallet {
 	pub use crate::weights::WeightInfo;
 	use frame_support::{
 		pallet_prelude::*,
-		traits::{Currency, OnUnbalanced, ReservableCurrency},
+		traits::{Currency, DefensiveSaturating, OnUnbalanced, ReservableCurrency},
 	};
 	use frame_system::pallet_prelude::*;
 	use sp_arithmetic::{PerThing, Perquintill};
@@ -599,10 +599,12 @@ pub mod pallet {
 								remaining -= amount;
 								// Should never underflow since it should track the total of the
 								// bids exactly, but we'll be defensive.
-								qs[queue_index].1 = qs[queue_index].1.saturating_sub(bid.amount);
+								qs[queue_index].1 =
+									qs[queue_index].1.defensive_saturating_sub(bid.amount);
 
 								// Now to activate the bid...
-								let nongilt_issuance = total_issuance.saturating_sub(totals.frozen);
+								let nongilt_issuance =
+									total_issuance.defensive_saturating_sub(totals.frozen);
 								let effective_issuance = totals
 									.proportion
 									.left_from_one()
@@ -613,7 +615,8 @@ pub mod pallet {
 								let who = bid.who;
 								let index = totals.index;
 								totals.frozen += bid.amount;
-								totals.proportion = totals.proportion.saturating_add(proportion);
+								totals.proportion =
+									totals.proportion.defensive_saturating_add(proportion);
 								totals.index += 1;
 								let e =
 									Event::GiltIssued { index, expiry, who: who.clone(), amount };

frame/identity/src/benchmarking.rs

@@ -75,7 +75,7 @@ fn create_sub_accounts<T: Config>(
 	}
 
 	// Set identity so `set_subs` does not fail.
-	let _ = T::Currency::make_free_balance_be(&who, BalanceOf::<T>::max_value());
+	let _ = T::Currency::make_free_balance_be(&who, BalanceOf::<T>::max_value() / 2u32.into());
 	let info = create_identity_info::<T>(1);
 	Identity::<T>::set_identity(who_origin.clone().into(), Box::new(info))?;
 

frame/proxy/src/benchmarking.rs

@@ -33,7 +33,7 @@ fn assert_last_event<T: Config>(generic_event: <T as Config>::Event) {
 
 fn add_proxies<T: Config>(n: u32, maybe_who: Option<T::AccountId>) -> Result<(), &'static str> {
 	let caller = maybe_who.unwrap_or_else(|| whitelisted_caller());
-	T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+	T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value() / 2u32.into());
 	for i in 0..n {
 		Proxy::<T>::add_proxy(
 			RawOrigin::Signed(caller.clone()).into(),
@@ -51,12 +51,12 @@ fn add_announcements<T: Config>(
 	maybe_real: Option<T::AccountId>,
 ) -> Result<(), &'static str> {
 	let caller = maybe_who.unwrap_or_else(|| account("caller", 0, SEED));
-	T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+	T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value() / 2u32.into());
 	let real = if let Some(real) = maybe_real {
 		real
 	} else {
 		let real = account("real", 0, SEED);
-		T::Currency::make_free_balance_be(&real, BalanceOf::<T>::max_value());
+		T::Currency::make_free_balance_be(&real, BalanceOf::<T>::max_value() / 2u32.into());
 		Proxy::<T>::add_proxy(
 			RawOrigin::Signed(real.clone()).into(),
 			caller.clone(),
@@ -80,7 +80,7 @@ benchmarks! {
 		let p in 1 .. (T::MaxProxies::get() - 1).into() => add_proxies::<T>(p, None)?;
 		// In this case the caller is the "target" proxy
 		let caller: T::AccountId = account("target", p - 1, SEED);
-		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value() / 2u32.into());
 		// ... and "real" is the traditional caller. This is not a typo.
 		let real: T::AccountId = whitelisted_caller();
 		let call: <T as Config>::Call = frame_system::Call::<T>::remark { remark: vec![] }.into();
@@ -95,7 +95,7 @@ benchmarks! {
 		// In this case the caller is the "target" proxy
 		let caller: T::AccountId = account("anonymous", 0, SEED);
 		let delegate: T::AccountId = account("target", p - 1, SEED);
-		T::Currency::make_free_balance_be(&delegate, BalanceOf::<T>::max_value());
+		T::Currency::make_free_balance_be(&delegate, BalanceOf::<T>::max_value() / 2u32.into());
 		// ... and "real" is the traditional caller. This is not a typo.
 		let real: T::AccountId = whitelisted_caller();
 		let call: <T as Config>::Call = frame_system::Call::<T>::remark { remark: vec![] }.into();
@@ -115,7 +115,7 @@ benchmarks! {
 		let p in 1 .. (T::MaxProxies::get() - 1).into() => add_proxies::<T>(p, None)?;
 		// In this case the caller is the "target" proxy
 		let caller: T::AccountId = account("target", p - 1, SEED);
-		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value() / 2u32.into());
 		// ... and "real" is the traditional caller. This is not a typo.
 		let real: T::AccountId = whitelisted_caller();
 		let call: <T as Config>::Call = frame_system::Call::<T>::remark { remark: vec![] }.into();
@@ -136,7 +136,7 @@ benchmarks! {
 		let p in 1 .. (T::MaxProxies::get() - 1).into() => add_proxies::<T>(p, None)?;
 		// In this case the caller is the "target" proxy
 		let caller: T::AccountId = account("target", p - 1, SEED);
-		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value() / 2u32.into());
 		// ... and "real" is the traditional caller. This is not a typo.
 		let real: T::AccountId = whitelisted_caller();
 		let call: <T as Config>::Call = frame_system::Call::<T>::remark { remark: vec![] }.into();
@@ -157,7 +157,7 @@ benchmarks! {
 		let p in 1 .. (T::MaxProxies::get() - 1).into() => add_proxies::<T>(p, None)?;
 		// In this case the caller is the "target" proxy
 		let caller: T::AccountId = account("target", p - 1, SEED);
-		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value() / 2u32.into());
 		// ... and "real" is the traditional caller. This is not a typo.
 		let real: T::AccountId = whitelisted_caller();
 		add_announcements::<T>(a, Some(caller.clone()), None)?;

frame/staking/src/pallet/impls.rs

@@ -24,8 +24,8 @@ use frame_election_provider_support::{
 use frame_support::{
 	pallet_prelude::*,
 	traits::{
-		Currency, CurrencyToVote, EstimateNextNewSession, Get, Imbalance, LockableCurrency,
-		OnUnbalanced, UnixTime, WithdrawReasons,
+		Currency, CurrencyToVote, Defensive, EstimateNextNewSession, Get, Imbalance,
+		LockableCurrency, OnUnbalanced, UnixTime, WithdrawReasons,
 	},
 	weights::{Weight, WithPostDispatchInfo},
 };
@@ -775,10 +775,8 @@ impl<T: Config> Pallet<T> {
 	pub fn do_add_nominator(who: &T::AccountId, nominations: Nominations<T::AccountId>) {
 		if !Nominators::<T>::contains_key(who) {
 			// maybe update sorted list. Error checking is defensive-only - this should never fail.
-			if T::SortedListProvider::on_insert(who.clone(), Self::weight_of(who)).is_err() {
-				log!(warn, "attempt to insert duplicate nominator ({:#?})", who);
-				debug_assert!(false, "attempt to insert duplicate nominator");
-			};
+			let _ = T::SortedListProvider::on_insert(who.clone(), Self::weight_of(who))
+				.defensive_unwrap_or_default();
 
 			debug_assert_eq!(T::SortedListProvider::sanity_check(), Ok(()));
 		}

frame/support/src/traits.rs

@@ -50,12 +50,13 @@ pub use filter::{ClearFilterGuard, FilterStack, FilterStackGuard, InstanceFilter
 
 mod misc;
 pub use misc::{
+	defensive_prelude::{self, *},
 	Backing, ConstBool, ConstI128, ConstI16, ConstI32, ConstI64, ConstI8, ConstU128, ConstU16,
-	ConstU32, ConstU64, ConstU8, EnsureInherentsAreFirst, EqualPrivilegeOnly, EstimateCallFee,
-	ExecuteBlock, ExtrinsicCall, Get, GetBacking, GetDefault, HandleLifetime, IsSubType, IsType,
-	Len, OffchainWorker, OnKilledAccount, OnNewAccount, PreimageProvider, PreimageRecipient,
-	PrivilegeCmp, SameOrOther, Time, TryCollect, TryDrop, UnixTime, WrapperKeepOpaque,
-	WrapperOpaque,
+	ConstU32, ConstU64, ConstU8, DefensiveSaturating, EnsureInherentsAreFirst, EqualPrivilegeOnly,
+	EstimateCallFee, ExecuteBlock, ExtrinsicCall, Get, GetBacking, GetDefault, HandleLifetime,
+	IsSubType, IsType, Len, OffchainWorker, OnKilledAccount, OnNewAccount, PreimageProvider,
+	PreimageRecipient, PrivilegeCmp, SameOrOther, Time, TryCollect, TryDrop, UnixTime,
+	WrapperKeepOpaque, WrapperOpaque,
 };
 
 mod stored_map;