Send back the actual received amount, not expected on HTLC fails

This resolves an incorrect implementation of the spec and fixes a
major privacy leak.

Fixes GH #289.

diff --git a/fuzz/fuzz_targets/chanmon_fail_consistency.rs b/fuzz/fuzz_targets/chanmon_fail_consistency.rs
index 4db158f28e173bd0be3c693f505b64ad54b8afb2..f457a30913b870e4005628697f2f665519108670 100644 (file)
--- a/fuzz/fuzz_targets/chanmon_fail_consistency.rs
+++ b/fuzz/fuzz_targets/chanmon_fail_consistency.rs
@@ -439,7 +439,7 @@ pub fn do_test(data: &[u8]) {
                                        match event {
                                                events::Event::PaymentReceived { payment_hash, .. } => {
                                                        if $fail {
-                                                               assert!(nodes[$node].fail_htlc_backwards(&payment_hash, 0));
+                                                               assert!(nodes[$node].fail_htlc_backwards(&payment_hash));
                                                        } else {
                                                                assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0)));
                                                        }

diff --git a/fuzz/fuzz_targets/full_stack_target.rs b/fuzz/fuzz_targets/full_stack_target.rs
index a4697fd939df6741fe4241636c5611e4ecee36c6..f0d21937750cde5bce0515a9980eca2510ecb1a9 100644 (file)
--- a/fuzz/fuzz_targets/full_stack_target.rs
+++ b/fuzz/fuzz_targets/full_stack_target.rs
@@ -451,7 +451,7 @@ pub fn do_test(data: &[u8], logger: &Arc<Logger>) {
                                        // fulfill this HTLC, but if they are, we can just take the first byte and
                                        // place that anywhere in our preimage.
                                        if &payment.0[1..] != &[0; 31] {
-                                               channelmanager.fail_htlc_backwards(&payment, 0);
+                                               channelmanager.fail_htlc_backwards(&payment);
                                        } else {
                                                let mut payment_preimage = PaymentPreimage([0; 32]);
                                                payment_preimage.0[0] = payment.0[0];
@@ -461,7 +461,7 @@ pub fn do_test(data: &[u8], logger: &Arc<Logger>) {
                        },
                        9 => {
                                for payment in payments_received.drain(..) {
-                                       channelmanager.fail_htlc_backwards(&payment, 0);
+                                       channelmanager.fail_htlc_backwards(&payment);
                                }
                        },
                        10 => {

diff --git a/src/ln/chanmon_update_fail_tests.rs b/src/ln/chanmon_update_fail_tests.rs
index fc481f8aac7bb8e5b3326e12e5f8d5dc83ddb895..69e056217f719b933aac1221b7d73d540a0ce53d 100644 (file)
--- a/src/ln/chanmon_update_fail_tests.rs
+++ b/src/ln/chanmon_update_fail_tests.rs
@@ -674,7 +674,7 @@ fn do_test_monitor_update_fail_raa(test_ignore_second_cs: bool) {
        let (_, payment_hash_1) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000);
 
        // Fail the payment backwards, failing the monitor update on nodes[1]'s receipt of the RAA
-       assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1, 0));
+       assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1));
        expect_pending_htlcs_forwardable!(nodes[2]);
        check_added_monitors!(nodes[2], 1);
 
@@ -1451,7 +1451,7 @@ fn test_monitor_update_on_pending_forwards() {
        send_payment(&nodes[0], &[&nodes[1], &nodes[2]], 5000000);
 
        let (_, payment_hash_1) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 1000000);
-       assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1, 1000000));
+       assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1));
        expect_pending_htlcs_forwardable!(nodes[2]);
        check_added_monitors!(nodes[2], 1);
 

diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs
index ae5842463b9c843531d4aca089eaf334d3459e61..6cee5d6941792a05d365ea8ee36b790e73d980f2 100644 (file)
--- a/src/ln/channelmanager.rs
+++ b/src/ln/channelmanager.rs
@@ -252,10 +252,12 @@ pub(super) struct ChannelHolder {
        /// guarantees are made about the existence of a channel with the short id here, nor the short
        /// ids in the PendingForwardHTLCInfo!
        pub(super) forward_htlcs: HashMap<u64, Vec<HTLCForwardInfo>>,
+       /// payment_hash -> Vec<(amount_received, htlc_source)> for tracking things that were to us and
+       /// can be failed/claimed by the user
        /// Note that while this is held in the same mutex as the channels themselves, no consistency
        /// guarantees are made about the channels given here actually existing anymore by the time you
        /// go to read them!
-       pub(super) claimable_htlcs: HashMap<PaymentHash, Vec<HTLCPreviousHopData>>,
+       pub(super) claimable_htlcs: HashMap<PaymentHash, Vec<(u64, HTLCPreviousHopData)>>,
        /// Messages to send to peers - pushed to in the same lock that they are generated in (except
        /// for broadcast messages, where ordering isn't as strict).
        pub(super) pending_msg_events: Vec<events::MessageSendEvent>,
@@ -265,7 +267,7 @@ pub(super) struct MutChannelHolder<'a> {
        pub(super) short_to_id: &'a mut HashMap<u64, [u8; 32]>,
        pub(super) next_forward: &'a mut Instant,
        pub(super) forward_htlcs: &'a mut HashMap<u64, Vec<HTLCForwardInfo>>,
-       pub(super) claimable_htlcs: &'a mut HashMap<PaymentHash, Vec<HTLCPreviousHopData>>,
+       pub(super) claimable_htlcs: &'a mut HashMap<PaymentHash, Vec<(u64, HTLCPreviousHopData)>>,
        pub(super) pending_msg_events: &'a mut Vec<events::MessageSendEvent>,
 }
 impl ChannelHolder {
@@ -1308,8 +1310,8 @@ impl ChannelManager {
                                                                        incoming_packet_shared_secret: forward_info.incoming_shared_secret,
                                                                };
                                                                match channel_state.claimable_htlcs.entry(forward_info.payment_hash) {
-                                                                       hash_map::Entry::Occupied(mut entry) => entry.get_mut().push(prev_hop_data),
-                                                                       hash_map::Entry::Vacant(entry) => { entry.insert(vec![prev_hop_data]); },
+                                                                       hash_map::Entry::Occupied(mut entry) => entry.get_mut().push((forward_info.amt_to_forward, prev_hop_data)),
+                                                                       hash_map::Entry::Vacant(entry) => { entry.insert(vec![(forward_info.amt_to_forward, prev_hop_data)]); },
                                                                };
                                                                new_events.push(events::Event::PaymentReceived {
                                                                        payment_hash: forward_info.payment_hash,
@@ -1354,20 +1356,21 @@ impl ChannelManager {
        }
 
        /// Indicates that the preimage for payment_hash is unknown or the received amount is incorrect
-       /// after a PaymentReceived event.
-       /// expected_value is the value you expected the payment to be for (not the amount it actually
-       /// was for from the PaymentReceived event).
-       pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash, expected_value: u64) -> bool {
+       /// after a PaymentReceived event, failing the HTLC back to its origin and freeing resources
+       /// along the path (including in our own channel on which we received it).
+       /// Returns false if no payment was found to fail backwards, true if the process of failing the
+       /// HTLC backwards has been started.
+       pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash) -> bool {
                let _ = self.total_consistency_lock.read().unwrap();
 
                let mut channel_state = Some(self.channel_state.lock().unwrap());
                let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(payment_hash);
                if let Some(mut sources) = removed_source {
-                       for htlc_with_hash in sources.drain(..) {
+                       for (recvd_value, htlc_with_hash) in sources.drain(..) {
                                if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
                                self.fail_htlc_backwards_internal(channel_state.take().unwrap(),
                                                HTLCSource::PreviousHopData(htlc_with_hash), payment_hash,
-                                               HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: byte_utils::be64_to_array(expected_value).to_vec() });
+                                               HTLCFailReason::Reason { failure_code: 0x4000 | 15, data: byte_utils::be64_to_array(recvd_value).to_vec() });
                        }
                        true
                } else { false }
@@ -1485,7 +1488,10 @@ impl ChannelManager {
                let mut channel_state = Some(self.channel_state.lock().unwrap());
                let removed_source = channel_state.as_mut().unwrap().claimable_htlcs.remove(&payment_hash);
                if let Some(mut sources) = removed_source {
-                       for htlc_with_hash in sources.drain(..) {
+                       // TODO: We should require the user specify the expected amount so that we can claim
+                       // only payments for the correct amount, and reject payments for incorrect amounts
+                       // (which are probably middle nodes probing to break our privacy).
+                       for (_, htlc_with_hash) in sources.drain(..) {
                                if channel_state.is_none() { channel_state = Some(self.channel_state.lock().unwrap()); }
                                self.claim_funds_internal(channel_state.take().unwrap(), HTLCSource::PreviousHopData(htlc_with_hash), payment_preimage);
                        }
@@ -2910,7 +2916,8 @@ impl Writeable for ChannelManager {
                for (payment_hash, previous_hops) in channel_state.claimable_htlcs.iter() {
                        payment_hash.write(writer)?;
                        (previous_hops.len() as u64).write(writer)?;
-                       for previous_hop in previous_hops {
+                       for &(recvd_amt, ref previous_hop) in previous_hops.iter() {
+                               recvd_amt.write(writer)?;
                                previous_hop.write(writer)?;
                        }
                }
@@ -3047,7 +3054,7 @@ impl<'a, R : ::std::io::Read> ReadableArgs<R, ChannelManagerReadArgs<'a>> for (S
                        let previous_hops_len: u64 = Readable::read(reader)?;
                        let mut previous_hops = Vec::with_capacity(cmp::min(previous_hops_len as usize, 2));
                        for _ in 0..previous_hops_len {
-                               previous_hops.push(Readable::read(reader)?);
+                               previous_hops.push((Readable::read(reader)?, Readable::read(reader)?));
                        }
                        claimable_htlcs.insert(payment_hash, previous_hops);
                }

diff --git a/src/ln/functional_test_utils.rs b/src/ln/functional_test_utils.rs
index 4ce090a2881ac718fc942a68aa8602dcbd1e1bf6..0ca9f29a89bf0e8cc24a682751ea38068fcdc4b6 100644 (file)
--- a/src/ln/functional_test_utils.rs
+++ b/src/ln/functional_test_utils.rs
@@ -713,7 +713,7 @@ pub fn send_payment(origin: &Node, expected_route: &[&Node], recv_value: u64) {
 }
 
 pub fn fail_payment_along_route(origin_node: &Node, expected_route: &[&Node], skip_last: bool, our_payment_hash: PaymentHash) {
-       assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash, 0));
+       assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash));
        expect_pending_htlcs_forwardable!(expected_route.last().unwrap());
        check_added_monitors!(expected_route.last().unwrap(), 1);
 

diff --git a/src/ln/functional_tests.rs b/src/ln/functional_tests.rs
index f9a8ac65a6e265b83650196dc116fbd26617b9f4..fffdb7fb887ee779d71c07f0170905b5873306e9 100644 (file)
--- a/src/ln/functional_tests.rs
+++ b/src/ln/functional_tests.rs
@@ -1912,7 +1912,7 @@ fn test_htlc_on_chain_timeout() {
        // Broadcast legit commitment tx from C on B's chain
        let commitment_tx = nodes[2].node.channel_state.lock().unwrap().by_id.get(&chan_2.2).unwrap().last_local_commitment_txn.clone();
        check_spends!(commitment_tx[0], chan_2.3.clone());
-       nodes[2].node.fail_htlc_backwards(&payment_hash, 0);
+       nodes[2].node.fail_htlc_backwards(&payment_hash);
        check_added_monitors!(nodes[2], 0);
        expect_pending_htlcs_forwardable!(nodes[2]);
        check_added_monitors!(nodes[2], 1);
@@ -2093,7 +2093,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
        let (_, second_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value);
        let (_, third_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], value);
 
-       assert!(nodes[2].node.fail_htlc_backwards(&first_payment_hash, 0));
+       assert!(nodes[2].node.fail_htlc_backwards(&first_payment_hash));
        expect_pending_htlcs_forwardable!(nodes[2]);
        check_added_monitors!(nodes[2], 1);
        let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
@@ -2106,7 +2106,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
        let bs_raa = commitment_signed_dance!(nodes[1], nodes[2], updates.commitment_signed, false, true, false, true);
        // Drop the last RAA from 3 -> 2
 
-       assert!(nodes[2].node.fail_htlc_backwards(&second_payment_hash, 0));
+       assert!(nodes[2].node.fail_htlc_backwards(&second_payment_hash));
        expect_pending_htlcs_forwardable!(nodes[2]);
        check_added_monitors!(nodes[2], 1);
        let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
@@ -2123,7 +2123,7 @@ fn do_test_commitment_revoked_fail_backward_exhaustive(deliver_bs_raa: bool, use
        nodes[2].node.handle_revoke_and_ack(&nodes[1].node.get_our_node_id(), &as_raa).unwrap();
        check_added_monitors!(nodes[2], 1);
 
-       assert!(nodes[2].node.fail_htlc_backwards(&third_payment_hash, 0));
+       assert!(nodes[2].node.fail_htlc_backwards(&third_payment_hash));
        expect_pending_htlcs_forwardable!(nodes[2]);
        check_added_monitors!(nodes[2], 1);
        let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id());
@@ -3769,10 +3769,10 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
 
        // Now fail back three of the over-dust-limit and three of the under-dust-limit payments in one go.
        // Fail 0th below-dust, 4th above-dust, 8th above-dust, 10th below-dust HTLCs
-       assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_1, ds_dust_limit*1000));
-       assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_3, 1000000));
-       assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_5, 1000000));
-       assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_6, ds_dust_limit*1000));
+       assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_1));
+       assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_3));
+       assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_5));
+       assert!(nodes[4].node.fail_htlc_backwards(&payment_hash_6));
        check_added_monitors!(nodes[4], 0);
        expect_pending_htlcs_forwardable!(nodes[4]);
        check_added_monitors!(nodes[4], 1);
@@ -3785,8 +3785,8 @@ fn do_test_fail_backwards_unrevoked_remote_announce(deliver_last_raa: bool, anno
        commitment_signed_dance!(nodes[3], nodes[4], four_removes.commitment_signed, false);
 
        // Fail 3rd below-dust and 7th above-dust HTLCs
-       assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_2, ds_dust_limit*1000));
-       assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_4, 1000000));
+       assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_2));
+       assert!(nodes[5].node.fail_htlc_backwards(&payment_hash_4));
        check_added_monitors!(nodes[5], 0);
        expect_pending_htlcs_forwardable!(nodes[5]);
        check_added_monitors!(nodes[5], 1);
@@ -4079,7 +4079,7 @@ fn do_htlc_claim_previous_remote_commitment_only(use_dust: bool, check_revoke_no
        // actually revoked.
        let htlc_value = if use_dust { 50000 } else { 3000000 };
        let (_, our_payment_hash) = route_payment(&nodes[0], &[&nodes[1]], htlc_value);
-       assert!(nodes[1].node.fail_htlc_backwards(&our_payment_hash, htlc_value));
+       assert!(nodes[1].node.fail_htlc_backwards(&our_payment_hash));
        expect_pending_htlcs_forwardable!(nodes[1]);
        check_added_monitors!(nodes[1], 1);
 
@@ -4405,7 +4405,7 @@ fn test_onion_failure() {
                let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap();
                msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], NODE|2, &[0;0]);
        }, ||{
-               nodes[2].node.fail_htlc_backwards(&payment_hash, 0);
+               nodes[2].node.fail_htlc_backwards(&payment_hash);
        }, true, Some(NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[1].pubkey, is_permanent: false}));
 
        // intermediate node failure
@@ -4423,7 +4423,7 @@ fn test_onion_failure() {
                let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap();
                msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], PERM|NODE|2, &[0;0]);
        }, ||{
-               nodes[2].node.fail_htlc_backwards(&payment_hash, 0);
+               nodes[2].node.fail_htlc_backwards(&payment_hash);
        }, false, Some(PERM|NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[1].pubkey, is_permanent: true}));
 
        // intermediate node failure
@@ -4434,7 +4434,7 @@ fn test_onion_failure() {
                let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap();
                msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], PERM|NODE|3, &[0;0]);
        }, ||{
-               nodes[2].node.fail_htlc_backwards(&payment_hash, 0);
+               nodes[2].node.fail_htlc_backwards(&payment_hash);
        }, true, Some(PERM|NODE|3), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[0].pubkey, is_permanent: true}));
 
        // final node failure
@@ -4443,7 +4443,7 @@ fn test_onion_failure() {
                let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap();
                msg.reason = onion_utils::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], PERM|NODE|3, &[0;0]);
        }, ||{
-               nodes[2].node.fail_htlc_backwards(&payment_hash, 0);
+               nodes[2].node.fail_htlc_backwards(&payment_hash);
        }, false, Some(PERM|NODE|3), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[1].pubkey, is_permanent: true}));
 
        run_onion_failure_test("invalid_onion_version", 0, &nodes, &route, &payment_hash, |msg| { msg.onion_routing_packet.version = 1; }, ||{}, true,
@@ -4510,7 +4510,7 @@ fn test_onion_failure() {
        }, ||{}, true, Some(UPDATE|14), Some(msgs::HTLCFailChannelUpdate::ChannelUpdateMessage{msg: ChannelUpdate::dummy()}));
 
        run_onion_failure_test("unknown_payment_hash", 2, &nodes, &route, &payment_hash, |_| {}, || {
-               nodes[2].node.fail_htlc_backwards(&payment_hash, 0);
+               nodes[2].node.fail_htlc_backwards(&payment_hash);
        }, false, Some(PERM|15), None);
 
        run_onion_failure_test("final_expiry_too_soon", 1, &nodes, &route, &payment_hash, |msg| {