Avoid needless on-chain channel failing for timing-out HTLCs
[rust-lightning] / src / ln / channelmanager.rs
index 8bc5bf88d9a9e8ba0e7241e5555fd574beb91932..033fdbd615985d73b58f4fb5695112d6b874a41a 100644 (file)
@@ -23,7 +23,7 @@ use secp256k1;
 use chain::chaininterface::{BroadcasterInterface,ChainListener,ChainWatchInterface,FeeEstimator};
 use chain::transaction::OutPoint;
 use ln::channel::{Channel, ChannelError, ChannelKeys};
-use ln::channelmonitor::ManyChannelMonitor;
+use ln::channelmonitor::{ManyChannelMonitor, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
 use ln::router::{Route,RouteHop};
 use ln::msgs;
 use ln::msgs::{HandleError,ChannelMessageHandler};
@@ -290,8 +290,27 @@ pub struct ChannelManager {
        logger: Arc<Logger>,
 }
 
+/// The minimum number of blocks between an inbound HTLC's CLTV and the corresponding outbound
+/// HTLC's CLTV. This should always be a few blocks greater than channelmonitor::CLTV_CLAIM_BUFFER,
+/// ie the node we forwarded the payment on to should always have enough room to reliably time out
+/// the HTLC via a full update_fail_htlc/commitment_signed dance before we hit the
+/// CLTV_CLAIM_BUFFER point (we static assert that its at least 3 blocks more).
 const CLTV_EXPIRY_DELTA: u16 = 6 * 24 * 2; //TODO?
 
+// Check that our CLTV_EXPIRY is at least CLTV_CLAIM_BUFFER + 2*HTLC_FAIL_TIMEOUT_BLOCKS, ie that
+// if the next-hop peer fails the HTLC within HTLC_FAIL_TIMEOUT_BLOCKS then we'll still have
+// HTLC_FAIL_TIMEOUT_BLOCKS left to fail it backwards ourselves before hitting the
+// CLTV_CLAIM_BUFFER point and failing the channel on-chain to time out the HTLC.
+#[deny(const_err)]
+#[allow(dead_code)]
+const CHECK_CLTV_EXPIRY_SANITY: u32 = CLTV_EXPIRY_DELTA as u32 - 2*HTLC_FAIL_TIMEOUT_BLOCKS - CLTV_CLAIM_BUFFER;
+
+// Check for ability of an attacker to make us fail on-chain by delaying inbound claim. See
+// ChannelMontior::would_broadcast_at_height for a description of why this is needed.
+#[deny(const_err)]
+#[allow(dead_code)]
+const CHECK_CLTV_EXPIRY_SANITY_2: u32 = CLTV_EXPIRY_DELTA as u32 - HTLC_FAIL_TIMEOUT_BLOCKS - 2*CLTV_CLAIM_BUFFER;
+
 macro_rules! secp_call {
        ( $res: expr, $err: expr ) => {
                match $res {
@@ -449,7 +468,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 +1019,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 +1072,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 +1082,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 +1124,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 +2019,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!();
@@ -2350,6 +2371,7 @@ mod tests {
        use chain::transaction::OutPoint;
        use chain::chaininterface::ChainListener;
        use ln::channelmanager::{ChannelManager,OnionKeys};
+       use ln::channelmonitor::{CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
        use ln::router::{Route, RouteHop, Router};
        use ln::msgs;
        use ln::msgs::{ChannelMessageHandler,RoutingMessageHandler};
@@ -2382,6 +2404,7 @@ mod tests {
        use std::default::Default;
        use std::rc::Rc;
        use std::sync::{Arc, Mutex};
+       use std::sync::atomic::Ordering;
        use std::time::Instant;
        use std::mem;
 
@@ -3025,7 +3048,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 +4012,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 +4048,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 +4073,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 +4129,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"),
                        }
                }
@@ -4267,13 +4290,22 @@ mod tests {
                assert_eq!(nodes[2].node.list_channels().len(), 0);
                assert_eq!(nodes[3].node.list_channels().len(), 1);
 
+               { // Cheat and reset nodes[4]'s height to 1
+                       let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+                       nodes[4].chain_monitor.block_connected_with_filtering(&Block { header, txdata: vec![] }, 1);
+               }
+
+               assert_eq!(nodes[3].node.latest_block_height.load(Ordering::Acquire), 1);
+               assert_eq!(nodes[4].node.latest_block_height.load(Ordering::Acquire), 1);
                // One pending HTLC to time out:
                let payment_preimage_2 = route_payment(&nodes[3], &vec!(&nodes[4])[..], 3000000).0;
+               // CLTV expires at TEST_FINAL_CLTV + 1 (current height) + 1 (added in send_payment for
+               // buffer space).
 
                {
                        let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-                       nodes[3].chain_monitor.block_connected_checked(&header, 1, &Vec::new()[..], &[0; 0]);
-                       for i in 2..TEST_FINAL_CLTV - 3 {
+                       nodes[3].chain_monitor.block_connected_checked(&header, 2, &Vec::new()[..], &[0; 0]);
+                       for i in 3..TEST_FINAL_CLTV + 2 + HTLC_FAIL_TIMEOUT_BLOCKS + 1 {
                                header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
                                nodes[3].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]);
                        }
@@ -4284,8 +4316,8 @@ mod tests {
                        claim_funds!(nodes[4], nodes[3], payment_preimage_2);
 
                        header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-                       nodes[4].chain_monitor.block_connected_checked(&header, 1, &Vec::new()[..], &[0; 0]);
-                       for i in 2..TEST_FINAL_CLTV - 3 {
+                       nodes[4].chain_monitor.block_connected_checked(&header, 2, &Vec::new()[..], &[0; 0]);
+                       for i in 3..TEST_FINAL_CLTV + 2 - CLTV_CLAIM_BUFFER + 1 {
                                header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
                                nodes[4].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]);
                        }
@@ -4935,6 +4967,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 +5065,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 +5119,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