Skip to content

Commit d2955be

Browse files
authored
Merge pull request #911 from TheBlueMatt/2021-05-fix-cltv-diff
2 parents 85f1a91 + 0ba727a commit d2955be

File tree

6 files changed

+36
-28
lines changed

6 files changed

+36
-28
lines changed

fuzz/src/full_stack.rs

Lines changed: 11 additions & 12 deletions
Large diffs are not rendered by default.

lightning-invoice/src/utils.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ where
9292
mod test {
9393
use {Currency, Description, InvoiceDescription};
9494
use lightning::ln::PaymentHash;
95+
use lightning::ln::channelmanager::MIN_FINAL_CLTV_EXPIRY;
9596
use lightning::ln::functional_test_utils::*;
9697
use lightning::ln::features::InitFeatures;
9798
use lightning::ln::msgs::ChannelMessageHandler;
@@ -107,7 +108,7 @@ mod test {
107108
let _chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
108109
let invoice = ::utils::create_invoice_from_channelmanager(&nodes[1].node, nodes[1].keys_manager, Currency::BitcoinTestnet, Some(10_000), "test".to_string()).unwrap();
109110
assert_eq!(invoice.amount_pico_btc(), Some(100_000));
110-
assert_eq!(invoice.min_final_cltv_expiry(), 9);
111+
assert_eq!(invoice.min_final_cltv_expiry(), MIN_FINAL_CLTV_EXPIRY as u64);
111112
assert_eq!(invoice.description(), InvoiceDescription::Direct(&Description("test".to_string())));
112113

113114
let mut route_hints = invoice.routes().clone();

lightning/src/chain/channelmonitor.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ pub(crate) const CLTV_SHARED_CLAIM_BUFFER: u32 = 12;
206206
/// HTLC-Success transaction.
207207
/// In other words, this is an upper bound on how many blocks we think it can take us to get a
208208
/// transaction confirmed (and we use it in a few more, equivalent, places).
209-
pub(crate) const CLTV_CLAIM_BUFFER: u32 = 6;
209+
pub(crate) const CLTV_CLAIM_BUFFER: u32 = 18;
210210
/// Number of blocks by which point we expect our counterparty to have seen new blocks on the
211211
/// network and done a full update_fail_htlc/commitment_signed dance (+ we've updated all our
212212
/// copies of ChannelMonitors, including watchtowers). We could enforce the contract by failing

lightning/src/ln/channelmanager.rs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -563,7 +563,7 @@ pub const BREAKDOWN_TIMEOUT: u16 = 6 * 24;
563563
pub(crate) const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 2 * 6 * 24 * 7;
564564

565565
/// The minimum number of blocks between an inbound HTLC's CLTV and the corresponding outbound
566-
/// HTLC's CLTV. The current default represents roughly six hours of blocks at six blocks/hour.
566+
/// HTLC's CLTV. The current default represents roughly seven hours of blocks at six blocks/hour.
567567
///
568568
/// This can be increased (but not decreased) through [`ChannelConfig::cltv_expiry_delta`]
569569
///
@@ -572,13 +572,16 @@ pub(crate) const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 2 * 6 * 24 * 7;
572572
// i.e. the node we forwarded the payment on to should always have enough room to reliably time out
573573
// the HTLC via a full update_fail_htlc/commitment_signed dance before we hit the
574574
// CLTV_CLAIM_BUFFER point (we static assert that it's at least 3 blocks more).
575-
pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6 * 6;
575+
pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6*7;
576576
pub(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?
577577

578578
/// Minimum CLTV difference between the current block height and received inbound payments.
579579
/// Invoices generated for payment to us must set their `min_final_cltv_expiry` field to at least
580580
/// this value.
581-
pub const MIN_FINAL_CLTV_EXPIRY: u32 = HTLC_FAIL_BACK_BUFFER;
581+
// Note that we fail if exactly HTLC_FAIL_BACK_BUFFER + 1 was used, so we need to add one for
582+
// any payments to succeed. Further, we don't want payments to fail if a block was found while
583+
// a payment was being routed, so we add an extra block to be safe.
584+
pub const MIN_FINAL_CLTV_EXPIRY: u32 = HTLC_FAIL_BACK_BUFFER + 3;
582585

583586
// Check that our CLTV_EXPIRY is at least CLTV_CLAIM_BUFFER + ANTI_REORG_DELAY + LATENCY_GRACE_PERIOD_BLOCKS,
584587
// ie that if the next-hop peer fails the HTLC within
@@ -590,7 +593,7 @@ pub const MIN_FINAL_CLTV_EXPIRY: u32 = HTLC_FAIL_BACK_BUFFER;
590593
#[allow(dead_code)]
591594
const CHECK_CLTV_EXPIRY_SANITY: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - CLTV_CLAIM_BUFFER - ANTI_REORG_DELAY - LATENCY_GRACE_PERIOD_BLOCKS;
592595

593-
// Check for ability of an attacker to make us fail on-chain by delaying inbound claim. See
596+
// Check for ability of an attacker to make us fail on-chain by delaying an HTLC claim. See
594597
// ChannelMontior::would_broadcast_at_height for a description of why this is needed.
595598
#[deny(const_err)]
596599
#[allow(dead_code)]

lightning/src/ln/functional_test_utils.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1144,7 +1144,7 @@ pub fn claim_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route:
11441144
claim_payment_along_route(origin_node, &[expected_route], false, our_payment_preimage);
11451145
}
11461146

1147-
pub const TEST_FINAL_CLTV: u32 = 50;
1147+
pub const TEST_FINAL_CLTV: u32 = 70;
11481148

11491149
pub fn route_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&Node<'a, 'b, 'c>], recv_value: u64) -> (PaymentPreimage, PaymentHash, PaymentSecret) {
11501150
let net_graph_msg_handler = &origin_node.net_graph_msg_handler;

lightning/src/ln/functional_tests.rs

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4038,7 +4038,8 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) {
40384038
};
40394039
connect_block(&nodes[0], &block);
40404040
connect_block(&nodes[1], &block);
4041-
for _ in CHAN_CONFIRM_DEPTH + 2 ..TEST_FINAL_CLTV + CHAN_CONFIRM_DEPTH + 2 - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS {
4041+
let block_count = TEST_FINAL_CLTV + CHAN_CONFIRM_DEPTH + 2 - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS;
4042+
for _ in CHAN_CONFIRM_DEPTH + 2..block_count {
40424043
block.header.prev_blockhash = block.block_hash();
40434044
connect_block(&nodes[0], &block);
40444045
connect_block(&nodes[1], &block);
@@ -4055,9 +4056,9 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) {
40554056

40564057
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_timeout_updates.update_fail_htlcs[0]);
40574058
commitment_signed_dance!(nodes[0], nodes[1], htlc_timeout_updates.commitment_signed, false);
4058-
// 100_000 msat as u64, followed by a height of TEST_FINAL_CLTV + 2 as u32
4059+
// 100_000 msat as u64, followed by the height at which we failed back above
40594060
let mut expected_failure_data = byte_utils::be64_to_array(100_000).to_vec();
4060-
expected_failure_data.extend_from_slice(&byte_utils::be32_to_array(TEST_FINAL_CLTV + 2));
4061+
expected_failure_data.extend_from_slice(&byte_utils::be32_to_array(block_count - 1));
40614062
expect_payment_failed!(nodes[0], our_payment_hash, true, 0x4000 | 15, &expected_failure_data[..]);
40624063
}
40634064

@@ -5182,7 +5183,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
51825183
let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
51835184
assert!(htlc_updates.update_add_htlcs.is_empty());
51845185
assert_eq!(htlc_updates.update_fail_htlcs.len(), 1);
5185-
assert_eq!(htlc_updates.update_fail_htlcs[0].htlc_id, 1);
5186+
let first_htlc_id = htlc_updates.update_fail_htlcs[0].htlc_id;
51865187
assert!(htlc_updates.update_fulfill_htlcs.is_empty());
51875188
assert!(htlc_updates.update_fail_malformed_htlcs.is_empty());
51885189
check_added_monitors!(nodes[1], 1);
@@ -5207,7 +5208,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
52075208
assert!(updates.update_add_htlcs.is_empty());
52085209
assert!(updates.update_fail_htlcs.is_empty());
52095210
assert_eq!(updates.update_fulfill_htlcs.len(), 1);
5210-
assert_eq!(updates.update_fulfill_htlcs[0].htlc_id, 0);
5211+
assert_ne!(updates.update_fulfill_htlcs[0].htlc_id, first_htlc_id);
52115212
assert!(updates.update_fail_malformed_htlcs.is_empty());
52125213
check_added_monitors!(nodes[1], 1);
52135214

@@ -7743,9 +7744,13 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
77437744
let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
77447745

77457746
let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 59000000, InitFeatures::known(), InitFeatures::known());
7746-
// Lock HTLC in both directions
7747-
let payment_preimage = route_payment(&nodes[0], &vec!(&nodes[1])[..], 3_000_000).0;
7748-
route_payment(&nodes[1], &vec!(&nodes[0])[..], 3_000_000).0;
7747+
// Lock HTLC in both directions (using a slightly lower CLTV delay to provide timely RBF bumps)
7748+
let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(),
7749+
&nodes[1].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 3_000_000, 50, nodes[0].logger).unwrap();
7750+
let payment_preimage = send_along_route(&nodes[0], route, &[&nodes[1]], 3_000_000).0;
7751+
let route = get_route(&nodes[1].node.get_our_node_id(), &nodes[1].net_graph_msg_handler.network_graph.read().unwrap(),
7752+
&nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 3_000_000, 50, nodes[0].logger).unwrap();
7753+
send_along_route(&nodes[1], route, &[&nodes[0]], 3_000_000);
77497754

77507755
let revoked_local_txn = get_local_commitment_txn!(nodes[1], chan.2);
77517756
assert_eq!(revoked_local_txn[0].input.len(), 1);
@@ -8058,7 +8063,7 @@ fn test_bump_txn_sanitize_tracking_maps() {
80588063
claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage);
80598064

80608065
// Broadcast set of revoked txn on A
8061-
connect_blocks(&nodes[0], 52 - CHAN_CONFIRM_DEPTH);
8066+
connect_blocks(&nodes[0], TEST_FINAL_CLTV + 2 - CHAN_CONFIRM_DEPTH);
80628067
expect_pending_htlcs_forwardable_ignore!(nodes[0]);
80638068
assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 0);
80648069

0 commit comments

Comments
 (0)