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.

Sign up for GitHub

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

Jump to bottom

Multi-Hop Route Hint now considered. Added in unit tests for same. #1040

Merged

TheBlueMatt merged 1 commit into lightningdevkit:main from abhik-99:Issue#945

Aug 31, 2021

Contributor

abhik-99 commented Aug 9, 2021 •

edited

Loading

This PR solves Issue #945 which deals with using multi-hop route hints from the invoice from payee. Previous behaviour was to only consider the last route hop hint per route. This PR solves the problems with PR #1030 and also includes the unit tests for testing out the Route Hint in a multi-hop setup.

Edit: Commit 914c159 solves a previously failing test where the channel capacity was calculated to be 0 which skipped certain hints.

Edit: The above has been squashed into fd4d3f

jkczyz self-requested a review

August 9, 2021 15:10

codecov bot commented Aug 9, 2021 •

edited

Loading

Codecov Report

Merging #1040 (4c5526b) into main (8530078) will increase coverage by 0.05%.
The diff coverage is 99.56%.

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

@@            Coverage Diff             @@
##             main    #1040      +/-   ##
==========================================
+ Coverage   90.80%   90.85%   +0.05%     
==========================================
  Files          61       65       +4     
  Lines       31582    33128    +1546     
==========================================
+ Hits        28678    30100    +1422     
- Misses       2904     3028     +124

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	`96.34% <99.56%> (+0.40%)`	⬆️
lightning/src/util/ser_macros.rs	`87.90% <0.00%> (-8.42%)`	⬇️
lightning/src/util/errors.rs	`67.30% <0.00%> (-4.13%)`	⬇️
lightning-background-processor/src/lib.rs	`93.93% <0.00%> (-1.65%)`	⬇️
lightning/src/util/enforcing_trait_impls.rs	`89.28% <0.00%> (-1.10%)`	⬇️
lightning/src/chain/keysinterface.rs	`94.68% <0.00%> (-0.68%)`	⬇️
lightning/src/ln/channel.rs	`88.73% <0.00%> (-0.50%)`	⬇️
lightning-net-tokio/src/lib.rs	`76.98% <0.00%> (-0.38%)`	⬇️
lightning/src/util/test_utils.rs	`82.33% <0.00%> (-0.22%)`	⬇️
lightning/src/ln/peer_handler.rs	`45.78% <0.00%> (-0.20%)`	⬇️
... and 18 more

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 8530078...fd4d3bf. Read the comment docs.

TheBlueMatt reviewed

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

abhik-99 requested a review from TheBlueMatt

August 10, 2021 08:45

TheBlueMatt reviewed

View reviewed changes

Collaborator

TheBlueMatt left a comment

This is a lot less logic to review compared to the last version, thanks. Makes it much easier to find bugs when there's fewer branches and ifs in the code :).

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jkczyz reviewed

View reviewed changes

Contributor

jkczyz left a comment

Just some minor comments for now. Will do a more thorough review once you push your most recent changes.

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed

View reviewed changes

Collaborator

TheBlueMatt left a comment

This is looking pretty good.

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

abhik-99 requested review from TheBlueMatt and jkczyz

August 17, 2021 17:37

jkczyz reviewed

View reviewed changes

Contributor

jkczyz left a comment

Getting late. Will take a close look at the tests tomorrow.

lightning/src/routing/router.rs Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jkczyz reviewed

View reviewed changes

Contributor

jkczyz left a comment

Noted some more ways to clean up the code and tests IIUC what's being tested (see comments). There's also some earlier comments that have not yet been addressed. Could you go through to get everything that was missed?

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

jkczyz reviewed

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

.gitignore Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

abhik-99 requested a review from jkczyz

August 20, 2021 15:23

Contributor Author

abhik-99 commented Aug 23, 2021

@TheBlueMatt and @jkczyz any more suggestions?

TheBlueMatt reviewed

View reviewed changes

Collaborator

TheBlueMatt left a comment

A few hiccups, but ACK aside from the comments.

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

lightning/src/routing/router.rs Show resolved Hide resolved

TheBlueMatt approved these changes

View reviewed changes

Collaborator

TheBlueMatt left a comment

This LGTM. It'll need a second reviewers' ACK. Once a second reviewer ACKs, you'll also need to squash down the commits to not have any commits which, on their own, leave things either failing tests, or otherwise in an intermediary state. For this PR, that probably means squashing it all down into one commit, unless you want to put the new tests in a separate one.

jkczyz approved these changes

View reviewed changes

lightning/src/routing/router.rs Outdated Show resolved Hide resolved

Collaborator

TheBlueMatt commented Aug 30, 2021

Cool! Can you squash the commits down to not have fixes in followup commits? https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits should be helpful if you have never done this.


          Multi-Hop Route Hint as per Bolt 12 now considered

fd4d3bf

Bolt 12 details the process of picking up route hints from payee
using the lightning invoice. This PR brings the changes to use
multiple route hints from payee picked from the invoice.

The route hints are processed in the following manner:-
 - `get_route()` receives the hints in `last_hops`.
 - Every `RouteHintHop` in `RouteHint` is processed based on
   feasiblity of channel capacity and fees.
 - If a `RouteHintHop` then preceeding `RouteHintHop`s are not
   processed.
 - A direct route is checked from `first_hops_targets` to the
   first `RouteHintHop` if the respective `RouteHint` is
   processed from the payee's end till the first `RouteHintHop`.

`partial_route_hint_test`, `ignores_empty_last_hops_test`,
`multi_hint_last_hops_test` and `last_hops_with_public_channel_test`
test usage of partial route hints for building optimal route,
processing empty route hint hops, complete usage of private route
hints and presence of public channels in route hints respectively.

Resolves: lightningdevkit#945

abhik-99 force-pushed the Issue#945 branch from 4c5526b to fd4d3bf Compare

August 31, 2021 05:52

Contributor

jkczyz commented Aug 31, 2021

TheBlueMatt merged commit a265f1e into lightningdevkit:main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet