Merge pull request #849 from TheBlueMatt/2021-03-config-cltv-delta

Make cltv_expiry_delta configurable and reduce the min/default some

Author: TheBlueMatt

codecov.yml

@@ -3,6 +3,11 @@ coverage:
     project:
       default:
         target: auto
-        threshold: 2%
+        threshold: 1%
         base: auto
         informational: false
+    patch:
+      default:
+        target: auto
+        threshold: 100%
+        base: auto

lightning/src/ln/channel.rs

@@ -25,7 +25,7 @@ use bitcoin::secp256k1;
 use ln::features::{ChannelFeatures, InitFeatures};
 use ln::msgs;
 use ln::msgs::{DecodeError, OptionalField, DataLossProtect};
-use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MAX_LOCAL_BREAKDOWN_TIMEOUT};
+use ln::channelmanager::{PendingHTLCStatus, HTLCSource, HTLCFailReason, HTLCFailureMsg, PendingHTLCInfo, RAACommitmentOrder, PaymentPreimage, PaymentHash, BREAKDOWN_TIMEOUT, MIN_CLTV_EXPIRY_DELTA, MAX_LOCAL_BREAKDOWN_TIMEOUT};
 use ln::chan_utils::{CounterpartyCommitmentSecrets, TxCreationKeys, HTLCOutputInCommitment, HTLC_SUCCESS_TX_WEIGHT, HTLC_TIMEOUT_TX_WEIGHT, make_funding_redeemscript, ChannelPublicKeys, CommitmentTransaction, HolderCommitmentTransaction, ChannelTransactionParameters, CounterpartyChannelTransactionParameters, MAX_HTLCS, get_commitment_transaction_number_obscure_factor};
 use ln::chan_utils;
 use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
@@ -3359,6 +3359,10 @@ impl<Signer: Sign> Channel<Signer> {
 		self.config.fee_proportional_millionths
 	}
 
+	pub fn get_cltv_expiry_delta(&self) -> u16 {
+		cmp::max(self.config.cltv_expiry_delta, MIN_CLTV_EXPIRY_DELTA)
+	}
+
 	#[cfg(test)]
 	pub fn get_feerate(&self) -> u32 {
 		self.feerate_per_kw

lightning/src/ln/channelmanager.rs

@@ -523,11 +523,16 @@ 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. This should always be a few blocks greater than channelmonitor::CLTV_CLAIM_BUFFER,
-/// ie 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).
-const CLTV_EXPIRY_DELTA: u16 = 6 * 12; //TODO?
+/// HTLC's CLTV. The current default represents roughly six hours of blocks at six blocks/hour.
+///
+/// This can be increased (but not decreased) through [`ChannelConfig::cltv_expiry_delta`]
+///
+/// [`ChannelConfig::cltv_expiry_delta`]: crate::util::config::ChannelConfig::cltv_expiry_delta
+// This should always be a few blocks greater than channelmonitor::CLTV_CLAIM_BUFFER,
+// 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(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?
 
 // Check that our CLTV_EXPIRY is at least CLTV_CLAIM_BUFFER + ANTI_REORG_DELAY + LATENCY_GRACE_PERIOD_BLOCKS,
@@ -538,13 +543,13 @@ pub(super) const CLTV_FAR_FAR_AWAY: u32 = 6 * 24 * 7; //TODO?
 // LATENCY_GRACE_PERIOD_BLOCKS.
 #[deny(const_err)]
 #[allow(dead_code)]
-const CHECK_CLTV_EXPIRY_SANITY: u32 = CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - CLTV_CLAIM_BUFFER - ANTI_REORG_DELAY - LATENCY_GRACE_PERIOD_BLOCKS;
+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;
 
 // Check for ability of an attacker to make us fail on-chain by delaying inbound claim. See
 // ChannelMontior::would_broadcast_at_height for a description of why this is needed.
 #[deny(const_err)]
 #[allow(dead_code)]
-const CHECK_CLTV_EXPIRY_SANITY_2: u32 = CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER;
+const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER;
 
 /// Details of a channel, as returned by ChannelManager::list_channels and ChannelManager::list_usable_channels
 #[derive(Clone)]
@@ -1271,7 +1276,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 					if fee.is_none() || msg.amount_msat < fee.unwrap() || (msg.amount_msat - fee.unwrap()) < *amt_to_forward { // fee_insufficient
 						break Some(("Prior hop has deviated from specified fees parameters or origin node has obsolete ones", 0x1000 | 12, Some(self.get_channel_update(chan).unwrap())));
 					}
-					if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + CLTV_EXPIRY_DELTA as u64 { // incorrect_cltv_expiry
+					if (msg.cltv_expiry as u64) < (*outgoing_cltv_value) as u64 + chan.get_cltv_expiry_delta() as u64 { // incorrect_cltv_expiry
 						break Some(("Forwarding node has tampered with the intended HTLC values or origin node has an obsolete cltv_expiry_delta", 0x1000 | 13, Some(self.get_channel_update(chan).unwrap())));
 					}
 					let cur_height = self.latest_block_height.load(Ordering::Acquire) as u32 + 1;
@@ -1329,7 +1334,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 			short_channel_id,
 			timestamp: chan.get_update_time_counter(),
 			flags: (!were_node_one) as u8 | ((!chan.is_live() as u8) << 1),
-			cltv_expiry_delta: CLTV_EXPIRY_DELTA,
+			cltv_expiry_delta: chan.get_cltv_expiry_delta(),
 			htlc_minimum_msat: chan.get_counterparty_htlc_minimum_msat(),
 			htlc_maximum_msat: OptionalField::Present(chan.get_announced_htlc_max_msat()),
 			fee_base_msat: chan.get_holder_fee_base_msat(&self.fee_estimator),

lightning/src/util/config.rs

@@ -23,18 +23,21 @@ pub struct ChannelHandshakeConfig {
 	///
 	/// Default value: 6.
 	pub minimum_depth: u32,
-	/// Set to the amount of time we require our counterparty to wait to claim their money.
+	/// Set to the number of blocks we require our counterparty to wait to claim their money (ie
+	/// the number of blocks we have to punish our counterparty if they broadcast a revoked
+	/// transaction).
 	///
-	/// It's one of the main parameter of our security model. We (or one of our watchtowers) MUST
-	/// be online to check for peer having broadcast a revoked transaction to steal our funds
-	/// at least once every our_to_self_delay blocks.
+	/// This is one of the main parameters of our security model. We (or one of our watchtowers) MUST
+	/// be online to check for revoked transactions on-chain at least once every our_to_self_delay
+	/// blocks (minus some margin to allow us enough time to broadcast and confirm a transaction,
+	/// possibly with time in between to RBF the spending transaction).
 	///
 	/// Meanwhile, asking for a too high delay, we bother peer to freeze funds for nothing in
 	/// case of an honest unilateral channel close, which implicitly decrease the economic value of
 	/// our channel.
 	///
-	/// Default value: [`BREAKDOWN_TIMEOUT`] (currently 144), 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`], we enforce it as a minimum at channel opening so you
+	/// can tweak config to ask for more security, not less.
 	pub our_to_self_delay: u16,
 	/// Set to the smallest value HTLC we will accept to process.
 	///
@@ -161,6 +164,26 @@ pub struct ChannelConfig {
 	///
 	/// Default value: 0.
 	pub fee_proportional_millionths: u32,
+	/// The difference in the CLTV value between incoming HTLCs and an outbound HTLC forwarded over
+	/// the channel this config applies to.
+	///
+	/// This is analogous to [`ChannelHandshakeConfig::our_to_self_delay`] but applies to in-flight
+	/// HTLC balance when a channel appears on-chain whereas
+	/// [`ChannelHandshakeConfig::our_to_self_delay`] applies to the remaining
+	/// (non-HTLC-encumbered) balance.
+	///
+	/// Thus, for HTLC-encumbered balances to be enforced on-chain when a channel is force-closed,
+	/// we (or one of our watchtowers) MUST be online to check for broadcast of the current
+	/// commitment transaction at least once per this many blocks (minus some margin to allow us
+	/// enough time to broadcast and confirm a transaction, possibly with time in between to RBF
+	/// the spending transaction).
+	///
+	/// Default value: 72 (12 hours at an average of 6 blocks/hour).
+	/// Minimum value: [`MIN_CLTV_EXPIRY_DELTA`], any values less than this will be treated as
+	///                [`MIN_CLTV_EXPIRY_DELTA`] instead.
+	///
+	/// [`MIN_CLTV_EXPIRY_DELTA`]: crate::ln::channelmanager::MIN_CLTV_EXPIRY_DELTA
+	pub cltv_expiry_delta: u16,
 	/// Set to announce the channel publicly and notify all nodes that they can route via this
 	/// channel.
 	///
@@ -192,15 +215,17 @@ impl Default for ChannelConfig {
 	fn default() -> Self {
 		ChannelConfig {
 			fee_proportional_millionths: 0,
+			cltv_expiry_delta: 6 * 12, // 6 blocks/hour * 12 hours
 			announced_channel: false,
 			commit_upfront_shutdown_pubkey: true,
 		}
 	}
 }
 
 //Add write and readable traits to channelconfig
-impl_writeable!(ChannelConfig, 8+1+1, {
+impl_writeable!(ChannelConfig, 8+2+1+1, {
 	fee_proportional_millionths,
+	cltv_expiry_delta,
 	announced_channel,
 	commit_upfront_shutdown_pubkey
 });