Proposal: Defensive trait for infallible frame operations

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,
@@ -1784,7 +1784,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,
@@ -1897,7 +1897,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
@@ -1930,8 +1930,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;
@@ -1977,8 +1977,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;
@@ -2037,12 +2037,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)
 									},
@@ -2057,7 +2057,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(
@@ -2080,7 +2080,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

@@ -157,7 +157,7 @@ use frame_support::{
 	ensure,
 	traits::{
 		schedule::{DispatchTime, Named as ScheduleNamed},
-		BalanceStatus, Currency, Get, LockIdentifier, LockableCurrency, OnUnbalanced,
+		BalanceStatus, Currency, Defensive, Get, LockIdentifier, LockableCurrency, OnUnbalanced,
 		ReservableCurrency, WithdrawReasons,
 	},
 	weights::Weight,
@@ -1629,7 +1629,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::{Currency, Defensive, 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/elections-phragmen/src/lib.rs

@@ -102,7 +102,7 @@ use codec::{Decode, Encode};
 use frame_support::{
 	dispatch::WithPostDispatchInfo,
 	traits::{
-		ChangeMembers, Contains, ContainsLengthBound, Currency, CurrencyToVote, Get,
+		ChangeMembers, Contains, ContainsLengthBound, Currency, CurrencyToVote, Defensive, Get,
 		InitializeMembers, LockIdentifier, LockableCurrency, OnUnbalanced, ReservableCurrency,
 		SortedMembers, StorageVersion, WithdrawReasons,
 	},
@@ -1027,7 +1027,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};
@@ -598,10 +598,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()
@@ -612,7 +614,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/support/src/traits.rs

@@ -51,11 +51,11 @@ pub use filter::{ClearFilterGuard, FilterStack, FilterStackGuard, InstanceFilter
 mod misc;
 pub use misc::{
 	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, Defensive, DefensiveSaturating, DefensiveWithProof,
+	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;

frame/support/src/traits/misc.rs

@@ -20,9 +20,187 @@
 use crate::dispatch::Parameter;
 use codec::{CompactLen, Decode, DecodeAll, Encode, EncodeLike, Input, MaxEncodedLen};
 use scale_info::{build::Fields, meta_type, Path, Type, TypeInfo, TypeParameter};
+use sp_arithmetic::traits::{Bounded, CheckedAdd, CheckedMul, CheckedSub};
 use sp_runtime::{traits::Block as BlockT, DispatchError};
 use sp_std::{cmp::Ordering, prelude::*};
 
+/// A trait to handle errors and options when you are really sure that a condition must hold, but
+/// not brave enough to `expect` on it, or a default fallback value makes more sense.
+///
+/// Each function in this trait will have two side effects, aside from behaving exactly as the name
+/// would suggest:
+///
+/// 1. It panics on `#[debug_assertions]`, so if the infallible code is reached in any of the tests,
+///    you realize.
+/// 2. It will log a warning using the runtime logging system. This might help you detect such bugs
+///    in production as well. Note that the log message, as of now, are not super expressive. Your
+///    best shot of fully diagnosing the error would be to infer the block number of which the log
+///    message was emitted, then re-execute that block using `check-block` or `try-runtime`
+///    subcommands in substrate client.
+pub trait Defensive<T> {
+	/// Exactly the same as `unwrap_or`, but it does the defensive warnings explained in the trait
+	/// docs.
+	fn defensive_unwrap_or(self, other: T) -> T;
+	/// Exactly the same as `unwrap_or_else`, but it does the defensive warnings explained in the
+	/// trait docs.
+	fn defensive_unwrap_or_else<F: FnOnce() -> T>(self, f: F) -> T;
+	/// Exactly the same as `unwrap_or_default`, but it does the defensive warnings explained in the
+	/// trait docs.
+	fn defensive_unwrap_or_default(self) -> T
+	where
+		T: Default;
+}
+
+/// Same as [`Defensive`], but it fixes the second issue (logs not being expressive) at the cost of
+/// having a more verbose API.
+///
+/// Each defensive operation must contain a proof as well, as it should, in principle, be
+/// infallible. The API of this trait expects this proof to be given as a string, which is in turn
+/// printed in the emitting logs and panic stack trace. This can help with easier diagnosis.
+pub trait DefensiveWithProof<T> {
+	/// Exactly the same as `unwrap_or`, but it does the defensive warnings explained in the trait
+	/// docs.
+	fn defensive_unwrap_or_proof(self, other: T, proof: &'static str) -> T;
+	/// Exactly the same as `unwrap_or_else`, but it does the defensive warnings explained in the
+	/// trait docs.
+	fn defensive_unwrap_or_else_proof<F: FnOnce() -> T>(self, f: F, proof: &'static str) -> T;
+	/// Exactly the same as `unwrap_or_default`, but it does the defensive warnings explained in the
+	/// trait docs.
+	fn defensive_unwrap_or_default_proof(self, proof: &'static str) -> T
+	where
+		T: Default;
+}
+
+/// A variant of [`Defensive`] with the same rationale, for the arithmetic operations where in case
+/// an infallible operation fails, it saturates.
+pub trait DefensiveSaturating {
+	/// Add `self` and `other` defensively.
+	fn defensive_saturating_add(self, other: Self) -> Self;
+	/// Subtract `other` from `self` defensively.
+	fn defensive_saturating_sub(self, other: Self) -> Self;
+	/// Multiply `self` and `other` defensively.
+	fn defensive_saturating_mul(self, other: Self) -> Self;
+}
+
+// NOTE: A bit unfortunate, since T has to be bound by all the traits needed. Could make it
+// `DefensiveSaturating<T>` to mitigate, but anyways okay for a draft PR.
+impl<T: CheckedAdd + CheckedMul + CheckedSub + Bounded> DefensiveSaturating for T {
+	fn defensive_saturating_add(self, other: Self) -> Self {
+		self.checked_add(&other).defensive_unwrap_or_else(Bounded::max_value)
+	}
+	fn defensive_saturating_mul(self, other: Self) -> Self {
+		self.checked_sub(&other).defensive_unwrap_or_else(Bounded::max_value)
+	}
+	fn defensive_saturating_sub(self, other: Self) -> Self {
+		self.checked_mul(&other).defensive_unwrap_or_else(Bounded::min_value)
+	}
+}
+
+const DEFENSIVE_UNWRAP_PUBLIC_ERROR: &'static str = "a defensive unwrap has been triggered; please report the block number at https://github.com/paritytech/substrate/issues";
+const DEFENSIVE_UNWRAP_INTERNAL_ERROR: &'static str = "Defensive unwrap has been triggered!";
+
+impl<T> Defensive<T> for Option<T> {
+	fn defensive_unwrap_or(self, or: T) -> T {
+		match self {
+			Some(inner) => inner,
+			None => {
+				debug_assert!(false, "{}", DEFENSIVE_UNWRAP_INTERNAL_ERROR);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}",
+					DEFENSIVE_UNWRAP_PUBLIC_ERROR
+				);
+				or
+			},
+		}
+	}
+
+	fn defensive_unwrap_or_else<F: FnOnce() -> T>(self, f: F) -> T {
+		match self {
+			Some(inner) => inner,
+			None => {
+				debug_assert!(false, "{}", DEFENSIVE_UNWRAP_INTERNAL_ERROR);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}",
+					DEFENSIVE_UNWRAP_PUBLIC_ERROR
+				);
+				f()
+			},
+		}
+	}
+
+	fn defensive_unwrap_or_default(self) -> T
+	where
+		T: Default,
+	{
+		match self {
+			Some(inner) => inner,
+			None => {
+				debug_assert!(false, "{}", DEFENSIVE_UNWRAP_INTERNAL_ERROR);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}",
+					DEFENSIVE_UNWRAP_PUBLIC_ERROR
+				);
+				Default::default()
+			},
+		}
+	}
+}
+impl<T, E: sp_std::fmt::Debug> Defensive<T> for Result<T, E> {
+	fn defensive_unwrap_or(self, or: T) -> T {
+		match self {
+			Ok(inner) => inner,
+			Err(e) => {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_UNWRAP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_UNWRAP_PUBLIC_ERROR,
+					e
+				);
+				or
+			},
+		}
+	}
+
+	fn defensive_unwrap_or_else<F: FnOnce() -> T>(self, f: F) -> T {
+		match self {
+			Ok(inner) => inner,
+			Err(e) => {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_UNWRAP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_UNWRAP_PUBLIC_ERROR,
+					e
+				);
+				f()
+			},
+		}
+	}
+
+	fn defensive_unwrap_or_default(self) -> T
+	where
+		T: Default,
+	{
+		match self {
+			Ok(inner) => inner,
+			Err(e) => {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_UNWRAP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_UNWRAP_PUBLIC_ERROR,
+					e
+				);
+				Default::default()
+			},
+		}
+	}
+}
+
 /// Try and collect into a collection `C`.
 pub trait TryCollect<C> {
 	type Error;