Skip to content

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