Set correct counterparty_spendable_height on c.p. revoked HTLCs

If the counterparty broadcasts a revoked transaction with offered
HTLCs, the output is not immediately pinnable as the counterparty
cannot claim the HTLC until the CLTV expires and they use an
HTLC-Timeout path.

Here we fix the `counterparty_spendable_height` value we set on
counterparty revoked HTLC claims to match reality. Note that
because we still consider these outputs `Pinnable` the value is
not used. In the next commit we'll start making them `Unpinnable`
which will actually change behavior.

Note that when upgrading we have to wipe the
`counterparty_spendable_height` value for non-offered HTLCs as
otherwise we'd consider them `Unpinnable` when they are, in fact,
`Pinnable`.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -3564,11 +3564,16 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 							return (claimable_outpoints, to_counterparty_output_info);
 						}
 						let revk_htlc_outp = RevokedHTLCOutput::build(per_commitment_point, self.counterparty_commitment_params.counterparty_delayed_payment_base_key, self.counterparty_commitment_params.counterparty_htlc_base_key, per_commitment_key, htlc.amount_msat / 1000, htlc.clone(), &self.onchain_tx_handler.channel_transaction_parameters.channel_type_features);
+						let counterparty_spendable_height = if htlc.offered {
+							htlc.cltv_expiry
+						} else {
+							height
+						};
 						let justice_package = PackageTemplate::build_package(
 							commitment_txid,
 							transaction_output_index,
 							PackageSolvingData::RevokedHTLCOutput(revk_htlc_outp),
-							htlc.cltv_expiry,
+							counterparty_spendable_height,
 						);
 						claimable_outpoints.push(justice_package);
 					}

lightning/src/chain/package.rs

@@ -771,10 +771,12 @@ pub struct PackageTemplate {
 	/// Block height at which our counterparty can potentially claim this output as well (assuming
 	/// they have the keys or information required to do so).
 	///
-	/// This is used primarily by external consumers to decide when an output becomes "pinnable"
-	/// because the counterparty can potentially spend it. It is also used internally by
-	/// [`Self::get_height_timer`] to identify when an output must be claimed by, depending on the
-	/// type of output.
+	/// This is used primarily to decide when an output becomes "pinnable" because the counterparty
+	/// can potentially spend it. It is also used internally by [`Self::get_height_timer`] to
+	/// identify when an output must be claimed by, depending on the type of output.
+	///
+	/// Note that for revoked counterparty HTLC outputs the value may be zero in some cases where
+	/// we upgraded from LDK 0.1 or prior.
 	counterparty_spendable_height: u32,
 	// Cache of package feerate committed at previous (re)broadcast. If bumping resources
 	// (either claimed output value or external utxo), it will keep increasing until holder
@@ -1218,6 +1220,18 @@ impl Readable for PackageTemplate {
 			(4, _height_original, option), // Written with a dummy value since 0.1
 			(6, height_timer, option),
 		});
+		for (_, input) in &inputs {
+			if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput { htlc, .. }) = input {
+				// LDK versions through 0.1 set the wrong counterparty_spendable_height for
+				// non-offered revoked HTLCs (ie HTLCs we sent to our counterparty which they can
+				// claim with a preimage immediately). Here we detect this and reset the value to
+				// zero, as the value is unused except for merging decisions which doesn't care
+				// about any values below the current height.
+				if !htlc.offered && htlc.cltv_expiry == counterparty_spendable_height {
+					counterparty_spendable_height = 0;
+				}
+			}
+		}
 		Ok(PackageTemplate {
 			inputs,
 			malleability,