From a6ddb973ea8ac173c700435e57a897bafc420fbb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 13 Nov 2021 22:47:42 +0000 Subject: [PATCH] Return struct, not long tuple, from `Channel::channel_reestablish` This improves readability and makes it easier to add additional return fields. --- lightning/src/ln/channel.rs | 90 ++++++++++++++++++++++++------ lightning/src/ln/channelmanager.rs | 11 ++-- 2 files changed, 79 insertions(+), 22 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 70342ef18..c9d54a048 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -401,6 +401,17 @@ pub(super) struct MonitorRestoreUpdates { pub funding_locked: Option, } +/// The return value of `channel_reestablish` +pub(super) struct ReestablishResponses { + pub funding_locked: Option, + pub raa: Option, + pub commitment_update: Option, + pub order: RAACommitmentOrder, + pub mon_update: Option, + pub holding_cell_failed_htlcs: Vec<(HTLCSource, PaymentHash)>, + pub shutdown_msg: Option, +} + /// If the majority of the channels funds are to the fundee and the initiator holds only just /// enough funds to cover their reserve value, channels are at risk of getting "stuck". Because the /// initiator controls the feerate, if they then go to increase the channel fee, they may have no @@ -3501,7 +3512,7 @@ impl Channel { /// 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(&mut self, msg: &msgs::ChannelReestablish, logger: &L) -> Result<(Option, Option, Option, Option, RAACommitmentOrder, Vec<(HTLCSource, PaymentHash)>, Option), ChannelError> where L::Target: Logger { + pub fn channel_reestablish(&mut self, msg: &msgs::ChannelReestablish, logger: &L) -> Result 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 @@ -3553,15 +3564,27 @@ impl Channel { 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(ReestablishResponses { + funding_locked: None, + raa: None, commitment_update: None, mon_update: None, + order: RAACommitmentOrder::CommitmentFirst, + holding_cell_failed_htlcs: Vec::new(), + 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(ReestablishResponses { + funding_locked: Some(msgs::FundingLocked { + channel_id: self.channel_id(), + next_per_commitment_point, + }), + raa: None, commitment_update: None, mon_update: None, + order: RAACommitmentOrder::CommitmentFirst, + holding_cell_failed_htlcs: Vec::new(), + shutdown_msg + }); } let required_revoke = if msg.next_remote_commitment_number + 1 == INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number { @@ -3585,7 +3608,7 @@ impl Channel { // the corresponding revoke_and_ack back yet. let next_counterparty_commitment_number = INITIAL_COMMITMENT_NUMBER - self.cur_counterparty_commitment_transaction_number + if (self.channel_state & ChannelState::AwaitingRemoteRevoke as u32) != 0 { 1 } else { 0 }; - let resend_funding_locked = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number == 1 { + let funding_locked = if msg.next_local_commitment_number == 1 && INITIAL_COMMITMENT_NUMBER - self.cur_holder_commitment_transaction_number == 1 { // We should never have to worry about MonitorUpdateFailed resending FundingLocked let next_per_commitment_point = self.holder_signer.get_per_commitment_point(self.cur_holder_commitment_transaction_number, &self.secp_ctx); Some(msgs::FundingLocked { @@ -3607,18 +3630,39 @@ impl Channel { // have received some updates while we were disconnected. Free the holding cell // now! match self.free_holding_cell_htlcs(logger) { - Err(ChannelError::Close(msg)) => return Err(ChannelError::Close(msg)), + Err(ChannelError::Close(msg)) => Err(ChannelError::Close(msg)), Err(ChannelError::Warn(_)) | 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)); + Ok((Some((commitment_update, monitor_update)), holding_cell_failed_htlcs)) => { + Ok(ReestablishResponses { + funding_locked, shutdown_msg, + raa: required_revoke, + commitment_update: Some(commitment_update), + order: self.resend_order.clone(), + mon_update: Some(monitor_update), + holding_cell_failed_htlcs, + }) }, - Ok((None, htlcs_to_fail)) => { - return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), htlcs_to_fail, shutdown_msg)); + Ok((None, holding_cell_failed_htlcs)) => { + Ok(ReestablishResponses { + funding_locked, shutdown_msg, + raa: required_revoke, + commitment_update: None, + order: self.resend_order.clone(), + mon_update: None, + holding_cell_failed_htlcs, + }) }, } } else { - return Ok((resend_funding_locked, required_revoke, None, None, self.resend_order.clone(), Vec::new(), shutdown_msg)); + Ok(ReestablishResponses { + funding_locked, shutdown_msg, + raa: required_revoke, + commitment_update: None, + order: self.resend_order.clone(), + mon_update: None, + holding_cell_failed_htlcs: Vec::new(), + }) } } else if msg.next_local_commitment_number == next_counterparty_commitment_number - 1 { if required_revoke.is_some() { @@ -3629,12 +3673,24 @@ impl Channel { 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)); + Ok(ReestablishResponses { + funding_locked, shutdown_msg, + commitment_update: None, raa: None, mon_update: None, + order: self.resend_order.clone(), + holding_cell_failed_htlcs: Vec::new(), + }) + } else { + Ok(ReestablishResponses { + funding_locked, shutdown_msg, + raa: required_revoke, + commitment_update: Some(self.get_last_commitment_update(logger)), + order: self.resend_order.clone(), + mon_update: None, + holding_cell_failed_htlcs: Vec::new(), + }) } - - return Ok((resend_funding_locked, required_revoke, Some(self.get_last_commitment_update(logger)), None, self.resend_order.clone(), Vec::new(), shutdown_msg)); } else { - return Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction".to_owned())); + Err(ChannelError::Close("Peer attempted to reestablish channel with a very old remote commitment transaction".to_owned())) } } diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index 27bfc5673..c67010151 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -4722,10 +4722,9 @@ impl ChannelMana // disconnect, so Channel's reestablish will never hand us any holding cell // freed HTLCs to fail backwards. If in the future we no longer drop pending // add-HTLCs on disconnect, we may be handed HTLCs to fail backwards here. - let (funding_locked, revoke_and_ack, commitment_update, monitor_update_opt, order, htlcs_failed_forward, shutdown) = - try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan); + let responses = try_chan_entry!(self, chan.get_mut().channel_reestablish(msg, &self.logger), channel_state, chan); let mut channel_update = None; - if let Some(msg) = shutdown { + if let Some(msg) = responses.shutdown_msg { channel_state.pending_msg_events.push(events::MessageSendEvent::SendShutdown { node_id: counterparty_node_id.clone(), msg, @@ -4740,11 +4739,13 @@ impl ChannelMana }); } let need_lnd_workaround = chan.get_mut().workaround_lnd_bug_4006.take(); - chan_restoration_res = handle_chan_restoration_locked!(self, channel_state_lock, channel_state, chan, revoke_and_ack, commitment_update, order, monitor_update_opt, Vec::new(), None, funding_locked); + chan_restoration_res = handle_chan_restoration_locked!( + self, channel_state_lock, channel_state, chan, responses.raa, responses.commitment_update, responses.order, + responses.mon_update, Vec::new(), None, responses.funding_locked); if let Some(upd) = channel_update { channel_state.pending_msg_events.push(upd); } - (htlcs_failed_forward, need_lnd_workaround) + (responses.holding_cell_failed_htlcs, need_lnd_workaround) }, hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel".to_owned(), msg.channel_id)) } -- 2.39.5