SyncTracker log warnings

[catena2w]

Ergo node have quite a lot of warnings like:

[ergoref-akka.actor.default-dispatcher-17] >> [WARN ] scorex.core.network.SyncTracker >> 20:31:27.260 Trying to clear status for 159.203.94.149/159.203.94.149:9001, but it is not found
[ergoref-akka.actor.default-dispatcher-17] >> [WARN ] scorex.core.network.SyncTracker >> 20:31:27.260 Trying to clear last sync time for 159.203.94.149/159.203.94.149:9001, but it is not found

If it's ok, it's better to reduce log level for this messages, or fix them.

[ceilican]

In my opinion, this is not ok and should be fixed. I inspected the code, and I think this is due to a problem in the handshake logic. Note the following:

1. the log warning are issued by the clearStatus method in SyncTracker.

2. the clearStatus method is only called when NodeViewSynchronizer receives a DisconnectedPeer message from PeerManager.

3. if the clearStatus method is not finding the remote peer whose status should be cleared, this is because the updateStatus method in SyncTracker has never been called for that remote peer.

4. the updateStatus method is called when the NodeViewSynchronizer receives a HandshakedPeer message from PeerManager.

5. But PeerManager only sends a HandshakedPeer message if the handshake succeeded.

From the observations above, we can conclude that the warnings appear because we are disconnecting from a peer with whom we have never successfully handshaked.

Maybe the appropriate solution to this problem would be call updateStatus as soon as we connect with a peer, and not only after we have handshaked with it.

@catena2w @kushti , what do you think?

[ceilican]

@catena2w @kushti, while investigating this issue, it also occurred to me that, since SyncTracker stores information about peers, it might make sense, from an architecture perspective, to move it from NodeViewSynchronizer to PeerManager. What do you think?

[ceilican]

@catena2w @kushti, another thing I noticed is that the peer disconnection logic currently seems to be more complex and messy than it needs to be. PeerManager sends a DisconnectedPeer message to NodeViewSynchronizer when it receives a Disconnected message either from NetworkControler or PeerConnectionHandler. There are 3 different situations in which NetworkController may send a Disconnected message. One of them is when it receives a DisconnectFrom message. However, I could not find any code in Scorex that sends a DisconnectFrom message. So, lines 145--148 of NetworkController seem to be dead code (May I remove it?). Another situation is when NetworkController receives a Blacklist message, but I think blacklisting logic should not be part of the NetworkController (May I move it?).

[ceilican]

So, in summary, here is how I propose that we address this issue and improve code quality:

1. call updateStatus as soon as we connect to a peer and not only if we have handshaked with it.

2. remove lines 145-148 from NetworkController (apparently dead code)

3. move all blacklisting logic to PeerManager

4. move SyncTracker to PeerManager