Skip to content
Closed
Show file tree
Hide file tree
Changes from 9 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
67 changes: 66 additions & 1 deletion lightning/src/routing/network_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use chain::chaininterface::{ChainError, ChainWatchInterface};
use ln::features::{ChannelFeatures, NodeFeatures};
use ln::msgs::{DecodeError,ErrorAction,LightningError,RoutingMessageHandler,NetAddress};
use ln::msgs;
use routing::router::RouteHop;
use util::ser::{Writeable, Readable, Writer};
use util::logger::Logger;

Expand Down Expand Up @@ -278,7 +279,6 @@ impl_writeable!(ChannelInfo, 0, {
announcement_message
});


/// Fees for routing via a given channel or a node
#[derive(Eq, PartialEq, Copy, Clone, Debug)]
pub struct RoutingFees {
Expand Down Expand Up @@ -428,6 +428,71 @@ impl Readable for NodeInfo {
}
}

/// Allows for updating user's network metadata.
pub trait RouteFeePenalty {
/// Gets a channel's fee penalty based on its channel_id (as stored in a NetworkGraph object).
fn get_channel_fee_penalty(&self, chan_id: u64) -> u64;
/// Informs metadata object that a route has successfully executed its payment.
fn route_succeeded(&mut self, route: Vec<RouteHop>);
/// Informs metadata object that a route has failed to execute a payment.
fn route_failed(&mut self, route: Vec<RouteHop>, failed_hop: RouteHop);
}

/// A default metadata object that is used to implement the default functionality for the
/// NetworkTracker trait. A user could make their own Metadata object and extend the
Copy link

Choose a reason for hiding this comment

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

I think there is a question of code sharing here, if people start to work on fancy scoring logic, they may want to combine them, and having each of them implement a custom UpdateMetadata they won't be able to do so.

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 see why? You could write a combiner, but ultimately we have to have a way to linearize any kind of fancy scoring logic users want into one parameter (namely fee paid) so that we can calculate a route based on it. No matter what users do, they have to be able to linearize it.

Copy link

Choose a reason for hiding this comment

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

Yes I think we agree on the need to linearize it, my point was more on dissociating linearization method (get_channel_fee_penalty) from score-feeding ones (route_succeeded/route_failed). Linearization one should be on such combiner. I don't think we have to do this now, but we may have to introduce such combiner in the future.

/// functionality of it by implementing other functions/traits for their metadata.
pub struct DefaultMetadata {
/// A lits of failed channels. Maps channel_id (as specified in the NetworkGraph object) to
/// the number of successful routes to participate before being removed from the list. All
/// channels in failed_channels are assumed to have a penalty of u64::max.
failed_channels: BTreeMap<u64, u64>,
}

impl RouteFeePenalty for DefaultMetadata {
fn get_channel_fee_penalty(&self, chan_id: u64) -> u64 {
if self.failed_channels.get(&chan_id) == None {
return 0;
} else {
return u64::max_value();
}
}

fn route_succeeded(&mut self, route: Vec<RouteHop>) {
for route_hop in route {
let chan_id = route_hop.short_channel_id;
let successes_needed = self.failed_channels.get(&chan_id);
if successes_needed == None {
self.failed_channels.insert(chan_id, 5);
} else {
let dec_successes_needed = successes_needed.unwrap() - 1;
if dec_successes_needed > 0 {
self.failed_channels.insert(chan_id, dec_successes_needed);
} else {
self.failed_channels.remove(&chan_id);
}
}
}
}

fn route_failed(&mut self, route: Vec<RouteHop>, failed_hop: RouteHop) {
for route_hop in route {
if route_hop == failed_hop {
*self.failed_channels.entry(failed_hop.short_channel_id).or_insert(5) += 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

TBH this feels a bit weird and arbitrary - you need 5 successes to un-fail a channel but once we have even 1 failure we refuse to route through a channel at all. I think maybe a more interesting approach would be to consider the time of the last failure and de-prioritize exponentially less as a failure ages, removing it entirely if we succeed. Because we cannot call current-time functions in the core library, we can't do that exactly, however something that may approximate would be to fail a channel until the next success - ie if we fail a channel, we wont use it the next time we route, but we'll use it any time after that. A user could, of course, do something better themselves by re-implementing the API. Separately, we could maybe add a feature flag for "we can call gettimeofday()".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree that it feels weird/arbitrary. I like the idea of having the exponential backoff too, but as mentioned we do not have access to time. For now, I am implementing "the opposite" where on success routing, all channels are removed from the failed_channels list, and on failure, channels have a "consecutive fails since last successful payment" count incremented.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, but because we fully-refuse to route through a channel after its failed once, we should never be able to send through it ever again if it fails once (because we'll never succeed routing through it if we never try...).

break;
}
}
}
}

/// Users store custom metadata about the network separately. This trait is implemented by users, who may use
/// the NetworkGraph to update whatever metadata they are storing about their view of the network.
pub trait NetworkTracker<T: RouteFeePenalty = DefaultMetadata> {
/// Return score for a given channel by using user-defined channel_scorers
fn calculate_minimum_fee_penalty_for_channel(&self, chan_id: u64, network_metadata: T) -> u64 {
return network_metadata.get_channel_fee_penalty(chan_id);
}
}

/// Represents the network as nodes and channels between them
#[derive(PartialEq)]
pub struct NetworkGraph {
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/routing/router.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use chain::chaininterface::ChainWatchInterface;
use ln::channelmanager;
use ln::features::{ChannelFeatures, NodeFeatures};
use ln::msgs::{DecodeError,ErrorAction,LightningError};
use routing::network_graph::{NetGraphMsgHandler, RoutingFees};
use routing::network_graph::{NetGraphMsgHandler, RoutingFees, RouteFeePenalty};
use util::ser::{Writeable, Readable};
use util::logger::Logger;

Expand Down Expand Up @@ -162,7 +162,7 @@ struct DummyDirectionalChannelInfo {
/// equal), however the enabled/disabled bit on such channels as well as the htlc_minimum_msat
/// *is* checked as they may change based on the receiving node.
pub fn get_route<C: Deref, L: Deref>(our_node_id: &PublicKey, net_graph_msg_handler: &NetGraphMsgHandler<C, L>, target: &PublicKey, first_hops: Option<&[channelmanager::ChannelDetails]>,
last_hops: &[RouteHint], final_value_msat: u64, final_cltv: u32, logger: L) -> Result<Route, LightningError> where C::Target: ChainWatchInterface, L::Target: Logger {
last_hops: &[RouteHint], final_value_msat: u64, final_cltv: u32, logger: L, net_metadata: impl RouteFeePenalty) -> Result<Route, LightningError> where C::Target: ChainWatchInterface, L::Target: Logger {
// TODO: Obviously *only* using total fee cost sucks. We should consider weighting by
// uptime/success in using a node in the past.
if *target == *our_node_id {
Expand Down Expand Up @@ -221,7 +221,7 @@ pub fn get_route<C: Deref, L: Deref>(our_node_id: &PublicKey, net_graph_msg_hand
// $directional_info.
( $chan_id: expr, $src_node_id: expr, $dest_node_id: expr, $directional_info: expr, $chan_features: expr, $starting_fee_msat: expr ) => {
//TODO: Explore simply adding fee to hit htlc_minimum_msat
if $starting_fee_msat as u64 + final_value_msat >= $directional_info.htlc_minimum_msat {
if $starting_fee_msat as u64 + final_value_msat >= $directional_info.htlc_minimum_msat + net_metadata.get_channel_fee_penalty($chan_id.clone()) {
let proportional_fee_millions = ($starting_fee_msat + final_value_msat).checked_mul($directional_info.fees.proportional_millionths as u64);
if let Some(new_fee) = proportional_fee_millions.and_then(|part| {
($directional_info.fees.base_msat as u64).checked_add(part / 1000000) })
Expand Down