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

Relay onion messages #2061

Merged
merged 14 commits into from
Nov 29, 2021
Merged

Relay onion messages #2061

merged 14 commits into from
Nov 29, 2021

Conversation

thomash-acinq
Copy link
Member

Actually relay onion messages and add API to send onion messages.
Follows #1957

@thomash-acinq
Copy link
Member Author

@t-bast Not ready yet but does the way I use actors look good?

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

@t-bast Not ready yet but does the way I use actors look good?

I think it's the right direction, yes!
I put a few comments to guide you to akka typed.

@thomash-acinq thomash-acinq force-pushed the onion-messages branch 4 times, most recently from c8a5960 to 92cf397 Compare November 10, 2021 15:39
@codecov-commenter
Copy link

codecov-commenter commented Nov 10, 2021

Codecov Report

Merging #2061 (42ff02b) into master (fb96e5e) will increase coverage by 0.05%.
The diff coverage is 85.10%.

@@            Coverage Diff             @@
##           master    #2061      +/-   ##
==========================================
+ Coverage   87.54%   87.60%   +0.05%     
==========================================
  Files         165      166       +1     
  Lines       12688    12717      +29     
  Branches      536      545       +9     
==========================================
+ Hits        11108    11141      +33     
+ Misses       1580     1576       -4     
Impacted Files Coverage Δ
...main/scala/fr/acinq/eclair/io/PeerConnection.scala 84.16% <66.66%> (ø)
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 46.66% <70.00%> (+2.71%) ⬆️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 89.85% <80.00%> (-0.15%) ⬇️
...rc/main/scala/fr/acinq/eclair/io/Switchboard.scala 82.22% <87.50%> (-0.28%) ⬇️
...c/main/scala/fr/acinq/eclair/io/MessageRelay.scala 100.00% <100.00%> (ø)
...in/scala/fr/acinq/eclair/io/ReconnectionTask.scala 97.82% <100.00%> (ø)
.../scala/fr/acinq/eclair/message/OnionMessages.scala 76.74% <100.00%> (ø)
...cinq/eclair/remote/EclairInternalsSerializer.scala 97.87% <100.00%> (ø)
...cala/fr/acinq/eclair/wire/protocol/TlvCodecs.scala 100.00% <100.00%> (ø)
... and 2 more

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

A couple of comments on the actor architecture itself.
I think that if you apply all these comments, it will both simplify the PR and make the system more testable and extensible in the future.

Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Looking good, thanks for the changes!
A few minor comments and additional tests (in a separate commit in my branch) and we should be good to go.

docs/release-notes/eclair-vnext.md Outdated Show resolved Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/Eclair.scala Outdated Show resolved Hide resolved
t-bast and others added 2 commits November 29, 2021 11:35
* Use typed actor framework for MessageRelaySpec
* Fix port conflicts in integration tests
* Use pipeTo instead of blocking for integration tests
* Increase timeout for integration spec
* Add tests for user content tlvs
* Add tests for automatic reconnection
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

Good job, LGTM 🚀

With this new API it should be easy to test onion messages against other implementations, have you been able to test with rusty's latest updates for c-lightning?

If we're compatible (again) it's an important milestone, it will be worth mentioning on the onion messages spec PR!

@thomash-acinq
Copy link
Member Author

I've tested it ElementsProject/lightning#4921 can relay messages between eclair nodes.

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

Successfully merging this pull request may close these issues.

4 participants