f - refactor claim_funds to only hold channel_state when necessary

Author: ViktorTigerstrom

lightning/src/ln/channelmanager.rs

@@ -4079,8 +4079,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 
 		let _persistence_guard = PersistenceNotifierGuard::notify_on_drop(&self.total_consistency_lock, &self.persistence_notifier);
 
-		let mut channel_state = Some(self.channel_state.lock().unwrap());
-		let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash);
+		let removed_source = self.channel_state.lock().unwrap().claimable_htlcs.remove(&payment_hash);
 		if let Some((payment_purpose, mut sources)) = removed_source {
 			assert!(!sources.is_empty());
 
@@ -4098,72 +4097,77 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			let mut claimable_amt_msat = 0;
 			let mut expected_amt_msat = None;
 			let mut valid_mpp = true;
-			for htlc in sources.iter() {
-				if let None = channel_state.as_ref().unwrap().short_to_chan_info.get(&htlc.prev_hop.short_channel_id) {
-					valid_mpp = false;
-					break;
-				}
-				if expected_amt_msat.is_some() && expected_amt_msat != Some(htlc.total_msat) {
-					log_error!(self.logger, "Somehow ended up with an MPP payment with different total amounts - this should not be reachable!");
-					debug_assert!(false);
-					valid_mpp = false;
-					break;
-				}
-				expected_amt_msat = Some(htlc.total_msat);
-				if let OnionPayload::Spontaneous(_) = &htlc.onion_payload {
-					// We don't currently support MPP for spontaneous payments, so just check
-					// that there's one payment here and move on.
-					if sources.len() != 1 {
-						log_error!(self.logger, "Somehow ended up with an MPP spontaneous payment - this should not be reachable!");
+			let mut errs = Vec::new();
+			let mut claimed_any_htlcs = false;
+			{
+				let mut channel_state_lock = self.channel_state.lock().unwrap();
+				let channel_state = &mut *channel_state_lock;
+				for htlc in sources.iter() {
+					if let None = channel_state.short_to_chan_info.get(&htlc.prev_hop.short_channel_id) {
+						valid_mpp = false;
+						break;
+					}
+					if expected_amt_msat.is_some() && expected_amt_msat != Some(htlc.total_msat) {
+						log_error!(self.logger, "Somehow ended up with an MPP payment with different total amounts - this should not be reachable!");
 						debug_assert!(false);
 						valid_mpp = false;
 						break;
 					}
-				}
+					expected_amt_msat = Some(htlc.total_msat);
+					if let OnionPayload::Spontaneous(_) = &htlc.onion_payload {
+						// We don't currently support MPP for spontaneous payments, so just check
+						// that there's one payment here and move on.
+						if sources.len() != 1 {
+							log_error!(self.logger, "Somehow ended up with an MPP spontaneous payment - this should not be reachable!");
+							debug_assert!(false);
+							valid_mpp = false;
+							break;
+						}
+					}
 
-				claimable_amt_msat += htlc.value;
-			}
-			if sources.is_empty() || expected_amt_msat.is_none() {
-				log_info!(self.logger, "Attempted to claim an incomplete payment which no longer had any available HTLCs!");
-				return;
-			}
-			if claimable_amt_msat != expected_amt_msat.unwrap() {
-				log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",
-					expected_amt_msat.unwrap(), claimable_amt_msat);
-				return;
+					claimable_amt_msat += htlc.value;
+				}
+				if sources.is_empty() || expected_amt_msat.is_none() {
+					log_info!(self.logger, "Attempted to claim an incomplete payment which no longer had any available HTLCs!");
+					return;
+				}
+				if claimable_amt_msat != expected_amt_msat.unwrap() {
+					log_info!(self.logger, "Attempted to claim an incomplete payment, expected {} msat, had {} available to claim.",
+						expected_amt_msat.unwrap(), claimable_amt_msat);
+					return;
+				}
+				if valid_mpp {
+					for htlc in sources.drain(..) {
+						match self.claim_funds_from_hop(&mut channel_state_lock, htlc.prev_hop, payment_preimage) {
+							ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
+								if let msgs::ErrorAction::IgnoreError = err.err.action {
+									// We got a temporary failure updating monitor, but will claim the
+									// HTLC when the monitor updating is restored (or on chain).
+									log_error!(self.logger, "Temporary failure claiming HTLC, treating as success: {}", err.err.err);
+									claimed_any_htlcs = true;
+								} else { errs.push((pk, err)); }
+							},
+							ClaimFundsFromHop::PrevHopForceClosed => unreachable!("We already checked for channel existence, we can't fail here!"),
+							ClaimFundsFromHop::DuplicateClaim => {
+								// While we should never get here in most cases, if we do, it likely
+								// indicates that the HTLC was timed out some time ago and is no longer
+								// available to be claimed. Thus, it does not make sense to set
+								// `claimed_any_htlcs`.
+							},
+							ClaimFundsFromHop::Success(_) => claimed_any_htlcs = true,
+						}
+					}
+				}
 			}
-
-			let mut errs = Vec::new();
-			let mut claimed_any_htlcs = false;
-			for htlc in sources.drain(..) {
-				if !valid_mpp {
-					if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
+			if !valid_mpp {
+				for htlc in sources.drain(..) {
 					let mut htlc_msat_height_data = byte_utils::be64_to_array(htlc.value).to_vec();
 					htlc_msat_height_data.extend_from_slice(&byte_utils::be32_to_array(
-							self.best_block.read().unwrap().height()));
+						self.best_block.read().unwrap().height()));
 					self.fail_htlc_backwards_internal(
-									 HTLCSource::PreviousHopData(htlc.prev_hop), &payment_hash,
-									 HTLCFailReason::Reason { failure_code: 0x4000|15, data: htlc_msat_height_data },
-									 HTLCDestination::FailedPayment { payment_hash } );
-				} else {
-					match self.claim_funds_from_hop(channel_state.as_mut().unwrap(), htlc.prev_hop, payment_preimage) {
-						ClaimFundsFromHop::MonitorUpdateFail(pk, err, _) => {
-							if let msgs::ErrorAction::IgnoreError = err.err.action {
-								// We got a temporary failure updating monitor, but will claim the
-								// HTLC when the monitor updating is restored (or on chain).
-								log_error!(self.logger, "Temporary failure claiming HTLC, treating as success: {}", err.err.err);
-								claimed_any_htlcs = true;
-							} else { errs.push((pk, err)); }
-						},
-						ClaimFundsFromHop::PrevHopForceClosed => unreachable!("We already checked for channel existence, we can't fail here!"),
-						ClaimFundsFromHop::DuplicateClaim => {
-							// While we should never get here in most cases, if we do, it likely
-							// indicates that the HTLC was timed out some time ago and is no longer
-							// available to be claimed. Thus, it does not make sense to set
-							// `claimed_any_htlcs`.
-						},
-						ClaimFundsFromHop::Success(_) => claimed_any_htlcs = true,
-					}
+						HTLCSource::PreviousHopData(htlc.prev_hop), &payment_hash,
+						HTLCFailReason::Reason { failure_code: 0x4000|15, data: htlc_msat_height_data },
+						HTLCDestination::FailedPayment { payment_hash } );
 				}
 			}
 
@@ -4175,10 +4179,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 				});
 			}
 
-			// Now that we've done the entire above loop in one lock, we can handle any errors
-			// which were generated.
-			channel_state.take();
-
+			// Now we can handle any errors which were generated.
 			for (counterparty_node_id, err) in errs.drain(..) {
 				let res: Result<(), _> = Err(err);
 				let _ = handle_error!(self, res, counterparty_node_id);