Merge pull request #219 from TheBlueMatt/2018-10-157-merge

Partially implement more onion error handling for sent payments

Author: TheBlueMatt

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,
 				}

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/channelmanager.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/channelmonitor.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/msgs.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::PaymentFailureNetworkUpdate { 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/peer_handler.rs

@@ -349,12 +349,19 @@ 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:_ } => {
+//XXX
 				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:_ } => {
+//XXX
+				//let mut network = self.network_map.write().unwrap();
+				//TODO: check _blamed_upstream_node
+				self.mark_node_bad(node_id, false);
+			},
 		}
 	}
 

src/ln/router.rs

@@ -75,6 +75,10 @@ pub enum Event {
 	PaymentFailed {
 		/// The hash which was given to ChannelManager::send_payment.
 		payment_hash: [u8; 32],
+		/// Indicates the payment was rejected for some reason by the recipient. This implies that
+		/// the payment has failed, not just the route in question. If this is not set, you may
+		/// retry the payment via a different route.
+		rejected_by_dest: bool,
 	},
 	/// Used to indicate that ChannelManager::process_pending_htlc_forwards should be called at a
 	/// time in the future.
@@ -161,6 +165,14 @@ pub enum Event {
 		node_id: PublicKey,
 		/// The action which should be taken.
 		action: Option<msgs::ErrorAction>
+	},
+	/// When a payment fails we may receive updates back from the hop where it failed. In such
+	/// cases this event is generated so that we can inform the router of this information.
+	///
+	/// This event is handled by PeerManager::process_events if you are using a PeerManager.
+	PaymentFailureNetworkUpdate {
+		/// The channel/node update which should be sent to router
+		update: msgs::HTLCFailChannelUpdate,
 	}
 }
 

src/util/events.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> {