From: Matt Corallo Date: Tue, 18 Dec 2018 03:43:05 +0000 (-0500) Subject: Update incorrect_payment_amount generation/handling for BOLT uptd X-Git-Tag: v0.0.12~254^2 X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=refs%2Fheads%2F2018-12-243-review;p=rust-lightning Update incorrect_payment_amount generation/handling for BOLT uptd ie dont generate them as they're a really obvious privacy leak. Luckily we were already handling them the same aside from log printing so don't have to touch anything there. I was lazy in updating tests but it only effects log printing, so whatever. --- diff --git a/fuzz/fuzz_targets/full_stack_target.rs b/fuzz/fuzz_targets/full_stack_target.rs index 406fa1e3e..3e105038c 100644 --- a/fuzz/fuzz_targets/full_stack_target.rs +++ b/fuzz/fuzz_targets/full_stack_target.rs @@ -17,7 +17,7 @@ use lightning::chain::chaininterface::{BroadcasterInterface,ConfirmationTarget,C use lightning::chain::transaction::OutPoint; use lightning::chain::keysinterface::{ChannelKeys, KeysInterface}; use lightning::ln::channelmonitor; -use lightning::ln::channelmanager::{ChannelManager, PaymentFailReason, PaymentHash, PaymentPreimage}; +use lightning::ln::channelmanager::{ChannelManager, PaymentHash, PaymentPreimage}; use lightning::ln::peer_handler::{MessageHandler,PeerManager,SocketDescriptor}; use lightning::ln::router::Router; use lightning::util::events::{EventsProvider,Event}; @@ -419,7 +419,7 @@ pub fn do_test(data: &[u8], logger: &Arc) { // 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, PaymentFailReason::PreimageUnknown); + channelmanager.fail_htlc_backwards(&payment, 0); } else { let mut payment_preimage = PaymentPreimage([0; 32]); payment_preimage.0[0] = payment.0[0]; @@ -429,7 +429,7 @@ pub fn do_test(data: &[u8], logger: &Arc) { }, 9 => { for payment in payments_received.drain(..) { - channelmanager.fail_htlc_backwards(&payment, PaymentFailReason::PreimageUnknown); + channelmanager.fail_htlc_backwards(&payment, 0); } }, 10 => { diff --git a/src/ln/channelmanager.rs b/src/ln/channelmanager.rs index 8b45fde93..2a96344e7 100644 --- a/src/ln/channelmanager.rs +++ b/src/ln/channelmanager.rs @@ -212,16 +212,6 @@ impl MsgHandleErrInternal { } } -/// Pass to fail_htlc_backwwards to indicate the reason to fail the payment -/// after a PaymentReceived event. -#[derive(PartialEq)] -pub enum PaymentFailReason { - /// Indicate the preimage for payment_hash is not known after a PaymentReceived event - PreimageUnknown, - /// Indicate the payment amount is incorrect ( received is < expected or > 2*expected ) after a PaymentReceived event - AmountMismatch, -} - /// We hold back HTLCs we intend to relay for a random interval in the range (this, 5*this). This /// provides some limited amount of privacy. Ideally this would range from somewhere like 1 second /// to 30 seconds, but people expect lightning to be, you know, kinda fast, sadly. We could @@ -1530,8 +1520,11 @@ impl ChannelManager { events.append(&mut new_events); } - /// Indicates that the preimage for payment_hash is unknown or the received amount is incorrect after a PaymentReceived event. - pub fn fail_htlc_backwards(&self, payment_hash: &PaymentHash, reason: PaymentFailReason) -> bool { + /// 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 { let _ = self.total_consistency_lock.read().unwrap(); let mut channel_state = Some(self.channel_state.lock().unwrap()); @@ -1539,7 +1532,9 @@ impl ChannelManager { if let Some(mut sources) = removed_source { for 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: if reason == PaymentFailReason::PreimageUnknown {0x4000 | 15} else {0x4000 | 16}, data: Vec::new() }); + 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() }); } true } else { false } @@ -3363,7 +3358,7 @@ mod tests { use chain::keysinterface::{KeysInterface, SpendableOutputDescriptor}; use chain::keysinterface; use ln::channel::{COMMITMENT_TX_BASE_WEIGHT, COMMITMENT_TX_WEIGHT_PER_HTLC}; - use ln::channelmanager::{ChannelManager,ChannelManagerReadArgs,OnionKeys,PaymentFailReason,RAACommitmentOrder, PaymentPreimage, PaymentHash}; + use ln::channelmanager::{ChannelManager,ChannelManagerReadArgs,OnionKeys,RAACommitmentOrder, PaymentPreimage, PaymentHash}; use ln::channelmonitor::{ChannelMonitor, ChannelMonitorUpdateErr, CLTV_CLAIM_BUFFER, HTLC_FAIL_TIMEOUT_BLOCKS, ManyChannelMonitor}; use ln::channel::{ACCEPTED_HTLC_SCRIPT_WEIGHT, OFFERED_HTLC_SCRIPT_WEIGHT}; use ln::router::{Route, RouteHop, Router}; @@ -4221,7 +4216,7 @@ mod tests { } 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, PaymentFailReason::PreimageUnknown)); + assert!(expected_route.last().unwrap().node.fail_htlc_backwards(&our_payment_hash, 0)); check_added_monitors!(expected_route.last().unwrap(), 1); let mut next_msgs: Option<(msgs::UpdateFailHTLC, msgs::CommitmentSigned)> = None; @@ -6348,7 +6343,7 @@ mod tests { // Brodacast 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, PaymentFailReason::PreimageUnknown); + nodes[2].node.fail_htlc_backwards(&payment_hash, 0); { let mut added_monitors = nodes[2].chan_monitor.added_monitors.lock().unwrap(); assert_eq!(added_monitors.len(), 1); @@ -6533,7 +6528,7 @@ mod tests { let (_, second_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 3000000); let (_, third_payment_hash) = route_payment(&nodes[0], &[&nodes[1], &nodes[2]], 3000000); - assert!(nodes[2].node.fail_htlc_backwards(&first_payment_hash, PaymentFailReason::PreimageUnknown)); + assert!(nodes[2].node.fail_htlc_backwards(&first_payment_hash, 0)); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); assert!(updates.update_add_htlcs.is_empty()); @@ -6545,7 +6540,7 @@ mod tests { 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, PaymentFailReason::PreimageUnknown)); + assert!(nodes[2].node.fail_htlc_backwards(&second_payment_hash, 0)); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); assert!(updates.update_add_htlcs.is_empty()); @@ -6561,7 +6556,7 @@ mod tests { 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, PaymentFailReason::PreimageUnknown)); + assert!(nodes[2].node.fail_htlc_backwards(&third_payment_hash, 0)); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); assert!(updates.update_add_htlcs.is_empty()); @@ -8156,7 +8151,7 @@ mod tests { 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, PaymentFailReason::PreimageUnknown)); + assert!(nodes[2].node.fail_htlc_backwards(&payment_hash_1, 0)); check_added_monitors!(nodes[2], 1); let updates = get_htlc_update_msgs!(nodes[2], nodes[1].node.get_our_node_id()); @@ -9721,7 +9716,7 @@ mod tests { let onion_keys = ChannelManager::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); msg.reason = ChannelManager::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], NODE|2, &[0;0]); }, ||{ - nodes[2].node.fail_htlc_backwards(&payment_hash, PaymentFailReason::PreimageUnknown); + nodes[2].node.fail_htlc_backwards(&payment_hash, 0); }, true, Some(NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[1].pubkey, is_permanent: false})); // intermediate node failure @@ -9739,7 +9734,7 @@ mod tests { let onion_keys = ChannelManager::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); msg.reason = ChannelManager::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], PERM|NODE|2, &[0;0]); }, ||{ - nodes[2].node.fail_htlc_backwards(&payment_hash, PaymentFailReason::PreimageUnknown); + nodes[2].node.fail_htlc_backwards(&payment_hash, 0); }, false, Some(PERM|NODE|2), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[1].pubkey, is_permanent: true})); // intermediate node failure @@ -9750,7 +9745,7 @@ mod tests { let onion_keys = ChannelManager::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); msg.reason = ChannelManager::build_first_hop_failure_packet(&onion_keys[0].shared_secret[..], PERM|NODE|3, &[0;0]); }, ||{ - nodes[2].node.fail_htlc_backwards(&payment_hash, PaymentFailReason::PreimageUnknown); + nodes[2].node.fail_htlc_backwards(&payment_hash, 0); }, true, Some(PERM|NODE|3), Some(msgs::HTLCFailChannelUpdate::NodeFailure{node_id: route.hops[0].pubkey, is_permanent: true})); // final node failure @@ -9759,7 +9754,7 @@ mod tests { let onion_keys = ChannelManager::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap(); msg.reason = ChannelManager::build_first_hop_failure_packet(&onion_keys[1].shared_secret[..], PERM|NODE|3, &[0;0]); }, ||{ - nodes[2].node.fail_htlc_backwards(&payment_hash, PaymentFailReason::PreimageUnknown); + nodes[2].node.fail_htlc_backwards(&payment_hash, 0); }, 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, @@ -9826,13 +9821,9 @@ mod tests { }, ||{}, 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, PaymentFailReason::PreimageUnknown); + nodes[2].node.fail_htlc_backwards(&payment_hash, 0); }, false, Some(PERM|15), None); - run_onion_failure_test("incorrect_payment_amount", 2, &nodes, &route, &payment_hash, |_| {}, || { - nodes[2].node.fail_htlc_backwards(&payment_hash, PaymentFailReason::AmountMismatch); - }, false, Some(PERM|16), None); - run_onion_failure_test("final_expiry_too_soon", 1, &nodes, &route, &payment_hash, |msg| { let height = msg.cltv_expiry - CLTV_CLAIM_BUFFER - HTLC_FAIL_TIMEOUT_BLOCKS + 1; let header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 }; diff --git a/src/util/errors.rs b/src/util/errors.rs index 8032ea46e..27f837775 100644 --- a/src/util/errors.rs +++ b/src/util/errors.rs @@ -84,7 +84,7 @@ pub(crate) fn get_onion_error_description(error_code: u16) -> (&'static str, &'s _c if _c == UPDATE|12 => ("Node indicated the fee amount does not meet the required level", "fee_insufficient"), _c if _c == UPDATE|13 => ("Node indicated the cltv_expiry does not comply with the cltv_expiry_delta required by the outgoing channel", "incorrect_cltv_expiry"), _c if _c == UPDATE|14 => ("Node indicated the CLTV expiry too close to the current block height for safe handling", "expiry_too_soon"), - _c if _c == PERM|15 => ("The final node indicated the payment hash is unknown", "unknown_payment_hash"), + _c if _c == PERM|15 => ("The final node indicated the payment hash is unknown or amount is incorrect", "incorrect_or_unknown_payment_details"), _c if _c == PERM|16 => ("The final node indicated the payment amount is incorrect", "incorrect_payment_amount"), _c if _c == 17 => ("The final node indicated the CLTV expiry is too close to the current block height for safe handling", "final_expiry_too_soon"), _c if _c == 18 => ("The final node indicated the CLTV expiry in the HTLC does not match the value in the onion", "final_incorrect_cltv_expiry"), diff --git a/src/util/events.rs b/src/util/events.rs index 6d3ac33d1..de9dd4286 100644 --- a/src/util/events.rs +++ b/src/util/events.rs @@ -54,14 +54,15 @@ pub enum Event { /// Indicates we've received money! Just gotta dig out that payment preimage and feed it to /// ChannelManager::claim_funds to get it.... /// Note that if the preimage is not known or the amount paid is incorrect, you must call - /// ChannelManager::fail_htlc_backwards with PaymentFailReason::PreimageUnknown or - /// PaymentFailReason::AmountMismatch, respectively, to free up resources for this HTLC. + /// 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. PaymentReceived { /// The hash for which the preimage should be handed to the ChannelManager. payment_hash: PaymentHash, - /// The value, in thousandths of a satoshi, that this payment is for. + /// The value, in thousandths of a satoshi, that this payment is for. Note that you must + /// compare this to the expected value before accepting the payment (as otherwise you are + /// providing proof-of-payment for less than the value you expected!). amt: u64, }, /// Indicates an outbound payment we made succeeded (ie it made it all the way to its target