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

Peer reconnection address from node announcements #1009

Merged
merged 30 commits into from
Jun 11, 2019

Conversation

araspitzu
Copy link
Contributor

@araspitzu araspitzu commented May 20, 2019

Currently the node address information in node_announcement messages is not used when establishing a connection. Peer's addresses are known only when the user initiated the connection providing a destination address, in case of incoming connection the peer's address is blanked out and we can't reconnect. According to the spec the addresses field of node_announcement should contain the network addresses where a peer expects an incoming connection, this PR leverages this information to provide the following enhancements:

  • Connect-to a pubkey (no address specified, will use node_announcements)
  • Reconnect to peers using their node_announcements address as fallback in case we don't know the address

Fixes #1003

@pm47
Copy link
Member

pm47 commented May 21, 2019

What would be nice is also implement a disconnect API call.

@pm47
Copy link
Member

pm47 commented May 23, 2019

We should also probably make the exponential backoff delay go all the way to 1 hour every reconnect attempt.

@ACINQ ACINQ deleted a comment from codecov-io May 28, 2019
@ACINQ ACINQ deleted a comment from codecov-io Jun 5, 2019
@codecov-io
Copy link

codecov-io commented Jun 5, 2019

Codecov Report

Merging #1009 into master will increase coverage by 0.81%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master    #1009      +/-   ##
==========================================
+ Coverage    80.2%   81.01%   +0.81%     
==========================================
  Files          98       98              
  Lines        7554     8124     +570     
  Branches      296      312      +16     
==========================================
+ Hits         6059     6582     +523     
- Misses       1495     1542      +47
Impacted Files Coverage Δ
.../src/main/scala/fr/acinq/eclair/db/NetworkDb.scala 100% <ø> (ø) ⬆️
...r-core/src/main/scala/fr/acinq/eclair/Eclair.scala 52.87% <0%> (+0.49%) ⬆️
...cala/fr/acinq/eclair/api/FormParamExtractors.scala 81.81% <100%> (+1.81%) ⬆️
...la/fr/acinq/eclair/db/sqlite/SqliteNetworkDb.scala 98.61% <100%> (+0.08%) ⬆️
...rc/main/scala/fr/acinq/eclair/io/Switchboard.scala 73.84% <25%> (-6.16%) ⬇️
...e/src/main/scala/fr/acinq/eclair/api/Service.scala 68.98% <66.66%> (-0.75%) ⬇️
...-core/src/main/scala/fr/acinq/eclair/io/Peer.scala 75.87% <89.65%> (+0.95%) ⬆️
...scala/fr/acinq/eclair/payment/PaymentRequest.scala 90.45% <0%> (-1.61%) ⬇️
...n/scala/fr/acinq/eclair/channel/ChannelTypes.scala 100% <0%> (ø) ⬆️
...c/main/scala/fr/acinq/eclair/channel/Channel.scala 84.16% <0%> (+1.15%) ⬆️
... and 7 more

@araspitzu araspitzu marked this pull request as ready for review June 6, 2019 14:30
@araspitzu
Copy link
Contributor Author

At commit 3a944e2 all the comments have been addressed, @pm47

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.

Almost there!

@pm47
Copy link
Member

pm47 commented Jun 11, 2019

How about this refactoring:

diff --git a/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala b/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala
index d7c1f4422..8560a4622 100644
--- a/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala
+++ b/eclair-core/src/main/scala/fr/acinq/eclair/io/Peer.scala
@@ -59,9 +59,9 @@ class Peer(nodeParams: NodeParams, remoteNodeId: PublicKey, authenticator: Actor

   when(DISCONNECTED) {
     case Event(Peer.Connect(_, address_opt), d: DisconnectedData) =>
-      address_opt.map(hAndp => new InetSocketAddress(hAndp.getHost, hAndp.getPort)).orElse {
-        getPeerAddressFromNodeAnnouncement()
-      } match {
+      address_opt
+        .map(hostAndPort2InetSocketAddress)
+        .orElse(getPeerAddressFromNodeAnnouncement) match {
         case None =>
           sender ! "no address found"
           stay
@@ -78,7 +78,7 @@ class Peer(nodeParams: NodeParams, remoteNodeId: PublicKey, authenticator: Actor
       }

     case Event(Reconnect, d: DisconnectedData) =>
-      d.address_opt.orElse(getPeerAddressFromNodeAnnouncement()) match {
+      d.address_opt.orElse(getPeerAddressFromNodeAnnouncement) match {
         case _ if d.channels.isEmpty => stay // no-op, no more channels with this peer
         case None                    => stay // no-op, we don't know any address to this peer and we won't try reconnecting again
         case Some(address) =>
@@ -518,7 +518,7 @@ class Peer(nodeParams: NodeParams, remoteNodeId: PublicKey, authenticator: Actor
   }

   // TODO gets the first of the list, improve selection?
-  def getPeerAddressFromNodeAnnouncement(): Option[InetSocketAddress] = {
+  def getPeerAddressFromNodeAnnouncement: Option[InetSocketAddress] = {
     nodeParams.db.network.getNode(remoteNodeId).flatMap(_.addresses.headOption.map(_.socketAddress))
   }

@@ -643,4 +643,6 @@ object Peer {
       case _ => true // if there is a filter and message doesn't have a timestamp (e.g. channel_announcement), then we send it
     }
   }
+
+  def hostAndPort2InetSocketAddress(hostAndPort: HostAndPort): InetSocketAddress = new InetSocketAddress(hostAndPort.getHost, hostAndPort.getPort)
 }

NB: for methods without side effect parentheses should be omitted

@pm47 pm47 assigned pm47 and sstone Jun 11, 2019
@pm47 pm47 unassigned sstone Jun 11, 2019
@araspitzu araspitzu merged commit 818199e into master Jun 11, 2019
@pm47 pm47 deleted the peer_address_from_node_announcement branch June 17, 2019 13:16
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.

Due to failed reconnection, paying an invoice incurs unnecessary fees.
4 participants