Correct spliced-stale SCID expiry for upgrades from pre-0.2 HTLC

If an HTLC was forwarded in 0.1, but waiting to be failed back, it
will ultimately be failed by adding it to the
`ChannelManager::pending_forwards` map with the channel's original
SCID. If that channel is spliced between when the HTLC was
forwarded (on 0.1) and when the HTLC is failed back (on 0.2), that
SCID may no longer exist, causing the HTLC fail-back to be lost.

Luckily, delaying when an SCID is expired is cheap - its just
storing an extra `u64` or two and generating one requires an
on-chain splice, so we simply delay removal of SCIDs for two months
at which point any incoming HTLCs should have been expired for six
weeks and the counterparty should have force-closed anyway.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -1373,6 +1373,27 @@ pub(crate) const COINBASE_MATURITY: u32 = 100;
 /// The number of blocks to wait for a channel_announcement to propagate such that payments using an
 /// older SCID can still be relayed. Once the spend of the previous funding transaction has reached
 /// this number of confirmations, the corresponding SCID will be forgotten.
+///
+/// Because HTLCs added prior to 0.1 which were waiting to be failed may reference a channel's
+/// pre-splice SCID, we need to ensure this is at least the maximum number of blocks before an HTLC
+/// gets failed-back due to a time-out. Luckily, in LDK prior to 0.2, this is enforced directly
+/// when checking the incoming HTLC, and compared against `CLTV_FAR_FAR_AWAY` (which prior to LDK
+/// 0.2, and still at the time of writing, is 14 * 24 * 6, i.e. two weeks).
+///
+/// Here we use four times that value to give us more time to fail an HTLC back (which does require
+/// the user call [`ChannelManager::process_pending_htlc_forwards`]) just in case (if an HTLC has
+/// been expired for 3 * 2 weeks our counterparty really should have closed the channel by now).
+/// Holding on to stale SCIDs doesn't really cost us much as each one costs an on-chain splice to
+/// generate anyway, so we might as well make this nearly arbitrarily long.
+///
+/// [`ChannelManager::process_pending_htlc_forwards`]: crate::ln::channelmanager::ChannelManager::process_pending_htlc_forwards
+#[cfg(not(test))]
+pub(crate) const CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY: u32 = 14 * 24 * 6 * 4;
+
+/// In test (not `_test_utils`, though, since that tests actual upgrading), we deliberately break
+/// the above condition so that we can ensure that HTLCs forwarded in 0.2 or later are handled
+/// correctly even if this constant is reduced and an HTLC can outlive the original channel's SCID.
+#[cfg(test)]
 pub(crate) const CHANNEL_ANNOUNCEMENT_PROPAGATION_DELAY: u32 = 144;
 
 struct PendingChannelMonitorUpdate {
@@ -11960,8 +11981,9 @@ where
 				debug_assert!(false);
 				self.funding.get_holder_pubkeys().funding_pubkey
 			},
-			(Some(prev_funding_txid), ChannelSignerType::Ecdsa(ecdsa)) =>
-				ecdsa.new_funding_pubkey(prev_funding_txid, &self.context.secp_ctx),
+			(Some(prev_funding_txid), ChannelSignerType::Ecdsa(ecdsa)) => {
+				ecdsa.new_funding_pubkey(prev_funding_txid, &self.context.secp_ctx)
+			},
 			#[cfg(taproot)]
 			_ => todo!(),
 		};
@@ -12064,8 +12086,9 @@ where
 				debug_assert!(false);
 				self.funding.get_holder_pubkeys().funding_pubkey
 			},
-			(Some(prev_funding_txid), ChannelSignerType::Ecdsa(ecdsa)) =>
-				ecdsa.new_funding_pubkey(prev_funding_txid, &self.context.secp_ctx),
+			(Some(prev_funding_txid), ChannelSignerType::Ecdsa(ecdsa)) => {
+				ecdsa.new_funding_pubkey(prev_funding_txid, &self.context.secp_ctx)
+			},
 			#[cfg(taproot)]
 			_ => todo!(),
 		};