Merge pull request #211 from TheBlueMatt/2018-10-misc-cleanups

Miscellaneous Cleanups

Author: TheBlueMatt

src/ln/channel.rs

@@ -1960,10 +1960,12 @@ impl Channel {
 		if !self.channel_outbound {
 			panic!("Cannot send fee from inbound channel");
 		}
-
 		if !self.is_usable() {
 			panic!("Cannot update fee until channel is fully established and we haven't started shutting down");
 		}
+		if !self.is_live() {
+			panic!("Cannot update fee while peer is disconnected (ChannelManager should have caught this)");
+		}
 
 		if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
 			self.holding_cell_update_fee = Some(feerate_per_kw);

src/ln/channelmanager.rs

@@ -449,7 +449,10 @@ impl ChannelManager {
 		let channel_state = self.channel_state.lock().unwrap();
 		let mut res = Vec::with_capacity(channel_state.by_id.len());
 		for (channel_id, channel) in channel_state.by_id.iter() {
-			if channel.is_usable() {
+			// Note we use is_live here instead of usable which leads to somewhat confused
+			// internal/external nomenclature, but that's ok cause that's probably what the user
+			// really wanted anyway.
+			if channel.is_live() {
 				res.push(ChannelDetails {
 					channel_id: (*channel_id).clone(),
 					short_channel_id: channel.get_short_channel_id(),
@@ -997,7 +1000,7 @@ impl ChannelManager {
 		};
 
 		let msg_hash = Sha256dHash::from_data(&unsigned.encode()[..]);
-		let sig = self.secp_ctx.sign(&Message::from_slice(&msg_hash[..]).unwrap(), &self.our_network_key); //TODO Can we unwrap here?
+		let sig = self.secp_ctx.sign(&Message::from_slice(&msg_hash[..]).unwrap(), &self.our_network_key);
 
 		Ok(msgs::ChannelUpdate {
 			signature: sig,
@@ -1050,7 +1053,7 @@ impl ChannelManager {
 			let channel_state = channel_state_lock.borrow_parts();
 
 			let id = match channel_state.short_to_id.get(&route.hops.first().unwrap().short_channel_id) {
-				None => return Err(APIError::RouteError{err: "No channel available with first hop!"}),
+				None => return Err(APIError::ChannelUnavailable{err: "No channel available with first hop!"}),
 				Some(id) => id.clone(),
 			};
 
@@ -1060,12 +1063,12 @@ impl ChannelManager {
 					return Err(APIError::RouteError{err: "Node ID mismatch on first hop!"});
 				}
 				if !chan.is_live() {
-					return Err(APIError::RouteError{err: "Peer for first hop currently disconnected!"});
+					return Err(APIError::ChannelUnavailable{err: "Peer for first hop currently disconnected!"});
 				}
 				chan.send_htlc_and_commit(htlc_msat, payment_hash.clone(), htlc_cltv, HTLCSource::OutboundRoute {
 					route: route.clone(),
 					session_priv: session_priv.clone(),
-				}, onion_packet).map_err(|he| APIError::RouteError{err: he.err})?
+				}, onion_packet).map_err(|he| APIError::ChannelUnavailable{err: he.err})?
 			};
 
 			let first_hop_node_id = route.hops.first().unwrap().pubkey;
@@ -1102,7 +1105,6 @@ impl ChannelManager {
 	/// May panic if the funding_txo is duplicative with some other channel (note that this should
 	/// be trivially prevented by using unique funding transaction keys per-channel).
 	pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_txo: OutPoint) {
-
 		macro_rules! add_pending_event {
 			($event: expr) => {
 				{
@@ -1998,12 +2000,12 @@ impl ChannelManager {
 		match channel_state.by_id.get_mut(&channel_id) {
 			None => return Err(APIError::APIMisuseError{err: "Failed to find corresponding channel"}),
 			Some(chan) => {
-				if !chan.is_usable() {
-					return Err(APIError::APIMisuseError{err: "Channel is not in usuable state"});
-				}
 				if !chan.is_outbound() {
 					return Err(APIError::APIMisuseError{err: "update_fee cannot be sent for an inbound channel"});
 				}
+				if !chan.is_live() {
+					return Err(APIError::ChannelUnavailable{err: "Channel is either not yet fully established or peer is currently disconnected"});
+				}
 				if let Some((update_fee, commitment_signed, chan_monitor)) = chan.send_update_fee_and_commit(feerate_per_kw).map_err(|e| APIError::APIMisuseError{err: e.err})? {
 					if let Err(_e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
 						unimplemented!();
@@ -3025,7 +3027,7 @@ mod tests {
 
 		let err = origin_node.node.send_payment(route, our_payment_hash).err().unwrap();
 		match err {
-			APIError::RouteError{err} => assert_eq!(err, "Cannot send value that would put us over our max HTLC value in flight"),
+			APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our max HTLC value in flight"),
 			_ => panic!("Unknown error variants"),
 		};
 	}
@@ -3989,7 +3991,7 @@ mod tests {
 			assert!(route.hops.iter().rev().skip(1).all(|h| h.fee_msat == feemsat));
 			let err = nodes[0].node.send_payment(route, our_payment_hash).err().unwrap();
 			match err {
-				APIError::RouteError{err} => assert_eq!(err, "Cannot send value that would put us over our max HTLC value in flight"),
+				APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our max HTLC value in flight"),
 				_ => panic!("Unknown error variants"),
 			}
 		}
@@ -4025,7 +4027,7 @@ mod tests {
 			let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value + 1);
 			let err = nodes[0].node.send_payment(route.clone(), our_payment_hash).err().unwrap();
 			match err {
-				APIError::RouteError{err} => assert_eq!(err, "Cannot send value that would put us over our reserve value"),
+				APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our reserve value"),
 				_ => panic!("Unknown error variants"),
 			}
 		}
@@ -4050,7 +4052,7 @@ mod tests {
 		{
 			let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_2 + 1);
 			match nodes[0].node.send_payment(route, our_payment_hash).err().unwrap() {
-				APIError::RouteError{err} => assert_eq!(err, "Cannot send value that would put us over our reserve value"),
+				APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our reserve value"),
 				_ => panic!("Unknown error variants"),
 			}
 		}
@@ -4106,7 +4108,7 @@ mod tests {
 		{
 			let (route, our_payment_hash, _) = get_route_and_payment_hash!(recv_value_22+1);
 			match nodes[0].node.send_payment(route, our_payment_hash).err().unwrap() {
-				APIError::RouteError{err} => assert_eq!(err, "Cannot send value that would put us over our reserve value"),
+				APIError::ChannelUnavailable{err} => assert_eq!(err, "Cannot send value that would put us over our reserve value"),
 				_ => panic!("Unknown error variants"),
 			}
 		}
@@ -4935,6 +4937,10 @@ mod tests {
 			_ => panic!("Unexpected event"),
 		};
 
+		nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+		nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+		reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+
 		nodes[1].node.channel_state.lock().unwrap().next_forward = Instant::now();
 		nodes[1].node.process_pending_htlc_forwards();
 
@@ -5029,6 +5035,10 @@ mod tests {
 			reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
 		}
 
+		nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+		nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
+		reconnect_nodes(&nodes[0], &nodes[1], false, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+
 		// Channel should still work fine...
 		let payment_preimage_2 = send_along_route(&nodes[0], route, &[&nodes[1]], 1000000).0;
 		claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2);
@@ -5079,6 +5089,9 @@ mod tests {
 			_ => panic!("Unexpected event"),
 		}
 
+		reconnect_nodes(&nodes[0], &nodes[1], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
+		nodes[0].node.peer_disconnected(&nodes[1].node.get_our_node_id(), false);
+		nodes[1].node.peer_disconnected(&nodes[0].node.get_our_node_id(), false);
 		reconnect_nodes(&nodes[0], &nodes[1], true, (0, 0), (0, 0), (0, 0), (0, 0), (false, false));
 
 		// TODO: We shouldn't need to manually pass list_usable_chanels here once we support

src/util/errors.rs

@@ -20,16 +20,15 @@ pub enum APIError {
 		/// The feerate which was too high.
 		feerate: u64
 	},
-
-	/// Invalid route or parameters (cltv_delta, fee, pubkey) was specified
+	/// A malformed Route was provided (eg overflowed value, node id mismatch, overly-looped route,
+	/// too-many-hops, etc).
 	RouteError {
 		/// A human-readable error message
 		err: &'static str
 	},
-
-
-	/// We were unable to complete the request since channel is disconnected or
-	/// shutdown in progress initiated by remote
+	/// We were unable to complete the request as the Channel required to do so is unable to
+	/// complete the request (or was not found). This can take many forms, including disconnected
+	/// peer, channel at capacity, channel shutting down, etc.
 	ChannelUnavailable {
 		/// A human-readable error message
 		err: &'static str