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 probing_diversity_penalty_msat to the scorer parameters #3422

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

TheBlueMatt
Copy link
Collaborator

When doing background probing, its important to get a diverse view of the network graph to ensure you have as many options when pathfinding as possible.

Sadly, the naive probing we currently recommend users do does not accomplish that - using the same success-optimized pathfinder when sending as when probing results in always testing the same (good) path over and over and over again.

Instead, here, we add a probing_diversity_penalty_msat parameter to the scorer, allowing us to penalize channels for which we already have recent data. It applies a penalty which scales with the square of the inverse time since the channel was last updated, up to one day.

@TheBlueMatt TheBlueMatt marked this pull request as draft November 23, 2024 17:11
@TheBlueMatt
Copy link
Collaborator Author

Needs a test, should otherwise work, plan to start using this asap tho.

@tnull
Copy link
Contributor

tnull commented Nov 23, 2024

Instead, here, we add a probing_diversity_penalty_msat parameter to the scorer, allowing us to penalize channels for which we already have recent data. It applies a penalty which scales with the square of the inverse time since the channel was last updated, up to one day.

Hmm. I think we had previously discussed this and I expressed the need for path diversity in background probing to explore the wider network. However, I'm not sure why this should be a parameter on our regular scorer we use to send payments.

Shouldn't this rather be handled by target/path selection of the background probing software?

We expect `ChannelLiquidity` to be exactly three cache lines to
ensure the first bytes we need are all one one cache line, but in
practice its a bit more ideal for `ChannelLiquidity`s to always
start on an even cache line as x86 CPUs will often load the
neighboring cache line automatically.

Further, it looks like some versions of `rustc` on some platforms
don't pack `ChannelLiquidity` as well (in lightningdevkit#3415) and the next
commit is going to push us over three cache lines anyway.

Instead, here we calculate out the proper padding for
`ChannelLiquidity` to make it align to four 64-byte cache lines.

Should fix lightningdevkit#3415.
In the next commit we'll enable users to assign a penalty to
channels which we don't have any recent information for to better
enable probing diversity. In order to do so, we need to actually
track the last time we received new information for a channel,
which we do here.
@TheBlueMatt TheBlueMatt force-pushed the 2024-11-probing-diversity branch from bc8a318 to 3fe0c91 Compare November 23, 2024 21:02
@TheBlueMatt
Copy link
Collaborator Author

Hmm. I think we had previously discussed this and I expressed the need for path diversity in background probing to explore the wider network. However, I'm not sure why this should be a parameter on our regular scorer we use to send payments.

I'm not sure exactly how we'd build it as a separate router. I think "just do the normal thing but try to explore backup options" seems like a pretty good strategy. Doing that as a separate router/scorer would be kinda hard - it really needs access to the internal scorer fields and scorer support.

Shouldn't this rather be handled by target/path selection of the background probing software?

It still would be. No sensible use of the scorer would ever set this parameter, you'd only ever set it when background probing.

@TheBlueMatt TheBlueMatt marked this pull request as ready for review November 23, 2024 21:08
When doing background probing, its important to get a diverse view
of the network graph to ensure you have as many options when
pathfinding as possible.

Sadly, the naive probing we currently recommend users do does not
accomplish that - using the same success-optimized pathfinder when
sending as when probing results in always testing the same (good)
path over and over and over again.

Instead, here, we add a `probing_diversity_penalty_msat` parameter
to the scorer, allowing us to penalize channels for which we
already have recent data. It applies a penalty which scales with
the square of the inverse time since the channel was last updated,
up to one day.
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.22%. Comparing base (2d6720e) to head (957f93f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3422      +/-   ##
==========================================
- Coverage   89.24%   89.22%   -0.02%     
==========================================
  Files         130      130              
  Lines      106959   107035      +76     
  Branches   106959   107035      +76     
==========================================
+ Hits        95452    95501      +49     
- Misses       8718     8732      +14     
- Partials     2789     2802      +13     

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


🚨 Try these New Features:

@TheBlueMatt TheBlueMatt force-pushed the 2024-11-probing-diversity branch from 3fe0c91 to 957f93f Compare November 23, 2024 21:11
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