Find and switch peers #4408

solo1g · 2022-06-20T10:36:28Z

Should do the following:

Find a peer to sync with
Try syncing with it
If any issues with that peer (disconnection, too many connection drops, ignored queries etc), then find and use another.

Key changes

Added a PeerFinder which deals with peer discovery. Moved relevant parts from PeerManager to PeerFinder.
Added functionality to query peers and get notified if the query times out. Used to switch peers in case the current one does not work. Adds timeouts to Initializing and a new state Waiting in PeerMessageReceiver.
Changed InitializeDisconnect state to now disconnect when connecting too. Added StopReconnect state to PeerMessageReceiver to stop reconnect from our side when not connected.
Some fixes dealing with edge cases in P2PClient and PeerMessageReceiver.

solo1g · 2022-06-21T14:33:43Z

So this should be working as intended now. I will leave comments on all the changes before taking the draft off. Meanwhile, this could be tested by running with the following config

peers = []
maxConnectedPeers = 8
enable-peer-discovery = true
use-default-peers = false

changes timeouts, fix issue with ipv6 dns seeds, initDisconnect when disconnected bug, dbPeers order fix

solo1g · 2022-06-22T10:13:21Z

core/src/main/scala/org/bitcoins/core/p2p/NetworkPayload.scala

@@ -166,13 +166,27 @@ object GetDataMessage extends Factory[GetDataMessage] {
    GetDataMessage(Seq(inventory))
 }

+sealed trait ExpectsResponse {


So I needed a way to if the peer failed to respond to a query for eg getheaders etc that is used in sync, so that I can switch to a different peer and continue sync from there. For NetworkPayload that extends this, an ExpectReponse message is sent to the P2PClient right after sending the message. The message initiates a timer in PeerMessageReceiver that notifies if the message timed out.

Was this common when testing? This is an interesting blog post about the bitcoin p2p network possibly slowing down: https://blog.lopp.net/is-bitcoin-network-slowing-down/

it details the logic bitcoin core has about detecting peers that are stalling, probably worth a read. It seems the behavior detailed there is what you experienced as well?

Interesting 👀 .
Not very common but did see this a few times. Instead of being connected and not responding, what I found more common was for the peer to straight up disconnect us after initialization. Reconnection would be successful followed by the same behavior.

Current behavior is for timeouts to complete instantly and requery another peer (same if that's all we have) in case of a disconnection and wait for the whole duration in case its still connected. This covers both cases.

solo1g · 2022-06-22T10:14:09Z

node-test/src/test/scala/org/bitcoins/node/NeutrinoNodeTest.scala

-        f <- isAllDisconnectedF(node)
-      } yield f
-      disconnF.map(assert(_))
+      def peerManager = node.peerManager


Did away with using indexes and added check to ensure we have 2 peers.

node-test/src/test/scala/org/bitcoins/node/UnsyncedNeutrinoNodeTest.scala

solo1g · 2022-06-22T10:20:32Z

node-test/src/test/scala/org/bitcoins/node/UnsyncedNeutrinoNodeTest.scala

+  }
+
+  //todo: so the disconnection should rather originate from the remote client rather than our side
+  ignore must "sync with another second peer if the first one is disconnected" in {


Reconnect after stopping was throwing some exception about a closed port. Looking into it, didn't want to block review on minor stuff so marked to ignore test at the moment,

node/src/main/resources/postgresql/node/migration/V3__peer_table.sql

solo1g · 2022-06-22T10:25:36Z

node/src/main/scala/org/bitcoins/node/NeutrinoNode.scala

@@ -26,7 +26,7 @@ case class NeutrinoNode(
    nodeConfig: NodeAppConfig,
    chainConfig: ChainAppConfig,
    actorSystem: ActorSystem,
-    configPeersOverride: Vector[Peer] = Vector.empty)
+    paramPeers: Vector[Peer] = Vector.empty)


So we don't override the config ones anymore, rather use both. This was one of the suggestion earlier so added in this.

solo1g · 2022-06-22T10:33:15Z

node/src/main/scala/org/bitcoins/node/NeutrinoNode.scala

    val sendCompactFilterHeaderMsgF = {
-      peerManager.randomPeerMsgSenderWithCompactFilters
+      syncPeerMsgSender


So unless the peers are all ideal, can't really use a random one, besides without parallel sync, not much point in switching. The previous change too was by me, so fixed it now. Would add this later with properly working parallel sync.

solo1g · 2022-06-22T10:33:39Z

node/src/main/scala/org/bitcoins/node/Node.scala

-      }
-    isInitializedF
+  def send(msg: NetworkPayload, peer: Peer): Future[Unit] = {
+    peerManager.peerData(peer).peerMessageSender.sendMsg(msg)


Alll of this is now in PeerManager

solo1g · 2022-06-22T10:36:06Z

node/src/main/scala/org/bitcoins/node/PeerData.scala

-      onReconnect = node.sync
+      onReconnect = node.peerManager.onReconnect,
+      onStop = node.peerManager.onP2PClientStopped,
+      maxReconnectionTries = 4


Changed max attempts to 4 as this would roughly be the time that we spend on each peer waiting for it to initialize. Ig all of these related constant need to be put together.

node/src/main/scala/org/bitcoins/node/PeerData.scala

solo1g · 2022-06-22T10:38:11Z

node/src/main/scala/org/bitcoins/node/PeerFinder.scala

+    extends P2PLogger {
+
+  /** Returns peers by querying each dns seed once. These will be IPv4 addresses. */
+  def getPeersFromDnsSeeds: Vector[Peer] = {


Moved this from manager to a new Finder to separate things.

solo1g · 2022-06-22T10:42:10Z

node/src/main/scala/org/bitcoins/node/PeerManager.scala

+    logger.debug(s"Removing persistent peer $peer")
+    val client = peerData(peer).client
+    _peerData.remove(peer)
+    //so we need to remove if from the map for connected peers so no more request could be sent to it but we before


When we don't want a peer as one of our persistent ones, we remove it from the peerData map, but still keep it around to check if the client actor for it actually stopped.

node/src/main/scala/org/bitcoins/node/NeutrinoNode.scala

Christewart · 2022-06-24T13:43:05Z

I was able to sync correctly on c5f0f9c with the default out of the box settings 🎉 . I'm going to start playing with the various settings introduced in this PR but wanted to document that things do seem to work out of the box.

Christewart · 2022-06-24T14:24:36Z

Sync worked correctly for me on c5f0f9c

with these settings

bitcoin-s.node.use-default-peers = false
bitcoin-s.node.enable-peer-discovery = true

Christewart · 2022-06-24T17:45:33Z

Things work on c5f0f9c for me with IBD and running a few hours with the following settings

bitcoin-s.node.use-default-peers = false
bitcoin-s.node.enable-peer-discovery = true
bitcoin-s.node.maxConnectedPeers = 8

IIUC what the logs means, it should be reworded to something about disconnecting. Also does this mean they disconnected from us or does it mean we disconnected from them?

2022-06-24T16:49:33UTC WARN [PeerManager] onP2PClientStopped called for unknown Peer(51.38.11.15:8333)

solo1g · 2022-06-25T06:37:05Z

2022-06-24T16:49:33UTC WARN [PeerManager] onP2PClientStopped called for unknown Peer(51.38.11.15:8333)

Was this common? Ideally, you should not be seeing this. This basically means somewhere down the line a peer got "lost" from the PeerManager before it's ciient actor was stopped.

Edit: Fixed this. This happened when one of the peers while it still is initalizing is received again in an addr message and another actor is created for it while still having only one entry in peerData.

* fix node test shared fixtures bug The cached bitcoind fixtures were used and stopped in UnsyncedNeutrinoNodeTest which causes an error if NeutrinoNodeTest is run at the same time on high performant systems, which is why it escaped CI. Merges NeutrinoNodeTest and UnsyncedNeutrinoNodeTest * fix possible issues with PeerMessageReceiverTest This reverts commit 55e7caf. * fix filter sync issue when wallet creation time indicates already synced * move switch peer test to NeutrinoNodeWithUnachedBitcoindTest

solo1g force-pushed the mp-switch branch 2 times, most recently from ea29d12 to 6a14c00 Compare June 21, 2022 13:25

Christewart added node work for the node project neutrino Implements the Bitcoin-S Neutrino node labels Jun 21, 2022

Christewart added this to the 2.0 milestone Jun 21, 2022

solo1g mentioned this pull request Jun 22, 2022

Update hardcoded seeds #4412

Merged

solo1g added 10 commits June 22, 2022 14:29

add support to find and switch peers

6869a83

fix compile on 2.12

9045a8b

allow empty config peers in regtest

3236be6

fix test

299bb71

minor fixes

e35c493

changes timeouts, fix issue with ipv6 dns seeds, initDisconnect when disconnected bug, dbPeers order fix

fix: not removing peers on initialization timeout

c0b68e8

fix: query again when previous failed

96e29c8

fix: wrong condition for deferred peers

dc8786c

restore log levels

57b3f25

clean up

d4b8c57

solo1g force-pushed the mp-switch branch from 6a14c00 to d4b8c57 Compare June 22, 2022 10:02

solo1g commented Jun 22, 2022

View reviewed changes

node-test/src/test/scala/org/bitcoins/node/UnsyncedNeutrinoNodeTest.scala Show resolved Hide resolved

solo1g commented Jun 22, 2022

View reviewed changes

node/src/main/resources/postgresql/node/migration/V3__peer_table.sql Outdated Show resolved Hide resolved

solo1g commented Jun 22, 2022

View reviewed changes

node/src/main/scala/org/bitcoins/node/PeerData.scala Show resolved Hide resolved

solo1g commented Jun 22, 2022

View reviewed changes

solo1g added 3 commits June 23, 2022 15:42

changes from comments

c5f0f9c

use StartStopAsync

e927120

changes from comments

7215743

Christewart reviewed Jun 24, 2022

View reviewed changes

node/src/main/scala/org/bitcoins/node/NeutrinoNode.scala Outdated Show resolved Hide resolved

solo1g added 2 commits June 27, 2022 09:31

fix switch if peer down test

dea9a0f

changes from comments

3c44c30

solo1g force-pushed the mp-switch branch from 42549d8 to 3c44c30 Compare June 27, 2022 07:47

solo1g requested a review from Christewart June 27, 2022 08:22

Christewart merged commit 42564bc into bitcoin-s:master Jun 28, 2022

Christewart added this to Done in 2.0 Jun 28, 2022

Christewart mentioned this pull request Jun 29, 2022

Read NULL value (null) for ResultSet column <computed> slick.SlickException: Read NULL value (null) for ResultSet column <computed> #4437

Closed

solo1g mentioned this pull request Jul 1, 2022

Fix sync issues post #4408 #4441

Merged

solo1g mentioned this pull request Jul 8, 2022

Unable to restart BitcoindRpcClient #4465

Open

Christewart modified the milestones: 2.0, 1.9.2, 1.9.3 Aug 6, 2022

Christewart removed this from Done in 2.0 Aug 6, 2022

Christewart added this to In Progress in 1.9.3 via automation Aug 6, 2022

Christewart moved this from In Progress to Done in 1.9.3 Aug 6, 2022

This was referenced Oct 13, 2022

bitcoin-s.node.enable-peer-discovery=true leads to errors when tor is enabled #4825

Closed

Don't do peer discovery forever #4826

Closed

Christewart mentioned this pull request Oct 25, 2022

Enable peer discovery by default #4862

Merged

Christewart mentioned this pull request Dec 31, 2022

Implement ability to cancel background task for querying peer with specific services #4937

Merged

Christewart mentioned this pull request Mar 23, 2023

Remove fetch compact filters in NeutrinoNode.sync() before fetching block headers / compact filter headers #5023

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Find and switch peers #4408

Find and switch peers #4408

solo1g commented Jun 20, 2022 •

edited

solo1g commented Jun 21, 2022

solo1g Jun 22, 2022

Christewart Jun 22, 2022

solo1g Jun 22, 2022 •

edited

solo1g Jun 22, 2022 •

edited

solo1g Jun 22, 2022 •

edited

solo1g Jun 22, 2022

solo1g Jun 22, 2022 •

edited

solo1g Jun 22, 2022

solo1g Jun 22, 2022

solo1g Jun 22, 2022

solo1g Jun 22, 2022

Christewart commented Jun 24, 2022

Christewart commented Jun 24, 2022

Christewart commented Jun 24, 2022 •

edited

solo1g commented Jun 25, 2022 •

edited

Find and switch peers #4408

Find and switch peers #4408

Conversation

solo1g commented Jun 20, 2022 • edited

Key changes

solo1g commented Jun 21, 2022

solo1g Jun 22, 2022

Choose a reason for hiding this comment

Christewart Jun 22, 2022

Choose a reason for hiding this comment

solo1g Jun 22, 2022 • edited

Choose a reason for hiding this comment

solo1g Jun 22, 2022 • edited

Choose a reason for hiding this comment

solo1g Jun 22, 2022 • edited

Choose a reason for hiding this comment

solo1g Jun 22, 2022

Choose a reason for hiding this comment

solo1g Jun 22, 2022 • edited

Choose a reason for hiding this comment

solo1g Jun 22, 2022

Choose a reason for hiding this comment

solo1g Jun 22, 2022

Choose a reason for hiding this comment

solo1g Jun 22, 2022

Choose a reason for hiding this comment

solo1g Jun 22, 2022

Choose a reason for hiding this comment

Christewart commented Jun 24, 2022

Christewart commented Jun 24, 2022

Christewart commented Jun 24, 2022 • edited

solo1g commented Jun 25, 2022 • edited

solo1g commented Jun 20, 2022 •

edited

solo1g Jun 22, 2022 •

edited

solo1g Jun 22, 2022 •

edited

solo1g Jun 22, 2022 •

edited

solo1g Jun 22, 2022 •

edited

Christewart commented Jun 24, 2022 •

edited

solo1g commented Jun 25, 2022 •

edited