Add local Bitcoin node configuration detection #3982

dmos62 · 2020-02-17T22:56:43Z

Refactors LocalBitcoinNode and adds detection for local Bitcoin node's
configuration, namely, whether it is pruning and whether it has bloom
filter queries enabled.

The local node's configuration (and its presence) is retrieved by
performing a Bitcoin protocol handshake, which includes the local
Bitcoin node sending us its version message (VersionMessage in
BitcoinJ), which contains the information we're interested in.

Due to some quirky BitcoinJ logic, sometimes the handshake is
interrupted, even though we have received the local node's version
message. That contributes to the handshake handling in LocalBitcoinNode
being a bit complicated.

Refactoring consists of two principle changes: the public interface is
split into methods that trigger checks and methods that retrieve the
cached results. The methods that trigger checks have names starting
with "check", and methods that retrieve the cached results have names
that start with "is".

The other major refactor is the use of Optional instead of
boolean for storing and returning the results, an empty Optional
signifying that the relevant check was not yet performed. Switching to
Optionals has caused other code that queries LocalBitcoinNode to throw
an exception in case the query is made before the checks are. Before,
the results were instantiated to "false" and that would be returned
in case the query was made before the checks completed. This change has
revealed one occasion (Preferences class) where this happens.

This is shown in case a local Bitcoin node is found running, but misconfigured:

This is shown when setup with a local Bitcoin node is completed, to remind the user to have the node fully synced before using Bisq, and what the other requirements are:

I hope to receive some phrasing corrections and maybe some UI ideas? I'm not totally happy with the "information" popup above: it's a bit redundant.

Possible next steps:

tell the user what exactly is misconfigured;
option to retry the check.

I could have spent a bit more time and implemented these features too, but it's already two weeks that I'm doing this, so I'm just thrilled that I have something to PR. I can work on these later; I'll let you decide if these features are worthwhile.

Relevant issue: #3273

Refactors LocalBitcoinNode and adds detection for local Bitcoin node's configuration, namely, whether it is pruning and whether it has bloom filter queries enabled. The local node's configuration (and its presence) is retrieved by performing a Bitcoin protocol handshake, which includes the local Bitcoin node sending us its version message (VersionMessage in BitcoinJ), which contains the information we're interested in. Due to some quirky BitcoinJ logic, sometimes the handshake is interrupted, even though we have received the local node's version message. That contributes to the handshake handling in LocalBitcoinNode being a bit complicated. Refactoring consists of two principle changes: the public interface is split into methods that trigger checks and methods that retrieve the cached results. The methods that trigger checks have names starting with "check", and methods that retrieve the cached results have names that start with "is". The other major refactor is the use of Optional<Boolean> instead of boolean for storing and returning the results, an empty Optional signifying that the relevant check was not yet performed. Switching to Optionals has caused other code that queries LocalBitcoinNode to throw an exception in case the query is made before the checks are. Before, the results were instantiated to "false" and that would be returned in case the query was made before the checks completed. This change has revealed one occasion (Preferences class) where this happens.

dmos62 · 2020-02-17T22:58:53Z

core/src/main/java/bisq/core/app/BisqSetup.java

@@ -559,7 +576,7 @@ else if (displayTorNetworkSettingsHandler != null)

        // We only init wallet service here if not using Tor for bitcoinj.
        // When using Tor, wallet init must be deferred until Tor is ready.
-        if (!preferences.getUseTorForBitcoinJ() || localBitcoinNode.isDetected()) {


Removed redundant LocalBitcoinNode call, because Preferences.getUseTorForBitcoinJ makes the same call and it has the same effect.

If localBitcoinNode.willUse() == true and config.useTorForBtcOptionSetExplicitly == true the new code will behave differently than before. So I don't think we should remove this from here.

You're right, I missed that. Thanks! My concern here is that this line decides whether to "initiate wallet" at this time, which implies that Tor will not be used. Ideally Preferences.getUseTorForBitcoinJ() would be a full authority on whether or not we're using Tor, but here it's only one half of the condition. So I can restore initial logic, but ideally I'd like to restructure this. I'll see what I can do.

dmos62 · 2020-02-17T23:01:27Z

desktop/src/main/java/bisq/desktop/main/MainViewModel.java

@@ -441,10 +451,12 @@ private void setupBtcNumPeersWatcher() {
                checkNumberOfBtcPeersTimer = UserThread.runAfter(() -> {
                    // check again numPeers
                    if (walletsSetup.numPeersProperty().get() == 0) {
-                        if (localBitcoinNode.isDetected())
-                            getWalletServiceErrorMsg().set(Res.get("mainView.networkWarning.localhostBitcoinLost", Res.getBaseCurrencyName().toLowerCase()));


Apart from the .isDetected() -> .isUsable.get() line, these are just formatting changes.

dmos62 · 2020-02-17T23:02:19Z

core/src/main/java/bisq/core/user/Preferences.java

+        // To keep the method's behaviour unchanged, until a fix is implemented, we use
+        // Optional.orElse(false). Here 'false' normally means that the checks were performed
+        // and a suitable local Bitcoin node wasn't found.
+        var usableLocalNodePresent = localBitcoinNode.isUsable().orElse(false);


This is the bug I mentioned.

sqrrm · 2020-02-18T08:19:00Z

@dmos62 Both travis and codacy fail.

I will test this locally once it's building ok.

I was erroniously targeting Java 11, when Travis requires Java 10. `Optional.isEmpty` shows up only in Java 11.

The workings of LocalBitcoinNode significantly changed, especially how detection works. Before, we were only checking if a port was open, but now we're actually performing a Bitcoin protocol handshake, which is difficult to stub. For these reasons the old tests are irrelevant and replacement tests were not written.

dmos62 · 2020-02-18T11:14:34Z

The implementation relies on BitcoinJ's NioClient attempting a handshake, but if that fails (for example when there isn't a locally running Bitcoin node), NioClient logs errors and exception to console, which is a bit annoying since we're already properly handling each case. Here's a screenshot:

We could get the relevant BitcoinJ's logger and suppress it while we're doing these checks, but that feels a bit hacky (or does it?). Tell me how you feel about that.

dmos62 · 2020-02-18T11:19:43Z

I have made some changes to satisfy one of two Codacy complaints, and to make the other clearer. The other is a future callback that does nothing in case of failure, so onFailure is implemented, as required, but empty. So I put in a semi-colon to signify that it's meant to be empty:

        public void onFailure(Throwable thr) {  
            ;                                                
        }

I guess, I could have also did public void onFailure(Throwable thr) {}, and I think that was the original, but I passed the files through IntelliJ's reformatter and it forced a multiline declaration, so I thought I'd leave it at that.

dmos62

I was surprised that the test suite for LocalBitcoinNode wasn't automatically executed, or compiled, when I was running ./gradlew desktop:build and desktop:run.

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

Co-Authored-By: sqrrm <sqrrm@users.noreply.github.com>

sqrrm

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.

sqrrm · 2020-02-18T17:19:57Z

core/src/main/java/bisq/core/app/BisqSetup.java

+
+        // 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.

I took your suggestion about getting rid of most of the Optional.get()s in other places, but in my opinion here a check would be superfluous, because it's easy to see that getting an empty Optional is impossible here with the current implementation of LocalBitcoinNode (.checkUsable() on the previous line guarantees that). That said if it were to change, all the calls would have to be reconsidered anyway, right?

sqrrm · 2020-02-18T17:24:55Z

core/src/main/java/bisq/core/app/BisqSetup.java

@@ -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().

sqrrm · 2020-02-18T17:45:34Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        // 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...

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.

sqrrm · 2020-02-18T18:26:41Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        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().

cbeams

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.

cbeams · 2020-02-19T14:03:14Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

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


To follow convention, formatting here should be:

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

cbeams · 2020-02-19T14:06:02Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        // 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.

cbeams · 2020-02-19T14:07:38Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.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.

cbeams · 2020-02-19T14:08:26Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        // The service bit is defined in BIP111.
+        int NODE_BLOOM = 1 << 2;
+
+        int BLOOM_FILTERS_BIP37_PROTOCOL_VERSION = 70000;


Local variables should be camelCase.

Do you feel strongly about this? These are constants, in the semantic sense, that I encapsulated in a single method with the code that uses them, and formating them as such helps single them out. I didn't want to separate them out to top-level static variables, because they're part of a small port from upstream BitcoinJ and I wanted to keep it completely separate from the rest of the class.

In my opinion this kind of containment is good. Camel case local variables would not be appropriate, but making them local seems right considering the situation.

sqrrm

utACK

I will test this before merging. @dmos62 if you feel like removing some of the extra empty lines in some of the methods meanwhile, please don't hesitate. I find the code harder to read with too much space as it breaks the flow compared with the other parts of the project.

sqrrm · 2020-02-22T17:17:17Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        // The service bit is defined in BIP111.
+        int NODE_BLOOM = 1 << 2;
+
+        int BLOOM_FILTERS_BIP37_PROTOCOL_VERSION = 70000;


In my opinion this kind of containment is good. Camel case local variables would not be appropriate, but making them local seems right considering the situation.

dmos62 · 2020-02-22T21:20:22Z

if you feel like removing some of the extra empty lines in some of the methods meanwhile, please don't hesitate.

@sqrrm could you point out examples of empty lines you would remove? I put some consideration into where I put empty lines. Usually to outline distinct steps in a method. Would appreciate feedback.

sqrrm

I added a comment on some of the blank lines I would remove. Personally I would probably make it even tighter though.

I understand the motivation of separating some of the concepts with blank lines, but it doesn't really help with readability since the methods themselves already contain the scope of activity.

sqrrm · 2020-02-22T22:59:25Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        // This initiates the handshake procedure, which, if successful, will complete
+        // the peerVersionMessageFuture, or be cancelled, in case of failure.
+        NioClient client = new NioClient(serverAddress, peer, connectionTimeout);
+


sqrrm · 2020-02-22T22:59:41Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        // We must construct a BitcoinJ Context before using BitcoinJ. We don't keep a
+        // reference, because it's automatically kept in a thread local storage.
+        new Context(networkParameters);
+


sqrrm · 2020-02-22T22:59:46Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        new Context(networkParameters);
+
+        var ourVersionMessage = new VersionMessage(networkParameters, 0);
+


sqrrm · 2020-02-22T22:59:52Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        var ourVersionMessage = new VersionMessage(networkParameters, 0);
+
+        var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port);
+


sqrrm · 2020-02-22T23:00:10Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+        var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port);
+
+        AbstractBlockChain blockchain = null;
+


sqrrm · 2020-02-22T23:02:16Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+
+        peer.close();
+        client.closeConnection();
+


sqrrm · 2020-02-22T23:02:30Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+
+    private ListenableFuture<VersionMessage> getVersionMessage(Peer peer) {
+        SettableFuture<VersionMessage> peerVersionMessageFuture = SettableFuture.create();
+


Here rationale is to separate the creation of the future from the parts that use it. The next three parts of the method operate on this future.

sqrrm · 2020-02-22T23:02:45Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+             * stacktrace).
+             */
+            var whenChecksNotFinished = false;
+


sqrrm · 2020-02-22T23:02:51Z

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java

+            var whenChecksNotFinished = false;
+
+            var throwable = new Throwable("LocalBitcoinNode was queried before it was ready");
+


sqrrm · 2020-02-22T23:03:03Z

core/src/main/java/bisq/core/user/Preferences.java

+        // Optional<Boolean> istead of boolean, an empty Optional signifying that the relevant
+        // check has not yet been performed.
+        var usableLocalNodePresent = localBitcoinNode.safeIsUsable();
+


dmos62 · 2020-02-23T07:48:44Z

@sqrrm thanks for the feedback. I appreciate it. In all your noted cases my motivation was to make visual some structure I saw in the methods. I commented under one of them detailing that a bit further. I would not feel comfortable removing those empty lines. We could write this off as stylistic differences. I'll say again that I do appreciate the feedback: I wasn't aware that this feature of my code was jarring to some.

Further thinking on this, I would say that I'm minimally sensitive to vertical verbosity (or horizontal for that matter). It might have to do with how I use my vim when working. I jump around the code a lot and information density on-screen at any given time is not really a direct factor for me.

cbeams · 2020-02-26T19:39:04Z

I suggest disengaging from style discussions at this point. If the substantive changes in this PR are otherwise ACK-worthy, then merge it, and someone can issue a follow-up PR correcting the deviations. It’ll be faster that way. I’ll do it myself, even. Alternatively, one of the maintainers could push those changes against the current PR branch, but it’s probably better in this situation to have a separate PR that documents the rework necessary. @dmos, thanks for your contributions, but in a large established codebase like Bisq, a contributor’s goal should be to produce stylistically perfect patches, where perfect is defined by not causing reviewers to digress into style discussions or needing to clean up after you. It takes some time to catch up with the nuances of style conventions in any large codebase and Bisq is far from perfectly consistent with its own conventions. As such, it’s expected that newcomers will make mistakes here and there; that’s not a problem. The main thing is to strive for patches that require as little overhead as possible for reviewers and maintainers, and style quibbles are overhead.

…

> On Feb 26, 2020, at 8:10 PM, dmos62 ***@***.***> wrote: @dmos62 commented on this pull request. In core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java: > + var mayInterruptWhileRunning = true; + peerVersionMessageFuture.cancel(mayInterruptWhileRunning); @sqrrm the reason why I use these is to not have "anonymous" values in calls. Example: buildHouse(false) v. buildHouse(withRoof). — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

cbeams · 2020-02-26T19:53:16Z

It looks like my previous message was sent too hastily. I sent it from my phone, where the last notifications I saw on this review thread were about style quibbles (explanatory variable declarations in particular). On returning to my laptop, I see that @dmos62 has followed up with commits that largely resolve the stylistic deviations.

I am wondering, however, why LocalBitcoinNodeTests was deleted with nothing seeming to replace it. Forgive me if that's been discussed somewhere already and I missed it.

cbeams · 2020-02-26T20:50:01Z

I've just pushed a style polishing commit to my own fork at cbeams@b93ca2b. @dmos62, please consider pushing it to this PR branch.

I am also working on an additional commit that makes several more substantive changes. I'll wrap it up and push it it to my branch in my (European) AM tomorrow (Thursday). Please let me know if you have any significant local changes that haven't been pushed, so I'm aware whether or not conflicts are likely. Thanks.

dmos62 · 2020-02-26T21:37:24Z

@cbeams I don't have changes in the works. The test suite was removed because I don't have ideas about how to replace it. Before the detection mechanism was just checking if a socket can be opened; now a whole Bitcoin protocol handshake has to be performed.

I don't mean to be a nuisance with the stylistic discussions. I guess I didn't consider the additional effort the differences created. I'll try and emulate the rest of the codebase.

- Rename #willUse => #shouldBeUsed - Rename #willIgnore => #shouldBeIgnored - Reduce visibility of methods where appropriate - Edit Javadoc typos and use @link syntax where appropriate

This was originally added with the intention that the local Bitcoin node port could be customized, but in fact it never could be, because Guice configuration always hard-wired the value to the default port for the CurrentBaseNetwork's Parameters (eg. 8333 for BTC_MAINNET). This change removes the constant, removes any Guice wiring and injection and localizes the hard-coded assignment to the LocalBitcoinNode constructor to simplify and make things explicit. If it is desired to allow users to specify a custom port for their local Bitcoin node, a proper option shoud be added to Config. In the meantime, users may work around this by using `--btcNodes=localhost:4242` where 4242 is the custom port. Note however, that the pruning and bloom filter checks will not occur in this case as the provided node address will not being treated as a LocalBitcoinNode.

cbeams · 2020-02-27T09:26:11Z

@dmos62, I've just submitted dmos62#1 with my changes, including the first polishing commit I mentioned above. Please merge and push that and the changes will show up here in this PR.

Note that I have NOT actually tested the functionality of this larger PR on my local machine, so I'm deferring to @ripcurlx's outstanding review comment regarding his current inability to get the behavior to work as expected.

ripcurlx · 2020-02-27T09:26:29Z

Just tested the current state again on Regtest.

Background information for my local Bitcoin Core is shown again when configured properly

Warning is now shown when peerbloomfilter is not enabled

When trying to set e.g. prune=550 somehow during debugging the app got stuck, but running it without breakpoints did work as expected.

Following exceptions are logged in the command line by bitcoinJ which is expected I guess.

Feb.-27 10:21:12.035 [JavaFX Application Thread] INFO  b.c.btc.nodes.LocalBitcoinNode: Local Bitcoin node detected on port 18444 
Feb.-27 10:21:12.035 [JavaFX Application Thread] INFO  b.c.btc.nodes.LocalBitcoinNode: Local Bitcoin node badly configured (it is pruning and/or bloom filters are disabled) 
Feb.-27 10:21:12.037 [NioClientManager] WARN  o.b.core.PeerSocketHandler: [127.0.0.1]:18444 -  org.bitcoinj.core.ProtocolException: Received AlertMessage before version handshake is complete.
	at org.bitcoinj.core.Peer.processMessage(Peer.java:478)
	at org.bitcoinj.core.PeerSocketHandler.receiveBytes(PeerSocketHandler.java:184)
	at org.bitcoinj.net.NioClient$Handler.receiveBytes(NioClient.java:71)
	at org.bitcoinj.net.ConnectionHandler.handleKey(ConnectionHandler.java:223)
	at org.bitcoinj.net.NioClientManager.handleKey(NioClientManager.java:86)
	at org.bitcoinj.net.NioClientManager.run(NioClientManager.java:122)
	at com.google.common.util.concurrent.AbstractExecutionThreadService$1$2.run(AbstractExecutionThreadService.java:66)
	at com.google.common.util.concurrent.Callables$4.run(Callables.java:122)
	at org.bitcoinj.utils.ContextPropagatingThreadFactory$1.run(ContextPropagatingThreadFactory.java:49)
	at java.base/java.lang.Thread.run(Thread.java:844)

org.bitcoinj.core.ProtocolException: Received AlertMessage before version handshake is complete.
	at org.bitcoinj.core.Peer.processMessage(Peer.java:478)
	at org.bitcoinj.core.PeerSocketHandler.receiveBytes(PeerSocketHandler.java:184)
	at org.bitcoinj.net.NioClient$Handler.receiveBytes(NioClient.java:71)
	at org.bitcoinj.net.ConnectionHandler.handleKey(ConnectionHandler.java:223)
	at org.bitcoinj.net.NioClientManager.handleKey(NioClientManager.java:86)
	at org.bitcoinj.net.NioClientManager.run(NioClientManager.java:122)
	at com.google.common.util.concurrent.AbstractExecutionThreadService$1$2.run(AbstractExecutionThreadService.java:66)
	at com.google.common.util.concurrent.Callables$4.run(Callables.java:122)
	at org.bitcoinj.utils.ContextPropagatingThreadFactory$1.run(ContextPropagatingThreadFactory.java:49)
	at java.base/java.lang.Thread.run(Thread.java:844)
Feb.-27 10:21:12.039 [NioClientManager] WARN  o.b.net.ConnectionHandler: Error handling SelectionKey: java.lang.IllegalStateException  java.lang.IllegalStateException: null
	at com.google.common.base.Preconditions.checkState(Preconditions.java:429)
	at org.bitcoinj.net.ConnectionHandler.handleKey(ConnectionHandler.java:224)
	at org.bitcoinj.net.NioClientManager.handleKey(NioClientManager.java:86)
	at org.bitcoinj.net.NioClientManager.run(NioClientManager.java:122)
	at com.google.common.util.concurrent.AbstractExecutionThreadService$1$2.run(AbstractExecutionThreadService.java:66)
	at com.google.common.util.concurrent.Callables$4.run(Callables.java:122)
	at org.bitcoinj.utils.ContextPropagatingThreadFactory$1.run(ContextPropagatingThreadFactory.java:49)
	at java.base/java.lang.Thread.run(Thread.java:844)

java.lang.IllegalStateException: null
	at com.google.common.base.Preconditions.checkState(Preconditions.java:429)
	at org.bitcoinj.net.ConnectionHandler.handleKey(ConnectionHandler.java:224)
	at org.bitcoinj.net.NioClientManager.handleKey(NioClientManager.java:86)
	at org.bitcoinj.net.NioClientManager.run(NioClientManager.java:122)
	at com.google.common.util.concurrent.AbstractExecutionThreadService$1$2.run(AbstractExecutionThreadService.java:66)
	at com.google.common.util.concurrent.Callables$4.run(Callables.java:122)
	at org.bitcoinj.utils.ContextPropagatingThreadFactory$1.run(ContextPropagatingThreadFactory.java:49)
	at java.base/java.lang.Thread.run(Thread.java:844)

So ACK from my side now from a functionality point of view. Please apply the polishing commit by @cbeams and I'm happy to merge this PR. Thanks for your contribution @dmos62 !

cbeams · 2020-02-27T09:35:11Z

Did we consider at any point in this process doing these same pruning and bloom filter checks not only against a local Bitcoin node, but also against (a) Bisq's provided federation of nodes and (b) any custom nodes provided by the user? In the case of (a), it's probably a waste to do these checks, since our own federation nodes should always be properly configured, but case (b) is more important, especially for users who are pointing their local Bisq node at their own remote Bitcoin full node. See the last paragraph of the commit comment at dmos62@57b7041 for more on this.

In any case, I'm not suggesting that doing these checks against the federation or user-specified custom nodes should be done as part of this PR. I just want to see if it's been thought about at all.

cbeams

NACK per #3982 (comment)

ripcurlx · 2020-02-27T09:42:11Z

In any case, I'm not suggesting that doing these checks against the federation or user-specified custom nodes should be done as part of this PR. I just want to see if it's been thought about at all.

Atm we temp deactivated the option to connect to the public network because of the lack of a node capabilities check. I think after this PR is merged this would be the next step if we are able to apply this checks also to our public network and user-specified custom node option.

cbeams · 2020-02-27T09:52:32Z

if we are able to apply this checks also to our public network

Did you mean "to our federation" here? I don't see a lot of value in re-enabling the public network option at this point. So long as Bisq relies on SPV, users are at risk on the public network because of chain surveillance. I think keeping it just to our own federation or a user's custom node(s) is sufficient. Doing checks against (at least) user-provided custom nodes would make sense to me for the same reasons doing it against their local node makes sense.

cbeams · 2020-02-27T11:16:54Z

core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java

@@ -50,7 +50,6 @@
 import java.io.File;

 import static bisq.common.config.Config.*;
-import static bisq.core.btc.nodes.LocalBitcoinNode.LOCAL_BITCOIN_NODE_PORT;


Good catch, thanks @dmos62.

ripcurlx · 2020-02-27T11:18:46Z

Did you mean "to our federation" here? I don't see a lot of value in re-enabling the public network option at this point. So long as Bisq relies on SPV, users are at risk on the public network because of chain surveillance. I think keeping it just to our own federation or a user's custom node(s) is sufficient. Doing checks against (at least) user-provided custom nodes would make sense to me for the same reasons doing it against their local node makes sense.

I was talking about "public" public network. But yes, we can keep it deactivated for security reasons if on-one wants to explicitly use this feature because of specific reasons.

Kixunil · 2020-02-29T11:54:03Z

Wow, @dmos62 that wording is really clear! More projects should be like this!

dmos62 commented Feb 17, 2020

View reviewed changes

ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Feb 18, 2020

Downgrade Optional usage to Java 10

18478d9

I was erroniously targeting Java 11, when Travis requires Java 10. `Optional.isEmpty` shows up only in Java 11.

ripcurlx linked an issue Feb 18, 2020 that may be closed by this pull request

Bisq doesn't connect to default Bitcoin Core version v0.19.0.1 #3935

Closed

ripcurlx linked an issue Feb 18, 2020 that may be closed by this pull request

Show warning if user has a pruned Bitcoin node locally running #3273

Closed

Minor changes to satisfy Codacy or clarify why it fails

daa1b0b

dmos62 requested review from ripcurlx and sqrrm February 18, 2020 11:20

dmos62 commented Feb 18, 2020

View reviewed changes

ripcurlx added this to the v1.2.8 milestone Feb 18, 2020

sqrrm reviewed Feb 18, 2020

View reviewed changes

core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java Outdated Show resolved Hide resolved

Improve marking that method is empty

e6dea3d

Co-Authored-By: sqrrm <sqrrm@users.noreply.github.com>

sqrrm requested changes Feb 18, 2020

View reviewed changes

cbeams suggested changes Feb 19, 2020

View reviewed changes

dmos62 added 2 commits February 21, 2020 23:52

Fix unchecked usage of LocalBitcoinNode.isUsable()

65177fc

Silence NioClient and NioClientManager loggers

08cd31b

dmos62 requested a review from sqrrm February 21, 2020 23:28

Formating changes

7848836

dmos62 requested a review from cbeams February 21, 2020 23:33

sqrrm previously approved these changes Feb 22, 2020

View reviewed changes

sqrrm reviewed Feb 22, 2020

View reviewed changes

Polish formatting

b93ca2b

cbeams added 2 commits February 27, 2020 09:35

Polish LocalBitcoinNode method names and visibility

c1a99cc

- Rename #willUse => #shouldBeUsed - Rename #willIgnore => #shouldBeIgnored - Reduce visibility of methods where appropriate - Edit Javadoc typos and use @link syntax where appropriate

cbeams mentioned this pull request Feb 27, 2020

Polish LocalBitcoinNode changes dmos62/bisq#1

Merged

cbeams suggested changes Feb 27, 2020

View reviewed changes

cbeams mentioned this pull request Feb 27, 2020

Temporarily deactivate public network option #3938

Merged

Remove reference to removed constant

85e4515

cbeams reviewed Feb 27, 2020

View reviewed changes

cbeams approved these changes Feb 27, 2020

View reviewed changes

ripcurlx approved these changes Feb 27, 2020

View reviewed changes

sqrrm merged commit 1d1cba1 into bisq-network:master Feb 27, 2020

dmos62 mentioned this pull request Feb 27, 2020

[Withdrawn] For Cycle 11 bisq-network/compensation#502

Closed

dmos62 deleted the local-btc-node-configuration-check branch February 27, 2020 16:18

sqrrm mentioned this pull request Mar 11, 2020

For Cycle 11 bisq-network/compensation#510

Closed

ripcurlx mentioned this pull request Mar 16, 2020

For Cycle 11 bisq-network/compensation#499

Closed

mattmill30 mentioned this pull request Jun 17, 2020

Bisq does not retain bitcoin addresses & transactions following wallet recovery (OP: 'erases all state data when launched alongside Bitcoin core') #4318

Closed

dmos62 mentioned this pull request Jun 17, 2020

For Cycle 14 bisq-network/compensation#588

Closed

mrosseel mentioned this pull request Aug 20, 2020

For Cycle 16 bisq-network/compensation#638

Closed

sqrrm mentioned this pull request Jun 14, 2023

For Cycle 48 bisq-network/compensation#1311

Closed

		new Context(networkParameters);

		var ourVersionMessage = new VersionMessage(networkParameters, 0);

		var ourVersionMessage = new VersionMessage(networkParameters, 0);

		var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port);

		var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port);

		AbstractBlockChain blockchain = null;


		private ListenableFuture<VersionMessage> getVersionMessage(Peer peer) {
		SettableFuture<VersionMessage> peerVersionMessageFuture = SettableFuture.create();

		var whenChecksNotFinished = false;

		var throwable = new Throwable("LocalBitcoinNode was queried before it was ready");

Add local Bitcoin node configuration detection #3982

Add local Bitcoin node configuration detection #3982

Conversation

dmos62 commented Feb 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 Feb 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sqrrm commented Feb 18, 2020

dmos62 commented Feb 18, 2020

dmos62 commented Feb 18, 2020 • edited Loading

dmos62 left a comment • edited Loading

Choose a reason for hiding this comment

sqrrm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbeams left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sqrrm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 commented Feb 22, 2020

sqrrm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmos62 commented Feb 23, 2020

cbeams commented Feb 26, 2020 via email

cbeams commented Feb 26, 2020

cbeams commented Feb 26, 2020 • edited Loading

dmos62 commented Feb 26, 2020

cbeams commented Feb 27, 2020

ripcurlx commented Feb 27, 2020

cbeams commented Feb 27, 2020

cbeams left a comment

Choose a reason for hiding this comment

ripcurlx commented Feb 27, 2020

cbeams commented Feb 27, 2020

Choose a reason for hiding this comment

ripcurlx commented Feb 27, 2020

Kixunil commented Feb 29, 2020

dmos62 Feb 26, 2020 •

edited

Loading

dmos62 commented Feb 18, 2020 •

edited

Loading

dmos62 left a comment •

edited

Loading

cbeams commented Feb 26, 2020 •

edited

Loading