Skip to content

Commit 1484fee

Browse files
arik-soarik-so
committed
Remove invalid state options from Hop
Now that each `InboundOnionPayload` variant corresponds to its own struct, we can reference these same types inside `Hop` and thereby avoid nesting that allowed invalid combinations, and instead store supplemental data as each variant calls for.
1 parent d3981e1 commit 1484fee

File tree

4 files changed

+153
-53
lines changed

4 files changed

+153
-53
lines changed

‎lightning/src/ln/blinded_payment_tests.rs

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -411,15 +411,23 @@ fn do_forward_checks_failure(check: ForwardCheckFail, intro_fails: bool) {
411411
do_commitment_signed_dance(&nodes[0], &nodes[1], &updates.commitment_signed, false, false);
412412
let failed_destination = match check {
413413
ForwardCheckFail::InboundOnionCheck => HTLCDestination::InvalidOnion,
414-
ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::FailedPayment { payment_hash },
414+
ForwardCheckFail::ForwardPayloadEncodedAsReceive => HTLCDestination::InvalidOnion,
415415
ForwardCheckFail::OutboundChannelCheck =>
416416
HTLCDestination::NextHopChannel { node_id: Some(nodes[2].node.get_our_node_id()), channel_id: chan_1_2.2 },
417417
};
418418
expect_htlc_handling_failed_destinations!(
419419
nodes[1].node.get_and_clear_pending_events(), &[failed_destination.clone()]
420420
);
421-
expect_payment_failed_conditions(&nodes[0], payment_hash, false,
422-
PaymentFailedConditions::new().expected_htlc_error_data(INVALID_ONION_BLINDING, &[0; 32]));
421+
match check {
422+
ForwardCheckFail::ForwardPayloadEncodedAsReceive => {
423+
expect_payment_failed_conditions(&nodes[0], payment_hash, false,
424+
PaymentFailedConditions::new().expected_htlc_error_data(0x4000 | 22, &[0; 0]));
425+
}
426+
_ => {
427+
expect_payment_failed_conditions(&nodes[0], payment_hash, false,
428+
PaymentFailedConditions::new().expected_htlc_error_data(INVALID_ONION_BLINDING, &[0; 32]));
429+
}
430+
};
423431
return
424432
}
425433

@@ -1560,10 +1568,10 @@ fn route_blinding_spec_test_vector() {
15601568
Ok(res) => res,
15611569
_ => panic!("Unexpected error")
15621570
};
1563-
let (carol_packet_bytes, carol_hmac) = if let onion_utils::Hop::Forward {
1564-
next_hop_data: msgs::InboundOnionPayload::BlindedForward(msgs::InboundOnionBlindedForwardPayload {
1571+
let (carol_packet_bytes, carol_hmac) = if let onion_utils::Hop::BlindedForward {
1572+
next_hop_data: msgs::InboundOnionBlindedForwardPayload {
15651573
short_channel_id, payment_relay, payment_constraints, features, intro_node_blinding_point, next_blinding_override
1566-
}), next_hop_hmac, new_packet_bytes
1574+
}, next_hop_hmac, new_packet_bytes
15671575
} = bob_peeled_onion {
15681576
assert_eq!(short_channel_id, 1729);
15691577
assert!(next_blinding_override.is_none());
@@ -1594,10 +1602,10 @@ fn route_blinding_spec_test_vector() {
15941602
Ok(res) => res,
15951603
_ => panic!("Unexpected error")
15961604
};
1597-
let (dave_packet_bytes, dave_hmac) = if let onion_utils::Hop::Forward {
1598-
next_hop_data: msgs::InboundOnionPayload::BlindedForward(msgs::InboundOnionBlindedForwardPayload {
1605+
let (dave_packet_bytes, dave_hmac) = if let onion_utils::Hop::BlindedForward {
1606+
next_hop_data: msgs::InboundOnionBlindedForwardPayload {
15991607
short_channel_id, payment_relay, payment_constraints, features, intro_node_blinding_point, next_blinding_override
1600-
}), next_hop_hmac, new_packet_bytes
1608+
}, next_hop_hmac, new_packet_bytes
16011609
} = carol_peeled_onion {
16021610
assert_eq!(short_channel_id, 1105);
16031611
assert_eq!(next_blinding_override, Some(pubkey_from_hex("031b84c5567b126440995d3ed5aaba0565d71e1834604819ff9c17f5e9d5dd078f")));
@@ -1628,10 +1636,10 @@ fn route_blinding_spec_test_vector() {
16281636
Ok(res) => res,
16291637
_ => panic!("Unexpected error")
16301638
};
1631-
let (eve_packet_bytes, eve_hmac) = if let onion_utils::Hop::Forward {
1632-
next_hop_data: msgs::InboundOnionPayload::BlindedForward(msgs::InboundOnionBlindedForwardPayload {
1639+
let (eve_packet_bytes, eve_hmac) = if let onion_utils::Hop::BlindedForward {
1640+
next_hop_data: msgs::InboundOnionBlindedForwardPayload {
16331641
short_channel_id, payment_relay, payment_constraints, features, intro_node_blinding_point, next_blinding_override
1634-
}), next_hop_hmac, new_packet_bytes
1642+
}, next_hop_hmac, new_packet_bytes
16351643
} = dave_peeled_onion {
16361644
assert_eq!(short_channel_id, 561);
16371645
assert!(next_blinding_override.is_none());

‎lightning/src/ln/channelmanager.rs

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4430,7 +4430,24 @@ where
44304430
onion_utils::Hop::Receive(next_hop_data) => {
44314431
// OUR PAYMENT!
44324432
let current_height: u32 = self.best_block.read().unwrap().height;
4433-
match create_recv_pending_htlc_info(next_hop_data, shared_secret, msg.payment_hash,
4433+
match create_recv_pending_htlc_info(msgs::InboundOnionPayload::Receive(next_hop_data), shared_secret, msg.payment_hash,
4434+
msg.amount_msat, msg.cltv_expiry, None, allow_underpay, msg.skimmed_fee_msat,
4435+
current_height)
4436+
{
4437+
Ok(info) => {
4438+
// Note that we could obviously respond immediately with an update_fulfill_htlc
4439+
// message, however that would leak that we are the recipient of this payment, so
4440+
// instead we stay symmetric with the forwarding case, only responding (after a
4441+
// delay) once they've send us a commitment_signed!
4442+
PendingHTLCStatus::Forward(info)
4443+
},
4444+
Err(InboundHTLCErr { err_code, err_data, msg }) => return_err!(msg, err_code, &err_data)
4445+
}
4446+
},
4447+
onion_utils::Hop::BlindedReceive(next_hop_data) => {
4448+
// OUR PAYMENT!
4449+
let current_height: u32 = self.best_block.read().unwrap().height;
4450+
match create_recv_pending_htlc_info(msgs::InboundOnionPayload::BlindedReceive(next_hop_data), shared_secret, msg.payment_hash,
44344451
msg.amount_msat, msg.cltv_expiry, None, allow_underpay, msg.skimmed_fee_msat,
44354452
current_height)
44364453
{
@@ -4445,7 +4462,14 @@ where
44454462
}
44464463
},
44474464
onion_utils::Hop::Forward { next_hop_data, next_hop_hmac, new_packet_bytes } => {
4448-
match create_fwd_pending_htlc_info(msg, next_hop_data, next_hop_hmac,
4465+
match create_fwd_pending_htlc_info(msg, msgs::InboundOnionPayload::Forward(next_hop_data), next_hop_hmac,
4466+
new_packet_bytes, shared_secret, next_packet_pubkey_opt) {
4467+
Ok(info) => PendingHTLCStatus::Forward(info),
4468+
Err(InboundHTLCErr { err_code, err_data, msg }) => return_err!(msg, err_code, &err_data)
4469+
}
4470+
},
4471+
onion_utils::Hop::BlindedForward { next_hop_data, next_hop_hmac, new_packet_bytes } => {
4472+
match create_fwd_pending_htlc_info(msg, msgs::InboundOnionPayload::BlindedForward(next_hop_data), next_hop_hmac,
44494473
new_packet_bytes, shared_secret, next_packet_pubkey_opt) {
44504474
Ok(info) => PendingHTLCStatus::Forward(info),
44514475
Err(InboundHTLCErr { err_code, err_data, msg }) => return_err!(msg, err_code, &err_data)
@@ -5863,7 +5887,21 @@ where
58635887
match next_hop {
58645888
onion_utils::Hop::Receive(hop_data) => {
58655889
let current_height: u32 = self.best_block.read().unwrap().height;
5866-
match create_recv_pending_htlc_info(hop_data,
5890+
match create_recv_pending_htlc_info(msgs::InboundOnionPayload::Receive(hop_data),
5891+
incoming_shared_secret, payment_hash, outgoing_amt_msat,
5892+
outgoing_cltv_value, Some(phantom_shared_secret), false, None,
5893+
current_height)
5894+
{
5895+
Ok(info) => phantom_receives.push((
5896+
prev_short_channel_id, prev_counterparty_node_id, prev_funding_outpoint,
5897+
prev_channel_id, prev_user_channel_id, vec![(info, prev_htlc_id)]
5898+
)),
5899+
Err(InboundHTLCErr { err_code, err_data, msg }) => failed_payment!(msg, err_code, err_data, Some(phantom_shared_secret))
5900+
}
5901+
},
5902+
onion_utils::Hop::BlindedReceive(hop_data) => {
5903+
let current_height: u32 = self.best_block.read().unwrap().height;
5904+
match create_recv_pending_htlc_info(msgs::InboundOnionPayload::BlindedReceive(hop_data),
58675905
incoming_shared_secret, payment_hash, outgoing_amt_msat,
58685906
outgoing_cltv_value, Some(phantom_shared_secret), false, None,
58695907
current_height)

‎lightning/src/ln/onion_payment.rs

Lines changed: 49 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ use crate::ln::channelmanager::{BlindedFailure, BlindedForward, CLTV_FAR_FAR_AWA
1616
use crate::types::features::BlindedHopFeatures;
1717
use crate::ln::msgs;
1818
use crate::ln::onion_utils;
19-
use crate::ln::onion_utils::{HTLCFailReason, INVALID_ONION_BLINDING};
19+
use crate::ln::onion_utils::{Hop, HTLCFailReason, INVALID_ONION_BLINDING};
2020
use crate::sign::{NodeSigner, Recipient};
2121
use crate::util::logger::Logger;
2222

@@ -323,13 +323,50 @@ where
323323
// onion here and check it.
324324

325325
create_fwd_pending_htlc_info(
326-
msg, next_hop_data, next_hop_hmac, new_packet_bytes, shared_secret,
326+
msg, msgs::InboundOnionPayload::Forward(next_hop_data), next_hop_hmac, new_packet_bytes, shared_secret,
327+
Some(next_packet_pubkey)
328+
)?
329+
},
330+
onion_utils::Hop::BlindedForward { next_hop_data, next_hop_hmac, new_packet_bytes } => {
331+
let NextPacketDetails {
332+
next_packet_pubkey, outgoing_amt_msat: _, outgoing_scid: _, outgoing_cltv_value
333+
} = match next_packet_details_opt {
334+
Some(next_packet_details) => next_packet_details,
335+
// Forward should always include the next hop details
336+
None => return Err(InboundHTLCErr {
337+
msg: "Failed to decode update add htlc onion",
338+
err_code: 0x4000 | 22,
339+
err_data: Vec::new(),
340+
}),
341+
};
342+
343+
if let Err((err_msg, code)) = check_incoming_htlc_cltv(
344+
cur_height, outgoing_cltv_value, msg.cltv_expiry
345+
) {
346+
return Err(InboundHTLCErr {
347+
msg: err_msg,
348+
err_code: code,
349+
err_data: Vec::new(),
350+
});
351+
}
352+
353+
// TODO: If this is potentially a phantom payment we should decode the phantom payment
354+
// onion here and check it.
355+
356+
create_fwd_pending_htlc_info(
357+
msg, msgs::InboundOnionPayload::BlindedForward(next_hop_data), next_hop_hmac, new_packet_bytes, shared_secret,
327358
Some(next_packet_pubkey)
328359
)?
329360
},
330361
onion_utils::Hop::Receive(received_data) => {
331362
create_recv_pending_htlc_info(
332-
received_data, shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry,
363+
msgs::InboundOnionPayload::Receive(received_data), shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry,
364+
None, allow_skimmed_fees, msg.skimmed_fee_msat, cur_height
365+
)?
366+
},
367+
onion_utils::Hop::BlindedReceive(received_data) => {
368+
create_recv_pending_htlc_info(
369+
msgs::InboundOnionPayload::BlindedReceive(received_data), shared_secret, msg.payment_hash, msg.amount_msat, msg.cltv_expiry,
333370
None, allow_skimmed_fees, msg.skimmed_fee_msat, cur_height
334371
)?
335372
}
@@ -424,23 +461,15 @@ where
424461
};
425462

426463
let next_packet_details = match next_hop {
427-
onion_utils::Hop::Forward {
428-
next_hop_data: msgs::InboundOnionPayload::Forward(msgs::InboundOnionForwardPayload {
429-
short_channel_id, amt_to_forward, outgoing_cltv_value
430-
}), ..
431-
} => {
464+
Hop::Forward { next_hop_data: msgs::InboundOnionForwardPayload { short_channel_id, amt_to_forward, outgoing_cltv_value }, .. } => {
432465
let next_packet_pubkey = onion_utils::next_hop_pubkey(secp_ctx,
433466
msg.onion_routing_packet.public_key.unwrap(), &shared_secret);
434-
NextPacketDetails {
467+
Some(NextPacketDetails {
435468
next_packet_pubkey, outgoing_scid: short_channel_id,
436469
outgoing_amt_msat: amt_to_forward, outgoing_cltv_value
437-
}
438-
},
439-
onion_utils::Hop::Forward {
440-
next_hop_data: msgs::InboundOnionPayload::BlindedForward (msgs::InboundOnionBlindedForwardPayload{
441-
short_channel_id, ref payment_relay, ref payment_constraints, ref features, ..
442-
}), ..
443-
} => {
470+
})
471+
}
472+
Hop::BlindedForward { next_hop_data: msgs::InboundOnionBlindedForwardPayload { short_channel_id, ref payment_relay, ref payment_constraints, ref features, .. }, .. } => {
444473
let (amt_to_forward, outgoing_cltv_value) = match check_blinded_forward(
445474
msg.amount_msat, msg.cltv_expiry, &payment_relay, &payment_constraints, &features
446475
) {
@@ -452,20 +481,15 @@ where
452481
};
453482
let next_packet_pubkey = onion_utils::next_hop_pubkey(&secp_ctx,
454483
msg.onion_routing_packet.public_key.unwrap(), &shared_secret);
455-
NextPacketDetails {
484+
Some(NextPacketDetails {
456485
next_packet_pubkey, outgoing_scid: short_channel_id, outgoing_amt_msat: amt_to_forward,
457486
outgoing_cltv_value
458-
}
459-
},
460-
onion_utils::Hop::Receive { .. } => return Ok((next_hop, shared_secret, None)),
461-
onion_utils::Hop::Forward { next_hop_data: msgs::InboundOnionPayload::Receive { .. }, .. } |
462-
onion_utils::Hop::Forward { next_hop_data: msgs::InboundOnionPayload::BlindedReceive { .. }, .. } =>
463-
{
464-
return_err!("Final Node OnionHopData provided for us as an intermediary node", 0x4000 | 22, &[0; 0]);
487+
})
465488
}
489+
_ => None
466490
};
467491

468-
Ok((next_hop, shared_secret, Some(next_packet_details)))
492+
Ok((next_hop, shared_secret, next_packet_details))
469493
}
470494

471495
pub(super) fn check_incoming_htlc_cltv(

‎lightning/src/ln/onion_utils.rs

Lines changed: 43 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1415,29 +1415,40 @@ impl NextPacketBytes for Vec<u8> {
14151415

14161416
/// Data decrypted from a payment's onion payload.
14171417
pub(crate) enum Hop {
1418-
/// This onion payload was for us, not for forwarding to a next-hop. Contains information for
1419-
/// verifying the incoming payment.
1420-
Receive(msgs::InboundOnionPayload),
14211418
/// This onion payload needs to be forwarded to a next-hop.
14221419
Forward {
14231420
/// Onion payload data used in forwarding the payment.
1424-
next_hop_data: msgs::InboundOnionPayload,
1421+
next_hop_data: msgs::InboundOnionForwardPayload,
14251422
/// HMAC of the next hop's onion packet.
14261423
next_hop_hmac: [u8; 32],
14271424
/// Bytes of the onion packet we're forwarding.
14281425
new_packet_bytes: [u8; ONION_DATA_LEN],
14291426
},
1427+
/// This onion payload needs to be forwarded to a next-hop.
1428+
BlindedForward {
1429+
/// Onion payload data used in forwarding the payment.
1430+
next_hop_data: msgs::InboundOnionBlindedForwardPayload,
1431+
/// HMAC of the next hop's onion packet.
1432+
next_hop_hmac: [u8; 32],
1433+
/// Bytes of the onion packet we're forwarding.
1434+
new_packet_bytes: [u8; ONION_DATA_LEN],
1435+
},
1436+
/// This onion payload was for us, not for forwarding to a next-hop. Contains information for
1437+
/// verifying the incoming payment.
1438+
Receive(msgs::InboundOnionReceivePayload),
1439+
/// This onion payload was for us, not for forwarding to a next-hop. Contains information for
1440+
/// verifying the incoming payment.
1441+
BlindedReceive(msgs::InboundOnionBlindedReceivePayload),
14301442
}
14311443

14321444
impl Hop {
14331445
pub(crate) fn is_intro_node_blinded_forward(&self) -> bool {
14341446
match self {
1435-
Self::Forward {
1447+
Self::BlindedForward {
14361448
next_hop_data:
1437-
msgs::InboundOnionPayload::BlindedForward(msgs::InboundOnionBlindedForwardPayload {
1438-
intro_node_blinding_point: Some(_),
1439-
..
1440-
}),
1449+
msgs::InboundOnionBlindedForwardPayload {
1450+
intro_node_blinding_point: Some(_), ..
1451+
},
14411452
..
14421453
} => true,
14431454
_ => false,
@@ -1461,16 +1472,35 @@ pub(crate) fn decode_next_payment_hop<NS: Deref>(
14611472
where
14621473
NS::Target: NodeSigner,
14631474
{
1464-
match decode_next_hop(
1475+
let decoded_hop: Result<(msgs::InboundOnionPayload, Option<_>), _> = decode_next_hop(
14651476
shared_secret,
14661477
hop_data,
14671478
hmac_bytes,
14681479
Some(payment_hash),
14691480
(blinding_point, node_signer),
1470-
) {
1471-
Ok((next_hop_data, None)) => Ok(Hop::Receive(next_hop_data)),
1481+
);
1482+
match decoded_hop {
14721483
Ok((next_hop_data, Some((next_hop_hmac, FixedSizeOnionPacket(new_packet_bytes))))) => {
1473-
Ok(Hop::Forward { next_hop_data, next_hop_hmac, new_packet_bytes })
1484+
match next_hop_data {
1485+
msgs::InboundOnionPayload::Forward(next_hop_data) => {
1486+
Ok(Hop::Forward { next_hop_data, next_hop_hmac, new_packet_bytes })
1487+
},
1488+
msgs::InboundOnionPayload::BlindedForward(next_hop_data) => {
1489+
Ok(Hop::BlindedForward { next_hop_data, next_hop_hmac, new_packet_bytes })
1490+
},
1491+
_ => Err(OnionDecodeErr::Relay {
1492+
err_msg: "Final Node OnionHopData provided for us as an intermediary node",
1493+
err_code: 0x4000 | 22,
1494+
}),
1495+
}
1496+
},
1497+
Ok((next_hop_data, None)) => match next_hop_data {
1498+
msgs::InboundOnionPayload::Receive(payload) => Ok(Hop::Receive(payload)),
1499+
msgs::InboundOnionPayload::BlindedReceive(payload) => Ok(Hop::BlindedReceive(payload)),
1500+
_ => Err(OnionDecodeErr::Relay {
1501+
err_msg: "Intermediate Node OnionHopData provided for us as a final node",
1502+
err_code: 0x4000 | 22,
1503+
}),
14741504
},
14751505
Err(e) => Err(e),
14761506
}

0 commit comments

Comments
 (0)