Avoid needless on-chain channel failing for timing-out HTLCs
authorMatt Corallo <git@bluematt.me>
Tue, 16 Oct 2018 15:40:21 +0000 (11:40 -0400)
committerMatt Corallo <git@bluematt.me>
Mon, 22 Oct 2018 15:49:57 +0000 (11:49 -0400)
See new comments in code for more details

src/ln/channelmanager.rs
src/ln/channelmonitor.rs

index c8ed743e1d04b4232ea21e01b264974aec1a27f1..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 {
@@ -2352,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};
@@ -2384,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;
 
@@ -4269,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]);
                        }
@@ -4286,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]);
                        }
index 8d6d23ad00a160b6342eade2dce0fffbf33ca06c..59ed177b38fb3da0743e4a2ee71332f0e9abc1b0 100644 (file)
@@ -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;
                                }
                        }
                }