Skip to content

Log penalty info in route-success line #1300

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

Closed
wants to merge 3 commits into from
Closed

Log penalty info in route-success line #1300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9914bac
Make selected_paths be a vector of tuple
jurvis Feb 9, 2022
9752595
Add path_penalties as a field to Route struct
jurvis Feb 9, 2022
0acc230
Log penalty for each path when logging Route
jurvis Feb 9, 2022
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
4 changes: 4 additions & 0 deletions lightning-invoice/src/payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,7 @@ mod tests {
}],
],
payment_params: None,
path_penalties: Vec::with_capacity(2),
}
}

Expand Down Expand Up @@ -1544,6 +1545,7 @@ mod tests {
}],
],
payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id())),
path_penalties: Vec::with_capacity(2),
};
let router = ManualRouter(RefCell::new(VecDeque::new()));
router.expect_find_route(Ok(route.clone()));
Expand Down Expand Up @@ -1588,6 +1590,7 @@ mod tests {
}],
],
payment_params: Some(PaymentParameters::from_node_id(nodes[1].node.get_our_node_id())),
path_penalties: Vec::with_capacity(1),
};
let router = ManualRouter(RefCell::new(VecDeque::new()));
router.expect_find_route(Ok(route.clone()));
Expand Down Expand Up @@ -1669,6 +1672,7 @@ mod tests {
}]
],
payment_params: Some(PaymentParameters::from_node_id(nodes[2].node.get_our_node_id())),
path_penalties: Vec::with_capacity(2)
};
let router = ManualRouter(RefCell::new(VecDeque::new()));
router.expect_find_route(Ok(route.clone()));
Expand Down
4 changes: 2 additions & 2 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1081,7 +1081,7 @@ fn fake_network_test() {
});
hops[1].fee_msat = chan_4.1.contents.fee_base_msat as u64 + chan_4.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000;
hops[0].fee_msat = chan_3.0.contents.fee_base_msat as u64 + chan_3.0.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000;
let payment_preimage_1 = send_along_route(&nodes[1], Route { paths: vec![hops], payment_params: None }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0;
let payment_preimage_1 = send_along_route(&nodes[1], Route { paths: vec![hops], payment_params: None, path_penalties: Vec::with_capacity(1) }, &vec!(&nodes[2], &nodes[3], &nodes[1])[..], 1000000).0;

let mut hops = Vec::with_capacity(3);
hops.push(RouteHop {
Expand Down Expand Up @@ -1110,7 +1110,7 @@ fn fake_network_test() {
});
hops[1].fee_msat = chan_2.1.contents.fee_base_msat as u64 + chan_2.1.contents.fee_proportional_millionths as u64 * hops[2].fee_msat as u64 / 1000000;
hops[0].fee_msat = chan_3.1.contents.fee_base_msat as u64 + chan_3.1.contents.fee_proportional_millionths as u64 * hops[1].fee_msat as u64 / 1000000;
let payment_hash_2 = send_along_route(&nodes[1], Route { paths: vec![hops], payment_params: None }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1;
let payment_hash_2 = send_along_route(&nodes[1], Route { paths: vec![hops], payment_params: None, path_penalties: Vec::with_capacity(1) }, &vec!(&nodes[3], &nodes[2], &nodes[1])[..], 1000000).1;

// Claim the rebalances...
fail_payment(&nodes[1], &vec!(&nodes[3], &nodes[2], &nodes[1])[..], payment_hash_2);
Expand Down
1 change: 1 addition & 0 deletions lightning/src/ln/onion_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,7 @@ mod tests {
},
]],
payment_params: None,
path_penalties: Vec::with_capacity(1),
};

let session_priv = SecretKey::from_slice(&hex::decode("4141414141414141414141414141414141414141414141414141414141414141").unwrap()[..]).unwrap();
Expand Down
33 changes: 27 additions & 6 deletions lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,9 @@ pub struct Route {
///
/// [`Event::PaymentPathFailed`]: crate::util::events::Event::PaymentPathFailed
pub payment_params: Option<PaymentParameters>,
/// List of penalties corresponding to the paths in the `paths` vector.
/// This is populated by taking the last [`PathBuildingHop`] of every path found in [`find_route`]
pub path_penalties: Vec<u64>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, do we care about actually exposing this programmatically? Presumably the user can just call their own scorer immediately after getting the route if they really want this info. I think just logging it may be sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @TheBlueMatt, thanks for taking a look! that is a good suggestion -- I'll change my implementation. I will take another stab over the weekend :)

}

pub(crate) trait RoutePath {
Expand Down Expand Up @@ -121,6 +124,12 @@ impl Writeable for Route {
hop.write(writer)?;
}
}

(self.path_penalties.len() as u64).write(writer)?;
for penalty in self.path_penalties.iter() {
penalty.write(writer)?;
}

write_tlv_fields!(writer, {
(1, self.payment_params, option),
});
Expand All @@ -133,6 +142,8 @@ impl Readable for Route {
let _ver = read_ver_prefix!(reader, SERIALIZATION_VERSION);
let path_count: u64 = Readable::read(reader)?;
let mut paths = Vec::with_capacity(cmp::min(path_count, 128) as usize);
let mut path_penalties: Vec<u64> = Vec::with_capacity(paths.len());

for _ in 0..path_count {
let hop_count: u8 = Readable::read(reader)?;
let mut hops = Vec::with_capacity(hop_count as usize);
Expand All @@ -141,11 +152,17 @@ impl Readable for Route {
}
paths.push(hops);
}

let path_penalty_count: u64 = Readable::read(reader)?;
for _ in 0..path_penalty_count {
path_penalties.push(Readable::read(reader)?);
}

let mut payment_params = None;
read_tlv_fields!(reader, {
(1, payment_params, option),
});
Ok(Route { paths, payment_params })
Ok(Route { paths, payment_params, path_penalties })
}
}

Expand Down Expand Up @@ -1484,7 +1501,7 @@ where L::Target: Logger {
// Step (9).
// Select the best route by lowest total fee.
drawn_routes.sort_by_key(|paths| paths.iter().map(|path| path.get_total_fee_paid_msat()).sum::<u64>());
let mut selected_paths = Vec::<Vec<Result<RouteHop, LightningError>>>::new();
let mut selected_paths = Vec::<(Vec<Result<RouteHop, LightningError>>, u64)>::new();
for payment_path in drawn_routes.first().unwrap() {
let mut path = payment_path.hops.iter().map(|(payment_hop, node_features)| {
Ok(RouteHop {
Expand All @@ -1501,21 +1518,23 @@ where L::Target: Logger {
path.iter_mut().rev().fold(final_cltv_expiry_delta, |prev_cltv_expiry_delta, hop| {
core::mem::replace(&mut hop.as_mut().unwrap().cltv_expiry_delta, prev_cltv_expiry_delta)
});
selected_paths.push(path);
selected_paths.push((path, payment_path.hops.last().unwrap().0.path_penalty_msat));
}

if let Some(features) = &payment_params.features {
for path in selected_paths.iter_mut() {
if let Ok(route_hop) = path.last_mut().unwrap() {
if let Ok(route_hop) = path.0.last_mut().unwrap() {
route_hop.node_features = features.to_context();
}
}
}

let route = Route {
paths: selected_paths.into_iter().map(|path| path.into_iter().collect()).collect::<Result<Vec<_>, _>>()?,
path_penalties: selected_paths.iter().map(|(_, penalty)| *penalty).collect(),
paths: selected_paths.into_iter().map(|(path, _)| path.into_iter().collect()).collect::<Result<Vec<_>, _>>()?,
payment_params: Some(payment_params.clone()),
};

log_info!(logger, "Got route to {}: {}", payment_params.payee_pubkey, log_route!(route));
Ok(route)
}
Expand Down Expand Up @@ -4795,6 +4814,7 @@ mod tests {
},
]],
payment_params: None,
path_penalties: Vec::with_capacity(1)
};

assert_eq!(route.get_total_fees(), 250);
Expand Down Expand Up @@ -4828,6 +4848,7 @@ mod tests {
},
]],
payment_params: None,
path_penalties: Vec::with_capacity(2)
};

assert_eq!(route.get_total_fees(), 200);
Expand All @@ -4839,7 +4860,7 @@ mod tests {
// In an earlier version of `Route::get_total_fees` and `Route::get_total_amount`, they
// would both panic if the route was completely empty. We test to ensure they return 0
// here, even though its somewhat nonsensical as a route.
let route = Route { paths: Vec::new(), payment_params: None };
let route = Route { paths: Vec::new(), payment_params: None, path_penalties: Vec::new() };

assert_eq!(route.get_total_fees(), 0);
assert_eq!(route.get_total_amount(), 0);
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/util/macro_logger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ pub(crate) struct DebugRoute<'a>(pub &'a Route);
impl<'a> core::fmt::Display for DebugRoute<'a> {
fn fmt(&self, f: &mut core::fmt::Formatter) -> Result<(), core::fmt::Error> {
for (idx, p) in self.0.paths.iter().enumerate() {
writeln!(f, "path {}:", idx)?;
writeln!(f, "path {} (penalty: {}):", idx, self.0.path_penalties[idx])?;
for h in p.iter() {
writeln!(f, " node_id: {}, short_channel_id: {}, fee_msat: {}, cltv_expiry_delta: {}", log_pubkey!(h.pubkey), h.short_channel_id, h.fee_msat, h.cltv_expiry_delta)?;
}
Expand Down