Track block hash, return via get_relevant_txids

Previously, `Confirm::get_relevant_txids()` only returned a list of
transactions that have to be monitored for reorganization out of the
chain. This interface however required double bookkeeping: while we
internally keep track of the best block, height, etc, it would also
require the user to keep track which transaction was previously
confirmed in which block and to take actions based on any change, e.g,
to reconfirm them when the block would be reorged-out and the
transactions had been reconfirmed in another block.

Here, we track the confirmation block hash internally and return it via
`Confirm::get_relevant_txids()` to the user, which alleviates the
requirement for double bookkeeping: the user can now simply check
whether the given transaction is still confirmed and in the given block,
and take action if not.

Author: tnull

lightning/src/chain/chainmonitor.rs

@@ -24,7 +24,7 @@
 //! servicing [`ChannelMonitor`] updates from the client.
 
 use bitcoin::blockdata::block::BlockHeader;
-use bitcoin::hash_types::Txid;
+use bitcoin::hash_types::{Txid, BlockHash};
 
 use crate::chain;
 use crate::chain::{ChannelMonitorUpdateStatus, Filter, WatchedOutput};
@@ -544,7 +544,7 @@ where
 		});
 	}
 
-	fn get_relevant_txids(&self) -> Vec<Txid> {
+	fn get_relevant_txids(&self) -> Vec<(Txid, Option<BlockHash>)> {
 		let mut txids = Vec::new();
 		let monitor_states = self.monitors.read().unwrap();
 		for monitor_state in monitor_states.values() {

lightning/src/chain/channelmonitor.rs

@@ -171,7 +171,8 @@ pub trait Confirm {
 	/// if they become available at the same time.
 	fn best_block_updated(&self, header: &BlockHeader, height: u32);
 
-	/// Returns transactions that should be monitored for reorganization out of the chain.
+	/// Returns transactions that should be monitored for reorganization out of the chain along
+	/// with the hash of the block as part of which had been previously confirmed.
 	///
 	/// Will include any transactions passed to [`transactions_confirmed`] that have insufficient
 	/// confirmations to be safe from a chain reorganization. Will not include any transactions
@@ -184,7 +185,7 @@ pub trait Confirm {
 	///
 	/// [`transactions_confirmed`]: Self::transactions_confirmed
 	/// [`transaction_unconfirmed`]: Self::transaction_unconfirmed
-	fn get_relevant_txids(&self) -> Vec<Txid>;
+	fn get_relevant_txids(&self) -> Vec<(Txid, Option<BlockHash>)>;
 }
 
 /// An enum representing the status of a channel monitor update persistence.

lightning/src/chain/mod.rs

@@ -16,7 +16,7 @@ use bitcoin::blockdata::transaction::Transaction;
 use bitcoin::blockdata::transaction::OutPoint as BitcoinOutPoint;
 use bitcoin::blockdata::script::Script;
 
-use bitcoin::hash_types::Txid;
+use bitcoin::hash_types::{Txid, BlockHash};
 
 use bitcoin::secp256k1::{Secp256k1, ecdsa::Signature};
 use bitcoin::secp256k1;
@@ -58,6 +58,7 @@ const MAX_ALLOC_SIZE: usize = 64*1024;
 struct OnchainEventEntry {
 	txid: Txid,
 	height: u32,
+	block_hash: Option<BlockHash>,
 	event: OnchainEvent,
 }
 
@@ -92,6 +93,7 @@ impl Writeable for OnchainEventEntry {
 	fn write<W: Writer>(&self, writer: &mut W) -> Result<(), io::Error> {
 		write_tlv_fields!(writer, {
 			(0, self.txid, required),
+			(1, self.block_hash, option),
 			(2, self.height, required),
 			(4, self.event, required),
 		});
@@ -103,14 +105,16 @@ impl MaybeReadable for OnchainEventEntry {
 	fn read<R: io::Read>(reader: &mut R) -> Result<Option<Self>, DecodeError> {
 		let mut txid = Txid::all_zeros();
 		let mut height = 0;
+		let mut block_hash = None;
 		let mut event = None;
 		read_tlv_fields!(reader, {
 			(0, txid, required),
+			(1, block_hash, option),
 			(2, height, required),
 			(4, event, ignorable),
 		});
 		if let Some(ev) = event {
-			Ok(Some(Self { txid, height, event: ev }))
+			Ok(Some(Self { txid, height, block_hash, event: ev }))
 		} else {
 			Ok(None)
 		}
@@ -543,17 +547,21 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 
 	/// Upon channelmonitor.block_connected(..) or upon provision of a preimage on the forward link
 	/// for this channel, provide new relevant on-chain transactions and/or new claim requests.
-	/// Formerly this was named `block_connected`, but it is now also used for claiming an HTLC output
-	/// if we receive a preimage after force-close.
-	/// `conf_height` represents the height at which the transactions in `txn_matched` were
-	/// confirmed. This does not need to equal the current blockchain tip height, which should be
-	/// provided via `cur_height`, however it must never be higher than `cur_height`.
-	pub(crate) fn update_claims_view<B: Deref, F: Deref, L: Deref>(&mut self, txn_matched: &[&Transaction], requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
+	/// Together with `update_claims_view_from_matched_txn` this used to be named
+	/// `block_connected`, but it is now also used for claiming an HTLC output if we receive a
+	/// preimage after force-close.
+	///
+	/// `conf_height` represents the minimal height at which the request could get confirmed. This
+	/// does not need to equal the current blockchain tip height, which should be provided via
+	/// `cur_height`, however it must never be higher than `cur_height`.
+	pub(crate) fn update_claims_view_from_requests<B: Deref, F: Deref, L: Deref>(&mut self,
+		requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B,
+		fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
 		where B::Target: BroadcasterInterface,
-		      F::Target: FeeEstimator,
-					L::Target: Logger,
+			  F::Target: FeeEstimator,
+			  L::Target: Logger,
 	{
-		log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {} and {} claim requests", cur_height, txn_matched.len(), conf_height, requests.len());
+		log_debug!(logger, "Updating claims view at height {} with {} claim requests", cur_height, requests.len());
 		let mut preprocessed_requests = Vec::with_capacity(requests.len());
 		let mut aggregated_request = None;
 
@@ -634,6 +642,25 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 			}
 		}
 
+		self.process_onchain_events_awaiting_threshold_conf(cur_height, logger);
+	}
+
+	/// Upon channelmonitor.block_connected(..) or upon provision of a preimage on the forward link
+	/// for this channel, provide new relevant on-chain transactions and/or new claim requests.
+	/// Together with `update_claims_view_from_requests` this used to be named `block_connected`,
+	/// but it is now also used for claiming an HTLC output if we receive a preimage after force-close.
+	///
+	/// `conf_height` represents the height at which the transactions in `txn_matched` were
+	/// confirmed. This does not need to equal the current blockchain tip height, which should be
+	/// provided via `cur_height`, however it must never be higher than `cur_height`.
+	pub(crate) fn update_claims_view_from_matched_txn<B: Deref, F: Deref, L: Deref>(&mut self,
+		txn_matched: &[&Transaction], conf_height: u32, conf_hash: BlockHash, cur_height: u32,
+		broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)
+		where B::Target: BroadcasterInterface,
+			  F::Target: FeeEstimator,
+			  L::Target: Logger,
+	{
+		log_debug!(logger, "Updating claims view at height {} with {} matched transactions in block {}", cur_height, txn_matched.len(), conf_height);
 		let mut bump_candidates = HashMap::new();
 		for tx in txn_matched {
 			// Scan all input to verify is one of the outpoint spent is of interest for us
@@ -661,6 +688,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 								let entry = OnchainEventEntry {
 									txid: tx.txid(),
 									height: conf_height,
+									block_hash: Some(conf_hash),
 									event: OnchainEvent::Claim { claim_request: first_claim_txid_height.0.clone() }
 								};
 								if !self.onchain_events_awaiting_threshold_conf.contains(&entry) {
@@ -701,6 +729,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 				let entry = OnchainEventEntry {
 					txid: tx.txid(),
 					height: conf_height,
+					block_hash: Some(conf_hash),
 					event: OnchainEvent::ContentiousOutpoint { package },
 				};
 				if !self.onchain_events_awaiting_threshold_conf.contains(&entry) {
@@ -709,34 +738,7 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 			}
 		}
 
-		// After security delay, either our claim tx got enough confs or outpoint is definetely out of reach
-		let onchain_events_awaiting_threshold_conf =
-			self.onchain_events_awaiting_threshold_conf.drain(..).collect::<Vec<_>>();
-		for entry in onchain_events_awaiting_threshold_conf {
-			if entry.has_reached_confirmation_threshold(cur_height) {
-				match entry.event {
-					OnchainEvent::Claim { claim_request } => {
-						// We may remove a whole set of claim outpoints here, as these one may have
-						// been aggregated in a single tx and claimed so atomically
-						if let Some(request) = self.pending_claim_requests.remove(&claim_request) {
-							for outpoint in request.outpoints() {
-								log_debug!(logger, "Removing claim tracking for {} due to maturation of claim tx {}.", outpoint, claim_request);
-								self.claimable_outpoints.remove(&outpoint);
-								#[cfg(anchors)]
-								self.pending_claim_events.remove(&claim_request);
-							}
-						}
-					},
-					OnchainEvent::ContentiousOutpoint { package } => {
-						log_debug!(logger, "Removing claim tracking due to maturation of claim tx for outpoints:");
-						log_debug!(logger, " {:?}", package.outpoints());
-						self.claimable_outpoints.remove(&package.outpoints()[0]);
-					}
-				}
-			} else {
-				self.onchain_events_awaiting_threshold_conf.push(entry);
-			}
-		}
+		self.process_onchain_events_awaiting_threshold_conf(cur_height, logger);
 
 		// Check if any pending claim request must be rescheduled
 		for (first_claim_txid, ref request) in self.pending_claim_requests.iter() {
@@ -770,6 +772,39 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 		}
 	}
 
+	fn process_onchain_events_awaiting_threshold_conf<L: Deref>(&mut self, cur_height: u32, logger: &L)
+		where L::Target: Logger,
+	{
+		// After security delay, either our claim tx got enough confs or outpoint is definetely out of reach
+		let onchain_events_awaiting_threshold_conf =
+			self.onchain_events_awaiting_threshold_conf.drain(..).collect::<Vec<_>>();
+		for entry in onchain_events_awaiting_threshold_conf {
+			if entry.has_reached_confirmation_threshold(cur_height) {
+				match entry.event {
+					OnchainEvent::Claim { claim_request } => {
+						// We may remove a whole set of claim outpoints here, as these one may have
+						// been aggregated in a single tx and claimed so atomically
+						if let Some(request) = self.pending_claim_requests.remove(&claim_request) {
+							for outpoint in request.outpoints() {
+								log_debug!(logger, "Removing claim tracking for {} due to maturation of claim tx {}.", outpoint, claim_request);
+								self.claimable_outpoints.remove(&outpoint);
+								#[cfg(anchors)]
+								self.pending_claim_events.remove(&claim_request);
+							}
+						}
+					},
+					OnchainEvent::ContentiousOutpoint { package } => {
+						log_debug!(logger, "Removing claim tracking due to maturation of claim tx for outpoints:");
+						log_debug!(logger, " {:?}", package.outpoints());
+						self.claimable_outpoints.remove(&package.outpoints()[0]);
+					}
+				}
+			} else {
+				self.onchain_events_awaiting_threshold_conf.push(entry);
+			}
+		}
+	}
+
 	pub(crate) fn transaction_unconfirmed<B: Deref, F: Deref, L: Deref>(
 		&mut self,
 		txid: &Txid,
@@ -860,12 +895,12 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 		self.claimable_outpoints.get(outpoint).is_some()
 	}
 
-	pub(crate) fn get_relevant_txids(&self) -> Vec<Txid> {
-		let mut txids: Vec<Txid> = self.onchain_events_awaiting_threshold_conf
+	pub(crate) fn get_relevant_txids(&self) -> Vec<(Txid, Option<BlockHash>)> {
+		let mut txids: Vec<(Txid, Option<BlockHash>)> = self.onchain_events_awaiting_threshold_conf
 			.iter()
-			.map(|entry| entry.txid)
+			.map(|entry| (entry.txid, entry.block_hash))
 			.collect();
-		txids.sort_unstable();
+		txids.sort_unstable_by_key(|(txid, _)| *txid);
 		txids.dedup();
 		txids
 	}

lightning/src/chain/onchaintx.rs

@@ -4520,6 +4520,11 @@ impl<Signer: Sign> Channel<Signer> {
 		self.channel_transaction_parameters.funding_outpoint
 	}
 
+	/// Returns the block hash in which our funding transaction was confirmed.
+	pub fn get_funding_tx_confirmed_in(&self) -> Option<BlockHash> {
+		self.funding_tx_confirmed_in
+	}
+
 	fn get_holder_selected_contest_delay(&self) -> u16 {
 		self.channel_transaction_parameters.holder_selected_contest_delay
 	}

lightning/src/ln/channel.rs

@@ -5853,12 +5853,12 @@ where
 		});
 	}
 
-	fn get_relevant_txids(&self) -> Vec<Txid> {
+	fn get_relevant_txids(&self) -> Vec<(Txid, Option<BlockHash>)> {
 		let channel_state = self.channel_state.lock().unwrap();
 		let mut res = Vec::with_capacity(channel_state.short_to_chan_info.len());
 		for chan in channel_state.by_id.values() {
-			if let Some(funding_txo) = chan.get_funding_txo() {
-				res.push(funding_txo.txid);
+			if let (Some(funding_txo), block_hash) = (chan.get_funding_txo(), chan.get_funding_tx_confirmed_in()) {
+				res.push((funding_txo.txid, block_hash));
 			}
 		}
 		res

lightning/src/ln/channelmanager.rs

@@ -271,6 +271,7 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
 	let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs);
 	*nodes[0].connect_style.borrow_mut() = connect_style;
 
+	let chan_conf_height = core::cmp::max(nodes[0].best_block_info().1 + 1, nodes[1].best_block_info().1 + 1);
 	let chan = create_announced_chan_between_nodes(&nodes, 0, 1, channelmanager::provided_init_features(), channelmanager::provided_init_features());
 
 	let channel_state = nodes[0].node.channel_state.lock().unwrap();
@@ -281,15 +282,25 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
 	if !reorg_after_reload {
 		if use_funding_unconfirmed {
 			let relevant_txids = nodes[0].node.get_relevant_txids();
-			assert_eq!(&relevant_txids[..], &[chan.3.txid()]);
-			nodes[0].node.transaction_unconfirmed(&relevant_txids[0]);
+			assert_eq!(relevant_txids.len(), 1);
+			let block_hash_opt = relevant_txids[0].1;
+			assert!(block_hash_opt.is_some());
+			let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
+			assert_eq!(block_hash_opt.unwrap(), expected_hash);
+			let txid = relevant_txids[0].0;
+			assert_eq!(txid, chan.3.txid());
+			nodes[0].node.transaction_unconfirmed(&txid);
 		} else if connect_style == ConnectStyle::FullBlockViaListen {
 			disconnect_blocks(&nodes[0], CHAN_CONFIRM_DEPTH - 1);
 			assert_eq!(nodes[0].node.list_usable_channels().len(), 1);
 			disconnect_blocks(&nodes[0], 1);
 		} else {
 			disconnect_all_blocks(&nodes[0]);
 		}
+
+		let relevant_txids = nodes[0].node.get_relevant_txids();
+		assert_eq!(relevant_txids.len(), 0);
+
 		handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.");
 		check_added_monitors!(nodes[1], 1);
 		{
@@ -350,15 +361,25 @@ fn do_test_unconf_chan(reload_node: bool, reorg_after_reload: bool, use_funding_
 	if reorg_after_reload {
 		if use_funding_unconfirmed {
 			let relevant_txids = nodes[0].node.get_relevant_txids();
-			assert_eq!(&relevant_txids[..], &[chan.3.txid()]);
-			nodes[0].node.transaction_unconfirmed(&relevant_txids[0]);
+			assert_eq!(relevant_txids.len(), 1);
+			let block_hash_opt = relevant_txids[0].1;
+			assert!(block_hash_opt.is_some());
+			let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
+			assert_eq!(block_hash_opt.unwrap(), expected_hash);
+			let txid = relevant_txids[0].0;
+			assert_eq!(txid, chan.3.txid());
+			nodes[0].node.transaction_unconfirmed(&txid);
 		} else if connect_style == ConnectStyle::FullBlockViaListen {
 			disconnect_blocks(&nodes[0], CHAN_CONFIRM_DEPTH - 1);
 			assert_eq!(nodes[0].node.list_channels().len(), 1);
 			disconnect_blocks(&nodes[0], 1);
 		} else {
 			disconnect_all_blocks(&nodes[0]);
 		}
+
+		let relevant_txids = nodes[0].node.get_relevant_txids();
+		assert_eq!(relevant_txids.len(), 0);
+
 		handle_announce_close_broadcast_events(&nodes, 0, 1, true, "Channel closed because of an exception: Funding transaction was un-confirmed. Locked at 6 confs, now have 0 confs.");
 		check_added_monitors!(nodes[1], 1);
 		{