Skip to content
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

Add the ability to fetch a probability from live liquidity bounds #3420

Merged

Conversation

TheBlueMatt
Copy link
Collaborator

We already expose the estimated success probability from the historical liquidity bounds from
historical_estimated_payment_success_probability, but we don't do that for the live liquidity bounds.

Here we add a live_estimated_payment_success_probability which exposes the probability result from the live liquidity bounds as well.

@TheBlueMatt
Copy link
Collaborator Author

Also pushed a new method which I actually need for the simulator.

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 40.35088% with 34 lines in your changes missing coverage. Please review.

Project coverage is 90.41%. Comparing base (8da30df) to head (399c151).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/routing/scoring.rs 40.35% 34 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3420      +/-   ##
==========================================
+ Coverage   89.61%   90.41%   +0.80%     
==========================================
  Files         129      130       +1     
  Lines      105506   113457    +7951     
  Branches   105506   113457    +7951     
==========================================
+ Hits        94544   102585    +8041     
+ Misses       8208     8150      -58     
+ Partials     2754     2722      -32     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Dec 2, 2024
@TheBlueMatt
Copy link
Collaborator Author

Tagging 0.1 just because I want this for the sim replay and don't want to rely on a branch there.

Copy link
Contributor

@jkczyz jkczyz left a comment

Choose a reason for hiding this comment

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

LGTM. Feel free to squash.

We already expose the estimated success probability from the
historical liquidity bounds from
`historical_estimated_payment_success_probability`, but we don't
do that for the live liquidity bounds.

Here we add a `live_estimated_payment_success_probability` which
exposes the probability result from the live liquidity bounds as
well.
`historical_estimated_payment_success_probability` exposes the
historical success probability estimator publicly, but only allows
fetching data from channels where we have sufficient information.

In the previous commit,
`live_estimated_payment_success_probability` was added to enable
querying the live bounds success probability estimator.

Sadly, while the historical success probability estimator falls
back to the live probability estimator, it does so with a different
final parameter to `success_probability`, making
`live_estimated_payment_success_probability` not useful for
calculating the actual historical model output when we have
insufficient data.

Instead, here, we add a new parameter to
`historical_estimated_payment_success_probability` which
determines whether it will return fallback data from the live
model instead.
@TheBlueMatt TheBlueMatt force-pushed the 2024-11-live-success-prob branch from 0b39756 to d3efe5c Compare December 2, 2024 16:45
@TheBlueMatt
Copy link
Collaborator Author

Squashed without further changes.

@TheBlueMatt
Copy link
Collaborator Author

Just landing this. Its a new public method that, if its wrong, just means peoples' stats will be wrong. Its not actually used when sending payments. Its also trivial.

@TheBlueMatt TheBlueMatt merged commit e1e8ce0 into lightningdevkit:main Dec 3, 2024
12 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants