Merge pull request #1796 from tnull/2022-10-track-confirmation-block-hash

Track confirmation block hash and return via `Confirm::get_relevant_txids`

Author: TheBlueMatt

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};
@@ -561,7 +561,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
@@ -180,11 +181,13 @@ pub trait Confirm {
 	/// May be called to determine the subset of transactions that must still be monitored for
 	/// reorganization. Will be idempotent between calls but may change as a result of calls to the
 	/// other interface methods. Thus, this is useful to determine which transactions may need to be
-	/// given to [`transaction_unconfirmed`].
+	/// given to [`transaction_unconfirmed`]. If any of the returned transactions are confirmed in
+	/// a block other than the one with the given hash, they need to be unconfirmed and reconfirmed
+	/// via [`transaction_unconfirmed`] and [`transactions_confirmed`], respectively.
 	///
 	/// [`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>, // Added as optional, will be filled in for any entry generated on 0.0.113 or after
 	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,22 @@ 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)
-		where B::Target: BroadcasterInterface,
-		      F::Target: FeeEstimator,
-					L::Target: Logger,
+	/// 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 height at which the request was generated. 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,
 	{
-		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;
 
@@ -633,7 +642,25 @@ impl<ChannelSigner: Sign> OnchainTxHandler<ChannelSigner> {
 				self.pending_claim_requests.insert(txid, req);
 			}
 		}
+	}
 
+	/// 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) {
@@ -860,12 +889,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

@@ -5906,12 +5906,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.by_id.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,24 @@ 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;
+			let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
+			assert_eq!(block_hash_opt, Some(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 +360,24 @@ 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;
+			let expected_hash = nodes[0].get_block_header(chan_conf_height).block_hash();
+			assert_eq!(block_hash_opt, Some(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);
 		{