Impl Jeff refactor: the advantage is that it keeps channel checks and updating pending_forward_info inside Channel rather than split between Channel and ChannelManager.

Author: valentinewallace

lightning/src/ln/channel.rs

@@ -1733,8 +1733,18 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 		self.commit_tx_fee_msat(their_acked_htlcs + addl_htlcs)
 	}
 
-  pub fn update_add_htlc<F>(&mut self, msg: &msgs::UpdateAddHTLC, mut pending_forward_state: PendingHTLCStatus, create_onion_error_packet: F) -> Result<(), ChannelError>
-  where F: for<'a, 'b> Fn(&'a Self, &'b PendingHTLCStatus) -> Option<msgs::OnionErrorPacket> {
+  pub fn update_add_htlc<F>(&mut self, msg: &msgs::UpdateAddHTLC, mut pending_forward_state: PendingHTLCStatus, create_pending_htlc_status: F) -> Result<(), ChannelError>
+  where F: for<'a, 'b> Fn(&'a Self, &'b PendingHTLCStatus, u16) -> Option<PendingHTLCStatus> {
+		if !self.is_usable() {
+			// TODO: Note that |20 is defined as "channel FROM the processing
+			// node has been disabled" (emphasis mine), which seems to imply
+			// that we can't return |20 for an inbound channel being disabled.
+			// This probably needs a spec update but should definitely be
+			// allowed.
+			if let Some(new_state) = create_pending_htlc_status(self, &pending_forward_state, 0x1000|20) {
+				pending_forward_state = new_state;
+			}
+		}
 		if (self.channel_state & (ChannelState::ChannelFunded as u32 | ChannelState::RemoteShutdownSent as u32)) != (ChannelState::ChannelFunded as u32) {
 			return Err(ChannelError::Close("Got add HTLC message when channel was not in an operational state"));
 		}
@@ -1807,13 +1817,8 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			// but should help protect us from stuck channels).
 			let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.next_remote_commit_tx_fee_msat(1 + 1);
 			if pending_remote_value_msat - msg.amount_msat - chan_reserve_msat < remote_fee_cost_incl_stuck_buffer_msat {
-        let onion_error_packet = create_onion_error_packet(self, &pending_forward_state);
-        if let Some(reason) = onion_error_packet {
-					pending_forward_state = PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
-						channel_id: msg.channel_id,
-						htlc_id: msg.htlc_id,
-						reason,
-					}));
+				if let Some(new_state) = create_pending_htlc_status(self, &pending_forward_state, 0x1000|7) {
+					pending_forward_state = new_state;
 				}
 			}
 		} else {
@@ -4431,11 +4436,11 @@ mod tests {
 	use bitcoin::network::constants::Network;
 	use bitcoin::hashes::hex::FromHex;
 	use hex;
-	use ln::channelmanager::{HTLCSource, PaymentPreimage, PaymentHash, PendingHTLCStatus, PendingHTLCInfo, PendingHTLCRouting};
+	use ln::channelmanager::{HTLCSource, PaymentPreimage, PaymentHash, PendingHTLCStatus, PendingHTLCInfo, PendingHTLCRouting, HTLCFailureMsg};
 	use ln::channel::{Channel,ChannelKeys,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,TxCreationKeys, ChannelError};
 	use ln::channel::MAX_FUNDING_SATOSHIS;
 	use ln::features::{InitFeatures, NodeFeatures};
-	use ln::msgs::{OptionalField, DataLossProtect, UpdateAddHTLC, OnionErrorPacket, OnionPacket};
+	use ln::msgs::{OptionalField, DataLossProtect, UpdateAddHTLC, UpdateFailHTLC, OnionErrorPacket, OnionPacket};
 	use ln::{chan_utils, onion_utils};
 	use ln::chan_utils::{LocalCommitmentTransaction, ChannelPublicKeys};
 	use ln::features::ChannelFeatures;
@@ -4691,12 +4696,18 @@ mod tests {
 			amt_to_forward: max_can_recv + 1,
 			outgoing_cltv_value: htlc_cltv
 		});
-		let create_onion_closure = |_chan: &Channel<EnforcingChannelKeys>, _pending_forward_info: &PendingHTLCStatus| {
-			Some(OnionErrorPacket{ data: vec![] })
+		let create_pending_htlc_status = |chan: &Channel<EnforcingChannelKeys>, _pending_forward_info: &PendingHTLCStatus, _error_code: u16| {
+			Some(PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(
+				UpdateFailHTLC{
+					channel_id: chan.channel_id,
+					htlc_id: 0,
+					reason: OnionErrorPacket { data: vec![] },
+				}
+			)))
 		};
 
 		// Assert that the HTLC was successfully added.
-		assert!(chans[1].update_add_htlc(&msg, pending_forward_state, create_onion_closure).is_ok());
+		assert!(chans[1].update_add_htlc(&msg, pending_forward_state, &create_pending_htlc_status).is_ok());
 
 		// Check that the HTLC's pending status is failed.
 		match &chans[1].pending_inbound_htlcs[0].state {
@@ -4707,8 +4718,14 @@ mod tests {
 		}
 
 		// Check that adding an HTLC worth 1 msat less will succeed.
-		let create_onion_closure = |_chan: &Channel<EnforcingChannelKeys>, _pending_forward_info: &PendingHTLCStatus| {
-			Some(OnionErrorPacket{ data: vec![] })
+		let create_pending_htlc_status = |chan: &Channel<EnforcingChannelKeys>, _pending_forward_info: &PendingHTLCStatus, _error_code: u16| {
+			Some(PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(
+				UpdateFailHTLC{
+					channel_id: chan.channel_id,
+					htlc_id: 0,
+					reason: OnionErrorPacket { data: vec![] },
+				}
+			)))
 		};
 		chans[1].pending_inbound_htlcs = vec![];
 		let pending_forward_state = PendingHTLCStatus::Forward(PendingHTLCInfo{
@@ -4720,7 +4737,7 @@ mod tests {
 		});
 		msg.htlc_id = 1;
 		msg.amount_msat = max_can_recv;
-		assert!(chans[1].update_add_htlc(&msg, pending_forward_state, create_onion_closure).is_ok());
+		assert!(chans[1].update_add_htlc(&msg, pending_forward_state, &create_pending_htlc_status).is_ok());
 		match &chans[1].pending_inbound_htlcs[0].state {
 			&InboundHTLCState::RemoteAnnounced(ref state) => {
 				if let &PendingHTLCStatus::Forward(_) = state { } else { panic!() }
@@ -4784,11 +4801,11 @@ mod tests {
 			outgoing_cltv_value: htlc_cltv
 		});
 
-		let create_onion_closure = |_chan: &Channel<EnforcingChannelKeys>, _pending_forward_info: &PendingHTLCStatus| {
+		let create_pending_htlc_status = |_chan: &Channel<EnforcingChannelKeys>, _pending_forward_info: &PendingHTLCStatus, _error_code: u16| {
 			None
 		};
 
-		match chans[0].update_add_htlc(&msg, pending_forward_state, create_onion_closure) {
+		match chans[0].update_add_htlc(&msg, pending_forward_state, &create_pending_htlc_status) {
 			Err(ChannelError::Ignore(msg)) => assert_eq!(msg, "Cannot receive value that would put us under local channel reserve value"),
 			_ => panic!()
 		}

lightning/src/ln/channelmanager.rs

@@ -2495,65 +2495,47 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
 		//encrypted with the same key. It's not immediately obvious how to usefully exploit that,
 		//but we should prevent it anyway.
 
-		let (mut pending_forward_info, mut channel_state_lock) = self.decode_update_add_htlc_onion(msg);
+		let (pending_forward_info, mut channel_state_lock) = self.decode_update_add_htlc_onion(msg);
 		let channel_state = &mut *channel_state_lock;
 
 		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!", msg.channel_id));
 				}
-				if !chan.get().is_usable() {
+
+				let create_pending_htlc_status = |chan: &Channel<ChanSigner>, pending_forward_info: &PendingHTLCStatus, error_code: u16| {
 					// If the update_add is completely bogus, the call will Err and we will close,
 					// but if we've sent a shutdown and they haven't acknowledged it yet, we just
 					// want to reject the new HTLC and fail it backwards instead of forwarding.
-					if let PendingHTLCStatus::Forward(PendingHTLCInfo { incoming_shared_secret, .. }) = pending_forward_info {
-						let chan_update = self.get_channel_update(chan.get());
-						pending_forward_info = PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msgs::UpdateFailHTLC {
-							channel_id: msg.channel_id,
-							htlc_id: msg.htlc_id,
-							reason: if let Ok(update) = chan_update {
-								// TODO: Note that |20 is defined as "channel FROM the processing
-								// node has been disabled" (emphasis mine), which seems to imply
-								// that we can't return |20 for an inbound channel being disabled.
-								// This probably needs a spec update but should definitely be
-								// allowed.
-								onion_utils::build_first_hop_failure_packet(&incoming_shared_secret, 0x1000|20, &{
-									let mut res = Vec::with_capacity(8 + 128);
-									res.extend_from_slice(&byte_utils::be16_to_array(update.contents.flags));
-									res.extend_from_slice(&update.encode_with_len()[..]);
-									res
-								}[..])
-							} else {
-								// This can only happen if the channel isn't in the fully-funded
-								// state yet, implying our counterparty is trying to route payments
-								// over the channel back to themselves (cause no one else should
-								// know the short_id is a lightning channel yet). We should have no
-								// problem just calling this unknown_next_peer
-								onion_utils::build_first_hop_failure_packet(&incoming_shared_secret, 0x4000|10, &[])
-							},
-						}));
-					}
-				}
-				let create_onion_error_packet = |chan: &Channel<ChanSigner>, pending_forward_info: &PendingHTLCStatus| {
 					match pending_forward_info {
 						&PendingHTLCStatus::Forward(PendingHTLCInfo { ref incoming_shared_secret, .. }) => {
-							match self.get_channel_update(chan) {
-								Ok(upd) => {
-									Some(onion_utils::build_first_hop_failure_packet(incoming_shared_secret, 0x1000|7, &{
-										let mut res = Vec::with_capacity(8 + 128);
-										res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
-										res.extend_from_slice(&upd.encode_with_len()[..]);
-										res
-									}[..]))
-								},
-								Err(_) => None,
+							let mut reason = onion_utils::build_first_hop_failure_packet(incoming_shared_secret, 0x4000|10, &[]);
+							// The only case where we'd be unable to successfully get a channel
+							// update here is if the channel isn't in the fully-funded
+							// state yet, implying our counterparty is trying to route payments
+							// over the channel back to themselves (cause no one else should
+							// know the short_id is a lightning channel yet). We should have no
+							// problem just calling this unknown_next_peer, as above (0x4000|10).
+							if let Ok(upd) = self.get_channel_update(chan) {
+								reason = onion_utils::build_first_hop_failure_packet(incoming_shared_secret, error_code, &{
+									let mut res = Vec::with_capacity(8 + 128);
+									res.extend_from_slice(&byte_utils::be16_to_array(upd.contents.flags));
+									res.extend_from_slice(&upd.encode_with_len()[..]);
+									res
+								}[..]);
 							}
+							let msg = msgs::UpdateFailHTLC {
+								channel_id: msg.channel_id,
+								htlc_id: msg.htlc_id,
+								reason
+							};
+							Some(PendingHTLCStatus::Fail(HTLCFailureMsg::Relay(msg)))
 						},
-						_ => None,
+						_ => None
 					}
 				};
-        try_chan_entry!(self, chan.get_mut().update_add_htlc(&msg, pending_forward_info, create_onion_error_packet), channel_state, chan);
+        try_chan_entry!(self, chan.get_mut().update_add_htlc(&msg, pending_forward_info, create_pending_htlc_status), channel_state, chan);
 			},
 			hash_map::Entry::Vacant(_) => return Err(MsgHandleErrInternal::send_err_msg_no_close("Failed to find corresponding channel", msg.channel_id))
 		}