[bisq-network/bitcoinj] [WIP] Bisq's bitcoinj connection handling changes audit (#30)

[Oscar Guindzberg]

- bisq’s bitcoinj connection handling changes audit
    - replaced commits with log lines and testing
        - in bitcoinj i can get “triggerConnectionsJob problem with size” sometimes, 2 extra peers are added
        - 
    - PeerGroup: Add check to not duplicate peers
        - Commit: https://github.com/bisq-network/bitcoinj/commit/91ab651c8cdc8137d3f99ffc65c98f664858b07e 
        - manfred: can be reproduced if u connect to provided bisq nodes and disconnect/reconnect several times. u will get duplicated nodes without that fix.
        - Change on triggerConnectionsJob
            - addrToTry was just removed from inactives and is being readded. The only scenario where this fix could help is when the peer was already duplicated on inactives
        - isAlreadyAdded() method
            - 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 comparation will not work on this case. Change implemented by oscar recently might make this problem more reproducible https://github.com/bisq-network/bitcoinj/commit/ebeecab00dd565eabc6bd064f96e5aa408b1d59f
        - Change on addInactive() 
            - backoffMap.containsKey(peerAddress) already checks for peer existence. The only scenario where this fix could help is where PeerAddress.equals() does not work well. 
        - Problems with PeerAddress.equals()
            - The same bitcoin instance could be reprensented with a hostname or an addr. If represented by addr it could be located with ipv4 or ipv6 (rare case)
            - time could be different if informed by different peers in “addr” msg
            - 
        - Investigate PeerGroup.handlePeerDeath() being called several times for the same peer.
            - if that is the case peer will be added several times to inactives collection. See: inactives.offer(address)
            - I found a case for this by running bisq
                - When fails to connect to onion peer
                - I see 2 “peer died” on the log for the same peer, which means handlePeerDeath() has been invoked twice.
                - Places where handlePeerDeath() is invoked
                    - BlockingClient constructor on finally calls connection.connectionClosed() (connection is of type Peer) which invokes handlePeerDeath
                    - Peer extends AbstractTimeoutHandler that eventually calls Peer.timeoutOccurred() which calls Peer.connectionClosed() which invokes handlePeerDeath
                    - PeerGroup channels.openConnection catch calls handlePeerDeath (but I that one is not being called because i see no "Failed to connect to” on the log and the thread name on the log is either BlockingClient or AbstractTimeoutHandler)
                - 
                - 
                - 
    - PeerGroup: Limit number of Bitcoin network peers at re-connect after connection loss
        - Commit: https://github.com/bisq-network/bitcoinj/commit/3f1382da7e4babd7cbec03c2208cd3f8f1f1e67c 
        - Maybe related to https://github.com/bisq-network/bitcoinj/commit/91ab651c8cdc8137d3f99ffc65c98f664858b07e 
        - many peers added to inactive collection
        - if kept, code should be: if (countConnectedAndPendingPeers() < getMaxConnections())
        - Maybe cause by executor executing several threads in parallel, so many connections are created?
        - Most likely does not cause any harm
    - PeerGroup: Add additional check isRunning() at handlePeerDeath to avoid errors at shutDown when using BlockingClient
        - Commit: https://github.com/bisq-network/bitcoinj/commit/f76d34195f2f1d0696830942a9c4df5a0b657200 
        - 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
        - Use upstream solution: PeerGroup.stopAsync() set downloadPeer to null before channels are stop. So, there is no download peer replacement on handlePeerDeath  .
        - Many times there are  errors when stopping bitcoinj
        - Test running bitcoinj with several peers and see how it works.
    - Do not close socket if it is already closed. (Should be called: Do not write to socket if it is already closed)
        - Commit: https://github.com/bisq-network/bitcoinj/commit/b90bce9fd0aa040e27f605e75b09f3e881e011b6 
        - Probably related to https://github.com/bisq-network/bitcoinj/commit/f76d34195f2f1d0696830942a9c4df5a0b657200 
            - Several ocurrences of writing to a closed socket was during app shutdown and caused by handlePeerDeath trying to download the blockchain from another peer
            - I saw one occurrence when app was trying to connect to peers during startup
            - Once that is solved, there is no need for this commit
            - If that is not the case… probably the if to not write the socket if the connection is ok. the catch code kind of duplicates the “if”… if we put the”if”, there is no need to catch and ignore that exception. I would add an “else” statement printing a warning “writing to a closed socket.
            - 
        - Fix PeerTest 
            - failing tests on bisq's bitcoinj branch https://github.com/bisq-network/bitcoinj/issues/19 
            - Fails because of https://github.com/bisq-network/bitcoinj/commit/b90bce9fd0aa040e27f605e75b09f3e881e011b6 
            - writeTarget.writeBytes() is used to check the connection in fact was closed
            - Implementation before the commit used to throw an exception if connection is closed().


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bitcoinj/issues/30
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190329/b6b362ae/attachment.html>