[bisq-network/bisq] Check BTC node config without IO overhead (#4081)

[chimp1984]

@chimp1984 requested changes on this pull request.



> @@ -224,11 +228,12 @@ protected void onSetupCompleted() {
                 if (preferences.getBitcoinNodes() != null && !preferences.getBitcoinNodes().isEmpty())
                     peerGroup.setAddPeersFromAddressMessage(false);
 
+                peerGroup.addVersionMessageReceivedEventListener(WalletsSetup.this::handleConfiguration);

We get called from the BitcoinJ thread. We should map it to our userThread to avoid threading issues.
```
peerGroup.addVersionMessageReceivedEventListener((peer, versionMessage) ->
                        UserThread.execute(handleConfiguration(peer, versionMessage)));
```

> @@ -2527,6 +2527,7 @@ It will make debugging easier if you include the bisq.log file by pressing "Open
 
 popup.error.tryRestart=Please try to restart your application and check your network connection to see if you can resolve the issue.
 popup.error.takeOfferRequestFailed=An error occurred when someone tried to take one of your offers:\n{0}
+popup.error.userBtcNodeMisconfigured.explanation=The user-provided Bitcoin node {0} was found to be misconfigured. A user-provided BTC node is either a locally running node that was automatically detected and used, or a node explicitly specified via the --btcNodes command-line option. Bisq requires BTC nodes to have bloom filters enabled and pruning disabled. Automatic use of a local Bitcoin node can be disabled by using the --ignoreLocalBtcNode option.

I think we should not show an Error popup as it comes with the bug-report text and is confusing for that use case, even it is somehow an error as Bisq cannot be used in that case. I would suggest to use warning popup and to reflect that in the display string we should use `popup.warning.userBtcNodeMisConfigured.explanation`. Also changed to camelCase as IDEA static code inspector complains about a typo otherwise. I think misconfigured should be mis-configured in the text.

> @@ -401,6 +401,15 @@ private void setupHandlers() {
         daoPresentation.getBsqSyncProgress().addListener((observable, oldValue, newValue) -> updateBtcSyncProgress());
 
         bisqSetup.setFilterWarningHandler(warning -> new Popup().warning(warning).show());
+
+        walletsSetup.setDisplayUserBtcNodeMisconfigurationHandler(
+                (String peer) ->
+                    new Popup()
+                        .hideCloseButton()
+                        .error(Res.get("popup.error.userBtcNodeMisconfigured.explanation", peer))

As per prev. comment:
-> ` .warning(Res.get("popup.warning.userBtcNodeMisConfigured.explanation", peer))`

> -    private static ListenableFuture<Void> checkPeerConfiguration(Peer peer) {
-      var versionMessageFuture = getVersionMessage(peer);
-
-      Function<VersionMessage,Void> handler =
-        (versionMessage) -> handleConfiguration(peer, versionMessage);
-
-      return Futures.transform(versionMessageFuture, handler);
-    }
-
-    private static Void handleConfiguration(Peer peer, VersionMessage versionMessage) {
+    /* Provided a Peer and its VersionMessage, will check and handle Peer's advertised
+     * configuration. The VersionMessage is provided separately, because it is not
+     * always available and this is meant to be called from a listener that is triggered
+     * when the VersionMessage becomes available.
+     */
+    private Void handleConfiguration(Peer peer, VersionMessage versionMessage) {

The uppercase Void is probably a leftover from the previous code. Should be `void`.

>        } else {
-        log.error("Connected to a peer that is _badly_ configured (will disconnect): {}", peer);
-        peer.close();
+        log.error("Peer _badly_ configured (pruning and/or bloom filters disabled): {}", peer);
+        var usingLocalNode = localBitcoinNode.shouldBeUsed();
+        var usingCustomNodes =
+            BtcNodes.BitcoinNodesOption.CUSTOM.ordinal() == preferences.getBitcoinNodesOptionOrdinal();
+        if (usingLocalNode || usingCustomNodes) {
+            displayUserBtcNodeMisconfigurationHandler.accept(peer.toString());
+        }
       }
       return null;

If `void` is used above the return need to be removed.

-- 
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/4081#pullrequestreview-483225381
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200906/c7022476/attachment.html>