f further simplify the return value of get_update_fulfill_htlc

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -1232,7 +1232,7 @@ impl<Signer: Sign> Channel<Signer> {
 		make_funding_redeemscript(&self.get_holder_pubkeys().funding_pubkey, self.counterparty_funding_pubkey())
 	}
 
-	fn get_update_fulfill_htlc<L: Deref>(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage, logger: &L) -> (Option<msgs::UpdateFulfillHTLC>, Option<ChannelMonitorUpdate>) where L::Target: Logger {
+	fn get_update_fulfill_htlc<L: Deref>(&mut self, htlc_id_arg: u64, payment_preimage_arg: PaymentPreimage, logger: &L) -> Option<(Option<msgs::UpdateFulfillHTLC>, ChannelMonitorUpdate)> where L::Target: Logger {
 		// Either ChannelFunded got set (which means it won't be unset) or there is no way any
 		// caller thought we could have something claimed (cause we wouldn't have accepted in an
 		// incoming HTLC anyway). If we got to ShutdownComplete, callers aren't allowed to call us,
@@ -1260,7 +1260,7 @@ impl<Signer: Sign> Channel<Signer> {
 							log_warn!(logger, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {}", log_bytes!(htlc.payment_hash.0), log_bytes!(self.channel_id()));
 							debug_assert!(false, "Tried to fulfill an HTLC that was already failed");
 						}
-						return (None, None);
+						return None;
 					},
 					_ => {
 						debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to");
@@ -1276,7 +1276,7 @@ impl<Signer: Sign> Channel<Signer> {
 			// If we failed to find an HTLC to fulfill, make sure it was previously fulfilled and
 			// this is simply a duplicate claim, not previously failed and we lost funds.
 			debug_assert!(self.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
-			return (None, None);
+			return None;
 		}
 
 		// Now update local state:
@@ -1300,7 +1300,7 @@ impl<Signer: Sign> Channel<Signer> {
 							self.latest_monitor_update_id -= 1;
 							#[cfg(any(test, feature = "fuzztarget"))]
 							debug_assert!(self.historical_inbound_htlc_fulfills.contains(&htlc_id_arg));
-							return (None, None);
+							return None;
 						}
 					},
 					&HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => {
@@ -1309,7 +1309,7 @@ impl<Signer: Sign> Channel<Signer> {
 							// TODO: We may actually be able to switch to a fulfill here, though its
 							// rare enough it may not be worth the complexity burden.
 							debug_assert!(false, "Tried to fulfill an HTLC that was already failed");
-							return (None, Some(monitor_update));
+							return Some((None, monitor_update));
 						}
 					},
 					_ => {}
@@ -1321,7 +1321,7 @@ impl<Signer: Sign> Channel<Signer> {
 			});
 			#[cfg(any(test, feature = "fuzztarget"))]
 			self.historical_inbound_htlc_fulfills.insert(htlc_id_arg);
-			return (None, Some(monitor_update));
+			return Some((None, monitor_update));
 		}
 		#[cfg(any(test, feature = "fuzztarget"))]
 		self.historical_inbound_htlc_fulfills.insert(htlc_id_arg);
@@ -1331,36 +1331,31 @@ impl<Signer: Sign> Channel<Signer> {
 			if let InboundHTLCState::Committed = htlc.state {
 			} else {
 				debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to");
-				return (None, Some(monitor_update));
+				return Some((None, monitor_update));
 			}
 			log_trace!(logger, "Upgrading HTLC {} to LocalRemoved with a Fulfill in channel {}!", log_bytes!(htlc.payment_hash.0), log_bytes!(self.channel_id));
 			htlc.state = InboundHTLCState::LocalRemoved(InboundHTLCRemovalReason::Fulfill(payment_preimage_arg.clone()));
 		}
 
-		(Some(msgs::UpdateFulfillHTLC {
+		Some((Some(msgs::UpdateFulfillHTLC {
 			channel_id: self.channel_id(),
 			htlc_id: htlc_id_arg,
 			payment_preimage: payment_preimage_arg,
-		}), Some(monitor_update))
+		}), monitor_update))
 	}
 
-	pub fn get_update_fulfill_htlc_and_commit<L: Deref>(&mut self, htlc_id: u64, payment_preimage: PaymentPreimage, logger: &L) -> Result<(Option<(msgs::UpdateFulfillHTLC, msgs::CommitmentSigned)>, Option<ChannelMonitorUpdate>), ChannelError> where L::Target: Logger {
+	pub fn get_update_fulfill_htlc_and_commit<L: Deref>(&mut self, htlc_id: u64, payment_preimage: PaymentPreimage, logger: &L) -> Result<Option<(Option<(msgs::UpdateFulfillHTLC, msgs::CommitmentSigned)>, ChannelMonitorUpdate)>, ChannelError> where L::Target: Logger {
 		match self.get_update_fulfill_htlc(htlc_id, payment_preimage, logger) {
-			(Some(update_fulfill_htlc), Some(mut monitor_update)) => {
+			Some((Some(update_fulfill_htlc), mut monitor_update)) => {
 				let (commitment, mut additional_update) = self.send_commitment_no_status_check(logger)?;
 				// send_commitment_no_status_check may bump latest_monitor_id but we want them to be
 				// strictly increasing by one, so decrement it here.
 				self.latest_monitor_update_id = monitor_update.update_id;
 				monitor_update.updates.append(&mut additional_update.updates);
-				Ok((Some((update_fulfill_htlc, commitment)), Some(monitor_update)))
-			},
-			(Some(_), None) => {
-				// If we generated a claim message, we absolutely should have generated a
-				// ChannelMonitorUpdate, otherwise we are going to probably lose funds.
-				unreachable!();
+				Ok(Some((Some((update_fulfill_htlc, commitment)), monitor_update)))
 			},
-			(None, Some(monitor_update)) => Ok((None, Some(monitor_update))),
-			(None, None) => Ok((None, None))
+			Some((None, monitor_update)) => Ok(Some((None, monitor_update))),
+			None => Ok(None)
 		}
 	}
 
@@ -2468,11 +2463,9 @@ impl<Signer: Sign> Channel<Signer> {
 						// not fail - any in between attempts to claim the HTLC will have resulted
 						// in it hitting the holding cell again and we cannot change the state of a
 						// holding cell HTLC from fulfill to anything else.
-						let (update_fulfill_msg_option, additional_monitor_update_opt) = self.get_update_fulfill_htlc(htlc_id, *payment_preimage, logger);
+						let (update_fulfill_msg_option, mut additional_monitor_update) = self.get_update_fulfill_htlc(htlc_id, *payment_preimage, logger).unwrap();
 						update_fulfill_htlcs.push(update_fulfill_msg_option.unwrap());
-						if let Some(mut additional_monitor_update) = additional_monitor_update_opt {
-							monitor_update.updates.append(&mut additional_monitor_update.updates);
-						}
+						monitor_update.updates.append(&mut additional_monitor_update.updates);
 					},
 					&HTLCUpdateAwaitingACK::FailHTLC { htlc_id, ref err_packet } => {
 						match self.get_update_fail_htlc(htlc_id, err_packet.clone(), logger) {

lightning/src/ln/channelmanager.rs

@@ -2681,30 +2681,30 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		if let hash_map::Entry::Occupied(mut chan) = channel_state.by_id.entry(chan_id) {
 			let was_frozen_for_monitor = chan.get().is_awaiting_monitor_update();
 			match chan.get_mut().get_update_fulfill_htlc_and_commit(prev_hop.htlc_id, payment_preimage, &self.logger) {
-				Ok((msgs, monitor_option)) => {
-					if let Some(monitor_update) = monitor_option {
+				Ok(msgs_monitor_option) => {
+					if let Some((msgs, monitor_update)) = msgs_monitor_option {
 						if let Err(e) = self.chain_monitor.update_channel(chan.get().get_funding_txo().unwrap(), monitor_update) {
 							if was_frozen_for_monitor {
 								assert!(msgs.is_none());
 							} else {
 								return Err(Some((chan.get().get_counterparty_node_id(), handle_monitor_err!(self, e, channel_state, chan, RAACommitmentOrder::CommitmentFirst, false, msgs.is_some()).unwrap_err())));
 							}
 						}
-					}
-					if let Some((msg, commitment_signed)) = msgs {
-						log_debug!(self.logger, "Claiming funds for HTLC with preimage {} resulted in a commitment_signed for channel {}",
-							log_bytes!(payment_preimage.0), log_bytes!(chan.get().channel_id()));
-						channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
-							node_id: chan.get().get_counterparty_node_id(),
-							updates: msgs::CommitmentUpdate {
-								update_add_htlcs: Vec::new(),
-								update_fulfill_htlcs: vec![msg],
-								update_fail_htlcs: Vec::new(),
-								update_fail_malformed_htlcs: Vec::new(),
-								update_fee: None,
-								commitment_signed,
-							}
-						});
+						if let Some((msg, commitment_signed)) = msgs {
+							log_debug!(self.logger, "Claiming funds for HTLC with preimage {} resulted in a commitment_signed for channel {}",
+								log_bytes!(payment_preimage.0), log_bytes!(chan.get().channel_id()));
+							channel_state.pending_msg_events.push(events::MessageSendEvent::UpdateHTLCs {
+								node_id: chan.get().get_counterparty_node_id(),
+								updates: msgs::CommitmentUpdate {
+									update_add_htlcs: Vec::new(),
+									update_fulfill_htlcs: vec![msg],
+									update_fail_htlcs: Vec::new(),
+									update_fail_malformed_htlcs: Vec::new(),
+									update_fee: None,
+									commitment_signed,
+								}
+							});
+						}
 					}
 					return Ok(())
 				},