fix: never broadcast our commitment transaction from restored channel monitors

src/builder.rs

@@ -985,6 +985,7 @@ impl NodeBuilder {
 			logger,
 			Arc::new(vss_store),
 			self.reset_state,
+			false,
 		)
 	}
 
@@ -1037,6 +1038,7 @@ impl NodeBuilder {
 			logger,
 			kv_store,
 			self.reset_state,
+			self.monitors_to_restore.is_some(),
 		)
 	}
 }
@@ -1416,6 +1418,7 @@ fn build_with_store_internal(
 	pathfinding_scores_sync_config: Option<&PathfindingScoresSyncConfig>,
 	async_payments_role: Option<AsyncPaymentsRole>, seed_bytes: [u8; 64], runtime: Arc<Runtime>,
 	logger: Arc<Logger>, kv_store: Arc<DynStore>, reset_state: Option<ResetState>,
+	restored_channel_monitors: bool,
 ) -> Result<Node, BuildError> {
 	optionally_install_rustls_cryptoprovider();
 
@@ -1671,6 +1674,17 @@ fn build_with_store_internal(
 		},
 	};
 
+	// Alby: the monitors restored from a static channel backup are outdated, so we must never
+	// broadcast our commitment transaction built from them. The channels are recovered by the
+	// counterparty force-closing them.
+	if restored_channel_monitors {
+		tx_broadcaster.block_funding_outpoint_spends(
+			channel_monitors.iter().map(|(_, channel_monitor)| {
+				channel_monitor.get_funding_txo().into_bitcoin_outpoint()
+			}),
+		);
+	}
+
 	// Initialize the ChainMonitor
 	let chain_monitor: Arc<ChainMonitor> = Arc::new(chainmonitor::ChainMonitor::new(
 		Some(Arc::clone(&chain_source)),

src/tx_broadcaster.rs

@@ -5,9 +5,10 @@
 // http://opensource.org/licenses/MIT>, at your option. You may not use this file except in
 // accordance with one or both of these licenses.
 
+use std::collections::HashSet;
 use std::ops::Deref;
 
-use bitcoin::Transaction;
+use bitcoin::{OutPoint, Transaction};
 use lightning::chain::chaininterface::BroadcasterInterface;
 use tokio::sync::{mpsc, Mutex, MutexGuard};
 
@@ -21,6 +22,8 @@ where
 {
 	queue_sender: mpsc::Sender<Vec<Transaction>>,
 	queue_receiver: Mutex<mpsc::Receiver<Vec<Transaction>>>,
+	// Alby: funding outputs of channels whose monitors were restored from a static channel backup
+	blocked_funding_outpoints: std::sync::Mutex<HashSet<OutPoint>>,
 	logger: L,
 }
 
@@ -30,7 +33,30 @@ where
 {
 	pub(crate) fn new(logger: L) -> Self {
 		let (queue_sender, queue_receiver) = mpsc::channel(BCAST_PACKAGE_QUEUE_SIZE);
-		Self { queue_sender, queue_receiver: Mutex::new(queue_receiver), logger }
+		Self {
+			queue_sender,
+			queue_receiver: Mutex::new(queue_receiver),
+			blocked_funding_outpoints: std::sync::Mutex::new(HashSet::new()),
+			logger,
+		}
+	}
+
+	/// Alby: never broadcast transactions spending the given channel funding outputs.
+	///
+	/// Channel monitors restored from a static channel backup are outdated, so a commitment
+	/// transaction built from them (e.g. to time out an expired HTLC) is likely revoked and
+	/// broadcasting it would allow the counterparty to claim the whole channel balance with a
+	/// penalty transaction. The only transactions spending a funding output are our commitment
+	/// transactions and cooperative closes, which cannot happen without a channel manager.
+	pub(crate) fn block_funding_outpoint_spends(
+		&self, outpoints: impl IntoIterator<Item = OutPoint>,
+	) {
+		self.blocked_funding_outpoints.lock().unwrap().extend(outpoints);
+	}
+
+	fn spends_blocked_funding_outpoint(&self, tx: &Transaction) -> bool {
+		let blocked_funding_outpoints = self.blocked_funding_outpoints.lock().unwrap();
+		tx.input.iter().any(|input| blocked_funding_outpoints.contains(&input.previous_output))
 	}
 
 	pub(crate) async fn get_broadcast_queue(
@@ -45,9 +71,67 @@ where
 	L::Target: LdkLogger,
 {
 	fn broadcast_transactions(&self, txs: &[&Transaction]) {
+		// Alby: drop the whole package, the other transactions (e.g. an anchor CPFP) depend on the
+		// commitment transaction.
+		if let Some(tx) = txs.iter().find(|tx| self.spends_blocked_funding_outpoint(tx)) {
+			log_error!(
+				self.logger,
+				"Refusing to broadcast transaction {} spending the funding output of a restored channel monitor",
+				tx.compute_txid()
+			);
+			return;
+		}
 		let package = txs.iter().map(|&t| t.clone()).collect::<Vec<Transaction>>();
 		self.queue_sender.try_send(package).unwrap_or_else(|e| {
 			log_error!(self.logger, "Failed to broadcast transactions: {}", e);
 		});
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use std::str::FromStr;
+	use std::sync::Arc;
+
+	use bitcoin::absolute::LockTime;
+	use bitcoin::transaction::Version;
+	use bitcoin::{Amount, ScriptBuf, Sequence, TxIn, TxOut, Txid, Witness};
+
+	use super::*;
+	use crate::logger::Logger;
+
+	fn spending_tx(previous_output: OutPoint) -> Transaction {
+		Transaction {
+			version: Version::TWO,
+			lock_time: LockTime::ZERO,
+			input: vec![TxIn {
+				previous_output,
+				script_sig: ScriptBuf::new(),
+				sequence: Sequence::MAX,
+				witness: Witness::new(),
+			}],
+			output: vec![TxOut { value: Amount::from_sat(1000), script_pubkey: ScriptBuf::new() }],
+		}
+	}
+
+	#[tokio::test]
+	async fn refuses_to_broadcast_spends_of_blocked_funding_outpoints() {
+		let broadcaster = TransactionBroadcaster::new(Arc::new(Logger::new_log_facade()));
+		let txid =
+			Txid::from_str("4141414141414141414141414141414141414141414141414141414141414141")
+				.unwrap();
+		let funding_outpoint = OutPoint { txid, vout: 0 };
+		broadcaster.block_funding_outpoint_spends([funding_outpoint]);
+
+		let commitment_tx = spending_tx(funding_outpoint);
+		let anchor_cpfp_tx = spending_tx(OutPoint { txid: commitment_tx.compute_txid(), vout: 0 });
+		broadcaster.broadcast_transactions(&[&commitment_tx, &anchor_cpfp_tx]);
+
+		let other_tx = spending_tx(OutPoint { txid, vout: 1 });
+		broadcaster.broadcast_transactions(&[&other_tx]);
+
+		let mut queue = broadcaster.get_broadcast_queue().await;
+		assert_eq!(queue.try_recv().unwrap(), vec![other_tx]);
+		assert!(queue.try_recv().is_err());
+	}
+}

tests/integration_tests_rust.rs

@@ -14,24 +14,26 @@ use std::sync::Arc;
 use bitcoin::address::NetworkUnchecked;
 use bitcoin::hashes::sha256::Hash as Sha256Hash;
 use bitcoin::hashes::Hash;
-use bitcoin::{Address, Amount, ScriptBuf};
+use bitcoin::{Address, Amount, OutPoint, ScriptBuf};
 use common::logging::{init_log_logger, validate_log_entry, MultiNodeLogger, TestLogWriter};
 use common::{
 	bump_fee_and_broadcast, distribute_funds_unconfirmed, do_channel_full_cycle,
 	expect_channel_pending_event, expect_channel_ready_event, expect_event,
 	expect_payment_claimable_event, expect_payment_received_event, expect_payment_successful_event,
-	expect_splice_pending_event, generate_blocks_and_wait, open_channel, open_channel_push_amt,
-	premine_and_distribute_funds, premine_blocks, prepare_rbf, random_config,
-	random_listening_addresses, setup_bitcoind_and_electrsd, setup_builder, setup_node,
-	setup_node_for_async_payments, setup_two_nodes, wait_for_tx, TestChainSource, TestSyncStore,
+	expect_splice_pending_event, exponential_backoff_poll, generate_blocks_and_wait, open_channel,
+	open_channel_push_amt, premine_and_distribute_funds, premine_blocks, prepare_rbf,
+	random_config, random_listening_addresses, setup_bitcoind_and_electrsd, setup_builder,
+	setup_node, setup_node_for_async_payments, setup_two_nodes, wait_for_tx, TestChainSource,
+	TestSyncStore,
 };
+use electrum_client::ElectrumApi;
 use ldk_node::config::{AsyncPaymentsRole, EsploraSyncConfig};
 use ldk_node::liquidity::LSPS2ServiceConfig;
 use ldk_node::payment::{
 	ConfirmationStatus, PaymentDetails, PaymentDirection, PaymentKind, PaymentStatus,
 	QrPaymentResult,
 };
-use ldk_node::{Builder, DynStore, Event, NodeError};
+use ldk_node::{Builder, DynStore, Event, LightningBalance, NodeError};
 use lightning::ln::channelmanager::PaymentId;
 use lightning::routing::gossip::{NodeAlias, NodeId};
 use lightning::routing::router::RouteParametersConfig;
@@ -2301,3 +2303,168 @@ async fn lsps2_lsp_trusts_client_but_client_does_not_claim() {
 		Some(6)
 	);
 }
+
+/// Returns whether the given output was spent by a transaction in the mempool or a block.
+fn is_outpoint_spent<E: ElectrumApi>(electrs: &E, outpoint: OutPoint) -> bool {
+	let tx = electrs.transaction_get(&outpoint.txid).unwrap();
+	let script = tx.output.get(outpoint.vout as usize).unwrap().script_pubkey.clone();
+	// The history also contains the transaction creating the output.
+	electrs.script_get_history(&script).unwrap().iter().any(|entry| entry.tx_hash != outpoint.txid)
+}
+
+/// Alby: a node recovering from a static channel backup must never broadcast our commitment
+/// transaction from the restored (outdated) channel monitors, e.g. because the backup contains
+/// an HTLC that has expired in the meantime. That commitment is revoked, so the counterparty
+/// would claim the whole channel balance with a penalty transaction.
+#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
+async fn restored_channel_monitors_never_broadcast_revoked_commitment() {
+	let (bitcoind, electrsd) = setup_bitcoind_and_electrsd();
+	let chain_source = TestChainSource::Esplora(&electrsd);
+
+	let seed_bytes = vec![42u8; 64];
+	let node_a = setup_node(&chain_source, random_config(true), Some(seed_bytes.clone()));
+	let node_b = setup_node(&chain_source, random_config(true), None);
+
+	let addr_a = node_a.onchain_payment().new_address().unwrap();
+	let addr_b = node_b.onchain_payment().new_address().unwrap();
+	premine_and_distribute_funds(
+		&bitcoind.client,
+		&electrsd.client,
+		vec![addr_a, addr_b],
+		Amount::from_sat(2_100_000),
+	)
+	.await;
+	node_a.sync_wallets().unwrap();
+	node_b.sync_wallets().unwrap();
+
+	let funding_amount_sat = 1_000_000;
+	let funding_txo = open_channel(&node_a, &node_b, funding_amount_sat, false, &electrsd).await;
+	generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 6).await;
+	node_a.sync_wallets().unwrap();
+	node_b.sync_wallets().unwrap();
+	expect_channel_ready_event!(node_a, node_b.node_id());
+	expect_channel_ready_event!(node_b, node_a.node_id());
+
+	// A pays a hold invoice of B, so the outbound HTLC stays pending.
+	let amount_msat = 100_000_000;
+	let preimage = PaymentPreimage([42u8; 32]);
+	let payment_hash = PaymentHash(Sha256Hash::hash(&preimage.0).to_byte_array());
+	let invoice_description =
+		Bolt11InvoiceDescription::Direct(Description::new(String::from("hold")).unwrap());
+	let invoice = node_b
+		.bolt11_payment()
+		.receive_for_hash_with_min_cltv_expiry_delta(
+			amount_msat,
+			&invoice_description.into(),
+			3600,
+			payment_hash,
+			40,
+		)
+		.unwrap();
+	let payment_id = node_a.bolt11_payment().send(&invoice, None).unwrap();
+	let claimable_amount_msat =
+		expect_payment_claimable_event!(node_b, payment_id, payment_hash, amount_msat);
+
+	let htlc_expiry_height = node_a
+		.list_balances()
+		.lightning_balances
+		.iter()
+		.find_map(|balance| match balance {
+			LightningBalance::MaybeTimeoutClaimableHTLC { claimable_height, .. } => {
+				Some(*claimable_height)
+			},
+			_ => None,
+		})
+		.expect("the outbound HTLC should be pending");
+
+	// The stored channel monitor is only fully rewritten every few blocks (or every 100
+	// updates), so sync block by block to make sure the stored monitor contains the HTLC.
+	for _ in 0..6 {
+		generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 1).await;
+		node_a.sync_wallets().unwrap();
+		node_b.sync_wallets().unwrap();
+	}
+	let backup = node_a.get_encoded_channel_monitors().unwrap();
+	assert_eq!(backup.len(), 1);
+
+	// B settles the payment, which revokes A's commitment transaction contained in the backup.
+	node_b.bolt11_payment().claim_for_hash(payment_hash, claimable_amount_msat, preimage).unwrap();
+	expect_payment_received_event!(node_b, claimable_amount_msat);
+	expect_payment_successful_event!(node_a, Some(payment_id), None);
+	exponential_backoff_poll(|| {
+		let htlc_pending =
+			node_a.list_balances().lightning_balances.iter().any(|balance| {
+				matches!(balance, LightningBalance::MaybeTimeoutClaimableHTLC { .. })
+			});
+		(!htlc_pending).then_some(())
+	})
+	.await;
+	tokio::time::sleep(std::time::Duration::from_secs(1)).await;
+
+	// A's disk is lost. By the time it recovers, the HTLC in the backup has expired.
+	node_a.stop().unwrap();
+	drop(node_a);
+	let cur_height = bitcoind.client.get_blockchain_info().unwrap().blocks as u32;
+	let blocks_to_expiry = (htlc_expiry_height + 10).saturating_sub(cur_height);
+	generate_blocks_and_wait(&bitcoind.client, &electrsd.client, blocks_to_expiry as usize).await;
+	node_b.sync_wallets().unwrap();
+
+	// Recover A from the backup like hub-recovery does: same seed, fresh storage, restored
+	// channel monitors.
+	let restored_config = random_config(true);
+	setup_builder!(builder, restored_config.node_config);
+	let esplora_url = format!("http://{}", electrsd.esplora_url.as_ref().unwrap());
+	builder.set_chain_source_esplora(
+		esplora_url,
+		Some(EsploraSyncConfig { background_sync_config: None }),
+	);
+	#[cfg(feature = "uniffi")]
+	builder.set_entropy_seed_bytes(seed_bytes).unwrap();
+	#[cfg(not(feature = "uniffi"))]
+	builder.set_entropy_seed_bytes(seed_bytes.try_into().unwrap());
+	builder.restore_encoded_channel_monitors(backup);
+	let kv_store =
+		Arc::new(TestSyncStore::new(restored_config.node_config.storage_dir_path.clone().into()));
+	let restored_node = builder.build_with_store(kv_store).unwrap();
+	restored_node.start().unwrap();
+
+	// Connecting new blocks makes the restored monitor want to go on-chain for the expired HTLC.
+	restored_node.sync_wallets().unwrap();
+	generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 1).await;
+	restored_node.sync_wallets().unwrap();
+	node_b.sync_wallets().unwrap();
+	tokio::time::sleep(std::time::Duration::from_secs(5)).await;
+	assert!(
+		!is_outpoint_spent(&electrsd.client, funding_txo),
+		"the restored node broadcast its revoked commitment transaction"
+	);
+
+	// The channel is recovered by B force-closing it once we connect.
+	let onchain_balance_before_sat = restored_node.list_balances().spendable_onchain_balance_sats;
+	restored_node
+		.connect(
+			node_b.node_id(),
+			node_b.listening_addresses().unwrap().first().unwrap().clone(),
+			false,
+		)
+		.unwrap();
+
+	// A's channel balance minus the payment and on-chain fees.
+	let min_recovered_sat = funding_amount_sat - amount_msat / 1000 - 50_000;
+	let mut recovered = false;
+	for _ in 0..30 {
+		generate_blocks_and_wait(&bitcoind.client, &electrsd.client, 1).await;
+		restored_node.sync_wallets().unwrap();
+		node_b.sync_wallets().unwrap();
+		let onchain_balance_sat = restored_node.list_balances().spendable_onchain_balance_sats;
+		if onchain_balance_sat >= onchain_balance_before_sat + min_recovered_sat {
+			recovered = true;
+			break;
+		}
+	}
+	assert!(is_outpoint_spent(&electrsd.client, funding_txo));
+	assert!(recovered, "the restored node did not recover its channel balance");
+
+	restored_node.stop().unwrap();
+	node_b.stop().unwrap();
+}