Make ChannelMonitor round-trip tests more robust

During testsing, we check that a `ChannelMonitor` will round-trip
through serialization exactly. However, we recently added a fix to
change a value in `PackageTemplate` on reload to fix some issues in
the field in 0.1. This can cause the round-trip tests to fail as a
field is modified during read.

We fix it here by simply exempting the field from the equality test
in the condition where it would be updated on read.

We also make the `ChannelMonitor` `PartialEq` trait implementation
non-public as weird workarounds like this make clear that such a
comparison is a britle API at best.

Author: TheBlueMatt

lightning/src/chain/channelmonitor.rs

@@ -77,7 +77,7 @@ use crate::util::ser::{
 use crate::prelude::*;
 
 use crate::io::{self, Error};
-use crate::sync::{LockTestExt, Mutex};
+use crate::sync::Mutex;
 use core::ops::Deref;
 use core::{cmp, mem};
 
@@ -1376,18 +1376,30 @@ macro_rules! holder_commitment_htlcs {
 /// Transaction outputs to watch for on-chain spends.
 pub type TransactionOutputs = (Txid, Vec<(u32, TxOut)>);
 
+// Because we have weird workarounds for `ChannelMonitor` equality checks in `OnchainTxHandler` and
+// `PackageTemplate` the equality implementation isn't really fit for public consumption. Instead,
+// we only expose it during tests.
+#[cfg(any(feature = "_test_utils", test))]
 impl<Signer: EcdsaChannelSigner> PartialEq for ChannelMonitor<Signer>
 where
 	Signer: PartialEq,
 {
-	#[rustfmt::skip]
 	fn eq(&self, other: &Self) -> bool {
+		use crate::sync::LockTestExt;
 		// We need some kind of total lockorder. Absent a better idea, we sort by position in
 		// memory and take locks in that order (assuming that we can't move within memory while a
 		// lock is held).
 		let ord = ((self as *const _) as usize) < ((other as *const _) as usize);
-		let a = if ord { self.inner.unsafe_well_ordered_double_lock_self() } else { other.inner.unsafe_well_ordered_double_lock_self() };
-		let b = if ord { other.inner.unsafe_well_ordered_double_lock_self() } else { self.inner.unsafe_well_ordered_double_lock_self() };
+		let a = if ord {
+			self.inner.unsafe_well_ordered_double_lock_self()
+		} else {
+			other.inner.unsafe_well_ordered_double_lock_self()
+		};
+		let b = if ord {
+			other.inner.unsafe_well_ordered_double_lock_self()
+		} else {
+			self.inner.unsafe_well_ordered_double_lock_self()
+		};
 		a.eq(&b)
 	}
 }

lightning/src/chain/package.rs

@@ -1093,7 +1093,7 @@ enum PackageMalleability {
 ///
 /// As packages are time-sensitive, we fee-bump and rebroadcast them at scheduled intervals.
 /// Failing to confirm a package translate as a loss of funds for the user.
-#[derive(Clone, Debug, PartialEq, Eq)]
+#[derive(Clone, Debug, Eq)]
 pub struct PackageTemplate {
 	// List of onchain outputs and solving data to generate satisfying witnesses.
 	inputs: Vec<(BitcoinOutPoint, PackageSolvingData)>,
@@ -1122,6 +1122,50 @@ pub struct PackageTemplate {
 	height_timer: u32,
 }
 
+impl PartialEq for PackageTemplate {
+	fn eq(&self, o: &Self) -> bool {
+		if self.inputs != o.inputs
+			|| self.malleability != o.malleability
+			|| self.feerate_previous != o.feerate_previous
+			|| self.height_timer != o.height_timer
+		{
+			return false;
+		}
+		#[cfg(test)]
+		{
+			// In some cases we may reset `counterparty_spendable_height` to zero on reload, which
+			// can cause our test assertions that ChannelMonitors round-trip exactly to trip. Here
+			// we allow exactly the same case as we tweak in the `PackageTemplate` `Readable`
+			// implementation.
+			if self.counterparty_spendable_height == 0 {
+				for (_, input) in self.inputs.iter() {
+					if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput {
+						htlc, ..
+					}) = input
+					{
+						if !htlc.offered && htlc.cltv_expiry != 0 {
+							return true;
+						}
+					}
+				}
+			}
+			if o.counterparty_spendable_height == 0 {
+				for (_, input) in o.inputs.iter() {
+					if let PackageSolvingData::RevokedHTLCOutput(RevokedHTLCOutput {
+						htlc, ..
+					}) = input
+					{
+						if !htlc.offered && htlc.cltv_expiry != 0 {
+							return true;
+						}
+					}
+				}
+			}
+		}
+		self.counterparty_spendable_height == o.counterparty_spendable_height
+	}
+}
+
 impl PackageTemplate {
 	#[rustfmt::skip]
 	pub(crate) fn can_merge_with(&self, other: &PackageTemplate, cur_height: u32) -> bool {