fixup! Split the `Peer` in two (#1347) #1357

pm47 · 2020-03-25T20:20:59Z

No description provided.

codecov-io · 2020-03-25T20:28:04Z

Codecov Report

Merging #1357 into master will increase coverage by 0.06%.
The diff coverage is 83.87%.

@@            Coverage Diff             @@
##           master    #1357      +/-   ##
==========================================
+ Coverage   86.29%   86.36%   +0.06%     
==========================================
  Files         119      119              
  Lines        9201     9225      +24     
  Branches      396      402       +6     
==========================================
+ Hits         7940     7967      +27     
+ Misses       1261     1258       -3

Impacted Files	Coverage Δ
...ore/src/main/scala/fr/acinq/eclair/io/Server.scala	`89.47% <ø> (-0.53%)`	⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala	`83.70% <76.19%> (-0.51%)`	⬇️
...air-core/src/main/scala/fr/acinq/eclair/Logs.scala	`91.48% <100.00%> (+3.73%)`	⬆️
...src/main/scala/fr/acinq/eclair/io/Monitoring.scala	`100.00% <100.00%> (ø)`
...main/scala/fr/acinq/eclair/io/PeerConnection.scala	`85.23% <100.00%> (+0.14%)`	⬆️
...in/scala/fr/acinq/eclair/io/ReconnectionTask.scala	`91.83% <100.00%> (+0.17%)`	⬆️
...rc/main/scala/fr/acinq/eclair/io/Switchboard.scala	`82.35% <100.00%> (+1.10%)`	⬆️
...src/main/scala/fr/acinq/eclair/router/Router.scala	`92.65% <100.00%> (ø)`
...ore/src/main/scala/fr/acinq/eclair/io/Client.scala	`63.63% <0.00%> (-0.65%)`	⬇️
...clair/payment/send/MultiPartPaymentLifecycle.scala	`97.70% <0.00%> (-0.58%)`	⬇️
... and 4 more

eclair-core/src/main/scala/fr/acinq/eclair/io/Monitoring.scala

t-bast · 2020-03-26T08:18:32Z

eclair-core/src/main/scala/fr/acinq/eclair/io/Monitoring.scala

@@ -21,8 +21,10 @@ import kamon.Kamon
 object Monitoring {

  object Metrics {


We could probably also instrument the reconnection tasks? Maybe a counter for successful/failed reconnections?

t-bast · 2020-03-26T08:21:24Z

eclair-core/src/main/scala/fr/acinq/eclair/io/PeerConnection.scala

@@ -456,7 +467,7 @@ object PeerConnection {
  sealed trait Data
  sealed trait HasTransport extends Data { def transport: ActorRef }
  case object Nothing extends Data
-  case class AuthenticatingData(pendingAuth: PendingAuth, transport: ActorRef) extends Data
+  case class AuthenticatingData(pendingAuth: PendingAuth, transport: ActorRef) extends Data with HasTransport


nit: HasTransport already extends Data, so you don't need extends Data with HasTransport (and same below).

I know, that's because I have used the cake pattern too much. Actually that's a perfect fit for the cake pattern and I could have defined HasTransport that way:

sealed trait HasTransport { this: Data => def transport: ActorRef }

wdyt @sstone?

Done with df54c28

What does that cake pattern bring here (apart from being less explicit than sealed trait HasTransport extends Data)?

Not much: it conveys the idea that HasTransport is just a behavior and not a definition, which is closer to how I see it.

Got it, I'm not very familiar with that pattern

In other words you have to use Data with HasTransport, you can't use HasTransport by itself.

eclair-core/src/main/scala/fr/acinq/eclair/io/Server.scala

pm47 · 2020-03-27T16:27:30Z

What do you guys think of removing the Authenticated/InitializeConnection dance between the PeerConnection and the Switchboard?

Meaning going from:

 PeerConnection        Switchboard              Peer
       |                    |                    |
       |   Authenticated    |                    |
       |------------------->|                    |
       |                    |                    |
       |   InitializeConn   |                    |
       |<-------------------|                    |
       |                    |                    |
       |                                         |
       |           ConnectionReady               |
       |---------------------------------------->|
       |                                         |
       |           LightningMessage              |
       |---------------------------------------->|
       |---------------------------------------->|
       |---------------------------------------->|
       |                                         |
       |                                         |

to:

 PeerConnection        Switchboard              Peer
       |                    |                    |
       |  ConnectionReady   |                    |
       |------------------->|  ConnectionReady   |
       |                    |------------------->|
       |  LightningMessage  |                    |
       |------------------->|  LightningMessage  |
       |                    |------------------->|
       |  LightningMessage  |                    |
       |------------------->|  LightningMessage  |
       |                    |------------------->|

Tradeoff being that the flow is simpler, but all messages have to go through the Switchboard, meaning that they also need to be encapsulated in an envelope message containing the remoteNodeId.

t-bast · 2020-03-27T17:03:51Z

I prefer how it is now. It's really not much code and avoids too much forwarding and encapsulating.
The switchboard acts as a manager during the authentication and then gets out of the way, I think this is its job.

t-bast · 2020-03-27T17:05:19Z

Apart from that, I'm in favor on merging this PR once the HasTransport thing is settled to get this fixup in eclair-0.3.4!

pm47 · 2020-03-27T18:20:50Z

I prefer how it is now. It's really not much code and avoid too much forwarding and encapsulating.
The switchboard acts as a manager during the authentication and then gets out of the way, I think this is its job.

Gave it a try with 27c7e68.

t-bast · 2020-03-30T07:25:39Z

eclair-core/src/main/scala/fr/acinq/eclair/io/Switchboard.scala

+      peer forward p
+
+    case p: Peer.PeerMessage =>
+      // we create a peer if it doesn't exist


We should never have to create one here, right?
Are there unwanted side-effects if we do (some assumption each of the FSMs make about the other FSMs)?
Shouldn't we reject those messages if we don't have a peer actor to handle it?

It feels to me that the peer FSMs already have to know about the other FSMs and assume some of their behavior, and this change is making it slightly worse...

We should never have to create one here, right?

Correct, this is only needed in the handler for PeerConnection.ConnectionReady.

It feels to me that the peer FSMs already have to know about the other FSMs and assume some of their behavior, and this change is making it slightly worse...

There has to be an interface contract, even if it less explicit with (untyped) actors. If anything the interface is a bit simpler now. But let's revert this and we can revisit later.

This reverts commit 27c7e68.

pm47 added 4 commits March 25, 2020 20:14

fixup! Split the Peer in two (#1347)

2d80eb0

fixup! Split the Peer in two (#1347)

b75d373

fixup! Split the Peer in two (#1347)

e90e4d2

fixup! Split the Peer in two (#1347)

a5604af

pm47 requested a review from t-bast March 25, 2020 20:20

t-bast reviewed Mar 26, 2020

View reviewed changes

pm47 added 3 commits March 26, 2020 18:22

fixup! fixup! Split the Peer in two (#1347)

c815e24

renamed metrics

e8b4874

tracking reconnection attempts

7111b6b

remove the Authenticated/InitializeConnection dance

27c7e68

use cake pattern for HasTransport

df54c28

t-bast reviewed Mar 30, 2020

View reviewed changes

Revert "remove the Authenticated/InitializeConnection dance"

9f4d895

This reverts commit 27c7e68.

t-bast approved these changes Mar 30, 2020

View reviewed changes

pm47 merged commit 90b7fed into master Mar 30, 2020

pm47 deleted the fix-peer branch March 30, 2020 09:05

pm47 added a commit that referenced this pull request Apr 2, 2020

fixup! fixup! Split the Peer in two (#1347) (#1357)

3add441

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixup! Split the `Peer` in two (#1347) #1357

fixup! Split the `Peer` in two (#1347) #1357

pm47 commented Mar 25, 2020

codecov-io commented Mar 25, 2020 •

edited

Loading

t-bast Mar 26, 2020

pm47 Mar 26, 2020

t-bast Mar 26, 2020

pm47 Mar 26, 2020 •

edited

Loading

pm47 Mar 27, 2020

t-bast Mar 30, 2020

pm47 Mar 30, 2020

t-bast Mar 30, 2020

pm47 Mar 30, 2020

pm47 commented Mar 27, 2020 •

edited

Loading

t-bast commented Mar 27, 2020 •

edited

Loading

t-bast commented Mar 27, 2020

pm47 commented Mar 27, 2020

t-bast Mar 30, 2020 •

edited

Loading

pm47 Mar 30, 2020

		@@ -21,8 +21,10 @@ import kamon.Kamon
		object Monitoring {

		object Metrics {

fixup! Split the Peer in two (#1347) #1357

fixup! Split the Peer in two (#1347) #1357

Conversation

pm47 commented Mar 25, 2020

codecov-io commented Mar 25, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pm47 Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pm47 commented Mar 27, 2020 • edited Loading

t-bast commented Mar 27, 2020 • edited Loading

t-bast commented Mar 27, 2020

pm47 commented Mar 27, 2020

t-bast Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fixup! Split the `Peer` in two (#1347) #1357

fixup! Split the `Peer` in two (#1347) #1357

codecov-io commented Mar 25, 2020 •

edited

Loading

pm47 Mar 26, 2020 •

edited

Loading

pm47 commented Mar 27, 2020 •

edited

Loading

t-bast commented Mar 27, 2020 •

edited

Loading

t-bast Mar 30, 2020 •

edited

Loading