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

Consider maximum path length during path finding. #1476

Merged
merged 1 commit into from
May 18, 2022

Conversation

tnull
Copy link
Contributor

@tnull tnull commented May 11, 2022

With this PR, we only consider returning paths from get_route which are shorter than our maximum path length estimate.

Rationale as given in the comment:

	// In the legacy onion format, the maximum number of hops used to be a fixed value of 20.
	// However, in the TLV onion format, there is no fixed maximum length, but the `hop_payloads`
	// field is always 1300 bytes. As the `tlv_payload` for each hop may vary in length, we have to
	// estimate how many hops the route may have so that it actually fits the `hop_payloads` field.

Fixes #1371.

@tnull tnull marked this pull request as draft May 11, 2022 07:19
@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #1476 (87c9684) into main (342698f) will increase coverage by 0.14%.
The diff coverage is 89.53%.

@@            Coverage Diff             @@
##             main    #1476      +/-   ##
==========================================
+ Coverage   90.89%   91.03%   +0.14%     
==========================================
  Files          76       77       +1     
  Lines       42075    43973    +1898     
  Branches    42075    43973    +1898     
==========================================
+ Hits        38244    40032    +1788     
- Misses       3831     3941     +110     
Impacted Files Coverage Δ
lightning/src/routing/router.rs 92.52% <89.53%> (+<0.01%) ⬆️
lightning-net-tokio/src/lib.rs 75.91% <0.00%> (-0.31%) ⬇️
lightning/src/ln/functional_tests.rs 97.16% <0.00%> (-0.02%) ⬇️
lightning/src/util/time.rs 81.25% <0.00%> (ø)
lightning/src/routing/scoring.rs 96.12% <0.00%> (+1.47%) ⬆️
lightning-invoice/src/lib.rs 89.12% <0.00%> (+1.73%) ⬆️
lightning/src/ln/peer_channel_encryptor.rs 96.97% <0.00%> (+1.98%) ⬆️
lightning-invoice/src/payment.rs 95.77% <0.00%> (+3.02%) ⬆️
lightning/src/ln/peer_handler.rs 62.29% <0.00%> (+5.46%) ⬆️

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 342698f...87c9684. Read the comment docs.

@tnull tnull marked this pull request as ready for review May 11, 2022 13:10
@tnull tnull force-pushed the 2022-05-maximum-path-length branch from ef962dc to 190bca7 Compare May 12, 2022 11:07
@tnull
Copy link
Contributor Author

tnull commented May 12, 2022

Squashed and added a minor refactoring commit, because I found spelling out these types could improve readability.

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, aside form some formatting nits.

@tnull tnull force-pushed the 2022-05-maximum-path-length branch from 462c1e9 to 6de687c Compare May 16, 2022 08:58
@tnull
Copy link
Contributor Author

tnull commented May 16, 2022

Rebased and squashed.

@tnull tnull force-pushed the 2022-05-maximum-path-length branch 2 times, most recently from 18d2671 to e5d06b4 Compare May 16, 2022 10:58
@tnull
Copy link
Contributor Author

tnull commented May 16, 2022

Kicked CI...

@tnull tnull force-pushed the 2022-05-maximum-path-length branch 3 times, most recently from 70899b6 to 178827f Compare May 17, 2022 13:13
@tnull
Copy link
Contributor Author

tnull commented May 17, 2022

Squashed & rebased.

@TheBlueMatt
Copy link
Collaborator

LGTM, didn't review the math too closely but its close enough, lets set the max to 19.

@tnull tnull force-pushed the 2022-05-maximum-path-length branch 2 times, most recently from 3f15720 to 6330487 Compare May 18, 2022 06:41
@tnull
Copy link
Contributor Author

tnull commented May 18, 2022

Squashed once more.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Minor questions, otherwise ACK

@tnull tnull force-pushed the 2022-05-maximum-path-length branch 4 times, most recently from 2f0ab89 to 966c99d Compare May 18, 2022 16:50
@tnull tnull force-pushed the 2022-05-maximum-path-length branch from 966c99d to 87c9684 Compare May 18, 2022 17:20
@@ -177,9 +176,23 @@ impl_writeable_tlv_based!(RouteParameters, {
/// Maximum total CTLV difference we allow for a full payment path.
pub const DEFAULT_MAX_TOTAL_CLTV_EXPIRY_DELTA: u32 = 1008;

/// The median hop CLTV expiry delta currently seen in the network.
// The median hop CLTV expiry delta currently seen in the network.
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit, though its not worth changing back - we can leave doc comments on private items just fine, cargo doc has a --document-private-... flag to generate the docs for them, even. That said, it doesn't matter much we don't really use private docs AFAIK, but some devs may in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, fair enough, just thought I'd keep it consistent with visibility.

@TheBlueMatt TheBlueMatt merged commit 36817e0 into lightningdevkit:main May 18, 2022
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.

Consider maximum path length limit during path finding.
5 participants