Fill out failure codes for onion packet

fuzz/fuzz_targets/router_target.rs

@@ -187,7 +187,7 @@ pub fn do_test(data: &[u8]) {
 					},
 					1 => {
 						let short_channel_id = slice_to_be64(get_slice!(8));
-						router.handle_htlc_fail_channel_update(&msgs::HTLCFailChannelUpdate::ChannelClosed {short_channel_id});
+						router.handle_htlc_fail_channel_update(&msgs::HTLCFailChannelUpdate::ChannelClosed {short_channel_id, is_permanent: false});
 					},
 					_ => return,
 				}
@@ -223,6 +223,7 @@ pub fn do_test(data: &[u8]) {
 							fee_proportional_millionths: slice_to_be32(get_slice!(4)),
 							cltv_expiry_delta: slice_to_be16(get_slice!(2)),
 							htlc_minimum_msat: slice_to_be64(get_slice!(8)),
+							last_update: 0,
 						});
 					}
 					&last_hops_vec[..]

src/ln/channel.rs

@@ -1545,7 +1545,7 @@ impl Channel {
 		Ok(())
 	}
 
-	/// Removes an outbound HTLC which has been commitment_signed by the remote end
+	/// Marks an outbound HTLC which we have received update_fail/fulfill/malformed
 	#[inline]
 	fn mark_outbound_htlc_removed(&mut self, htlc_id: u64, check_preimage: Option<[u8; 32]>, fail_reason: Option<HTLCFailReason>) -> Result<&HTLCSource, ChannelError> {
 		for htlc in self.pending_outbound_htlcs.iter_mut() {
@@ -1559,13 +1559,13 @@ impl Channel {
 				};
 				match htlc.state {
 					OutboundHTLCState::LocalAnnounced(_) =>
-						return Err(ChannelError::Close("Remote tried to fulfill HTLC before it had been committed")),
+						return Err(ChannelError::Close("Remote tried to fulfill/fail HTLC before it had been committed")),
 					OutboundHTLCState::Committed => {
 						htlc.state = OutboundHTLCState::RemoteRemoved;
 						htlc.fail_reason = fail_reason;
 					},
 					OutboundHTLCState::AwaitingRemoteRevokeToRemove | OutboundHTLCState::AwaitingRemovedRemoteRevoke | OutboundHTLCState::RemoteRemoved =>
-						return Err(ChannelError::Close("Remote tried to fulfill HTLC that they'd already fulfilled")),
+						return Err(ChannelError::Close("Remote tried to fulfill/fail HTLC that they'd already fulfilled/failed")),
 				}
 				return Ok(&htlc.source);
 			}
@@ -2450,6 +2450,11 @@ impl Channel {
 		self.our_htlc_minimum_msat
 	}
 
+	/// Allowed in any state (including after shutdown)
+	pub fn get_their_htlc_minimum_msat(&self) -> u64 {
+		self.our_htlc_minimum_msat
+	}
+
 	pub fn get_value_satoshis(&self) -> u64 {
 		self.channel_value_satoshis
 	}

src/ln/channelmonitor.rs

@@ -155,7 +155,13 @@ impl ManyChannelMonitor for SimpleManyChannelMonitor<OutPoint> {
 const CLTV_SHARED_CLAIM_BUFFER: u32 = 12;
 /// If an HTLC expires within this many blocks, force-close the channel to broadcast the
 /// HTLC-Success transaction.
-const CLTV_CLAIM_BUFFER: u32 = 6;
+/// In other words, this is an upper bound on how many blocks we think it can take us to get a
+/// transaction confirmed (and we use it in a few more, equivalent, places).
+pub(crate) const CLTV_CLAIM_BUFFER: u32 = 6;
+/// Number of blocks by which point we expect our counterparty to have seen new blocks on the
+/// network and done a full update_fail_htlc/commitment_signed dance (+ we've updated all our
+/// copies of ChannelMonitors, including watchtowers).
+pub(crate) const HTLC_FAIL_TIMEOUT_BLOCKS: u32 = 3;
 
 #[derive(Clone, PartialEq)]
 enum KeyStorage {
@@ -1184,16 +1190,7 @@ impl ChannelMonitor {
 			}
 		}
 		if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx {
-			let mut needs_broadcast = false;
-			for &(ref htlc, _, _) in cur_local_tx.htlc_outputs.iter() {
-				if htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER {
-					if htlc.offered || self.payment_preimages.contains_key(&htlc.payment_hash) {
-						needs_broadcast = true;
-					}
-				}
-			}
-
-			if needs_broadcast {
+			if self.would_broadcast_at_height(height) {
 				broadcaster.broadcast_transaction(&cur_local_tx.tx);
 				for tx in self.broadcast_by_local_state(&cur_local_tx) {
 					broadcaster.broadcast_transaction(&tx);
@@ -1206,10 +1203,29 @@ impl ChannelMonitor {
 	pub(super) fn would_broadcast_at_height(&self, height: u32) -> bool {
 		if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx {
 			for &(ref htlc, _, _) in cur_local_tx.htlc_outputs.iter() {
-				if htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER {
-					if htlc.offered || self.payment_preimages.contains_key(&htlc.payment_hash) {
-						return true;
-					}
+				// For inbound HTLCs which we know the preimage for, we have to ensure we hit the
+				// chain with enough room to claim the HTLC without our counterparty being able to
+				// time out the HTLC first.
+				// For outbound HTLCs which our counterparty hasn't failed/claimed, our primary
+				// concern is being able to claim the corresponding inbound HTLC (on another
+				// channel) before it expires. In fact, we don't even really care if our
+				// counterparty here claims such an outbound HTLC after it expired as long as we
+				// can still claim the corresponding HTLC. Thus, to avoid needlessly hitting the
+				// chain when our counterparty is waiting for expiration to off-chain fail an HTLC
+				// we give ourselves a few blocks of headroom after expiration before going
+				// on-chain for an expired HTLC.
+				// Note that, to avoid a potential attack whereby a node delays claiming an HTLC
+				// from us until we've reached the point where we go on-chain with the
+				// corresponding inbound HTLC, we must ensure that outbound HTLCs go on chain at
+				// least CLTV_CLAIM_BUFFER blocks prior to the inbound HTLC.
+				//  aka outbound_cltv + HTLC_FAIL_TIMEOUT_BLOCKS == height - CLTV_CLAIM_BUFFER
+				//      inbound_cltv == height + CLTV_CLAIM_BUFFER
+				//      outbound_cltv + HTLC_FAIL_TIMEOUT_BLOCKS + CLTV_CLAIM_BUFER <= inbound_cltv - CLTV_CLAIM_BUFFER
+				//      HTLC_FAIL_TIMEOUT_BLOCKS + 2*CLTV_CLAIM_BUFER <= inbound_cltv - outbound_cltv
+				//      HTLC_FAIL_TIMEOUT_BLOCKS + 2*CLTV_CLAIM_BUFER <= CLTV_EXPIRY_DELTA
+				if ( htlc.offered && htlc.cltv_expiry + HTLC_FAIL_TIMEOUT_BLOCKS <= height) ||
+				   (!htlc.offered && htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER && self.payment_preimages.contains_key(&htlc.payment_hash)) {
+					return true;
 				}
 			}
 		}

src/ln/msgs.rs

@@ -485,7 +485,19 @@ pub enum HTLCFailChannelUpdate {
 	ChannelClosed {
 		/// The short_channel_id which has now closed.
 		short_channel_id: u64,
+		/// when this true, this channel should be permanently removed from the
+		/// consideration. Otherwise, this channel can be restored as new channel_update is received
+		is_permanent: bool,
 	},
+	/// We received an error which indicated only that a node has failed
+	NodeFailure {
+		/// The node_id that has failed.
+		node_id: PublicKey,
+		/// when this true, node should be permanently removed from the
+		/// consideration. Otherwise, the channels connected to this node can be
+		/// restored as new channel_update is received
+		is_permanent: bool,
+	}
 }
 
 /// A trait to describe an object which can receive channel messages.
@@ -517,7 +529,7 @@ pub trait ChannelMessageHandler : events::EventsProvider + Send + Sync {
 	/// Handle an incoming update_fulfill_htlc message from the given peer.
 	fn handle_update_fulfill_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFulfillHTLC) -> Result<(), HandleError>;
 	/// Handle an incoming update_fail_htlc message from the given peer.
-	fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailHTLC) -> Result<Option<HTLCFailChannelUpdate>, HandleError>;
+	fn handle_update_fail_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailHTLC) -> Result<(), HandleError>;
 	/// Handle an incoming update_fail_malformed_htlc message from the given peer.
 	fn handle_update_fail_malformed_htlc(&self, their_node_id: &PublicKey, msg: &UpdateFailMalformedHTLC) -> Result<(), HandleError>;
 	/// Handle an incoming commitment_signed message from the given peer.

src/ln/peer_handler.rs

@@ -615,10 +615,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 											},
 											131 => {
 												let msg = try_potential_decodeerror!(msgs::UpdateFailHTLC::read(&mut reader));
-												let chan_update = try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fail_htlc(&peer.their_node_id.unwrap(), &msg));
-												if let Some(update) = chan_update {
-													self.message_handler.route_handler.handle_htlc_fail_channel_update(&update);
-												}
+												try_potential_handleerror!(self.message_handler.chan_handler.handle_update_fail_htlc(&peer.their_node_id.unwrap(), &msg));
 											},
 											135 => {
 												let msg = try_potential_decodeerror!(msgs::UpdateFailMalformedHTLC::read(&mut reader));
@@ -906,6 +903,10 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 						}
 						continue;
 					},
+					Event::RouteUpdate { ref update } => {
+						self.message_handler.route_handler.handle_htlc_fail_channel_update(update);
+						continue;
+					},
 					Event::HandleError { ref node_id, ref action } => {
 						if let Some(ref action) = *action {
 							match *action {

src/ln/router.rs

@@ -36,6 +36,9 @@ pub struct RouteHop {
 	/// The CLTV delta added for this hop. For the last hop, this should be the full CLTV value
 	/// expected at the destination, in excess of the current block height.
 	pub cltv_expiry_delta: u32,
+	/// channel update timestamp of the outbound channel of each hop when this route is
+	/// calculated
+	pub channel_update_timestamp: u32,
 }
 
 /// A route from us through the network to a destination
@@ -168,6 +171,8 @@ impl NetworkMap {
 pub struct RouteHint {
 	/// The node_id of the non-target end of the route
 	pub src_node_id: PublicKey,
+	/// last update
+	pub last_update: u32,
 	/// The short_channel_id of this channel
 	pub short_channel_id: u64,
 	/// The static msat-denominated fee which must be paid to use this channel
@@ -349,12 +354,17 @@ impl RoutingMessageHandler for Router {
 			&msgs::HTLCFailChannelUpdate::ChannelUpdateMessage { ref msg } => {
 				let _ = self.handle_channel_update(msg);
 			},
-			&msgs::HTLCFailChannelUpdate::ChannelClosed { ref short_channel_id } => {
+			&msgs::HTLCFailChannelUpdate::ChannelClosed { ref short_channel_id, is_permanent:_ } => {
 				let mut network = self.network_map.write().unwrap();
 				if let Some(chan) = network.channels.remove(short_channel_id) {
 					Self::remove_channel_in_nodes(&mut network.nodes, &chan, *short_channel_id);
 				}
 			},
+			&msgs::HTLCFailChannelUpdate::NodeFailure { ref node_id, is_permanent:_ } => {
+				//let mut network = self.network_map.write().unwrap();
+				//TODO: check _blamed_upstream_node
+				self.mark_node_bad(node_id, false);
+			},
 		}
 	}
 
@@ -450,6 +460,7 @@ impl cmp::PartialOrd for RouteGraphNode {
 
 struct DummyDirectionalChannelInfo {
 	src_node_id: PublicKey,
+	last_update: u32,
 	cltv_expiry_delta: u32,
 	htlc_minimum_msat: u64,
 	fee_base_msat: u32,
@@ -560,6 +571,7 @@ impl Router {
 
 		let dummy_directional_info = DummyDirectionalChannelInfo { // used for first_hops routes
 			src_node_id: network.our_node_id.clone(),
+			last_update: 0,
 			cltv_expiry_delta: 0,
 			htlc_minimum_msat: 0,
 			fee_base_msat: 0,
@@ -580,6 +592,7 @@ impl Router {
 							short_channel_id,
 							fee_msat: final_value_msat,
 							cltv_expiry_delta: final_cltv,
+							channel_update_timestamp: 0, //TODO: related to local channel_update_handling?
 						}],
 					});
 				}
@@ -613,6 +626,7 @@ impl Router {
 									short_channel_id: 0,
 									fee_msat: 0,
 									cltv_expiry_delta: 0,
+									channel_update_timestamp: $directional_info.last_update,
 							})
 						});
 						if $directional_info.src_node_id != network.our_node_id {
@@ -639,6 +653,7 @@ impl Router {
 								short_channel_id: $chan_id.clone(),
 								fee_msat: new_fee, // This field is ignored on the last-hop anyway
 								cltv_expiry_delta: $directional_info.cltv_expiry_delta as u32,
+								channel_update_timestamp: $directional_info.last_update,
 							}
 						}
 					}
@@ -1161,20 +1176,23 @@ mod tests {
 		let mut last_hops = vec!(RouteHint {
 				src_node_id: node4.clone(),
 				short_channel_id: 8,
+				last_update: 0,
 				fee_base_msat: 0,
 				fee_proportional_millionths: 0,
 				cltv_expiry_delta: (8 << 8) | 1,
 				htlc_minimum_msat: 0,
 			}, RouteHint {
 				src_node_id: node5.clone(),
 				short_channel_id: 9,
+				last_update: 0,
 				fee_base_msat: 1001,
 				fee_proportional_millionths: 0,
 				cltv_expiry_delta: (9 << 8) | 1,
 				htlc_minimum_msat: 0,
 			}, RouteHint {
 				src_node_id: node6.clone(),
 				short_channel_id: 10,
+				last_update: 0,
 				fee_base_msat: 0,
 				fee_proportional_millionths: 0,
 				cltv_expiry_delta: (10 << 8) | 1,

src/util/events.rs

@@ -75,6 +75,10 @@ pub enum Event {
 	PaymentFailed {
 		/// The hash which was given to ChannelManager::send_payment.
 		payment_hash: [u8; 32],
+		/// Inindicates if the payment can be retried.
+		retryable: bool,
+		/// Error code from Onion failure message
+		error_code: Option<u16>,
 	},
 	/// Used to indicate that ChannelManager::process_pending_htlc_forwards should be called at a
 	/// time in the future.
@@ -161,6 +165,13 @@ pub enum Event {
 		node_id: PublicKey,
 		/// The action which should be taken.
 		action: Option<msgs::ErrorAction>
+	},
+
+	/// Used to indicate a change should be made into network map
+	///
+	RouteUpdate {
+		/// The channel/node update which should be sent to router
+		update: msgs::HTLCFailChannelUpdate,
 	}
 }
 

src/util/test_utils.rs

@@ -110,7 +110,7 @@ impl msgs::ChannelMessageHandler for TestChannelMessageHandler {
 	fn handle_update_fulfill_htlc(&self, _their_node_id: &PublicKey, _msg: &msgs::UpdateFulfillHTLC) -> Result<(), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
-	fn handle_update_fail_htlc(&self, _their_node_id: &PublicKey, _msg: &msgs::UpdateFailHTLC) -> Result<Option<msgs::HTLCFailChannelUpdate>, HandleError> {
+	fn handle_update_fail_htlc(&self, _their_node_id: &PublicKey, _msg: &msgs::UpdateFailHTLC) -> Result<(), HandleError> {
 		Err(HandleError { err: "", action: None })
 	}
 	fn handle_update_fail_malformed_htlc(&self, _their_node_id: &PublicKey, _msg: &msgs::UpdateFailMalformedHTLC) -> Result<(), HandleError> {