Remove BlockNotifier

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.

Author: jkczyz

lightning/src/chain/chaininterface.rs

@@ -4,16 +4,12 @@
 //! 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 {
@@ -152,137 +148,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.
-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<Target = ChainListener + 'a> + 'a> {
-	listeners: Mutex<Vec<CL>>,
-	phantom: PhantomData<&'a ()>,
-}
-
-impl<'a, CL: Deref<Target = ChainListener + 'a> + 'a> BlockNotifier<'a, CL> {
-	/// 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.
-	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));
-	}
-}

lightning/src/ln/channelmanager.rs

@@ -705,10 +705,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();
 

lightning/src/ln/functional_test_utils.rs

@@ -3,7 +3,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;
@@ -75,7 +75,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);
@@ -98,7 +97,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 {
@@ -119,7 +119,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>,
@@ -1141,11 +1140,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(),

lightning/src/ln/functional_tests.rs

@@ -5,7 +5,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,HTLCForwardInfo,RAACommitmentOrder, PaymentPreimage, PaymentHash, PaymentSecret, PaymentSendFailure, BREAKDOWN_TIMEOUT};
 use ln::channelmonitor::{ChannelMonitor, CLTV_CLAIM_BUFFER, LATENCY_GRACE_PERIOD_BLOCKS, ANTI_REORG_DELAY};
@@ -4126,7 +4126,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);
 
@@ -4249,7 +4248,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);
 
@@ -7377,10 +7375,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() });