Increase the CLTV delay required on payments and forwards

This increases the CLTV_CLAIM_BUFFER constant to 18, much better
capturing how long it takes to go on chain to claim payments.
This is also more in line with other clients, and the spec, which
sets the default CLTV delay in invoices to 18.

As a side effect, we have to increase MIN_CLTV_EXPIRY_DELTA as
otherwise as are subject to an attack where someone can hold an
HTLC being forwarded long enough that we *also* close the channel
on which we received the HTLC.

Author: TheBlueMatt

fuzz/src/full_stack.rs

@@ -92,6 +92,7 @@ where
 mod test {
 	use {Currency, Description, InvoiceDescription};
 	use lightning::ln::PaymentHash;
+	use lightning::ln::channelmanager::MIN_FINAL_CLTV_EXPIRY;
 	use lightning::ln::functional_test_utils::*;
 	use lightning::ln::features::InitFeatures;
 	use lightning::ln::msgs::ChannelMessageHandler;
@@ -107,7 +108,7 @@ mod test {
 		let _chan = create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known());
 		let invoice = ::utils::create_invoice_from_channelmanager(&nodes[1].node, nodes[1].keys_manager, Currency::BitcoinTestnet, Some(10_000), "test".to_string()).unwrap();
 		assert_eq!(invoice.amount_pico_btc(), Some(100_000));
-		assert_eq!(invoice.min_final_cltv_expiry(), 9);
+		assert_eq!(invoice.min_final_cltv_expiry(), MIN_FINAL_CLTV_EXPIRY as u64);
 		assert_eq!(invoice.description(), InvoiceDescription::Direct(&Description("test".to_string())));
 
 		let mut route_hints = invoice.routes().clone();

lightning-invoice/src/utils.rs

@@ -206,7 +206,7 @@ pub(crate) const CLTV_SHARED_CLAIM_BUFFER: u32 = 12;
 /// HTLC-Success transaction.
 /// 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;
+pub(crate) const CLTV_CLAIM_BUFFER: u32 = 18;
 /// 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). We could enforce the contract by failing

lightning/src/chain/channelmonitor.rs

@@ -563,7 +563,7 @@ pub const BREAKDOWN_TIMEOUT: u16 = 6 * 24;
 pub(crate) const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 2 * 6 * 24 * 7;
 
 /// The minimum number of blocks between an inbound HTLC's CLTV and the corresponding outbound
-/// HTLC's CLTV. The current default represents roughly six hours of blocks at six blocks/hour.
+/// HTLC's CLTV. The current default represents roughly seven hours of blocks at six blocks/hour.
 ///
 /// This can be increased (but not decreased) through [`ChannelConfig::cltv_expiry_delta`]
 ///
@@ -572,7 +572,7 @@ pub(crate) const MAX_LOCAL_BREAKDOWN_TIMEOUT: u16 = 2 * 6 * 24 * 7;
 // i.e. 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 it's at least 3 blocks more).
-pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6 * 6;
+pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6*7;
 pub(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?
 
 /// Minimum CLTV difference between the current block height and received inbound payments.

lightning/src/ln/channelmanager.rs

@@ -1144,7 +1144,7 @@ pub fn claim_payment<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route:
 	claim_payment_along_route(origin_node, &[expected_route], false, our_payment_preimage);
 }
 
-pub const TEST_FINAL_CLTV: u32 = 50;
+pub const TEST_FINAL_CLTV: u32 = 70;
 
 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) {
 	let net_graph_msg_handler = &origin_node.net_graph_msg_handler;

lightning/src/ln/functional_test_utils.rs

@@ -4038,7 +4038,8 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) {
 	};
 	connect_block(&nodes[0], &block);
 	connect_block(&nodes[1], &block);
-	for _ in CHAN_CONFIRM_DEPTH + 2 ..TEST_FINAL_CLTV + CHAN_CONFIRM_DEPTH + 2 - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS {
+	let block_count = TEST_FINAL_CLTV + CHAN_CONFIRM_DEPTH + 2 - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS;
+	for _ in CHAN_CONFIRM_DEPTH + 2..block_count {
 		block.header.prev_blockhash = block.block_hash();
 		connect_block(&nodes[0], &block);
 		connect_block(&nodes[1], &block);
@@ -4055,9 +4056,9 @@ fn do_test_htlc_timeout(send_partial_mpp: bool) {
 
 	nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_timeout_updates.update_fail_htlcs[0]);
 	commitment_signed_dance!(nodes[0], nodes[1], htlc_timeout_updates.commitment_signed, false);
-	// 100_000 msat as u64, followed by a height of TEST_FINAL_CLTV + 2 as u32
+	// 100_000 msat as u64, followed by the height at which we failed back above
 	let mut expected_failure_data = byte_utils::be64_to_array(100_000).to_vec();
-	expected_failure_data.extend_from_slice(&byte_utils::be32_to_array(TEST_FINAL_CLTV + 2));
+	expected_failure_data.extend_from_slice(&byte_utils::be32_to_array(block_count - 1));
 	expect_payment_failed!(nodes[0], our_payment_hash, true, 0x4000 | 15, &expected_failure_data[..]);
 }
 
@@ -5182,7 +5183,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
 	let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
 	assert!(htlc_updates.update_add_htlcs.is_empty());
 	assert_eq!(htlc_updates.update_fail_htlcs.len(), 1);
-	assert_eq!(htlc_updates.update_fail_htlcs[0].htlc_id, 1);
+	let first_htlc_id = htlc_updates.update_fail_htlcs[0].htlc_id;
 	assert!(htlc_updates.update_fulfill_htlcs.is_empty());
 	assert!(htlc_updates.update_fail_malformed_htlcs.is_empty());
 	check_added_monitors!(nodes[1], 1);
@@ -5207,7 +5208,7 @@ fn test_duplicate_payment_hash_one_failure_one_success() {
 	assert!(updates.update_add_htlcs.is_empty());
 	assert!(updates.update_fail_htlcs.is_empty());
 	assert_eq!(updates.update_fulfill_htlcs.len(), 1);
-	assert_eq!(updates.update_fulfill_htlcs[0].htlc_id, 0);
+	assert_ne!(updates.update_fulfill_htlcs[0].htlc_id, first_htlc_id);
 	assert!(updates.update_fail_malformed_htlcs.is_empty());
 	check_added_monitors!(nodes[1], 1);
 
@@ -7743,9 +7744,13 @@ fn test_bump_penalty_txn_on_revoked_htlcs() {
 	let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 
 	let chan = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1000000, 59000000, InitFeatures::known(), InitFeatures::known());
-	// Lock HTLC in both directions
-	let payment_preimage = route_payment(&nodes[0], &vec!(&nodes[1])[..], 3_000_000).0;
-	route_payment(&nodes[1], &vec!(&nodes[0])[..], 3_000_000).0;
+	// Lock HTLC in both directions (using a slightly lower CLTV delay to provide timely RBF bumps)
+	let route = get_route(&nodes[0].node.get_our_node_id(), &nodes[0].net_graph_msg_handler.network_graph.read().unwrap(),
+		&nodes[1].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 3_000_000, 50, nodes[0].logger).unwrap();
+	let payment_preimage = send_along_route(&nodes[0], route, &[&nodes[1]], 3_000_000).0;
+	let route = get_route(&nodes[1].node.get_our_node_id(), &nodes[1].net_graph_msg_handler.network_graph.read().unwrap(),
+		&nodes[0].node.get_our_node_id(), Some(InvoiceFeatures::known()), None, &Vec::new(), 3_000_000, 50, nodes[0].logger).unwrap();
+	send_along_route(&nodes[1], route, &[&nodes[0]], 3_000_000);
 
 	let revoked_local_txn = get_local_commitment_txn!(nodes[1], chan.2);
 	assert_eq!(revoked_local_txn[0].input.len(), 1);
@@ -8058,7 +8063,7 @@ fn test_bump_txn_sanitize_tracking_maps() {
 	claim_payment(&nodes[0], &vec!(&nodes[1])[..], payment_preimage);
 
 	// Broadcast set of revoked txn on A
-	connect_blocks(&nodes[0], 52 - CHAN_CONFIRM_DEPTH);
+	connect_blocks(&nodes[0], TEST_FINAL_CLTV + 2 - CHAN_CONFIRM_DEPTH);
 	expect_pending_htlcs_forwardable_ignore!(nodes[0]);
 	assert_eq!(nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().len(), 0);