Bisq's bitcoinj connection handling changes audit

[oscarguindzberg]

• I found a couple of problems that are not related to changes done by bisq on bitcoinj. Here they are:
  • On Bisq project: Different timeouts should be used for socket connection and version message exchange.
    • Use different timeouts because version message exchange with the peer by definition happens after connecting the socket to the peer.
    • See Bitcoin p2p: Use different timeouts  bisq#2609
  • PeerGroup.handlePeerDeath() is called twice for the same peer.
    • The problem is the peer will be added twice to the inactives collection. See: inactives.offer(address)
    • I could reproduce this by running bisq when bitcoinj fails to connect to an onion peer
    • I see 2 “Peer died” on the log for the same peer, which means handlePeerDeath() has been invoked twice.
    • Action
      • As a temporary workaround: PeerGroup.handlePeerDeath(): check for duplicates before calling inactives.offer(address). See oscarguindzberg@2d7c24c
      • Report upstream PeerGroup.handlePeerDeath() + BlockingClient.socket.close() called twice bitcoinj/bitcoinj#1765
• Commit review
  • PeerGroup: Add check to not duplicate peers
    • Commit: 91ab651
    • Manfred’s comment: can be reproduced if u connect to provided bisq nodes and disconnect/reconnect several times. u will get duplicated nodes without that fix.
    • Oscar’s comments:
      • Changes on triggerConnectionsJob
        • addrToTry was just removed from inactives and is being re-added, so there is no chance for duplication here, so no need to check for isAlreadyAdded(). This change probably caused a positive effect because it prevents re-adding a peer if it was duplicated already.
        • Action: Revert change. See oscarguindzberg@b8d1534
      • isAlreadyAdded() method
        • I understand PeerAddress.equals() is not trustworthy since PeerAddress.equals() includes comparison of “time” field which could be different if informed by different peers in “addr” msgs.
        • Problems
          • It just compares by hostname, 2 peers could be run on the same host and different port.
          • PeerAddress could use addr (InetAddress) instead of hostname so the comparison will not work on this case.
        • Actions:
          • Replace usages of PeerAddress.equals() by PeerAddress.equalsIgnoringMetadata()
          • See
            • oscarguindzberg@e06453c
            • oscarguindzberg@3834e1f
            • oscarguindzberg@a1ed4be
      • Change on addInactive()
        • backoffMap.containsKey(peerAddress) already checks for peer existence, there is no need to add an extra check to see if the peer is part of the inactives collection.
        • Action: Revert change. See oscarguindzberg@26889a8
  • PeerGroup: Limit number of Bitcoin network peers at re-connect after connection loss
    • Commit: 3f1382d
    • I could reproduce the bug running the code without the fix.
    • Not sure what is the reason for the bug, but the fix for sure does not cause any harm.
    • Action: keep the commit, just use wrapper methods, see oscarguindzberg@f36a1ea
  • PeerGroup: Add additional check isRunning() at handlePeerDeath to avoid errors at shutDown when using BlockingClient
    • Commit: f76d341
    • The idea is ok, but the implementation is wrong: When the PeerGroup is stopped, first peers are stopped and then vRunning is set to false, specially in BlockingClientManager case, so isRunning() is going to return true during shutdown.
    • Action: Revert change and use upstream solution instead: PeerGroup.stopAsync() set downloadPeer to null before channels are stoped. So, there is no download peer replacement on handlePeerDeath(). See oscarguindzberg@630662f
  • Do not close socket if it is already closed. (Should be called: Do not write to socket if it is already closed)
    • Commit: b90bce9
      • The “if” to not write the socket if the connection is ok. the catch code kind of duplicates the “if”.
      • Action: Remove the special catch, add an “else” statement printing a warning “writing to a closed socket”. See oscarguindzberg@4baf363
• Extra
  • Testing Bisq using all the changes suggested on this audit, I still find bitcoinj errors on the log when the Bisq node is being shut down. I think the problem is the Tor node is shutdown before bitcoinj is told to shutdown, then connection to btc peers fail.
• PR containing the fixes: Connection handling fixes #31