Remove BlockNotifier
authorJeffrey Czyz <jkczyz@gmail.com>
Wed, 29 Jul 2020 01:08:46 +0000 (18:08 -0700)
committerJeffrey Czyz <jkczyz@gmail.com>
Thu, 1 Oct 2020 05:39:56 +0000 (22:39 -0700)
BlockNotifier is a convenience for handing blocks to listeners. However,
it requires that each listener conforms to the ChainListener interface.
Additionally, there are only two listeners, ChannelManager and
ChainMonitor, the latter of which may not be used when monitoring
channels remotely. Remove BlockNotifier since it doesn't provide much
value and constrains each listener to a specific interface.

lightning/src/chain/chaininterface.rs
lightning/src/ln/channelmanager.rs
lightning/src/ln/functional_test_utils.rs
lightning/src/ln/functional_tests.rs

index aa69d8596f63efd15c09011fc50c5af8795c74a2..f65ae3611f623af549da1f75a556463363f8e0ae 100644 (file)
 //! Includes traits for monitoring and receiving notifications of new blocks and block
 //! disconnections, transaction broadcasting, and feerate information requests.
 
-use bitcoin::blockdata::block::{Block, BlockHeader};
+use bitcoin::blockdata::block::BlockHeader;
 use bitcoin::blockdata::transaction::Transaction;
 use bitcoin::blockdata::script::Script;
 use bitcoin::hash_types::Txid;
 
-use std::sync::{Mutex, Arc};
 use std::collections::HashSet;
-use std::ops::Deref;
-use std::marker::PhantomData;
-use std::ptr;
 
 /// An interface to send a transaction to the Bitcoin network.
 pub trait BroadcasterInterface: Sync + Send {
@@ -161,143 +157,3 @@ impl ChainWatchedUtil {
                false
        }
 }
-
-/// BlockNotifierArc is useful when you need a BlockNotifier that points to ChainListeners with
-/// static lifetimes, e.g. when you're using lightning-net-tokio (since tokio::spawn requires
-/// parameters with static lifetimes). Other times you can afford a reference, which is more
-/// efficient, in which case BlockNotifierRef is a more appropriate type. Defining these type
-/// aliases prevents issues such as overly long function definitions.
-///
-/// (C-not exported) as we let clients handle any reference counting they need to do
-pub type BlockNotifierArc = Arc<BlockNotifier<'static, Arc<ChainListener>>>;
-
-/// BlockNotifierRef is useful when you want a BlockNotifier that points to ChainListeners
-/// with nonstatic lifetimes. This is useful for when static lifetimes are not needed. Nonstatic
-/// lifetimes are more efficient but less flexible, and should be used by default unless static
-/// lifetimes are required, e.g. when you're using lightning-net-tokio (since tokio::spawn
-/// requires parameters with static lifetimes), in which case BlockNotifierArc is a more
-/// appropriate type. Defining these type aliases for common usages prevents issues such as
-/// overly long function definitions.
-pub type BlockNotifierRef<'a> = BlockNotifier<'a, &'a ChainListener>;
-
-/// Utility for notifying listeners when blocks are connected or disconnected.
-///
-/// Rather than using a plain BlockNotifier, it is preferable to use either a BlockNotifierArc
-/// or a BlockNotifierRef for conciseness. See their documentation for more details, but essentially
-/// you should default to using a BlockNotifierRef, and use a BlockNotifierArc instead when you
-/// require ChainListeners with static lifetimes, such as when you're using lightning-net-tokio.
-pub struct BlockNotifier<'a, CL: Deref + 'a>
-               where CL::Target: ChainListener + 'a {
-       listeners: Mutex<Vec<CL>>,
-       phantom: PhantomData<&'a ()>,
-}
-
-impl<'a, CL: Deref + 'a> BlockNotifier<'a, CL>
-               where CL::Target: ChainListener + 'a {
-       /// Constructs a new BlockNotifier without any listeners.
-       pub fn new() -> BlockNotifier<'a, CL> {
-               BlockNotifier {
-                       listeners: Mutex::new(Vec::new()),
-                       phantom: PhantomData,
-               }
-       }
-
-       /// Register the given listener to receive events.
-       pub fn register_listener(&self, listener: CL) {
-               let mut vec = self.listeners.lock().unwrap();
-               vec.push(listener);
-       }
-       /// Unregister the given listener to no longer
-       /// receive events.
-       ///
-       /// If the same listener is registered multiple times, unregistering
-       /// will remove ALL occurrences of that listener. Comparison is done using
-       /// the pointer returned by the Deref trait implementation.
-       ///
-       /// (C-not exported) because the equality check would always fail
-       pub fn unregister_listener(&self, listener: CL) {
-               let mut vec = self.listeners.lock().unwrap();
-               // item is a ref to an abstract thing that dereferences to a ChainListener,
-               // so dereference it twice to get the ChainListener itself
-               vec.retain(|item | !ptr::eq(&(**item), &(*listener)));
-       }
-
-       /// Notify listeners that a block was connected.
-       pub fn block_connected(&self, block: &Block, height: u32) {
-               let txdata: Vec<_> = block.txdata.iter().enumerate().collect();
-               let listeners = self.listeners.lock().unwrap();
-               for listener in listeners.iter() {
-                       listener.block_connected(&block.header, &txdata, height);
-               }
-       }
-
-       /// Notify listeners that a block was disconnected.
-       pub fn block_disconnected(&self, header: &BlockHeader, disconnected_height: u32) {
-               let listeners = self.listeners.lock().unwrap();
-               for listener in listeners.iter() {
-                       listener.block_disconnected(&header, disconnected_height);
-               }
-       }
-}
-
-#[cfg(test)]
-mod tests {
-       use bitcoin::blockdata::block::BlockHeader;
-       use bitcoin::blockdata::transaction::Transaction;
-       use super::{BlockNotifier, ChainListener};
-       use std::ptr;
-
-       struct TestChainListener(u8);
-
-       impl ChainListener for TestChainListener {
-               fn block_connected(&self, _header: &BlockHeader, _txdata: &[(usize, &Transaction)], _height: u32) {}
-               fn block_disconnected(&self, _header: &BlockHeader, _disconnected_height: u32) {}
-       }
-
-       #[test]
-       fn register_listener_test() {
-               let block_notifier = BlockNotifier::new();
-               assert_eq!(block_notifier.listeners.lock().unwrap().len(), 0);
-               let listener = &TestChainListener(0);
-               block_notifier.register_listener(listener as &ChainListener);
-               let vec = block_notifier.listeners.lock().unwrap();
-               assert_eq!(vec.len(), 1);
-               let item = vec.first().unwrap();
-               assert!(ptr::eq(&(**item), listener));
-       }
-
-       #[test]
-       fn unregister_single_listener_test() {
-               let block_notifier = BlockNotifier::new();
-               let listener1 = &TestChainListener(1);
-               let listener2 = &TestChainListener(2);
-               block_notifier.register_listener(listener1 as &ChainListener);
-               block_notifier.register_listener(listener2 as &ChainListener);
-               let vec = block_notifier.listeners.lock().unwrap();
-               assert_eq!(vec.len(), 2);
-               drop(vec);
-               block_notifier.unregister_listener(listener1);
-               let vec = block_notifier.listeners.lock().unwrap();
-               assert_eq!(vec.len(), 1);
-               let item = vec.first().unwrap();
-               assert!(ptr::eq(&(**item), listener2));
-       }
-
-       #[test]
-       fn unregister_multiple_of_the_same_listeners_test() {
-               let block_notifier = BlockNotifier::new();
-               let listener1 = &TestChainListener(1);
-               let listener2 = &TestChainListener(2);
-               block_notifier.register_listener(listener1 as &ChainListener);
-               block_notifier.register_listener(listener1 as &ChainListener);
-               block_notifier.register_listener(listener2 as &ChainListener);
-               let vec = block_notifier.listeners.lock().unwrap();
-               assert_eq!(vec.len(), 3);
-               drop(vec);
-               block_notifier.unregister_listener(listener1);
-               let vec = block_notifier.listeners.lock().unwrap();
-               assert_eq!(vec.len(), 1);
-               let item = vec.first().unwrap();
-               assert!(ptr::eq(&(**item), listener2));
-       }
-}
index 913bbd62d3b946f061cb7b5148539e5382e004ce..cfc83deb833c7e194c83f6a57c0936fb3a25d4e4 100644 (file)
@@ -719,10 +719,6 @@ impl<ChanSigner: ChannelKeys, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
        ///
        /// Users need to notify the new ChannelManager when a new block is connected or
        /// disconnected using its `block_connected` and `block_disconnected` methods.
-       /// However, rather than calling these methods directly, the user should register
-       /// the ChannelManager as a listener to the BlockNotifier and call the BlockNotifier's
-       /// `block_(dis)connected` methods, which will notify all registered listeners in one
-       /// go.
        pub fn new(network: Network, fee_est: F, chain_monitor: M, tx_broadcaster: T, logger: L, keys_manager: K, config: UserConfig, current_blockchain_height: usize) -> Self {
                let secp_ctx = Secp256k1::new();
 
index 962f8001f4359173cfd840999a9f4ac4a56c7d93..eadc5787832f0b404199d9fe0f230e1f9bc41b52 100644 (file)
@@ -12,7 +12,7 @@
 
 use chain;
 use chain::Watch;
-use chain::chaininterface;
+use chain::chaininterface::ChainListener;
 use chain::transaction::OutPoint;
 use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentPreimage, PaymentHash, PaymentSecret, PaymentSendFailure};
 use ln::channelmonitor::ChannelMonitor;
@@ -83,7 +83,6 @@ pub fn connect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, depth: u32, he
 
 pub fn connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: &Block, height: u32) {
        use chain::WatchEventProvider;
-       use chain::chaininterface::ChainListener;
 
        let watch_events = node.chain_monitor.chain_monitor.release_pending_watch_events();
        process_chain_watch_events(&watch_events);
@@ -106,7 +105,8 @@ pub fn connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: &Block,
 fn process_chain_watch_events(_events: &Vec<chain::WatchEvent>) {}
 
 pub fn disconnect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, header: &BlockHeader, height: u32) {
-       node.block_notifier.block_disconnected(header, height)
+       node.chain_monitor.chain_monitor.block_disconnected(header, height);
+       node.node.block_disconnected(header, height);
 }
 
 pub struct TestChanMonCfg {
@@ -127,7 +127,6 @@ pub struct NodeCfg<'a> {
 }
 
 pub struct Node<'a, 'b: 'a, 'c: 'b> {
-       pub block_notifier: chaininterface::BlockNotifierRef<'a>,
        pub chain_source: &'c test_utils::TestChainSource,
        pub tx_broadcaster: &'c test_utils::TestBroadcaster,
        pub chain_monitor: &'b test_utils::TestChainMonitor<'c>,
@@ -1149,11 +1148,8 @@ pub fn create_network<'a, 'b: 'a, 'c: 'b>(node_count: usize, cfgs: &'b Vec<NodeC
        let payment_count = Rc::new(RefCell::new(0));
 
        for i in 0..node_count {
-               let block_notifier = chaininterface::BlockNotifier::new();
-               block_notifier.register_listener(&cfgs[i].chain_monitor.chain_monitor as &chaininterface::ChainListener);
-               block_notifier.register_listener(&chan_mgrs[i] as &chaininterface::ChainListener);
                let net_graph_msg_handler = NetGraphMsgHandler::new(None, cfgs[i].logger);
-               nodes.push(Node{ chain_source: cfgs[i].chain_source, block_notifier,
+               nodes.push(Node{ chain_source: cfgs[i].chain_source,
                                 tx_broadcaster: cfgs[i].tx_broadcaster, chain_monitor: &cfgs[i].chain_monitor,
                                 keys_manager: &cfgs[i].keys_manager, node: &chan_mgrs[i], net_graph_msg_handler,
                                 node_seed: cfgs[i].node_seed, network_chan_count: chan_count.clone(),
index 08e8187ad9fe4ebf78bcaedb75da357440f2dcbf..1b664508b8aa49a7685385e442b7a7718c0c2478 100644 (file)
@@ -14,7 +14,7 @@
 use chain::Watch;
 use chain::transaction::OutPoint;
 use chain::keysinterface::{ChannelKeys, KeysInterface, SpendableOutputDescriptor};
-use chain::chaininterface::{ChainListener, BlockNotifier};
+use chain::chaininterface::ChainListener;
 use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC};
 use ln::channelmanager::{ChannelManager, ChannelManagerReadArgs, RAACommitmentOrder, PaymentPreimage, PaymentHash, PaymentSecret, PaymentSendFailure, BREAKDOWN_TIMEOUT};
 use ln::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
@@ -4352,7 +4352,6 @@ fn test_no_txn_manager_serialize_deserialize() {
 
        assert!(nodes[0].chain_monitor.watch_channel(chan_0_monitor.get_funding_txo().0, chan_0_monitor).is_ok());
        nodes[0].node = &nodes_0_deserialized;
-       nodes[0].block_notifier.register_listener(nodes[0].node);
        assert_eq!(nodes[0].node.list_channels().len(), 1);
        check_added_monitors!(nodes[0], 1);
 
@@ -4475,7 +4474,6 @@ fn test_manager_serialize_deserialize_events() {
        };
 
        // Make sure the channel is functioning as though the de/serialization never happened
-       nodes[0].block_notifier.register_listener(nodes[0].node);
        assert_eq!(nodes[0].node.list_channels().len(), 1);
        check_added_monitors!(nodes[0], 1);
 
@@ -7518,10 +7516,6 @@ fn test_data_loss_protect() {
        nodes[0].chain_monitor = &monitor;
        nodes[0].chain_source = &chain_source;
 
-       nodes[0].block_notifier = BlockNotifier::new();
-       nodes[0].block_notifier.register_listener(&nodes[0].chain_monitor.chain_monitor);
-       nodes[0].block_notifier.register_listener(nodes[0].node);
-
        check_added_monitors!(nodes[0], 1);
 
        nodes[0].node.peer_connected(&nodes[1].node.get_our_node_id(), &msgs::Init { features: InitFeatures::empty() });