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

Return local channel alias in payment failures #2323

Merged
merged 6 commits into from
Jul 1, 2022
Merged

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Jun 16, 2022

When using option_scid_alias, we must be careful not to expose the real scid to remote payers. We never generate a channel_update with the real scid so this isn't a risk, but we're incorrectly returning the remote alias instead of the returning the local one (which should match the invoice).

@t-bast t-bast requested a review from pm47 June 16, 2022 13:57
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.

Good catch!

I think I would have addressed this differently:

  • always generate our channel_update with our local alias or real scid depending on channel params (this is the fix to the present bug)
  • re-sign the channel_update with remote alias in the onTransition within the if (sendToPeer) {...} so we make it clear that using remote alias is the exception not the rule

@t-bast
Copy link
Member Author

t-bast commented Jun 17, 2022

I think I would have addressed this differently

That makes sense, it feels more consistent this way, I'll try it out.

@t-bast t-bast force-pushed the scid-alias-onion-error branch 2 times, most recently from d65dad1 to aa3fbc7 Compare June 17, 2022 13:53
@t-bast
Copy link
Member Author

t-bast commented Jun 17, 2022

I've updated the PR to:

  • always use either our local alias or the real scid in our channel updates
  • when directly sending to our peer, override it with their remote alias when necessary
  • when returning onion failures, use the incoming scid when applicable

@t-bast t-bast force-pushed the scid-alias-onion-error branch 3 times, most recently from 0cb89e3 to 416efb5 Compare June 17, 2022 14:40
@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2022

Codecov Report

Merging #2323 (794a287) into master (8af14ac) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2323      +/-   ##
==========================================
- Coverage   84.71%   84.67%   -0.05%     
==========================================
  Files         194      194              
  Lines       14633    14643      +10     
  Branches      612      625      +13     
==========================================
+ Hits        12397    12399       +2     
- Misses       2236     2244       +8     
Impacted Files Coverage Δ
...c/main/scala/fr/acinq/eclair/channel/Helpers.scala 95.04% <100.00%> (+<0.01%) ⬆️
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 87.77% <100.00%> (ø)
...q/eclair/channel/fsm/ChannelOpenSingleFunder.scala 91.37% <100.00%> (ø)
...a/fr/acinq/eclair/payment/relay/ChannelRelay.scala 99.08% <100.00%> (+1.02%) ⬆️
...fr/acinq/eclair/payment/relay/ChannelRelayer.scala 100.00% <100.00%> (ø)
.../scala/fr/acinq/eclair/payment/relay/Relayer.scala 90.90% <100.00%> (ø)
...n/scala/fr/acinq/eclair/router/Announcements.scala 100.00% <100.00%> (ø)
...la/fr/acinq/eclair/channel/fsm/ErrorHandlers.scala 81.63% <0.00%> (-2.73%) ⬇️
...cinq/eclair/channel/publish/MempoolTxMonitor.scala 88.23% <0.00%> (-2.36%) ⬇️
...main/scala/fr/acinq/eclair/router/Validation.scala 93.35% <0.00%> (-1.33%) ⬇️
... and 3 more

t-bast added 2 commits June 29, 2022 16:12
We now use either our local alias or the real scid in the channel update
that we store internally. When we send that channel update directly to
our peer, we override it to use the remote alias when it makes sense.
There are multiple valid scids for public channels depending on the
confirmation status of the funding tx. A payer may use either of these
scids, and we must return the one they used in case of error.
@t-bast t-bast force-pushed the scid-alias-onion-error branch from 416efb5 to 18ff971 Compare June 29, 2022 14:20
@t-bast t-bast force-pushed the scid-alias-onion-error branch from c1c25ed to 794a287 Compare June 30, 2022 14:38
pm47
pm47 previously approved these changes Jun 30, 2022
We shouldn't need them since we don't add the real scid to the routing
table when scid_alias is negotiated.
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.

The router is left unchanged despite us using localAlias instead of remoteAlias for our own channel_update, because we resolve based on the channelId or the realScid:
https://github.com/ACINQ/eclair/blob/master/eclair-core/src/main/scala/fr/acinq/eclair/router/Validation.scala#L300

* Revert change to makeChannelUpdate
* Improve zero-conf tests
@t-bast t-bast merged commit a1f7c1e into master Jul 1, 2022
@t-bast t-bast deleted the scid-alias-onion-error branch July 1, 2022 13:13
t-bast added a commit that referenced this pull request Dec 1, 2022
The release introduces a few API changes:

- channelbalances retrieves information about the balances of all local
  channels (#2196)
- channelbalances and usablebalances return a shortIds object instead
  of a single shortChannelId (#2323)
- stop stops eclair: please note that the recommended way of stopping
  eclair is simply to kill its process (#2233)
- rbfopen lets the initiator of a dual-funded channel RBF the funding
  transaction (#2275)
- listinvoices and listpendinginvoices now accept --count and --skip
  parameters to limit the number of retrieved items (#2474)
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.

3 participants