-
Notifications
You must be signed in to change notification settings - Fork 345
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
Fix division by zero in ProbabilisticScorer
#2072
Fix division by zero in ProbabilisticScorer
#2072
Conversation
ProbabilisticScorer takes a ChannelUsage when computing a penalty for a channel. The formula for calculating the liquidity penalty reduces the maximum capacity by the amount of in-flight HTLCs (available capacity) and adds one to prevent division by zero. However, since the available capacity is passed to DirectedChannelLiquidity as the capacity, other penalty formulas may use the available (i.e., reduced) capacity inadvertently. In practice, this has two ramifications for the historical liquidity penalty computation: 1. The bucket formula doesn't have a consistent denominator for a given channel. 2. The bucket formula may divide by zero when the in-flight HTLC amount equals or exceeds the effective capacity. Fixing this involves only using the available capacity when appropriate.
Even when there is no change in min/max liquidity knowledge, tracking should still be updated to include the additional data point.
Since a node may announce that the htlc_maximum_msat of a channel is zero, adding one to the denominator in the bucket formulas will prevent the panic from ever happening. While the routing algorithm may never select such a channel to score, this precaution may still be useful in case the algorithm changes or if the scorer is used with a different routing algorithm.
@@ -1022,7 +1033,16 @@ impl<L: Deref<Target = u64>, BRT: Deref<Target = HistoricalBucketRangeTracker>, | |||
|
|||
if params.historical_liquidity_penalty_multiplier_msat != 0 || | |||
params.historical_liquidity_penalty_amount_multiplier_msat != 0 { | |||
let payment_amt_64th_bucket = amount_msat * 64 / self.capacity_msat; | |||
let payment_amt_64th_bucket = if amount_msat < u64::max_value() / 64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future I think we should add the in-flight amount to the amount here to properly account for the total transiting a channel.
@@ -1234,13 +1256,13 @@ impl<G: Deref<Target = NetworkGraph<L>>, L: Deref, T: Time> Score for Probabilis | |||
self.channel_liquidities | |||
.entry(hop.short_channel_id) | |||
.or_insert_with(ChannelLiquidity::new) | |||
.as_directed_mut(source, &target, capacity_msat, &self.params) | |||
.as_directed_mut(source, &target, 0, capacity_msat, &self.params) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a followup we should consider the in-flight amounts on failure as well, I think.
ProbabilisticScorer takes a ChannelUsage when computing a penalty for a
channel. The formula for calculating the liquidity penalty reduces the
maximum capacity by the amount of in-flight HTLCs (available capacity)
and adds one to prevent division by zero.
However, since the available capacity is passed to
DirectedChannelLiquidity as the capacity, other penalty formulas may use
the available (i.e., reduced) capacity inadvertently. In practice, this
has two ramifications for the historical liquidity penalty computation:
channel.
equals or exceeds the effective capacity.
Fixing this involves only using the available capacity when appropriate.
Since a node may announce that the htlc_maximum_msat of a channel is
zero, adding one to the denominator in the bucket formulas will prevent
the panic from ever happening. While the routing algorithm may never
select such a channel to score, this precaution may still be useful in
case the algorithm changes or if the scorer is used with a different
routing algorithm.