diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index 5f2bb248cd1..a171b831861 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -3823,7 +3823,7 @@ impl ChannelMonitorImpl { // First check if a counterparty commitment transaction has been broadcasted: macro_rules! claim_htlcs { ($commitment_number: expr, $txid: expr, $htlcs: expr) => { - let (htlc_claim_reqs, _) = self.get_counterparty_output_claim_info(funding_spent, $commitment_number, $txid, None, $htlcs, confirmed_spend_height); + let htlc_claim_reqs = self.get_counterparty_output_claims_for_preimage(*payment_preimage, funding_spent, $commitment_number, $txid, $htlcs, confirmed_spend_height); let conf_target = self.closure_conf_target(); self.onchain_tx_handler.update_claims_view_from_requests( htlc_claim_reqs, self.best_block.height, self.best_block.height, broadcaster, @@ -4722,7 +4722,7 @@ impl ChannelMonitorImpl { (htlc, htlc_source.as_ref().map(|htlc_source| htlc_source.as_ref())) ), logger); let (htlc_claim_reqs, counterparty_output_info) = - self.get_counterparty_output_claim_info(funding_spent, commitment_number, commitment_txid, Some(commitment_tx), per_commitment_option, Some(height)); + self.get_counterparty_output_claim_info(funding_spent, commitment_number, commitment_txid, commitment_tx, per_commitment_claimable_data, Some(height)); to_counterparty_output_info = counterparty_output_info; for req in htlc_claim_reqs { claimable_outpoints.push(req); @@ -4732,87 +4732,156 @@ impl ChannelMonitorImpl { (claimable_outpoints, to_counterparty_output_info) } + fn get_point_for_commitment_number(&self, commitment_number: u64) -> Option { + let per_commitment_points = &self.their_cur_per_commitment_points?; + + // If the counterparty commitment tx is the latest valid state, use their latest + // per-commitment point + if per_commitment_points.0 == commitment_number { + Some(per_commitment_points.1) + } else if let Some(point) = per_commitment_points.2.as_ref() { + // If counterparty commitment tx is the state previous to the latest valid state, use + // their previous per-commitment point (non-atomicity of revocation means it's valid for + // them to temporarily have two valid commitment txns from our viewpoint) + if per_commitment_points.0 == commitment_number + 1 { + Some(*point) + } else { + None + } + } else { + None + } + } + + fn get_counterparty_output_claims_for_preimage( + &self, preimage: PaymentPreimage, funding_spent: &FundingScope, commitment_number: u64, + commitment_txid: Txid, + per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option>)>>, + confirmation_height: Option, + ) -> Vec { + let per_commitment_claimable_data = match per_commitment_option { + Some(outputs) => outputs, + None => return Vec::new(), + }; + let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) { + Some(point) => point, + None => return Vec::new(), + }; + + let matching_payment_hash = PaymentHash::from(preimage); + per_commitment_claimable_data + .iter() + .filter_map(|(htlc, _)| { + if let Some(transaction_output_index) = htlc.transaction_output_index { + if htlc.offered && htlc.payment_hash == matching_payment_hash { + let htlc_data = PackageSolvingData::CounterpartyOfferedHTLCOutput( + CounterpartyOfferedHTLCOutput::build( + per_commitment_point, + preimage, + htlc.clone(), + funding_spent.channel_parameters.clone(), + confirmation_height, + ), + ); + Some(PackageTemplate::build_package( + commitment_txid, + transaction_output_index, + htlc_data, + htlc.cltv_expiry, + )) + } else { + None + } + } else { + None + } + }) + .collect() + } + /// Returns the HTLC claim package templates and the counterparty output info - #[rustfmt::skip] fn get_counterparty_output_claim_info( &self, funding_spent: &FundingScope, commitment_number: u64, commitment_txid: Txid, - tx: Option<&Transaction>, - per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option>)>>, + tx: &Transaction, + per_commitment_claimable_data: &[(HTLCOutputInCommitment, Option>)], confirmation_height: Option, ) -> (Vec, CommitmentTxCounterpartyOutputInfo) { let mut claimable_outpoints = Vec::new(); let mut to_counterparty_output_info: CommitmentTxCounterpartyOutputInfo = None; - let per_commitment_claimable_data = match per_commitment_option { - Some(outputs) => outputs, - None => return (claimable_outpoints, to_counterparty_output_info), - }; - let per_commitment_points = match self.their_cur_per_commitment_points { - Some(points) => points, + let per_commitment_point = match self.get_point_for_commitment_number(commitment_number) { + Some(point) => point, None => return (claimable_outpoints, to_counterparty_output_info), }; - let per_commitment_point = - // If the counterparty commitment tx is the latest valid state, use their latest - // per-commitment point - if per_commitment_points.0 == commitment_number { &per_commitment_points.1 } - else if let Some(point) = per_commitment_points.2.as_ref() { - // If counterparty commitment tx is the state previous to the latest valid state, use - // their previous per-commitment point (non-atomicity of revocation means it's valid for - // them to temporarily have two valid commitment txns from our viewpoint) - if per_commitment_points.0 == commitment_number + 1 { - point - } else { return (claimable_outpoints, to_counterparty_output_info); } - } else { return (claimable_outpoints, to_counterparty_output_info); }; - - if let Some(transaction) = tx { - let revocation_pubkey = RevocationKey::from_basepoint( - &self.onchain_tx_handler.secp_ctx, &self.holder_revocation_basepoint, &per_commitment_point); - - let delayed_key = DelayedPaymentKey::from_basepoint(&self.onchain_tx_handler.secp_ctx, &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, &per_commitment_point); - - let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript(&revocation_pubkey, - self.counterparty_commitment_params.on_counterparty_tx_csv, - &delayed_key).to_p2wsh(); - for (idx, outp) in transaction.output.iter().enumerate() { - if outp.script_pubkey == revokeable_p2wsh { - to_counterparty_output_info = - Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); - } + let revocation_pubkey = RevocationKey::from_basepoint( + &self.onchain_tx_handler.secp_ctx, + &self.holder_revocation_basepoint, + &per_commitment_point, + ); + let delayed_key = DelayedPaymentKey::from_basepoint( + &self.onchain_tx_handler.secp_ctx, + &self.counterparty_commitment_params.counterparty_delayed_payment_base_key, + &per_commitment_point, + ); + let revokeable_p2wsh = chan_utils::get_revokeable_redeemscript( + &revocation_pubkey, + self.counterparty_commitment_params.on_counterparty_tx_csv, + &delayed_key, + ) + .to_p2wsh(); + for (idx, outp) in tx.output.iter().enumerate() { + if outp.script_pubkey == revokeable_p2wsh { + to_counterparty_output_info = + Some((idx.try_into().expect("Can't have > 2^32 outputs"), outp.value)); } } - for &(ref htlc, _) in per_commitment_claimable_data.iter() { + for &(ref htlc, _) in per_commitment_claimable_data.iter() { if let Some(transaction_output_index) = htlc.transaction_output_index { - if let Some(transaction) = tx { - if transaction_output_index as usize >= transaction.output.len() || - transaction.output[transaction_output_index as usize].value != htlc.to_bitcoin_amount() { - // per_commitment_data is corrupt or our commitment signing key leaked! - return (claimable_outpoints, to_counterparty_output_info); - } + if transaction_output_index as usize >= tx.output.len() + || tx.output[transaction_output_index as usize].value + != htlc.to_bitcoin_amount() + { + // per_commitment_data is corrupt or our commitment signing key leaked! + return (claimable_outpoints, to_counterparty_output_info); } - let preimage = if htlc.offered { if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) { Some(*p) } else { None } } else { None }; + let preimage = if htlc.offered { + if let Some((p, _)) = self.payment_preimages.get(&htlc.payment_hash) { + Some(*p) + } else { + None + } + } else { + None + }; if preimage.is_some() || !htlc.offered { let counterparty_htlc_outp = if htlc.offered { PackageSolvingData::CounterpartyOfferedHTLCOutput( CounterpartyOfferedHTLCOutput::build( - *per_commitment_point, preimage.unwrap(), + per_commitment_point, + preimage.unwrap(), htlc.clone(), funding_spent.channel_parameters.clone(), confirmation_height, - ) + ), ) } else { PackageSolvingData::CounterpartyReceivedHTLCOutput( CounterpartyReceivedHTLCOutput::build( - *per_commitment_point, + per_commitment_point, htlc.clone(), funding_spent.channel_parameters.clone(), confirmation_height, - ) + ), ) }; - let counterparty_package = PackageTemplate::build_package(commitment_txid, transaction_output_index, counterparty_htlc_outp, htlc.cltv_expiry); + let counterparty_package = PackageTemplate::build_package( + commitment_txid, + transaction_output_index, + counterparty_htlc_outp, + htlc.cltv_expiry, + ); claimable_outpoints.push(counterparty_package); } } diff --git a/lightning/src/ln/monitor_tests.rs b/lightning/src/ln/monitor_tests.rs index b316381398e..354cd48c6be 100644 --- a/lightning/src/ln/monitor_tests.rs +++ b/lightning/src/ln/monitor_tests.rs @@ -3836,3 +3836,85 @@ fn test_lost_timeout_monitor_events() { do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, false, true); do_test_lost_timeout_monitor_events(CommitmentType::LocalWithLastHTLC, true, true); } + +#[test] +fn test_ladder_preimage_htlc_claims() { + // Tests that when we learn of a preimage via a monitor update we only claim HTLCs with the + // corresponding payment hash. This test is a reproduction of a scenario that happened in + // production where the second HTLC claim also included the first HTLC (even though it was + // already claimed) resulting in an invalid claim transaction. + let chanmon_cfgs = create_chanmon_cfgs(2); + let node_cfgs = create_node_cfgs(2, &chanmon_cfgs); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let mut nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + let node_id_0 = nodes[0].node.get_our_node_id(); + let node_id_1 = nodes[1].node.get_our_node_id(); + + let (_, _, channel_id, _) = create_announced_chan_between_nodes_with_value(&nodes, 0, 1, 1_000_000, 0); + + let (payment_preimage1, payment_hash1, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000); + let (payment_preimage2, payment_hash2, _, _) = route_payment(&nodes[0], &[&nodes[1]], 1_000_000); + + nodes[0].node.force_close_broadcasting_latest_txn(&channel_id, &node_id_1, "test".to_string()).unwrap(); + check_added_monitors(&nodes[0], 1); + check_closed_broadcast(&nodes[0], 1, true); + let reason = ClosureReason::HolderForceClosed { broadcasted_latest_txn: Some(true), message: "test".to_string() }; + check_closed_event(&nodes[0], 1, reason, false, &[node_id_1], 1_000_000); + + let commitment_tx = { + let mut txn = nodes[0].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1); + txn.remove(0) + }; + mine_transaction(&nodes[0], &commitment_tx); + mine_transaction(&nodes[1], &commitment_tx); + + check_closed_broadcast(&nodes[1], 1, true); + check_added_monitors(&nodes[1], 1); + check_closed_event(&nodes[1], 1, ClosureReason::CommitmentTxConfirmed, false, &[node_id_0], 1_000_000); + + nodes[1].node.claim_funds(payment_preimage1); + expect_payment_claimed!(&nodes[1], payment_hash1, 1_000_000); + check_added_monitors(&nodes[1], 1); + + let (htlc1, htlc_claim_tx1) = { + let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1); + let htlc_claim_tx = txn.remove(0); + assert_eq!(htlc_claim_tx.input.len(), 1); + check_spends!(htlc_claim_tx, commitment_tx); + (htlc_claim_tx.input[0].previous_output, htlc_claim_tx) + }; + mine_transaction(&nodes[0], &htlc_claim_tx1); + mine_transaction(&nodes[1], &htlc_claim_tx1); + + connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + + expect_payment_sent(&nodes[0], payment_preimage1, None, true, false); + check_added_monitors(&nodes[0], 1); + + nodes[1].node.claim_funds(payment_preimage2); + expect_payment_claimed!(&nodes[1], payment_hash2, 1_000_000); + check_added_monitors(&nodes[1], 1); + + let (htlc2, htlc_claim_tx2) = { + let mut txn = nodes[1].tx_broadcaster.txn_broadcast(); + assert_eq!(txn.len(), 1, "{:?}", txn.iter().map(|tx| tx.compute_txid()).collect::>()); + let htlc_claim_tx = txn.remove(0); + assert_eq!(htlc_claim_tx.input.len(), 1); + check_spends!(htlc_claim_tx, commitment_tx); + (htlc_claim_tx.input[0].previous_output, htlc_claim_tx) + }; + assert_ne!(htlc1, htlc2); + + mine_transaction(&nodes[0], &htlc_claim_tx2); + mine_transaction(&nodes[1], &htlc_claim_tx2); + + connect_blocks(&nodes[0], ANTI_REORG_DELAY - 1); + connect_blocks(&nodes[1], ANTI_REORG_DELAY - 1); + + expect_payment_sent(&nodes[0], payment_preimage2, None, true, false); + check_added_monitors(&nodes[0], 1); +}