f - fix comment and add extra error check

Author: elnosh

lightning/src/ln/resource_manager.rs

@@ -36,8 +36,7 @@ const MIN_ACCEPTED_HTLCS: u16 = 12;
 const MIN_MAX_IN_FLIGHT_MSAT: u64 = 1_000_000;
 
 /// Resolution time in seconds that is considered "good". HTLCs resolved within this period are
-/// considered normal and are rewarded in the reputation score. HTLCs resolved slower than this
-/// will incur an opportunity cost to penalize slow resolving payments.
+/// considered normal and are rewarded in the reputation score.
 const ACCEPTABLE_RESOLUTION_PERIOD_SECS: u8 = 90;
 
 /// The maximum time (in seconds) that a HTLC can be held. Corresponds to the largest cltv delta
@@ -556,13 +555,10 @@ impl Channel {
 		Ok(congestion_resources_available && !misused_congestion && below_liquidity_limit)
 	}
 
-	fn pending_htlcs_in_congestion(&self, channel_id: u64) -> bool {
+	fn pending_htlcs_in_congestion(&self) -> bool {
 		self.pending_htlcs
 			.iter()
-			.find(|(htlc_ref, pending_htlc)| {
-				htlc_ref.incoming_channel_id == channel_id
-					&& pending_htlc.bucket == BucketAssigned::Congestion
-			})
+			.find(|(_, pending_htlc)| pending_htlc.bucket == BucketAssigned::Congestion)
 			.is_some()
 	}
 
@@ -610,7 +606,10 @@ impl DefaultResourceManager {
 	fn opportunity_cost(&self, resolution_time: Duration, fee_msat: u64) -> u64 {
 		// Computed using f64 as a linear function of how far resolution_time exceeds the
 		// acceptable resolution_period, scaled by the fee. This is done instead of stepwise at
-		// each resolution_period multiple.
+		// each resolution_period multiple because a stepwise function could possibly allow an
+		// attacker to hold HTLCs just under the acceptable resolution_period without incurring
+		// any cost but causing the next hop slower's resolution to get fully penalized by the
+		// fee.
 		let resolution_period = self.config.resolution_period.as_secs_f64();
 		let opportunity_cost = 0_f64
 			.max((resolution_time.as_secs_f64() - resolution_period) / resolution_period)
@@ -697,6 +696,7 @@ impl DefaultResourceManager {
 		// Release bucket resources on each incoming channel for its pending HTLCs.
 		if let Some(removed_channel) = channels_lock.remove(&channel_id) {
 			for (htlc_ref, pending_htlc) in &removed_channel.pending_htlcs {
+				debug_assert!(channels_lock.get(&htlc_ref.incoming_channel_id).is_some());
 				if let Some(incoming_channel) = channels_lock.get_mut(&htlc_ref.incoming_channel_id)
 				{
 					let _ = match pending_htlc.bucket {
@@ -755,8 +755,7 @@ impl DefaultResourceManager {
 		let outgoing_in_flight_risk: u64 = outgoing_channel.outgoing_in_flight_risk();
 		let fee = incoming_amount_msat - outgoing_amount_msat;
 		let in_flight_htlc_risk = self.htlc_in_flight_risk(fee, incoming_cltv_expiry, height_added);
-		let pending_htlcs_in_congestion =
-			outgoing_channel.pending_htlcs_in_congestion(incoming_channel_id);
+		let pending_htlcs_in_congestion = outgoing_channel.pending_htlcs_in_congestion();
 
 		let incoming_channel = channels_lock.get_mut(&incoming_channel_id).ok_or(())?;
 
@@ -854,10 +853,16 @@ impl DefaultResourceManager {
 		resolved_at: u64,
 	) -> Result<(), ()> {
 		let mut channels_lock = self.channels.lock().unwrap();
+		if channels_lock.get(&incoming_channel_id).is_none()
+			|| channels_lock.get(&outgoing_channel_id).is_none()
+		{
+			return Err(());
+		}
+
 		let outgoing_channel = channels_lock.get_mut(&outgoing_channel_id).ok_or(())?;
 
 		let htlc_ref = HtlcRef { incoming_channel_id, htlc_id };
-		let pending_htlc = outgoing_channel.pending_htlcs.get(&htlc_ref).ok_or(())?.clone();
+		let pending_htlc = outgoing_channel.pending_htlcs.remove(&htlc_ref).ok_or(())?;
 
 		if resolved_at < pending_htlc.added_at_unix_seconds {
 			return Err(());
@@ -869,8 +874,11 @@ impl DefaultResourceManager {
 			pending_htlc.outgoing_accountable,
 			settled,
 		);
+		// Note that the decaying averages for reputation and revenue clamp `resolved_at` to
+		// `last_updated_unix_secs` if it falls behind. This could happen because
+		// `last_updated_unix_secs` is frequently mutated. We accept the clamping rather
+		// than erroring, as rejecting out-of-order timestamps would leave resources stuck.
 		outgoing_channel.outgoing_reputation.add_value(effective_fee, resolved_at);
-		outgoing_channel.pending_htlcs.remove(&htlc_ref).ok_or(())?;
 
 		let incoming_channel = channels_lock.get_mut(&incoming_channel_id).ok_or(())?;
 		match pending_htlc.bucket {
@@ -1461,8 +1469,7 @@ mod tests {
 	fn test_congestion_eligible(rm: &DefaultResourceManager, incoming_htlc_amount: u64) -> bool {
 		let mut channels_lock = rm.channels.lock().unwrap();
 		let outgoing_channel = channels_lock.get_mut(&OUTGOING_SCID).unwrap();
-		let pending_htlcs_in_congestion =
-			outgoing_channel.pending_htlcs_in_congestion(INCOMING_SCID);
+		let pending_htlcs_in_congestion = outgoing_channel.pending_htlcs_in_congestion();
 
 		let incoming_channel = channels_lock.get_mut(&INCOMING_SCID).unwrap();
 		incoming_channel
@@ -1877,7 +1884,7 @@ mod tests {
 					.get(&INCOMING_SCID)
 					.unwrap()
 					.incoming_revenue
-					.aggregated_revenue_decaying
+					.aggregated_decaying_average
 					.value,
 				case.expected_revenue,
 			);
@@ -2120,20 +2127,20 @@ mod tests {
 		let conflicting_slots = {
 			let mut channels = rm.channels.lock().unwrap();
 			let incoming = channels.get_mut(&INCOMING_SCID).unwrap();
-			let entry = assign_slots_for_channel(
+			let salt = entropy_source.get_secure_random_bytes();
+			let slots = assign_slots_for_channel(
 				incoming.general_bucket.scid,
 				OUTGOING_SCID_2,
-				None,
-				&entropy_source,
+				salt,
 				incoming.general_bucket.per_channel_slots,
 				incoming.general_bucket.total_slots,
 			)
 			.unwrap();
 
 			let slots_1 = &incoming.general_bucket.channels_slots.get(&OUTGOING_SCID).unwrap().0;
-			let ret = entry.0.iter().filter(|s| slots_1.contains(s)).count();
+			let ret = slots.iter().filter(|s| slots_1.contains(s)).count();
 
-			incoming.general_bucket.channels_slots.insert(OUTGOING_SCID_2, entry);
+			incoming.general_bucket.channels_slots.insert(OUTGOING_SCID_2, (slots, salt));
 			ret
 		};