diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 5c861b2eae7..1b7db97d3d3 100644 --- a/src/ln/channel.rs +++ b/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); diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 8bc5bf88d9a..c8ed743e1d0 100644 --- a/src/ln/channelmanager.rs +++ b/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 diff --git a/src/util/errors.rs b/src/util/errors.rs index 6513beefe79..9446b35854b 100644 --- a/src/util/errors.rs +++ b/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