Use return struct in monitor_updating_restored+channel_reestablish

As pointed out by Jeff, using a return struct instead of an
incredibly-long tuple improves readability in several places.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -187,6 +187,21 @@ enum HTLCUpdateAwaitingACK {
 	},
 }
 
+/// A struct capturing the set of things we may need to do after a reconnect or after monitor
+/// updating has been restored.
+pub(super) struct ChannelRestoredUpdates {
+	pub(super) revoke_and_ack: Option<msgs::RevokeAndACK>,
+	pub(super) commitment_update: Option<msgs::CommitmentUpdate>,
+	pub(super) raa_commitment_order: RAACommitmentOrder,
+	pub(super) chanmon_update: Option<ChannelMonitorUpdate>,
+	pub(super) pending_forwards: Vec<(PendingHTLCInfo, u64)>,
+	pub(super) pending_failures: Vec<(HTLCSource, PaymentHash, HTLCFailReason)>,
+	pub(super) forwarding_failures: Vec<(HTLCSource, PaymentHash)>,
+	pub(super) needs_broadcast_safe: bool,
+	pub(super) funding_locked: Option<msgs::FundingLocked>,
+	pub(super) shutdown: Option<msgs::Shutdown>,
+}
+
 /// There are a few "states" and then a number of flags which can be applied:
 /// We first move through init with OurInitSent -> TheirInitSent -> FundingCreated -> FundingSent.
 /// TheirFundingLocked and OurFundingLocked then get set on FundingSent, and when both are set we
@@ -2739,10 +2754,7 @@ impl<Signer: Sign> Channel<Signer> {
 	/// Indicates that the latest ChannelMonitor update has been committed by the client
 	/// successfully and we should restore normal operation. Returns messages which should be sent
 	/// to the remote side.
-	pub fn monitor_updating_restored<L: Deref>(&mut self, logger: &L) -> (
-			Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, RAACommitmentOrder, Option<ChannelMonitorUpdate>,
-			Vec<(PendingHTLCInfo, u64)>, Vec<(HTLCSource, PaymentHash, HTLCFailReason)>, Vec<(HTLCSource, PaymentHash)>,
-			bool, Option<msgs::FundingLocked>) where L::Target: Logger {
+	pub fn monitor_updating_restored<L: Deref>(&mut self, logger: &L) -> ChannelRestoredUpdates where L::Target: Logger {
 		assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32);
 		self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32);
 
@@ -2764,35 +2776,46 @@ impl<Signer: Sign> Channel<Signer> {
 			})
 		} else { None };
 
-		let mut forwards = Vec::new();
-		mem::swap(&mut forwards, &mut self.monitor_pending_forwards);
-		let mut failures = Vec::new();
-		mem::swap(&mut failures, &mut self.monitor_pending_failures);
+		let mut pending_forwards = Vec::new();
+		mem::swap(&mut pending_forwards, &mut self.monitor_pending_forwards);
+		let mut pending_failures = Vec::new();
+		mem::swap(&mut pending_failures, &mut self.monitor_pending_failures);
 
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) != 0 {
 			self.monitor_pending_revoke_and_ack = false;
 			self.monitor_pending_commitment_signed = false;
-			return (None, None, RAACommitmentOrder::RevokeAndACKFirst, None, forwards, failures, Vec::new(), needs_broadcast_safe, funding_locked);
+			return ChannelRestoredUpdates {
+				revoke_and_ack: None,
+				commitment_update: None,
+				raa_commitment_order: RAACommitmentOrder::RevokeAndACKFirst,
+				chanmon_update: None,
+				pending_forwards,
+				pending_failures,
+				forwarding_failures: Vec::new(),
+				needs_broadcast_safe,
+				funding_locked,
+				shutdown: None,
+			}
 		}
 
-		let raa = if self.monitor_pending_revoke_and_ack {
+		let revoke_and_ack = if self.monitor_pending_revoke_and_ack {
 			Some(self.get_last_revoke_and_ack())
 		} else { None };
 		let mut commitment_update = if self.monitor_pending_commitment_signed {
 			Some(self.get_last_commitment_update(logger))
 		} else { None };
 
-		let mut order = self.resend_order.clone();
+		let mut raa_commitment_order = self.resend_order.clone();
 		self.monitor_pending_revoke_and_ack = false;
 		self.monitor_pending_commitment_signed = false;
 
-		let mut htlcs_failed_to_forward = Vec::new();
+		let mut forwarding_failures = Vec::new();
 		let mut chanmon_update = None;
 		if commitment_update.is_none() && self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32) == 0 {
-			order = RAACommitmentOrder::RevokeAndACKFirst;
+			raa_commitment_order = RAACommitmentOrder::RevokeAndACKFirst;
 
 			let (update_opt, mut failed_htlcs) = self.free_holding_cell_htlcs(logger).unwrap();
-			htlcs_failed_to_forward.append(&mut failed_htlcs);
+			forwarding_failures.append(&mut failed_htlcs);
 			if let Some((com_update, mon_update)) = update_opt {
 				commitment_update = Some(com_update);
 				chanmon_update = Some(mon_update);
@@ -2802,9 +2825,21 @@ impl<Signer: Sign> Channel<Signer> {
 		log_trace!(logger, "Restored monitor updating resulting in {}{} commitment update and {} RAA, with {} first",
 			if needs_broadcast_safe { "a funding broadcast safe, " } else { "" },
 			if commitment_update.is_some() { "a" } else { "no" },
-			if raa.is_some() { "an" } else { "no" },
-			match order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
-		(raa, commitment_update, order, chanmon_update, forwards, failures, htlcs_failed_to_forward, needs_broadcast_safe, funding_locked)
+			if revoke_and_ack.is_some() { "an" } else { "no" },
+			match raa_commitment_order { RAACommitmentOrder::CommitmentFirst => "commitment", RAACommitmentOrder::RevokeAndACKFirst => "RAA"});
+
+		ChannelRestoredUpdates {
+			revoke_and_ack,
+			commitment_update,
+			raa_commitment_order,
+			chanmon_update,
+			pending_forwards,
+			pending_failures,
+			forwarding_failures,
+			needs_broadcast_safe,
+			funding_locked,
+			shutdown: None,
+		}
 	}
 
 	pub fn update_fee<F: Deref>(&mut self, fee_estimator: &F, msg: &msgs::UpdateFee) -> Result<(), ChannelError>
@@ -2891,7 +2926,7 @@ impl<Signer: Sign> Channel<Signer> {
 
 	/// May panic if some calls other than message-handling calls (which will all Err immediately)
 	/// have been called between remove_uncommitted_htlcs_and_mark_paused and this call.
-	pub fn channel_reestablish<L: Deref>(&mut self, msg: &msgs::ChannelReestablish, logger: &L) -> Result<(Option<msgs::FundingLocked>, Option<msgs::RevokeAndACK>, Option<msgs::CommitmentUpdate>, Option<ChannelMonitorUpdate>, RAACommitmentOrder, Vec<(HTLCSource, PaymentHash)>, Option<msgs::Shutdown>), ChannelError> where L::Target: Logger {
+	pub fn channel_reestablish<L: Deref>(&mut self, msg: &msgs::ChannelReestablish, logger: &L) -> Result<ChannelRestoredUpdates, ChannelError> where L::Target: Logger {
 		if self.channel_state & (ChannelState::PeerDisconnected as u32) == 0 {
 			// While BOLT 2 doesn't indicate explicitly we should error this channel here, it
 			// almost certainly indicates we are going to end up out-of-sync in some way, so we
@@ -2942,15 +2977,38 @@ impl<Signer: Sign> Channel<Signer> {
 					return Err(ChannelError::Close("Peer claimed they saw a revoke_and_ack but we haven't sent funding_locked yet".to_owned()));
 				}
 				// Short circuit the whole handler as there is nothing we can resend them
-				return Ok((None, None, None, None, RAACommitmentOrder::CommitmentFirst, Vec::new(), shutdown_msg));
+				return Ok(ChannelRestoredUpdates {
+					revoke_and_ack: None,
+					commitment_update: None,
+					raa_commitment_order: RAACommitmentOrder::CommitmentFirst,
+					chanmon_update: None,
+					pending_forwards: Vec::new(),
+					pending_failures: Vec::new(),
+					forwarding_failures: Vec::new(),
+					needs_broadcast_safe: false,
+					funding_locked: None,
+					shutdown: shutdown_msg,
+				});
 			}
 
 			// We have OurFundingLocked set!
 			let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx);
-			return Ok((Some(msgs::FundingLocked {
-				channel_id: self.channel_id(),
-				next_per_commitment_point,
-			}), None, None, None, RAACommitmentOrder::CommitmentFirst, Vec::new(), shutdown_msg));
+
+			return Ok(ChannelRestoredUpdates {
+				revoke_and_ack: None,
+				commitment_update: None,
+				raa_commitment_order: RAACommitmentOrder::CommitmentFirst,
+				chanmon_update: None,
+				pending_forwards: Vec::new(),
+				pending_failures: Vec::new(),
+				forwarding_failures: Vec::new(),
+				needs_broadcast_safe: false,
+				funding_locked: Some(msgs::FundingLocked {
+					channel_id: self.channel_id(),
+					next_per_commitment_point,
+				}),
+				shutdown: shutdown_msg,
+			});
 		}
 
 		let required_revoke = if msg.next_remote_commitment_number + 1 == INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number {
@@ -2999,14 +3057,47 @@ impl<Signer: Sign> Channel<Signer> {
 					Err(ChannelError::Close(msg)) => return Err(ChannelError::Close(msg)),
 					Err(ChannelError::Ignore(_)) | Err(ChannelError::CloseDelayBroadcast(_)) => panic!("Got non-channel-failing result from free_holding_cell_htlcs"),
 					Ok((Some((commitment_update, monitor_update)), htlcs_to_fail)) => {
-						return Ok((resend_funding_locked, required_revoke, Some(commitment_update), Some(monitor_update), self.resend_order.clone(), htlcs_to_fail, shutdown_msg));
+						return Ok(ChannelRestoredUpdates {
+							revoke_and_ack: required_revoke,
+							commitment_update: Some(commitment_update),
+							raa_commitment_order: self.resend_order.clone(),
+							chanmon_update: Some(monitor_update),
+							pending_forwards: Vec::new(),
+							pending_failures: Vec::new(),
+							forwarding_failures: htlcs_to_fail,
+							needs_broadcast_safe: false,
+							funding_locked: resend_funding_locked,
+							shutdown: shutdown_msg,
+						});
 					},
 					Ok((None, htlcs_to_fail)) => {
-						return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), htlcs_to_fail, shutdown_msg));
+						return Ok(ChannelRestoredUpdates {
+							revoke_and_ack: required_revoke,
+							commitment_update: None,
+							raa_commitment_order: self.resend_order.clone(),
+							chanmon_update: None,
+							pending_forwards: Vec::new(),
+							pending_failures: Vec::new(),
+							forwarding_failures: htlcs_to_fail,
+							needs_broadcast_safe: false,
+							funding_locked: resend_funding_locked,
+							shutdown: shutdown_msg,
+						});
 					},
 				}
 			} else {
-				return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), Vec::new(), shutdown_msg));
+				return Ok(ChannelRestoredUpdates {
+					revoke_and_ack: required_revoke,
+					commitment_update: None,
+					raa_commitment_order: self.resend_order.clone(),
+					chanmon_update: None,
+					pending_forwards: Vec::new(),
+					pending_failures: Vec::new(),
+					forwarding_failures: Vec::new(),
+					needs_broadcast_safe: false,
+					funding_locked: resend_funding_locked,
+					shutdown: shutdown_msg,
+				});
 			}
 		} else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 {
 			if required_revoke.is_some() {
@@ -3017,10 +3108,32 @@ impl<Signer: Sign> Channel<Signer> {
 
 			if self.channel_state & (ChannelState::MonitorUpdateFailed as u32) != 0 {
 				self.monitor_pending_commitment_signed = true;
-				return Ok((resend_funding_locked, None, None, None, self.resend_order.clone(), Vec::new(), shutdown_msg));
+				return Ok(ChannelRestoredUpdates {
+					revoke_and_ack: None,
+					commitment_update: None,
+					raa_commitment_order: self.resend_order.clone(),
+					chanmon_update: None,
+					pending_forwards: Vec::new(),
+					pending_failures: Vec::new(),
+					forwarding_failures: Vec::new(),
+					needs_broadcast_safe: false,
+					funding_locked: resend_funding_locked,
+					shutdown: shutdown_msg,
+				});
 			}
 
-			return Ok((resend_funding_locked, required_revoke, Some(self.get_last_commitment_update(logger)), None, self.resend_order.clone(), Vec::new(), shutdown_msg));
+			return Ok(ChannelRestoredUpdates {
+				revoke_and_ack: required_revoke,
+				commitment_update: Some(self.get_last_commitment_update(logger)),
+				raa_commitment_order: self.resend_order.clone(),
+				chanmon_update: None,
+				pending_forwards: Vec::new(),
+				pending_failures: Vec::new(),
+				forwarding_failures: Vec::new(),
+				needs_broadcast_safe: false,
+				funding_locked: resend_funding_locked,
+				shutdown: shutdown_msg,
+			});
 		} else {
 			return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction".to_owned()));
 		}