WIP: improve error message.

... for ChannelError and APIMisuseError
Before this commit, When rl returns error, we don't know
The actual parameter which caused the error.
By returning parameterised `String` instead of predefined `&'static str`,
We can give a caller improved error message.

Author: joemphilips

lightning/src/ln/channel.rs

@@ -21,6 +21,7 @@ use ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdate, ChannelMonitorUpd
 use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MAX_LOCAL_BREAKDOWN_TIMEOUT};
 use ln::chan_utils::{CounterpartyCommitmentSecrets, LocalCommitmentTransaction, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys};
 use ln::chan_utils;
+use ln::checker::{predicates, check_or_close, checker_or_api_misuse, check_or_ignore};
 use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
 use chain::transaction::OutPoint;
 use chain::keysinterface::{ChannelKeys, KeysInterface};
@@ -409,17 +410,17 @@ pub const MAX_FUNDING_SATOSHIS: u64 = 1 << 24;
 /// msgs::ErrorAction::SendErrorMessage or msgs::ErrorAction::IgnoreError as appropriate with our
 /// channel_id in ChannelManager.
 pub(super) enum ChannelError {
-	Ignore(&'static str),
-	Close(&'static str),
-	CloseDelayBroadcast(&'static str),
+	Ignore(String),
+	Close(String),
+	CloseDelayBroadcast(String),
 }
 
 impl fmt::Debug for ChannelError {
 	fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
 		match self {
-			&ChannelError::Ignore(e) => write!(f, "Ignore : {}", e),
-			&ChannelError::Close(e) => write!(f, "Close : {}", e),
-			&ChannelError::CloseDelayBroadcast(e) => write!(f, "CloseDelayBroadcast : {}", e)
+			&ChannelError::Ignore(ref e) => write!(f, "Ignore : {}", e),
+			&ChannelError::Close(ref e) => write!(f, "Close : {}", e),
+			&ChannelError::CloseDelayBroadcast(ref e) => write!(f, "CloseDelayBroadcast : {}", e)
 		}
 	}
 }
@@ -433,6 +434,7 @@ macro_rules! secp_check {
 	};
 }
 
+
 impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	// Convert constants + channel value to limits:
 	fn get_our_max_htlc_value_in_flight_msat(channel_value_satoshis: u64) -> u64 {
@@ -459,17 +461,9 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	{
 		let chan_keys = keys_provider.get_channel_keys(false, channel_value_satoshis);
 
-		if channel_value_satoshis >= MAX_FUNDING_SATOSHIS {
-			return Err(APIError::APIMisuseError{err: "funding value > 2^24"});
-		}
-
-		if push_msat > channel_value_satoshis * 1000 {
-			return Err(APIError::APIMisuseError{err: "push value > channel value"});
-		}
-		if config.own_channel_config.our_to_self_delay < BREAKDOWN_TIMEOUT {
-			return Err(APIError::APIMisuseError{err: "Configured with an unreasonable our_to_self_delay putting user funds at risks"});
-		}
-
+		check_or_api_misuse(channel_value_satoshis, MAX_FUNDING_SATOSHIS, predicates::lt, "Funding value must be smaller than {expected}. it was {actual}")?;
+		check_or_api_misuse(push_msat, channel_value_satoshis * 1000, predicates::ge, "push value must be greater than or equal to {expected}. it was {actual}")?;
+		check_or_api_misuse(config.own_channel_config.our_to_self_delay, BREAKDOWN_TIMEOUT, predicates::ge, "our configs, our_to_self_delay must be greater than or equal to {expected}. it was {actual}")?;
 
 		let background_feerate = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
 		if Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(channel_value_satoshis) < Channel::<ChanSigner>::derive_our_dust_limit_satoshis(background_feerate) {
@@ -558,12 +552,10 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	fn check_remote_fee<F: Deref>(fee_estimator: &F, feerate_per_kw: u32) -> Result<(), ChannelError>
 		where F::Target: FeeEstimator
 	{
-		if feerate_per_kw < fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background) {
-			return Err(ChannelError::Close("Peer's feerate much too low"));
-		}
-		if feerate_per_kw as u64 > fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64  * 2 {
-			return Err(ChannelError::Close("Peer's feerate much too high"));
-		}
+        let lower_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::Background);
+		check_or_close(feerate_per_kw, lower_limit, predicates::ge, "Peer's feerate much too low. actual: ({actual}). expected lower limit({lower_limit})")?;
+		let upper_limit = fee_estimator.get_est_sat_per_1000_weight(ConfirmationTarget::HighPriority) as u64  * 2;
+		check_or_close(feerate_per_kw, upper_limit, predicates::le, "Peer's feerate much too high. actual: ({actual}). expected upper limit({lower_limit})")?;
 		Ok(())
 	}
 

lightning/src/ln/checker.rs

@@ -0,0 +1,53 @@
+
+pub fn check_or_error<T, Q, F>(actual: T, expected: Q, predicate: F, msg: &str) -> Result<(), String>
+    where
+        F: Fn(&T, &Q) -> bool
+{
+    if predicate(&actual, &expected) {
+        Ok(())
+    } else {
+        let astr = format!("{}", actual);
+        let bstr =  format!("{}", expected);
+        let final_msg = msg.replace("{actual}", &astr[..]).replace("{expected}", &bstr[..]);
+        Err(final_msg)
+    }
+}
+
+pub(crate) fn check_or_close<T, Q, F>(actual: T, expected: Q, predicate: F, msg: &str) -> Result<(), ChannelError>
+    where
+        F: Fn(&T, &Q) -> bool
+{
+    check_or_error(actual, expected, predicate, msg).map_err(|err| { ChannelError::Close(err) })
+}
+
+pub(crate) fn check_or_ignore<T, Q, F>(actual: T, expected: Q, predicate: F, msg: &str) -> Result<(), ChannelError>
+    where
+        F: Fn(&T, &Q) -> bool
+{
+    check_or_error(actual, expected, predicate, msg).map_err(|err| { ChannelError::Ignore(err) })
+}
+
+pub(crate) fn check_or_api_misuse<T, Q, F>(actual: T, expected: Q, predicate: F, msg: &str) -> Result<(), ChannelError>
+    where
+        F: Fn(&T, &Q) -> bool
+{
+    check_or_error(actual, expected, predicate, msg).map_err(|err| { APIMisuseError{ err } })
+}
+
+pub(crate) mod predicates {
+    pub(crate) fn lt<T>(a: T, expected: T) -> bool
+        where T: std::cmp::Ord
+    { a < expected }
+
+    pub(crate) fn le<T>(a: T, expected: T) -> bool
+        where T: std::cmp::Ord
+    { a <= expected }
+
+    pub(crate) fn gt<T>(a: T, expected: T) -> bool
+        where T: std::cmp::Ord
+    { a > expected }
+
+    pub(crate) fn ge<T>(a: T, expected: T) -> bool
+        where T: std::cmp::Ord
+    { a >= expected }
+}

lightning/src/util/errors.rs

@@ -9,7 +9,7 @@ pub enum APIError {
 	/// are documented, but generally indicates some precondition of a function was violated.
 	APIMisuseError {
 		/// A human-readable error message
-		err: &'static str
+		err: String
 	},
 	/// Due to a high feerate, we were unable to complete the request.
 	/// For example, this may be returned if the feerate implies we cannot open a channel at the
@@ -24,14 +24,14 @@ pub enum APIError {
 	/// too-many-hops, etc).
 	RouteError {
 		/// A human-readable error message
-		err: &'static str
+		err: String
 	},
 	/// We were unable to complete the request as the Channel required to do so is unable to
 	/// complete the request (or was not found). This can take many forms, including disconnected
 	/// peer, channel at capacity, channel shutting down, etc.
 	ChannelUnavailable {
 		/// A human-readable error message
-		err: &'static str
+		err: String
 	},
 	/// An attempt to call add/update_monitor returned an Err (ie you did this!), causing the
 	/// attempted action to fail.