From 21804de70c3ff37970ead98e5e72c4fccd14b1fb Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 18 Nov 2022 18:54:16 +0000 Subject: [PATCH] Ensure `transactions_confirmed` is idempotent In many complexity-reduced implementations of chain syncing using esplora `transactions_confirmed` may be called redundantly for transactions which were already confirmed. To ensure this is idempotent we add two new `ConnectionStyle`s in our tests which (a) call `transactions_confirmed` twice for each call, ensuring simple idempotency is ensured and (b) call `transactions_confirmed` once for each historical block every time we're connecting a new block, ensuring we're fully idempotent even if every call is repeated constantly. In order to actually behave correctly this requires a simple already-confirmed check in `ChannelMonitor`, which is included. --- lightning/src/chain/channelmonitor.rs | 35 +++++++++++++++-- lightning/src/ln/functional_test_utils.rs | 46 ++++++++++++++++++++--- lightning/src/ln/functional_tests.rs | 22 +++++++---- lightning/src/ln/payment_tests.rs | 5 ++- 4 files changed, 90 insertions(+), 18 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 1cbc8535..bee2bfd0 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -2869,7 +2869,37 @@ impl ChannelMonitorImpl { let mut watch_outputs = Vec::new(); let mut claimable_outpoints = Vec::new(); - for tx in &txn_matched { + 'tx_iter: for tx in &txn_matched { + let txid = tx.txid(); + // If a transaction has already been confirmed, ensure we don't bother processing it duplicatively. + if Some(txid) == self.funding_spend_confirmed { + log_debug!(logger, "Skipping redundant processing of funding-spend tx {} as it was previously confirmed", txid); + continue 'tx_iter; + } + for ev in self.onchain_events_awaiting_threshold_conf.iter() { + if ev.txid == txid { + if let Some(conf_hash) = ev.block_hash { + assert_eq!(header.block_hash(), conf_hash, + "Transaction {} was already confirmed and is being re-confirmed in a different block.\n\ + This indicates a severe bug in the transaction connection logic - a reorg should have been processed first!", ev.txid); + } + log_debug!(logger, "Skipping redundant processing of confirming tx {} as it was previously confirmed", txid); + continue 'tx_iter; + } + } + for htlc in self.htlcs_resolved_on_chain.iter() { + if Some(txid) == htlc.resolving_txid { + log_debug!(logger, "Skipping redundant processing of HTLC resolution tx {} as it was previously confirmed", txid); + continue 'tx_iter; + } + } + for spendable_txid in self.spendable_txids_confirmed.iter() { + if txid == *spendable_txid { + log_debug!(logger, "Skipping redundant processing of spendable tx {} as it was previously confirmed", txid); + continue 'tx_iter; + } + } + if tx.input.len() == 1 { // Assuming our keys were not leaked (in which case we're screwed no matter what), // commitment transactions and HTLC transactions will all only ever have one input, @@ -2879,7 +2909,7 @@ impl ChannelMonitorImpl { if prevout.txid == self.funding_info.0.txid && prevout.vout == self.funding_info.0.index as u32 { let mut balance_spendable_csv = None; log_info!(logger, "Channel {} closed by funding output spend in txid {}.", - log_bytes!(self.funding_info.0.to_channel_id()), tx.txid()); + log_bytes!(self.funding_info.0.to_channel_id()), txid); self.funding_spend_seen = true; let mut commitment_tx_to_counterparty_output = None; if (tx.input[0].sequence.0 >> 8*3) as u8 == 0x80 && (tx.lock_time.0 >> 8*3) as u8 == 0x20 { @@ -2902,7 +2932,6 @@ impl ChannelMonitorImpl { } } } - let txid = tx.txid(); self.onchain_events_awaiting_threshold_conf.push(OnchainEventEntry { txid, transaction: Some((*tx).clone()), diff --git a/lightning/src/ln/functional_test_utils.rs b/lightning/src/ln/functional_test_utils.rs index 677fdccf..64699dad 100644 --- a/lightning/src/ln/functional_test_utils.rs +++ b/lightning/src/ln/functional_test_utils.rs @@ -107,6 +107,14 @@ pub enum ConnectStyle { /// The same as `TransactionsFirst`, however when we have multiple blocks to connect, we only /// make a single `best_block_updated` call. TransactionsFirstSkippingBlocks, + /// The same as `TransactionsFirst`, however when we have multiple blocks to connect, we only + /// make a single `best_block_updated` call. Further, we call `transactions_confirmed` multiple + /// times to ensure it's idempotent. + TransactionsDuplicativelyFirstSkippingBlocks, + /// The same as `TransactionsFirst`, however when we have multiple blocks to connect, we only + /// make a single `best_block_updated` call. Further, we call `transactions_confirmed` multiple + /// times to ensure it's idempotent. + HighlyRedundantTransactionsFirstSkippingBlocks, /// The same as `TransactionsFirst` when connecting blocks. During disconnection only /// `transaction_unconfirmed` is called. TransactionsFirstReorgsOnlyTip, @@ -121,14 +129,16 @@ impl ConnectStyle { use core::hash::{BuildHasher, Hasher}; // Get a random value using the only std API to do so - the DefaultHasher let rand_val = std::collections::hash_map::RandomState::new().build_hasher().finish(); - let res = match rand_val % 7 { + let res = match rand_val % 9 { 0 => ConnectStyle::BestBlockFirst, 1 => ConnectStyle::BestBlockFirstSkippingBlocks, 2 => ConnectStyle::BestBlockFirstReorgsOnlyTip, 3 => ConnectStyle::TransactionsFirst, 4 => ConnectStyle::TransactionsFirstSkippingBlocks, - 5 => ConnectStyle::TransactionsFirstReorgsOnlyTip, - 6 => ConnectStyle::FullBlockViaListen, + 5 => ConnectStyle::TransactionsDuplicativelyFirstSkippingBlocks, + 6 => ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks, + 7 => ConnectStyle::TransactionsFirstReorgsOnlyTip, + 8 => ConnectStyle::FullBlockViaListen, _ => unreachable!(), }; eprintln!("Using Block Connection Style: {:?}", res); @@ -143,6 +153,7 @@ impl ConnectStyle { pub fn connect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, depth: u32) -> BlockHash { let skip_intermediaries = match *node.connect_style.borrow() { ConnectStyle::BestBlockFirstSkippingBlocks|ConnectStyle::TransactionsFirstSkippingBlocks| + ConnectStyle::TransactionsDuplicativelyFirstSkippingBlocks|ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks| ConnectStyle::BestBlockFirstReorgsOnlyTip|ConnectStyle::TransactionsFirstReorgsOnlyTip => true, _ => false, }; @@ -193,8 +204,32 @@ fn do_connect_block<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, block: Block, sk node.node.best_block_updated(&block.header, height); node.node.transactions_confirmed(&block.header, &txdata, height); }, - ConnectStyle::TransactionsFirst|ConnectStyle::TransactionsFirstSkippingBlocks|ConnectStyle::TransactionsFirstReorgsOnlyTip => { + ConnectStyle::TransactionsFirst|ConnectStyle::TransactionsFirstSkippingBlocks| + ConnectStyle::TransactionsDuplicativelyFirstSkippingBlocks|ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks| + ConnectStyle::TransactionsFirstReorgsOnlyTip => { + if *node.connect_style.borrow() == ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks { + let mut connections = Vec::new(); + for (block, height) in node.blocks.lock().unwrap().iter() { + if !block.txdata.is_empty() { + // Reconnect all transactions we've ever seen to ensure transaction connection + // is *really* idempotent. This is a somewhat likely deployment for some + // esplora implementations of chain sync which try to reduce state and + // complexity as much as possible. + // + // Sadly we have to clone the block here to maintain lockorder. In the + // future we should consider Arc'ing the blocks to avoid this. + connections.push((block.clone(), *height)); + } + } + for (old_block, height) in connections { + node.chain_monitor.chain_monitor.transactions_confirmed(&old_block.header, + &old_block.txdata.iter().enumerate().collect::>(), height); + } + } node.chain_monitor.chain_monitor.transactions_confirmed(&block.header, &txdata, height); + if *node.connect_style.borrow() == ConnectStyle::TransactionsDuplicativelyFirstSkippingBlocks { + node.chain_monitor.chain_monitor.transactions_confirmed(&block.header, &txdata, height); + } call_claimable_balances(node); node.chain_monitor.chain_monitor.best_block_updated(&block.header, height); node.node.transactions_confirmed(&block.header, &txdata, height); @@ -226,7 +261,8 @@ pub fn disconnect_blocks<'a, 'b, 'c, 'd>(node: &'a Node<'b, 'c, 'd>, count: u32) node.chain_monitor.chain_monitor.block_disconnected(&orig.0.header, orig.1); Listen::block_disconnected(node.node, &orig.0.header, orig.1); }, - ConnectStyle::BestBlockFirstSkippingBlocks|ConnectStyle::TransactionsFirstSkippingBlocks => { + ConnectStyle::BestBlockFirstSkippingBlocks|ConnectStyle::TransactionsFirstSkippingBlocks| + ConnectStyle::HighlyRedundantTransactionsFirstSkippingBlocks|ConnectStyle::TransactionsDuplicativelyFirstSkippingBlocks => { if i == count - 1 { node.chain_monitor.chain_monitor.best_block_updated(&prev.0.header, prev.1); node.node.best_block_updated(&prev.0.header, prev.1); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index 33eec98c..d240a155 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2814,12 +2814,17 @@ fn test_htlc_on_chain_success() { check_added_monitors!(nodes[1], 1); check_closed_event!(nodes[1], 1, ClosureReason::CommitmentTxConfirmed); let node_txn = nodes[1].tx_broadcaster.txn_broadcasted.lock().unwrap().clone(); - assert_eq!(node_txn.len(), 6); // ChannelManager : 3 (commitment tx + HTLC-Sucess * 2), ChannelMonitor : 3 (HTLC-Success, 2* RBF bumps of above HTLC txn) + assert!(node_txn.len() == 4 || node_txn.len() == 6); // ChannelManager : 3 (commitment tx + HTLC-Sucess * 2), ChannelMonitor : 3 (HTLC-Success, 2* RBF bumps of above HTLC txn) let commitment_spend = if node_txn[0].input[0].previous_output.txid == node_a_commitment_tx[0].txid() { - check_spends!(node_txn[1], commitment_tx[0]); - check_spends!(node_txn[2], commitment_tx[0]); - assert_ne!(node_txn[1].input[0].previous_output.vout, node_txn[2].input[0].previous_output.vout); + if node_txn.len() == 6 { + // In some block `ConnectionStyle`s we may avoid broadcasting the double-spending + // transactions spending the HTLC outputs of C's commitment transaction. Otherwise, + // check that the extra broadcasts (double-)spend those here. + check_spends!(node_txn[1], commitment_tx[0]); + check_spends!(node_txn[2], commitment_tx[0]); + assert_ne!(node_txn[1].input[0].previous_output.vout, node_txn[2].input[0].previous_output.vout); + } &node_txn[0] } else { check_spends!(node_txn[0], commitment_tx[0]); @@ -2834,10 +2839,11 @@ fn test_htlc_on_chain_success() { assert_eq!(commitment_spend.input[1].witness.last().unwrap().len(), OFFERED_HTLC_SCRIPT_WEIGHT); assert_eq!(commitment_spend.lock_time.0, 0); assert!(commitment_spend.output[0].script_pubkey.is_v0_p2wpkh()); // direct payment - check_spends!(node_txn[3], chan_1.3); - assert_eq!(node_txn[3].input[0].witness.clone().last().unwrap().len(), 71); - check_spends!(node_txn[4], node_txn[3]); - check_spends!(node_txn[5], node_txn[3]); + let funding_spend_offset = if node_txn.len() == 6 { 3 } else { 1 }; + check_spends!(node_txn[funding_spend_offset], chan_1.3); + assert_eq!(node_txn[funding_spend_offset].input[0].witness.clone().last().unwrap().len(), 71); + check_spends!(node_txn[funding_spend_offset + 1], node_txn[funding_spend_offset]); + check_spends!(node_txn[funding_spend_offset + 2], node_txn[funding_spend_offset]); // We don't bother to check that B can claim the HTLC output on its commitment tx here as // we already checked the same situation with A. diff --git a/lightning/src/ln/payment_tests.rs b/lightning/src/ln/payment_tests.rs index 27dc67a0..57c3140d 100644 --- a/lightning/src/ln/payment_tests.rs +++ b/lightning/src/ln/payment_tests.rs @@ -785,8 +785,9 @@ fn do_test_dup_htlc_onchain_fails_on_reload(persist_manager_post_event: bool, co let funding_txo = OutPoint { txid: funding_tx.txid(), index: 0 }; let mon_updates: Vec<_> = chanmon_cfgs[0].persister.chain_sync_monitor_persistences.lock().unwrap() .get_mut(&funding_txo).unwrap().drain().collect(); - // If we are using chain::Confirm instead of chain::Listen, we will get the same update twice - assert!(mon_updates.len() == 1 || mon_updates.len() == 2); + // If we are using chain::Confirm instead of chain::Listen, we will get the same update twice. + // If we're testing connection idempotency we may get substantially more. + assert!(mon_updates.len() >= 1); assert!(nodes[0].chain_monitor.release_pending_monitor_events().is_empty()); assert!(nodes[0].node.get_and_clear_pending_events().is_empty()); -- 2.30.2