f possible solution to locking thing

Author: valentinewallace

lightning/src/ln/channelmanager.rs

@@ -1825,23 +1825,17 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	// Fail a list of HTLCs that were just freed from the holding cell. The HTLCs need to be
 	// failed backwards or, if they were one of our outgoing HTLCs, then their failure needs to
 	// be surfaced to the user.
-	fn fail_holding_cell_htlcs(&self, mut htlcs_to_fail: Vec<(HTLCSource, PaymentHash)>, channel_id: [u8; 32]) -> Result<(), MsgHandleErrInternal> {
+	fn fail_holding_cell_htlcs(&self, channel: &Channel<ChanSigner>, pending_msg_events: &mut Vec<events::MessageSendEvent>, forward_htlcs: &mut HashMap<u64, Vec<HTLCForwardInfo>>, mut htlcs_to_fail: Vec<(HTLCSource, PaymentHash)>) -> Result<(), MsgHandleErrInternal> {
 		for (htlc_src, payment_hash) in htlcs_to_fail.drain(..) {
 			match htlc_src {
 				HTLCSource::PreviousHopData(HTLCPreviousHopData { .. }) => {
 					let (failure_code, onion_failure_data) =
-						match self.channel_state.lock().unwrap().by_id.entry(channel_id) {
-							hash_map::Entry::Occupied(chan_entry) => {
-								if let Ok(upd) = self.get_channel_update(&chan_entry.get()) {
-									(0x1000|7, upd.encode_with_len())
-								} else {
-									(0x4000|10, Vec::new())
-								}
-							},
-							hash_map::Entry::Vacant(_) => (0x4000|10, Vec::new())
+						if let Ok(upd) = self.get_channel_update(channel) {
+							(0x1000|7, upd.encode_with_len())
+						} else {
+							(0x4000|10, Vec::new())
 						};
-					let channel_state = self.channel_state.lock().unwrap();
-					self.fail_htlc_backwards_internal(channel_state,
+					self.fail_htlc_backwards_internal_unlocked(pending_msg_events, forward_htlcs,
 						htlc_src, &payment_hash, HTLCFailReason::Reason { failure_code, data: onion_failure_data});
 				},
 				HTLCSource::OutboundRoute { .. } => {
@@ -1868,14 +1862,21 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	/// drop it). In other words, no assumptions are made that entries in claimable_htlcs point to
 	/// still-available channels.
 	fn fail_htlc_backwards_internal(&self, mut channel_state_lock: MutexGuard<ChannelHolder<ChanSigner>>, source: HTLCSource, payment_hash: &PaymentHash, onion_error: HTLCFailReason) {
+		let channel_state = &mut *channel_state_lock;
+		let pending_msg_events = &mut channel_state.pending_msg_events;
+		let forward_htlcs = &mut channel_state.forward_htlcs;
+		self.fail_htlc_backwards_internal_unlocked(pending_msg_events, forward_htlcs, source, payment_hash, onion_error)
+	}
+
+	// Sometimes it's necessary to fail an HTLC backwards while a lock is still held.
+	fn fail_htlc_backwards_internal_unlocked(&self, pending_msg_events: &mut Vec<events::MessageSendEvent>, forward_htlcs: &mut HashMap<u64, Vec<HTLCForwardInfo>>, source: HTLCSource, payment_hash: &PaymentHash, onion_error: HTLCFailReason) {
 		//TODO: There is a timing attack here where if a node fails an HTLC back to us they can
 		//identify whether we sent it or not based on the (I presume) very different runtime
 		//between the branches here. We should make this async and move it into the forward HTLCs
 		//timer handling.
 		match source {
 			HTLCSource::OutboundRoute { ref path, .. } => {
 				log_trace!(self.logger, "Failing outbound payment HTLC with payment_hash {}", log_bytes!(payment_hash.0));
-				mem::drop(channel_state_lock);
 				match &onion_error {
 					&HTLCFailReason::LightningError { ref err } => {
 #[cfg(test)]
@@ -1886,7 +1887,7 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 						// process_onion_failure we should close that channel as it implies our
 						// next-hop is needlessly blaming us!
 						if let Some(update) = channel_update {
-							self.channel_state.lock().unwrap().pending_msg_events.push(
+							pending_msg_events.push(
 								events::MessageSendEvent::PaymentFailureNetworkUpdate {
 									update,
 								}
@@ -1943,18 +1944,17 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 				};
 
 				let mut forward_event = None;
-				if channel_state_lock.forward_htlcs.is_empty() {
+				if forward_htlcs.is_empty() {
 					forward_event = Some(Duration::from_millis(MIN_HTLC_RELAY_HOLDING_CELL_MILLIS));
 				}
-				match channel_state_lock.forward_htlcs.entry(short_channel_id) {
+				match forward_htlcs.entry(short_channel_id) {
 					hash_map::Entry::Occupied(mut entry) => {
 						entry.get_mut().push(HTLCForwardInfo::FailHTLC { htlc_id, err_packet });
 					},
 					hash_map::Entry::Vacant(entry) => {
 						entry.insert(vec!(HTLCForwardInfo::FailHTLC { htlc_id, err_packet }));
 					}
 				}
-				mem::drop(channel_state_lock);
 				if let Some(time) = forward_event {
 					let mut pending_events = self.pending_events.lock().unwrap();
 					pending_events.push(events::Event::PendingHTLCsForwardable {
@@ -2709,33 +2709,20 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 	}
 
 	fn internal_revoke_and_ack(&self, their_node_id: &PublicKey, msg: &msgs::RevokeAndACK) -> Result<(), MsgHandleErrInternal> {
-		let (commitment_update, pending_forwards, mut pending_failures, closing_signed, monitor_update, htlcs_to_fail) = {
+		let (pending_forwards, mut pending_failures, short_channel_id) = {
 			let mut channel_state_lock = self.channel_state.lock().unwrap();
 			let channel_state = &mut *channel_state_lock;
+			let pending_msg_events = &mut channel_state.pending_msg_events;
+			let forward_htlcs = &mut channel_state.forward_htlcs;
 			match channel_state.by_id.entry(msg.channel_id) {
 				hash_map::Entry::Occupied(mut chan) => {
 					if chan.get().get_their_node_id() != *their_node_id {
 						return Err(MsgHandleErrInternal::send_err_msg_no_close("Got a message for a channel from the wrong node!".to_owned(), msg.channel_id));
 					}
-					try_chan_entry!(self, chan.get_mut().revoke_and_ack(&msg, &self.fee_estimator, &self.logger), channel_state, chan)
-				},
-				hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
-			}
-		};
-
-		// Failing back holding cell HTLCs requires acquiring the channel state
-		// lock, hence why this processing can't be done above.
-		if let Err(e) = self.fail_holding_cell_htlcs(htlcs_to_fail, msg.channel_id) { return Err(e) };
-
-		// Updating the corresponding ChannelMonitor may result in returning early,
-		// hence why we couldn't do this processing when we originally acquired the
-		// lock above.
-		let short_channel_id = {
-			let mut channel_state_lock = self.channel_state.lock().unwrap();
-			let channel_state = &mut *channel_state_lock;
-			match channel_state.by_id.entry(msg.channel_id) {
-				hash_map::Entry::Occupied(mut chan) => {
 					let was_frozen_for_monitor = chan.get().is_awaiting_monitor_update();
+					let (commitment_update, pending_forwards, pending_failures, closing_signed, monitor_update, htlcs_to_fail) =
+						try_chan_entry!(self, chan.get_mut().revoke_and_ack(&msg, &self.fee_estimator, &self.logger), channel_state, chan);
+					if let Err(e) = self.fail_holding_cell_htlcs(chan.get(), pending_msg_events, forward_htlcs, htlcs_to_fail) { return Err(e) };
 					if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) {
 						if was_frozen_for_monitor {
 							assert!(commitment_update.is_none() && closing_signed.is_none() && pending_forwards.is_empty() && pending_failures.is_empty());
@@ -2745,18 +2732,18 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 						}
 					}
 					if let Some(updates) = commitment_update {
-						channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
+						pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
 							node_id: their_node_id.clone(),
 							updates,
 						});
 					}
 					if let Some(msg) = closing_signed {
-						channel_state.pending_msg_events.push(events::MessageSendEvent::SendClosingSigned {
+						pending_msg_events.push(events::MessageSendEvent::SendClosingSigned {
 							node_id: their_node_id.clone(),
 							msg,
 						});
 					}
-					chan.get().get_short_channel_id().expect("RAA should only work on a short-id-available channel")
+					(pending_forwards, pending_failures, chan.get().get_short_channel_id().expect("RAA should only work on a short-id-available channel"))
 				},
 				hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id))
 			}