Remove ChainWatchInterface from BlockNotifier

ChainListeners should be independent of each other, but in practice this
is not the case because ChainWatchInterface introduces a dependency
between them. Push ChainWatchInterface down into the ChainListener
implementations where needed. Update ChainListener's block_connected
method to take the entire block.

Author: jkczyz

fuzz/src/chanmon_consistency.rs

@@ -10,7 +10,7 @@
 //! channel being force-closed.
 
 use bitcoin::BitcoinHash;
-use bitcoin::blockdata::block::BlockHeader;
+use bitcoin::blockdata::block::{Block, BlockHeader};
 use bitcoin::blockdata::transaction::{Transaction, TxOut};
 use bitcoin::blockdata::script::{Builder, Script};
 use bitcoin::blockdata::opcodes;
@@ -306,17 +306,17 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
 
 	macro_rules! confirm_txn {
 		($node: expr) => { {
-			let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-			let mut txn = Vec::with_capacity(channel_txn.len());
-			let mut posn = Vec::with_capacity(channel_txn.len());
-			for i in 0..channel_txn.len() {
-				txn.push(&channel_txn[i]);
-				posn.push(i + 1);
-			}
-			$node.block_connected(&header, 1, &txn, &posn);
+			let mut block = Block {
+				header: BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
+				txdata: channel_txn.clone(),
+			};
+			$node.block_connected(&block, 1);
 			for i in 2..100 {
-				header = BlockHeader { version: 0x20000000, prev_blockhash: header.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-				$node.block_connected(&header, i, &Vec::new(), &[0; 0]);
+				block = Block {
+					header: BlockHeader { version: 0x20000000, prev_blockhash: block.bitcoin_hash(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 },
+					txdata: vec![],
+				};
+				$node.block_connected(&block, i);
 			}
 		} }
 	}

fuzz/src/full_stack.rs

@@ -4,7 +4,7 @@
 //! or payments to send/ways to handle events generated.
 //! This test has been very useful, though due to its complexity good starting inputs are critical.
 
-use bitcoin::blockdata::block::BlockHeader;
+use bitcoin::blockdata::block::{Block, BlockHeader};
 use bitcoin::blockdata::transaction::{Transaction, TxOut};
 use bitcoin::blockdata::script::{Builder, Script};
 use bitcoin::blockdata::opcodes;
@@ -175,30 +175,29 @@ impl<'a> MoneyLossDetector<'a> {
 	}
 
 	fn connect_block(&mut self, all_txn: &[Transaction]) {
-		let mut txn = Vec::with_capacity(all_txn.len());
-		let mut txn_idxs = Vec::with_capacity(all_txn.len());
-		for (idx, tx) in all_txn.iter().enumerate() {
+		for tx in all_txn.iter() {
 			let txid = tx.txid();
 			match self.txids_confirmed.entry(txid) {
 				hash_map::Entry::Vacant(e) => {
 					e.insert(self.height);
-					txn.push(tx);
-					txn_idxs.push(idx + 1);
 				},
 				_ => {},
 			}
 		}
 
-		let header = BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height], merkle_root: Default::default(), time: self.blocks_connected, bits: 42, nonce: 42 };
+		let block = Block {
+			header: BlockHeader { version: 0x20000000, prev_blockhash: self.header_hashes[self.height], merkle_root: Default::default(), time: self.blocks_connected, bits: 42, nonce: 42 },
+			txdata: all_txn.to_vec(),
+		};
 		self.height += 1;
 		self.blocks_connected += 1;
-		self.manager.block_connected(&header, self.height as u32, &txn[..], &txn_idxs[..]);
-		(*self.monitor).block_connected(&header, self.height as u32, &txn[..], &txn_idxs[..]);
+		self.manager.block_connected(&block, self.height as u32);
+		(*self.monitor).block_connected(&block, self.height as u32);
 		if self.header_hashes.len() > self.height {
-			self.header_hashes[self.height] = header.bitcoin_hash();
+			self.header_hashes[self.height] = block.bitcoin_hash();
 		} else {
 			assert_eq!(self.header_hashes.len(), self.height);
-			self.header_hashes.push(header.bitcoin_hash());
+			self.header_hashes.push(block.bitcoin_hash());
 		}
 		self.max_height = cmp::max(self.height, self.max_height);
 	}

lightning/src/chain/chaininterface.rs

@@ -72,21 +72,7 @@ pub trait BroadcasterInterface: Sync + Send {
 /// A trait indicating a desire to listen for events from the chain
 pub trait ChainListener: Sync + Send {
 	/// Notifies a listener that a block was connected.
-	///
-	/// The txn_matched array should be set to references to transactions which matched the
-	/// relevant installed watch outpoints/txn, or the full set of transactions in the block.
-	///
-	/// Note that if txn_matched includes only matched transactions, and a new
-	/// transaction/outpoint is watched during a block_connected call, the block *must* be
-	/// re-scanned with the new transaction/outpoints and block_connected should be called
-	/// again with the same header and (at least) the new transactions.
-	///
-	/// Note that if non-new transaction/outpoints are be registered during a call, a second call
-	/// *must not* happen.
-	///
-	/// This also means those counting confirmations using block_connected callbacks should watch
-	/// for duplicate headers and not count them towards confirmations!
-	fn block_connected(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[usize]);
+	fn block_connected(&self, block: &Block, height: u32);
 	/// Notifies a listener that a block was disconnected.
 	/// Unlike block_connected, this *must* never be called twice for the same disconnect event.
 	/// Height must be the one of the block which was disconnected (not new height of the best chain)
@@ -218,7 +204,7 @@ impl ChainWatchedUtil {
 /// 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<C> = Arc<BlockNotifier<'static, Arc<ChainListener>, C>>;
+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
@@ -227,27 +213,24 @@ pub type BlockNotifierArc<C> = Arc<BlockNotifier<'static, Arc<ChainListener>, C>
 /// 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, C> = BlockNotifier<'a, &'a ChainListener, C>;
+pub type BlockNotifierRef<'a> = BlockNotifier<'a, &'a ChainListener>;
 
-/// Utility for notifying listeners about new blocks, and handling block rescans if new watch
-/// data is registered.
+/// 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, C: Deref> where C::Target: ChainWatchInterface {
+pub struct BlockNotifier<'a, CL: Deref<Target = ChainListener + 'a> + 'a> {
 	listeners: Mutex<Vec<CL>>,
-	chain_monitor: C,
 	phantom: PhantomData<&'a ()>,
 }
 
-impl<'a, CL: Deref<Target = ChainListener + 'a> + 'a, C: Deref> BlockNotifier<'a, CL, C> where C::Target: ChainWatchInterface {
+impl<'a, CL: Deref<Target = ChainListener + 'a> + 'a> BlockNotifier<'a, CL> {
 	/// Constructs a new BlockNotifier without any listeners.
-	pub fn new(chain_monitor: C) -> BlockNotifier<'a, CL, C> {
+	pub fn new() -> BlockNotifier<'a, CL> {
 		BlockNotifier {
 			listeners: Mutex::new(Vec::new()),
-			chain_monitor,
 			phantom: PhantomData,
 		}
 	}
@@ -270,36 +253,12 @@ impl<'a, CL: Deref<Target = ChainListener + 'a> + 'a, C: Deref> BlockNotifier<'a
 		vec.retain(|item | !ptr::eq(&(**item), &(*listener)));
 	}
 
-	/// Notify listeners that a block was connected given a full, unfiltered block.
-	///
-	/// Handles re-scanning the block and calling block_connected again if listeners register new
-	/// watch data during the callbacks for you (see ChainListener::block_connected for more info).
-	pub fn block_connected(&self, block: &Block, height: u32) {
-		let mut reentered = true;
-		while reentered {
-			let matched_indexes = self.chain_monitor.filter_block(block);
-			let mut matched_txn = Vec::new();
-			for index in matched_indexes.iter() {
-				matched_txn.push(&block.txdata[*index]);
-			}
-			reentered = self.block_connected_checked(&block.header, height, matched_txn.as_slice(), matched_indexes.as_slice());
-		}
-	}
-
-	/// Notify listeners that a block was connected, given pre-filtered list of transactions in the
-	/// block which matched the filter (probably using does_match_tx).
-	///
-	/// Returns true if notified listeners registered additional watch data (implying that the
-	/// block must be re-scanned and this function called again prior to further block_connected
-	/// calls, see ChainListener::block_connected for more info).
-	pub fn block_connected_checked(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[usize]) -> bool {
-		let last_seen = self.chain_monitor.reentered();
-
+	/// Notify listeners that a block was connected.
+	pub fn block_connected<'b>(&self, block: &'b Block, height: u32) {
 		let listeners = self.listeners.lock().unwrap();
 		for listener in listeners.iter() {
-			listener.block_connected(header, height, txn_matched, indexes_of_txn_matched);
+			listener.block_connected(block, height);
 		}
-		return last_seen != self.chain_monitor.reentered();
 	}
 
 	/// Notify listeners that a block was disconnected.
@@ -410,7 +369,7 @@ mod tests {
 	fn register_listener_test() {
 		let chanmon_cfgs = create_chanmon_cfgs(1);
 		let node_cfgs = create_node_cfgs(1, &chanmon_cfgs);
-		let block_notifier = BlockNotifier::new(node_cfgs[0].chain_monitor);
+		let block_notifier = BlockNotifier::new();
 		assert_eq!(block_notifier.listeners.lock().unwrap().len(), 0);
 		let listener = &node_cfgs[0].chan_monitor.simple_monitor as &ChainListener;
 		block_notifier.register_listener(listener);
@@ -424,7 +383,7 @@ mod tests {
 	fn unregister_single_listener_test() {
 		let chanmon_cfgs = create_chanmon_cfgs(2);
 		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
-		let block_notifier = BlockNotifier::new(node_cfgs[0].chain_monitor);
+		let block_notifier = BlockNotifier::new();
 		let listener1 = &node_cfgs[0].chan_monitor.simple_monitor as &ChainListener;
 		let listener2 = &node_cfgs[1].chan_monitor.simple_monitor as &ChainListener;
 		block_notifier.register_listener(listener1);
@@ -443,7 +402,7 @@ mod tests {
 	fn unregister_single_listener_ref_test() {
 		let chanmon_cfgs = create_chanmon_cfgs(2);
 		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
-		let block_notifier = BlockNotifier::new(node_cfgs[0].chain_monitor);
+		let block_notifier = BlockNotifier::new();
 		block_notifier.register_listener(&node_cfgs[0].chan_monitor.simple_monitor as &ChainListener);
 		block_notifier.register_listener(&node_cfgs[1].chan_monitor.simple_monitor as &ChainListener);
 		let vec = block_notifier.listeners.lock().unwrap();
@@ -460,7 +419,7 @@ mod tests {
 	fn unregister_multiple_of_the_same_listeners_test() {
 		let chanmon_cfgs = create_chanmon_cfgs(2);
 		let node_cfgs = create_node_cfgs(2, &chanmon_cfgs);
-		let block_notifier = BlockNotifier::new(node_cfgs[0].chain_monitor);
+		let block_notifier = BlockNotifier::new();
 		let listener1 = &node_cfgs[0].chan_monitor.simple_monitor as &ChainListener;
 		let listener2 = &node_cfgs[1].chan_monitor.simple_monitor as &ChainListener;
 		block_notifier.register_listener(listener1);

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -1747,11 +1747,11 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
 	};
 
 	if confirm_a_first {
-		confirm_transaction(&nodes[0].block_notifier, &nodes[0].chain_monitor, &funding_tx, funding_tx.version);
+		confirm_transaction(&nodes[0].block_notifier, &funding_tx);
 		nodes[1].node.handle_funding_locked(&nodes[0].node.get_our_node_id(), &get_event_msg!(nodes[0], MessageSendEvent::SendFundingLocked, nodes[1].node.get_our_node_id()));
 	} else {
 		assert!(!restore_b_before_conf);
-		confirm_transaction(&nodes[1].block_notifier, &nodes[1].chain_monitor, &funding_tx, funding_tx.version);
+		confirm_transaction(&nodes[1].block_notifier, &funding_tx);
 		assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 	}
 
@@ -1763,7 +1763,7 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
 	assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 
 	if !restore_b_before_conf {
-		confirm_transaction(&nodes[1].block_notifier, &nodes[1].chain_monitor, &funding_tx, funding_tx.version);
+		confirm_transaction(&nodes[1].block_notifier, &funding_tx);
 		assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());
 		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 	}
@@ -1776,12 +1776,12 @@ fn do_during_funding_monitor_fail(confirm_a_first: bool, restore_b_before_conf:
 	let (channel_id, (announcement, as_update, bs_update)) = if !confirm_a_first {
 		nodes[0].node.handle_funding_locked(&nodes[1].node.get_our_node_id(), &get_event_msg!(nodes[1], MessageSendEvent::SendFundingLocked, nodes[0].node.get_our_node_id()));
 
-		confirm_transaction(&nodes[0].block_notifier, &nodes[0].chain_monitor, &funding_tx, funding_tx.version);
+		confirm_transaction(&nodes[0].block_notifier, &funding_tx);
 		let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[1], &nodes[0]);
 		(channel_id, create_chan_between_nodes_with_value_b(&nodes[0], &nodes[1], &funding_locked))
 	} else {
 		if restore_b_before_conf {
-			confirm_transaction(&nodes[1].block_notifier, &nodes[1].chain_monitor, &funding_tx, funding_tx.version);
+			confirm_transaction(&nodes[1].block_notifier, &funding_tx);
 		}
 		let (funding_locked, channel_id) = create_chan_between_nodes_with_value_confirm_second(&nodes[0], &nodes[1]);
 		(channel_id, create_chan_between_nodes_with_value_b(&nodes[1], &nodes[0], &funding_locked))

lightning/src/ln/channel.rs

@@ -1,4 +1,4 @@
-use bitcoin::blockdata::block::BlockHeader;
+use bitcoin::blockdata::block::{Block, BlockHeader};
 use bitcoin::blockdata::script::{Script,Builder};
 use bitcoin::blockdata::transaction::{TxIn, TxOut, Transaction, SigHashType};
 use bitcoin::blockdata::opcodes;
@@ -3299,7 +3299,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 	///
 	/// May return some HTLCs (and their payment_hash) which have timed out and should be failed
 	/// back.
-	pub fn block_connected(&mut self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[usize]) -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage> {
+	pub fn block_connected(&mut self, block: &Block, height: u32) -> Result<(Option<msgs::FundingLocked>, Vec<(HTLCSource, PaymentHash)>), msgs::ErrorMessage> {
 		let mut timed_out_htlcs = Vec::new();
 		self.holding_cell_htlc_updates.retain(|htlc_update| {
 			match htlc_update {
@@ -3313,13 +3313,13 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 			}
 		});
 		let non_shutdown_state = self.channel_state & (!MULTI_STATE_FLAGS);
-		if header.bitcoin_hash() != self.last_block_connected {
+		if block.bitcoin_hash() != self.last_block_connected {
 			if self.funding_tx_confirmations > 0 {
 				self.funding_tx_confirmations += 1;
 			}
 		}
 		if non_shutdown_state & !(ChannelState::TheirFundingLocked as u32) == ChannelState::FundingSent as u32 {
-			for (ref tx, index_in_block) in txn_matched.iter().zip(indexes_of_txn_matched) {
+			for (index_in_block, ref tx) in block.txdata.iter().enumerate() {
 				if tx.txid() == self.funding_txo.unwrap().txid {
 					let txo_idx = self.funding_txo.unwrap().index as usize;
 					if txo_idx >= tx.output.len() || tx.output[txo_idx].script_pubkey != self.get_funding_redeemscript().to_v0_p2wsh() ||
@@ -3350,21 +3350,21 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 								}
 							}
 						}
-						if height > 0xff_ff_ff || (*index_in_block) > 0xff_ff_ff {
+						if height > 0xff_ff_ff || (index_in_block) > 0xff_ff_ff {
 							panic!("Block was bogus - either height 16 million or had > 16 million transactions");
 						}
 						assert!(txo_idx <= 0xffff); // txo_idx is a (u16 as usize), so this is just listed here for completeness
 						self.funding_tx_confirmations = 1;
-						self.short_channel_id = Some(((height as u64)          << (5*8)) |
-						                             ((*index_in_block as u64) << (2*8)) |
-						                             ((txo_idx as u64)         << (0*8)));
+						self.short_channel_id = Some(((height as u64)         << (5*8)) |
+						                             ((index_in_block as u64) << (2*8)) |
+						                             ((txo_idx as u64)        << (0*8)));
 					}
 				}
 			}
 		}
-		if header.bitcoin_hash() != self.last_block_connected {
-			self.last_block_connected = header.bitcoin_hash();
-			self.update_time_counter = cmp::max(self.update_time_counter, header.time);
+		if block.bitcoin_hash() != self.last_block_connected {
+			self.last_block_connected = block.bitcoin_hash();
+			self.update_time_counter = cmp::max(self.update_time_counter, block.header.time);
 			if let Some(channel_monitor) = self.channel_monitor.as_mut() {
 				channel_monitor.last_block_hash = self.last_block_connected;
 			}
@@ -3388,7 +3388,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
 						// funding_tx_confirmed_in and return.
 						false
 					};
-					self.funding_tx_confirmed_in = Some(header.bitcoin_hash());
+					self.funding_tx_confirmed_in = Some(block.bitcoin_hash());
 
 					//TODO: Note that this must be a duplicate of the previous commitment point they sent us,
 					//as otherwise we will have a commitment transaction that they can't revoke (well, kinda,