Skip to content

Pass EffectiveCapacity through to scorer instead of a u64 #1383

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 1 commit into from
Closed

Pass EffectiveCapacity through to scorer instead of a u64 #1383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions lightning-invoice/src/payment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
//! # use lightning::ln::channelmanager::{ChannelDetails, PaymentId, PaymentSendFailure};
//! # use lightning::ln::msgs::LightningError;
//! # use lightning::routing::scoring::Score;
//! # use lightning::routing::network_graph::NodeId;
//! # use lightning::routing::network_graph::{EffectiveCapacity, NodeId};
//! # use lightning::routing::router::{Route, RouteHop, RouteParameters};
//! # use lightning::util::events::{Event, EventHandler, EventsProvider};
//! # use lightning::util::logger::{Logger, Record};
Expand Down Expand Up @@ -90,7 +90,7 @@
//! # }
//! # impl Score for FakeScorer {
//! # fn channel_penalty_msat(
//! # &self, _short_channel_id: u64, _send_amt: u64, _chan_amt: u64, _source: &NodeId, _target: &NodeId
//! # &self, _short_channel_id: u64, _send_amt: u64, _cap: EffectiveCapacity, _source: &NodeId, _target: &NodeId
//! # ) -> u64 { 0 }
//! # fn payment_path_failed(&mut self, _path: &[&RouteHop], _short_channel_id: u64) {}
//! # fn payment_path_successful(&mut self, _path: &[&RouteHop]) {}
Expand Down Expand Up @@ -532,7 +532,7 @@ mod tests {
use lightning::ln::features::{ChannelFeatures, NodeFeatures, InitFeatures};
use lightning::ln::functional_test_utils::*;
use lightning::ln::msgs::{ChannelMessageHandler, ErrorAction, LightningError};
use lightning::routing::network_graph::NodeId;
use lightning::routing::network_graph::{EffectiveCapacity, NodeId};
use lightning::routing::router::{PaymentParameters, Route, RouteHop};
use lightning::util::test_utils::TestLogger;
use lightning::util::errors::APIError;
Expand Down Expand Up @@ -1327,7 +1327,7 @@ mod tests {

impl Score for TestScorer {
fn channel_penalty_msat(
&self, _short_channel_id: u64, _send_amt: u64, _chan_amt: u64, _source: &NodeId, _target: &NodeId
&self, _short_channel_id: u64, _send_amt: u64, _capacity: EffectiveCapacity, _source: &NodeId, _target: &NodeId
) -> u64 { 0 }

fn payment_path_failed(&mut self, actual_path: &[&RouteHop], actual_short_channel_id: u64) {
Expand Down
56 changes: 50 additions & 6 deletions lightning/src/routing/network_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,7 +758,7 @@ impl<'a> DirectedChannelInfo<'a> {
})
.or_else(|| capacity_msat.map(|capacity_msat|
EffectiveCapacity::Total { capacity_msat }))
.unwrap_or(EffectiveCapacity::Unknown)
.unwrap_or(EffectiveCapacity::Unknown { previously_used_msat: 0 })
}

/// Returns `Some` if [`ChannelUpdateInfo`] is available in the direction.
Expand Down Expand Up @@ -807,7 +807,8 @@ impl<'a> fmt::Debug for DirectedChannelInfoWithUpdate<'a> {
/// The effective capacity of a channel for routing purposes.
///
/// While this may be smaller than the actual channel capacity, amounts greater than
/// [`Self::as_msat`] should not be routed through the channel.
/// [`Self::as_msat_with_default`] should not be routed through the channel.
#[derive(Copy, Clone, PartialEq)]
pub enum EffectiveCapacity {
/// The available liquidity in the channel known from being a channel counterparty, and thus a
/// direct hop.
Expand All @@ -831,22 +832,65 @@ pub enum EffectiveCapacity {
Infinite,
/// A capacity that is unknown possibly because either the chain state is unavailable to know
/// the total capacity or the `htlc_maximum_msat` was not advertised on the gossip network.
Unknown,
Unknown {
/// An amount which should be considered "already used". Used during routing to keep track
/// of how much we're already considering routing through this channel.
previously_used_msat: u64,
Copy link
Contributor

@jkczyz jkczyz Mar 24, 2022

Choose a reason for hiding this comment

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

Hmmm... I was hoping this would be a separate parameter to the scorer since it is relevant for all variants.

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Mar 24, 2022

Choose a reason for hiding this comment

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

Hmm, good point, let me benchmark that.

Copy link
Contributor

@jkczyz jkczyz Mar 24, 2022

Choose a reason for hiding this comment

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

Sure, but its preferrable to tick something off the TODO list while we're at it rather than just fix the narrow case, no?

Not sure exactly how it would ultimately shake out, but I was thinking something along these lines: #1227 (comment). Had a rough start to this as part of some tabled work from #1227 (jkczyz@b88b37e).

Then each scorer could use the in-use amount as they wish. ProbabilisticScorer would just need to subtract it from max_liquidity_msat.

I'd prefer if we make a small fix for 0.0.106 to avoid adding more review to be honest. Also, it somewhat conflicts with some work I pulled out of #1227 but haven't PR'ed: jkczyz@a926913.

Copy link
Collaborator Author

@TheBlueMatt TheBlueMatt Mar 24, 2022

Choose a reason for hiding this comment

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

it somewhat conflicts with some work I pulled out of #1227 but haven't PR'ed: jkczyz@a926913.

Okay, sounds great! I'll close this and circle back around on the panic. I sent you the patch I'd started to build for this on discord, it looked like a performance win so probably good to incorporate it in your work.

},
}

/// The presumed channel capacity denominated in millisatoshi for [`EffectiveCapacity::Unknown`] to
/// use when making routing decisions.
pub const UNKNOWN_CHANNEL_CAPACITY_MSAT: u64 = 250_000 * 1000;

impl EffectiveCapacity {
/// Returns the effective capacity denominated in millisatoshi.
pub fn as_msat(&self) -> u64 {
#[inline]
pub(crate) fn as_msat_without_bounds(&self) -> u64 {
match self {
EffectiveCapacity::ExactLiquidity { liquidity_msat } => *liquidity_msat,
EffectiveCapacity::MaximumHTLC { amount_msat } => *amount_msat,
EffectiveCapacity::Total { capacity_msat } => *capacity_msat,
EffectiveCapacity::Infinite => u64::max_value(),
EffectiveCapacity::Unknown => UNKNOWN_CHANNEL_CAPACITY_MSAT,
EffectiveCapacity::Unknown { previously_used_msat } => UNKNOWN_CHANNEL_CAPACITY_MSAT.saturating_sub(*previously_used_msat),
}
}

/// Returns the effective capacity denominated in millisatoshi.
///
/// Returns [`UNKNOWN_CHANNEL_CAPACITY_MSAT`] minus the `previously_used_msat` for
/// [`EffectiveCapacity::Unknown`].
#[inline]
pub fn as_msat_with_default(&self) -> Option<u64> {
match self {
EffectiveCapacity::ExactLiquidity { liquidity_msat } => Some(*liquidity_msat),
EffectiveCapacity::MaximumHTLC { amount_msat } => Some(*amount_msat),
EffectiveCapacity::Total { capacity_msat } => Some(*capacity_msat),
EffectiveCapacity::Infinite => None,
EffectiveCapacity::Unknown { previously_used_msat } => Some(UNKNOWN_CHANNEL_CAPACITY_MSAT.saturating_sub(*previously_used_msat)),
}
}

/// Returns a new [`EffectiveCapacity`] which is reduced by the given number of millisatoshis
#[inline]
pub fn checked_sub(&self, msats: u64) -> Option<EffectiveCapacity> {
match self {
Self::ExactLiquidity { liquidity_msat } => match liquidity_msat.checked_sub(msats) {
Some(liquidity_msat) => Some(Self::ExactLiquidity { liquidity_msat }),
None => None,
},
Self::MaximumHTLC { amount_msat } => match amount_msat.checked_sub(msats) {
Some(amount_msat) => Some(Self::MaximumHTLC { amount_msat }),
None => None,
},
Self::Total { capacity_msat } => match capacity_msat.checked_sub(msats) {
Some(capacity_msat) => Some(Self::Total { capacity_msat }),
None => None,
},
Self::Infinite => Some(Self::Infinite),
Self::Unknown { previously_used_msat } => match previously_used_msat.checked_add(msats) {
Some(previously_used_msat) => Some(Self::Unknown { previously_used_msat }),
None => None,
},
}
}
}
Expand Down
21 changes: 11 additions & 10 deletions lightning/src/routing/router.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -854,7 +854,7 @@ where L::Target: Logger {
let short_channel_id = $candidate.short_channel_id();
let available_liquidity_msat = bookkept_channels_liquidity_available_msat
.entry(short_channel_id)
.or_insert_with(|| $candidate.effective_capacity().as_msat());
.or_insert_with(|| $candidate.effective_capacity());

// It is tricky to substract $next_hops_fee_msat from available liquidity here.
// It may be misleading because we might later choose to reduce the value transferred
Expand All @@ -863,7 +863,7 @@ where L::Target: Logger {
// fees caused by one expensive channel, but then this channel could have been used
// if the amount being transferred over this path is lower.
// We do this for now, but this is a subject for removal.
if let Some(available_value_contribution_msat) = available_liquidity_msat.checked_sub($next_hops_fee_msat) {
if let Some(available_value_contribution) = available_liquidity_msat.checked_sub($next_hops_fee_msat) {

// Routing Fragmentation Mitigation heuristic:
//
Expand All @@ -886,6 +886,7 @@ where L::Target: Logger {
final_value_msat
};
// Verify the liquidity offered by this channel complies to the minimal contribution.
let available_value_contribution_msat = available_value_contribution.as_msat_without_bounds();
let contributes_sufficient_value = available_value_contribution_msat >= minimal_value_contribution_msat;

// Do not consider candidates that exceed the maximum total cltv expiry limit.
Expand Down Expand Up @@ -1220,9 +1221,8 @@ where L::Target: Logger {
short_channel_id: hop.short_channel_id,
})
.unwrap_or_else(|| CandidateRouteHop::PrivateHop { hint: hop });
let capacity_msat = candidate.effective_capacity().as_msat();
aggregate_next_hops_path_penalty_msat = aggregate_next_hops_path_penalty_msat
.checked_add(scorer.channel_penalty_msat(hop.short_channel_id, final_value_msat, capacity_msat, &source, &target))
.checked_add(scorer.channel_penalty_msat(hop.short_channel_id, path_value_msat, candidate.effective_capacity(), &source, &target))
.unwrap_or_else(|| u64::max_value());

aggregate_next_hops_cltv_delta = aggregate_next_hops_cltv_delta
Expand Down Expand Up @@ -1382,12 +1382,13 @@ where L::Target: Logger {
let mut spent_on_hop_msat = value_contribution_msat;
let next_hops_fee_msat = payment_hop.next_hops_fee_msat;
spent_on_hop_msat += next_hops_fee_msat;
if spent_on_hop_msat == *channel_liquidity_available_msat {
if spent_on_hop_msat == channel_liquidity_available_msat.as_msat_without_bounds() {
// If this path used all of this channel's available liquidity, we know
// this path will not be selected again in the next loop iteration.
prevented_redundant_path_selection = true;
}
*channel_liquidity_available_msat -= spent_on_hop_msat;
*channel_liquidity_available_msat = channel_liquidity_available_msat
.checked_sub(spent_on_hop_msat).unwrap_or(EffectiveCapacity::ExactLiquidity { liquidity_msat: 0 });
}
if !prevented_redundant_path_selection {
// If we weren't capped by hitting a liquidity limit on a channel in the path,
Expand All @@ -1396,7 +1397,7 @@ where L::Target: Logger {
let victim_scid = payment_path.hops[(payment_path.hops.len() - 1) / 2].0.candidate.short_channel_id();
log_trace!(logger, "Disabling channel {} for future path building iterations to avoid duplicates.", victim_scid);
let victim_liquidity = bookkept_channels_liquidity_available_msat.get_mut(&victim_scid).unwrap();
*victim_liquidity = 0;
*victim_liquidity = EffectiveCapacity::ExactLiquidity { liquidity_msat: 0 };
}

// Track the total amount all our collected paths allow to send so that we:
Expand Down Expand Up @@ -1664,7 +1665,7 @@ fn add_random_cltv_offset(route: &mut Route, payment_params: &PaymentParameters,

#[cfg(test)]
mod tests {
use routing::network_graph::{NetworkGraph, NetGraphMsgHandler, NodeId};
use routing::network_graph::{EffectiveCapacity, NetworkGraph, NetGraphMsgHandler, NodeId};
use routing::router::{get_route, add_random_cltv_offset, PaymentParameters, Route, RouteHint, RouteHintHop, RouteHop, RoutingFees, DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA};
use routing::scoring::Score;
use chain::transaction::OutPoint;
Expand Down Expand Up @@ -5001,7 +5002,7 @@ mod tests {
fn write<W: Writer>(&self, _w: &mut W) -> Result<(), ::io::Error> { unimplemented!() }
}
impl Score for BadChannelScorer {
fn channel_penalty_msat(&self, short_channel_id: u64, _send_amt: u64, _capacity_msat: u64, _source: &NodeId, _target: &NodeId) -> u64 {
fn channel_penalty_msat(&self, short_channel_id: u64, _send_amt: u64, _capacity: EffectiveCapacity, _source: &NodeId, _target: &NodeId) -> u64 {
if short_channel_id == self.short_channel_id { u64::max_value() } else { 0 }
}

Expand All @@ -5019,7 +5020,7 @@ mod tests {
}

impl Score for BadNodeScorer {
fn channel_penalty_msat(&self, _short_channel_id: u64, _send_amt: u64, _capacity_msat: u64, _source: &NodeId, target: &NodeId) -> u64 {
fn channel_penalty_msat(&self, _short_channel_id: u64, _send_amt: u64, _capacity: EffectiveCapacity, _source: &NodeId, target: &NodeId) -> u64 {
if *target == self.node_id { u64::max_value() } else { 0 }
}

Expand Down
Loading