From 5db24ebfb58a9f68e28081328e1ec65023f9b4d1 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 9 Jan 2020 14:09:25 -0500 Subject: [PATCH] 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. --- lightning/src/ln/channelmanager.rs | 36 +++++++++++++-- lightning/src/ln/functional_tests.rs | 69 ++++++++++++++++++++++++++++ lightning/src/util/events.rs | 3 ++ 3 files changed, 105 insertions(+), 3 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index ad0d3b189..c7d7e3085 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -76,6 +76,7 @@ enum PendingForwardReceiveHTLCInfo { }, Receive { payment_data: Option, + incoming_cltv_expiry: u32, // Used to track when we should expire pending HTLCs that go unclaimed }, } @@ -125,6 +126,7 @@ struct ClaimableHTLC { src: HTLCPreviousHopData, value: u64, payment_data: Option, + cltv_expiry: u32, } /// Tracks the inbound corresponding to an outbound HTLC @@ -1013,7 +1015,10 @@ impl ChannelManager where M::T // delay) once they've send us a commitment_signed! PendingHTLCStatus::Forward(PendingHTLCInfo { - type_data: PendingForwardReceiveHTLCInfo::Receive { payment_data }, + type_data: PendingForwardReceiveHTLCInfo::Receive { + payment_data, + incoming_cltv_expiry: msg.cltv_expiry, + }, payment_hash: msg.payment_hash.clone(), incoming_shared_secret: shared_secret, amt_to_forward: next_hop_data.amt_to_forward, @@ -1623,7 +1628,7 @@ impl ChannelManager where M::T for forward_info in pending_forwards.drain(..) { match forward_info { HTLCForwardInfo::AddHTLC { prev_short_channel_id, prev_htlc_id, forward_info: PendingHTLCInfo { - type_data: PendingForwardReceiveHTLCInfo::Receive { payment_data }, + type_data: PendingForwardReceiveHTLCInfo::Receive { payment_data, incoming_cltv_expiry }, incoming_shared_secret, payment_hash, amt_to_forward, .. }, } => { let prev_hop_data = HTLCPreviousHopData { short_channel_id: prev_short_channel_id, @@ -1639,6 +1644,7 @@ impl ChannelManager where M::T src: prev_hop_data, value: amt_to_forward, payment_data: payment_data.clone(), + cltv_expiry: incoming_cltv_expiry, }); if let &Some(ref data) = &payment_data { for htlc in htlcs.iter() { @@ -2791,6 +2797,7 @@ impl ChainListener for ChannelM log_trace!(self, "Block {} at height {} connected with {} txn matched", header_hash, height, txn_matched.len()); 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 = &mut *channel_lock; @@ -2857,10 +2864,29 @@ impl ChainListener for ChannelM } true }); + + channel_state.claimable_htlcs.retain(|&(ref payment_hash, _), htlcs| { + htlcs.retain(|htlc| { + if height >= htlc.cltv_expiry - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS { + timed_out_htlcs.push((HTLCSource::PreviousHopData(htlc.src.clone()), payment_hash.clone(), htlc.value)); + false + } else { true } + }); + !htlcs.is_empty() + }); } for failure in failed_channels.drain(..) { self.finish_force_close_channel(failure); } + + for (source, payment_hash, value) in timed_out_htlcs.drain(..) { + // Call it preimage_unknown as the issue, ultimately, is that the user failed to + // provide us a preimage within the cltv_expiry time window. + self.fail_htlc_backwards_internal(self.channel_state.lock().unwrap(), source, &payment_hash, HTLCFailReason::Reason { + failure_code: 0x4000 | 15, + data: byte_utils::be64_to_array(value).to_vec() + }); + } 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_hash; } @@ -3197,9 +3223,10 @@ impl Writeable for PendingHTLCInfo { onion_packet.write(writer)?; short_channel_id.write(writer)?; }, - &PendingForwardReceiveHTLCInfo::Receive { ref payment_data } => { + &PendingForwardReceiveHTLCInfo::Receive { ref payment_data, ref incoming_cltv_expiry } => { 1u8.write(writer)?; payment_data.write(writer)?; + incoming_cltv_expiry.write(writer)?; }, } self.incoming_shared_secret.write(writer)?; @@ -3220,6 +3247,7 @@ impl Readable for PendingHTLCInfo { }, 1u8 => PendingForwardReceiveHTLCInfo::Receive { payment_data: Readable::read(reader)?, + incoming_cltv_expiry: Readable::read(reader)?, }, _ => return Err(DecodeError::InvalidValue), }, @@ -3429,6 +3457,7 @@ impl Writeable for ChannelManager htlc.src.write(writer)?; htlc.value.write(writer)?; htlc.payment_data.write(writer)?; + htlc.cltv_expiry.write(writer)?; } } @@ -3574,6 +3603,7 @@ impl<'a, R : ::std::io::Read, ChanSigner: ChannelKeys + Readable, M: Deref> R src: Readable::read(reader)?, value: Readable::read(reader)?, payment_data: Readable::read(reader)?, + cltv_expiry: Readable::read(reader)?, }); } claimable_htlcs.insert(payment_hash, previous_hops); diff --git a/lightning/src/ln/functional_tests.rs b/lightning/src/ln/functional_tests.rs index a35070cb5..3d2214f2f 100644 --- a/lightning/src/ln/functional_tests.rs +++ b/lightning/src/ln/functional_tests.rs @@ -2152,6 +2152,15 @@ fn claim_htlc_outputs_single_tx() { let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; nodes[0].block_notifier.block_connected(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 200); nodes[1].block_notifier.block_connected(&Block { header, txdata: vec![revoked_local_txn[0].clone()] }, 200); + + // Expect pending failures, but we don't bother trying to update the channel state with them. + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + Event::PendingHTLCsForwardable { .. } => { }, + _ => panic!("Unexpected event"), + }; + connect_blocks(&nodes[1].block_notifier, ANTI_REORG_DELAY - 1, 200, true, header.bitcoin_hash()); let events = nodes[1].node.get_and_clear_pending_events(); @@ -3406,6 +3415,48 @@ fn test_drop_messages_peer_disconnect_dual_htlc() { claim_payment(&nodes[0], &[&nodes[1]], payment_preimage_2, 1_000_000); } +#[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 node_cfgs = create_node_cfgs(2); + let node_chanmgrs = create_node_chanmgrs(2, &node_cfgs, &[None, None]); + let nodes = create_network(2, &node_cfgs, &node_chanmgrs); + + create_announced_chan_between_nodes(&nodes, 0, 1, InitFeatures::supported(), InitFeatures::supported()); + 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].block_notifier.block_connected_checked(&header, 101, &[], &[]); + nodes[1].block_notifier.block_connected_checked(&header, 101, &[], &[]); + for i in 102..TEST_FINAL_CLTV + 100 + 1 - CLTV_CLAIM_BUFFER - LATENCY_GRACE_PERIOD_BLOCKS { + header.prev_blockhash = header.bitcoin_hash(); + nodes[0].block_notifier.block_connected_checked(&header, i, &[], &[]); + nodes[1].block_notifier.block_connected_checked(&header, i, &[], &[]); + } + + expect_pending_htlcs_forwardable!(nodes[1]); + + 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]); + 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, error_code } => { + assert_eq!(payment_hash, our_payment_hash); + assert!(rejected_by_dest); + assert_eq!(error_code.unwrap(), 0x4000 | 15); + }, + _ => 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 @@ -6663,6 +6714,15 @@ fn test_bump_penalty_txn_on_revoked_htlcs() { // Broadcast set of revoked txn on A let header_128 = connect_blocks(&nodes[0].block_notifier, 128, 0, true, header.bitcoin_hash()); + + // Expect pending failures, but we don't bother trying to update the channel state with them. + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + Event::PendingHTLCsForwardable { .. } => { }, + _ => panic!("Unexpected event"), + }; + let header_129 = BlockHeader { version: 0x20000000, prev_blockhash: header_128, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; nodes[0].block_notifier.block_connected(&Block { header: header_129, txdata: vec![revoked_local_txn[0].clone(), revoked_htlc_txn[0].clone(), revoked_htlc_txn[1].clone()] }, 129); let first; @@ -6994,6 +7054,15 @@ fn test_bump_txn_sanitize_tracking_maps() { // Broadcast set of revoked txn on A let header_128 = connect_blocks(&nodes[0].block_notifier, 128, 0, false, Default::default()); + + // Expect pending failures, but we don't bother trying to update the channel state with them. + let events = nodes[0].node.get_and_clear_pending_events(); + assert_eq!(events.len(), 1); + match events[0] { + Event::PendingHTLCsForwardable { .. } => { }, + _ => panic!("Unexpected event"), + }; + let header_129 = BlockHeader { version: 0x20000000, prev_blockhash: header_128, merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; nodes[0].block_notifier.block_connected(&Block { header: header_129, txdata: vec![revoked_local_txn[0].clone()] }, 129); check_closed_broadcast!(nodes[0], false); diff --git a/lightning/src/util/events.rs b/lightning/src/util/events.rs index 3d0b680ac..b3e85bad0 100644 --- a/lightning/src/util/events.rs +++ b/lightning/src/util/events.rs @@ -57,6 +57,9 @@ pub enum Event { /// ChannelManager::fail_htlc_backwards 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: PaymentHash, -- 2.39.5