[bisq-network/bisq] Fix premature disconnections from seeds (#5057)

chimp1984 notifications at github.com
Wed Jan 6 04:01:08 CET 2021


Fixes issues with disconntions while peer is still in the initital data request phase.

We have several Request/Repsonse cycles at startup:
1. PreliminaryGetDataRequest  -> GetDataResponse
2. GetUpdatedDataRequest -> GetDataResponse
3. GetBlocksRequest ->GetBlocksResponse
4. GetDaoStateHashesRequest -> GetDaoStateHashesResponse
5. GetBlindVoteStateHashesRequest -> GetBlindVoteStateHashesResponse
6. GetProposalStateHashesRequest -> GetProposalStateHashesResponse

We set the PeerType to INITIAL_DATA_REQUEST but we had reset the PeerType to PEER once seed sent the GetDataResponse for the GetUpdatedDataRequest. This was too early as the PeerManager at connection management disconnects the low-prio connection (PEER) if there are too many connections.
This has led to the problem that the seed closed the connection before the peer can start the GetBlocksRequest. 
Also the last GetDataResponse was at risk to no arrive as the connection might got closed too fast.

We changed that model now so that we count the requests and responses of those bootstrap protocol and if we meet the correct number we assume we are done and reset the PeerType to PEER. As we cannot rely on that we also start a timer to reset after 4 minutes. 
When we exceed 20 connections at a normal peer or 34 at seed nodes (with maxConnections 20) we start to disconnect also peer with type INITIAL_DATA_EXCHANGE. We sort by oldest date when we sent or received a msg, so increase chances that this peer is already done.

We also removed the removeSuperfluousSeedNodes method which was a bit dangerous from the app side as it was protected only by the allowDisconnectSeedNodes which was handled by the GetBlockHandlers, but there was risk that we disconnect before we can request the StateHashes.

We have to take care that seed nodes do not get too weakly connected to healty nodes as they would miss then broadcast messages if all their connections would be occupied by bootstrapping nodes. I doubt that the changes here would have any impact on that behaviour but as its pretty complex we have to be careful with deployment.

I will test it on a small group of seed nodes first and add some more statistics to see which types of connections the seed has.
You can view, comment on, or merge this pull request online at:

  https://github.com/bisq-network/bisq/pull/5057

-- Commit Summary --

  * Refactoring: Move PeerType outside of Connection
  * Add onMessageSent method to MessageListener
  * Add InitialDataRequest and InitialDataResponse marker interface for
  * Refactoring: Rename INITIAL_DATA_REQUEST to INITIAL_DATA_EXCHANGE
  * Add ConnectionState class
  * Remove PeerType from Connection. Use ConnectionState instead.
  * Fix null pointer
  * Use isSeedNode
  * Remove PeerType.SEED_NODE
  * Update display string and UI representation
  * Do sorting at candidates stream.
  * Refactor: Rename lastInitialDataExchangeMessageTimeStamp to lastInitialDataMsgTimeStamp
  * Behaviour change:
  * Refactor:
  * Fix wrong return value for getMaxConnections
  * Behaviour change: Remove removeSuperfluousSeedNodes method
  * Behaviour change: Remove setAllowDisconnectSeedNodes method
  * Add safety filter to removeAnonymousPeers

-- File Changes --

    M core/src/main/java/bisq/core/app/P2PNetworkSetup.java (2)
    M core/src/main/java/bisq/core/app/misc/AppSetupWithP2P.java (6)
    M core/src/main/java/bisq/core/dao/monitoring/network/messages/GetStateHashesRequest.java (4)
    M core/src/main/java/bisq/core/dao/monitoring/network/messages/GetStateHashesResponse.java (4)
    M core/src/main/java/bisq/core/dao/node/full/network/FullNodeNetworkService.java (1)
    M core/src/main/java/bisq/core/dao/node/lite/network/LiteNodeNetworkService.java (9)
    M core/src/main/java/bisq/core/dao/node/messages/GetBlocksRequest.java (3)
    M core/src/main/java/bisq/core/dao/node/messages/GetBlocksResponse.java (4)
    M core/src/main/java/bisq/core/user/Cookie.java (7)
    M core/src/main/resources/i18n/displayStrings.properties (1)
    M desktop/src/main/java/bisq/desktop/main/settings/network/P2pNetworkListItem.java (13)
    M p2p/src/main/java/bisq/network/p2p/ExtendedDataSizePermission.java (2)
    A p2p/src/main/java/bisq/network/p2p/InitialDataRequest.java (22)
    A p2p/src/main/java/bisq/network/p2p/InitialDataResponse.java (22)
    M p2p/src/main/java/bisq/network/p2p/P2PService.java (1)
    M p2p/src/main/java/bisq/network/p2p/network/Connection.java (54)
    A p2p/src/main/java/bisq/network/p2p/network/ConnectionState.java (151)
    M p2p/src/main/java/bisq/network/p2p/network/MessageListener.java (3)
    A p2p/src/main/java/bisq/network/p2p/network/PeerType.java (27)
    M p2p/src/main/java/bisq/network/p2p/peers/PeerManager.java (119)
    M p2p/src/main/java/bisq/network/p2p/peers/getdata/RequestDataManager.java (3)
    M p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataRequest.java (4)
    M p2p/src/main/java/bisq/network/p2p/peers/getdata/messages/GetDataResponse.java (4)
    M p2p/src/main/java/bisq/network/p2p/peers/peerexchange/PeerExchangeHandler.java (3)
    M p2p/src/main/java/bisq/network/p2p/peers/peerexchange/PeerExchangeManager.java (3)
    M p2p/src/test/java/bisq/network/p2p/MockNode.java (14)
    M p2p/src/test/java/bisq/network/p2p/peers/PeerManagerTest.java (28)

-- Patch Links --

https://github.com/bisq-network/bisq/pull/5057.patch
https://github.com/bisq-network/bisq/pull/5057.diff

-- 
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/bisq/pull/5057
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20210105/210fb8f9/attachment.htm>


More information about the bisq-github mailing list