Skip to content

Followup: custom HTLC TLVs #2504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 17, 2023
Merged

Followup: custom HTLC TLVs #2504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd90bd2
Simplify custom HTLC TLV tests
alecchendev Aug 16, 2023
83f0dbc
Address custom HTLC TLV fixups
alecchendev Aug 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions lightning/src/ln/msgs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1987,10 +1987,8 @@ impl Writeable for OutboundOnionPayload {
// We need to update [`ln::outbound_payment::RecipientOnionFields::with_custom_tlvs`]
// to reject any reserved types in the experimental range if new ones are ever
// standardized.
let preimage = if let Some(ref preimage) = keysend_preimage {
Some((5482373484, preimage.encode()))
} else { None };
let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(preimage.iter()).collect();
let keysend_tlv = keysend_preimage.map(|preimage| (5482373484, preimage.encode()));
let mut custom_tlvs: Vec<&(u64, Vec<u8>)> = custom_tlvs.iter().chain(keysend_tlv.iter()).collect();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't buy that we need to collect here. We construct an iterator and then turn it into a vec, and then pass it to the macro as an iterator. The macro uses the iterator twice, iirc, so we can't just generate the iterator here and then give it to the macro, but we can paste this line in the macro call and it should just work (or have the macro clone the iterator before use).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We sort after we collect into a vec?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh oops, right, hmmm, I mean its definitely possible to avoid the collect but probably not entirely worth writing a custom iterator for it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess its not that crazy to build the iterator by hand using https://doc.rust-lang.org/std/iter/fn.from_fn.html but no need to bother.

custom_tlvs.sort_unstable_by_key(|(typ, _)| *typ);
_encode_varint_length_prefixed_tlv!(w, {
(2, HighZeroBytesDroppedBigSize(*amt_msat), required),
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/outbound_payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -515,9 +515,9 @@ impl RecipientOnionFields {
let tlvs = &mut self.custom_tlvs;
let further_tlvs = &mut further_htlc_fields.custom_tlvs;

let even_tlvs: Vec<&(u64, Vec<u8>)> = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();
let further_even_tlvs: Vec<&(u64, Vec<u8>)> = further_tlvs.iter().filter(|(typ, _)| *typ % 2 == 0).collect();
if even_tlvs != further_even_tlvs { return Err(()) }
let even_tlvs = tlvs.iter().filter(|(typ, _)| *typ % 2 == 0);
let further_even_tlvs = further_tlvs.iter().filter(|(typ, _)| *typ % 2 == 0);
if even_tlvs.ne(further_even_tlvs) { return Err(()) }

tlvs.retain(|tlv| further_tlvs.iter().any(|further_tlv| tlv == further_tlv));
further_tlvs.retain(|further_tlv| tlvs.iter().any(|tlv| tlv == further_tlv));
Expand Down
84 changes: 28 additions & 56 deletions lightning/src/ln/payment_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3451,17 +3451,7 @@ fn do_test_custom_tlvs(spontaneous: bool, even_tlvs: bool, known_tlvs: bool) {
let events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentClaimable { ref purpose, amount_msat, ref onion_fields, .. } => {
match &purpose {
PaymentPurpose::InvoicePayment { payment_secret, .. } => {
assert_eq!(our_payment_secret, *payment_secret);
assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);
},
PaymentPurpose::SpontaneousPayment(payment_preimage) => {
assert_eq!(our_payment_preimage, *payment_preimage);
},
}
assert_eq!(amount_msat, amt_msat);
Event::PaymentClaimable { ref onion_fields, .. } => {
assert_eq!(onion_fields.clone().unwrap().custom_tlvs().clone(), custom_tlvs);
},
_ => panic!("Unexpected event"),
Expand Down Expand Up @@ -3518,26 +3508,12 @@ fn test_retry_custom_tlvs() {
nodes[0].node.send_payment(payment_hash, onion_fields,
payment_id, route_params.clone(), Retry::Attempts(1)).unwrap();
check_added_monitors!(nodes[0], 1); // one monitor per path
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);

// Add the HTLC along the first hop.
let fail_path_msgs_1 = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut events);
let (update_add, commitment_signed) = match fail_path_msgs_1 {
MessageSendEvent::UpdateHTLCs { node_id: _, updates: msgs::CommitmentUpdate {
ref update_add_htlcs, ref update_fulfill_htlcs, ref update_fail_htlcs,
ref update_fail_malformed_htlcs, ref update_fee, ref commitment_signed }
} => {
assert_eq!(update_add_htlcs.len(), 1);
assert!(update_fail_htlcs.is_empty());
assert!(update_fulfill_htlcs.is_empty());
assert!(update_fail_malformed_htlcs.is_empty());
assert!(update_fee.is_none());
(update_add_htlcs[0].clone(), commitment_signed.clone())
},
_ => panic!("Unexpected event"),
};
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add);
let htlc_updates = get_htlc_update_msgs(&nodes[0], &nodes[1].node.get_our_node_id());
let msgs::CommitmentUpdate { update_add_htlcs, commitment_signed, .. } = htlc_updates;
assert_eq!(update_add_htlcs.len(), 1);
nodes[1].node.handle_update_add_htlc(&nodes[0].node.get_our_node_id(), &update_add_htlcs[0]);
commitment_signed_dance!(nodes[1], nodes[0], commitment_signed, false);

// Attempt to forward the payment and complete the path's failure.
Expand All @@ -3547,15 +3523,14 @@ fn test_retry_custom_tlvs() {
node_id: Some(nodes[2].node.get_our_node_id()),
channel_id: chan_2_id
}]);
let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
assert!(htlc_updates.update_add_htlcs.is_empty());
assert_eq!(htlc_updates.update_fail_htlcs.len(), 1);
assert!(htlc_updates.update_fulfill_htlcs.is_empty());
assert!(htlc_updates.update_fail_malformed_htlcs.is_empty());
check_added_monitors!(nodes[1], 1);
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(),
&htlc_updates.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[0], nodes[1], htlc_updates.commitment_signed, false);

let htlc_updates = get_htlc_update_msgs!(nodes[1], nodes[0].node.get_our_node_id());
let msgs::CommitmentUpdate { update_fail_htlcs, commitment_signed, .. } = htlc_updates;
assert_eq!(update_fail_htlcs.len(), 1);
nodes[0].node.handle_update_fail_htlc(&nodes[1].node.get_our_node_id(), &update_fail_htlcs[0]);
commitment_signed_dance!(nodes[0], nodes[1], commitment_signed, false);

let mut events = nodes[0].node.get_and_clear_pending_events();
match events[1] {
Event::PendingHTLCsForwardable { .. } => {},
Expand All @@ -3577,11 +3552,12 @@ fn test_retry_custom_tlvs() {
assert_eq!(events.len(), 1);
let payment_claimable = pass_along_path(&nodes[0], &[&nodes[1], &nodes[2]], 1_000_000,
payment_hash, Some(payment_secret), events.pop().unwrap(), true, None).unwrap();
let onion_fields = match payment_claimable {
Event::PaymentClaimable { onion_fields, .. } => onion_fields,
match payment_claimable {
Event::PaymentClaimable { onion_fields, .. } => {
assert_eq!(onion_fields.unwrap().custom_tlvs(), &custom_tlvs);
},
_ => panic!("Unexpected event"),
};
assert_eq!(onion_fields.unwrap().custom_tlvs(), &custom_tlvs);
claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[2]]], false, payment_preimage);
}

Expand Down Expand Up @@ -3665,7 +3641,8 @@ fn do_test_custom_tlvs_consistency(first_tlvs: Vec<(u64, Vec<u8>)>, second_tlvs:
{
let mut events = nodes[0].node.get_and_clear_pending_msg_events();
assert_eq!(events.len(), 1);
pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], amt_msat, our_payment_hash, Some(our_payment_secret), events.pop().unwrap(), false, None);
pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], amt_msat, our_payment_hash,
Some(our_payment_secret), events.pop().unwrap(), false, None);
}
assert!(nodes[3].node.get_and_clear_pending_events().is_empty());

Expand Down Expand Up @@ -3704,26 +3681,16 @@ fn do_test_custom_tlvs_consistency(first_tlvs: Vec<(u64, Vec<u8>)>, second_tlvs:
if let Some(expected_tlvs) = expected_receive_tlvs {
// Claim and match expected
let events = nodes[3].node.get_and_clear_pending_events();
println!("events: {:?}", events);
assert_eq!(events.len(), 1);
match events[0] {
Event::PaymentClaimable { ref purpose, amount_msat, ref onion_fields, .. } => {
match &purpose {
PaymentPurpose::InvoicePayment { payment_secret, .. } => {
assert_eq!(our_payment_secret, *payment_secret);
assert_eq!(Some(*payment_secret), onion_fields.as_ref().unwrap().payment_secret);
},
PaymentPurpose::SpontaneousPayment(payment_preimage) => {
assert_eq!(our_payment_preimage, *payment_preimage);
},
}
assert_eq!(amount_msat, amt_msat);
Event::PaymentClaimable { ref onion_fields, .. } => {
assert_eq!(onion_fields.clone().unwrap().custom_tlvs, expected_tlvs);
},
_ => panic!("Unexpected event"),
}

do_claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]], false, our_payment_preimage);
do_claim_payment_along_route(&nodes[0], &[&[&nodes[1], &nodes[3]], &[&nodes[2], &nodes[3]]],
false, our_payment_preimage);
expect_payment_sent(&nodes[0], our_payment_preimage, Some(Some(2000)), true);
} else {
// Expect fail back
Expand All @@ -3735,14 +3702,19 @@ fn do_test_custom_tlvs_consistency(first_tlvs: Vec<(u64, Vec<u8>)>, second_tlvs:
nodes[2].node.handle_update_fail_htlc(&nodes[3].node.get_our_node_id(), &fail_updates_1.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[2], nodes[3], fail_updates_1.commitment_signed, false);

expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], vec![HTLCDestination::NextHopChannel { node_id: Some(nodes[3].node.get_our_node_id()), channel_id: chan_2_3.2 }]);
expect_pending_htlcs_forwardable_and_htlc_handling_failed!(nodes[2], vec![
HTLCDestination::NextHopChannel {
node_id: Some(nodes[3].node.get_our_node_id()),
channel_id: chan_2_3.2
}]);
check_added_monitors!(nodes[2], 1);

let fail_updates_2 = get_htlc_update_msgs!(nodes[2], nodes[0].node.get_our_node_id());
nodes[0].node.handle_update_fail_htlc(&nodes[2].node.get_our_node_id(), &fail_updates_2.update_fail_htlcs[0]);
commitment_signed_dance!(nodes[0], nodes[2], fail_updates_2.commitment_signed, false);

expect_payment_failed_conditions(&nodes[0], our_payment_hash, true, PaymentFailedConditions::new().mpp_parts_remain());
expect_payment_failed_conditions(&nodes[0], our_payment_hash, true,
PaymentFailedConditions::new().mpp_parts_remain());
}
}

Expand Down
26 changes: 11 additions & 15 deletions lightning/src/util/ser_macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,9 @@ macro_rules! encode_tlv_stream {
#[macro_export]
macro_rules! _encode_tlv_stream {
($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt)),* $(,)*}) => { {
$crate::_encode_tlv_stream!($stream, { $(($type, $field, $fieldty)),* }, &[])
} };
($stream: expr, {$(($type: expr, $field: expr, $fieldty: tt)),* $(,)*}, $extra_tlvs: expr) => { {
#[allow(unused_imports)]
use $crate::{
ln::msgs::DecodeError,
Expand All @@ -154,6 +157,10 @@ macro_rules! _encode_tlv_stream {
$(
$crate::_encode_tlv!($stream, $type, $field, $fieldty);
)*
for tlv in $extra_tlvs {
let (typ, value): &(u64, Vec<u8>) = tlv;
$crate::_encode_tlv!($stream, *typ, *value, required_vec);
}

#[allow(unused_mut, unused_variables, unused_assignments)]
#[cfg(debug_assertions)]
Expand All @@ -162,18 +169,8 @@ macro_rules! _encode_tlv_stream {
$(
$crate::_check_encoded_tlv_order!(last_seen, $type, $fieldty);
)*
}
} };
($stream: expr, $tlvs: expr) => { {
for tlv in $tlvs {
let (typ, value): &&(u64, Vec<u8>) = tlv;
$crate::_encode_tlv!($stream, *typ, *value, required_vec);
}

#[cfg(debug_assertions)] {
let mut last_seen: Option<u64> = None;
for tlv in $tlvs {
let (typ, _): &&(u64, Vec<u8>) = tlv;
for tlv in $extra_tlvs {
let (typ, _): &(u64, Vec<u8>) = tlv;
$crate::_check_encoded_tlv_order!(last_seen, *typ, required_vec);
}
}
Expand Down Expand Up @@ -246,14 +243,13 @@ macro_rules! _encode_varint_length_prefixed_tlv {
$crate::_get_varint_length_prefixed_tlv_length!(len, $type, $field, $fieldty);
)*
for tlv in $extra_tlvs {
let (typ, value): &&(u64, Vec<u8>) = tlv;
let (typ, value): &(u64, Vec<u8>) = tlv;
$crate::_get_varint_length_prefixed_tlv_length!(len, *typ, *value, required_vec);
}
len.0
};
BigSize(len as u64).write($stream)?;
$crate::_encode_tlv_stream!($stream, { $(($type, $field, $fieldty)),* });
$crate::_encode_tlv_stream!($stream, $extra_tlvs);
$crate::_encode_tlv_stream!($stream, { $(($type, $field, $fieldty)),* }, $extra_tlvs);
} };
}

Expand Down