Skip to content

Fix per-path minimal value contribution during route finding #1526

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

Merged

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Jun 7, 2022

Fixes #1520

This PR ensures that we don't return an MPP route with more paths than ChannelManager would actually send payments over.

@TheBlueMatt
Copy link
Collaborator

SGTM, could use a test, though, IMO.

@TheBlueMatt TheBlueMatt added this to the 0.0.108 milestone Jun 7, 2022
@tnull tnull force-pushed the 2022-06-fix-minimal-value-contrib branch from 5baea6e to be095a2 Compare June 9, 2022 11:51
@tnull
Copy link
Contributor Author

tnull commented Jun 9, 2022

Since fixating the maximum number of MPP payment paths at 10 seemed at the same time rather arbitrary but also a sane default, I now opted to make max_mpp_path_count a user configurable option still defaulting to 10. This also allows me to choose different values during testing.

The minimal_value_contribution is currently however still depending on it (value/max_mpp_path_count) to ensure that we never return routes that violate the max path count limit.

@tnull tnull force-pushed the 2022-06-fix-minimal-value-contrib branch from a8865ca to 57cf608 Compare June 9, 2022 13:21
@tnull
Copy link
Contributor Author

tnull commented Jun 9, 2022

Squashed commits.

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #1526 (4617555) into main (6e00c28) will increase coverage by 0.02%.
The diff coverage is 90.62%.

❗ Current head 4617555 differs from pull request most recent head 3380687. Consider uploading reports for the commit 3380687 to get more accurate results

@@            Coverage Diff             @@
##             main    #1526      +/-   ##
==========================================
+ Coverage   90.95%   90.97%   +0.02%     
==========================================
  Files          80       80              
  Lines       43431    43661     +230     
  Branches    43431    43661     +230     
==========================================
+ Hits        39503    39722     +219     
- Misses       3928     3939      +11     
Impacted Files Coverage Δ
lightning/src/ln/channelmanager.rs 84.34% <ø> (+0.01%) ⬆️
lightning/src/routing/router.rs 92.46% <90.62%> (-0.01%) ⬇️
lightning/src/ln/functional_tests.rs 96.91% <0.00%> (-0.24%) ⬇️
lightning/src/ln/functional_test_utils.rs 96.65% <0.00%> (+1.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e00c28...3380687. Read the comment docs.

wpaulino
wpaulino previously approved these changes Jun 9, 2022
Copy link
Collaborator

@TheBlueMatt TheBlueMatt 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.

@tnull tnull force-pushed the 2022-06-fix-minimal-value-contrib branch from 3380687 to 1dfabcb Compare June 13, 2022 16:27
@tnull
Copy link
Contributor Author

tnull commented Jun 13, 2022

Squashed.

@TheBlueMatt TheBlueMatt merged commit 4356809 into lightningdevkit:main Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Router allows paths with ~useless value contributions
4 participants