From: Matt Corallo Date: Sun, 4 Nov 2018 21:04:07 +0000 (+1100) Subject: Time out incoming HTLCs when we reach cltv_expiry (+ test) X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=refs%2Fheads%2F2018-11-fail-inbound;p=rust-lightning Time out incoming HTLCs when we reach cltv_expiry (+ test) We only do this for incoming HTLCs directly as we rely on channel closure and HTLC-Timeout broadcast to fail any HTLCs which we relayed onwards where our next-hop doesn't update_fail in time. --- diff --git a/src/ln/channel.rs b/src/ln/channel.rs index b9a81f6eb..10fa525d0 100644 --- a/src/ln/channel.rs +++ b/src/ln/channel.rs @@ -264,7 +264,7 @@ pub(super) struct Channel { monitor_pending_revoke_and_ack: bool, monitor_pending_commitment_signed: bool, monitor_pending_order: Option, - monitor_pending_forwards: Vec<(PendingForwardHTLCInfo, u64)>, + monitor_pending_forwards: Vec<(PendingForwardHTLCInfo, u64, u32)>, monitor_pending_failures: Vec<(HTLCSource, [u8; 32], HTLCFailReason)>, // pending_update_fee is filled when sending and receiving update_fee @@ -1854,7 +1854,7 @@ impl Channel { /// waiting on this revoke_and_ack. The generation of this new commitment_signed may also fail, /// generating an appropriate error *after* the channel state has been updated based on the /// revoke_and_ack message. - pub fn revoke_and_ack(&mut self, msg: &msgs::RevokeAndACK, fee_estimator: &FeeEstimator) -> Result<(Option, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, [u8; 32], HTLCFailReason)>, Option, ChannelMonitor), HandleError> { + pub fn revoke_and_ack(&mut self, msg: &msgs::RevokeAndACK, fee_estimator: &FeeEstimator) -> Result<(Option, Vec<(PendingForwardHTLCInfo, u64, u32)>, Vec<(HTLCSource, [u8; 32], HTLCFailReason)>, Option, ChannelMonitor), HandleError> { if (self.channel_state & (ChannelState::ChannelFunded as u32)) != (ChannelState::ChannelFunded as u32) { return Err(HandleError{err: "Got revoke/ACK message when channel was not in an operational state", action: None}); } @@ -1942,7 +1942,7 @@ impl Channel { } }, PendingHTLCStatus::Forward(forward_info) => { - to_forward_infos.push((forward_info, htlc.htlc_id)); + to_forward_infos.push((forward_info, htlc.htlc_id, htlc.cltv_expiry)); htlc.state = InboundHTLCState::Committed; } } @@ -2152,7 +2152,7 @@ impl Channel { /// Indicates that the latest ChannelMonitor update has been committed by the client /// successfully and we should restore normal operation. Returns messages which should be sent /// to the remote side. - pub fn monitor_updating_restored(&mut self) -> (Option, Option, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64)>, Vec<(HTLCSource, [u8; 32], HTLCFailReason)>) { + pub fn monitor_updating_restored(&mut self) -> (Option, Option, RAACommitmentOrder, Vec<(PendingForwardHTLCInfo, u64, u32)>, Vec<(HTLCSource, [u8; 32], HTLCFailReason)>) { assert_eq!(self.channel_state & ChannelState::MonitorUpdateFailed as u32, ChannelState::MonitorUpdateFailed as u32); self.channel_state &= !(ChannelState::MonitorUpdateFailed as u32); @@ -3489,9 +3489,10 @@ impl Writeable for Channel { } (self.monitor_pending_forwards.len() as u64).write(writer)?; - for &(ref pending_forward, ref htlc_id) in self.monitor_pending_forwards.iter() { + for &(ref pending_forward, ref htlc_id, ref cltv_expiry) in self.monitor_pending_forwards.iter() { pending_forward.write(writer)?; htlc_id.write(writer)?; + cltv_expiry.write(writer)?; } (self.monitor_pending_failures.len() as u64).write(writer)?; @@ -3670,7 +3671,7 @@ impl ReadableArgs> for Channel { let monitor_pending_forwards_count: u64 = Readable::read(reader)?; let mut monitor_pending_forwards = Vec::with_capacity(cmp::min(monitor_pending_forwards_count as usize, OUR_MAX_HTLCS as usize)); for _ in 0..monitor_pending_forwards_count { - monitor_pending_forwards.push((Readable::read(reader)?, Readable::read(reader)?)); + monitor_pending_forwards.push((Readable::read(reader)?, Readable::read(reader)?, Readable::read(reader)?)); } let monitor_pending_failures_count: u64 = Readable::read(reader)?; diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index a650c89bb..058780dbb 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -96,6 +96,8 @@ mod channel_held_info { pub(super) short_channel_id: u64, pub(super) htlc_id: u64, pub(super) incoming_packet_shared_secret: [u8; 32], + /// Only used to track expiry of claimable_htlcs and fail them backwards + pub(super) cltv_expiry: u32, } /// Tracks the inbound corresponding to an outbound HTLC @@ -240,6 +242,7 @@ const MIN_HTLC_RELAY_HOLDING_CELL_MILLIS: u32 = 50; struct HTLCForwardInfo { prev_short_channel_id: u64, prev_htlc_id: u64, + prev_cltv_expiry: u32, forward_info: PendingForwardHTLCInfo, } @@ -1326,11 +1329,12 @@ impl ChannelManager { Some(chan_id) => chan_id.clone(), None => { failed_forwards.reserve(pending_forwards.len()); - for HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, forward_info } in pending_forwards.drain(..) { + for HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, forward_info, prev_cltv_expiry } in pending_forwards.drain(..) { let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id: prev_short_channel_id, htlc_id: prev_htlc_id, incoming_packet_shared_secret: forward_info.incoming_shared_secret, + cltv_expiry: prev_cltv_expiry, }); failed_forwards.push((htlc_source, forward_info.payment_hash, 0x4000 | 10, None)); } @@ -1340,11 +1344,12 @@ impl ChannelManager { let forward_chan = &mut channel_state.by_id.get_mut(&forward_chan_id).unwrap(); let mut add_htlc_msgs = Vec::new(); - for HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, forward_info } in pending_forwards.drain(..) { + for HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, prev_cltv_expiry, forward_info } in pending_forwards.drain(..) { let htlc_source = HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id: prev_short_channel_id, htlc_id: prev_htlc_id, incoming_packet_shared_secret: forward_info.incoming_shared_secret, + cltv_expiry: prev_cltv_expiry, }); match forward_chan.send_htlc(forward_info.amt_to_forward, forward_info.payment_hash, forward_info.outgoing_cltv_value, htlc_source.clone(), forward_info.onion_packet.unwrap()) { Err(_e) => { @@ -1398,11 +1403,12 @@ impl ChannelManager { }); } } else { - for HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, forward_info } in pending_forwards.drain(..) { + for HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, prev_cltv_expiry, forward_info } in pending_forwards.drain(..) { let prev_hop_data = HTLCPreviousHopData { short_channel_id: prev_short_channel_id, htlc_id: prev_htlc_id, incoming_packet_shared_secret: forward_info.incoming_shared_secret, + cltv_expiry: prev_cltv_expiry, }; match channel_state.claimable_htlcs.entry(forward_info.payment_hash) { hash_map::Entry::Occupied(mut entry) => entry.get_mut().push(prev_hop_data), @@ -1471,7 +1477,7 @@ impl ChannelManager { panic!("should have onion error packet here"); } }, - HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret }) => { + HTLCSource::PreviousHopData(HTLCPreviousHopData { short_channel_id, htlc_id, incoming_packet_shared_secret, .. }) => { let err_packet = match onion_error { HTLCFailReason::Reason { failure_code, data } => { let packet = ChannelManager::build_failure_packet(&incoming_packet_shared_secret, failure_code, &data[..]).encode(); @@ -2248,7 +2254,7 @@ impl ChannelManager { } #[inline] - fn forward_htlcs(&self, per_source_pending_forwards: &mut [(u64, Vec<(PendingForwardHTLCInfo, u64)>)]) { + fn forward_htlcs(&self, per_source_pending_forwards: &mut [(u64, Vec<(PendingForwardHTLCInfo, u64, u32)>)]) { for &mut (prev_short_channel_id, ref mut pending_forwards) in per_source_pending_forwards { let mut forward_event = None; if !pending_forwards.is_empty() { @@ -2257,13 +2263,13 @@ impl ChannelManager { forward_event = Some(Instant::now() + Duration::from_millis(((rng::rand_f32() * 4.0 + 1.0) * MIN_HTLC_RELAY_HOLDING_CELL_MILLIS as f32) as u64)); channel_state.next_forward = forward_event.unwrap(); } - for (forward_info, prev_htlc_id) in pending_forwards.drain(..) { + for (forward_info, prev_htlc_id, prev_cltv_expiry) in pending_forwards.drain(..) { match channel_state.forward_htlcs.entry(forward_info.short_channel_id) { hash_map::Entry::Occupied(mut entry) => { - entry.get_mut().push(HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, forward_info }); + entry.get_mut().push(HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, prev_cltv_expiry, forward_info }); }, hash_map::Entry::Vacant(entry) => { - entry.insert(vec!(HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, forward_info })); + entry.insert(vec!(HTLCForwardInfo { prev_short_channel_id, prev_htlc_id, prev_cltv_expiry, forward_info })); } } } @@ -2500,6 +2506,7 @@ impl ChainListener for ChannelManager { fn block_connected(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[u32]) { let _ = self.total_consistency_lock.read().unwrap(); let mut failed_channels = Vec::new(); + let mut timed_out_htlcs = Vec::new(); { let mut channel_lock = self.channel_state.lock().unwrap(); let channel_state = channel_lock.borrow_parts(); @@ -2567,10 +2574,27 @@ impl ChainListener for ChannelManager { } true }); + channel_state.claimable_htlcs.retain(|payment_hash, htlcs| { + htlcs.retain(|htlc| { + if htlc.cltv_expiry <= height { // XXX: Or Readable for PendingHTLCStatus { impl_writeable!(HTLCPreviousHopData, 0, { short_channel_id, htlc_id, - incoming_packet_shared_secret + incoming_packet_shared_secret, + cltv_expiry }); impl Writeable for HTLCSource { @@ -2984,6 +3009,7 @@ impl Readable for HTLCFailReason { impl_writeable!(HTLCForwardInfo, 0, { prev_short_channel_id, prev_htlc_id, + prev_cltv_expiry, forward_info }); @@ -7263,6 +7289,44 @@ mod tests { do_test_monitor_temporary_update_fail(3 | 8 | 16); } + #[test] + fn test_htlc_timeout() { + // If the user fails to claim/fail an HTLC within the HTLC CLTV timeout we fail it for them + // to avoid our counterparty failing the channel. + let secp_ctx = Secp256k1::new(); + let nodes = create_network(2); + + create_announced_chan_between_nodes(&nodes, 0, 1); + let (_, our_payment_hash) = route_payment(&nodes[0], &[&nodes[1]], 100000); + + let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; + nodes[0].chain_monitor.block_connected_checked(&header, 101, &[], &[]); + nodes[1].chain_monitor.block_connected_checked(&header, 101, &[], &[]); + for i in 102..TEST_FINAL_CLTV + 100 + 1 { + header.prev_blockhash = header.bitcoin_hash(); + nodes[0].chain_monitor.block_connected_checked(&header, i, &[], &[]); + nodes[1].chain_monitor.block_connected_checked(&header, i, &[], &[]); + } + + check_added_monitors!(nodes[1], 1); + let htlc_timeout_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id()); + assert!(htlc_timeout_updates.update_add_htlcs.is_empty()); + assert_eq!(htlc_timeout_updates.update_fail_htlcs.len(), 1); + assert!(htlc_timeout_updates.update_fail_malformed_htlcs.is_empty()); + assert!(htlc_timeout_updates.update_fee.is_none()); + + nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &htlc_timeout_updates.update_fail_htlcs[0]).unwrap(); + commitment_signed_dance!(nodes[0], nodes[1], htlc_timeout_updates.commitment_signed, false); + let events = nodes[0].node.get_and_clear_pending_events(); + match events[0] { + Event::PaymentFailed { payment_hash, rejected_by_dest } => { + assert_eq!(payment_hash, our_payment_hash); + assert!(rejected_by_dest); + }, + _ => panic!("Unexpected event"), + } + } + #[test] fn test_invalid_channel_announcement() { //Test BOLT 7 channel_announcement msg requirement for final node, gather data to build customed channel_announcement msgs diff --git a/src/util/events.rs b/src/util/events.rs index a11321794..42d6cdcc5 100644 --- a/src/util/events.rs +++ b/src/util/events.rs @@ -57,6 +57,9 @@ pub enum Event { /// PaymentFailReason::AmountMismatch, respectively, to free up resources for this HTLC. /// The amount paid should be considered 'incorrect' when it is less than or more than twice /// the amount expected. + /// If you fail to call either ChannelManager::claim_funds of + /// ChannelManager::fail_htlc_backwards within the HTLC's timeout, the HTLC will be + /// automatically failed with PaymentFailReason::PreimageUnknown. PaymentReceived { /// The hash for which the preimage should be handed to the ChannelManager. payment_hash: [u8; 32],