Skip to content
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.

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

Test blinded forwarding #2823

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
531 changes: 434 additions & 97 deletions lightning/src/ln/blinded_payment_tests.rs

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion lightning/src/ln/chanmon_update_fail_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3405,7 +3405,7 @@ fn do_test_reload_mon_update_completion_actions(close_during_reload: bool) {
let mut events = nodes[1].node.get_and_clear_pending_events();
assert_eq!(events.len(), if close_during_reload { 2 } else { 1 });
expect_payment_forwarded(events.pop().unwrap(), &nodes[1], &nodes[0], &nodes[2], Some(1000),
None, close_during_reload, false);
None, close_during_reload, false, false);
if close_during_reload {
match events[0] {
Event::ChannelClosed { .. } => {},
Expand Down
147 changes: 125 additions & 22 deletions lightning/src/ln/functional_test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2223,17 +2223,26 @@ macro_rules! expect_payment_path_successful {
}
}

/// Returns the total fee earned by this HTLC forward, in msat.
pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
event: Event, node: &H, prev_node: &H, next_node: &H, expected_fee: Option<u64>,
expected_extra_fees_msat: Option<u64>, upstream_force_closed: bool,
downstream_force_closed: bool
) {
downstream_force_closed: bool, allow_1_msat_fee_overpay: bool,
Comment on lines 2229 to +2230
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we make a struct for these args?

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 can but I opted to skip that refactor here because we also have the macro expect_payment_forwarded which is used in the majority of cases. So it seems ok to me to just always add rarely-used args to the function and keep the macro for the general case.

) -> Option<u64> {
valentinewallace marked this conversation as resolved.
Show resolved Hide resolved
match event {
Event::PaymentForwarded {
total_fee_earned_msat, prev_channel_id, claim_from_onchain_tx, next_channel_id,
outbound_amount_forwarded_msat: _, skimmed_fee_msat
} => {
assert_eq!(total_fee_earned_msat, expected_fee);
if allow_1_msat_fee_overpay {
// Aggregating fees for blinded paths may result in a rounding error, causing slight
// overpayment in fees.
let actual_fee = total_fee_earned_msat.unwrap();
let expected_fee = expected_fee.unwrap();
assert!(actual_fee == expected_fee || actual_fee == expected_fee + 1);
} else {
assert_eq!(total_fee_earned_msat, expected_fee);
}

// Check that the (knowingly) withheld amount is always less or equal to the expected
// overpaid amount.
Expand All @@ -2248,6 +2257,7 @@ pub fn expect_payment_forwarded<CM: AChannelManager, H: NodeHolder<CM=CM>>(
assert!(node.node().list_channels().iter().any(|x| x.counterparty.node_id == next_node.node().get_our_node_id() && x.channel_id == next_channel_id.unwrap()));
}
assert_eq!(claim_from_onchain_tx, downstream_force_closed);
total_fee_earned_msat
},
_ => panic!("Unexpected event"),
}
Expand All @@ -2260,7 +2270,7 @@ macro_rules! expect_payment_forwarded {
assert_eq!(events.len(), 1);
$crate::ln::functional_test_utils::expect_payment_forwarded(
events.pop().unwrap(), &$node, &$prev_node, &$next_node, $expected_fee, None,
$upstream_force_closed, $downstream_force_closed
$upstream_force_closed, $downstream_force_closed, false
);
}
}
Expand Down Expand Up @@ -2472,7 +2482,60 @@ fn fail_payment_along_path<'a, 'b, 'c>(expected_path: &[&Node<'a, 'b, 'c>]) {
}
}

pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>, ev: MessageSendEvent, payment_claimable_expected: bool, clear_recipient_events: bool, expected_preimage: Option<PaymentPreimage>, is_probe: bool) -> Option<Event> {
pub struct PassAlongPathArgs<'a, 'b, 'c, 'd> {
pub origin_node: &'a Node<'b, 'c, 'd>,
pub expected_path: &'a [&'a Node<'b, 'c, 'd>],
pub recv_value: u64,
pub payment_hash: PaymentHash,
pub payment_secret: Option<PaymentSecret>,
pub event: MessageSendEvent,
pub payment_claimable_expected: bool,
pub clear_recipient_events: bool,
pub expected_preimage: Option<PaymentPreimage>,
pub is_probe: bool,
}

impl<'a, 'b, 'c, 'd> PassAlongPathArgs<'a, 'b, 'c, 'd> {
pub fn new(
origin_node: &'a Node<'b, 'c, 'd>, expected_path: &'a [&'a Node<'b, 'c, 'd>], recv_value: u64,
payment_hash: PaymentHash, event: MessageSendEvent,
) -> Self {
Self {
origin_node, expected_path, recv_value, payment_hash, payment_secret: None, event,
payment_claimable_expected: true, clear_recipient_events: true, expected_preimage: None,
is_probe: false,
}
}
pub fn without_clearing_recipient_events(mut self) -> Self {
self.clear_recipient_events = false;
self
}
pub fn is_probe(mut self) -> Self {
self.payment_claimable_expected = false;
self.is_probe = true;
self
}
pub fn without_claimable_event(mut self) -> Self {
self.payment_claimable_expected = false;
self
}
pub fn with_payment_secret(mut self, payment_secret: PaymentSecret) -> Self {
self.payment_secret = Some(payment_secret);
self
}
pub fn with_payment_preimage(mut self, payment_preimage: PaymentPreimage) -> Self {
self.expected_preimage = Some(payment_preimage);
self
}
}

pub fn do_pass_along_path<'a, 'b, 'c>(args: PassAlongPathArgs) -> Option<Event> {
let PassAlongPathArgs {
origin_node, expected_path, recv_value, payment_hash: our_payment_hash,
payment_secret: our_payment_secret, event: ev, payment_claimable_expected,
clear_recipient_events, expected_preimage, is_probe
} = args;

let mut payment_event = SendEvent::from_event(ev);
let mut prev_node = origin_node;
let mut event = None;
Expand Down Expand Up @@ -2539,7 +2602,17 @@ pub fn do_pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_p
}

pub fn pass_along_path<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_path: &[&Node<'a, 'b, 'c>], recv_value: u64, our_payment_hash: PaymentHash, our_payment_secret: Option<PaymentSecret>, ev: MessageSendEvent, payment_claimable_expected: bool, expected_preimage: Option<PaymentPreimage>) -> Option<Event> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also take PassAlongPathArgs?

Copy link
Contributor Author

@valentinewallace valentinewallace Mar 5, 2024

Choose a reason for hiding this comment

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

Yeah probably, it has like 50 callsites though so I skipped it for now. Open to doing it in follow-up.

do_pass_along_path(origin_node, expected_path, recv_value, our_payment_hash, our_payment_secret, ev, payment_claimable_expected, true, expected_preimage, false)
let mut args = PassAlongPathArgs::new(origin_node, expected_path, recv_value, our_payment_hash, ev);
if !payment_claimable_expected {
args = args.without_claimable_event();
}
if let Some(payment_secret) = our_payment_secret {
args = args.with_payment_secret(payment_secret);
}
if let Some(payment_preimage) = expected_preimage {
args = args.with_payment_preimage(payment_preimage);
}
do_pass_along_path(args)
}

pub fn send_probe_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expected_route: &[&[&Node<'a, 'b, 'c>]]) {
Expand All @@ -2551,7 +2624,10 @@ pub fn send_probe_along_route<'a, 'b, 'c>(origin_node: &Node<'a, 'b, 'c>, expect
for path in expected_route.iter() {
let ev = remove_first_msg_event_to_node(&path[0].node.get_our_node_id(), &mut events);

do_pass_along_path(origin_node, path, 0, PaymentHash([0_u8; 32]), None, ev, false, false, None, true);
do_pass_along_path(PassAlongPathArgs::new(origin_node, path, 0, PaymentHash([0_u8; 32]), ev)
.is_probe()
.without_clearing_recipient_events());

let nodes_to_fail_payment: Vec<_> = vec![origin_node].into_iter().chain(path.iter().cloned()).collect();

fail_payment_along_path(nodes_to_fail_payment.as_slice());
Expand Down Expand Up @@ -2598,6 +2674,14 @@ pub struct ClaimAlongRouteArgs<'a, 'b, 'c, 'd> {
pub expected_min_htlc_overpay: Vec<u32>,
pub skip_last: bool,
pub payment_preimage: PaymentPreimage,
// Allow forwarding nodes to have taken 1 msat more fee than expected based on the downstream
// fulfill amount.
//
// Necessary because our test utils calculate the expected fee for an intermediate node based on
// the amount was claimed in their downstream peer's fulfill, but blinded intermediate nodes
// calculate their fee based on the inbound amount from their upstream peer, causing a difference
// in rounding.
pub allow_1_msat_fee_overpay: bool,
}

impl<'a, 'b, 'c, 'd> ClaimAlongRouteArgs<'a, 'b, 'c, 'd> {
Expand All @@ -2608,6 +2692,7 @@ impl<'a, 'b, 'c, 'd> ClaimAlongRouteArgs<'a, 'b, 'c, 'd> {
Self {
origin_node, expected_paths, expected_extra_fees: vec![0; expected_paths.len()],
expected_min_htlc_overpay: vec![0; expected_paths.len()], skip_last: false, payment_preimage,
allow_1_msat_fee_overpay: false,
}
}
pub fn skip_last(mut self, skip_last: bool) -> Self {
Expand All @@ -2622,15 +2707,21 @@ impl<'a, 'b, 'c, 'd> ClaimAlongRouteArgs<'a, 'b, 'c, 'd> {
self.expected_min_htlc_overpay = extra_fees;
self
}
pub fn allow_1_msat_fee_overpay(mut self) -> Self {
self.allow_1_msat_fee_overpay = true;
self
}
}

pub fn pass_claimed_payment_along_route<'a, 'b, 'c, 'd>(args: ClaimAlongRouteArgs) -> u64 {
let ClaimAlongRouteArgs {
origin_node, expected_paths, expected_extra_fees, expected_min_htlc_overpay, skip_last,
payment_preimage: our_payment_preimage
payment_preimage: our_payment_preimage, allow_1_msat_fee_overpay,
} = args;
let claim_event = expected_paths[0].last().unwrap().node.get_and_clear_pending_events();
assert_eq!(claim_event.len(), 1);
#[allow(unused)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Then why are we adding it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only unused if mid_update_fulfill_dance isn't called, IIRC, which can happen for 2-hop paths.

let mut fwd_amt_msat = 0;
match claim_event[0] {
Event::PaymentClaimed {
purpose: PaymentPurpose::SpontaneousPayment(preimage),
Expand All @@ -2647,6 +2738,7 @@ pub fn pass_claimed_payment_along_route<'a, 'b, 'c, 'd>(args: ClaimAlongRouteArg
assert_eq!(htlcs.len(), expected_paths.len()); // One per path.
assert_eq!(htlcs.iter().map(|h| h.value_msat).sum::<u64>(), amount_msat);
expected_paths.iter().zip(htlcs).for_each(|(path, htlc)| check_claimed_htlc_channel(origin_node, path, htlc));
fwd_amt_msat = amount_msat;
},
Event::PaymentClaimed {
purpose: PaymentPurpose::InvoicePayment { .. },
Expand All @@ -2659,6 +2751,7 @@ pub fn pass_claimed_payment_along_route<'a, 'b, 'c, 'd>(args: ClaimAlongRouteArg
assert_eq!(htlcs.len(), expected_paths.len()); // One per path.
assert_eq!(htlcs.iter().map(|h| h.value_msat).sum::<u64>(), amount_msat);
expected_paths.iter().zip(htlcs).for_each(|(path, htlc)| check_claimed_htlc_channel(origin_node, path, htlc));
fwd_amt_msat = amount_msat;
valentinewallace marked this conversation as resolved.
Show resolved Hide resolved
}
_ => panic!(),
}
Expand Down Expand Up @@ -2690,8 +2783,12 @@ pub fn pass_claimed_payment_along_route<'a, 'b, 'c, 'd>(args: ClaimAlongRouteArg
per_path_msgs.push(msgs_from_ev!(&events[0]));
} else {
for expected_path in expected_paths.iter() {
// For MPP payments, we always want the message to the first node in the path.
let ev = remove_first_msg_event_to_node(&expected_path[0].node.get_our_node_id(), &mut events);
// For MPP payments, we want the fulfill message from the payee to the penultimate hop in the
// path.
let penultimate_hop_node_id = expected_path.iter().rev().skip(1).next()
.map(|n| n.node.get_our_node_id())
.unwrap_or(origin_node.node.get_our_node_id());
let ev = remove_first_msg_event_to_node(&penultimate_hop_node_id, &mut events);
per_path_msgs.push(msgs_from_ev!(&ev));
}
}
Expand All @@ -2715,15 +2812,20 @@ pub fn pass_claimed_payment_along_route<'a, 'b, 'c, 'd>(args: ClaimAlongRouteArg
{
$node.node.handle_update_fulfill_htlc(&$prev_node.node.get_our_node_id(), &next_msgs.as_ref().unwrap().0);
let mut fee = {
let per_peer_state = $node.node.per_peer_state.read().unwrap();
let peer_state = per_peer_state.get(&$prev_node.node.get_our_node_id())
.unwrap().lock().unwrap();
let channel = peer_state.channel_by_id.get(&next_msgs.as_ref().unwrap().0.channel_id).unwrap();
if let Some(prev_config) = channel.context().prev_config() {
prev_config.forwarding_fee_base_msat
} else {
channel.context().config().forwarding_fee_base_msat
}
let (base_fee, prop_fee) = {
let per_peer_state = $node.node.per_peer_state.read().unwrap();
let peer_state = per_peer_state.get(&$prev_node.node.get_our_node_id())
.unwrap().lock().unwrap();
let channel = peer_state.channel_by_id.get(&next_msgs.as_ref().unwrap().0.channel_id).unwrap();
if let Some(prev_config) = channel.context().prev_config() {
(prev_config.forwarding_fee_base_msat as u64,
prev_config.forwarding_fee_proportional_millionths as u64)
} else {
(channel.context().config().forwarding_fee_base_msat as u64,
channel.context().config().forwarding_fee_proportional_millionths as u64)
}
};
((fwd_amt_msat * prop_fee / 1_000_000) + base_fee) as u32
};

let mut expected_extra_fee = None;
Expand All @@ -2734,9 +2836,10 @@ pub fn pass_claimed_payment_along_route<'a, 'b, 'c, 'd>(args: ClaimAlongRouteArg
}
let mut events = $node.node.get_and_clear_pending_events();
assert_eq!(events.len(), 1);
expect_payment_forwarded(events.pop().unwrap(), *$node, $next_node, $prev_node,
Some(fee as u64), expected_extra_fee, false, false);
expected_total_fee_msat += fee as u64;
let actual_fee = expect_payment_forwarded(events.pop().unwrap(), *$node, $next_node, $prev_node,
Some(fee as u64), expected_extra_fee, false, false, allow_1_msat_fee_overpay);
expected_total_fee_msat += actual_fee.unwrap();
fwd_amt_msat += actual_fee.unwrap();
check_added_monitors!($node, 1);
let new_next_msgs = if $new_msgs {
let events = $node.node.get_and_clear_pending_msg_events();
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/ln/offers_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,9 @@ fn route_bolt12_payment<'a, 'b, 'c>(
// invoice contains the payment_hash but it was encrypted inside an onion message.
let amount_msats = invoice.amount_msats();
let payment_hash = invoice.payment_hash();
do_pass_along_path(
node, path, amount_msats, payment_hash, None, ev, false, false, None, false
);
let args = PassAlongPathArgs::new(node, path, amount_msats, payment_hash, ev)
.without_clearing_recipient_events();
do_pass_along_path(args);
}

fn claim_bolt12_payment<'a, 'b, 'c>(node: &Node<'a, 'b, 'c>, path: &[&Node<'a, 'b, 'c>]) {
Expand Down
8 changes: 6 additions & 2 deletions lightning/src/ln/reload_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -823,8 +823,12 @@ fn do_test_partial_claim_before_restart(persist_both_monitors: bool) {
assert_eq!(send_events.len(), 2);
let node_1_msgs = remove_first_msg_event_to_node(&nodes[1].node.get_our_node_id(), &mut send_events);
let node_2_msgs = remove_first_msg_event_to_node(&nodes[2].node.get_our_node_id(), &mut send_events);
do_pass_along_path(&nodes[0], &[&nodes[1], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), node_1_msgs, true, false, None, false);
do_pass_along_path(&nodes[0], &[&nodes[2], &nodes[3]], 15_000_000, payment_hash, Some(payment_secret), node_2_msgs, true, false, None, false);
do_pass_along_path(PassAlongPathArgs::new(&nodes[0],&[&nodes[1], &nodes[3]], 15_000_000, payment_hash, node_1_msgs)
.with_payment_secret(payment_secret)
.without_clearing_recipient_events());
do_pass_along_path(PassAlongPathArgs::new(&nodes[0], &[&nodes[2], &nodes[3]], 15_000_000, payment_hash, node_2_msgs)
.with_payment_secret(payment_secret)
.without_clearing_recipient_events());

// Now that we have an MPP payment pending, get the latest encoded copies of nodes[3]'s
// monitors and ChannelManager, for use later, if we don't want to persist both monitors.
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/shutdown_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -437,7 +437,7 @@ fn do_htlc_fail_async_shutdown(blinded_recipient: bool) {
let (_, our_payment_hash, our_payment_secret) = get_payment_preimage_hash(&nodes[2], Some(amt_msat), None);
let route_params = if blinded_recipient {
crate::ln::blinded_payment_tests::get_blinded_route_parameters(
amt_msat, our_payment_secret,
amt_msat, our_payment_secret, 1, 100000000,
nodes.iter().skip(1).map(|n| n.node.get_our_node_id()).collect(), &[&chan_2.0.contents],
&chanmon_cfgs[2].keys_manager)
} else {
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/routing/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -986,7 +986,7 @@ impl Payee {
_ => None,
}
}
fn blinded_route_hints(&self) -> &[(BlindedPayInfo, BlindedPath)] {
pub(crate) fn blinded_route_hints(&self) -> &[(BlindedPayInfo, BlindedPath)] {
valentinewallace marked this conversation as resolved.
Show resolved Hide resolved
match self {
Self::Blinded { route_hints, .. } => &route_hints[..],
Self::Clear { .. } => &[]
Expand Down
Loading