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

Chris Beams notifications at github.com
Wed Feb 19 14:11:38 UTC 2020


cbeams requested changes on this pull request.

NACK on formatting and style issues. There are more than just a couple minor nits here, stuff that's worth getting right with a view toward future PRs being correct from the start.

This is by no means a complete review. I just took a quick look to understand what's changed with the new `LocalBitcoinNode` class and saw these superficial issues and commented on them. Unfortunately I don't have time to do more than that now.

> +    private static NioClient createClient(
+            Peer peer, int port, int connectionTimeout
+    ) throws IOException {

To follow convention, formatting here should be:

```java
private static NioClient createClient(Peer peer, int port, int connectionTimeout) throws IOException {
```

>       */
-    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();
+

Yes, please apply same formatting advice here as I mentioned above on lines 65–67. And apply it consistently to any other similar changes in this PR as well. The general rule in effect here is to keep things on one line up to the 120-char right margin.

> +    private static boolean checkWellConfigured(VersionMessage versionMessage) {
+        var notPruning = versionMessage.hasBlockChain();
+        var supportsAndAllowsBloomFilters =
+                isBloomFilteringSupportedAndEnabled(versionMessage);
+        return notPruning && supportsAndAllowsBloomFilters;
+    }
+
+    /**
+     * Method backported from upstream bitcoinj: at the time of writing, our version is
+     * not BIP111-aware.
+     * Source routines and data can be found in Bitcoinj under:
+     * core/src/main/java/org/bitcoinj/core/VersionMessage.java
+     * and
+     * core/src/main/java/org/bitcoinj/core/NetworkParameters.java
+     */
+

Nit: unnecessary newline here, and this need not be a (`/**`) Javadoc comment as it's on a private method and will never be published. Wrap comments at 90-char right margin, thanks.

> +
+    /**
+     * Method backported from upstream bitcoinj: at the time of writing, our version is
+     * not BIP111-aware.
+     * Source routines and data can be found in Bitcoinj under:
+     * core/src/main/java/org/bitcoinj/core/VersionMessage.java
+     * and
+     * core/src/main/java/org/bitcoinj/core/NetworkParameters.java
+     */
+
+    private static boolean isBloomFilteringSupportedAndEnabled(VersionMessage versionMessage) {
+        // A service bit that denotes whether the peer supports BIP37 bloom filters or not.
+        // The service bit is defined in BIP111.
+        int NODE_BLOOM = 1 << 2;
+
+        int BLOOM_FILTERS_BIP37_PROTOCOL_VERSION = 70000;

Local variables should be camelCase.

-- 
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-361133401
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200219/6de80212/attachment.html>


More information about the bisq-github mailing list