From 2cbb8358f114f52976c9f51ee17f71f36f23f20a Mon Sep 17 00:00:00 2001 From: Devrandom Date: Wed, 2 Dec 2020 18:50:17 +0100 Subject: [PATCH 1/4] Use TestKeysInterface in functional tests This allows stateful validation in EnforcingChannelKeys --- lightning-persister/src/lib.rs | 4 +- lightning/src/ln/chanmon_update_fail_tests.rs | 2 +- lightning/src/ln/functional_test_utils.rs | 22 ++++--- lightning/src/ln/functional_tests.rs | 60 +++++++++---------- lightning/src/util/test_utils.rs | 9 ++- 5 files changed, 51 insertions(+), 46 deletions(-) diff --git a/lightning-persister/src/lib.rs b/lightning-persister/src/lib.rs index bb6eb54c9af..bc718789490 100644 --- a/lightning-persister/src/lib.rs +++ b/lightning-persister/src/lib.rs @@ -198,8 +198,8 @@ mod tests { let persister_1 = FilesystemPersister::new("test_filesystem_persister_1".to_string()); let chanmon_cfgs = create_chanmon_cfgs(2); let mut node_cfgs = create_node_cfgs(2, &chanmon_cfgs); - let chain_mon_0 = test_utils::TestChainMonitor::new(Some(&chanmon_cfgs[0].chain_source), &chanmon_cfgs[0].tx_broadcaster, &chanmon_cfgs[0].logger, &chanmon_cfgs[0].fee_estimator, &persister_0); - let chain_mon_1 = test_utils::TestChainMonitor::new(Some(&chanmon_cfgs[1].chain_source), &chanmon_cfgs[1].tx_broadcaster, &chanmon_cfgs[1].logger, &chanmon_cfgs[1].fee_estimator, &persister_1); + let chain_mon_0 = test_utils::TestChainMonitor::new(Some(&chanmon_cfgs[0].chain_source), &chanmon_cfgs[0].tx_broadcaster, &chanmon_cfgs[0].logger, &chanmon_cfgs[0].fee_estimator, &persister_0, &node_cfgs[0].keys_manager); + let chain_mon_1 = test_utils::TestChainMonitor::new(Some(&chanmon_cfgs[1].chain_source), &chanmon_cfgs[1].tx_broadcaster, &chanmon_cfgs[1].logger, &chanmon_cfgs[1].fee_estimator, &persister_1, &node_cfgs[1].keys_manager); node_cfgs[0].chain_monitor = chain_mon_0; node_cfgs[1].chain_monitor = chain_mon_1; let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs index b9376029eb0..426b7cb23b9 100644 --- a/lightning/src/ln/chanmon_update_fail_tests.rs +++ b/lightning/src/ln/chanmon_update_fail_tests.rs @@ -109,7 +109,7 @@ fn test_monitor_and_persister_update_fail() { let new_monitor = <(BlockHash, ChannelMonitor)>::read( &mut ::std::io::Cursor::new(&w.0), &test_utils::OnlyReadsKeysInterface {}).unwrap().1; assert!(new_monitor == *monitor); - let chain_mon = test_utils::TestChainMonitor::new(Some(&chain_source), &chanmon_cfgs[0].tx_broadcaster, &logger, &chanmon_cfgs[0].fee_estimator, &persister); + let chain_mon = test_utils::TestChainMonitor::new(Some(&chain_source), &chanmon_cfgs[0].tx_broadcaster, &logger, &chanmon_cfgs[0].fee_estimator, &persister, &node_cfgs[0].keys_manager); assert!(chain_mon.watch_channel(outpoint, new_monitor).is_ok()); chain_mon }; diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 7863286f77a..e487b152840 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -21,7 +21,7 @@ use ln::msgs; use ln::msgs::{ChannelMessageHandler,RoutingMessageHandler}; use util::enforcing_trait_impls::EnforcingChannelKeys; use util::test_utils; -use util::test_utils::{TestChainMonitor, OnlyReadsKeysInterface}; +use util::test_utils::TestChainMonitor; use util::events::{Event, EventsProvider, MessageSendEvent, MessageSendEventsProvider}; use util::errors::APIError; use util::config::UserConfig; @@ -96,6 +96,8 @@ pub struct TestChanMonCfg { pub chain_source: test_utils::TestChainSource, pub persister: test_utils::TestPersister, pub logger: test_utils::TestLogger, + pub keys_manager: test_utils::TestKeysInterface, + } pub struct NodeCfg<'a> { @@ -103,7 +105,7 @@ pub struct NodeCfg<'a> { pub tx_broadcaster: &'a test_utils::TestBroadcaster, pub fee_estimator: &'a test_utils::TestFeeEstimator, pub chain_monitor: test_utils::TestChainMonitor<'a>, - pub keys_manager: test_utils::TestKeysInterface, + pub keys_manager: &'a test_utils::TestKeysInterface, pub logger: &'a test_utils::TestLogger, pub node_seed: [u8; 32], } @@ -172,7 +174,7 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> { let mut w = test_utils::TestVecWriter(Vec::new()); old_monitor.write(&mut w).unwrap(); let (_, deserialized_monitor) = <(BlockHash, ChannelMonitor)>::read( - &mut ::std::io::Cursor::new(&w.0), &OnlyReadsKeysInterface {}).unwrap(); + &mut ::std::io::Cursor::new(&w.0), self.keys_manager).unwrap(); deserialized_monitors.push(deserialized_monitor); } } @@ -205,7 +207,7 @@ impl<'a, 'b, 'c> Drop for Node<'a, 'b, 'c> { txn_broadcasted: Mutex::new(self.tx_broadcaster.txn_broadcasted.lock().unwrap().clone()) }; let chain_source = test_utils::TestChainSource::new(Network::Testnet); - let chain_monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &broadcaster, &self.logger, &feeest, &persister); + let chain_monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &broadcaster, &self.logger, &feeest, &persister, &self.keys_manager); for deserialized_monitor in deserialized_monitors.drain(..) { if let Err(_) = chain_monitor.watch_channel(deserialized_monitor.get_funding_txo().0, deserialized_monitor) { panic!(); @@ -1132,7 +1134,10 @@ pub fn create_chanmon_cfgs(node_count: usize) -> Vec { let chain_source = test_utils::TestChainSource::new(Network::Testnet); let logger = test_utils::TestLogger::with_id(format!("node {}", i)); let persister = test_utils::TestPersister::new(); - chan_mon_cfgs.push(TestChanMonCfg{ tx_broadcaster, fee_estimator, chain_source, logger, persister }); + let seed = [i as u8; 32]; + let keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet); + + chan_mon_cfgs.push(TestChanMonCfg{ tx_broadcaster, fee_estimator, chain_source, logger, persister, keys_manager }); } chan_mon_cfgs @@ -1142,10 +1147,9 @@ pub fn create_node_cfgs<'a>(node_count: usize, chanmon_cfgs: &'a Vec(node_count: usize, cfgs: &'a Vec default_config.channel_options.announced_channel = true; default_config.peer_channel_config_limits.force_announced_channel_preference = false; default_config.own_channel_config.our_htlc_minimum_msat = 1000; // sanitization being done by the sender, to exerce receiver logic we need to lift of limit - let node = ChannelManager::new(Network::Testnet, cfgs[i].fee_estimator, &cfgs[i].chain_monitor, cfgs[i].tx_broadcaster, cfgs[i].logger, &cfgs[i].keys_manager, if node_config[i].is_some() { node_config[i].clone().unwrap() } else { default_config }, 0); + let node = ChannelManager::new(Network::Testnet, cfgs[i].fee_estimator, &cfgs[i].chain_monitor, cfgs[i].tx_broadcaster, cfgs[i].logger, cfgs[i].keys_manager, if node_config[i].is_some() { node_config[i].clone().unwrap() } else { default_config }, 0); chanmgrs.push(node); } diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index a9367d9c3f2..9575fd7f000 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -53,7 +53,7 @@ use regex; use std::collections::{BTreeSet, HashMap, HashSet}; use std::default::Default; -use std::sync::{Arc, Mutex}; +use std::sync::Mutex; use std::sync::atomic::Ordering; use std::mem; @@ -4284,9 +4284,9 @@ fn test_no_txn_manager_serialize_deserialize() { logger = test_utils::TestLogger::new(); fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; persister = test_utils::TestPersister::new(); - new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister); - nodes[0].chain_monitor = &new_chain_monitor; keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet); + new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, &keys_manager); + nodes[0].chain_monitor = &new_chain_monitor; let mut chan_0_monitor_read = &chan_0_monitor_serialized.0[..]; let (_, mut chan_0_monitor) = <(BlockHash, ChannelMonitor)>::read( &mut chan_0_monitor_read, &keys_manager).unwrap(); @@ -4394,8 +4394,8 @@ fn test_manager_serialize_deserialize_events() { fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; logger = test_utils::TestLogger::new(); persister = test_utils::TestPersister::new(); - new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister); keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet); + new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, &keys_manager); nodes[0].chain_monitor = &new_chain_monitor; let mut chan_0_monitor_read = &chan_0_monitor_serialized.0[..]; let (_, mut chan_0_monitor) = <(BlockHash, ChannelMonitor)>::read( @@ -4470,7 +4470,7 @@ fn test_simple_manager_serialize_deserialize() { let fee_estimator: test_utils::TestFeeEstimator; let persister: test_utils::TestPersister; let new_chain_monitor: test_utils::TestChainMonitor; - let keys_manager: test_utils::TestKeysInterface; + let keys_manager: &test_utils::TestKeysInterface; let nodes_0_deserialized: ChannelManager; let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); @@ -4487,12 +4487,12 @@ fn test_simple_manager_serialize_deserialize() { logger = test_utils::TestLogger::new(); fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; persister = test_utils::TestPersister::new(); - new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister); - keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet); + keys_manager = &chanmon_cfgs[0].keys_manager; + new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager); nodes[0].chain_monitor = &new_chain_monitor; let mut chan_0_monitor_read = &chan_0_monitor_serialized.0[..]; let (_, mut chan_0_monitor) = <(BlockHash, ChannelMonitor)>::read( - &mut chan_0_monitor_read, &keys_manager).unwrap(); + &mut chan_0_monitor_read, keys_manager).unwrap(); assert!(chan_0_monitor_read.is_empty()); let mut nodes_0_read = &nodes_0_serialized[..]; @@ -4501,7 +4501,7 @@ fn test_simple_manager_serialize_deserialize() { channel_monitors.insert(chan_0_monitor.get_funding_txo().0, &mut chan_0_monitor); <(BlockHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs { default_config: UserConfig::default(), - keys_manager: &keys_manager, + keys_manager, fee_estimator: &fee_estimator, chain_monitor: nodes[0].chain_monitor, tx_broadcaster: nodes[0].tx_broadcaster.clone(), @@ -4532,7 +4532,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { let fee_estimator: test_utils::TestFeeEstimator; let persister: test_utils::TestPersister; let new_chain_monitor: test_utils::TestChainMonitor; - let keys_manager: test_utils::TestKeysInterface; + let keys_manager: &test_utils::TestKeysInterface; let nodes_0_deserialized: ChannelManager; let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs); create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); @@ -4568,15 +4568,15 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { logger = test_utils::TestLogger::new(); fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; persister = test_utils::TestPersister::new(); - new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister); + keys_manager = &chanmon_cfgs[0].keys_manager; + new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager); nodes[0].chain_monitor = &new_chain_monitor; - keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet); let mut node_0_stale_monitors = Vec::new(); for serialized in node_0_stale_monitors_serialized.iter() { let mut read = &serialized[..]; - let (_, monitor) = <(BlockHash, ChannelMonitor)>::read(&mut read, &keys_manager).unwrap(); + let (_, monitor) = <(BlockHash, ChannelMonitor)>::read(&mut read, keys_manager).unwrap(); assert!(read.is_empty()); node_0_stale_monitors.push(monitor); } @@ -4584,7 +4584,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { let mut node_0_monitors = Vec::new(); for serialized in node_0_monitors_serialized.iter() { let mut read = &serialized[..]; - let (_, monitor) = <(BlockHash, ChannelMonitor)>::read(&mut read, &keys_manager).unwrap(); + let (_, monitor) = <(BlockHash, ChannelMonitor)>::read(&mut read, keys_manager).unwrap(); assert!(read.is_empty()); node_0_monitors.push(monitor); } @@ -4593,7 +4593,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { if let Err(msgs::DecodeError::InvalidValue) = <(BlockHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs { default_config: UserConfig::default(), - keys_manager: &keys_manager, + keys_manager, fee_estimator: &fee_estimator, chain_monitor: nodes[0].chain_monitor, tx_broadcaster: nodes[0].tx_broadcaster.clone(), @@ -4607,7 +4607,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { let (_, nodes_0_deserialized_tmp) = <(BlockHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs { default_config: UserConfig::default(), - keys_manager: &keys_manager, + keys_manager, fee_estimator: &fee_estimator, chain_monitor: nodes[0].chain_monitor, tx_broadcaster: nodes[0].tx_broadcaster.clone(), @@ -5713,8 +5713,8 @@ fn test_key_derivation_params() { // We manually create the node configuration to backup the seed. let seed = [42; 32]; let keys_manager = test_utils::TestKeysInterface::new(&seed, Network::Testnet); - let chain_monitor = test_utils::TestChainMonitor::new(Some(&chanmon_cfgs[0].chain_source), &chanmon_cfgs[0].tx_broadcaster, &chanmon_cfgs[0].logger, &chanmon_cfgs[0].fee_estimator, &chanmon_cfgs[0].persister); - let node = NodeCfg { chain_source: &chanmon_cfgs[0].chain_source, logger: &chanmon_cfgs[0].logger, tx_broadcaster: &chanmon_cfgs[0].tx_broadcaster, fee_estimator: &chanmon_cfgs[0].fee_estimator, chain_monitor, keys_manager, node_seed: seed }; + let chain_monitor = test_utils::TestChainMonitor::new(Some(&chanmon_cfgs[0].chain_source), &chanmon_cfgs[0].tx_broadcaster, &chanmon_cfgs[0].logger, &chanmon_cfgs[0].fee_estimator, &chanmon_cfgs[0].persister, &keys_manager); + let node = NodeCfg { chain_source: &chanmon_cfgs[0].chain_source, logger: &chanmon_cfgs[0].logger, tx_broadcaster: &chanmon_cfgs[0].tx_broadcaster, fee_estimator: &chanmon_cfgs[0].fee_estimator, chain_monitor, keys_manager: &keys_manager, node_seed: seed }; let mut node_cfgs = create_node_cfgs(3, &chanmon_cfgs); node_cfgs.remove(0); node_cfgs.insert(0, node); @@ -7325,8 +7325,7 @@ fn test_user_configurable_csv_delay() { let nodes = create_network(2, &node_cfgs, &node_chanmgrs); // We test config.our_to_self > BREAKDOWN_TIMEOUT is enforced in Channel::new_outbound() - let keys_manager = Arc::new(test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet)); - if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &keys_manager, nodes[1].node.get_our_node_id(), 1000000, 1000000, 0, &low_our_to_self_config) { + if let Err(error) = Channel::new_outbound(&&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), 1000000, 1000000, 0, &low_our_to_self_config) { match error { APIError::APIMisuseError { err } => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), @@ -7337,7 +7336,7 @@ fn test_user_configurable_csv_delay() { nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap(); let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id()); open_channel.to_self_delay = 200; - if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), &open_channel, 0, &low_our_to_self_config) { + if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), &open_channel, 0, &low_our_to_self_config) { match error { ChannelError::Close(err) => { assert!(regex::Regex::new(r"Configured with an unreasonable our_to_self_delay \(\d+\) putting user funds at risks").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), @@ -7363,7 +7362,7 @@ fn test_user_configurable_csv_delay() { nodes[1].node.create_channel(nodes[0].node.get_our_node_id(), 1000000, 1000000, 42, None).unwrap(); let mut open_channel = get_event_msg!(nodes[1], MessageSendEvent::SendOpenChannel, nodes[0].node.get_our_node_id()); open_channel.to_self_delay = 200; - if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), &open_channel, 0, &high_their_to_self_config) { + if let Err(error) = Channel::new_from_req(&&test_utils::TestFeeEstimator { sat_per_kw: 253 }, &nodes[0].keys_manager, nodes[1].node.get_our_node_id(), InitFeatures::known(), &open_channel, 0, &high_their_to_self_config) { match error { ChannelError::Close(err) => { assert!(regex::Regex::new(r"They wanted our payments to be delayed by a needlessly long period\. Upper limit: \d+\. Actual: \d+").unwrap().is_match(err.as_str())); }, _ => panic!("Unexpected event"), @@ -7377,15 +7376,15 @@ fn test_data_loss_protect() { // * we don't broadcast our Local Commitment Tx in case of fallen behind // * we close channel in case of detecting other being fallen behind // * we are able to claim our own outputs thanks to to_remote being static - let keys_manager; let persister; let logger; let fee_estimator; let tx_broadcaster; let chain_source; + let chanmon_cfgs = create_chanmon_cfgs(2); + let keys_manager = &chanmon_cfgs[0].keys_manager; let monitor; let node_state_0; - let chanmon_cfgs = create_chanmon_cfgs(2); let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -7405,18 +7404,17 @@ fn test_data_loss_protect() { // Restore node A from previous state logger = test_utils::TestLogger::with_id(format!("node {}", 0)); - keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet); - let mut chain_monitor = <(BlockHash, ChannelMonitor)>::read(&mut ::std::io::Cursor::new(previous_chain_monitor_state.0), &keys_manager).unwrap().1; + let mut chain_monitor = <(BlockHash, ChannelMonitor)>::read(&mut ::std::io::Cursor::new(previous_chain_monitor_state.0), keys_manager).unwrap().1; chain_source = test_utils::TestChainSource::new(Network::Testnet); tx_broadcaster = test_utils::TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new())}; fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; persister = test_utils::TestPersister::new(); - monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister); + monitor = test_utils::TestChainMonitor::new(Some(&chain_source), &tx_broadcaster, &logger, &fee_estimator, &persister, keys_manager); node_state_0 = { let mut channel_monitors = HashMap::new(); channel_monitors.insert(OutPoint { txid: chan.3.txid(), index: 0 }, &mut chain_monitor); <(BlockHash, ChannelManager)>::read(&mut ::std::io::Cursor::new(previous_node_state), ChannelManagerReadArgs { - keys_manager: &keys_manager, + keys_manager: keys_manager, fee_estimator: &fee_estimator, chain_monitor: &monitor, logger: &logger, @@ -8281,7 +8279,7 @@ fn test_update_err_monitor_lockdown() { let new_monitor = <(BlockHash, channelmonitor::ChannelMonitor)>::read( &mut ::std::io::Cursor::new(&w.0), &test_utils::OnlyReadsKeysInterface {}).unwrap().1; assert!(new_monitor == *monitor); - let watchtower = test_utils::TestChainMonitor::new(Some(&chain_source), &chanmon_cfgs[0].tx_broadcaster, &logger, &chanmon_cfgs[0].fee_estimator, &persister); + let watchtower = test_utils::TestChainMonitor::new(Some(&chain_source), &chanmon_cfgs[0].tx_broadcaster, &logger, &chanmon_cfgs[0].fee_estimator, &persister, &node_cfgs[0].keys_manager); assert!(watchtower.watch_channel(outpoint, new_monitor).is_ok()); watchtower }; @@ -8340,7 +8338,7 @@ fn test_concurrent_monitor_claim() { let new_monitor = <(BlockHash, channelmonitor::ChannelMonitor)>::read( &mut ::std::io::Cursor::new(&w.0), &test_utils::OnlyReadsKeysInterface {}).unwrap().1; assert!(new_monitor == *monitor); - let watchtower = test_utils::TestChainMonitor::new(Some(&chain_source), &chanmon_cfgs[0].tx_broadcaster, &logger, &chanmon_cfgs[0].fee_estimator, &persister); + let watchtower = test_utils::TestChainMonitor::new(Some(&chain_source), &chanmon_cfgs[0].tx_broadcaster, &logger, &chanmon_cfgs[0].fee_estimator, &persister, &node_cfgs[0].keys_manager); assert!(watchtower.watch_channel(outpoint, new_monitor).is_ok()); watchtower }; @@ -8366,7 +8364,7 @@ fn test_concurrent_monitor_claim() { let new_monitor = <(BlockHash, channelmonitor::ChannelMonitor)>::read( &mut ::std::io::Cursor::new(&w.0), &test_utils::OnlyReadsKeysInterface {}).unwrap().1; assert!(new_monitor == *monitor); - let watchtower = test_utils::TestChainMonitor::new(Some(&chain_source), &chanmon_cfgs[0].tx_broadcaster, &logger, &chanmon_cfgs[0].fee_estimator, &persister); + let watchtower = test_utils::TestChainMonitor::new(Some(&chain_source), &chanmon_cfgs[0].tx_broadcaster, &logger, &chanmon_cfgs[0].fee_estimator, &persister, &node_cfgs[0].keys_manager); assert!(watchtower.watch_channel(outpoint, new_monitor).is_ok()); watchtower }; diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index 3680a724557..b55c19e3936 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -39,6 +39,7 @@ use std::sync::Mutex; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::{cmp, mem}; use std::collections::{HashMap, HashSet}; +use chain::keysinterface::InMemoryChannelKeys; pub struct TestVecWriter(pub Vec); impl Writer for TestVecWriter { @@ -79,17 +80,19 @@ pub struct TestChainMonitor<'a> { pub added_monitors: Mutex)>>, pub latest_monitor_update_id: Mutex>, pub chain_monitor: chainmonitor::ChainMonitor>, + pub keys_manager: &'a TestKeysInterface, pub update_ret: Mutex>>, // If this is set to Some(), after the next return, we'll always return this until update_ret // is changed: pub next_update_ret: Mutex>>, } impl<'a> TestChainMonitor<'a> { - pub fn new(chain_source: Option<&'a TestChainSource>, broadcaster: &'a chaininterface::BroadcasterInterface, logger: &'a TestLogger, fee_estimator: &'a TestFeeEstimator, persister: &'a channelmonitor::Persist) -> Self { + pub fn new(chain_source: Option<&'a TestChainSource>, broadcaster: &'a chaininterface::BroadcasterInterface, logger: &'a TestLogger, fee_estimator: &'a TestFeeEstimator, persister: &'a channelmonitor::Persist, keys_manager: &'a TestKeysInterface) -> Self { Self { added_monitors: Mutex::new(Vec::new()), latest_monitor_update_id: Mutex::new(HashMap::new()), chain_monitor: chainmonitor::ChainMonitor::new(chain_source, broadcaster, logger, fee_estimator, persister), + keys_manager, update_ret: Mutex::new(None), next_update_ret: Mutex::new(None), } @@ -104,7 +107,7 @@ impl<'a> chain::Watch for TestChainMonitor<'a> { let mut w = TestVecWriter(Vec::new()); monitor.write(&mut w).unwrap(); let new_monitor = <(BlockHash, channelmonitor::ChannelMonitor)>::read( - &mut ::std::io::Cursor::new(&w.0), &OnlyReadsKeysInterface {}).unwrap().1; + &mut ::std::io::Cursor::new(&w.0), self.keys_manager).unwrap().1; assert!(new_monitor == monitor); self.latest_monitor_update_id.lock().unwrap().insert(funding_txo.to_channel_id(), (funding_txo, monitor.get_latest_update_id())); self.added_monitors.lock().unwrap().push((funding_txo, monitor)); @@ -137,7 +140,7 @@ impl<'a> chain::Watch for TestChainMonitor<'a> { w.0.clear(); monitor.write(&mut w).unwrap(); let new_monitor = <(BlockHash, channelmonitor::ChannelMonitor)>::read( - &mut ::std::io::Cursor::new(&w.0), &OnlyReadsKeysInterface {}).unwrap().1; + &mut ::std::io::Cursor::new(&w.0), self.keys_manager).unwrap().1; assert!(new_monitor == *monitor); self.added_monitors.lock().unwrap().push((funding_txo, new_monitor)); From a5869b92818afd938ff60db9d74d2f06bbfa846f Mon Sep 17 00:00:00 2001 From: Devrandom Date: Sun, 12 Jul 2020 09:00:10 -0700 Subject: [PATCH 2/4] Revocation enforcement in signer We want to make sure that we don't sign revoked transactions. Given that ChannelKeys are not singletons and revocation enforcement is stateful, we need to store the revocation state in KeysInterface. --- lightning/src/chain/keysinterface.rs | 16 +++--- lightning/src/ln/functional_tests.rs | 56 ++++++++++----------- lightning/src/ln/onchaintx.rs | 2 +- lightning/src/util/enforcing_trait_impls.rs | 41 ++++++++++++--- lightning/src/util/test_utils.rs | 39 +++++++++++--- 5 files changed, 107 insertions(+), 47 deletions(-) diff --git a/lightning/src/chain/keysinterface.rs b/lightning/src/chain/keysinterface.rs index 80f734edbde..e56ac96f063 100644 --- a/lightning/src/chain/keysinterface.rs +++ b/lightning/src/chain/keysinterface.rs @@ -254,7 +254,7 @@ pub trait ChannelKeys : Send+Clone + Writeable { /// state. Thus, needs its own method as sign_holder_commitment may enforce that we only ever /// get called once. #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] - fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result; + fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()>; /// Create a signature for the given input in a transaction spending an HTLC or commitment /// transaction output when our counterparty broadcasts an old state. @@ -499,18 +499,22 @@ impl ChannelKeys for InMemoryChannelKeys { fn sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey); - let sig = commitment_tx.trust().built_transaction().sign(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, secp_ctx); - let channel_parameters = self.get_channel_parameters(); let trusted_tx = commitment_tx.trust(); + let sig = trusted_tx.built_transaction().sign(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, secp_ctx); + let channel_parameters = self.get_channel_parameters(); let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), secp_ctx)?; Ok((sig, htlc_sigs)) } #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] - fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { + fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { let funding_pubkey = PublicKey::from_secret_key(secp_ctx, &self.funding_key); - let channel_funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey); - Ok(commitment_tx.trust().built_transaction().sign(&self.funding_key, &channel_funding_redeemscript, self.channel_value_satoshis, secp_ctx)) + let funding_redeemscript = make_funding_redeemscript(&funding_pubkey, &self.counterparty_pubkeys().funding_pubkey); + let trusted_tx = commitment_tx.trust(); + let sig = trusted_tx.built_transaction().sign(&self.funding_key, &funding_redeemscript, self.channel_value_satoshis, secp_ctx); + let channel_parameters = self.get_channel_parameters(); + let htlc_sigs = trusted_tx.get_htlc_sigs(&self.htlc_base_key, &channel_parameters.as_holder_broadcastable(), secp_ctx)?; + Ok((sig, htlc_sigs)) } fn sign_justice_transaction(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &Option, secp_ctx: &Secp256k1) -> Result { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 9575fd7f000..6bac24911ad 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -1616,27 +1616,26 @@ fn test_fee_spike_violation_fails_htlc() { // Get the EnforcingChannelKeys for each channel, which will be used to (1) get the keys // needed to sign the new commitment tx and (2) sign the new commitment tx. - let (local_revocation_basepoint, local_htlc_basepoint, local_secret, local_secret2) = { + let (local_revocation_basepoint, local_htlc_basepoint, local_secret, next_local_point) = { let chan_lock = nodes[0].node.channel_state.lock().unwrap(); let local_chan = chan_lock.by_id.get(&chan.2).unwrap(); let chan_keys = local_chan.get_keys(); let pubkeys = chan_keys.pubkeys(); (pubkeys.revocation_basepoint, pubkeys.htlc_basepoint, - chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER), chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)) + chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER), + chan_keys.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 2, &secp_ctx)) }; - let (remote_delayed_payment_basepoint, remote_htlc_basepoint, remote_secret1) = { + let (remote_delayed_payment_basepoint, remote_htlc_basepoint,remote_point) = { let chan_lock = nodes[1].node.channel_state.lock().unwrap(); let remote_chan = chan_lock.by_id.get(&chan.2).unwrap(); let chan_keys = remote_chan.get_keys(); let pubkeys = chan_keys.pubkeys(); (pubkeys.delayed_payment_basepoint, pubkeys.htlc_basepoint, - chan_keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1)) + chan_keys.get_per_commitment_point(INITIAL_COMMITMENT_NUMBER - 1, &secp_ctx)) }; // Assemble the set of keys we can use for signatures for our commitment_signed message. - let commitment_secret = SecretKey::from_slice(&remote_secret1).unwrap(); - let per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &commitment_secret); - let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &per_commitment_point, &remote_delayed_payment_basepoint, + let commit_tx_keys = chan_utils::TxCreationKeys::derive_new(&secp_ctx, &remote_point, &remote_delayed_payment_basepoint, &remote_htlc_basepoint, &local_revocation_basepoint, &local_htlc_basepoint).unwrap(); // Build the remote commitment transaction so we can sign it, and then later use the @@ -1680,10 +1679,11 @@ fn test_fee_spike_violation_fails_htlc() { let _ = nodes[1].node.get_and_clear_pending_msg_events(); // Send the RAA to nodes[1]. - let per_commitment_secret = local_secret; - let next_secret = SecretKey::from_slice(&local_secret2).unwrap(); - let next_per_commitment_point = PublicKey::from_secret_key(&secp_ctx, &next_secret); - let raa_msg = msgs::RevokeAndACK{ channel_id: chan.2, per_commitment_secret, next_per_commitment_point}; + let raa_msg = msgs::RevokeAndACK { + channel_id: chan.2, + per_commitment_secret: local_secret, + next_per_commitment_point: next_local_point + }; nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &raa_msg); let events = nodes[1].node.get_and_clear_pending_msg_events(); @@ -4269,7 +4269,6 @@ fn test_no_txn_manager_serialize_deserialize() { let fee_estimator: test_utils::TestFeeEstimator; let persister: test_utils::TestPersister; let new_chain_monitor: test_utils::TestChainMonitor; - let keys_manager: test_utils::TestKeysInterface; let nodes_0_deserialized: ChannelManager; let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -4284,12 +4283,12 @@ fn test_no_txn_manager_serialize_deserialize() { logger = test_utils::TestLogger::new(); fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; persister = test_utils::TestPersister::new(); - keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet); - new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, &keys_manager); + let keys_manager = &chanmon_cfgs[0].keys_manager; + new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager); nodes[0].chain_monitor = &new_chain_monitor; let mut chan_0_monitor_read = &chan_0_monitor_serialized.0[..]; let (_, mut chan_0_monitor) = <(BlockHash, ChannelMonitor)>::read( - &mut chan_0_monitor_read, &keys_manager).unwrap(); + &mut chan_0_monitor_read, keys_manager).unwrap(); assert!(chan_0_monitor_read.is_empty()); let mut nodes_0_read = &nodes_0_serialized[..]; @@ -4299,7 +4298,7 @@ fn test_no_txn_manager_serialize_deserialize() { channel_monitors.insert(chan_0_monitor.get_funding_txo().0, &mut chan_0_monitor); <(BlockHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs { default_config: config, - keys_manager: &keys_manager, + keys_manager, fee_estimator: &fee_estimator, chain_monitor: nodes[0].chain_monitor, tx_broadcaster: nodes[0].tx_broadcaster.clone(), @@ -4346,7 +4345,6 @@ fn test_manager_serialize_deserialize_events() { let persister: test_utils::TestPersister; let logger: test_utils::TestLogger; let new_chain_monitor: test_utils::TestChainMonitor; - let keys_manager: test_utils::TestKeysInterface; let nodes_0_deserialized: ChannelManager; let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -4355,8 +4353,8 @@ fn test_manager_serialize_deserialize_events() { let push_msat = 10001; let a_flags = InitFeatures::known(); let b_flags = InitFeatures::known(); - let node_a = nodes.pop().unwrap(); - let node_b = nodes.pop().unwrap(); + let node_a = nodes.remove(0); + let node_b = nodes.remove(0); node_a.node.create_channel(node_b.node.get_our_node_id(), channel_value, push_msat, 42, None).unwrap(); node_b.node.handle_open_channel(&node_a.node.get_our_node_id(), a_flags, &get_event_msg!(node_a, MessageSendEvent::SendOpenChannel, node_b.node.get_our_node_id())); node_a.node.handle_accept_channel(&node_b.node.get_our_node_id(), b_flags, &get_event_msg!(node_b, MessageSendEvent::SendAcceptChannel, node_a.node.get_our_node_id())); @@ -4394,12 +4392,12 @@ fn test_manager_serialize_deserialize_events() { fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; logger = test_utils::TestLogger::new(); persister = test_utils::TestPersister::new(); - keys_manager = test_utils::TestKeysInterface::new(&nodes[0].node_seed, Network::Testnet); - new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, &keys_manager); + let keys_manager = &chanmon_cfgs[0].keys_manager; + new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager); nodes[0].chain_monitor = &new_chain_monitor; let mut chan_0_monitor_read = &chan_0_monitor_serialized.0[..]; let (_, mut chan_0_monitor) = <(BlockHash, ChannelMonitor)>::read( - &mut chan_0_monitor_read, &keys_manager).unwrap(); + &mut chan_0_monitor_read, keys_manager).unwrap(); assert!(chan_0_monitor_read.is_empty()); let mut nodes_0_read = &nodes_0_serialized[..]; @@ -4409,7 +4407,7 @@ fn test_manager_serialize_deserialize_events() { channel_monitors.insert(chan_0_monitor.get_funding_txo().0, &mut chan_0_monitor); <(BlockHash, ChannelManager)>::read(&mut nodes_0_read, ChannelManagerReadArgs { default_config: config, - keys_manager: &keys_manager, + keys_manager, fee_estimator: &fee_estimator, chain_monitor: nodes[0].chain_monitor, tx_broadcaster: nodes[0].tx_broadcaster.clone(), @@ -4470,7 +4468,6 @@ fn test_simple_manager_serialize_deserialize() { let fee_estimator: test_utils::TestFeeEstimator; let persister: test_utils::TestPersister; let new_chain_monitor: test_utils::TestChainMonitor; - let keys_manager: &test_utils::TestKeysInterface; let nodes_0_deserialized: ChannelManager; let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); @@ -4487,7 +4484,7 @@ fn test_simple_manager_serialize_deserialize() { logger = test_utils::TestLogger::new(); fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; persister = test_utils::TestPersister::new(); - keys_manager = &chanmon_cfgs[0].keys_manager; + let keys_manager = &chanmon_cfgs[0].keys_manager; new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager); nodes[0].chain_monitor = &new_chain_monitor; let mut chan_0_monitor_read = &chan_0_monitor_serialized.0[..]; @@ -4532,7 +4529,6 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { let fee_estimator: test_utils::TestFeeEstimator; let persister: test_utils::TestPersister; let new_chain_monitor: test_utils::TestChainMonitor; - let keys_manager: &test_utils::TestKeysInterface; let nodes_0_deserialized: ChannelManager; let mut nodes = create_network(4, &node_cfgs, &node_chanmgrs); create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::known(), InitFeatures::known()); @@ -4568,7 +4564,7 @@ fn test_manager_serialize_deserialize_inconsistent_monitor() { logger = test_utils::TestLogger::new(); fee_estimator = test_utils::TestFeeEstimator { sat_per_kw: 253 }; persister = test_utils::TestPersister::new(); - keys_manager = &chanmon_cfgs[0].keys_manager; + let keys_manager = &chanmon_cfgs[0].keys_manager; new_chain_monitor = test_utils::TestChainMonitor::new(Some(nodes[0].chain_source), nodes[0].tx_broadcaster.clone(), &logger, &fee_estimator, &persister, keys_manager); nodes[0].chain_monitor = &new_chain_monitor; @@ -7374,8 +7370,10 @@ fn test_user_configurable_csv_delay() { fn test_data_loss_protect() { // We want to be sure that : // * we don't broadcast our Local Commitment Tx in case of fallen behind + // (but this is not quite true - we broadcast during Drop because chanmon is out of sync with chanmgr) // * we close channel in case of detecting other being fallen behind // * we are able to claim our own outputs thanks to to_remote being static + // TODO: this test is incomplete and the data_loss_protect implementation is incomplete - see issue #775 let persister; let logger; let fee_estimator; @@ -8084,9 +8082,11 @@ fn test_counterparty_raa_skip_no_crash() { let mut guard = nodes[0].node.channel_state.lock().unwrap(); let keys = &guard.by_id.get_mut(&channel_id).unwrap().holder_keys; const INITIAL_COMMITMENT_NUMBER: u64 = (1 << 48) - 1; + let per_commitment_secret = keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER); + // Must revoke without gaps + keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 1); let next_per_commitment_point = PublicKey::from_secret_key(&Secp256k1::new(), &SecretKey::from_slice(&keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER - 2)).unwrap()); - let per_commitment_secret = keys.release_commitment_secret(INITIAL_COMMITMENT_NUMBER); nodes[1].node.handle_revoke_and_ack(&nodes[0].node.get_our_node_id(), &msgs::RevokeAndACK { channel_id, per_commitment_secret, next_per_commitment_point }); diff --git a/lightning/src/ln/onchaintx.rs b/lightning/src/ln/onchaintx.rs index 02776fdefe5..2d7d417d255 100644 --- a/lightning/src/ln/onchaintx.rs +++ b/lightning/src/ln/onchaintx.rs @@ -953,7 +953,7 @@ impl OnchainTxHandler { #[cfg(any(test, feature="unsafe_revoked_tx_signing"))] pub(crate) fn get_fully_signed_copy_holder_tx(&mut self, funding_redeemscript: &Script) -> Transaction { - let (sig, htlc_sigs) = self.key_storage.sign_holder_commitment_and_htlcs(&self.holder_commitment, &self.secp_ctx).expect("sign holder commitment"); + let (sig, htlc_sigs) = self.key_storage.unsafe_sign_holder_commitment_and_htlcs(&self.holder_commitment, &self.secp_ctx).expect("sign holder commitment"); self.holder_htlc_sigs = Some(Self::extract_holder_sigs(&self.holder_commitment, htlc_sigs)); self.holder_commitment.add_holder_sig(funding_redeemscript, sig) } diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index 8229f604860..fa46edeb7e4 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -24,6 +24,9 @@ use util::ser::{Writeable, Writer, Readable}; use std::io::Error; use ln::msgs::DecodeError; +/// Initial value for revoked commitment downward counter +pub const INITIAL_REVOKED_COMMITMENT_NUMBER: u64 = 1 << 48; + /// An implementation of ChannelKeys that enforces some policy checks. /// /// Eventually we will probably want to expose a variant of this which would essentially @@ -31,7 +34,8 @@ use ln::msgs::DecodeError; #[derive(Clone)] pub struct EnforcingChannelKeys { pub inner: InMemoryChannelKeys, - last_commitment_number: Arc>>, + pub(crate) last_commitment_number: Arc>>, + pub(crate) revoked_commitment: Arc>, } impl EnforcingChannelKeys { @@ -39,6 +43,15 @@ impl EnforcingChannelKeys { Self { inner, last_commitment_number: Arc::new(Mutex::new(None)), + revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)) + } + } + + pub fn new_with_revoked(inner: InMemoryChannelKeys, revoked_commitment: Arc>) -> Self { + Self { + inner, + last_commitment_number: Arc::new(Mutex::new(None)), + revoked_commitment } } } @@ -49,7 +62,13 @@ impl ChannelKeys for EnforcingChannelKeys { } fn release_commitment_secret(&self, idx: u64) -> [u8; 32] { - // TODO: enforce the ChannelKeys contract - error here if we already signed this commitment + println!("XXX revoke {} for {}", idx, self.inner.commitment_seed[0]); + + { + let mut revoked = self.revoked_commitment.lock().unwrap(); + assert!(idx == *revoked || idx == *revoked - 1, "can only revoke the current or next unrevoked commitment - trying {}, revoked {}", idx, *revoked); + *revoked = idx; + } self.inner.release_commitment_secret(idx) } @@ -77,6 +96,15 @@ impl ChannelKeys for EnforcingChannelKeys { let commitment_txid = trusted_tx.txid(); let holder_csv = self.inner.counterparty_selected_contest_delay(); + let revoked = self.revoked_commitment.lock().unwrap(); + let commitment_number = trusted_tx.commitment_number(); + println!("XXX sign {} for {}", commitment_number, self.inner.commitment_seed[0]); + if *revoked - 1 != commitment_number && *revoked - 2 != commitment_number { + println!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}", + *revoked, commitment_number, self.inner.commitment_seed[0]); + return Err(()); + } + for (this_htlc, sig) in trusted_tx.htlcs().iter().zip(&commitment_tx.counterparty_htlc_sigs) { assert!(this_htlc.transaction_output_index.is_some()); let keys = trusted_tx.keys(); @@ -94,8 +122,8 @@ impl ChannelKeys for EnforcingChannelKeys { } #[cfg(any(test,feature = "unsafe_revoked_tx_signing"))] - fn unsafe_sign_holder_commitment(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result { - Ok(self.inner.unsafe_sign_holder_commitment(commitment_tx, secp_ctx).unwrap()) + fn unsafe_sign_holder_commitment_and_htlcs(&self, commitment_tx: &HolderCommitmentTransaction, secp_ctx: &Secp256k1) -> Result<(Signature, Vec), ()> { + Ok(self.inner.unsafe_sign_holder_commitment_and_htlcs(commitment_tx, secp_ctx).unwrap()) } fn sign_justice_transaction(&self, justice_tx: &Transaction, input: usize, amount: u64, per_commitment_key: &SecretKey, htlc: &Option, secp_ctx: &Secp256k1) -> Result { @@ -131,11 +159,12 @@ impl Writeable for EnforcingChannelKeys { impl Readable for EnforcingChannelKeys { fn read(reader: &mut R) -> Result { - let inner = Readable::read(reader)?; + let inner: InMemoryChannelKeys = Readable::read(reader)?; let last_commitment_number = Readable::read(reader)?; Ok(EnforcingChannelKeys { inner, - last_commitment_number: Arc::new(Mutex::new(last_commitment_number)) + last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), + revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)), }) } } diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index b55c19e3936..a93ce3cfb73 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -18,7 +18,7 @@ use chain::keysinterface; use ln::features::{ChannelFeatures, InitFeatures}; use ln::msgs; use ln::msgs::OptionalField; -use util::enforcing_trait_impls::EnforcingChannelKeys; +use util::enforcing_trait_impls::{EnforcingChannelKeys, INITIAL_REVOKED_COMMITMENT_NUMBER}; use util::events; use util::logger::{Logger, Level, Record}; use util::ser::{Readable, ReadableArgs, Writer, Writeable}; @@ -35,7 +35,7 @@ use bitcoin::secp256k1::{SecretKey, PublicKey, Secp256k1, Signature}; use regex; use std::time::Duration; -use std::sync::Mutex; +use std::sync::{Mutex, Arc}; use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; use std::{cmp, mem}; use std::collections::{HashMap, HashSet}; @@ -422,6 +422,7 @@ pub struct TestKeysInterface { backing: keysinterface::KeysManager, pub override_session_priv: Mutex>, pub override_channel_id_priv: Mutex>, + revoked_commitments: Mutex>>>, } impl keysinterface::KeysInterface for TestKeysInterface { @@ -431,7 +432,9 @@ impl keysinterface::KeysInterface for TestKeysInterface { fn get_destination_script(&self) -> Script { self.backing.get_destination_script() } fn get_shutdown_pubkey(&self) -> PublicKey { self.backing.get_shutdown_pubkey() } fn get_channel_keys(&self, inbound: bool, channel_value_satoshis: u64) -> EnforcingChannelKeys { - EnforcingChannelKeys::new(self.backing.get_channel_keys(inbound, channel_value_satoshis)) + let keys = self.backing.get_channel_keys(inbound, channel_value_satoshis); + let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); + EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment) } fn get_secure_random_bytes(&self) -> [u8; 32] { @@ -449,11 +452,23 @@ impl keysinterface::KeysInterface for TestKeysInterface { self.backing.get_secure_random_bytes() } - fn read_chan_signer(&self, reader: &[u8]) -> Result { - EnforcingChannelKeys::read(&mut std::io::Cursor::new(reader)) + fn read_chan_signer(&self, buffer: &[u8]) -> Result { + let mut reader = std::io::Cursor::new(buffer); + + let inner: InMemoryChannelKeys = Readable::read(&mut reader)?; + let revoked_commitment = self.make_revoked_commitment_cell(inner.commitment_seed); + + let last_commitment_number = Readable::read(&mut reader)?; + + Ok(EnforcingChannelKeys { + inner, + last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), + revoked_commitment, + }) } } + impl TestKeysInterface { pub fn new(seed: &[u8; 32], network: Network) -> Self { let now = Duration::from_secs(genesis_block(network).header.time as u64); @@ -461,10 +476,22 @@ impl TestKeysInterface { backing: keysinterface::KeysManager::new(seed, network, now.as_secs(), now.subsec_nanos()), override_session_priv: Mutex::new(None), override_channel_id_priv: Mutex::new(None), + revoked_commitments: Mutex::new(HashMap::new()), } } pub fn derive_channel_keys(&self, channel_value_satoshis: u64, user_id_1: u64, user_id_2: u64) -> EnforcingChannelKeys { - EnforcingChannelKeys::new(self.backing.derive_channel_keys(channel_value_satoshis, user_id_1, user_id_2)) + let keys = self.backing.derive_channel_keys(channel_value_satoshis, user_id_1, user_id_2); + let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); + EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment) + } + + fn make_revoked_commitment_cell(&self, commitment_seed: [u8; 32]) -> Arc> { + let mut revoked_commitments = self.revoked_commitments.lock().unwrap(); + if !revoked_commitments.contains_key(&commitment_seed) { + revoked_commitments.insert(commitment_seed, Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER))); + } + let cell = revoked_commitments.get(&commitment_seed).unwrap(); + Arc::clone(cell) } } From bd4345d6a23fa1b842374e0c31122e8987f0d03b Mon Sep 17 00:00:00 2001 From: Devrandom Date: Sat, 5 Dec 2020 18:56:27 +0100 Subject: [PATCH 3/4] Fix fuzzing issue with revocation --- fuzz/src/chanmon_consistency.rs | 58 +++++++++++++++------ lightning/src/util/enforcing_trait_impls.rs | 4 +- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index 5466414b2cd..cfb8f41b5a2 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -38,7 +38,7 @@ use lightning::chain::keysinterface::{KeysInterface, InMemoryChannelKeys}; use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage, PaymentSecret, PaymentSendFailure, ChannelManagerReadArgs}; use lightning::ln::features::{ChannelFeatures, InitFeatures, NodeFeatures}; use lightning::ln::msgs::{CommitmentUpdate, ChannelMessageHandler, DecodeError, ErrorAction, UpdateAddHTLC, Init}; -use lightning::util::enforcing_trait_impls::EnforcingChannelKeys; +use lightning::util::enforcing_trait_impls::{EnforcingChannelKeys, INITIAL_REVOKED_COMMITMENT_NUMBER}; use lightning::util::errors::APIError; use lightning::util::events; use lightning::util::logger::Logger; @@ -146,6 +146,7 @@ impl chain::Watch for TestChainMonitor { struct KeyProvider { node_id: u8, rand_bytes_id: atomic::AtomicU8, + revoked_commitments: Mutex>>>, } impl KeysInterface for KeyProvider { type ChanKeySigner = EnforcingChannelKeys; @@ -168,17 +169,20 @@ impl KeysInterface for KeyProvider { fn get_channel_keys(&self, _inbound: bool, channel_value_satoshis: u64) -> EnforcingChannelKeys { let secp_ctx = Secp256k1::signing_only(); - EnforcingChannelKeys::new(InMemoryChannelKeys::new( + let id = self.rand_bytes_id.fetch_add(1, atomic::Ordering::Relaxed); + let keys = InMemoryChannelKeys::new( &secp_ctx, SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, self.node_id]).unwrap(), SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 5, self.node_id]).unwrap(), SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, self.node_id]).unwrap(), SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 7, self.node_id]).unwrap(), SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 8, self.node_id]).unwrap(), - [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 9, self.node_id], + [id, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 9, self.node_id], channel_value_satoshis, (0, 0), - )) + ); + let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); + EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment) } fn get_secure_random_bytes(&self) -> [u8; 32] { @@ -186,8 +190,30 @@ impl KeysInterface for KeyProvider { [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, id, 11, self.node_id] } - fn read_chan_signer(&self, data: &[u8]) -> Result { - EnforcingChannelKeys::read(&mut std::io::Cursor::new(data)) + fn read_chan_signer(&self, buffer: &[u8]) -> Result { + let mut reader = std::io::Cursor::new(buffer); + + let inner: InMemoryChannelKeys = Readable::read(&mut reader)?; + let revoked_commitment = self.make_revoked_commitment_cell(inner.commitment_seed); + + let last_commitment_number = Readable::read(&mut reader)?; + + Ok(EnforcingChannelKeys { + inner, + last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), + revoked_commitment, + }) + } +} + +impl KeyProvider { + fn make_revoked_commitment_cell(&self, commitment_seed: [u8; 32]) -> Arc> { + let mut revoked_commitments = self.revoked_commitments.lock().unwrap(); + if !revoked_commitments.contains_key(&commitment_seed) { + revoked_commitments.insert(commitment_seed, Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER))); + } + let cell = revoked_commitments.get(&commitment_seed).unwrap(); + Arc::clone(cell) } } @@ -288,22 +314,22 @@ pub fn do_test(data: &[u8], out: Out) { let logger: Arc = Arc::new(test_logger::TestLogger::new($node_id.to_string(), out.clone())); let monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), fee_est.clone(), Arc::new(TestPersister{}))); - let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU8::new(0) }); + let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU8::new(0), revoked_commitments: Mutex::new(HashMap::new()) }); let mut config = UserConfig::default(); config.channel_options.fee_proportional_millionths = 0; config.channel_options.announced_channel = true; config.peer_channel_config_limits.min_dust_limit_satoshis = 0; (ChannelManager::new(Network::Bitcoin, fee_est.clone(), monitor.clone(), broadcast.clone(), Arc::clone(&logger), keys_manager.clone(), config, 0), - monitor) + monitor, keys_manager) } } } macro_rules! reload_node { - ($ser: expr, $node_id: expr, $old_monitors: expr) => { { + ($ser: expr, $node_id: expr, $old_monitors: expr, $keys_manager: expr) => { { + let keys_manager = Arc::clone(& $keys_manager); let logger: Arc = Arc::new(test_logger::TestLogger::new($node_id.to_string(), out.clone())); let chain_monitor = Arc::new(TestChainMonitor::new(broadcast.clone(), logger.clone(), fee_est.clone(), Arc::new(TestPersister{}))); - let keys_manager = Arc::new(KeyProvider { node_id: $node_id, rand_bytes_id: atomic::AtomicU8::new(0) }); let mut config = UserConfig::default(); config.channel_options.fee_proportional_millionths = 0; config.channel_options.announced_channel = true; @@ -440,9 +466,9 @@ pub fn do_test(data: &[u8], out: Out) { // 3 nodes is enough to hit all the possible cases, notably unknown-source-unknown-dest // forwarding. - let (node_a, mut monitor_a) = make_node!(0); - let (node_b, mut monitor_b) = make_node!(1); - let (node_c, mut monitor_c) = make_node!(2); + let (node_a, mut monitor_a, keys_manager_a) = make_node!(0); + let (node_b, mut monitor_b, keys_manager_b) = make_node!(1); + let (node_c, mut monitor_c, keys_manager_c) = make_node!(2); let mut nodes = [node_a, node_b, node_c]; @@ -793,7 +819,7 @@ pub fn do_test(data: &[u8], out: Out) { chan_a_disconnected = true; drain_msg_events_on_disconnect!(0); } - let (new_node_a, new_monitor_a) = reload_node!(node_a_ser, 0, monitor_a); + let (new_node_a, new_monitor_a) = reload_node!(node_a_ser, 0, monitor_a, keys_manager_a); nodes[0] = new_node_a; monitor_a = new_monitor_a; }, @@ -810,7 +836,7 @@ pub fn do_test(data: &[u8], out: Out) { nodes[2].get_and_clear_pending_msg_events(); bc_events.clear(); } - let (new_node_b, new_monitor_b) = reload_node!(node_b_ser, 1, monitor_b); + let (new_node_b, new_monitor_b) = reload_node!(node_b_ser, 1, monitor_b, keys_manager_b); nodes[1] = new_node_b; monitor_b = new_monitor_b; }, @@ -820,7 +846,7 @@ pub fn do_test(data: &[u8], out: Out) { chan_b_disconnected = true; drain_msg_events_on_disconnect!(2); } - let (new_node_c, new_monitor_c) = reload_node!(node_c_ser, 2, monitor_c); + let (new_node_c, new_monitor_c) = reload_node!(node_c_ser, 2, monitor_c, keys_manager_c); nodes[2] = new_node_c; monitor_c = new_monitor_c; }, diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index fa46edeb7e4..af497cbbe5b 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -34,8 +34,8 @@ pub const INITIAL_REVOKED_COMMITMENT_NUMBER: u64 = 1 << 48; #[derive(Clone)] pub struct EnforcingChannelKeys { pub inner: InMemoryChannelKeys, - pub(crate) last_commitment_number: Arc>>, - pub(crate) revoked_commitment: Arc>, + pub last_commitment_number: Arc>>, + pub revoked_commitment: Arc>, } impl EnforcingChannelKeys { From 142b0d624e8ef71aea4aeb1d3591c6a5b59a771d Mon Sep 17 00:00:00 2001 From: Devrandom Date: Wed, 13 Jan 2021 17:36:07 -0800 Subject: [PATCH 4/4] Let some tests disable revocation policy check When simulating a bad actor that broadcasts a revoked tx, the policy check would otherwise panic. --- fuzz/src/chanmon_consistency.rs | 3 +- lightning/src/ln/functional_test_utils.rs | 1 - lightning/src/ln/functional_tests.rs | 27 ++++++++++---- lightning/src/util/enforcing_trait_impls.rs | 41 ++++++++++++++------- lightning/src/util/test_utils.rs | 7 +++- 5 files changed, 54 insertions(+), 25 deletions(-) diff --git a/fuzz/src/chanmon_consistency.rs b/fuzz/src/chanmon_consistency.rs index cfb8f41b5a2..8e79ac5f9e9 100644 --- a/fuzz/src/chanmon_consistency.rs +++ b/fuzz/src/chanmon_consistency.rs @@ -182,7 +182,7 @@ impl KeysInterface for KeyProvider { (0, 0), ); let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); - EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment) + EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment, false) } fn get_secure_random_bytes(&self) -> [u8; 32] { @@ -202,6 +202,7 @@ impl KeysInterface for KeyProvider { inner, last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), revoked_commitment, + disable_revocation_policy_check: false, }) } } diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index e487b152840..a7394a84b75 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -97,7 +97,6 @@ pub struct TestChanMonCfg { pub persister: test_utils::TestPersister, pub logger: test_utils::TestLogger, pub keys_manager: test_utils::TestKeysInterface, - } pub struct NodeCfg<'a> { diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 6bac24911ad..e4d7a33c8c1 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2470,7 +2470,9 @@ fn test_justice_tx() { bob_config.peer_channel_config_limits.force_announced_channel_preference = false; bob_config.own_channel_config.our_to_self_delay = 6 * 24 * 3; let user_cfgs = [Some(alice_config), Some(bob_config)]; - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; + chanmon_cfgs[1].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &user_cfgs); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -2600,7 +2602,8 @@ fn revoked_output_claim() { #[test] fn claim_htlc_outputs_shared_tx() { // Node revoked old state, htlcs haven't time out yet, claim them in shared justice tx - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -2670,7 +2673,8 @@ fn claim_htlc_outputs_shared_tx() { #[test] fn claim_htlc_outputs_single_tx() { // Node revoked old state, htlcs have timed out, claim each of them in separated justice tx - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -4993,7 +4997,8 @@ fn test_static_spendable_outputs_justice_tx_revoked_commitment_tx() { #[test] fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() { - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -5059,7 +5064,8 @@ fn test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx() { #[test] fn test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx() { - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[1].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -7040,7 +7046,8 @@ fn do_test_failure_delay_dust_htlc_local_commitment(announce_latest: bool) { // We can have at most two valid local commitment tx, so both cases must be covered, and both txs must be checked to get them all as // HTLC could have been removed from lastest local commitment tx but still valid until we get remote RAA - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); @@ -7379,7 +7386,10 @@ fn test_data_loss_protect() { let fee_estimator; let tx_broadcaster; let chain_source; - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + // We broadcast during Drop because chanmon is out of sync with chanmgr, which would cause a panic + // during signing due to revoked tx + chanmon_cfgs[0].keys_manager.disable_revocation_policy_check = true; let keys_manager = &chanmon_cfgs[0].keys_manager; let monitor; let node_state_0; @@ -7699,7 +7709,8 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { // In case of penalty txn with too low feerates for getting into mempools, RBF-bump them to sure // we're able to claim outputs on revoked HTLC transactions before timelocks expiration - let chanmon_cfgs = create_chanmon_cfgs(2); + let mut chanmon_cfgs = create_chanmon_cfgs(2); + chanmon_cfgs[1].keys_manager.disable_revocation_policy_check = true; let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); let nodes = create_network(2, &node_cfgs, &node_chanmgrs); diff --git a/lightning/src/util/enforcing_trait_impls.rs b/lightning/src/util/enforcing_trait_impls.rs index af497cbbe5b..88c1754f135 100644 --- a/lightning/src/util/enforcing_trait_impls.rs +++ b/lightning/src/util/enforcing_trait_impls.rs @@ -27,31 +27,49 @@ use ln::msgs::DecodeError; /// Initial value for revoked commitment downward counter pub const INITIAL_REVOKED_COMMITMENT_NUMBER: u64 = 1 << 48; -/// An implementation of ChannelKeys that enforces some policy checks. +/// An implementation of ChannelKeys that enforces some policy checks. The current checks +/// are an incomplete set. They include: +/// +/// - When signing, the holder transaction has not been revoked +/// - When revoking, the holder transaction has not been signed +/// - The holder commitment number is monotonic and without gaps +/// - The counterparty commitment number is monotonic and without gaps +/// - The pre-derived keys and pre-built transaction in CommitmentTransaction were correctly built /// /// Eventually we will probably want to expose a variant of this which would essentially /// be what you'd want to run on a hardware wallet. #[derive(Clone)] pub struct EnforcingChannelKeys { pub inner: InMemoryChannelKeys, + /// The last counterparty commitment number we signed, backwards counting pub last_commitment_number: Arc>>, + /// The last holder commitment number we revoked, backwards counting pub revoked_commitment: Arc>, + pub disable_revocation_policy_check: bool, } impl EnforcingChannelKeys { + /// Construct an EnforcingChannelKeys pub fn new(inner: InMemoryChannelKeys) -> Self { Self { inner, last_commitment_number: Arc::new(Mutex::new(None)), - revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)) + revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)), + disable_revocation_policy_check: false } } - pub fn new_with_revoked(inner: InMemoryChannelKeys, revoked_commitment: Arc>) -> Self { + /// Construct an EnforcingChannelKeys with externally managed storage + /// + /// Since there are multiple copies of this struct for each channel, some coordination is needed + /// so that all copies are aware of revocations. A pointer to this state is provided here, usually + /// by an implementation of KeysInterface. + pub fn new_with_revoked(inner: InMemoryChannelKeys, revoked_commitment: Arc>, disable_revocation_policy_check: bool) -> Self { Self { inner, last_commitment_number: Arc::new(Mutex::new(None)), - revoked_commitment + revoked_commitment, + disable_revocation_policy_check } } } @@ -62,8 +80,6 @@ impl ChannelKeys for EnforcingChannelKeys { } fn release_commitment_secret(&self, idx: u64) -> [u8; 32] { - println!("XXX revoke {} for {}", idx, self.inner.commitment_seed[0]); - { let mut revoked = self.revoked_commitment.lock().unwrap(); assert!(idx == *revoked || idx == *revoked - 1, "can only revoke the current or next unrevoked commitment - trying {}, revoked {}", idx, *revoked); @@ -98,11 +114,11 @@ impl ChannelKeys for EnforcingChannelKeys { let revoked = self.revoked_commitment.lock().unwrap(); let commitment_number = trusted_tx.commitment_number(); - println!("XXX sign {} for {}", commitment_number, self.inner.commitment_seed[0]); if *revoked - 1 != commitment_number && *revoked - 2 != commitment_number { - println!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}", - *revoked, commitment_number, self.inner.commitment_seed[0]); - return Err(()); + if !self.disable_revocation_policy_check { + panic!("can only sign the next two unrevoked commitment numbers, revoked={} vs requested={} for {}", + *revoked, commitment_number, self.inner.commitment_seed[0]) + } } for (this_htlc, sig) in trusted_tx.htlcs().iter().zip(&commitment_tx.counterparty_htlc_sigs) { @@ -116,8 +132,6 @@ impl ChannelKeys for EnforcingChannelKeys { secp_ctx.verify(&sighash, sig, &keys.countersignatory_htlc_key).unwrap(); } - // TODO: enforce the ChannelKeys contract - error if this commitment was already revoked - // TODO: need the commitment number Ok(self.inner.sign_holder_commitment_and_htlcs(commitment_tx, secp_ctx).unwrap()) } @@ -159,12 +173,13 @@ impl Writeable for EnforcingChannelKeys { impl Readable for EnforcingChannelKeys { fn read(reader: &mut R) -> Result { - let inner: InMemoryChannelKeys = Readable::read(reader)?; + let inner = Readable::read(reader)?; let last_commitment_number = Readable::read(reader)?; Ok(EnforcingChannelKeys { inner, last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), revoked_commitment: Arc::new(Mutex::new(INITIAL_REVOKED_COMMITMENT_NUMBER)), + disable_revocation_policy_check: false, }) } } diff --git a/lightning/src/util/test_utils.rs b/lightning/src/util/test_utils.rs index a93ce3cfb73..4bf8aa39db7 100644 --- a/lightning/src/util/test_utils.rs +++ b/lightning/src/util/test_utils.rs @@ -422,6 +422,7 @@ pub struct TestKeysInterface { backing: keysinterface::KeysManager, pub override_session_priv: Mutex>, pub override_channel_id_priv: Mutex>, + pub disable_revocation_policy_check: bool, revoked_commitments: Mutex>>>, } @@ -434,7 +435,7 @@ impl keysinterface::KeysInterface for TestKeysInterface { fn get_channel_keys(&self, inbound: bool, channel_value_satoshis: u64) -> EnforcingChannelKeys { let keys = self.backing.get_channel_keys(inbound, channel_value_satoshis); let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); - EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment) + EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment, self.disable_revocation_policy_check) } fn get_secure_random_bytes(&self) -> [u8; 32] { @@ -464,6 +465,7 @@ impl keysinterface::KeysInterface for TestKeysInterface { inner, last_commitment_number: Arc::new(Mutex::new(last_commitment_number)), revoked_commitment, + disable_revocation_policy_check: self.disable_revocation_policy_check, }) } } @@ -476,13 +478,14 @@ impl TestKeysInterface { backing: keysinterface::KeysManager::new(seed, network, now.as_secs(), now.subsec_nanos()), override_session_priv: Mutex::new(None), override_channel_id_priv: Mutex::new(None), + disable_revocation_policy_check: false, revoked_commitments: Mutex::new(HashMap::new()), } } pub fn derive_channel_keys(&self, channel_value_satoshis: u64, user_id_1: u64, user_id_2: u64) -> EnforcingChannelKeys { let keys = self.backing.derive_channel_keys(channel_value_satoshis, user_id_1, user_id_2); let revoked_commitment = self.make_revoked_commitment_cell(keys.commitment_seed); - EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment) + EnforcingChannelKeys::new_with_revoked(keys, revoked_commitment, self.disable_revocation_policy_check) } fn make_revoked_commitment_cell(&self, commitment_seed: [u8; 32]) -> Arc> {