Bump default CSV delay on counterparty states to 3 days of blocks

Author: Antoine Riard

lightning-background-processor/src/lib.rs

@@ -1019,7 +1019,7 @@ mod tests {
 		// Force close the channel and check that the SpendableOutputs event was handled.
 		nodes[0].node.force_close_broadcasting_latest_txn(&nodes[0].node.list_channels()[0].channel_id, &nodes[1].node.get_our_node_id()).unwrap();
 		let commitment_tx = nodes[0].tx_broadcaster.txn_broadcasted.lock().unwrap().pop().unwrap();
-		confirm_transaction_depth(&mut nodes[0], &commitment_tx, BREAKDOWN_TIMEOUT as u32);
+		confirm_transaction_depth(&mut nodes[0], &commitment_tx, (BREAKDOWN_TIMEOUT * 4) as u32);
 
 		let event = receiver
 			.recv_timeout(Duration::from_secs(EVENT_DEADLINE))

lightning/src/ln/functional_tests.rs

@@ -4222,13 +4222,13 @@ fn test_claim_sizeable_push_msat() {
 	assert_eq!(node_txn[0].output.len(), 2); // We can't force trimming of to_remote output as channel_reserve_satoshis block us to do so at channel opening
 
 	mine_transaction(&nodes[1], &node_txn[0]);
-	connect_blocks(&nodes[1], BREAKDOWN_TIMEOUT as u32 - 1);
+	connect_blocks(&nodes[1], (BREAKDOWN_TIMEOUT * 4) as u32 - 1);
 
 	let spend_txn = check_spendable_outputs!(nodes[1], node_cfgs[1].keys_manager);
 	assert_eq!(spend_txn.len(), 1);
 	assert_eq!(spend_txn[0].input.len(), 1);
 	check_spends!(spend_txn[0], node_txn[0]);
-	assert_eq!(spend_txn[0].input[0].sequence.0, BREAKDOWN_TIMEOUT as u32);
+	assert_eq!(spend_txn[0].input[0].sequence.0, (BREAKDOWN_TIMEOUT * 4) as u32);
 }
 
 #[test]
@@ -4894,14 +4894,14 @@ fn test_dynamic_spendable_outputs_local_htlc_success_tx() {
 	};
 
 	mine_transaction(&nodes[1], &node_tx);
-	connect_blocks(&nodes[1], BREAKDOWN_TIMEOUT as u32 - 1);
+	connect_blocks(&nodes[1], (BREAKDOWN_TIMEOUT * 4) as u32 - 1);
 
 	// Verify that B is able to spend its own HTLC-Success tx thanks to spendable output event given back by its ChannelMonitor
 	let spend_txn = check_spendable_outputs!(nodes[1], node_cfgs[1].keys_manager);
 	assert_eq!(spend_txn.len(), 1);
 	assert_eq!(spend_txn[0].input.len(), 1);
 	check_spends!(spend_txn[0], node_tx);
-	assert_eq!(spend_txn[0].input[0].sequence.0, BREAKDOWN_TIMEOUT as u32);
+	assert_eq!(spend_txn[0].input[0].sequence.0, (BREAKDOWN_TIMEOUT * 4) as u32);
 }
 
 fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, announce_latest: bool) {
@@ -5240,7 +5240,7 @@ fn test_dynamic_spendable_outputs_local_htlc_timeout_tx() {
 	};
 
 	mine_transaction(&nodes[0], &htlc_timeout);
-	connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1);
+	connect_blocks(&nodes[0], (BREAKDOWN_TIMEOUT * 4) as u32 - 1);
 	expect_payment_failed!(nodes[0], our_payment_hash, false);
 
 	// Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor
@@ -5249,11 +5249,11 @@ fn test_dynamic_spendable_outputs_local_htlc_timeout_tx() {
 	check_spends!(spend_txn[0], local_txn[0]);
 	assert_eq!(spend_txn[1].input.len(), 1);
 	check_spends!(spend_txn[1], htlc_timeout);
-	assert_eq!(spend_txn[1].input[0].sequence.0, BREAKDOWN_TIMEOUT as u32);
+	assert_eq!(spend_txn[1].input[0].sequence.0, (BREAKDOWN_TIMEOUT * 4) as u32);
 	assert_eq!(spend_txn[2].input.len(), 2);
 	check_spends!(spend_txn[2], local_txn[0], htlc_timeout);
-	assert!(spend_txn[2].input[0].sequence.0 == BREAKDOWN_TIMEOUT as u32 ||
-	        spend_txn[2].input[1].sequence.0 == BREAKDOWN_TIMEOUT as u32);
+	assert!(spend_txn[2].input[0].sequence.0 == (BREAKDOWN_TIMEOUT * 4) as u32 ||
+	        spend_txn[2].input[1].sequence.0 == (BREAKDOWN_TIMEOUT * 4) as u32);
 }
 
 #[test]
@@ -5322,7 +5322,7 @@ fn test_key_derivation_params() {
 	};
 
 	mine_transaction(&nodes[0], &htlc_timeout);
-	connect_blocks(&nodes[0], BREAKDOWN_TIMEOUT as u32 - 1);
+	connect_blocks(&nodes[0], (BREAKDOWN_TIMEOUT * 4) as u32 - 1);
 	expect_payment_failed!(nodes[0], our_payment_hash, false);
 
 	// Verify that A is able to spend its own HTLC-Timeout tx thanks to spendable output event given back by its ChannelMonitor
@@ -5332,11 +5332,11 @@ fn test_key_derivation_params() {
 	check_spends!(spend_txn[0], local_txn_1[0]);
 	assert_eq!(spend_txn[1].input.len(), 1);
 	check_spends!(spend_txn[1], htlc_timeout);
-	assert_eq!(spend_txn[1].input[0].sequence.0, BREAKDOWN_TIMEOUT as u32);
+	assert_eq!(spend_txn[1].input[0].sequence.0, (BREAKDOWN_TIMEOUT * 4) as u32);
 	assert_eq!(spend_txn[2].input.len(), 2);
 	check_spends!(spend_txn[2], local_txn_1[0], htlc_timeout);
-	assert!(spend_txn[2].input[0].sequence.0 == BREAKDOWN_TIMEOUT as u32 ||
-	        spend_txn[2].input[1].sequence.0 == BREAKDOWN_TIMEOUT as u32);
+	assert!(spend_txn[2].input[0].sequence.0 == (BREAKDOWN_TIMEOUT * 4) as u32 ||
+	        spend_txn[2].input[1].sequence.0 == (BREAKDOWN_TIMEOUT * 4) as u32);
 }
 
 #[test]
@@ -5574,7 +5574,7 @@ fn bolt2_open_channel_sending_node_checks_part2() {
 
 	// BOLT #2 spec: Sending node should set to_self_delay sufficient to ensure the sender can irreversibly spend a commitment transaction output, in case of misbehaviour by the receiver.
 	assert!(BREAKDOWN_TIMEOUT>0);
-	assert!(node0_to_1_send_open_channel.to_self_delay==BREAKDOWN_TIMEOUT);
+	assert!(node0_to_1_send_open_channel.to_self_delay==BREAKDOWN_TIMEOUT*4);
 
 	// BOLT #2 spec: Sending node must ensure the chain_hash value identifies the chain it wishes to open the channel within.
 	let chain_hash=genesis_block(Network::Testnet).header.block_hash();
@@ -8889,7 +8889,7 @@ fn do_test_tx_confirmed_skipping_blocks_immediate_broadcast(test_height_before_t
 
 	let conf_height = nodes[1].best_block_info().1;
 	if !test_height_before_timelock {
-		connect_blocks(&nodes[1], 24 * 6);
+		connect_blocks(&nodes[1], 24 * 6 * 4);
 	}
 	nodes[1].chain_monitor.chain_monitor.transactions_confirmed(
 		&nodes[1].get_block_header(conf_height), &[(0, &node_txn[0])], conf_height);

lightning/src/ln/monitor_tests.rs

@@ -359,7 +359,7 @@ fn do_test_claim_value_force_close(prev_commitment_tx: bool) {
 
 	// Broadcast the closing transaction (which has both pending HTLCs in it) and get B's
 	// broadcasted HTLC claim transaction with preimage.
-	let node_b_commitment_claimable = nodes[1].best_block_info().1 + BREAKDOWN_TIMEOUT as u32;
+	let node_b_commitment_claimable = nodes[1].best_block_info().1 + (BREAKDOWN_TIMEOUT * 4) as u32;
 	mine_transaction(&nodes[0], &remote_txn[0]);
 	mine_transaction(&nodes[1], &remote_txn[0]);
 
@@ -515,7 +515,7 @@ fn do_test_claim_value_force_close(prev_commitment_tx: bool) {
 	// Node B will no longer consider the HTLC "contentious" after the HTLC claim transaction
 	// confirms, and consider it simply "awaiting confirmations". Note that it has to wait for the
 	// standard revocable transaction CSV delay before receiving a `SpendableOutputs`.
-	let node_b_htlc_claimable = nodes[1].best_block_info().1 + BREAKDOWN_TIMEOUT as u32;
+	let node_b_htlc_claimable = nodes[1].best_block_info().1 + (BREAKDOWN_TIMEOUT * 4) as u32;
 	mine_transaction(&nodes[1], &b_broadcast_txn[0]);
 
 	assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
@@ -645,7 +645,7 @@ fn test_balances_on_local_commitment_htlcs() {
 
 	// First confirm the commitment transaction on nodes[0], which should leave us with three
 	// claimable balances.
-	let node_a_commitment_claimable = nodes[0].best_block_info().1 + BREAKDOWN_TIMEOUT as u32;
+	let node_a_commitment_claimable = nodes[0].best_block_info().1 + (BREAKDOWN_TIMEOUT * 4) as u32;
 	mine_transaction(&nodes[0], &as_txn[0]);
 	check_added_monitors!(nodes[0], 1);
 	check_closed_broadcast!(nodes[0], true);
@@ -694,7 +694,7 @@ fn test_balances_on_local_commitment_htlcs() {
 
 	// Now confirm nodes[0]'s HTLC-Timeout transaction, which changes the claimable balance to an
 	// "awaiting confirmations" one.
-	let node_a_htlc_claimable = nodes[0].best_block_info().1 + BREAKDOWN_TIMEOUT as u32;
+	let node_a_htlc_claimable = nodes[0].best_block_info().1 + (BREAKDOWN_TIMEOUT * 4) as u32;
 	mine_transaction(&nodes[0], &as_txn[1]);
 	// Note that prior to the fix in the commit which introduced this test, this (and the next
 	// balance) check failed. With this check removed, the code panicked in the `connect_blocks`
@@ -825,7 +825,7 @@ fn test_no_preimage_inbound_htlc_balances() {
 
 	// Now close the channel by confirming A's commitment transaction on both nodes, checking the
 	// claimable balances remain the same except for the non-HTLC balance changing variant.
-	let node_a_commitment_claimable = nodes[0].best_block_info().1 + BREAKDOWN_TIMEOUT as u32;
+	let node_a_commitment_claimable = nodes[0].best_block_info().1 + (BREAKDOWN_TIMEOUT * 4) as u32;
 	let as_pre_spend_claims = sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
 			claimable_amount_satoshis: 1_000_000 - 500_000 - 10_000 - chan_feerate *
 				(channel::commitment_tx_base_weight(opt_anchors) + 2 * channel::COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000,
@@ -909,7 +909,7 @@ fn test_no_preimage_inbound_htlc_balances() {
 	// Now confirm the two HTLC timeout transactions for A, checking that the inbound HTLC resolves
 	// after ANTI_REORG_DELAY confirmations and the other takes BREAKDOWN_TIMEOUT confirmations.
 	mine_transaction(&nodes[0], &as_htlc_timeout_claim[0]);
-	let as_timeout_claimable_height = nodes[0].best_block_info().1 + (BREAKDOWN_TIMEOUT as u32) - 1;
+	let as_timeout_claimable_height = nodes[0].best_block_info().1 + ((BREAKDOWN_TIMEOUT * 4) as u32) - 1;
 	assert_eq!(sorted_vec(vec![Balance::ClaimableAwaitingConfirmations {
 			claimable_amount_satoshis: 1_000_000 - 500_000 - 10_000 - chan_feerate *
 				(channel::commitment_tx_base_weight(opt_anchors) + 2 * channel::COMMITMENT_TX_WEIGHT_PER_HTLC) / 1000,

lightning/src/util/config.rs

@@ -45,8 +45,11 @@ pub struct ChannelHandshakeConfig {
 	/// case of an honest unilateral channel close, which implicitly decrease the economic value of
 	/// our channel.
 	///
-	/// Default value: [`BREAKDOWN_TIMEOUT`], we enforce it as a minimum at channel opening so you
-	/// can tweak config to ask for more security, not less.
+	/// Default value: [`BREAKDOWN_TIMEOUT`] * 4, we enforce [`BREAKDOWN_TIMEOUT`] as a minimum at
+	/// channel opening so you can tweak config to ask for less security than the default of 4 days
+	/// of block. When setting this value, consider how long it may take to upgrade node(s) after
+	/// a bug was discovered a patch releaaed. While not all potential sources of error can be
+	/// recovered, some classes of bugs may allow this much time to react.
 	pub our_to_self_delay: u16,
 	/// Set to the smallest value HTLC we will accept to process.
 	///
@@ -156,7 +159,7 @@ impl Default for ChannelHandshakeConfig {
 	fn default() -> ChannelHandshakeConfig {
 		ChannelHandshakeConfig {
 			minimum_depth: 6,
-			our_to_self_delay: BREAKDOWN_TIMEOUT,
+			our_to_self_delay: BREAKDOWN_TIMEOUT * 4,
 			our_htlc_minimum_msat: 1,
 			max_inbound_htlc_value_in_flight_percent_of_channel: 10,
 			negotiate_scid_privacy: false,