Dedup confirmed_txs Vec

Previously, we would just push to the `confirmed_txs` `Vec`, leading to
redundant `Confirm::transactions_confirmed` calls, especially now that
we re-confirm previously disconnected spends.

Here, we ensure that we don't push additional `ConfirmedTx` entries if
already one with matching `Txid` is present. This not only gets rid of
the spurious `transactions_confirmed` calls (which are harmless), but
more importantly saves us from issuing unnecessary network calls, which
improves latency.

Author: tnull

lightning-transaction-sync/src/common.rs

@@ -128,6 +128,7 @@ impl FilterQueue {
 #[derive(Debug)]
 pub(crate) struct ConfirmedTx {
 	pub tx: Transaction,
+	pub txid: Txid,
 	pub block_header: Header,
 	pub block_height: u32,
 	pub pos: usize,

lightning-transaction-sync/src/electrum.rs

@@ -257,7 +257,7 @@ where
 
 		// First, check the confirmation status of registered transactions as well as the
 		// status of dependent transactions of registered outputs.
-		let mut confirmed_txs = Vec::new();
+		let mut confirmed_txs: Vec<ConfirmedTx> = Vec::new();
 		let mut watched_script_pubkeys = Vec::with_capacity(
 			sync_state.watched_transactions.len() + sync_state.watched_outputs.len());
 		let mut watched_txs = Vec::with_capacity(sync_state.watched_transactions.len());
@@ -305,6 +305,9 @@ where
 
 				for (i, script_history) in tx_results.iter().enumerate() {
 					let (txid, tx) = &watched_txs[i];
+					if confirmed_txs.iter().any(|ctx| ctx.txid == **txid) {
+						continue;
+					}
 					let mut filtered_history = script_history.iter().filter(|h| h.tx_hash == **txid);
 					if let Some(history) = filtered_history.next()
 					{
@@ -324,6 +327,10 @@ where
 						}
 
 						let txid = possible_output_spend.tx_hash;
+						if confirmed_txs.iter().any(|ctx| ctx.txid == txid) {
+							continue;
+						}
+
 						match self.client.transaction_get(&txid) {
 							Ok(tx) => {
 								let mut is_spend = false;
@@ -419,6 +426,7 @@ where
 						}
 						let confirmed_tx = ConfirmedTx {
 							tx: tx.clone(),
+							txid,
 							block_header, block_height: prob_conf_height,
 							pos,
 						};

lightning-transaction-sync/src/esplora.rs

@@ -267,10 +267,13 @@ where
 		// First, check the confirmation status of registered transactions as well as the
 		// status of dependent transactions of registered outputs.
 
-		let mut confirmed_txs = Vec::new();
+		let mut confirmed_txs: Vec<ConfirmedTx> = Vec::new();
 
 		for txid in &sync_state.watched_transactions {
-			if let Some(confirmed_tx) = maybe_await!(self.get_confirmed_tx(&txid, None, None))? {
+			if confirmed_txs.iter().any(|ctx| ctx.txid == *txid) {
+				continue;
+			}
+			if let Some(confirmed_tx) = maybe_await!(self.get_confirmed_tx(*txid, None, None))? {
 				confirmed_txs.push(confirmed_tx);
 			}
 		}
@@ -281,9 +284,19 @@ where
 			{
 				if let Some(spending_txid) = output_status.txid {
 					if let Some(spending_tx_status) = output_status.status {
+						if confirmed_txs.iter().any(|ctx| ctx.txid == spending_txid) {
+							if spending_tx_status.confirmed {
+								// Skip inserting duplicate ConfirmedTx entry
+								continue;
+							} else {
+								log_trace!(self.logger, "Inconsistency: Detected previously-confirmed Tx {} as unconfirmed", spending_txid);
+								return Err(InternalError::Inconsistency);
+							}
+						}
+
 						if let Some(confirmed_tx) = maybe_await!(self
 							.get_confirmed_tx(
-								&spending_txid,
+								spending_txid,
 								spending_tx_status.block_hash,
 								spending_tx_status.block_height,
 							))?
@@ -306,7 +319,7 @@ where
 
 	#[maybe_async]
 	fn get_confirmed_tx(
-		&self, txid: &Txid, expected_block_hash: Option<BlockHash>, known_block_height: Option<u32>,
+		&self, txid: Txid, expected_block_hash: Option<BlockHash>, known_block_height: Option<u32>,
 	) -> Result<Option<ConfirmedTx>, InternalError> {
 		if let Some(merkle_block) = maybe_await!(self.client.get_merkle_block(&txid))? {
 			let block_header = merkle_block.header;
@@ -321,22 +334,27 @@ where
 			let mut matches = Vec::new();
 			let mut indexes = Vec::new();
 			let _ = merkle_block.txn.extract_matches(&mut matches, &mut indexes);
-			if indexes.len() != 1 || matches.len() != 1 || matches[0] != *txid {
+			if indexes.len() != 1 || matches.len() != 1 || matches[0] != txid {
 				log_error!(self.logger, "Retrieved Merkle block for txid {} doesn't match expectations. This should not happen. Please verify server integrity.", txid);
 				return Err(InternalError::Failed);
 			}
 
 			// unwrap() safety: len() > 0 is checked above
 			let pos = *indexes.first().unwrap() as usize;
 			if let Some(tx) = maybe_await!(self.client.get_tx(&txid))? {
+				if tx.txid() != txid {
+					log_error!(self.logger, "Retrieved transaction for txid {} doesn't match expectations. This should not happen. Please verify server integrity.", txid);
+					return Err(InternalError::Failed);
+				}
+
 				if let Some(block_height) = known_block_height {
 					// We can take a shortcut here if a previous call already gave us the height.
-					return Ok(Some(ConfirmedTx { tx, block_header, pos, block_height }));
+					return Ok(Some(ConfirmedTx { tx, txid, block_header, pos, block_height }));
 				}
 
 				let block_status = maybe_await!(self.client.get_block_status(&block_hash))?;
 				if let Some(block_height) = block_status.height {
-					return Ok(Some(ConfirmedTx { tx, block_header, pos, block_height }));
+					return Ok(Some(ConfirmedTx { tx, txid, block_header, pos, block_height }));
 				} else {
 					// If any previously-confirmed block suddenly is no longer confirmed, we found
 					// an inconsistency and should start over.