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.

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

Variable-length onion payloads #976

Merged
merged 28 commits into from
Jul 23, 2019
Merged

Variable-length onion payloads #976

merged 28 commits into from
Jul 23, 2019

Conversation

t-bast
Copy link
Member

@t-bast t-bast commented Apr 29, 2019 •
edited
Loading

This pull request implements the variable-length onion proposal submitted in lightning/bolts#619

I also took this opportunity to heavily refactor our onion code:

  • Moved everything encoding-related to scodec
  • Split the contents of the Sphinx object into sub-objects (in particular this will let us easily add support for onions of different sizes such as trampoline onions)
  • Renamed most methods to (hopefully) make them clearer
  • We only returned BAD_ONION errors and not the more fine-grained errors detailed in the spec -> that's now fixed
  • Added more tests
  • Removed the misleading LAST_PACKET: this was misleading because this was the (invalid) next packet created when unwrapping the last packet, not actually the last packet
  • I considered using the same abstraction for the payment onion and the failure onion, but unfortunately they have very different mechanisms: I think it would be a bad idea to try to fit them in the same abstraction

There are two minor spec details that may change depending on the discussion on the spec PR:

  • The bits used for the feature flag will likely align with the feature bit unification effort
  • The termination condition may (or not) be simplified (to only the hmac being 0x00)

These are fairly minor changes so I think you can start reviewing now to fix all eclair/scala specific stuff @pm47 @sstone

@t-bast t-bast force-pushed the multi-frame-onion branch from 356c3a2 to c7434fe Compare May 15, 2019 15:20
@codecov-io
Copy link

codecov-io commented May 15, 2019 •
edited
Loading

Codecov Report

Merging #976 into master will increase coverage by 0.14%.
The diff coverage is 95.91%.

@@            Coverage Diff             @@
##           master     #976      +/-   ##
==========================================
+ Coverage    82.6%   82.74%   +0.14%     
==========================================
  Files          99      101       +2     
  Lines        7612     7616       +4     
  Branches      295      301       +6     
==========================================
+ Hits         6288     6302      +14     
+ Misses       1324     1314      -10
Impacted Files Coverage Δ
...a/fr/acinq/eclair/wire/LightningMessageTypes.scala 100% <ø> (ø) ⬆️
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala 100% <ø> (ø) ⬆️
...main/scala/fr/acinq/eclair/payment/Autoprobe.scala 0% <ø> (ø) ⬆️
...re/src/main/scala/fr/acinq/eclair/crypto/Mac.scala 100% <100%> (ø)
.../fr/acinq/eclair/wire/LightningMessageCodecs.scala 100% <100%> (ø) ⬆️
...re/src/main/scala/fr/acinq/eclair/wire/Onion.scala 100% <100%> (ø)
...ala/fr/acinq/eclair/payment/PaymentLifecycle.scala 88.49% <100%> (ø) ⬆️
...n/scala/fr/acinq/eclair/router/Announcements.scala 100% <100%> (ø) ⬆️
...src/main/scala/fr/acinq/eclair/crypto/Sphinx.scala 100% <100%> (+4.91%) ⬆️
...core/src/main/scala/fr/acinq/eclair/Features.scala 100% <100%> (ø) ⬆️
... and 16 more

@t-bast t-bast force-pushed the multi-frame-onion branch from c7434fe to 407bc83 Compare May 22, 2019 08:23
@t-bast t-bast marked this pull request as ready for review May 22, 2019 08:45
@t-bast t-bast requested review from pm47 and sstone May 22, 2019 08:45
@t-bast t-bast force-pushed the multi-frame-onion branch 2 times, most recently from 41afd9d to fe81564 Compare May 28, 2019 16:01
@t-bast t-bast closed this May 28, 2019
@t-bast t-bast deleted the multi-frame-onion branch May 28, 2019 16:08
@t-bast t-bast restored the multi-frame-onion branch May 28, 2019 16:09
@t-bast t-bast reopened this May 28, 2019
@t-bast t-bast force-pushed the multi-frame-onion branch from fe81564 to fcf8114 Compare June 7, 2019 08:16
@t-bast t-bast force-pushed the multi-frame-onion branch 2 times, most recently from 32f0740 to 2b2dc52 Compare June 19, 2019 09:42
@pm47

This comment has been minimized.

Sign in to view
@t-bast

This comment has been minimized.

Sign in to view
@pm47

This comment has been minimized.

Sign in to view
@t-bast

This comment has been minimized.

Sign in to view
@t-bast t-bast force-pushed the multi-frame-onion branch 4 times, most recently from 08d5c8e to 5c0a2e5 Compare July 1, 2019 12:19
@t-bast t-bast force-pushed the multi-frame-onion branch from 5c0a2e5 to b8f329f Compare July 2, 2019 11:27
@pm47 pm47 removed request for sstone and pm47 July 4, 2019 11:09
@t-bast t-bast force-pushed the multi-frame-onion branch 2 times, most recently from d2c6516 to c618698 Compare July 4, 2019 12:35
@t-bast t-bast requested a review from pm47 July 4, 2019 12:51
t-bast added 18 commits July 15, 2019 11:56
@t-bast
SphinxSpec: add official test vector.
02de66a
@t-bast
Sphinx: use variable-length payloads.
7bd8942
@t-bast
SphinxSpec: add official variable-length test vector
b4cbed5
@t-bast
features: rename variable-length onion flag
9c42e55
@t-bast
FeaturesSpec: update test name
9a434d5
@t-bast
Sphinx: refactor onion objects.
367d6eb 
Grouped functions that belonged together under objects.
Abstracted the OnionPacket in a sealed trait.
This paves the way for onions of different sizes (trampoline, hornet) and with different stopping conditions.
@t-bast
Update varint codec name
97622d4
@t-bast
Use scodec for onion packet.
22b5faa 
Refactor and rename sphinx onion packet functions.
Comply with BAD_ONION spec sub-errors.
@t-bast
Use scodec for onion failure packet.
0f28e8a 
Add codec that prepends a valid mac.
Refactor to harmonize methods with onion packet.
@t-bast
Address PR comments:
5022847 
- Rename Mac to Mac32
- Move onion types and codecs to their own files
- Small refactorings
@t-bast
Create a decode-only codec to extract onion payload length
b39769e
@t-bast
Clean-up a few nits:
52d7023 
* Remove redundant comments
* Rename InvalidOnion error
@t-bast
Add function to extract failure code from failure message
5946b6a
@t-bast
Clean-up failureOnionCodec
4c6a05a
@t-bast
Rename to OnionRouting everywhere.
0c13aea 
It's clearer like that.
@t-bast
Add error log if onion payload decoder has remaining bytes
9506dd7
@t-bast
Update onion feature bit.
9fa1dc3
@t-bast
Clean-up: sort imports
5a1c3b2
@t-bast t-bast force-pushed the multi-frame-onion branch from d378a44 to 5a1c3b2 Compare July 15, 2019 09:59
pm47
pm47 previously approved these changes Jul 23, 2019
View reviewed changes
@t-bast t-bast merged commit 93d9369 into master Jul 23, 2019
@t-bast t-bast deleted the multi-frame-onion branch July 23, 2019 10:22
@pm47 pm47 mentioned this pull request Sep 11, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sstone sstone sstone left review comments

@pm47 pm47 pm47 approved these changes

Assignees

@sstone sstone

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@t-bast @codecov-io @pm47 @sstone