From 96d17ee7370af557330ccd8618648e82a6899ac0 Mon Sep 17 00:00:00 2001 From: Antoine Riard Date: Mon, 10 Dec 2018 23:56:02 -0500 Subject: [PATCH] Add is_resolving_output in ChannelMonitor Called in ChannelMonitor block_connected, returning HTLCUpdate upstream via ManyChannelMonitor to link htlcs between monitors. Used by ChannelManager to fulfill/fail htlcs backwards accordingly If spurrious HTLCUpdate are generated due to block re-scan and htlc are already LocalRemoved, discard them in channel get_update_*_htlc --- src/ln/channel.rs | 51 ++++++++----- src/ln/channelmanager.rs | 14 ++-- src/ln/channelmonitor.rs | 161 +++++++++++++++++++++++++++++++++++---- src/util/test_utils.rs | 5 ++ 4 files changed, 193 insertions(+), 38 deletions(-) diff --git a/src/ln/channel.rs b/src/ln/channel.rs index 0eef7fbc..2b29fb5f 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -1150,21 +1150,33 @@ impl Channel { let mut payment_hash_calc = [0; 32]; sha.result(&mut payment_hash_calc); + // ChannelManager may generate duplicate claims/fails due to HTLC update events from + // on-chain ChannelsMonitors during block rescan. Ideally we'd figure out a way to drop + // these, but for now we just have to treat them as normal. + let mut pending_idx = std::usize::MAX; for (idx, htlc) in self.pending_inbound_htlcs.iter().enumerate() { if htlc.htlc_id == htlc_id_arg { assert_eq!(htlc.payment_hash, payment_hash_calc); - if let InboundHTLCState::Committed = htlc.state { - } else { - debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to"); - // Don't return in release mode here so that we can update channel_monitor + match htlc.state { + InboundHTLCState::Committed => {}, + InboundHTLCState::LocalRemoved(ref reason) => { + if let &InboundHTLCRemovalReason::Fulfill(_) = reason { + } else { + log_warn!(self, "Have preimage and want to fulfill HTLC with payment hash {} we already failed against channel {}", log_bytes!(htlc.payment_hash), log_bytes!(self.channel_id())); + } + return Ok((None, None)); + }, + _ => { + debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to"); + // Don't return in release mode here so that we can update channel_monitor + } } pending_idx = idx; break; } } if pending_idx == std::usize::MAX { - debug_assert!(false, "Unable to find a pending HTLC which matched the given HTLC ID"); return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID")); } @@ -1179,15 +1191,14 @@ impl Channel { match pending_update { &HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => { if htlc_id_arg == htlc_id { - debug_assert!(false, "Tried to fulfill an HTLC we already had a pending fulfill for"); return Ok((None, None)); } }, &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => { if htlc_id_arg == htlc_id { - debug_assert!(false, "Tried to fulfill an HTLC we already had a holding-cell failure on"); - // Return the new channel monitor in a last-ditch effort to hit the - // chain and claim the funds + log_warn!(self, "Have preimage and want to fulfill HTLC with pending failure against channel {}", log_bytes!(self.channel_id())); + // TODO: We may actually be able to switch to a fulfill here, though its + // rare enough it may not be worth the complexity burden. return Ok((None, Some(self.channel_monitor.clone()))); } }, @@ -1237,19 +1248,27 @@ impl Channel { } assert_eq!(self.channel_state & ChannelState::ShutdownComplete as u32, 0); + // ChannelManager may generate duplicate claims/fails due to HTLC update events from + // on-chain ChannelsMonitors during block rescan. Ideally we'd figure out a way to drop + // these, but for now we just have to treat them as normal. + let mut pending_idx = std::usize::MAX; for (idx, htlc) in self.pending_inbound_htlcs.iter().enumerate() { if htlc.htlc_id == htlc_id_arg { - if let InboundHTLCState::Committed = htlc.state { - } else { - debug_assert!(false, "Have an inbound HTLC we tried to fail before it was fully committed to"); - return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID")); + match htlc.state { + InboundHTLCState::Committed => {}, + InboundHTLCState::LocalRemoved(_) => { + return Ok(None); + }, + _ => { + debug_assert!(false, "Have an inbound HTLC we tried to claim before it was fully committed to"); + return Err(ChannelError::Ignore("Unable to find a pending HTLC which matchd the given HTLC ID")); + } } pending_idx = idx; } } if pending_idx == std::usize::MAX { - debug_assert!(false, "Unable to find a pending HTLC which matched the given HTLC ID"); return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID")); } @@ -1259,14 +1278,12 @@ impl Channel { match pending_update { &HTLCUpdateAwaitingACK::ClaimHTLC { htlc_id, .. } => { if htlc_id_arg == htlc_id { - debug_assert!(false, "Unable to find a pending HTLC which matched the given HTLC ID"); return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID")); } }, &HTLCUpdateAwaitingACK::FailHTLC { htlc_id, .. } => { if htlc_id_arg == htlc_id { - debug_assert!(false, "Tried to fail an HTLC that we already had a pending failure for"); - return Ok(None); + return Err(ChannelError::Ignore("Unable to find a pending HTLC which matched the given HTLC ID")); } }, _ => {} diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 26224dc5..52819526 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -679,13 +679,6 @@ impl ChannelManager { for tx in local_txn { self.tx_broadcaster.broadcast_transaction(&tx); } - //TODO: We need to have a way where outbound HTLC claims can result in us claiming the - //now-on-chain HTLC output for ourselves (and, thereafter, passing the HTLC backwards). - //TODO: We need to handle monitoring of pending offered HTLCs which just hit the chain and - //may be claimed, resulting in us claiming the inbound HTLCs (and back-failing after - //timeouts are hit and our claims confirm). - //TODO: In any case, we need to make sure we remove any pending htlc tracking (via - //fail_backwards or claim_funds) eventually for all HTLCs that were in the channel } /// Force closes a channel, immediately broadcasting the latest local commitment transaction to @@ -2718,6 +2711,13 @@ impl ChainListener for ChannelManager { for failure in failed_channels.drain(..) { self.finish_force_close_channel(failure); } + { + for htlc_update in self.monitor.fetch_pending_htlc_updated() { + if let Some(preimage) = htlc_update.payment_preimage { + self.claim_funds_internal(self.channel_state.lock().unwrap(), htlc_update.source, preimage); + } + } + } self.latest_block_height.store(height as usize, Ordering::Release); *self.last_block_hash.try_lock().expect("block_(dis)connected must not be called in parallel") = header.bitcoin_hash(); } diff --git a/src/ln/channelmonitor.rs b/src/ln/channelmonitor.rs index 15be91c3..7c4dd735 100644 --- a/src/ln/channelmonitor.rs +++ b/src/ln/channelmonitor.rs @@ -38,7 +38,7 @@ use util::ser::{ReadableArgs, Readable, Writer, Writeable, WriterWriteAdaptor, U use util::sha2::Sha256; use util::{byte_utils, events}; -use std::collections::HashMap; +use std::collections::{HashMap, hash_map}; use std::sync::{Arc,Mutex}; use std::{hash,cmp, mem}; @@ -85,6 +85,14 @@ pub enum ChannelMonitorUpdateErr { #[derive(Debug)] pub struct MonitorUpdateError(pub &'static str); +/// Simple structure send back by ManyChannelMonitor in case of HTLC detected onchain from a +/// forward channel and from which info are needed to update HTLC in a backward channel. +pub struct HTLCUpdate { + pub(super) payment_hash: [u8; 32], + pub(super) payment_preimage: Option<[u8; 32]>, + pub(super) source: HTLCSource +} + /// Simple trait indicating ability to track a set of ChannelMonitors and multiplex events between /// them. Generally should be implemented by keeping a local SimpleManyChannelMonitor and passing /// events to it, while also taking any add_update_monitor events and passing them to some remote @@ -101,6 +109,10 @@ pub trait ManyChannelMonitor: Send + Sync { /// ChainWatchInterfaces such that the provided monitor receives block_connected callbacks with /// any spends of it. fn add_update_monitor(&self, funding_txo: OutPoint, monitor: ChannelMonitor) -> Result<(), ChannelMonitorUpdateErr>; + + /// Used by ChannelManager to get list of HTLC resolved onchain and which needed to be updated + /// with success or failure backward + fn fetch_pending_htlc_updated(&self) -> Vec; } /// A simple implementation of a ManyChannelMonitor and ChainListener. Can be used to create a @@ -122,6 +134,7 @@ pub struct SimpleManyChannelMonitor { chain_monitor: Arc, broadcaster: Arc, pending_events: Mutex>, + pending_htlc_updated: Mutex)>>>, logger: Arc, } @@ -129,20 +142,55 @@ impl ChainListener for SimpleManyChannelMonit fn block_connected(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], _indexes_of_txn_matched: &[u32]) { let block_hash = header.bitcoin_hash(); let mut new_events: Vec = Vec::with_capacity(0); + let mut htlc_updated_infos = Vec::new(); { let mut monitors = self.monitors.lock().unwrap(); for monitor in monitors.values_mut() { - let (txn_outputs, spendable_outputs) = monitor.block_connected(txn_matched, height, &block_hash, &*self.broadcaster); + let (txn_outputs, spendable_outputs, mut htlc_updated) = monitor.block_connected(txn_matched, height, &block_hash, &*self.broadcaster); if spendable_outputs.len() > 0 { new_events.push(events::Event::SpendableOutputs { outputs: spendable_outputs, }); } + for (ref txid, ref outputs) in txn_outputs { for (idx, output) in outputs.iter().enumerate() { self.chain_monitor.install_watch_outpoint((txid.clone(), idx as u32), &output.script_pubkey); } } + htlc_updated_infos.append(&mut htlc_updated); + } + } + { + // ChannelManager will just need to fetch pending_htlc_updated and pass state backward + let mut pending_htlc_updated = self.pending_htlc_updated.lock().unwrap(); + for htlc in htlc_updated_infos.drain(..) { + match pending_htlc_updated.entry(htlc.2) { + hash_map::Entry::Occupied(mut e) => { + // In case of reorg we may have htlc outputs solved in a different way so + // we prefer to keep claims but don't store duplicate updates for a given + // (payment_hash, HTLCSource) pair. + // TODO: Note that we currently don't really use this as ChannelManager + // will fail/claim backwards after the first block. We really should delay + // a few blocks before failing backwards (but can claim backwards + // immediately) as long as we have a few blocks of headroom. + let mut existing_claim = false; + e.get_mut().retain(|htlc_data| { + if htlc.0 == htlc_data.0 { + if htlc_data.1.is_some() { + existing_claim = true; + true + } else { false } + } else { true } + }); + if !existing_claim { + e.get_mut().push((htlc.0, htlc.1)); + } + } + hash_map::Entry::Vacant(e) => { + e.insert(vec![(htlc.0, htlc.1)]); + } + } } } let mut pending_events = self.pending_events.lock().unwrap(); @@ -161,6 +209,7 @@ impl SimpleManyChannelMonitor chain_monitor, broadcaster, pending_events: Mutex::new(Vec::new()), + pending_htlc_updated: Mutex::new(HashMap::new()), logger, }); let weak_res = Arc::downgrade(&res); @@ -207,6 +256,21 @@ impl ManyChannelMonitor for SimpleManyChannelMonitor { Err(_) => Err(ChannelMonitorUpdateErr::PermanentFailure), } } + + fn fetch_pending_htlc_updated(&self) -> Vec { + let mut updated = self.pending_htlc_updated.lock().unwrap(); + let mut pending_htlcs_updated = Vec::with_capacity(updated.len()); + for (k, v) in updated.drain() { + for htlc_data in v { + pending_htlcs_updated.push(HTLCUpdate { + payment_hash: k, + payment_preimage: htlc_data.1, + source: htlc_data.0, + }); + } + } + pending_htlcs_updated + } } impl events::EventsProvider for SimpleManyChannelMonitor { @@ -928,12 +992,15 @@ impl ChannelMonitor { /// data in remote_claimable_outpoints. Will directly claim any HTLC outputs which expire at a /// height > height + CLTV_SHARED_CLAIM_BUFFER. In any case, will install monitoring for /// HTLC-Success/HTLC-Timeout transactions. - fn check_spend_remote_transaction(&mut self, tx: &Transaction, height: u32) -> (Vec, (Sha256dHash, Vec), Vec) { + /// Return updates for HTLC pending in the channel and failed automatically by the broadcast of + /// revoked remote commitment tx + fn check_spend_remote_transaction(&mut self, tx: &Transaction, height: u32) -> (Vec, (Sha256dHash, Vec), Vec, Vec<(HTLCSource, Option<[u8;32]>, [u8;32])>) { // Most secp and related errors trying to create keys means we have no hope of constructing // a spend transaction...so we return no transactions to broadcast let mut txn_to_broadcast = Vec::new(); let mut watch_outputs = Vec::new(); let mut spendable_outputs = Vec::new(); + let mut htlc_updated = Vec::new(); let commitment_txid = tx.txid(); //TODO: This is gonna be a performance bottleneck for watchtowers! let per_commitment_option = self.remote_claimable_outpoints.get(&commitment_txid); @@ -942,7 +1009,7 @@ impl ChannelMonitor { ( $thing : expr ) => { match $thing { Ok(a) => a, - Err(_) => return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs) + Err(_) => return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs, htlc_updated) } }; } @@ -967,7 +1034,7 @@ impl ChannelMonitor { }; let delayed_key = ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &self.their_delayed_payment_base_key.unwrap())); let a_htlc_key = match self.their_htlc_base_key { - None => return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs), + None => return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs, htlc_updated), Some(their_htlc_base_key) => ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, &PublicKey::from_secret_key(&self.secp_ctx, &per_commitment_key), &their_htlc_base_key)), }; @@ -1046,7 +1113,7 @@ impl ChannelMonitor { if htlc.transaction_output_index as usize >= tx.output.len() || tx.output[htlc.transaction_output_index as usize].value != htlc.amount_msat / 1000 || tx.output[htlc.transaction_output_index as usize].script_pubkey != expected_script.to_v0_p2wsh() { - return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs); // Corrupted per_commitment_data, fuck this user + return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs, htlc_updated); // Corrupted per_commitment_data, fuck this user } let input = TxIn { previous_output: BitcoinOutPoint { @@ -1084,7 +1151,7 @@ impl ChannelMonitor { watch_outputs.append(&mut tx.output.clone()); self.remote_commitment_txn_on_chain.insert(commitment_txid, (commitment_number, tx.output.iter().map(|output| { output.script_pubkey.clone() }).collect())); } - if inputs.is_empty() { return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs); } // Nothing to be done...probably a false positive/local tx + if inputs.is_empty() { return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs, htlc_updated); } // Nothing to be done...probably a false positive/local tx let outputs = vec!(TxOut { script_pubkey: self.destination_script.clone(), @@ -1139,7 +1206,7 @@ impl ChannelMonitor { }, }; let a_htlc_key = match self.their_htlc_base_key { - None => return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs), + None => return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs, htlc_updated), Some(their_htlc_base_key) => ignore_error!(chan_utils::derive_public_key(&self.secp_ctx, revocation_point, &their_htlc_base_key)), }; @@ -1193,7 +1260,7 @@ impl ChannelMonitor { if htlc.transaction_output_index as usize >= tx.output.len() || tx.output[htlc.transaction_output_index as usize].value != htlc.amount_msat / 1000 || tx.output[htlc.transaction_output_index as usize].script_pubkey != expected_script.to_v0_p2wsh() { - return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs); // Corrupted per_commitment_data, fuck this user + return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs, htlc_updated); // Corrupted per_commitment_data, fuck this user } if let Some(payment_preimage) = self.payment_preimages.get(&htlc.payment_hash) { let input = TxIn { @@ -1230,7 +1297,7 @@ impl ChannelMonitor { } } - if inputs.is_empty() { return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs); } // Nothing to be done...probably a false positive/local tx + if inputs.is_empty() { return (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs, htlc_updated); } // Nothing to be done...probably a false positive/local tx let outputs = vec!(TxOut { script_pubkey: self.destination_script.clone(), @@ -1260,7 +1327,7 @@ impl ChannelMonitor { } } - (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs) + (txn_to_broadcast, (commitment_txid, watch_outputs), spendable_outputs, htlc_updated) } /// Attempst to claim a remote HTLC-Success/HTLC-Timeout s outputs using the revocation key @@ -1500,9 +1567,10 @@ impl ChannelMonitor { } } - fn block_connected(&mut self, txn_matched: &[&Transaction], height: u32, block_hash: &Sha256dHash, broadcaster: &BroadcasterInterface)-> (Vec<(Sha256dHash, Vec)>, Vec) { + fn block_connected(&mut self, txn_matched: &[&Transaction], height: u32, block_hash: &Sha256dHash, broadcaster: &BroadcasterInterface)-> (Vec<(Sha256dHash, Vec)>, Vec, Vec<(HTLCSource, Option<[u8 ; 32]>, [u8; 32])>) { let mut watch_outputs = Vec::new(); let mut spendable_outputs = Vec::new(); + let mut htlc_updated = Vec::new(); for tx in txn_matched { if tx.input.len() == 1 { // Assuming our keys were not leaked (in which case we're screwed no matter what), @@ -1520,7 +1588,7 @@ impl ChannelMonitor { } }; if funding_txo.is_none() || (prevout.txid == funding_txo.as_ref().unwrap().0.txid && prevout.vout == funding_txo.as_ref().unwrap().0.index as u32) { - let (remote_txn, new_outputs, mut spendable_output) = self.check_spend_remote_transaction(tx, height); + let (remote_txn, new_outputs, mut spendable_output, mut updated) = self.check_spend_remote_transaction(tx, height); txn = remote_txn; spendable_outputs.append(&mut spendable_output); if !new_outputs.1.is_empty() { @@ -1539,6 +1607,9 @@ impl ChannelMonitor { spendable_outputs.push(spendable_output); } } + if updated.len() > 0 { + htlc_updated.append(&mut updated); + } } else { if let Some(&(commitment_number, _)) = self.remote_commitment_txn_on_chain.get(&prevout.txid) { let (tx, spendable_output) = self.check_spend_remote_htlc(tx, commitment_number); @@ -1553,6 +1624,10 @@ impl ChannelMonitor { for tx in txn.iter() { broadcaster.broadcast_transaction(tx); } + let mut updated = self.is_resolving_htlc_output(tx); + if updated.len() > 0 { + htlc_updated.append(&mut updated); + } } } if let Some(ref cur_local_tx) = self.current_local_signed_commitment_tx { @@ -1583,7 +1658,7 @@ impl ChannelMonitor { } } self.last_block_hash = block_hash.clone(); - (watch_outputs, spendable_outputs) + (watch_outputs, spendable_outputs, htlc_updated) } pub(super) fn would_broadcast_at_height(&self, height: u32) -> bool { @@ -1617,6 +1692,64 @@ impl ChannelMonitor { } false } + + /// Check if any transaction broadcasted is resolving HTLC output by a success or timeout on a local + /// or remote commitment tx, if so send back the source, preimage if found and payment_hash of resolved HTLC + fn is_resolving_htlc_output(&mut self, tx: &Transaction) -> Vec<(HTLCSource, Option<[u8;32]>, [u8;32])> { + let mut htlc_updated = Vec::new(); + + 'outer_loop: for input in &tx.input { + let mut payment_data = None; + + macro_rules! scan_commitment { + ($htlc_outputs: expr, $htlc_sources: expr) => { + for &(ref payment_hash, ref source, ref vout) in $htlc_sources.iter() { + if &Some(input.previous_output.vout) == vout { + payment_data = Some((source.clone(), *payment_hash)); + } + } + if payment_data.is_none() { + for htlc_output in $htlc_outputs { + if input.previous_output.vout == htlc_output.transaction_output_index { + log_info!(self, "Inbound HTLC timeout at {} from {} resolved by {}", input.previous_output.vout, input.previous_output.txid, tx.txid()); + continue 'outer_loop; + } + } + } + } + } + + if let Some(ref current_local_signed_commitment_tx) = self.current_local_signed_commitment_tx { + if input.previous_output.txid == current_local_signed_commitment_tx.txid { + scan_commitment!(current_local_signed_commitment_tx.htlc_outputs.iter().map(|&(ref a, _, _)| a), current_local_signed_commitment_tx.htlc_sources); + } + } + if let Some(ref prev_local_signed_commitment_tx) = self.prev_local_signed_commitment_tx { + if input.previous_output.txid == prev_local_signed_commitment_tx.txid { + scan_commitment!(prev_local_signed_commitment_tx.htlc_outputs.iter().map(|&(ref a, _, _)| a), prev_local_signed_commitment_tx.htlc_sources); + } + } + if let Some(&(ref htlc_outputs, ref htlc_sources)) = self.remote_claimable_outpoints.get(&input.previous_output.txid) { + scan_commitment!(htlc_outputs, htlc_sources); + } + + // If tx isn't solving htlc output from local/remote commitment tx and htlc isn't outbound we don't need + // to broadcast solving backward + if let Some((source, payment_hash)) = payment_data { + let mut payment_preimage = [0; 32]; + if input.witness.len() == 5 && input.witness[4].len() == 138 { + payment_preimage.copy_from_slice(&tx.input[0].witness[3]); + htlc_updated.push((source, Some(payment_preimage), payment_hash)); + } else if input.witness.len() == 3 && input.witness[2].len() == 133 { + payment_preimage.copy_from_slice(&tx.input[0].witness[1]); + htlc_updated.push((source, Some(payment_preimage), payment_hash)); + } else { + htlc_updated.push((source, None, payment_hash)); + } + } + } + htlc_updated + } } const MAX_ALLOC_SIZE: usize = 64*1024; diff --git a/src/util/test_utils.rs b/src/util/test_utils.rs index 33fb63b5..4cd4f440 100644 --- a/src/util/test_utils.rs +++ b/src/util/test_utils.rs @@ -4,6 +4,7 @@ use chain::transaction::OutPoint; use ln::channelmonitor; use ln::msgs; use ln::msgs::{HandleError}; +use ln::channelmonitor::HTLCUpdate; use util::events; use util::logger::{Logger, Level, Record}; use util::ser::{ReadableArgs, Writer}; @@ -64,6 +65,10 @@ impl channelmonitor::ManyChannelMonitor for TestChannelMonitor { assert!(self.simple_monitor.add_update_monitor(funding_txo, monitor).is_ok()); self.update_ret.lock().unwrap().clone() } + + fn fetch_pending_htlc_updated(&self) -> Vec { + return self.simple_monitor.fetch_pending_htlc_updated(); + } } pub struct TestBroadcaster { -- 2.30.2