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

Create routes to self using remote alias #2507

Merged
merged 1 commit into from Nov 30, 2022
Merged

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Nov 24, 2022

The introduction of scid-alias broke the ability to create a valid route to our own node using FinalizeRoute for private or unconfirmed channels because we use our local alias as the shortChannelId of the graph edge in both directions.

Using the same identifier for both directions makes sense, because it's a stable identifier whereas the remote alias could be updated by our peer. It's generally not an issue, except when we are building a route, because our peer will not know how to route if we give them our local alias in a payment onion (they expect their own local alias, which from our point of view is the remote alias).

The only way to build routes to ourselves is by using FinalizeRoute, so we fix the issue only in this handler by looking at our private channels when we notice that we are the destination.

The introduction of `scid-alias` broke the ability to create a valid route
to our own node using `FinalizeRoute` for private or unconfirmed channels
because we use our local alias as the shortChannelId of the graph edge in
both directions.

Using the same identifier for both directions makes sense, because it's a
stable identifier whereas the remote alias could be updated by our peer.
It's generally not an issue, except when we are building a route, because
our peer will not know how to route if we give them our local alias in a
payment onion (they expect their own local alias, which from our point of
view is the remote alias).

The only way to build routes to ourselves is by using `FinalizeRoute`, so
we fix the issue only in this handler by looking at our private channels
when we notice that we are the destination.
@thomash-acinq
Copy link
Member

Are you sure we need this?
In the failing tests, e.desc.shortChannelId was good, the problem was with e.params.shortChannelId.

@codecov-commenter
Copy link

Codecov Report

Merging #2507 (6cec591) into master (e32697e) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2507      +/-   ##
==========================================
+ Coverage   84.94%   84.99%   +0.04%     
==========================================
  Files         200      200              
  Lines       15827    15832       +5     
  Branches      693      696       +3     
==========================================
+ Hits        13445    13456      +11     
+ Misses       2382     2376       -6     
Impacted Files Coverage Δ
...cala/fr/acinq/eclair/router/RouteCalculation.scala 94.69% <100.00%> (+0.20%) ⬆️
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 86.27% <0.00%> (+0.11%) ⬆️
...fr/acinq/eclair/channel/InteractiveTxBuilder.scala 85.89% <0.00%> (+0.51%) ⬆️
...cala/fr/acinq/eclair/crypto/TransportHandler.scala 91.16% <0.00%> (+0.55%) ⬆️
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 85.76% <0.00%> (+0.72%) ⬆️

@t-bast
Copy link
Member Author

t-bast commented Nov 25, 2022

In the failing tests, e.desc.shortChannelId was good, the problem was with e.params.shortChannelId.

I think that's only because you didn't test the private channels case. For private channels, e.desc.shortChannelId would be the local alias (as the newly added unit tests show: they fail without the change to RouteCalculation), which our peer doesn't use for relay.

Copy link
Member

@pm47 pm47 left a comment

Choose a reason for hiding this comment

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

This is really hacky, but should be fixed once we finally make the key of our graph the logical (node1,node2), as opposed to the current physical (scid, node1, node2), because we will then have to handle the scid selection explicitly.

@t-bast
Copy link
Member Author

t-bast commented Nov 30, 2022

Agreed, that will be a nice refactoring, we should aim for this. It won't be a trivial change though, it impacts many places in the codebase, but it's a necessary one.

@t-bast t-bast merged commit 5b0aa35 into master Nov 30, 2022
@t-bast t-bast deleted the finalize-route-scid-alias branch November 30, 2022 13:24
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.

None yet

4 participants