Test channelmonitor serialization roundtrip doesn't mutate state

fuzz/Cargo.toml

@@ -42,6 +42,10 @@ path = "fuzz_targets/channel_target.rs"
 name = "full_stack_target"
 path = "fuzz_targets/full_stack_target.rs"
 
+[[bin]]
+name = "channelmonitor_deserialize_target"
+path = "fuzz_targets/channelmonitor_deserialize_target.rs"
+
 # message fuzz targets
 [[bin]]
 name = "msg_ping_target"

fuzz/fuzz_targets/channelmonitor_deserialize_target.rs

@@ -0,0 +1,63 @@
+// This file is auto-generated by gen_target.sh based on msg_target_template.txt
+// To modify it, modify msg_target_template.txt and run gen_target.sh instead.
+
+extern crate lightning;
+
+use lightning::ln::channelmonitor;
+use lightning::util::reset_rng_state;
+
+#[inline]
+pub fn do_test(data: &[u8]) {
+	reset_rng_state();
+	if let Some(monitor) = channelmonitor::ChannelMonitor::deserialize(data) {
+		assert!(channelmonitor::ChannelMonitor::deserialize(&monitor.serialize_for_disk()[..]).unwrap() == monitor);
+		monitor.serialize_for_watchtower();
+	}
+}
+
+#[cfg(feature = "afl")]
+extern crate afl;
+#[cfg(feature = "afl")]
+fn main() {
+	afl::read_stdio_bytes(|data| {
+		do_test(&data);
+	});
+}
+
+#[cfg(feature = "honggfuzz")]
+#[macro_use] extern crate honggfuzz;
+#[cfg(feature = "honggfuzz")]
+fn main() {
+	loop {
+		fuzz!(|data| {
+			do_test(data);
+		});
+	}
+}
+
+#[cfg(test)]
+mod tests {
+	fn extend_vec_from_hex(hex: &str, out: &mut Vec<u8>) {
+		let mut b = 0;
+		for (idx, c) in hex.as_bytes().iter().enumerate() {
+			b <<= 4;
+			match *c {
+				b'A'...b'F' => b |= c - b'A' + 10,
+				b'a'...b'f' => b |= c - b'a' + 10,
+				b'0'...b'9' => b |= c - b'0',
+				_ => panic!("Bad hex"),
+			}
+			if (idx & 1) == 1 {
+				out.push(b);
+				b = 0;
+			}
+		}
+	}
+
+	#[test]
+	fn duplicate_crash() {
+		let mut a = Vec::new();
+		extend_vec_from_hex("00", &mut a);
+		super::do_test(&a);
+	}
+}

src/ln/chan_utils.rs

@@ -151,7 +151,7 @@ pub fn get_revokeable_redeemscript(revocation_key: &PublicKey, to_self_delay: u1
 	              .into_script()
 }
 
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 pub struct HTLCOutputInCommitment {
 	pub offered: bool,
 	pub amount_msat: u64,

src/ln/channelmonitor.rs

@@ -117,7 +117,7 @@ const CLTV_SHARED_CLAIM_BUFFER: u32 = 12;
 /// HTLC-Success transaction.
 const CLTV_CLAIM_BUFFER: u32 = 6;
 
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 enum KeyStorage {
 	PrivMode {
 		revocation_base_key: SecretKey,
@@ -130,7 +130,7 @@ enum KeyStorage {
 	}
 }
 
-#[derive(Clone)]
+#[derive(Clone, PartialEq)]
 struct LocalSignedTx {
 	/// txid of the transaction in tx, just used to make comparison faster
 	txid: Sha256dHash,
@@ -215,6 +215,40 @@ impl Clone for ChannelMonitor {
 	}
 }
 
+#[cfg(any(test, feature = "fuzztarget"))]
+/// Used only in testing and fuzztarget to check serialization roundtrips don't change the
+/// underlying object
+impl PartialEq for ChannelMonitor {
+	fn eq(&self, other: &Self) -> bool {
+		if self.funding_txo != other.funding_txo ||
+			self.commitment_transaction_number_obscure_factor != other.commitment_transaction_number_obscure_factor ||
+			self.key_storage != other.key_storage ||
+			self.delayed_payment_base_key != other.delayed_payment_base_key ||
+			self.their_htlc_base_key != other.their_htlc_base_key ||
+			self.their_cur_revocation_points != other.their_cur_revocation_points ||
+			self.our_to_self_delay != other.our_to_self_delay ||
+			self.their_to_self_delay != other.their_to_self_delay ||
+			self.remote_claimable_outpoints != other.remote_claimable_outpoints ||
+			self.remote_hash_commitment_number != other.remote_hash_commitment_number ||
+			self.prev_local_signed_commitment_tx != other.prev_local_signed_commitment_tx ||
+			self.current_local_signed_commitment_tx != other.current_local_signed_commitment_tx ||
+			self.payment_preimages != other.payment_preimages ||
+			self.destination_script != other.destination_script
+		{
+			false
+		} else {
+			for (&(ref secret, ref idx), &(ref o_secret, ref o_idx)) in self.old_secrets.iter().zip(other.old_secrets.iter()) {
+				if secret != o_secret || idx != o_idx {
+					return false
+				}
+			}
+			let us = self.remote_commitment_txn_on_chain.lock().unwrap();
+			let them = other.remote_commitment_txn_on_chain.lock().unwrap();
+			*us == *them
+		}
+	}
+}
+
 impl ChannelMonitor {
 	pub fn new(revocation_base_key: &SecretKey, delayed_payment_base_key: &PublicKey, htlc_base_key: &SecretKey, our_to_self_delay: u16, destination_script: Script) -> ChannelMonitor {
 		ChannelMonitor {
@@ -603,7 +637,7 @@ impl ChannelMonitor {
 		macro_rules! read_bytes {
 			($byte_count: expr) => {
 				{
-					if ($byte_count as usize) + read_pos > data.len() {
+					if ($byte_count as usize) > data.len() - read_pos {
 						return None;
 					}
 					read_pos += $byte_count as usize;

src/util/test_utils.rs

@@ -30,6 +30,10 @@ impl TestChannelMonitor {
 }
 impl channelmonitor::ManyChannelMonitor for TestChannelMonitor {
 	fn add_update_monitor(&self, funding_txo: OutPoint, monitor: channelmonitor::ChannelMonitor) -> Result<(), channelmonitor::ChannelMonitorUpdateErr> {
+		// At every point where we get a monitor update, we should be able to send a useful monitor
+		// to a watchtower and disk...
+		assert!(channelmonitor::ChannelMonitor::deserialize(&monitor.serialize_for_disk()[..]).unwrap() == monitor);
+		monitor.serialize_for_watchtower(); // This at least shouldn't crash...
 		self.added_monitors.lock().unwrap().push((funding_txo, monitor.clone()));
 		self.simple_monitor.add_update_monitor(funding_txo, monitor)
 	}