Fix serialization rt bug in Channel and test in functional_tests

Previously, when attempting to write out a channel with some
RemoteAnnounced pending inbound HTLCs, we'd write out the count
without them, but write out some of their fields. We should drop
them as intended as they will need to be reannounced upon
reconnection.

This was found while attempting to simply reproduce a different
bug by adding tests for ChannelManager serialization rount-trip at
the end of each functional_test (in Node::drop). That test is
included here to prevent some classes of similar bugs in the future.

Author: TheBlueMatt

lightning/src/ln/channel.rs

@@ -3670,12 +3670,17 @@ impl<ChanSigner: ChannelKeys + Writeable> Writeable for Channel<ChanSigner> {
 		}
 		(self.pending_inbound_htlcs.len() as u64 - dropped_inbound_htlcs).write(writer)?;
 		for htlc in self.pending_inbound_htlcs.iter() {
+			if let &InboundHTLCState::RemoteAnnounced(_) = &htlc.state {
+				// remove_uncommitted_htlcs_and_mark_paused drops these (as the remote never
+				// committed to them and will have to rebroadcast them on reconnect), so skip them.
+				continue;
+			}
 			htlc.htlc_id.write(writer)?;
 			htlc.amount_msat.write(writer)?;
 			htlc.cltv_expiry.write(writer)?;
 			htlc.payment_hash.write(writer)?;
 			match &htlc.state {
-				&InboundHTLCState::RemoteAnnounced(_) => {}, // Drop
+				&InboundHTLCState::RemoteAnnounced(_) => unreachable!(),
 				&InboundHTLCState::AwaitingRemoteRevokeToAnnounce(ref htlc_state) => {
 					1u8.write(writer)?;
 					htlc_state.write(writer)?;

lightning/src/ln/functional_test_utils.rs

@@ -5,7 +5,7 @@ use chain::chaininterface;
 use chain::transaction::OutPoint;
 use chain::keysinterface::KeysInterface;
 use ln::channelmonitor::{ChannelMonitor, ManyChannelMonitor};
-use ln::channelmanager::{ChannelManager,RAACommitmentOrder, PaymentPreimage, PaymentHash};
+use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentPreimage, PaymentHash};
 use ln::router::{Route, Router};
 use ln::features::InitFeatures;
 use ln::msgs;
@@ -17,7 +17,7 @@ use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsPro
 use util::errors::APIError;
 use util::logger::Logger;
 use util::config::UserConfig;
-use util::ser::ReadableArgs;
+use util::ser::{ReadableArgs, Writeable};
 
 use bitcoin::util::hash::BitcoinHash;
 use bitcoin::blockdata::block::BlockHeader;
@@ -37,7 +37,7 @@ use std::cell::RefCell;
 use std::rc::Rc;
 use std::sync::{Arc, Mutex};
 use std::mem;
-use std::collections::HashSet;
+use std::collections::{HashSet, HashMap};
 
 pub const CHAN_CONFIRM_DEPTH: u32 = 100;
 pub fn confirm_transaction<'a, 'b: 'a>(notifier: &'a chaininterface::BlockNotifierRef<'b>, chain: &chaininterface::ChainWatchInterfaceUtil, tx: &Transaction, chan_id: u32) {
@@ -95,20 +95,45 @@ impl<'a, 'b> Drop for Node<'a, 'b> {
 			// Check that if we serialize and then deserialize all our channel monitors we the same
 			// set of outputs to watch for on chain as we have now. Note that if we write tests
 			// that fully close channels and remove the monitors at some point this may break.
-			let new_watch = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Testnet, Arc::clone(&self.logger) as Arc<Logger>));
 			let feeest = Arc::new(test_utils::TestFeeEstimator { sat_per_kw: 253 });
-			let new_monitor = test_utils::TestChannelMonitor::new(new_watch.clone(), self.tx_broadcaster.clone(), self.logger.clone(), feeest);
 			let old_monitors = self.chan_monitor.simple_monitor.monitors.lock().unwrap();
+			let mut new_monitors = Vec::new();
 			for (_, monitor) in old_monitors.iter() {
 				let mut w = test_utils::TestVecWriter(Vec::new());
 				monitor.write_for_disk(&mut w).unwrap();
 				let (_, new_mon) = <(Sha256d, ChannelMonitor<EnforcingChannelKeys>)>::read(
 					&mut ::std::io::Cursor::new(&w.0), Arc::clone(&self.logger) as Arc<Logger>).unwrap();
+				new_monitors.push(new_mon);
+			}
+
+			// Before using all the new monitors to check the watch outpoints, use the full set of
+			// them to ensure we can write and reload our ChannelManager.
+			{
+				let mut channel_monitors = HashMap::new();
+				for monitor in new_monitors.iter_mut() {
+					channel_monitors.insert(monitor.get_funding_txo().unwrap(), monitor);
+				}
+
+				let mut w = test_utils::TestVecWriter(Vec::new());
+				self.node.write(&mut w).unwrap();
+				<(Sha256d, ChannelManager<EnforcingChannelKeys, &test_utils::TestChannelMonitor>)>::read(&mut ::std::io::Cursor::new(w.0), ChannelManagerReadArgs {
+					default_config: UserConfig::default(),
+					keys_manager: self.keys_manager.clone(),
+					fee_estimator: Arc::new(test_utils::TestFeeEstimator { sat_per_kw: 253 }),
+					monitor: self.chan_monitor,
+					tx_broadcaster: self.tx_broadcaster.clone(),
+					logger: Arc::new(test_utils::TestLogger::new()),
+					channel_monitors: &mut channel_monitors,
+				}).unwrap();
+			}
+
+			let new_watch = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Testnet, Arc::clone(&self.logger) as Arc<Logger>));
+			let new_monitor = test_utils::TestChannelMonitor::new(new_watch.clone(), self.tx_broadcaster.clone(), self.logger.clone(), feeest);
+			for new_mon in new_monitors.drain(..) {
 				if let Err(_) = new_monitor.add_update_monitor(new_mon.get_funding_txo().unwrap(), new_mon) {
 					panic!();
 				}
 			}
-
 			if *new_watch != *self.chain_monitor {
 				panic!();
 			}