Correct sanity checking of MIN_CLTV_EXPIRY_DELTA constants

The `CHECK_CLTV_EXPIRY_SANITY_2` check actually made no sense -
its use of `2*CLTV_CLAIM_BUFFER` implicitly assumed that we were
failing HTLCs `CLTV_CLAIM_BUFFER` after they expired, which is
nonsense.

Instead, we improve the readability of the docs on
`_CHECK_CLTV_EXPIRY_SANITY` and add a new
`_CHECK_CLTV_EXPIRY_OFFCHAIN` that correctly tests for what the
`CHECK_CLTV_EXPIRY_SANITY_2` check was supposed to look at.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -4508,18 +4508,6 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 					// chain when our counterparty is waiting for expiration to off-chain fail an HTLC
 					// we give ourselves a few blocks of headroom after expiration before going
 					// on-chain for an expired HTLC.
-					// Note that, to avoid a potential attack whereby a node delays claiming an HTLC
-					// from us until we've reached the point where we go on-chain with the
-					// corresponding inbound HTLC, we must ensure that outbound HTLCs go on chain at
-					// least CLTV_CLAIM_BUFFER blocks prior to the inbound HTLC.
-					//  aka outbound_cltv + LATENCY_GRACE_PERIOD_BLOCKS == height - CLTV_CLAIM_BUFFER
-					//      inbound_cltv == height + CLTV_CLAIM_BUFFER
-					//      outbound_cltv + LATENCY_GRACE_PERIOD_BLOCKS + CLTV_CLAIM_BUFFER <= inbound_cltv - CLTV_CLAIM_BUFFER
-					//      LATENCY_GRACE_PERIOD_BLOCKS + 2*CLTV_CLAIM_BUFFER <= inbound_cltv - outbound_cltv
-					//      CLTV_EXPIRY_DELTA <= inbound_cltv - outbound_cltv (by check in ChannelManager::decode_update_add_htlc_onion)
-					//      LATENCY_GRACE_PERIOD_BLOCKS + 2*CLTV_CLAIM_BUFFER <= CLTV_EXPIRY_DELTA
-					//  The final, above, condition is checked for statically in channelmanager
-					//  with CHECK_CLTV_EXPIRY_SANITY_2.
 					let htlc_outbound = $holder_tx == htlc.offered;
 					if ( htlc_outbound && htlc.cltv_expiry + LATENCY_GRACE_PERIOD_BLOCKS <= height) ||
 					   (!htlc_outbound && htlc.cltv_expiry <= height + CLTV_CLAIM_BUFFER && self.payment_preimages.contains_key(&htlc.payment_hash)) {

lightning/src/ln/channelmanager.rs

@@ -2840,19 +2840,35 @@ pub(super) const CLTV_FAR_FAR_AWAY: u32 = 14 * 24 * 6;
 // a payment was being routed, so we add an extra block to be safe.
 pub const MIN_FINAL_CLTV_EXPIRY_DELTA: u16 = HTLC_FAIL_BACK_BUFFER as u16 + 3;
 
-// Check that our CLTV_EXPIRY is at least CLTV_CLAIM_BUFFER + ANTI_REORG_DELAY + LATENCY_GRACE_PERIOD_BLOCKS,
-// ie that if the next-hop peer fails the HTLC within
-// LATENCY_GRACE_PERIOD_BLOCKS then we'll still have CLTV_CLAIM_BUFFER left to timeout it onchain,
-// then waiting ANTI_REORG_DELAY to be reorg-safe on the outbound HLTC and
-// failing the corresponding htlc backward, and us now seeing the last block of ANTI_REORG_DELAY before
-// LATENCY_GRACE_PERIOD_BLOCKS.
-#[allow(dead_code)]
-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 an HTLC claim. See
-// ChannelMonitor::should_broadcast_holder_commitment_txn for a description of why this is needed.
-#[allow(dead_code)]
-const CHECK_CLTV_EXPIRY_SANITY_2: u32 = MIN_CLTV_EXPIRY_DELTA as u32 - LATENCY_GRACE_PERIOD_BLOCKS - 2*CLTV_CLAIM_BUFFER;
+// Check that our MIN_CLTV_EXPIRY_DELTA gives us enough time to get everything on chain and locked
+// in with enough time left to fail the corresponding HTLC back to our inbound edge before they
+// force-close on us.
+// ie that if the next-hop peer fails the HTLC LATENCY_GRACE_PERIOD_BLOCKS after our
+// CLTV_CLAIM_BUFFER (because that's how many blocks we allow them after expiry), we'll still have
+// CLTV_CLAIM_BUFFER + ANTI_REORG_DELAY left to get two transactions on chain and the second
+// fully locked in before the peer force-closes on us (LATENCY_GRACE_PERIOD_BOLOCKS before the
+// expiry, i.e. assuming the peer force-closes right at the expiry and we're behind by
+// LATENCY_GRACE_PERIOD_BLOCKS).
+// second fully locked in before the peer force-closes on us.
+const _CHECK_CLTV_EXPIRY_SANITY: () = assert!(
+	MIN_CLTV_EXPIRY_DELTA as u32 >= 2*LATENCY_GRACE_PERIOD_BLOCKS + CLTV_CLAIM_BUFFER + ANTI_REORG_DELAY
+);
+
+// Check that our MIN_CLTV_EXPIRY_DELTA gives us enough time to get the HTLC preimage back to our
+// counterparty if the outbound edge gives us the preimage only one block before we'd force-close
+// the channel.
+// ie they provide the preimage LATENCY_GRACE_PERIOD_BLOCKS - 1 after the HTLC expires, then we
+// pass the preimage back, which takes LATENCY_GRACE_PERIOD_BLOCKS to complete, and we want to make
+// sure this all happens at least N blocks before the inbound HTLC expires (where N is the
+// counterparty's CLTV_CLAIM_BUFFER or equivalent).
+const _ASSUMED_COUNTERPARTY_CLTV_CLAIM_BUFFER: u32 = 6 * 6;
+
+const _CHECK_COUNTERPARTY_REALISTIC: () =
+	assert!(_ASSUMED_COUNTERPARTY_CLTV_CLAIM_BUFFER >= CLTV_CLAIM_BUFFER);
+
+const _CHECK_CLTV_EXPIRY_OFFCHAIN: () = assert!(
+	MIN_CLTV_EXPIRY_DELTA as u32 >= 2*LATENCY_GRACE_PERIOD_BLOCKS - 1 + _ASSUMED_COUNTERPARTY_CLTV_CLAIM_BUFFER
+);
 
 /// The number of ticks of [`ChannelManager::timer_tick_occurred`] until expiry of incomplete MPPs
 pub(crate) const MPP_TIMEOUT_TICKS: u8 = 3;