Skip to content

Commit ccf8684

Browse files
TheBlueMattTheBlueMatt
committed
Avoid needless on-chain channel failing for timing-out HTLCs
See new comments in code for more details
1 parent 37d1dac commit ccf8684

File tree

2 files changed

+62
-20
lines changed

2 files changed

+62
-20
lines changed

‎src/ln/channelmanager.rs

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use secp256k1;
2323
use chain::chaininterface::{BroadcasterInterface,ChainListener,ChainWatchInterface,FeeEstimator};
2424
use chain::transaction::OutPoint;
2525
use ln::channel::{Channel, ChannelError, ChannelKeys};
26-
use ln::channelmonitor::ManyChannelMonitor;
26+
use ln::channelmonitor::{ManyChannelMonitor, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
2727
use ln::router::{Route,RouteHop};
2828
use ln::msgs;
2929
use ln::msgs::{HandleError,ChannelMessageHandler};
@@ -300,7 +300,27 @@ pub struct ChannelManager {
300300
logger: Arc<Logger>,
301301
}
302302

303+
/// The minimum number of blocks between an inbound HTLC's CLTV and the corresponding outbound
304+
/// HTLC's CLTV. This should always be a few blocks greater than channelmonitor::CLTV_CLAIM_BUFFER,
305+
/// ie the node we forwarded the payment on to should always have enough room to reliably time out
306+
/// the HTLC via a full update_fail_htlc/commitment_signed dance before we hit the
307+
/// CLTV_CLAIM_BUFFER point (we static assert that its at least 3 blocks more).
303308
const CLTV_EXPIRY_DELTA: u16 = 6 * 24 * 2; //TODO?
309+
310+
// Check that our CLTV_EXPIRY is at least CLTV_CLAIM_BUFFER + 2*HTLC_FAIL_TIMEOUT_BLOCKS, ie that
311+
// if the next-hop peer fails the HTLC within HTLC_FAIL_TIMEOUT_BLOCKS then we'll still have
312+
// HTLC_FAIL_TIMEOUT_BLOCKS left to fail it backwards ourselves before hitting the
313+
// CLTV_CLAIM_BUFFER point and failing the channel on-chain to time out the HTLC.
314+
#[deny(const_err)]
315+
#[allow(dead_code)]
316+
const CHECK_CLTV_EXPIRY_SANITY: u32 = CLTV_EXPIRY_DELTA as u32 - 2*HTLC_FAIL_TIMEOUT_BLOCKS - CLTV_CLAIM_BUFFER;
317+
318+
// Check for ability of an attacker to make us fail on-chain by delaying inbound claim. See
319+
// ChannelMontior::would_broadcast_at_height for a description of why this is needed.
320+
#[deny(const_err)]
321+
#[allow(dead_code)]
322+
const CHECK_CLTV_EXPIRY_SANITY_2: u32 = CLTV_EXPIRY_DELTA as u32 - HTLC_FAIL_TIMEOUT_BLOCKS - 2*CLTV_CLAIM_BUFFER;
323+
304324
const CLTV_FAR_FAR_AWAY: u16 = 6 * 24 * 7; //TODO?
305325
const FINAL_NODE_TIMEOUT: u16 = 3; //TODO?
306326

@@ -2542,6 +2562,7 @@ mod tests {
25422562
use chain::transaction::OutPoint;
25432563
use chain::chaininterface::ChainListener;
25442564
use ln::channelmanager::{ChannelManager,OnionKeys};
2565+
use ln::channelmonitor::{CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS};
25452566
use ln::router::{Route, RouteHop, Router};
25462567
use ln::msgs;
25472568
use ln::msgs::{ChannelMessageHandler,RoutingMessageHandler};
@@ -2574,6 +2595,7 @@ mod tests {
25742595
use std::default::Default;
25752596
use std::rc::Rc;
25762597
use std::sync::{Arc, Mutex};
2598+
use std::sync::atomic::Ordering;
25772599
use std::time::Instant;
25782600
use std::mem;
25792601

@@ -4446,13 +4468,17 @@ mod tests {
44464468
assert_eq!(nodes[2].node.list_channels().len(), 0);
44474469
assert_eq!(nodes[3].node.list_channels().len(), 1);
44484470

4471+
assert_eq!(nodes[3].node.latest_block_height.load(Ordering::Acquire), 1);
4472+
assert_eq!(nodes[4].node.latest_block_height.load(Ordering::Acquire), 1);
44494473
// One pending HTLC to time out:
44504474
let payment_preimage_2 = route_payment(&nodes[3], &vec!(&nodes[4])[..], 3000000).0;
4475+
// CLTV expires at TEST_FINAL_CLTV + 1 (current height) + 1 (added in send_payment for
4476+
// buffer space).
44514477

44524478
{
44534479
let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
4454-
nodes[3].chain_monitor.block_connected_checked(&header, 1, &Vec::new()[..], &[0; 0]);
4455-
for i in 2..TEST_FINAL_CLTV - 3 {
4480+
nodes[3].chain_monitor.block_connected_checked(&header, 2, &Vec::new()[..], &[0; 0]);
4481+
for i in 3..TEST_FINAL_CLTV + 2 + HTLC_FAIL_TIMEOUT_BLOCKS + 1 {
44564482
header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
44574483
nodes[3].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]);
44584484
}
@@ -4463,8 +4489,8 @@ mod tests {
44634489
claim_funds!(nodes[4], nodes[3], payment_preimage_2);
44644490

44654491
header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
4466-
nodes[4].chain_monitor.block_connected_checked(&header, 1, &Vec::new()[..], &[0; 0]);
4467-
for i in 2..TEST_FINAL_CLTV - 3 {
4492+
nodes[4].chain_monitor.block_connected_checked(&header, 2, &Vec::new()[..], &[0; 0]);
4493+
for i in 3..TEST_FINAL_CLTV + 2 - CLTV_CLAIM_BUFFER + 1 {
44684494
header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
44694495
nodes[4].chain_monitor.block_connected_checked(&header, i, &Vec::new()[..], &[0; 0]);
44704496
}

‎src/ln/channelmonitor.rs

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,13 @@ impl ManyChannelMonitor for SimpleManyChannelMonitor<OutPoint> {
155155
const CLTV_SHARED_CLAIM_BUFFER: u32 = 12;
156156
/// If an HTLC expires within this many blocks, force-close the channel to broadcast the
157157
/// HTLC-Success transaction.
158-
const CLTV_CLAIM_BUFFER: u32 = 6;
158+
/// In other words, this is an upper bound on how many blocks we think it can take us to get a
159+
/// transaction confirmed (and we use it in a few more, equivalent, places).
160+
pub(crate) const CLTV_CLAIM_BUFFER: u32 = 6;
161+
/// Number of blocks by which point we expect our counterparty to have seen new blocks on the
162+
/// network and done a full update_fail_htlc/commitment_signed dance (+ we've updated all our
163+
/// copies of ChannelMonitors, including watchtowers).
164+
pub(crate) const HTLC_FAIL_TIMEOUT_BLOCKS: u32 = 3;
159165

160166
#[derive(Clone, PartialEq)]
161167
enum KeyStorage {
@@ -1184,16 +1190,7 @@ impl ChannelMonitor {
11841190
}
11851191
}
11861192
if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx {
1187-
let mut needs_broadcast = false;
1188-
for &(ref htlc, _, _) in cur_local_tx.htlc_outputs.iter() {
1189-
if htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER {
1190-
if htlc.offered || self.payment_preimages.contains_key(&htlc.payment_hash) {
1191-
needs_broadcast = true;
1192-
}
1193-
}
1194-
}
1195-
1196-
if needs_broadcast {
1193+
if self.would_broadcast_at_height(height) {
11971194
broadcaster.broadcast_transaction(&cur_local_tx.tx);
11981195
for tx in self.broadcast_by_local_state(&cur_local_tx) {
11991196
broadcaster.broadcast_transaction(&tx);
@@ -1206,10 +1203,29 @@ impl ChannelMonitor {
12061203
pub(super) fn would_broadcast_at_height(&self, height: u32) -> bool {
12071204
if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx {
12081205
for &(ref htlc, _, _) in cur_local_tx.htlc_outputs.iter() {
1209-
if htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER {
1210-
if htlc.offered || self.payment_preimages.contains_key(&htlc.payment_hash) {
1211-
return true;
1212-
}
1206+
// For inbound HTLCs which we know the preimage for, we have to ensure we hit the
1207+
// chain with enough room to claim the HTLC without our counterparty being able to
1208+
// time out the HTLC first.
1209+
// For outbound HTLCs which our counterparty hasn't failed/claimed, our primary
1210+
// concern is being able to claim the corresponding inbound HTLC (on another
1211+
// channel) before it expires. In fact, we don't even really care if our
1212+
// counterparty here claims such an outbound HTLC after it expired as long as we
1213+
// can still claim the corresponding HTLC. Thus, to avoid needlessly hitting the
1214+
// chain when our counterparty is waiting for expiration to off-chain fail an HTLC
1215+
// we give ourselves a few blocks of headroom after expiration before going
1216+
// on-chain for an expired HTLC.
1217+
// Note that, to avoid a potential attack whereby a node delays claiming an HTLC
1218+
// from us until we've reached the point where we go on-chain with the
1219+
// corresponding inbound HTLC, we must ensure that outbound HTLCs go on chain at
1220+
// least CLTV_CLAIM_BUFFER blocks prior to the inbound HTLC.
1221+
// aka outbound_cltv + HTLC_FAIL_TIMEOUT_BLOCKS == height - CLTV_CLAIM_BUFFER
1222+
// inbound_cltv == height + CLTV_CLAIM_BUFFER
1223+
// outbound_cltv + HTLC_FAIL_TIMEOUT_BLOCKS + CLTV_CLAIM_BUFER <= inbound_cltv - CLTV_CLAIM_BUFFER
1224+
// HTLC_FAIL_TIMEOUT_BLOCKS + 2*CLTV_CLAIM_BUFER <= inbound_cltv - outbound_cltv
1225+
// HTLC_FAIL_TIMEOUT_BLOCKS + 2*CLTV_CLAIM_BUFER <= CLTV_EXPIRY_DELTA
1226+
if ( htlc.offered && htlc.cltv_expiry + HTLC_FAIL_TIMEOUT_BLOCKS <= height) ||
1227+
(!htlc.offered && htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER && self.payment_preimages.contains_key(&htlc.payment_hash)) {
1228+
return true;
12131229
}
12141230
}
12151231
}

0 commit comments

Comments
 (0)