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

Refactor Sphinx failures #2955

Merged
merged 5 commits into from
Dec 5, 2024
Merged

Refactor Sphinx failures #2955

merged 5 commits into from
Dec 5, 2024

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Dec 3, 2024

This PR contains multiple independent commits, that should be reviewed separately. This is purely refactoring without any behavioral changes: the goal is to simplify the changes needed to implement the final spec version of trampoline (see #2819).

The main change is the third commit, in which we change CMD_FAIL_HTLC from an Either[ByteVector, FailureMessage] to a sealed trait. See the commit message for more details.

@t-bast t-bast force-pushed the refactor-sphinx-failure branch from 3b7b421 to 0330fff Compare December 3, 2024 13:54
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 91.52542% with 5 lines in your changes missing coverage. Please review.

Project coverage is 86.13%. Comparing base (5410146) to head (0330fff).
Report is 8 commits behind head on master.

Files with missing lines Patch % Lines
...in/scala/fr/acinq/eclair/channel/fsm/Channel.scala 33.33% 2 Missing ⚠️
...ain/scala/fr/acinq/eclair/payment/Monitoring.scala 50.00% 1 Missing ⚠️
...cinq/eclair/payment/receive/MultiPartHandler.scala 87.50% 1 Missing ⚠️
.../scala/fr/acinq/eclair/payment/relay/Relayer.scala 66.66% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2955      +/-   ##
==========================================
- Coverage   86.19%   86.13%   -0.06%     
==========================================
  Files         224      225       +1     
  Lines       20074    20136      +62     
  Branches      813      812       -1     
==========================================
+ Hits        17302    17345      +43     
- Misses       2772     2791      +19     
Files with missing lines Coverage Δ
...in/scala/fr/acinq/eclair/channel/ChannelData.scala 100.00% <ø> (ø)
...src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala 100.00% <100.00%> (ø)
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 89.63% <100.00%> (ø)
.../scala/fr/acinq/eclair/payment/PaymentPacket.scala 94.59% <100.00%> (+0.59%) ⬆️
...a/fr/acinq/eclair/payment/relay/ChannelRelay.scala 96.42% <100.00%> (ø)
...cala/fr/acinq/eclair/payment/relay/NodeRelay.scala 91.50% <100.00%> (+0.77%) ⬆️
...r/acinq/eclair/payment/relay/OnTheFlyFunding.scala 90.35% <100.00%> (ø)
.../eclair/payment/relay/PostRestartHtlcCleaner.scala 91.53% <100.00%> (ø)
...r/acinq/eclair/payment/send/PaymentLifecycle.scala 83.33% <100.00%> (-1.02%) ⬇️
.../fr/acinq/eclair/wire/internal/CommandCodecs.scala 100.00% <100.00%> (ø)
... and 5 more

... and 7 files with indirect coverage changes

@t-bast t-bast force-pushed the refactor-sphinx-failure branch 3 times, most recently from bc3d507 to 8011fa3 Compare December 3, 2024 14:50
When we fail to decrypt an onion failure packet, we should return the
result of the unwrapping process. When using trampoline, this will let
us properly re-encrypt the failure and relay it upstream to the previous
trampoline node, until it reaches the sender.
We refactor the shared secret extraction to a dedicated function.
@t-bast t-bast force-pushed the refactor-sphinx-failure branch from 8011fa3 to 95931a4 Compare December 4, 2024 09:47
We previously used an `Either[ByteVector, FailureMessage]` to encode:

- a downstream error that we couldn't decrypt and must re-wrap (left)
- a local error that we must encrypt (right)

This won't be sufficient for trampoline, because we will need to handle
the following cases:

- a downstream error that we couldn't decrypt and must re-wrap
- a local error for the node who created the *outer* onion (which we
  encrypt with the sphinx shared secret of the outer onion)
- a local error for the node who created the *trampoline* onion (which
  we encrypt with the sphinx shared secret of the trampoline onion and
  then with the shared secret of the outer onion)

We thus introduce a trait, which currently only contains the first two
cases. We will extend this trait when adding support for trampoline
failures. This is a pure refactoring without any behavior changes so
far, which will simplify the future trampoline changes.
When we receive a (non-blinded) payment that uses trampoline, we keep
the trampoline onion to be able to distinguish this payment from a
non-trampoline payment.
@t-bast t-bast force-pushed the refactor-sphinx-failure branch from 95931a4 to 1376f0b Compare December 4, 2024 16:21
@t-bast t-bast marked this pull request as ready for review December 4, 2024 16:21
@t-bast t-bast requested a review from thomash-acinq December 4, 2024 16:21
@t-bast t-bast merged commit 2ad2260 into master Dec 5, 2024
1 check passed
@t-bast t-bast deleted the refactor-sphinx-failure branch December 5, 2024 09:47
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