From ed2a5fdab91a03e7fca38101a4f235564580912d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 17 Feb 2020 13:50:46 -0500 Subject: [PATCH] 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. --- lightning/src/ln/channel.rs | 5 ++- lightning/src/ln/functional_test_utils.rs | 37 +++++++++++++++++++---- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/lightning/src/ln/channel.rs b/lightning/src/ln/channel.rs index 58ade789..31741970 100644 --- a/lightning/src/ln/channel.rs +++ b/lightning/src/ln/channel.rs @@ -3670,12 +3670,15 @@ impl Writeable for Channel { } (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 { + continue; // Drop + } 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)?; diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 674e7d0f..9acbc7ed 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -4,7 +4,7 @@ use chain::chaininterface; use chain::transaction::OutPoint; use chain::keysinterface::KeysInterface; -use ln::channelmanager::{ChannelManager,RAACommitmentOrder, PaymentPreimage, PaymentHash}; +use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentPreimage, PaymentHash}; use ln::channelmonitor::{ChannelMonitor, ManyChannelMonitor}; use ln::router::{Route, Router}; use ln::features::InitFeatures; @@ -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 get 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 chain_watch = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Testnet, Arc::clone(&self.logger) as Arc)); let feeest = Arc::new(test_utils::TestFeeEstimator { sat_per_kw: 253 }); - let channel_monitor = test_utils::TestChannelMonitor::new(chain_watch.clone(), self.tx_broadcaster.clone(), self.logger.clone(), feeest); let old_monitors = self.chan_monitor.simple_monitor.monitors.lock().unwrap(); + let mut deserialized_monitors = Vec::new(); for (_, old_monitor) in old_monitors.iter() { let mut w = test_utils::TestVecWriter(Vec::new()); old_monitor.write_for_disk(&mut w).unwrap(); let (_, deserialized_monitor) = <(Sha256d, ChannelMonitor)>::read( &mut ::std::io::Cursor::new(&w.0), Arc::clone(&self.logger) as Arc).unwrap(); + deserialized_monitors.push(deserialized_monitor); + } + + // 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 deserialized_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)>::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 chain_watch = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Testnet, Arc::clone(&self.logger) as Arc)); + let channel_monitor = test_utils::TestChannelMonitor::new(chain_watch.clone(), self.tx_broadcaster.clone(), self.logger.clone(), feeest); + for deserialized_monitor in deserialized_monitors.drain(..) { if let Err(_) = channel_monitor.add_update_monitor(deserialized_monitor.get_funding_txo().unwrap(), deserialized_monitor) { panic!(); } } - if *chain_watch != *self.chain_monitor { panic!(); } -- 2.30.2