From cc6d9aa05b63f3d93a100f2ca2bd346e086d9752 Mon Sep 17 00:00:00 2001 From: rolandgx Date: Thu, 15 Oct 2026 05:21:07 +0000 Subject: [PATCH 1/2] fix: never broadcast our commitment transaction from restored channel monitors 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 contained in the backup) is likely revoked and allows the counterparty to claim the whole channel balance with a penalty transaction. When monitors are restored, refuse to broadcast any transaction spending their funding outputs. The channels are recovered by the counterparty force-closing them. Co-Authored-By: Claude Opus 5.5 --- src/builder.rs | 14 +++++++ src/tx_broadcaster.rs | 88 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 100 insertions(+), 2 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index 6d0d9e8ad5..a8b47de255 100644 --- a/src/builder.rs +++ b/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, seed_bytes: [u8; 64], runtime: Arc, logger: Arc, kv_store: Arc, reset_state: Option, + restored_channel_monitors: bool, ) -> Result { 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 = Arc::new(chainmonitor::ChainMonitor::new( Some(Arc::clone(&chain_source)), diff --git a/src/tx_broadcaster.rs b/src/tx_broadcaster.rs index 12a1fe650c..1b268523b7 100644 --- a/src/tx_broadcaster.rs +++ b/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>, queue_receiver: Mutex>>, + // Alby: funding outputs of channels whose monitors were restored from a static channel backup + blocked_funding_outpoints: std::sync::Mutex>, 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, + ) { + 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::>(); 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()); + } +} From dcef87e9787663e9162c4001f18ee321c8387327 Mon Sep 17 00:00:00 2001 From: rolandgx Date: Thu, 15 Oct 2026 06:08:00 +0000 Subject: [PATCH 2/2] test: recover from a backup containing an expired HTLC End-to-end regtest test for restoring channel monitors from a static channel backup that contains an outbound HTLC which has expired by the time of the recovery. The restored node must not broadcast its revoked commitment transaction, and must recover its channel balance once the counterparty force-closes the channel. Fails without the previous commit: the restored node force-closes due to the HTLC timeout and broadcasts the revoked commitment transaction. Co-Authored-By: Claude Opus 5.5 --- tests/integration_tests_rust.rs | 179 ++++++++++++++++++++++++++++++-- 1 file changed, 173 insertions(+), 6 deletions(-) diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs index d6c7c9447f..4e100ea1b6 100644 --- a/tests/integration_tests_rust.rs +++ b/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(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(); +}