Increase min CLTV delta and delta before CLTV at which chans close

In the previous commit it was observed that we actually have to run
the whole "get a transaction confirmed" process from start to
finish twice to claim an HTLC on an anchor channel. This leaves us
with only 9 blocks to get each transaction confirmed, which is
quite aggressive.

Here we double this threshold, force-closing channels which have an
expiring HTLC 36 blocks before expiry instead of 18. We also
increase the minimum CLTV expiry delta to 48 to ensure we have at
least a few blocks after the transactions get confirmed before we
need to fail the inbound edge of a forwarded HTLC back.

We do not change the default CLTV expiry delta of 72 blocks.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -223,13 +223,13 @@ impl_writeable_tlv_based!(HTLCUpdate, {
 /// transaction.
 pub(crate) const COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE: u32 = 12;
 
-/// When we go to force-close a channel because an HTLC is expiring, we should ensure that the
-/// HTLC(s) expiring are not considered pinnable, allowing us to aggregate them with other HTLC(s)
-/// expiring at the same time.
-const _: () = assert!(CLTV_CLAIM_BUFFER > COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE);
+/// When we go to force-close a channel because an HTLC is expiring, by the time we've gotten the
+/// commitment transaction confirmed, we should ensure that the HTLC(s) expiring are not considered
+/// pinnable, allowing us to aggregate them with other HTLC(s) expiring at the same time.
+const _: () = assert!(MAX_BLOCKS_FOR_CONF > COUNTERPARTY_CLAIMABLE_WITHIN_BLOCKS_PINNABLE);
 
 /// The upper bound on how many blocks we think it can take for us to get a transaction confirmed.
-pub(crate) const MAX_BLOCKS_FOR_CONF: u32 = 9;
+pub(crate) const MAX_BLOCKS_FOR_CONF: u32 = 18;
 
 /// If an HTLC expires within this many blocks, force-close the channel to broadcast the
 /// HTLC-Success transaction.

lightning/src/ln/channelmanager.rs

@@ -2824,7 +2824,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 seven hours of blocks at six blocks/hour.
+/// HTLC's CLTV. The current default represents roughly eight hours of blocks at six blocks/hour.
 ///
 /// This can be increased (but not decreased) through [`ChannelConfig::cltv_expiry_delta`]
 ///
@@ -2833,7 +2833,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*7;
+pub const MIN_CLTV_EXPIRY_DELTA: u16 = 6*8;
 // This should be long enough to allow a payment path drawn across multiple routing hops with substantial
 // `cltv_expiry_delta`. Indeed, the length of those values is the reaction delay offered to a routing node
 // in case of HTLC on-chain settlement. While appearing less competitive, a node operator could decide to

lightning/src/ln/functional_tests.rs

@@ -3500,7 +3500,7 @@ fn do_test_htlc_on_chain_timeout(connect_style: ConnectStyle) {
 	let node_txn = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); // 1 timeout tx
 	assert_eq!(node_txn.len(), 1);
 	check_spends!(node_txn[0], commitment_tx[0]);
-	assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT);
+	assert_eq!(node_txn[0].clone().input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT + 1);
 }
 
 #[xtest(feature = "_externalize_tests")]
@@ -9387,25 +9387,19 @@ fn do_test_onchain_htlc_settlement_after_close(broadcast_alice: bool, go_onchain
 	// Step (6):
 	// Finally, check that Bob broadcasted a preimage-claiming transaction for the HTLC output on the
 	// broadcasted commitment transaction.
-	{
-		let script_weight = match broadcast_alice {
-			true => OFFERED_HTLC_SCRIPT_WEIGHT,
-			false => ACCEPTED_HTLC_SCRIPT_WEIGHT
-		};
-		// If Alice force-closed, Bob only broadcasts a HTLC-output-claiming transaction. Otherwise,
-		// Bob force-closed and broadcasts the commitment transaction along with a
-		// HTLC-output-claiming transaction.
-		let mut bob_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
-		if broadcast_alice {
-			assert_eq!(bob_txn.len(), 1);
-			check_spends!(bob_txn[0], txn_to_broadcast[0]);
-			assert_eq!(bob_txn[0].input[0].witness.last().unwrap().len(), script_weight);
-		} else {
-			assert_eq!(bob_txn.len(), if nodes[1].connect_style.borrow().updates_best_block_first() { 3 } else { 2 });
-			let htlc_tx = bob_txn.pop().unwrap();
-			check_spends!(htlc_tx, txn_to_broadcast[0]);
-			assert_eq!(htlc_tx.input[0].witness.last().unwrap().len(), script_weight);
-		}
+	// If Alice force-closed, Bob only broadcasts a HTLC-output-claiming transaction. Otherwise,
+	// Bob force-closed and broadcasts the commitment transaction along with a
+	// HTLC-output-claiming transaction.
+	let mut bob_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone();
+	if broadcast_alice {
+		assert_eq!(bob_txn.len(), 1);
+		check_spends!(bob_txn[0], txn_to_broadcast[0]);
+		assert_eq!(bob_txn[0].input[0].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT);
+	} else {
+		assert_eq!(bob_txn.len(), if nodes[1].connect_style.borrow().updates_best_block_first() { 3 } else { 2 });
+		let htlc_tx = bob_txn.pop().unwrap();
+		check_spends!(htlc_tx, txn_to_broadcast[0]);
+		assert_eq!(htlc_tx.input[0].witness.last().unwrap().len(), ACCEPTED_HTLC_SCRIPT_WEIGHT + 1);
 	}
 }