Pass RoutingFees to channel_penalty_msat

[TheBlueMatt]

This can help in some custom filtering based on fees, and there's very little cost to this - we already have the fees available! Should be as simple as just adding a new parameter to the trait function and making it compile.

[jbesraa]

so change the signature to

fn channel_penalty_msat(
		&self, short_channel_id: u64, source: &NodeId, target: &NodeId, usage: ChannelUsage, score_params: &Self::ScoreParams, routing_fees: &RoutingFees
	) -> u64;

and then pass RoutingFees { base_msat: 0, proportional_millionths: 0 } to the different tests and so on?
are we going to actually use it somewhere in the implementation(s) or is it for users who will want to use if the implement the trait?

[jkczyz]

Should we rather add a single field to ChannelUsage with the fees charged (base + proportional * amount) for the given amount? A scorer shouldn't care about how the fees are broken down nor need to calculate them for the amount, IMO.

@jbesraa also noticed that TestRouter is set up with expected Routes, so either tests would need to pass RoutingFees for all hops in the Route (across all paths) or just set the base fee to the fees taken, I suppose.

[TheBlueMatt]

Should we rather add a single field to ChannelUsage with the fees charged (base + proportional * amount) for the given amount? A scorer shouldn't care about how the fees are broken down nor need to calculate them for the amount, IMO.

I'm open to either, though I don't really buy that there's anything a scorer shouldn't care about - what if a user decides that nodes with zero base fee tend to be less balanced overall as they get rebalancing traffic through them (just speculating, though I honestly wouldn't be surprised) and wants to score based on it?

[jkczyz]

I don't have a strong opinion either. But it has the potential to be a little confusing given the context. These are the fees advertised by the channel, but the context is the usage of the channel for a particular amount.

I guess we could always do both (possibly later). The need for the fees charged in ChannelUsage was something that we previously talked about for combining fees and penalty within the scorer for the cost function. Though I can't recall all the details of why this was important. Was just something I assumed (probably wrongly) that this issue would help resolve.

[TheBlueMatt]

I don't have a strong opinion either. But it has the potential to be a little confusing given the context. These are the fees advertised by the channel, but the context is the usage of the channel for a particular amount.

I mean its pretty trivial to just go ahead and provide both now, would that make it less confusing to you? We could alternatively, instead of just providing a RoutingFees, provide some kind of ChannelInfo struct which wraps it, or even expose the full CandidateRouteHop enum itself (which would imply removing the effective_capacity field from ChannelUsage and getting it via the enum.

The need for the fees charged in ChannelUsage was something that we previously talked about for combining fees and penalty within the scorer for the cost function. Though I can't recall all the details of why this was important. Was just something I assumed (probably wrongly) that this issue would help resolve.

Ah, yes, we'd previously talked about removing all the fee + score aggregation in the router and just giving that info to the scorer and letting the scorer use the fee information (or not!) to build the aggregate score. I was actually not looking to address that here, the issue is I wanted to do more aggressive filtering by per-hop fess locally, as an easy way to get #2417, but without the the weird path-missing behavior that you kinda get stuck with with any non-backtracking version of second-parameter limiting in Dijkstra's. In general, ISTM in general that any info we already have while doing routing should kinda just be passed to the scorer cause users may want it for any number of hairbrained reasons.

[jkczyz]

I mean its pretty trivial to just go ahead and provide both now, would that make it less confusing to you?

Yeah, potentially or just make sure the RoutingFees parameter is well documented.

We could alternatively, instead of just providing a RoutingFees, provide some kind of ChannelInfo struct which wraps it,

Probably the Directed version so the Score implementation doesn't need to think about it, but that is only for channels in the NetworkGraph.

or even expose the full CandidateRouteHop enum itself (which would imply removing the effective_capacity field from ChannelUsage and getting it via the enum.

Do we consider the Score interface specific to DefaultRouter. Using CandidateRouteHop would tie the Score interface to that particular Router implementation.

Ah, yes, we'd previously talked about removing all the fee + score aggregation in the router and just giving that info to the scorer and letting the scorer use the fee information (or not!) to build the aggregate score. I was actually not looking to address that here, the issue is I wanted to do more aggressive filtering by per-hop fess locally, as an easy way to get #2417, but without the the weird path-missing behavior that you kinda get stuck with with any non-backtracking version of second-parameter limiting in Dijkstra's. In general, ISTM in general that any info we already have while doing routing should kinda just be passed to the scorer cause users may want it for any number of hairbrained reasons.

Yeah, that's fair assuming we consider Score a detail of DefaultRouter. FWIW, any scorer with a NetworkGraph can look up the fees, but that is another lookup that could be avoided, of course, and only applies to announced channels.

[TheBlueMatt]

Do we consider the Score interface specific to DefaultRouter. Using CandidateRouteHop would tie the Score interface to that particular Router implementation.

Hmm? CandidateRouteHop is used for any routing in find_route, I guess you meant do we consider it specific to our find_route/router in general? I think so, if someone has a totally different router they'll presumably have a totally different scorer.

[jkczyz]

Gotcha. I'd be a bit more inclined to settle on CandidateRouteHop eventually, if we're fine exposing it. Could even add source and target methods to reduce the number of parameters being passed to channel_penalty_msat to just three: candidate, usage, and score_params.

Forming CandidateRouteHops for the scoring tests will be a bit of work, though. Maybe a couple helpers would do. One for the underlying hop data and another mapping them to appropriate enum variants, which hold references.

[TheBlueMatt]

Yea, I think that makes sense - want to give that a go @jbesraa?

[jbesraa]

yup yup

[jbesraa]

for implementing the source and target functions we would need a bit more info in CandidateRouteHop.
is this the correct way forward?

|   14 pub enum CandidateRouteHop<'a> {
    13         /// A hop from the payer, where the outbound liquidity is known.
    12         FirstHop {
    11                 details: &'a ChannelDetails,
|   10                 node_id: &'a NodeId
     9         },
     8         /// A hop found in the [`ReadOnlyNetworkGraph`], where the channel capacity may be unknown.
     7         PublicHop {
     6                  info: DirectedChannelInfo<'a>,
     5                  short_channel_id: u64,
     4         },
     3         /// A hop to the payee found in the BOLT 11 payment invoice, though not necessarily a direct
     2         /// channel.
     1         PrivateHop {
  971                   hint: &'a RouteHintHop,
|    1                  target_node_id: &'a NodeId,
     2         },
     3         /// The payee's identity is concealed behind blinded paths provided in a BOLT 12 invoice.
     4         Blinded {
     5                  hint: &'a (BlindedPayInfo, BlindedPath),
     6                  hint_idx: usize,
     7         },
     8         /// Similar to [`Self::Blinded`], but the path here has 1 blinded hop. `BlindedPayInfo` provided
     9         /// for 1-hop blinded paths is ignored because it is meant to apply to the hops *between* the
    10         /// introduction node and the destination. Useful for tracking that we need to include a blinded
    11         /// path at the end of our [`Route`].
    12         OneHopBlinded {
    13                 hint: &'a (BlindedPayInfo, BlindedPath),
    14                 hint_idx: usize,
    15         },
    16 }

| 1080         fn source(&self) -> &NodeId {
|    1                 match self {
|    2                         CandidateRouteHop::FirstHop { details, node_id } => node_id,
|    3                         CandidateRouteHop::PublicHop { info, .. } => &info.channel.node_one.into(),
|    4                         CandidateRouteHop::PrivateHop { hint, ..} => &hint.src_node_id.into(),
|    5                         CandidateRouteHop::Blinded { hint, .. } => &hint.1.introduction_node_id.into(),
|    6                         CandidateRouteHop::OneHopBlinded { hint, .. } => &hint.1.introduction_node_id.into()
|    7                 }
|    8         }
|    9         fn target(&self) -> &NodeId {
|   10                 match self {
|   11                         CandidateRouteHop::FirstHop { details, .. } => &details.counterparty.node_id.into(),
|   12                         CandidateRouteHop::PublicHop { info, .. } => &info.channel.node_two.into(),
|   13                         CandidateRouteHop::PrivateHop { hint, target_node_id } => target_node_id,
|   14                         CandidateRouteHop::Blinded { hint, .. } => &hint.1.blinding_point.into(),
|   15                         CandidateRouteHop::OneHopBlinded { hint, .. } => &hint.1.blinding_point.into()
|   16                 }
|   17         }

[jkczyz]

Yeah, though I think we may need to have target return an Option in the blinded cases, as we don't know the target node.

[jbesraa]

yea was wondering about that, why then the current channel_penalty_msat is not taking an Option for target?

[jbesraa]

so the channel_penalty_msat is not expecting Option for channel_id and target, but in CandidateRouteHop we do define them as Option

so for exmaple in this impl i get the errors marked as >>
what should I do in this case?

 19 impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> ScoreLookUp for ProbabilisticScorerUsingTime<G, L, T> where L::Target: Logger {
    18         type ScoreParams = ProbabilisticScoringFeeParameters;
    17         fn channel_penalty_msat(
|   16                 &self, candidate: CandidateRouteHop, usage: ChannelUsage, score_params: &ProbabilisticScoringFeeParameters
    15         ) -> u64 {
>>  14                 if let Some(penalty) = score_params.manual_node_penalties.get(candidate.target()) {
    13                         return *penalty;
    12                 }
    11
    10                 let base_penalty_msat = score_params.base_penalty_msat.saturating_add(
     9                         score_params.base_penalty_amount_multiplier_msat
     8                                 .saturating_mul(usage.amount_msat) / BASE_AMOUNT_PENALTY_DIVISOR);
     7
     6                 let mut anti_probing_penalty_msat = 0;
     5                 match usage.effective_capacity {
     4                         EffectiveCapacity::ExactLiquidity { liquidity_msat: amount_msat } |
     3                                 EffectiveCapacity::HintMaxHTLC { amount_msat } =>
     2                         {
     1                                 if usage.amount_msat > amount_msat {
  1207                                         return u64::max_value();
     1                                 } else {
     2                                         return base_penalty_msat;
     3                                 }
     4                         },
     5                         EffectiveCapacity::Total { capacity_msat, htlc_maximum_msat } => {
     6                                 if htlc_maximum_msat >= capacity_msat/2 {
     7                                         anti_probing_penalty_msat = score_params.anti_probing_penalty_msat;
     8                                 }
     9                         },
    10                         _ => {},
    11                 }
    12
    13                 let amount_msat = usage.amount_msat;
    14                 let capacity_msat = usage.effective_capacity.as_msat();
    15                 let inflight_htlc_msat = usage.inflight_htlc_msat;
    16                 self.channel_liquidities
>>  17                         .get(&candidate.short_channel_id())
    18                         .unwrap_or(&ChannelLiquidity::new())
>>  19                         .as_directed(candidate.source(), candidate.target(), inflight_htlc_msat, capacity_msat, self.decay_params)
    20                         .penalty_msat(amount_msat, score_params)
    21                         .saturating_add(anti_probing_penalty_msat)
    22                         .saturating_add(base_penalty_msat)
    23         }
    24 }
    

[jkczyz]

Hmm... I guessing the router will never call the scorer in those cases. But it's weird that channel_penalty_msat would need to deal with those cases even if they never occur. @TheBlueMatt What do you think?

[TheBlueMatt]

Oops, I think the current code re: blinded paths is kinda buggy, it currently calls the scorer with maybe_dummy_payee_node_id, but that just gives the scorer nonsense data and expects it to score based on that. We should really be wrapping the target in an Option. The scorer we have can then simply ignore it - if we have None just return 0 and move on.