Merge pull request #503 from TheBlueMatt/2020-02-chanmon-ser-roundtrip
authorMatt Corallo <649246+TheBlueMatt@users.noreply.github.com>
Wed, 19 Feb 2020 00:26:47 +0000 (00:26 +0000)
committerGitHub <noreply@github.com>
Wed, 19 Feb 2020 00:26:47 +0000 (00:26 +0000)
Fix Two Bugs around ChannelManager serialization round-trip

lightning/src/ln/channel.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/functional_tests.rs

index 58ade789fbb16c16039b7c1cd200d224093aa180..317419702095cf7a8adc3e11415b2954b3d522bd 100644 (file)
@@ -3670,12 +3670,15 @@ 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 {
+                               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)?;
index e5f8acc2a8d6f48a6fe2bcca3cc95d525f4271c1..015894e594dc1246efecef452d312171eed099ba 100644 (file)
@@ -3310,7 +3310,7 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable<R>, M: Deref> R
                let mut short_to_id = HashMap::with_capacity(cmp::min(channel_count as usize, 128));
                for _ in 0..channel_count {
                        let mut channel: Channel<ChanSigner> = ReadableArgs::read(reader, args.logger.clone())?;
-                       if channel.last_block_connected != last_block_hash {
+                       if channel.last_block_connected != Default::default() && channel.last_block_connected != last_block_hash {
                                return Err(DecodeError::InvalidValue);
                        }
 
index 674e7d0ff322df1b17f7db5926601789c4ddbe50..9acbc7eddbf7fdd0c45a37c37a00b51bcdf56ea2 100644 (file)
@@ -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<Logger>));
                        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<EnforcingChannelKeys>)>::read(
                                        &mut ::std::io::Cursor::new(&w.0), Arc::clone(&self.logger) as Arc<Logger>).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<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 chain_watch = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Testnet, Arc::clone(&self.logger) as Arc<Logger>));
+                       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!();
                        }
index 9b1907cbb776e4b048482ba21328947b4f68b497..a38ef6f34acc7ef034bd54fb7a7b4bc47cc0f4e4 100644 (file)
@@ -413,6 +413,104 @@ fn test_1_conf_open() {
        }
 }
 
+fn do_test_sanity_on_in_flight_opens(steps: u8) {
+       // Previously, we had issues deserializing channels when we hadn't connected the first block
+       // after creation. To catch that and similar issues, we lean on the Node::drop impl to test
+       // serialization round-trips and simply do steps towards opening a channel and then drop the
+       // Node objects.
+
+       let node_cfgs = create_node_cfgs(2);
+       let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]);
+       let nodes = create_network(2, &node_cfgs, &node_chanmgrs);
+
+       if steps & 0b1000_0000 != 0{
+               let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
+               nodes[0].block_notifier.block_connected_checked(&header, 1, &Vec::new(), &[0; 0]);
+               nodes[1].block_notifier.block_connected_checked(&header, 1, &Vec::new(), &[0; 0]);
+       }
+
+       if steps & 0x0f == 0 { return; }
+       nodes[0].node.create_channel(nodes[1].node.get_our_node_id(), 100000, 10001, 42).unwrap();
+       let open_channel = get_event_msg!(nodes[0], MessageSendEvent::SendOpenChannel, nodes[1].node.get_our_node_id());
+
+       if steps & 0x0f == 1 { return; }
+       nodes[1].node.handle_open_channel(&nodes[0].node.get_our_node_id(), InitFeatures::supported(), &open_channel);
+       let accept_channel = get_event_msg!(nodes[1], MessageSendEvent::SendAcceptChannel, nodes[0].node.get_our_node_id());
+
+       if steps & 0x0f == 2 { return; }
+       nodes[0].node.handle_accept_channel(&nodes[1].node.get_our_node_id(), InitFeatures::supported(), &accept_channel);
+
+       let (temporary_channel_id, tx, funding_output) = create_funding_transaction(&nodes[0], 100000, 42);
+
+       if steps & 0x0f == 3 { return; }
+       {
+               nodes[0].node.funding_transaction_generated(&temporary_channel_id, funding_output);
+               let mut added_monitors = nodes[0].chan_monitor.added_monitors.lock().unwrap();
+               assert_eq!(added_monitors.len(), 1);
+               assert_eq!(added_monitors[0].0, funding_output);
+               added_monitors.clear();
+       }
+       let funding_created = get_event_msg!(nodes[0], MessageSendEvent::SendFundingCreated, nodes[1].node.get_our_node_id());
+
+       if steps & 0x0f == 4 { return; }
+       nodes[1].node.handle_funding_created(&nodes[0].node.get_our_node_id(), &funding_created);
+       {
+               let mut added_monitors = nodes[1].chan_monitor.added_monitors.lock().unwrap();
+               assert_eq!(added_monitors.len(), 1);
+               assert_eq!(added_monitors[0].0, funding_output);
+               added_monitors.clear();
+       }
+       let funding_signed = get_event_msg!(nodes[1], MessageSendEvent::SendFundingSigned, nodes[0].node.get_our_node_id());
+
+       if steps & 0x0f == 5 { return; }
+       nodes[0].node.handle_funding_signed(&nodes[1].node.get_our_node_id(), &funding_signed);
+       {
+               let mut added_monitors = nodes[0].chan_monitor.added_monitors.lock().unwrap();
+               assert_eq!(added_monitors.len(), 1);
+               assert_eq!(added_monitors[0].0, funding_output);
+               added_monitors.clear();
+       }
+
+       let events_4 = nodes[0].node.get_and_clear_pending_events();
+       assert_eq!(events_4.len(), 1);
+       match events_4[0] {
+               Event::FundingBroadcastSafe { ref funding_txo, user_channel_id } => {
+                       assert_eq!(user_channel_id, 42);
+                       assert_eq!(*funding_txo, funding_output);
+               },
+               _ => panic!("Unexpected event"),
+       };
+
+       if steps & 0x0f == 6 { return; }
+       create_chan_between_nodes_with_value_confirm_first(&nodes[0], &nodes[1], &tx);
+
+       if steps & 0x0f == 7 { return; }
+       confirm_transaction(&nodes[0].block_notifier, &nodes[0].chain_monitor, &tx, tx.version);
+       create_chan_between_nodes_with_value_confirm_second(&nodes[1], &nodes[0]);
+}
+
+#[test]
+fn test_sanity_on_in_flight_opens() {
+       do_test_sanity_on_in_flight_opens(0);
+       do_test_sanity_on_in_flight_opens(0 | 0b1000_0000);
+       do_test_sanity_on_in_flight_opens(1);
+       do_test_sanity_on_in_flight_opens(1 | 0b1000_0000);
+       do_test_sanity_on_in_flight_opens(2);
+       do_test_sanity_on_in_flight_opens(2 | 0b1000_0000);
+       do_test_sanity_on_in_flight_opens(3);
+       do_test_sanity_on_in_flight_opens(3 | 0b1000_0000);
+       do_test_sanity_on_in_flight_opens(4);
+       do_test_sanity_on_in_flight_opens(4 | 0b1000_0000);
+       do_test_sanity_on_in_flight_opens(5);
+       do_test_sanity_on_in_flight_opens(5 | 0b1000_0000);
+       do_test_sanity_on_in_flight_opens(6);
+       do_test_sanity_on_in_flight_opens(6 | 0b1000_0000);
+       do_test_sanity_on_in_flight_opens(7);
+       do_test_sanity_on_in_flight_opens(7 | 0b1000_0000);
+       do_test_sanity_on_in_flight_opens(8);
+       do_test_sanity_on_in_flight_opens(8 | 0b1000_0000);
+}
+
 #[test]
 fn test_update_fee_vanilla() {
        let node_cfgs = create_node_cfgs(2);