Skip to content

Skip channels used for probing based on available liquidity - #156

Merged
tnull merged 1 commit into
lightningdevkit:mainfrom
tnull:2023-08-refuse-large-probes
Aug 17, 2023
Merged

Skip channels used for probing based on available liquidity#156
tnull merged 1 commit into
lightningdevkit:mainfrom
tnull:2023-08-refuse-large-probes

Conversation

@tnull

@tnulltnull commented Aug 16, 2023•
edited
Loading

Copy link
Copy Markdown
Collaborator

Fixes#155.

As pre-flight probes might take up some of the available liquidity, we here introduce that channels whose available liquidity is less than the required amount times Config::probing_liquidity_limit_multiplier won't be used to send pre-flight probes.

(cc @TonyGiorgio)

@tnull
tnull requested a review from jkczyzAugust 16, 2023 08:33
@tnull
tnullforce-pushed the 2023-08-refuse-large-probes branch 4 times, most recently from 2dfa7bf to 9c92be3CompareAugust 16, 2023 09:38
@tnulltnull changed the title Pre-filter channels used for probing based on available liquiditySkip channels used for probing based on available liquidityAug 16, 2023
@tnull
tnullforce-pushed the 2023-08-refuse-large-probes branch from 9c92be3 to 7d52209CompareAugust 16, 2023 10:25
@moneyball
moneyball self-requested a review August 16, 2023 13:32
@moneyball

moneyball commented Aug 16, 2023

Copy link
Copy Markdown

ACK

moneyball
moneyball previously approved these changes Aug 16, 2023
View reviewed changes
jkczyz
jkczyz reviewed Aug 16, 2023
View reviewed changes
Comment threadsrc/lib.rs Outdated
G8XSU
G8XSU reviewed Aug 16, 2023
View reviewed changes
Comment threadsrc/lib.rs
Comment threadsrc/lib.rs Outdated
Comment threadsrc/lib.rs Outdated
@TheBlueMatt

TheBlueMatt commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

Since ChannelManager upstream has a router in it, do we want to upstream the core guts here?

@tnull

tnull commented Aug 16, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Since ChannelManager upstream has a router in it, do we want to upstream the core guts here?

Sure, happy to.

@G8XSU

G8XSU commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

So are we closing this PR in favor of upstream PR or we will upstream separately later?

@tnull

tnull commented Aug 16, 2023

Copy link
Copy Markdown
CollaboratorAuthor

So are we closing this PR in favor of upstream PR or we will upstream separately later?

Nah, as we'd want to expose the same API anyways I'd just land this and migrate to use upstream's version after 0.0.117 is released.

G8XSU
G8XSU reviewed Aug 16, 2023
View reviewed changes

@G8XSUG8XSU left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM for squash.

Comment threadsrc/lib.rs Outdated
@G8XSU

G8XSU commented Aug 16, 2023

Copy link
Copy Markdown
Contributor

Do we want to add testcases for this ?

jkczyz
jkczyz reviewed Aug 16, 2023
View reviewed changes
Comment threadsrc/lib.rs Outdated
Comment threadsrc/lib.rs Outdated
Comment on lines 1419 to 1420
if *remaining_liquidity
< path_value * self.config.probing_liquidity_limit_multiplier

@jkczyzjkczyzAug 16, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You'll have the same problem as before because the way the division is moved to the other side of the inequality as a multiplication. From the previous example:

remaining_liquidity: 100, 81, 62
path_value * self.config.probing_liquidity_limit_multiplier: 19 * 3 = 57

Time to write a test? 😅

@tnulltnullAug 17, 2023•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

Grrr, my bad. 🤦‍♂️
Now dropped the commit in question and replaced it with the additive approach you originally suggested.

In regard to testing: as this requires a non-trivial topology I took this as an opportunity to finally introduce a multi-hop testing scenario with #157 based on which I'll be able to check that the probe will indeed not obstruct payment sending. However, still running into some gossip issues there (as these need to be announced channels) which I'm investigating. Happy to wait until this is resolved before landing this, or landing this as is and adding testing later, your call. I'll also add unit tests when upstreaming this logic.

@jkczyzjkczyzAug 17, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm fine with testing upstream. Indifferent on whether you want to land this first.

@tnull
tnullforce-pushed the 2023-08-refuse-large-probes branch from 2f4690e to bd9f1aeCompareAugust 17, 2023 13:49
@tnull

tnull commented Aug 17, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Do we want to add testcases for this ?

Yes, however, see #156 (comment).

jkczyz
jkczyz reviewed Aug 17, 2023
View reviewed changes
Comment threadsrc/lib.rs Outdated
jkczyz
jkczyz reviewed Aug 17, 2023
View reviewed changes

@jkczyzjkczyz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Feel free to squash.

@tnull
Skip channels used for probing based on available liquidity
758f91b
As pre-flight probes might take up some of the available liquidity, we
here introduce that channels whose available liquidity is less than the
required amount times `Config::probing_liquidity_limit_multiplier` won't
be used to send pre-flight probes.
@tnull
tnullforce-pushed the 2023-08-refuse-large-probes branch from bd9f1ae to 758f91bCompareAugust 17, 2023 16:57
@tnull

tnull commented Aug 17, 2023

Copy link
Copy Markdown
CollaboratorAuthor

Feel free to squash.

Squashed without further changes.

@tnull
tnull merged commit a9bb2e9 into lightningdevkit:mainAug 17, 2023
@tnulltnull mentioned this pull request Aug 21, 2023
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jkczyzjkczyzjkczyz approved these changes
@moneyballmoneyballmoneyball left review comments
+1 more reviewer
@G8XSUG8XSUG8XSU left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add heuristic to LDK Node code

5 participants

@tnull@moneyball@TheBlueMatt@G8XSU@jkczyz