From 492983f54fb035d5c46db25078fabc1518bdaa28 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 19 Mar 2020 19:15:06 -0400 Subject: [PATCH 1/2] Fail to deserialize ChannelManager if it is ahead of any monitor(s) If any monitors are out of sync with the Channel, we previously closed the channel, but we should really only do that if the monitor is ahead of the channel, opting to call the whole thing invalid if the channel is ahead of the monitor. --- lightning/src/ln/channelmanager.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index cde40ee3232..1afce1f7bb9 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -3470,10 +3470,17 @@ impl<'a, ChanSigner: ChannelKeys + Readable, M: Deref, T: Deref, K: Deref, F: De let funding_txo = channel.get_funding_txo().ok_or(DecodeError::InvalidValue)?; funding_txo_set.insert(funding_txo.clone()); if let Some(ref mut monitor) = args.channel_monitors.get_mut(&funding_txo) { - if channel.get_cur_local_commitment_transaction_number() != monitor.get_cur_local_commitment_number() || - channel.get_revoked_remote_commitment_transaction_number() != monitor.get_min_seen_secret() || - channel.get_cur_remote_commitment_transaction_number() != monitor.get_cur_remote_commitment_number() || - channel.get_latest_monitor_update_id() != monitor.get_latest_update_id() { + if channel.get_cur_local_commitment_transaction_number() < monitor.get_cur_local_commitment_number() || + channel.get_revoked_remote_commitment_transaction_number() < monitor.get_min_seen_secret() || + channel.get_cur_remote_commitment_transaction_number() < monitor.get_cur_remote_commitment_number() || + channel.get_latest_monitor_update_id() > monitor.get_latest_update_id() { + // If the channel is ahead of the monitor, return InvalidValue: + return Err(DecodeError::InvalidValue); + } else if channel.get_cur_local_commitment_transaction_number() > monitor.get_cur_local_commitment_number() || + channel.get_revoked_remote_commitment_transaction_number() > monitor.get_min_seen_secret() || + channel.get_cur_remote_commitment_transaction_number() > monitor.get_cur_remote_commitment_number() || + channel.get_latest_monitor_update_id() < monitor.get_latest_update_id() { + // But if the channel is behind of the monitor, close the channel: let (_, _, mut new_failed_htlcs) = channel.force_shutdown(true); failed_htlcs.append(&mut new_failed_htlcs); monitor.broadcast_latest_local_commitment_txn(&args.tx_broadcaster); From 4aa95af27219b73badae4d237f8921b874c5025c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 19 Mar 2020 21:31:18 -0400 Subject: [PATCH 2/2] Test that ChannelManager fails to deserialize if monitors are stale --- lightning/src/ln/functional_tests.rs | 36 ++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index e00e036db53..7cd546dbe8e 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -3839,6 +3839,13 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { create_announced_chan_between_nodes(&nodes, 2, 0, InitFeatures::supported(), InitFeatures::supported()); let (_, _, channel_id, funding_tx) = create_announced_chan_between_nodes(&nodes, 0, 3, InitFeatures::supported(), InitFeatures::supported()); + let mut node_0_stale_monitors_serialized = Vec::new(); + for monitor in nodes[0].chan_monitor.simple_monitor.monitors.lock().unwrap().iter() { + let mut writer = test_utils::TestVecWriter(Vec::new()); + monitor.1.write_for_disk(&mut writer).unwrap(); + node_0_stale_monitors_serialized.push(writer.0); + } + let (our_payment_preimage, _) = route_payment(&nodes[2], &[&nodes[0], &nodes[1]], 1000000); // Serialize the ChannelManager here, but the monitor we keep up-to-date @@ -3861,6 +3868,15 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; new_chan_monitor = test_utils::TestChannelMonitor::new(nodes[0].chain_monitor.clone(), nodes[0].tx_broadcaster.clone(), Arc::new(test_utils::TestLogger::new()), &fee_estimator); nodes[0].chan_monitor = &new_chan_monitor; + + let mut node_0_stale_monitors = Vec::new(); + for serialized in node_0_stale_monitors_serialized.iter() { + let mut read = &serialized[..]; + let (_, monitor) = <(Sha256dHash, ChannelMonitor)>::read(&mut read, Arc::new(test_utils::TestLogger::new())).unwrap(); + assert!(read.is_empty()); + node_0_stale_monitors.push(monitor); + } + let mut node_0_monitors = Vec::new(); for serialized in node_0_monitors_serialized.iter() { let mut read = &serialized[..]; @@ -3869,9 +3885,25 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { node_0_monitors.push(monitor); } - let mut nodes_0_read = &nodes_0_serialized[..]; keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet, Arc::new(test_utils::TestLogger::new())); - let (_, nodes_0_deserialized_tmp) = <(Sha256dHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs { + + let mut nodes_0_read = &nodes_0_serialized[..]; + if let Err(msgs::DecodeError::InvalidValue) = + <(Sha256dHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs { + default_config: UserConfig::default(), + keys_manager: &keys_manager, + fee_estimator: &fee_estimator, + monitor: nodes[0].chan_monitor, + tx_broadcaster: nodes[0].tx_broadcaster.clone(), + logger: Arc::new(test_utils::TestLogger::new()), + channel_monitors: &mut node_0_stale_monitors.iter_mut().map(|monitor| { (monitor.get_funding_txo().unwrap(), monitor) }).collect(), + }) { } else { + panic!("If the monitor(s) are stale, this indicates a bug and we should get an Err return"); + }; + + let mut nodes_0_read = &nodes_0_serialized[..]; + let (_, nodes_0_deserialized_tmp) = + <(Sha256dHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs { default_config: UserConfig::default(), keys_manager: &keys_manager, fee_estimator: &fee_estimator,