[bisq-network/bisq] Add local Bitcoin node configuration detection (#3982)

[sqrrm]

sqrrm requested changes on this pull request.

Tested it and it works as expected, nice.

The errors logged by NioClientManager would be good to get rid of since most users won't be running with a local node so they would have big errors on startup. Not that familiar with logging, but if you have a way to do it I would say it's good.

I think the unchecked usages of `Optional` should be checked, perhaps use `orElse()`, didn't comment on all uses as there are quite a few.

NIT: Prefer no empty line between comment and method.

>              nextStep.run();
             return;
         }
 
-        localBitcoinNode.detectAndRun(nextStep);
+        // Results of the check don't have to be passed to nextStep,
+        // because they're cached in LocalBitcoinNode and dependent routines query it themselves.
+        localBitcoinNode.checkUsable();
+
+        // Here we only want to provide the user with a choice (in a popup) in case a local node is
+        // detected, but badly configured.
+        var detectedButMisconfigured = localBitcoinNode.isDetectedButMisconfigured().get();

Since `localBitcoinNode.isDetectedButMisconfigured()` is an optional it would be prudent to check that it's present here. Although it might be certain for now that it's never null the characteristics of an optional is that it should be able to handle the null state.

> @@ -862,7 +879,7 @@ private void maybeShowSecurityRecommendation() {
     }
 
     private void maybeShowLocalhostRunningInfo() {
-        maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.isDetected());
+        maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.isUsable().get());

Same here with the usage of `Optional` without checking `isPresent()`.

>       */
-    public void detectAndRun(Runnable callback) {
-        try (Socket socket = new Socket()) {
-            socket.connect(new InetSocketAddress("127.0.0.1", port), CONNECTION_TIMEOUT);
+
+    private static Peer createLocalPeer(
+            int port
+    ) throws UnknownHostException {
+        // TODO: what's the effect of NetworkParameters on handshake?
+        // i.e. is it fine to just always use MainNetParams?
+        var networkParameters = new MainNetParams();
+

So much space in this method...

> +        if (clientVersion >= whenBloomFiltersWereIntroduced
+                && clientVersion < whenBloomFiltersWereDisabledByDefault)
+            return true;
+        return (localServices & NODE_BLOOM) == NODE_BLOOM;
+    }
+
+    /**
+     * Initiates detection and configuration checks. The results are cached so that the public
+     * methods isUsable, isDetected, isWellConfigured don't trigger a recheck.
+     */
+
+    public boolean checkUsable() {
+        var optionalVersionMessage = attemptHandshakeForVersionMessage();
+        handleHandshakeAttempt(optionalVersionMessage);
+        // We know that the Optional/s will be populated by the end of the checks.
+        return isUsable().get();

Same with the checks for `Optional.isPresent()`.

-- 
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/3982#pullrequestreview-360525804
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200218/3addd6a6/attachment.html>