From f895da416fbe4c75acdaebe9f9f22e534edf09d1 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Thu, 13 Feb 2020 21:14:41 +0200 Subject: [PATCH 01/20] Add local Bitcoin node configuration detection 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. --- .../main/java/bisq/core/app/BisqSetup.java | 31 +- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 297 ++++++++++++++++-- .../bisq/core/btc/setup/WalletConfig.java | 7 +- .../bisq/core/btc/setup/WalletsSetup.java | 2 +- .../main/java/bisq/core/user/Preferences.java | 17 +- .../resources/i18n/displayStrings.properties | 8 +- .../java/bisq/desktop/main/MainViewModel.java | 18 +- .../settings/network/NetworkSettingsView.java | 6 +- 8 files changed, 345 insertions(+), 41 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index b904609a038..89aaf39d532 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -193,7 +193,7 @@ default void onRequestWalletPassword() { @Setter @Nullable - private Consumer displayTacHandler; + private Consumer displayTacHandler, displayLocalNodeMisconfigurationHandler; @Setter @Nullable private Consumer cryptoSetupFailedHandler, chainFileLockedExceptionHandler, @@ -348,7 +348,7 @@ public void start() { } private void step2() { - detectLocalBitcoinNode(this::step3); + maybeCheckLocalBitcoinNode(this::step3); } private void step3() { @@ -482,14 +482,31 @@ private void maybeShowTac() { } } - private void detectLocalBitcoinNode(Runnable nextStep) { + private void maybeCheckLocalBitcoinNode(Runnable nextStep) { BaseCurrencyNetwork baseCurrencyNetwork = config.baseCurrencyNetwork; - if (config.ignoreLocalBtcNode || baseCurrencyNetwork.isDaoRegTest() || baseCurrencyNetwork.isDaoTestNet()) { + + var shouldIgnoreLocalNode = + config.ignoreLocalBtcNode + || baseCurrencyNetwork.isDaoRegTest() + || baseCurrencyNetwork.isDaoTestNet(); + if (shouldIgnoreLocalNode) { 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(); + if (detectedButMisconfigured) { + displayLocalNodeMisconfigurationHandler.accept(nextStep); + return; + } + + nextStep.run(); } private void readMapsFromResources(Runnable nextStep) { @@ -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()) { + if (!preferences.getUseTorForBitcoinJ()) { initWallet(); } @@ -862,7 +879,7 @@ private void maybeShowSecurityRecommendation() { } private void maybeShowLocalhostRunningInfo() { - maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.isDetected()); + maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.isUsable().get()); } private void maybeShowAccountSigningStateInfo() { diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index 7de869c8f8b..dfbd829546b 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -1,19 +1,43 @@ package bisq.core.btc.nodes; +import org.bitcoinj.core.AbstractBlockChain; +import org.bitcoinj.core.Context; +import org.bitcoinj.core.Peer; +import org.bitcoinj.core.PeerAddress; +import org.bitcoinj.core.VersionMessage; +import org.bitcoinj.core.listeners.PeerDisconnectedEventListener; +import org.bitcoinj.net.NioClient; +import org.bitcoinj.params.MainNetParams; + import javax.inject.Inject; import javax.inject.Named; import javax.inject.Singleton; +import com.google.common.util.concurrent.FutureCallback; +import com.google.common.util.concurrent.Futures; +import com.google.common.util.concurrent.ListenableFuture; +import com.google.common.util.concurrent.SettableFuture; + +import java.net.InetAddress; import java.net.InetSocketAddress; -import java.net.Socket; +import java.net.UnknownHostException; import java.io.IOException; +import java.util.Optional; +import java.util.concurrent.CancellationException; +import java.util.concurrent.ExecutionException; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * Detects whether a Bitcoin node is running on localhost. + * Detects whether a Bitcoin node is running on localhost and whether it is well configured + * (meaning it's not pruning and has bloom filters enabled). The class is split into + * methods that perform relevant checks and cache the result (methods that start with "check"), + * and methods that query that cache (methods that start with "is"). The querying methods return + * an Optional, whose emptiness shows if the relevant check is pending, and whose + * contents show the result of the check. * @see bisq.common.config.Config#ignoreLocalBtcNode */ @Singleton @@ -25,7 +49,8 @@ public class LocalBitcoinNode { private static final int CONNECTION_TIMEOUT = 5000; private final int port; - private boolean detected = false; + private Optional detected = Optional.empty(); + private Optional wellConfigured = Optional.empty(); @Inject public LocalBitcoinNode(@Named(LOCAL_BITCOIN_NODE_PORT) int port) { @@ -33,31 +58,263 @@ public LocalBitcoinNode(@Named(LOCAL_BITCOIN_NODE_PORT) int port) { } /** - * Detect whether a Bitcoin node is running on localhost by attempting to connect - * to the node's port and run the given callback regardless of whether the connection - * was successful. If the connection is successful, subsequent calls to - * {@link #isDetected()} will return {@code true}. - * @param callback code to run after detecting whether the node is running + * Creates an NioClient that is expected to only be used to coerce a VersionMessage out of a + * local Bitcoin node and be closed right after. + */ + + private static NioClient createClient( + Peer peer, int port, int connectionTimeout + ) throws IOException { + InetSocketAddress serverAddress = + new InetSocketAddress(InetAddress.getLocalHost(), port); + + // 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); + + return client; + } + + /** + * Creates a Peer that is expected to only be used to coerce a VersionMessage out of a + * local Bitcoin node and be closed right after. */ - 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(); + + var context = new Context(networkParameters); + + var ourVersionMessage = new VersionMessage(networkParameters, 0); + + var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port); + + AbstractBlockChain blockchain = null; + + var peer = new Peer(networkParameters, ourVersionMessage, localPeerAddress, blockchain); + return peer; + } + + 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 + */ + + 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; + var whenBloomFiltersWereIntroduced = BLOOM_FILTERS_BIP37_PROTOCOL_VERSION; + + int BLOOM_FILTERS_BIP111_PROTOCOL_VERSION = 70011; + var whenBloomFiltersWereDisabledByDefault = BLOOM_FILTERS_BIP111_PROTOCOL_VERSION; + + int clientVersion = versionMessage.clientVersion; + long localServices = versionMessage.localServices; + + 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(); + } + + private void handleHandshakeAttempt(Optional optionalVersionMessage) { + if (optionalVersionMessage.isEmpty()) { + detected = Optional.of(false); + wellConfigured = Optional.of(false); + log.info("No local Bitcoin node detected on port {}," + + " or the connection was prematurely closed" + + " (before a version messages could be coerced)", + port); + } else { + detected = Optional.of(true); log.info("Local Bitcoin node detected on port {}", port); - detected = true; + + var versionMessage = optionalVersionMessage.get(); + var configurationCheckResult = checkWellConfigured(versionMessage); + + if (configurationCheckResult) { + wellConfigured = Optional.of(true); + log.info("Local Bitcoin node found to be well configured" + + " (not pruning and allows bloom filters)"); + } else { + wellConfigured = Optional.of(false); + log.info("Local Bitcoin node badly configured" + + " (it is pruning and/or bloom filters are disabled)"); + } + } + } + + /** Performs a blocking Bitcoin protocol handshake, which includes exchanging version messages and acks. + * Its purpose is to check if a local Bitcoin node is running, and, if it is, check its advertised + * configuration. The returned Optional is empty, if a local peer wasn't found, or if handshake failed + * for some reason. This method could be noticably simplified, by turning connection failure callback + * into a future and using a first-future-to-complete type of construct, but I couldn't find a + * ready-made implementation. + */ + + private Optional attemptHandshakeForVersionMessage() { + Peer peer; + try { + peer = createLocalPeer(port); + } catch (UnknownHostException ex) { + log.error("Local bitcoin node handshake attempt unexpectedly threw: {}", ex); + return Optional.empty(); + } + + NioClient client; + try { + log.info("Initiating attempt to connect to and handshake with a local " + + "Bitcoin node (which may or may not be running) on port {}.", + port); + client = createClient(peer, port, CONNECTION_TIMEOUT); } catch (IOException ex) { - log.info("No local Bitcoin node detected on port {}.", port); + log.error("Local bitcoin node handshake attempt unexpectedly threw: {}", ex); + return Optional.empty(); + } + + ListenableFuture peerVersionMessageFuture = getVersionMessage(peer); + + Optional optionalPeerVersionMessage; + + // block for VersionMessage or cancellation (in case of connection failure) + try { + var peerVersionMessage = peerVersionMessageFuture.get(); + optionalPeerVersionMessage = Optional.of(peerVersionMessage); + } catch (ExecutionException | InterruptedException | CancellationException ex) { + optionalPeerVersionMessage = Optional.empty(); } - callback.run(); + + peer.close(); + client.closeConnection(); + + return optionalPeerVersionMessage; + } + + private ListenableFuture getVersionMessage(Peer peer) { + SettableFuture peerVersionMessageFuture = SettableFuture.create(); + + var versionHandshakeDone = peer.getVersionHandshakeFuture(); + FutureCallback fetchPeerVersionMessage = new FutureCallback() { + public void onSuccess(Peer peer) { + peerVersionMessageFuture.set(peer.getPeerVersionMessage()); + } + public void onFailure(Throwable thr) { + } + }; + Futures.addCallback( + versionHandshakeDone, + fetchPeerVersionMessage + ); + + PeerDisconnectedEventListener cancelIfConnectionFails = + new PeerDisconnectedEventListener() { + public void onPeerDisconnected(Peer peer, int peerCount) { + var peerVersionMessageAlreadyReceived = + peerVersionMessageFuture.isDone(); + if (peerVersionMessageAlreadyReceived) { + // This method is called whether or not the handshake was successful. + // In case it was successful, we don't want to do anything here. + return; + } + // In some cases Peer will self-disconnect after receiving + // node's VersionMessage, but before completing the handshake. + // In such a case, we want to retrieve the VersionMessage. + var peerVersionMessage = peer.getPeerVersionMessage(); + if (peerVersionMessage != null) { + log.info("Handshake attempt was interrupted;" + + " however, the local node's version message was coerced."); + peerVersionMessageFuture.set(peerVersionMessage); + } else { + log.info("Handshake attempt did not result in a version message exchange."); + var mayInterruptWhileRunning = true; + peerVersionMessageFuture.cancel(mayInterruptWhileRunning); + } + } + }; + + // Cancel peerVersionMessageFuture if connection failed + peer.addDisconnectedEventListener(cancelIfConnectionFails); + + return peerVersionMessageFuture; } /** - * Returns whether or not a Bitcoin node was running on localhost at the time - * {@link #detectAndRun(Runnable)} was called. No further monitoring is performed, so - * if the node goes up or down in the meantime, this method will continue to return - * its original value. See {@code MainViewModel#setupBtcNumPeersWatcher} to understand - * how disconnection and reconnection of the local Bitcoin node is actually handled. + * Returns an optional that, in case it is not empty, shows whether or not the + * local node was fit for usage at the time the checks were performed called, + * meaning it's been detected and its configuration satisfied our checks; or, in + * the case that it is empty, it signifies that the checks have not yet completed. */ - public boolean isDetected() { + + public Optional isUsable() { + // If a node is found to be well configured, it implies that it was also detected, + // so this is query is enough to show if the relevant checks were performed and if + // their results are positive. + return isWellConfigured(); + } + + /** + * Returns an Optional that, when not empty, tells you whether the local node + * was detected, but misconfigured. + */ + + public Optional isDetectedButMisconfigured() { + return isDetected().flatMap(goodDetect -> + isWellConfigured().map(goodConfig -> + goodDetect && !goodConfig + )); + } + + /** + * Returns an optional, which is empty in case detection has not yet completed, or + * which contains a Boolean, in case detection has been performed, which signifies + * whether or not a Bitcoin node was running on localhost at the time the checks were + * performed. No further monitoring is performed, so if the node goes up or down in the + * meantime, this method will continue to return its original value. See + * {@code MainViewModel#setupBtcNumPeersWatcher} to understand how disconnection and + * reconnection of the local Bitcoin node is actually handled. + */ + public Optional isDetected() { return detected; } + + /** + * Returns an optional whose emptiness signifies whether or not configuration checks + * have been performed, and its Boolean contents whether the local node's configuration + * satisfied our checks at the time they were performed. We check if the local node is + * not pruning and has bloom filters enabled. + */ + + public Optional isWellConfigured() { + return wellConfigured; + } } diff --git a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java index 237d8709b97..c6cae1c4ced 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java @@ -239,11 +239,12 @@ private PeerGroup createPeerGroup() { peerGroup.setConnectTimeoutMillis(TOR_VERSION_EXCHANGE_TIMEOUT); } - // For dao testnet (server side regtest) we prevent to connect to a localhost node to avoid confusion - // if local btc node is not synced with our dao testnet master node. + // For dao testnet (server side regtest) we disable the use of local bitcoin node to + // avoid confusion if local btc node is not synced with our dao testnet master node. + // It is also disabled if the local node was not found or was found to be misconfigured. if (Config.baseCurrencyNetwork().isDaoRegTest() || Config.baseCurrencyNetwork().isDaoTestNet() || - !localBitcoinNode.isDetected()) + !localBitcoinNode.isUsable().get()) peerGroup.setUseLocalhostPeerWhenPossible(false); return peerGroup; diff --git a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java index 8c420366c2a..4dd9d908631 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java @@ -278,7 +278,7 @@ protected void onSetupCompleted() { return; } } - } else if (localBitcoinNode.isDetected()) { + } else if (localBitcoinNode.isUsable().get()) { walletConfig.setMinBroadcastConnections(1); walletConfig.setPeerNodesForLocalHost(); } else { diff --git a/core/src/main/java/bisq/core/user/Preferences.java b/core/src/main/java/bisq/core/user/Preferences.java index 08d9e0edbaa..db1a2629c18 100644 --- a/core/src/main/java/bisq/core/user/Preferences.java +++ b/core/src/main/java/bisq/core/user/Preferences.java @@ -736,11 +736,24 @@ public boolean showAgain(String key) { } public boolean getUseTorForBitcoinJ() { - // We override the useTorForBitcoinJ and set it to false if we detected a localhost node or if we are not on mainnet, + // We override the useTorForBitcoinJ and set it to false if we found a usable localhost node or if we are not on mainnet, // unless the useTorForBtc parameter is explicitly provided. // On testnet there are very few Bitcoin tor nodes and we don't provide tor nodes. + + // TODO bug. Non-critical, apparently. + // Sometimes this method, which queries LocalBitcoinNode for whether or not there's a + // usable local Bitcoin node, is called before LocalBitcoinNode has performed its + // checks. This was noticed when LocalBitcoinNode was refactored to return + // Optional istead of boolean, an empty Optional signifying that the relevant + // check has not yet been performed. + // + // 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); + if ((!Config.baseCurrencyNetwork().isMainnet() - || localBitcoinNode.isDetected()) + || usableLocalNodePresent) && !config.useTorForBtcOptionSetExplicitly) return false; else diff --git a/core/src/main/resources/i18n/displayStrings.properties b/core/src/main/resources/i18n/displayStrings.properties index c46a9942081..b033080bf99 100644 --- a/core/src/main/resources/i18n/displayStrings.properties +++ b/core/src/main/resources/i18n/displayStrings.properties @@ -2659,9 +2659,13 @@ popup.privateNotification.headline=Important private notification! popup.securityRecommendation.headline=Important security recommendation popup.securityRecommendation.msg=We would like to remind you to consider using password protection for your wallet if you have not already enabled that.\n\nIt is also highly recommended to write down the wallet seed words. Those seed words are like a master password for recovering your Bitcoin wallet.\nAt the \"Wallet Seed\" section you find more information.\n\nAdditionally you should backup the complete application data folder at the \"Backup\" section. +popup.warning.localNodeMisconfigured.explanation=Bisq detected a locally running Bitcoin Core node (at localhost); however, its configuration is incompatible with Bisq.\n\n\ +For Bisq to use a local Bitcoin node, the node has to have pruning disabled and bloom filters enabled. Starting with Bitcoin Core v0.19 you have to manually enable bloom filters by setting peerbloomfilters=1 in your bitcoin.conf configuration file. +popup.warning.localNodeMisconfigured.continueWithoutLocalNode=Continue without using local node + popup.bitcoinLocalhostNode.msg=Bisq detected a locally running Bitcoin Core node (at localhost).\n\ - Please make sure that this node is fully synced before you start Bisq and that it is not running in pruned mode. -popup.bitcoinLocalhostNode.additionalRequirements=\n\nStarting with Bitcoin Core v0.19 you have to manually enable bloom filters by setting peerbloomfilters=1 in your bitcoin.conf configuration file. + Please make sure that this node is fully synced before you start Bisq. +popup.bitcoinLocalhostNode.additionalRequirements=\n\nThe node was found to be well configured. For reference, the requirements are for the node to have pruning disabled and bloom filters enabled. popup.shutDownInProgress.headline=Shut down in progress popup.shutDownInProgress.msg=Shutting down application can take a few seconds.\nPlease don't interrupt this process. diff --git a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java index 25586aa284e..3e4bdcd731d 100644 --- a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java +++ b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java @@ -305,6 +305,16 @@ private void setupHandlers() { else torNetworkSettingsWindow.hide(); }); + bisqSetup.setDisplayLocalNodeMisconfigurationHandler( + (Runnable continueWithoutLocalNode) -> + new Popup() + .hideCloseButton() + .warning(Res.get("popup.warning.localNodeMisconfigured.explanation")) + .useShutDownButton() + .secondaryActionButtonText(Res.get("popup.warning.localNodeMisconfigured.continueWithoutLocalNode")) + .onSecondaryAction(continueWithoutLocalNode) + .show() + ); bisqSetup.setSpvFileCorruptedHandler(msg -> new Popup().warning(msg) .actionButtonText(Res.get("settings.net.reSyncSPVChainButton")) .onAction(() -> GUIUtil.reSyncSPVChain(preferences)) @@ -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())); + if (localBitcoinNode.isUsable().get()) + getWalletServiceErrorMsg().set( + Res.get("mainView.networkWarning.localhostBitcoinLost", Res.getBaseCurrencyName().toLowerCase())); else - getWalletServiceErrorMsg().set(Res.get("mainView.networkWarning.allConnectionsLost", Res.getBaseCurrencyName().toLowerCase())); + getWalletServiceErrorMsg().set( + Res.get("mainView.networkWarning.allConnectionsLost", Res.getBaseCurrencyName().toLowerCase())); } else { getWalletServiceErrorMsg().set(null); } diff --git a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java index 5b18283d9e7..9004ebc75a5 100644 --- a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java +++ b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java @@ -165,7 +165,7 @@ public void initialize() { bitcoinPeerSubVersionColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.subVersionColumn"))); bitcoinPeerHeightColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.heightColumn"))); localhostBtcNodeInfoLabel.setText(Res.get("settings.net.localhostBtcNodeInfo")); - if (!localBitcoinNode.isDetected()) { + if (!localBitcoinNode.isUsable().get()) { localhostBtcNodeInfoLabel.setVisible(false); } useProvidedNodesRadio.setText(Res.get("settings.net.useProvidedNodesRadio")); @@ -380,7 +380,7 @@ private void showShutDownPopup() { } private void onBitcoinPeersToggleSelected(boolean calledFromUser) { - boolean bitcoinLocalhostNodeRunning = localBitcoinNode.isDetected(); + boolean bitcoinLocalhostNodeRunning = localBitcoinNode.isUsable().get(); useTorForBtcJCheckBox.setDisable(bitcoinLocalhostNodeRunning); bitcoinNodesLabel.setDisable(bitcoinLocalhostNodeRunning); btcNodesLabel.setDisable(bitcoinLocalhostNodeRunning); @@ -454,7 +454,7 @@ private void onBitcoinPeersToggleSelected(boolean calledFromUser) { private void applyPreventPublicBtcNetwork() { final boolean preventPublicBtcNetwork = isPreventPublicBtcNetwork(); - usePublicNodesRadio.setDisable(localBitcoinNode.isDetected() || preventPublicBtcNetwork); + usePublicNodesRadio.setDisable(localBitcoinNode.isUsable().get() || preventPublicBtcNetwork); if (preventPublicBtcNetwork && selectedBitcoinNodesOption == BtcNodes.BitcoinNodesOption.PUBLIC) { selectedBitcoinNodesOption = BtcNodes.BitcoinNodesOption.PROVIDED; preferences.setBitcoinNodesOptionOrdinal(selectedBitcoinNodesOption.ordinal()); From 18478d9b0dba78e5cb662bb10f72debd3973226d Mon Sep 17 00:00:00 2001 From: dmos62 <2715476+dmos62@users.noreply.github.com> Date: Tue, 18 Feb 2020 12:21:17 +0200 Subject: [PATCH 02/20] Downgrade Optional usage to Java 10 I was erroniously targeting Java 11, when Travis requires Java 10. `Optional.isEmpty` shows up only in Java 11. --- core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index dfbd829546b..5dd7966ab37 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -148,7 +148,7 @@ public boolean checkUsable() { } private void handleHandshakeAttempt(Optional optionalVersionMessage) { - if (optionalVersionMessage.isEmpty()) { + if (!optionalVersionMessage.isPresent()) { detected = Optional.of(false); wellConfigured = Optional.of(false); log.info("No local Bitcoin node detected on port {}," From 74c946a28b9237c8bf9d2ea96a699723ba19a2c9 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Tue, 18 Feb 2020 12:31:15 +0200 Subject: [PATCH 03/20] Remove defunct test suite 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. --- .../core/btc/nodes/LocalBitcoinNodeTests.java | 45 ------------------- 1 file changed, 45 deletions(-) delete mode 100644 core/src/test/java/bisq/core/btc/nodes/LocalBitcoinNodeTests.java diff --git a/core/src/test/java/bisq/core/btc/nodes/LocalBitcoinNodeTests.java b/core/src/test/java/bisq/core/btc/nodes/LocalBitcoinNodeTests.java deleted file mode 100644 index 9233263fd75..00000000000 --- a/core/src/test/java/bisq/core/btc/nodes/LocalBitcoinNodeTests.java +++ /dev/null @@ -1,45 +0,0 @@ -package bisq.core.btc.nodes; - -import java.net.ServerSocket; - -import java.io.IOException; - -import java.util.concurrent.atomic.AtomicBoolean; - -import org.junit.Before; -import org.junit.Test; - -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - -public class LocalBitcoinNodeTests { - - private AtomicBoolean called = new AtomicBoolean(false); - private Runnable callback = () -> called.set(true); - private ServerSocket socket; - private LocalBitcoinNode localBitcoinNode; - - @Before - public void setUp() throws IOException { - // Bind to and listen on an available port - socket = new ServerSocket(0); - localBitcoinNode = new LocalBitcoinNode(socket.getLocalPort()); - } - - @Test - public void whenLocalBitcoinNodeIsDetected_thenCallbackGetsRun_andIsDetectedReturnsTrue() { - // Continue listening on the port, indicating the local Bitcoin node is running - localBitcoinNode.detectAndRun(callback); - assertTrue(called.get()); - assertTrue(localBitcoinNode.isDetected()); - } - - @Test - public void whenLocalBitcoinNodeIsNotDetected_thenCallbackGetsRun_andIsDetectedReturnsFalse() throws IOException { - // Stop listening on the port, indicating the local Bitcoin node is not running - socket.close(); - localBitcoinNode.detectAndRun(callback); - assertTrue(called.get()); - assertFalse(localBitcoinNode.isDetected()); - } -} From daa1b0b20b02c13daf58036a41486087b8328e07 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Tue, 18 Feb 2020 13:02:39 +0200 Subject: [PATCH 04/20] Minor changes to satisfy Codacy or clarify why it fails --- core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index 5dd7966ab37..343d121808c 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -87,7 +87,9 @@ private static Peer createLocalPeer( // i.e. is it fine to just always use MainNetParams? var networkParameters = new MainNetParams(); - var context = new Context(networkParameters); + // 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); var ourVersionMessage = new VersionMessage(networkParameters, 0); @@ -229,6 +231,7 @@ public void onSuccess(Peer peer) { peerVersionMessageFuture.set(peer.getPeerVersionMessage()); } public void onFailure(Throwable thr) { + ; } }; Futures.addCallback( From e6dea3d3edf28f9dc6e3474b648c0b0f9f74487f Mon Sep 17 00:00:00 2001 From: dmos62 <2715476+dmos62@users.noreply.github.com> Date: Tue, 18 Feb 2020 17:30:02 +0200 Subject: [PATCH 05/20] Improve marking that method is empty Co-Authored-By: sqrrm --- core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index 343d121808c..e7c15787262 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -231,7 +231,7 @@ public void onSuccess(Peer peer) { peerVersionMessageFuture.set(peer.getPeerVersionMessage()); } public void onFailure(Throwable thr) { - ; + // No action } }; Futures.addCallback( From 65177fcc4c12cd1d3cdb410cc34a35867aaf1ae0 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Fri, 21 Feb 2020 21:20:26 +0200 Subject: [PATCH 06/20] Fix unchecked usage of LocalBitcoinNode.isUsable() --- .../main/java/bisq/core/app/BisqSetup.java | 2 +- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 37 ++++++++++++++++++- .../bisq/core/btc/setup/WalletConfig.java | 2 +- .../bisq/core/btc/setup/WalletsSetup.java | 2 +- .../main/java/bisq/core/user/Preferences.java | 6 +-- .../java/bisq/desktop/main/MainViewModel.java | 2 +- .../settings/network/NetworkSettingsView.java | 6 +-- 7 files changed, 44 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index 89aaf39d532..f98c1c49a01 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -879,7 +879,7 @@ private void maybeShowSecurityRecommendation() { } private void maybeShowLocalhostRunningInfo() { - maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.isUsable().get()); + maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.safeIsUsable()); } private void maybeShowAccountSigningStateInfo() { diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index e7c15787262..7236ba9880b 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -37,7 +37,9 @@ * methods that perform relevant checks and cache the result (methods that start with "check"), * and methods that query that cache (methods that start with "is"). The querying methods return * an Optional, whose emptiness shows if the relevant check is pending, and whose - * contents show the result of the check. + * contents show the result of the check. Method/s starting with "safeIs" are provided to be + * used where the calling code was not refactored to handle Optionals (see + * {@code LocalBitcoinNode#handleUnsafeQuery}). * @see bisq.common.config.Config#ignoreLocalBtcNode */ @Singleton @@ -320,4 +322,37 @@ public Optional isDetected() { public Optional isWellConfigured() { return wellConfigured; } + + /** + * A "safe" variant, which, in case LocalBitcoinNode checks were + * not performed, reverts to legacy behaviour and logs an error message. See + * {@code LocalBitcoinNode#handleUnsafeQuery}. + */ + public boolean safeIsUsable() { + return handleUnsafeQuery(isUsable()); + } + + private boolean handleUnsafeQuery(Optional opt) { + return opt.orElseGet(() -> { + /* Returning false when checks haven't been performed yet is what the behaviour + * was before we switched to using Optionals. More specifically, the only query + * method at the time, isDetected(), would return false in such a case. We are + * relatively confident that the previous behaviour doesn't cause fatal bugs, + * so, in case LocalBitcoinNode is queried too early, we revert to it, instead + * of letting Optional.empty().get() throw an exception. The advantage over + * plain booleans then is that we can log the below error message (with + * stacktrace). + */ + var whenChecksNotFinished = false; + + var throwable = new Throwable("LocalBitcoinNode was queried before it was ready"); + + log.error("Unexpectedly queried LocalBitcoinNode before its checks were performed." + + " This should never happen." + + " Please report this on Bisq's issue tracker, including the following stacktrace:", + throwable); + + return whenChecksNotFinished; + }); + } } diff --git a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java index c6cae1c4ced..b72edd261d7 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java @@ -244,7 +244,7 @@ private PeerGroup createPeerGroup() { // It is also disabled if the local node was not found or was found to be misconfigured. if (Config.baseCurrencyNetwork().isDaoRegTest() || Config.baseCurrencyNetwork().isDaoTestNet() || - !localBitcoinNode.isUsable().get()) + !localBitcoinNode.safeIsUsable()) peerGroup.setUseLocalhostPeerWhenPossible(false); return peerGroup; diff --git a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java index 4dd9d908631..82afc42240e 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java @@ -278,7 +278,7 @@ protected void onSetupCompleted() { return; } } - } else if (localBitcoinNode.isUsable().get()) { + } else if (localBitcoinNode.safeIsUsable()) { walletConfig.setMinBroadcastConnections(1); walletConfig.setPeerNodesForLocalHost(); } else { diff --git a/core/src/main/java/bisq/core/user/Preferences.java b/core/src/main/java/bisq/core/user/Preferences.java index db1a2629c18..3417f059d0e 100644 --- a/core/src/main/java/bisq/core/user/Preferences.java +++ b/core/src/main/java/bisq/core/user/Preferences.java @@ -746,11 +746,7 @@ public boolean getUseTorForBitcoinJ() { // checks. This was noticed when LocalBitcoinNode was refactored to return // Optional istead of boolean, an empty Optional signifying that the relevant // check has not yet been performed. - // - // 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); + var usableLocalNodePresent = localBitcoinNode.safeIsUsable(); if ((!Config.baseCurrencyNetwork().isMainnet() || usableLocalNodePresent) diff --git a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java index 3e4bdcd731d..d2eb60d5bb5 100644 --- a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java +++ b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java @@ -451,7 +451,7 @@ private void setupBtcNumPeersWatcher() { checkNumberOfBtcPeersTimer = UserThread.runAfter(() -> { // check again numPeers if (walletsSetup.numPeersProperty().get() == 0) { - if (localBitcoinNode.isUsable().get()) + if (localBitcoinNode.safeIsUsable()) getWalletServiceErrorMsg().set( Res.get("mainView.networkWarning.localhostBitcoinLost", Res.getBaseCurrencyName().toLowerCase())); else diff --git a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java index 9004ebc75a5..cfecd105fb3 100644 --- a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java +++ b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java @@ -165,7 +165,7 @@ public void initialize() { bitcoinPeerSubVersionColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.subVersionColumn"))); bitcoinPeerHeightColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.heightColumn"))); localhostBtcNodeInfoLabel.setText(Res.get("settings.net.localhostBtcNodeInfo")); - if (!localBitcoinNode.isUsable().get()) { + if (!localBitcoinNode.safeIsUsable()) { localhostBtcNodeInfoLabel.setVisible(false); } useProvidedNodesRadio.setText(Res.get("settings.net.useProvidedNodesRadio")); @@ -380,7 +380,7 @@ private void showShutDownPopup() { } private void onBitcoinPeersToggleSelected(boolean calledFromUser) { - boolean bitcoinLocalhostNodeRunning = localBitcoinNode.isUsable().get(); + boolean bitcoinLocalhostNodeRunning = localBitcoinNode.safeIsUsable(); useTorForBtcJCheckBox.setDisable(bitcoinLocalhostNodeRunning); bitcoinNodesLabel.setDisable(bitcoinLocalhostNodeRunning); btcNodesLabel.setDisable(bitcoinLocalhostNodeRunning); @@ -454,7 +454,7 @@ private void onBitcoinPeersToggleSelected(boolean calledFromUser) { private void applyPreventPublicBtcNetwork() { final boolean preventPublicBtcNetwork = isPreventPublicBtcNetwork(); - usePublicNodesRadio.setDisable(localBitcoinNode.isUsable().get() || preventPublicBtcNetwork); + usePublicNodesRadio.setDisable(localBitcoinNode.safeIsUsable() || preventPublicBtcNetwork); if (preventPublicBtcNetwork && selectedBitcoinNodesOption == BtcNodes.BitcoinNodesOption.PUBLIC) { selectedBitcoinNodesOption = BtcNodes.BitcoinNodesOption.PROVIDED; preferences.setBitcoinNodesOptionOrdinal(selectedBitcoinNodesOption.ordinal()); From 08cd31b242892c519e58c3dfdac4757411d8fca7 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Fri, 21 Feb 2020 23:37:13 +0200 Subject: [PATCH 07/20] Silence NioClient and NioClientManager loggers --- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index 7236ba9880b..36a06dee698 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -7,6 +7,7 @@ import org.bitcoinj.core.VersionMessage; import org.bitcoinj.core.listeners.PeerDisconnectedEventListener; import org.bitcoinj.net.NioClient; +import org.bitcoinj.net.NioClientManager; import org.bitcoinj.params.MainNetParams; import javax.inject.Inject; @@ -31,6 +32,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import ch.qos.logback.classic.Level; + /** * Detects whether a Bitcoin node is running on localhost and whether it is well configured * (meaning it's not pruning and has bloom filters enabled). The class is split into @@ -195,7 +198,15 @@ private Optional attemptHandshakeForVersionMessage() { return Optional.empty(); } + /* We temporarily silence BitcoinJ NioClient's and NioClientManager's loggers, + * because when a local Bitcoin node is not found they pollute console output + * with "connection refused" error messages. + */ + var originalNioClientLoggerLevel = silence(NioClient.class); + var originalNioClientManagerLoggerLevel = silence(NioClientManager.class); + NioClient client; + try { log.info("Initiating attempt to connect to and handshake with a local " + "Bitcoin node (which may or may not be running) on port {}.", @@ -221,9 +232,27 @@ private Optional attemptHandshakeForVersionMessage() { peer.close(); client.closeConnection(); + restoreLoggerLevel(NioClient.class, originalNioClientLoggerLevel); + restoreLoggerLevel(NioClientManager.class, originalNioClientManagerLoggerLevel); + return optionalPeerVersionMessage; } + private static Level silence(Class klass) { + var logger = getLogger(klass); + var originalLevel = logger.getLevel(); + logger.setLevel(Level.OFF); + return originalLevel; + } + + private static void restoreLoggerLevel(Class klass, Level originalLevel) { + getLogger(klass).setLevel(originalLevel); + } + + private static ch.qos.logback.classic.Logger getLogger(Class klass) { + return (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(klass); + } + private ListenableFuture getVersionMessage(Peer peer) { SettableFuture peerVersionMessageFuture = SettableFuture.create(); From 7848836adc83ef5393d0af821d81c3fe63ea80f7 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Sat, 22 Feb 2020 01:30:09 +0200 Subject: [PATCH 08/20] Formating changes --- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 83 ++++++++----------- 1 file changed, 35 insertions(+), 48 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index 36a06dee698..147540b7b0e 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -35,14 +35,14 @@ import ch.qos.logback.classic.Level; /** - * Detects whether a Bitcoin node is running on localhost and whether it is well configured - * (meaning it's not pruning and has bloom filters enabled). The class is split into - * methods that perform relevant checks and cache the result (methods that start with "check"), - * and methods that query that cache (methods that start with "is"). The querying methods return - * an Optional, whose emptiness shows if the relevant check is pending, and whose - * contents show the result of the check. Method/s starting with "safeIs" are provided to be - * used where the calling code was not refactored to handle Optionals (see - * {@code LocalBitcoinNode#handleUnsafeQuery}). + * Detects whether a Bitcoin node is running on localhost and whether it is well + * configured (meaning it's not pruning and has bloom filters enabled). The class is + * split into methods that perform relevant checks and cache the result (methods that + * start with "check"), and methods that query that cache (methods that start with "is"). + * The querying methods return an Optional, whose emptiness shows if the + * relevant check is pending, and whose contents show the result of the check. Method/s + * starting with "safeIs" are provided to be used where the calling code was not + * refactored to handle Optionals (see {@code LocalBitcoinNode#handleUnsafeQuery}). * @see bisq.common.config.Config#ignoreLocalBtcNode */ @Singleton @@ -62,14 +62,10 @@ public LocalBitcoinNode(@Named(LOCAL_BITCOIN_NODE_PORT) int port) { this.port = port; } - /** - * Creates an NioClient that is expected to only be used to coerce a VersionMessage out of a - * local Bitcoin node and be closed right after. + /* Creates an NioClient that is expected to only be used to coerce a VersionMessage + * out of a local Bitcoin node and be closed right after. */ - - private static NioClient createClient( - Peer peer, int port, int connectionTimeout - ) throws IOException { + private static NioClient createClient(Peer peer, int port, int connectionTimeout) throws IOException { InetSocketAddress serverAddress = new InetSocketAddress(InetAddress.getLocalHost(), port); @@ -80,20 +76,16 @@ private static NioClient createClient( return client; } - /** - * Creates a Peer that is expected to only be used to coerce a VersionMessage out of a + /* Creates a Peer that is expected to only be used to coerce a VersionMessage out of a * local Bitcoin node and be closed right after. */ - - private static Peer createLocalPeer( - int port - ) throws UnknownHostException { + 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(); - // 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. + // 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); var ourVersionMessage = new VersionMessage(networkParameters, 0); @@ -113,18 +105,16 @@ private static boolean checkWellConfigured(VersionMessage versionMessage) { return notPruning && supportsAndAllowsBloomFilters; } - /** - * Method backported from upstream bitcoinj: at the time of writing, our version is + /* 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. + // 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; @@ -143,10 +133,9 @@ private static boolean isBloomFilteringSupportedAndEnabled(VersionMessage versio } /** - * Initiates detection and configuration checks. The results are cached so that the public - * methods isUsable, isDetected, isWellConfigured don't trigger a recheck. + * 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); @@ -181,14 +170,14 @@ private void handleHandshakeAttempt(Optional optionalVersionMess } } - /** Performs a blocking Bitcoin protocol handshake, which includes exchanging version messages and acks. - * Its purpose is to check if a local Bitcoin node is running, and, if it is, check its advertised - * configuration. The returned Optional is empty, if a local peer wasn't found, or if handshake failed - * for some reason. This method could be noticably simplified, by turning connection failure callback - * into a future and using a first-future-to-complete type of construct, but I couldn't find a - * ready-made implementation. + /* Performs a blocking Bitcoin protocol handshake, which includes exchanging version + * messages and acks. Its purpose is to check if a local Bitcoin node is running, + * and, if it is, check its advertised configuration. The returned Optional is empty, + * if a local peer wasn't found, or if handshake failed for some reason. This method + * could be noticably simplified, by turning connection failure callback into a + * future and using a first-future-to-complete type of construct, but I couldn't find + * a ready-made implementation. */ - private Optional attemptHandshakeForVersionMessage() { Peer peer; try { @@ -276,8 +265,9 @@ public void onPeerDisconnected(Peer peer, int peerCount) { var peerVersionMessageAlreadyReceived = peerVersionMessageFuture.isDone(); if (peerVersionMessageAlreadyReceived) { - // This method is called whether or not the handshake was successful. - // In case it was successful, we don't want to do anything here. + // This method is called whether or not the handshake was + // successful. In case it was successful, we don't want to do + // anything here. return; } // In some cases Peer will self-disconnect after receiving @@ -308,7 +298,6 @@ public void onPeerDisconnected(Peer peer, int peerCount) { * meaning it's been detected and its configuration satisfied our checks; or, in * the case that it is empty, it signifies that the checks have not yet completed. */ - public Optional isUsable() { // If a node is found to be well configured, it implies that it was also detected, // so this is query is enough to show if the relevant checks were performed and if @@ -320,7 +309,6 @@ public Optional isUsable() { * Returns an Optional that, when not empty, tells you whether the local node * was detected, but misconfigured. */ - public Optional isDetectedButMisconfigured() { return isDetected().flatMap(goodDetect -> isWellConfigured().map(goodConfig -> @@ -332,8 +320,8 @@ public Optional isDetectedButMisconfigured() { * Returns an optional, which is empty in case detection has not yet completed, or * which contains a Boolean, in case detection has been performed, which signifies * whether or not a Bitcoin node was running on localhost at the time the checks were - * performed. No further monitoring is performed, so if the node goes up or down in the - * meantime, this method will continue to return its original value. See + * performed. No further monitoring is performed, so if the node goes up or down in + * the meantime, this method will continue to return its original value. See * {@code MainViewModel#setupBtcNumPeersWatcher} to understand how disconnection and * reconnection of the local Bitcoin node is actually handled. */ @@ -343,11 +331,10 @@ public Optional isDetected() { /** * Returns an optional whose emptiness signifies whether or not configuration checks - * have been performed, and its Boolean contents whether the local node's configuration - * satisfied our checks at the time they were performed. We check if the local node is - * not pruning and has bloom filters enabled. + * have been performed, and its Boolean contents whether the local node's + * configuration satisfied our checks at the time they were performed. We check if the + * local node is not pruning and has bloom filters enabled. */ - public Optional isWellConfigured() { return wellConfigured; } From 0bbbe8c1e90c6f2be9e3c15520f2ae87eadffaf2 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Tue, 25 Feb 2020 13:47:18 +0100 Subject: [PATCH 09/20] Perform checks automatically on first query It's quite amazing how obvious this was, yet I missed it for such a long time. Simplifies usage of LocalBitcoinNode and its internals even more so. Fixes #4005. The way we structured LocalBitcoinNode was as if the detection checks were expensive, but they're not. Previously, in some cases we would notice that a local BTC node wouldn't be used even if it was detected, so we would skip these checks. This optimization now doesn't happen. It might be reimplemented in a coming change where more local BTC node logic is moved into LocalBitcoinNode, but, even if it's not, this check is fairly cheap. A notable exception is if the local BTC node is not responding, which would cause us to wait for a timeout, but if that is the case the mentioned optimization wouldn't help (most of the time). --- .../main/java/bisq/core/app/BisqSetup.java | 12 +- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 122 +++++++----------- .../bisq/core/btc/setup/WalletConfig.java | 2 +- .../bisq/core/btc/setup/WalletsSetup.java | 2 +- .../main/java/bisq/core/user/Preferences.java | 10 +- .../java/bisq/desktop/main/MainViewModel.java | 2 +- .../settings/network/NetworkSettingsView.java | 6 +- 7 files changed, 56 insertions(+), 100 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index f98c1c49a01..f15722d64aa 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -494,13 +494,9 @@ private void maybeCheckLocalBitcoinNode(Runnable nextStep) { return; } - // 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(); + // 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(); if (detectedButMisconfigured) { displayLocalNodeMisconfigurationHandler.accept(nextStep); return; @@ -879,7 +875,7 @@ private void maybeShowSecurityRecommendation() { } private void maybeShowLocalhostRunningInfo() { - maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.safeIsUsable()); + maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.isUsable()); } private void maybeShowAccountSigningStateInfo() { diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index 147540b7b0e..a323ec42a8d 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -36,13 +36,10 @@ /** * Detects whether a Bitcoin node is running on localhost and whether it is well - * configured (meaning it's not pruning and has bloom filters enabled). The class is - * split into methods that perform relevant checks and cache the result (methods that - * start with "check"), and methods that query that cache (methods that start with "is"). - * The querying methods return an Optional, whose emptiness shows if the - * relevant check is pending, and whose contents show the result of the check. Method/s - * starting with "safeIs" are provided to be used where the calling code was not - * refactored to handle Optionals (see {@code LocalBitcoinNode#handleUnsafeQuery}). + * configured (meaning it's not pruning and has bloom filters enabled). The public + * methods automatically trigger detection and (if detected) configuration checks, + * and cache the results, and consequent queries to `LocalBitcoinNode` will always + * return the cached results. * @see bisq.common.config.Config#ignoreLocalBtcNode */ @Singleton @@ -54,8 +51,9 @@ public class LocalBitcoinNode { private static final int CONNECTION_TIMEOUT = 5000; private final int port; - private Optional detected = Optional.empty(); - private Optional wellConfigured = Optional.empty(); + + private Boolean detected; + private Boolean wellConfigured; @Inject public LocalBitcoinNode(@Named(LOCAL_BITCOIN_NODE_PORT) int port) { @@ -132,38 +130,41 @@ private static boolean isBloomFilteringSupportedAndEnabled(VersionMessage versio 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. + /* Performs checks that the query methods might be interested in. + */ + private void performChecks() { + checkUsable(); + } + + /* Initiates detection and configuration checks. The results are cached so that the + * public methods isUsable, isDetected, etc. don't trigger a recheck. */ - public boolean checkUsable() { + private void checkUsable() { var optionalVersionMessage = attemptHandshakeForVersionMessage(); handleHandshakeAttempt(optionalVersionMessage); - // We know that the Optional/s will be populated by the end of the checks. - return isUsable().get(); } private void handleHandshakeAttempt(Optional optionalVersionMessage) { if (!optionalVersionMessage.isPresent()) { - detected = Optional.of(false); - wellConfigured = Optional.of(false); + detected = false; + wellConfigured = false; log.info("No local Bitcoin node detected on port {}," + " or the connection was prematurely closed" + " (before a version messages could be coerced)", port); } else { - detected = Optional.of(true); + detected = true; log.info("Local Bitcoin node detected on port {}", port); var versionMessage = optionalVersionMessage.get(); var configurationCheckResult = checkWellConfigured(versionMessage); if (configurationCheckResult) { - wellConfigured = Optional.of(true); + wellConfigured = true; log.info("Local Bitcoin node found to be well configured" + " (not pruning and allows bloom filters)"); } else { - wellConfigured = Optional.of(false); + wellConfigured = false; log.info("Local Bitcoin node badly configured" + " (it is pruning and/or bloom filters are disabled)"); } @@ -293,12 +294,11 @@ public void onPeerDisconnected(Peer peer, int peerCount) { } /** - * Returns an optional that, in case it is not empty, shows whether or not the - * local node was fit for usage at the time the checks were performed called, - * meaning it's been detected and its configuration satisfied our checks; or, in - * the case that it is empty, it signifies that the checks have not yet completed. + * Returns whether or not a local Bitcion node was detected and was well configured + * at the time the checks were performed. All checks are triggered in case they have + * not been performed. */ - public Optional isUsable() { + public boolean isUsable() { // If a node is found to be well configured, it implies that it was also detected, // so this is query is enough to show if the relevant checks were performed and if // their results are positive. @@ -306,69 +306,37 @@ public Optional isUsable() { } /** - * Returns an Optional that, when not empty, tells you whether the local node - * was detected, but misconfigured. + * Returns whether the local node was detected, but misconfigured. Combination of + * methods isDetected and isWellConfigured. */ - public Optional isDetectedButMisconfigured() { - return isDetected().flatMap(goodDetect -> - isWellConfigured().map(goodConfig -> - goodDetect && !goodConfig - )); + public boolean isDetectedButMisconfigured() { + return isDetected() && !isWellConfigured(); } /** - * Returns an optional, which is empty in case detection has not yet completed, or - * which contains a Boolean, in case detection has been performed, which signifies - * whether or not a Bitcoin node was running on localhost at the time the checks were - * performed. No further monitoring is performed, so if the node goes up or down in - * the meantime, this method will continue to return its original value. See - * {@code MainViewModel#setupBtcNumPeersWatcher} to understand how disconnection and - * reconnection of the local Bitcoin node is actually handled. + * Returns whether a local Bitcoin node was detected. All checks are triggered in case + * they have not been performed. No further monitoring is performed, so if the node + * goes up or down in the meantime, this method will continue to return its original + * value. See {@code MainViewModel#setupBtcNumPeersWatcher} to understand how + * disconnection and reconnection of the local Bitcoin node is actually handled. */ - public Optional isDetected() { + public boolean isDetected() { + if (detected == null) { + performChecks(); + } return detected; } /** - * Returns an optional whose emptiness signifies whether or not configuration checks - * have been performed, and its Boolean contents whether the local node's - * configuration satisfied our checks at the time they were performed. We check if the - * local node is not pruning and has bloom filters enabled. + * Returns whether the local node's configuration satisfied our checks at the time + * they were performed. All checks are triggered in case they have not been performed. + * We check if the local node is not pruning and has bloom filters enabled. */ - public Optional isWellConfigured() { + public boolean isWellConfigured() { + if (wellConfigured == null) { + performChecks(); + } return wellConfigured; } - /** - * A "safe" variant, which, in case LocalBitcoinNode checks were - * not performed, reverts to legacy behaviour and logs an error message. See - * {@code LocalBitcoinNode#handleUnsafeQuery}. - */ - public boolean safeIsUsable() { - return handleUnsafeQuery(isUsable()); - } - - private boolean handleUnsafeQuery(Optional opt) { - return opt.orElseGet(() -> { - /* Returning false when checks haven't been performed yet is what the behaviour - * was before we switched to using Optionals. More specifically, the only query - * method at the time, isDetected(), would return false in such a case. We are - * relatively confident that the previous behaviour doesn't cause fatal bugs, - * so, in case LocalBitcoinNode is queried too early, we revert to it, instead - * of letting Optional.empty().get() throw an exception. The advantage over - * plain booleans then is that we can log the below error message (with - * stacktrace). - */ - var whenChecksNotFinished = false; - - var throwable = new Throwable("LocalBitcoinNode was queried before it was ready"); - - log.error("Unexpectedly queried LocalBitcoinNode before its checks were performed." - + " This should never happen." - + " Please report this on Bisq's issue tracker, including the following stacktrace:", - throwable); - - return whenChecksNotFinished; - }); - } } diff --git a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java index b72edd261d7..67187c66ce4 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java @@ -244,7 +244,7 @@ private PeerGroup createPeerGroup() { // It is also disabled if the local node was not found or was found to be misconfigured. if (Config.baseCurrencyNetwork().isDaoRegTest() || Config.baseCurrencyNetwork().isDaoTestNet() || - !localBitcoinNode.safeIsUsable()) + !localBitcoinNode.isUsable()) peerGroup.setUseLocalhostPeerWhenPossible(false); return peerGroup; diff --git a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java index 82afc42240e..d97a78c25e2 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java @@ -278,7 +278,7 @@ protected void onSetupCompleted() { return; } } - } else if (localBitcoinNode.safeIsUsable()) { + } else if (localBitcoinNode.isUsable()) { walletConfig.setMinBroadcastConnections(1); walletConfig.setPeerNodesForLocalHost(); } else { diff --git a/core/src/main/java/bisq/core/user/Preferences.java b/core/src/main/java/bisq/core/user/Preferences.java index 3417f059d0e..bcb0ab118e9 100644 --- a/core/src/main/java/bisq/core/user/Preferences.java +++ b/core/src/main/java/bisq/core/user/Preferences.java @@ -740,16 +740,8 @@ public boolean getUseTorForBitcoinJ() { // unless the useTorForBtc parameter is explicitly provided. // On testnet there are very few Bitcoin tor nodes and we don't provide tor nodes. - // TODO bug. Non-critical, apparently. - // Sometimes this method, which queries LocalBitcoinNode for whether or not there's a - // usable local Bitcoin node, is called before LocalBitcoinNode has performed its - // checks. This was noticed when LocalBitcoinNode was refactored to return - // Optional istead of boolean, an empty Optional signifying that the relevant - // check has not yet been performed. - var usableLocalNodePresent = localBitcoinNode.safeIsUsable(); - if ((!Config.baseCurrencyNetwork().isMainnet() - || usableLocalNodePresent) + || localBitcoinNode.isUsable()) && !config.useTorForBtcOptionSetExplicitly) return false; else diff --git a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java index d2eb60d5bb5..b549d5c5915 100644 --- a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java +++ b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java @@ -451,7 +451,7 @@ private void setupBtcNumPeersWatcher() { checkNumberOfBtcPeersTimer = UserThread.runAfter(() -> { // check again numPeers if (walletsSetup.numPeersProperty().get() == 0) { - if (localBitcoinNode.safeIsUsable()) + if (localBitcoinNode.isUsable()) getWalletServiceErrorMsg().set( Res.get("mainView.networkWarning.localhostBitcoinLost", Res.getBaseCurrencyName().toLowerCase())); else diff --git a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java index cfecd105fb3..41fe41f166a 100644 --- a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java +++ b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java @@ -165,7 +165,7 @@ public void initialize() { bitcoinPeerSubVersionColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.subVersionColumn"))); bitcoinPeerHeightColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.heightColumn"))); localhostBtcNodeInfoLabel.setText(Res.get("settings.net.localhostBtcNodeInfo")); - if (!localBitcoinNode.safeIsUsable()) { + if (!localBitcoinNode.isUsable()) { localhostBtcNodeInfoLabel.setVisible(false); } useProvidedNodesRadio.setText(Res.get("settings.net.useProvidedNodesRadio")); @@ -380,7 +380,7 @@ private void showShutDownPopup() { } private void onBitcoinPeersToggleSelected(boolean calledFromUser) { - boolean bitcoinLocalhostNodeRunning = localBitcoinNode.safeIsUsable(); + boolean bitcoinLocalhostNodeRunning = localBitcoinNode.isUsable(); useTorForBtcJCheckBox.setDisable(bitcoinLocalhostNodeRunning); bitcoinNodesLabel.setDisable(bitcoinLocalhostNodeRunning); btcNodesLabel.setDisable(bitcoinLocalhostNodeRunning); @@ -454,7 +454,7 @@ private void onBitcoinPeersToggleSelected(boolean calledFromUser) { private void applyPreventPublicBtcNetwork() { final boolean preventPublicBtcNetwork = isPreventPublicBtcNetwork(); - usePublicNodesRadio.setDisable(localBitcoinNode.safeIsUsable() || preventPublicBtcNetwork); + usePublicNodesRadio.setDisable(localBitcoinNode.isUsable() || preventPublicBtcNetwork); if (preventPublicBtcNetwork && selectedBitcoinNodesOption == BtcNodes.BitcoinNodesOption.PUBLIC) { selectedBitcoinNodesOption = BtcNodes.BitcoinNodesOption.PROVIDED; preferences.setBitcoinNodesOptionOrdinal(selectedBitcoinNodesOption.ordinal()); From aceb608e3a13c113fa7568b9228466d1edd5757e Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Tue, 25 Feb 2020 14:18:36 +0100 Subject: [PATCH 10/20] Reorder methods --- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 210 +++++++++--------- 1 file changed, 105 insertions(+), 105 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index a323ec42a8d..e145c7c8cac 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -60,74 +60,50 @@ public LocalBitcoinNode(@Named(LOCAL_BITCOIN_NODE_PORT) int port) { this.port = port; } - /* Creates an NioClient that is expected to only be used to coerce a VersionMessage - * out of a local Bitcoin node and be closed right after. + /** + * Returns whether or not a local Bitcion node was detected and was well configured + * at the time the checks were performed. All checks are triggered in case they have + * not been performed. */ - private static NioClient createClient(Peer peer, int port, int connectionTimeout) throws IOException { - InetSocketAddress serverAddress = - new InetSocketAddress(InetAddress.getLocalHost(), port); - - // 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); - - return client; + public boolean isUsable() { + // If a node is found to be well configured, it implies that it was also detected, + // so this is query is enough to show if the relevant checks were performed and if + // their results are positive. + return isWellConfigured(); } - /* Creates a Peer that is expected to only be used to coerce a VersionMessage out of a - * local Bitcoin node and be closed right after. + /** + * Returns whether the local node was detected, but misconfigured. Combination of + * methods isDetected and isWellConfigured. */ - 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(); - - // 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); - - var ourVersionMessage = new VersionMessage(networkParameters, 0); - - var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port); - - AbstractBlockChain blockchain = null; - - var peer = new Peer(networkParameters, ourVersionMessage, localPeerAddress, blockchain); - return peer; + public boolean isDetectedButMisconfigured() { + return isDetected() && !isWellConfigured(); } - private static boolean checkWellConfigured(VersionMessage versionMessage) { - var notPruning = versionMessage.hasBlockChain(); - var supportsAndAllowsBloomFilters = - isBloomFilteringSupportedAndEnabled(versionMessage); - return notPruning && supportsAndAllowsBloomFilters; + /** + * Returns whether a local Bitcoin node was detected. All checks are triggered in case + * they have not been performed. No further monitoring is performed, so if the node + * goes up or down in the meantime, this method will continue to return its original + * value. See {@code MainViewModel#setupBtcNumPeersWatcher} to understand how + * disconnection and reconnection of the local Bitcoin node is actually handled. + */ + public boolean isDetected() { + if (detected == null) { + performChecks(); + } + return detected; } - /* 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 + /** + * Returns whether the local node's configuration satisfied our checks at the time + * they were performed. All checks are triggered in case they have not been performed. + * We check if the local node is not pruning and has bloom filters enabled. */ - 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; - var whenBloomFiltersWereIntroduced = BLOOM_FILTERS_BIP37_PROTOCOL_VERSION; - - int BLOOM_FILTERS_BIP111_PROTOCOL_VERSION = 70011; - var whenBloomFiltersWereDisabledByDefault = BLOOM_FILTERS_BIP111_PROTOCOL_VERSION; - - int clientVersion = versionMessage.clientVersion; - long localServices = versionMessage.localServices; - - if (clientVersion >= whenBloomFiltersWereIntroduced - && clientVersion < whenBloomFiltersWereDisabledByDefault) - return true; - return (localServices & NODE_BLOOM) == NODE_BLOOM; + public boolean isWellConfigured() { + if (wellConfigured == null) { + performChecks(); + } + return wellConfigured; } /* Performs checks that the query methods might be interested in. @@ -171,6 +147,40 @@ private void handleHandshakeAttempt(Optional optionalVersionMess } } + 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 + */ + 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; + var whenBloomFiltersWereIntroduced = BLOOM_FILTERS_BIP37_PROTOCOL_VERSION; + + int BLOOM_FILTERS_BIP111_PROTOCOL_VERSION = 70011; + var whenBloomFiltersWereDisabledByDefault = BLOOM_FILTERS_BIP111_PROTOCOL_VERSION; + + int clientVersion = versionMessage.clientVersion; + long localServices = versionMessage.localServices; + + if (clientVersion >= whenBloomFiltersWereIntroduced + && clientVersion < whenBloomFiltersWereDisabledByDefault) + return true; + return (localServices & NODE_BLOOM) == NODE_BLOOM; + } + /* Performs a blocking Bitcoin protocol handshake, which includes exchanging version * messages and acks. Its purpose is to check if a local Bitcoin node is running, * and, if it is, check its advertised configuration. The returned Optional is empty, @@ -228,6 +238,42 @@ private Optional attemptHandshakeForVersionMessage() { return optionalPeerVersionMessage; } + /* Creates a Peer that is expected to only be used to coerce a VersionMessage out of a + * local Bitcoin node and be closed right after. + */ + 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(); + + // 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); + + var ourVersionMessage = new VersionMessage(networkParameters, 0); + + var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port); + + AbstractBlockChain blockchain = null; + + var peer = new Peer(networkParameters, ourVersionMessage, localPeerAddress, blockchain); + return peer; + } + + /* Creates an NioClient that is expected to only be used to coerce a VersionMessage + * out of a local Bitcoin node and be closed right after. + */ + private static NioClient createClient(Peer peer, int port, int connectionTimeout) throws IOException { + InetSocketAddress serverAddress = + new InetSocketAddress(InetAddress.getLocalHost(), port); + + // 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); + + return client; + } + private static Level silence(Class klass) { var logger = getLogger(klass); var originalLevel = logger.getLevel(); @@ -293,50 +339,4 @@ public void onPeerDisconnected(Peer peer, int peerCount) { return peerVersionMessageFuture; } - /** - * Returns whether or not a local Bitcion node was detected and was well configured - * at the time the checks were performed. All checks are triggered in case they have - * not been performed. - */ - public boolean isUsable() { - // If a node is found to be well configured, it implies that it was also detected, - // so this is query is enough to show if the relevant checks were performed and if - // their results are positive. - return isWellConfigured(); - } - - /** - * Returns whether the local node was detected, but misconfigured. Combination of - * methods isDetected and isWellConfigured. - */ - public boolean isDetectedButMisconfigured() { - return isDetected() && !isWellConfigured(); - } - - /** - * Returns whether a local Bitcoin node was detected. All checks are triggered in case - * they have not been performed. No further monitoring is performed, so if the node - * goes up or down in the meantime, this method will continue to return its original - * value. See {@code MainViewModel#setupBtcNumPeersWatcher} to understand how - * disconnection and reconnection of the local Bitcoin node is actually handled. - */ - public boolean isDetected() { - if (detected == null) { - performChecks(); - } - return detected; - } - - /** - * Returns whether the local node's configuration satisfied our checks at the time - * they were performed. All checks are triggered in case they have not been performed. - * We check if the local node is not pruning and has bloom filters enabled. - */ - public boolean isWellConfigured() { - if (wellConfigured == null) { - performChecks(); - } - return wellConfigured; - } - } From 6b4878ada8c43d31410556e28df1becdc3f09238 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Tue, 25 Feb 2020 15:56:02 +0100 Subject: [PATCH 11/20] Centralize some of local BTC node logic Introduces LocalBitcoinNode::willUse and ::willIgnore to move logic that was previously littered throughout the codebase into one place. Also, changes the usages of LocalBitcoinNode to be more precise, specifying which of these questions are being asked: - "is there a local BTC node" (isDetected); - "is it well configured" (isWellConfigured and isUsable); - "will we ignore a local BTC node even if we found a usable one" (willIgnore); - "is there a usable local BTC node and will we use it" (willUse). These changes make related logic much easier to maintain and to read. --- .../main/java/bisq/core/app/BisqSetup.java | 8 +---- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 33 ++++++++++++++++++- .../bisq/core/btc/setup/WalletConfig.java | 7 +--- .../bisq/core/btc/setup/WalletsSetup.java | 2 +- .../main/java/bisq/core/user/Preferences.java | 9 ++--- .../java/bisq/desktop/main/MainViewModel.java | 2 +- .../settings/network/NetworkSettingsView.java | 20 +++++------ 7 files changed, 51 insertions(+), 30 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index f15722d64aa..f3a6815d601 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -483,13 +483,7 @@ private void maybeShowTac() { } private void maybeCheckLocalBitcoinNode(Runnable nextStep) { - BaseCurrencyNetwork baseCurrencyNetwork = config.baseCurrencyNetwork; - - var shouldIgnoreLocalNode = - config.ignoreLocalBtcNode - || baseCurrencyNetwork.isDaoRegTest() - || baseCurrencyNetwork.isDaoTestNet(); - if (shouldIgnoreLocalNode) { + if (localBitcoinNode.willIgnore()) { nextStep.run(); return; } diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index e145c7c8cac..32d9ee7d610 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -1,5 +1,8 @@ package bisq.core.btc.nodes; +import bisq.common.config.BaseCurrencyNetwork; +import bisq.common.config.Config; + import org.bitcoinj.core.AbstractBlockChain; import org.bitcoinj.core.Context; import org.bitcoinj.core.Peer; @@ -50,16 +53,44 @@ public class LocalBitcoinNode { private static final Logger log = LoggerFactory.getLogger(LocalBitcoinNode.class); private static final int CONNECTION_TIMEOUT = 5000; + private final Config config; + private final int port; private Boolean detected; private Boolean wellConfigured; @Inject - public LocalBitcoinNode(@Named(LOCAL_BITCOIN_NODE_PORT) int port) { + public LocalBitcoinNode(Config config, @Named(LOCAL_BITCOIN_NODE_PORT) int port) { + this.config = config; this.port = port; } + /** + * Returns whether Bisq will use a local Bitcoin node. Meaning a usable node was found + * and conditions under which we would ignore it are not met. If we're ignoring the + * local node, a call to this method will not trigger an unnecessary detection + * attempt. + */ + public boolean willUse() { + return !willIgnore() && isUsable(); + } + + /** + * Returns whether Bisq will ignore a local Bitcoin node even if it is usable. + */ + public boolean willIgnore() { + BaseCurrencyNetwork baseCurrencyNetwork = config.baseCurrencyNetwork; + + // For dao testnet (server side regtest) we disable the use of local bitcoin node to + // avoid confusion if local btc node is not synced with our dao testnet master node. + // Note: above comment was previously in WalletConfig::createPeerGroup. + + return config.ignoreLocalBtcNode + || baseCurrencyNetwork.isDaoRegTest() + || baseCurrencyNetwork.isDaoTestNet(); + } + /** * Returns whether or not a local Bitcion node was detected and was well configured * at the time the checks were performed. All checks are triggered in case they have diff --git a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java index 67187c66ce4..1aa79bcb70d 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java @@ -239,12 +239,7 @@ private PeerGroup createPeerGroup() { peerGroup.setConnectTimeoutMillis(TOR_VERSION_EXCHANGE_TIMEOUT); } - // For dao testnet (server side regtest) we disable the use of local bitcoin node to - // avoid confusion if local btc node is not synced with our dao testnet master node. - // It is also disabled if the local node was not found or was found to be misconfigured. - if (Config.baseCurrencyNetwork().isDaoRegTest() || - Config.baseCurrencyNetwork().isDaoTestNet() || - !localBitcoinNode.isUsable()) + if (!localBitcoinNode.willUse()) peerGroup.setUseLocalhostPeerWhenPossible(false); return peerGroup; diff --git a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java index d97a78c25e2..acc14699746 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java @@ -278,7 +278,7 @@ protected void onSetupCompleted() { return; } } - } else if (localBitcoinNode.isUsable()) { + } else if (localBitcoinNode.willUse()) { walletConfig.setMinBroadcastConnections(1); walletConfig.setPeerNodesForLocalHost(); } else { diff --git a/core/src/main/java/bisq/core/user/Preferences.java b/core/src/main/java/bisq/core/user/Preferences.java index bcb0ab118e9..f353c22d076 100644 --- a/core/src/main/java/bisq/core/user/Preferences.java +++ b/core/src/main/java/bisq/core/user/Preferences.java @@ -736,12 +736,13 @@ public boolean showAgain(String key) { } public boolean getUseTorForBitcoinJ() { - // We override the useTorForBitcoinJ and set it to false if we found a usable localhost node or if we are not on mainnet, - // unless the useTorForBtc parameter is explicitly provided. - // On testnet there are very few Bitcoin tor nodes and we don't provide tor nodes. + // We override the useTorForBitcoinJ and set it to false if we will use a + // localhost Bitcoin node or if we are not on mainnet, unless the useTorForBtc + // parameter is explicitly provided. On testnet there are very few Bitcoin tor + // nodes and we don't provide tor nodes. if ((!Config.baseCurrencyNetwork().isMainnet() - || localBitcoinNode.isUsable()) + || localBitcoinNode.willUse()) && !config.useTorForBtcOptionSetExplicitly) return false; else diff --git a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java index b549d5c5915..a2035647258 100644 --- a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java +++ b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java @@ -451,7 +451,7 @@ private void setupBtcNumPeersWatcher() { checkNumberOfBtcPeersTimer = UserThread.runAfter(() -> { // check again numPeers if (walletsSetup.numPeersProperty().get() == 0) { - if (localBitcoinNode.isUsable()) + if (localBitcoinNode.willUse()) getWalletServiceErrorMsg().set( Res.get("mainView.networkWarning.localhostBitcoinLost", Res.getBaseCurrencyName().toLowerCase())); else diff --git a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java index 41fe41f166a..5482f4d0dde 100644 --- a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java +++ b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java @@ -165,7 +165,7 @@ public void initialize() { bitcoinPeerSubVersionColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.subVersionColumn"))); bitcoinPeerHeightColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.heightColumn"))); localhostBtcNodeInfoLabel.setText(Res.get("settings.net.localhostBtcNodeInfo")); - if (!localBitcoinNode.isUsable()) { + if (!localBitcoinNode.willUse()) { localhostBtcNodeInfoLabel.setVisible(false); } useProvidedNodesRadio.setText(Res.get("settings.net.useProvidedNodesRadio")); @@ -380,14 +380,14 @@ private void showShutDownPopup() { } private void onBitcoinPeersToggleSelected(boolean calledFromUser) { - boolean bitcoinLocalhostNodeRunning = localBitcoinNode.isUsable(); - useTorForBtcJCheckBox.setDisable(bitcoinLocalhostNodeRunning); - bitcoinNodesLabel.setDisable(bitcoinLocalhostNodeRunning); - btcNodesLabel.setDisable(bitcoinLocalhostNodeRunning); - btcNodesInputTextField.setDisable(bitcoinLocalhostNodeRunning); - useProvidedNodesRadio.setDisable(bitcoinLocalhostNodeRunning || !btcNodes.useProvidedBtcNodes()); - useCustomNodesRadio.setDisable(bitcoinLocalhostNodeRunning); - usePublicNodesRadio.setDisable(bitcoinLocalhostNodeRunning || isPreventPublicBtcNetwork()); + boolean bitcoinLocalhostNodeBeingUsed = localBitcoinNode.willUse(); + useTorForBtcJCheckBox.setDisable(bitcoinLocalhostNodeBeingUsed); + bitcoinNodesLabel.setDisable(bitcoinLocalhostNodeBeingUsed); + btcNodesLabel.setDisable(bitcoinLocalhostNodeBeingUsed); + btcNodesInputTextField.setDisable(bitcoinLocalhostNodeBeingUsed); + useProvidedNodesRadio.setDisable(bitcoinLocalhostNodeBeingUsed || !btcNodes.useProvidedBtcNodes()); + useCustomNodesRadio.setDisable(bitcoinLocalhostNodeBeingUsed); + usePublicNodesRadio.setDisable(bitcoinLocalhostNodeBeingUsed || isPreventPublicBtcNetwork()); BtcNodes.BitcoinNodesOption currentBitcoinNodesOption = BtcNodes.BitcoinNodesOption.values()[preferences.getBitcoinNodesOptionOrdinal()]; @@ -454,7 +454,7 @@ private void onBitcoinPeersToggleSelected(boolean calledFromUser) { private void applyPreventPublicBtcNetwork() { final boolean preventPublicBtcNetwork = isPreventPublicBtcNetwork(); - usePublicNodesRadio.setDisable(localBitcoinNode.isUsable() || preventPublicBtcNetwork); + usePublicNodesRadio.setDisable(localBitcoinNode.willUse() || preventPublicBtcNetwork); if (preventPublicBtcNetwork && selectedBitcoinNodesOption == BtcNodes.BitcoinNodesOption.PUBLIC) { selectedBitcoinNodesOption = BtcNodes.BitcoinNodesOption.PROVIDED; preferences.setBitcoinNodesOptionOrdinal(selectedBitcoinNodesOption.ordinal()); From 2a57ecddfc41b1d0219155df9fac2e9fa6780181 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Tue, 25 Feb 2020 16:22:51 +0100 Subject: [PATCH 12/20] Fix failing test --- core/src/test/java/bisq/core/user/PreferencesTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/bisq/core/user/PreferencesTest.java b/core/src/test/java/bisq/core/user/PreferencesTest.java index 4c8f89b95de..fcde9455f59 100644 --- a/core/src/test/java/bisq/core/user/PreferencesTest.java +++ b/core/src/test/java/bisq/core/user/PreferencesTest.java @@ -60,7 +60,7 @@ public void setUp() { storage = mock(Storage.class); Config config = new Config(); - LocalBitcoinNode localBitcoinNode = new LocalBitcoinNode(config.baseCurrencyNetworkParameters.getPort()); + LocalBitcoinNode localBitcoinNode = new LocalBitcoinNode(config, config.baseCurrencyNetworkParameters.getPort()); preferences = new Preferences( storage, config, localBitcoinNode, null, null, Config.DEFAULT_FULL_DAO_NODE, null, null, Config.UNSPECIFIED_PORT); From 6dec780ab4ee828433af4c0212044fe96f9f1d04 Mon Sep 17 00:00:00 2001 From: dmos62 <2715476+dmos62@users.noreply.github.com> Date: Wed, 26 Feb 2020 14:21:03 +0100 Subject: [PATCH 13/20] Minor requested changes (github batch) Co-Authored-By: Christoph Atteneder --- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index 32d9ee7d610..ac9da87aac6 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -87,8 +87,8 @@ public boolean willIgnore() { // Note: above comment was previously in WalletConfig::createPeerGroup. return config.ignoreLocalBtcNode - || baseCurrencyNetwork.isDaoRegTest() - || baseCurrencyNetwork.isDaoTestNet(); + || baseCurrencyNetwork.isDaoRegTest() + || baseCurrencyNetwork.isDaoTestNet(); } /** @@ -156,8 +156,8 @@ private void handleHandshakeAttempt(Optional optionalVersionMess detected = false; wellConfigured = false; log.info("No local Bitcoin node detected on port {}," - + " or the connection was prematurely closed" - + " (before a version messages could be coerced)", + + " or the connection was prematurely closed" + + " (before a version messages could be coerced)", port); } else { detected = true; @@ -225,7 +225,7 @@ private Optional attemptHandshakeForVersionMessage() { try { peer = createLocalPeer(port); } catch (UnknownHostException ex) { - log.error("Local bitcoin node handshake attempt unexpectedly threw: {}", ex); + log.error("Local bitcoin node handshake attempt was unexpectedly interrupted", ex); return Optional.empty(); } @@ -240,11 +240,11 @@ private Optional attemptHandshakeForVersionMessage() { try { log.info("Initiating attempt to connect to and handshake with a local " - + "Bitcoin node (which may or may not be running) on port {}.", + + "Bitcoin node (which may or may not be running) on port {}.", port); client = createClient(peer, port, CONNECTION_TIMEOUT); } catch (IOException ex) { - log.error("Local bitcoin node handshake attempt unexpectedly threw: {}", ex); + log.error("Local bitcoin node handshake attempt was unexpectedly interrupted", ex); return Optional.empty(); } @@ -305,18 +305,18 @@ private static NioClient createClient(Peer peer, int port, int connectionTimeout return client; } - private static Level silence(Class klass) { + private static Level silence(Class klass) { var logger = getLogger(klass); var originalLevel = logger.getLevel(); logger.setLevel(Level.OFF); return originalLevel; } - private static void restoreLoggerLevel(Class klass, Level originalLevel) { + private static void restoreLoggerLevel(Class klass, Level originalLevel) { getLogger(klass).setLevel(originalLevel); } - private static ch.qos.logback.classic.Logger getLogger(Class klass) { + private static ch.qos.logback.classic.Logger getLogger(Class klass) { return (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(klass); } @@ -324,11 +324,11 @@ private ListenableFuture getVersionMessage(Peer peer) { SettableFuture peerVersionMessageFuture = SettableFuture.create(); var versionHandshakeDone = peer.getVersionHandshakeFuture(); - FutureCallback fetchPeerVersionMessage = new FutureCallback() { + FutureCallback fetchPeerVersionMessage = new FutureCallback<>() { public void onSuccess(Peer peer) { peerVersionMessageFuture.set(peer.getPeerVersionMessage()); } - public void onFailure(Throwable thr) { + public void onFailure(@NotNull Throwable thr) { // No action } }; From a92b6ad9dcda54c5ff3e1fb8893034e3049097e9 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Wed, 26 Feb 2020 14:20:40 +0100 Subject: [PATCH 14/20] Minor requested changes (non-github batch) --- .../main/java/bisq/core/app/BisqSetup.java | 11 ++-- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 53 +++++++++---------- 2 files changed, 32 insertions(+), 32 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index f3a6815d601..6f8061aede1 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -492,8 +492,12 @@ private void maybeCheckLocalBitcoinNode(Runnable nextStep) { // local node is detected, but badly configured. var detectedButMisconfigured = localBitcoinNode.isDetectedButMisconfigured(); if (detectedButMisconfigured) { - displayLocalNodeMisconfigurationHandler.accept(nextStep); - return; + if (displayLocalNodeMisconfigurationHandler != null) { + displayLocalNodeMisconfigurationHandler.accept(nextStep); + return; + } else { + log.error("displayLocalNodeMisconfigurationHandler undefined", new RuntimeException()); + } } nextStep.run(); @@ -566,7 +570,8 @@ 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()) { + // TODO encapsulate below conditional inside getUseTorForBitcoinJ + if (!preferences.getUseTorForBitcoinJ() || localBitcoinNode.willUse()) { initWallet(); } diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index ac9da87aac6..ae073c76c36 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -37,6 +37,8 @@ import ch.qos.logback.classic.Level; +import org.jetbrains.annotations.NotNull; + /** * Detects whether a Bitcoin node is running on localhost and whether it is well * configured (meaning it's not pruning and has bloom filters enabled). The public @@ -285,10 +287,7 @@ private static Peer createLocalPeer(int port) throws UnknownHostException { var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port); - AbstractBlockChain blockchain = null; - - var peer = new Peer(networkParameters, ourVersionMessage, localPeerAddress, blockchain); - return peer; + return new Peer(networkParameters, ourVersionMessage, localPeerAddress, null); } /* Creates an NioClient that is expected to only be used to coerce a VersionMessage @@ -300,9 +299,8 @@ private static NioClient createClient(Peer peer, int port, int connectionTimeout // 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); - return client; + return new NioClient(serverAddress, peer, connectionTimeout); } private static Level silence(Class klass) { @@ -338,29 +336,26 @@ public void onFailure(@NotNull Throwable thr) { ); PeerDisconnectedEventListener cancelIfConnectionFails = - new PeerDisconnectedEventListener() { - public void onPeerDisconnected(Peer peer, int peerCount) { - var peerVersionMessageAlreadyReceived = - peerVersionMessageFuture.isDone(); - if (peerVersionMessageAlreadyReceived) { - // This method is called whether or not the handshake was - // successful. In case it was successful, we don't want to do - // anything here. - return; - } - // In some cases Peer will self-disconnect after receiving - // node's VersionMessage, but before completing the handshake. - // In such a case, we want to retrieve the VersionMessage. - var peerVersionMessage = peer.getPeerVersionMessage(); - if (peerVersionMessage != null) { - log.info("Handshake attempt was interrupted;" - + " however, the local node's version message was coerced."); - peerVersionMessageFuture.set(peerVersionMessage); - } else { - log.info("Handshake attempt did not result in a version message exchange."); - var mayInterruptWhileRunning = true; - peerVersionMessageFuture.cancel(mayInterruptWhileRunning); - } + (Peer disconnectedPeer, int peerCount) -> { + var peerVersionMessageAlreadyReceived = + peerVersionMessageFuture.isDone(); + if (peerVersionMessageAlreadyReceived) { + // This method is called whether or not the handshake was + // successful. In case it was successful, we don't want to do + // anything here. + return; + } + // In some cases Peer will self-disconnect after receiving + // node's VersionMessage, but before completing the handshake. + // In such a case, we want to retrieve the VersionMessage. + var peerVersionMessage = disconnectedPeer.getPeerVersionMessage(); + if (peerVersionMessage != null) { + log.info("Handshake attempt was interrupted;" + + " however, the local node's version message was coerced."); + peerVersionMessageFuture.set(peerVersionMessage); + } else { + log.info("Handshake attempt did not result in a version message exchange."); + peerVersionMessageFuture.cancel(true); } }; From 30578bfa9dc5e01372e118a9b1bcd15eb61e5085 Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Wed, 26 Feb 2020 17:09:01 +0100 Subject: [PATCH 15/20] Have detection work on other network modes This makes detection work on other BTC network modes, not only mainnet. --- .../src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index ae073c76c36..c15362d7d11 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -274,10 +274,8 @@ private Optional attemptHandshakeForVersionMessage() { /* Creates a Peer that is expected to only be used to coerce a VersionMessage out of a * local Bitcoin node and be closed right after. */ - 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(); + private Peer createLocalPeer(int port) throws UnknownHostException { + var networkParameters = config.baseCurrencyNetwork.getParameters(); // 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. From fdaced460fea014c034c30f83e9103c5744300df Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Wed, 26 Feb 2020 17:20:55 +0100 Subject: [PATCH 16/20] Changes to Background information popup --- core/src/main/java/bisq/core/app/BisqSetup.java | 2 +- core/src/main/resources/i18n/displayStrings.properties | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index 6f8061aede1..b4e9e498be7 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -874,7 +874,7 @@ private void maybeShowSecurityRecommendation() { } private void maybeShowLocalhostRunningInfo() { - maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.isUsable()); + maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.willUse()); } private void maybeShowAccountSigningStateInfo() { diff --git a/core/src/main/resources/i18n/displayStrings.properties b/core/src/main/resources/i18n/displayStrings.properties index b033080bf99..31b5c04a6a3 100644 --- a/core/src/main/resources/i18n/displayStrings.properties +++ b/core/src/main/resources/i18n/displayStrings.properties @@ -2663,7 +2663,7 @@ popup.warning.localNodeMisconfigured.explanation=Bisq detected a locally running For Bisq to use a local Bitcoin node, the node has to have pruning disabled and bloom filters enabled. Starting with Bitcoin Core v0.19 you have to manually enable bloom filters by setting peerbloomfilters=1 in your bitcoin.conf configuration file. popup.warning.localNodeMisconfigured.continueWithoutLocalNode=Continue without using local node -popup.bitcoinLocalhostNode.msg=Bisq detected a locally running Bitcoin Core node (at localhost).\n\ +popup.bitcoinLocalhostNode.msg=Bisq detected a locally running Bitcoin Core node (at localhost) and is using it.\n\ Please make sure that this node is fully synced before you start Bisq. popup.bitcoinLocalhostNode.additionalRequirements=\n\nThe node was found to be well configured. For reference, the requirements are for the node to have pruning disabled and bloom filters enabled. From b93ca2b2b1dd24cd53014f9bd85269d572fde032 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Wed, 26 Feb 2020 21:24:38 +0100 Subject: [PATCH 17/20] Polish formatting --- .../main/java/bisq/core/app/BisqSetup.java | 3 +- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 72 ++++++++----------- .../java/bisq/desktop/main/MainViewModel.java | 22 +++--- 3 files changed, 44 insertions(+), 53 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index b4e9e498be7..176bccb0195 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -490,8 +490,7 @@ private void maybeCheckLocalBitcoinNode(Runnable nextStep) { // 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(); - if (detectedButMisconfigured) { + if (localBitcoinNode.isDetectedButMisconfigured()) { if (displayLocalNodeMisconfigurationHandler != null) { displayLocalNodeMisconfigurationHandler.accept(nextStep); return; diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index c15362d7d11..2b22c2a9438 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -3,7 +3,6 @@ import bisq.common.config.BaseCurrencyNetwork; import bisq.common.config.Config; -import org.bitcoinj.core.AbstractBlockChain; import org.bitcoinj.core.Context; import org.bitcoinj.core.Peer; import org.bitcoinj.core.PeerAddress; @@ -11,7 +10,6 @@ import org.bitcoinj.core.listeners.PeerDisconnectedEventListener; import org.bitcoinj.net.NioClient; import org.bitcoinj.net.NioClientManager; -import org.bitcoinj.params.MainNetParams; import javax.inject.Inject; import javax.inject.Named; @@ -56,7 +54,6 @@ public class LocalBitcoinNode { private static final int CONNECTION_TIMEOUT = 5000; private final Config config; - private final int port; private Boolean detected; @@ -87,10 +84,9 @@ public boolean willIgnore() { // For dao testnet (server side regtest) we disable the use of local bitcoin node to // avoid confusion if local btc node is not synced with our dao testnet master node. // Note: above comment was previously in WalletConfig::createPeerGroup. - - return config.ignoreLocalBtcNode - || baseCurrencyNetwork.isDaoRegTest() - || baseCurrencyNetwork.isDaoTestNet(); + return config.ignoreLocalBtcNode || + baseCurrencyNetwork.isDaoRegTest() || + baseCurrencyNetwork.isDaoTestNet(); } /** @@ -139,13 +135,15 @@ public boolean isWellConfigured() { return wellConfigured; } - /* Performs checks that the query methods might be interested in. + /** + * Performs checks that the query methods might be interested in. */ private void performChecks() { checkUsable(); } - /* Initiates detection and configuration checks. The results are cached so that the + /** + * Initiates detection and configuration checks. The results are cached so that the * public methods isUsable, isDetected, etc. don't trigger a recheck. */ private void checkUsable() { @@ -157,10 +155,8 @@ private void handleHandshakeAttempt(Optional optionalVersionMess if (!optionalVersionMessage.isPresent()) { detected = false; wellConfigured = false; - log.info("No local Bitcoin node detected on port {}," - + " or the connection was prematurely closed" - + " (before a version messages could be coerced)", - port); + log.info("No local Bitcoin node detected on port {}, or the connection was prematurely closed" + + " (before a version messages could be coerced)", port); } else { detected = true; log.info("Local Bitcoin node detected on port {}", port); @@ -170,12 +166,10 @@ private void handleHandshakeAttempt(Optional optionalVersionMess if (configurationCheckResult) { wellConfigured = true; - log.info("Local Bitcoin node found to be well configured" - + " (not pruning and allows bloom filters)"); + log.info("Local Bitcoin node found to be well configured (not pruning and allows bloom filters)"); } else { wellConfigured = false; - log.info("Local Bitcoin node badly configured" - + " (it is pruning and/or bloom filters are disabled)"); + log.info("Local Bitcoin node badly configured (it is pruning and/or bloom filters are disabled)"); } } } @@ -187,7 +181,8 @@ private static boolean checkWellConfigured(VersionMessage versionMessage) { return notPruning && supportsAndAllowsBloomFilters; } - /* Method backported from upstream bitcoinj: at the time of writing, our version is + /** + * 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 @@ -211,10 +206,12 @@ private static boolean isBloomFilteringSupportedAndEnabled(VersionMessage versio if (clientVersion >= whenBloomFiltersWereIntroduced && clientVersion < whenBloomFiltersWereDisabledByDefault) return true; + return (localServices & NODE_BLOOM) == NODE_BLOOM; } - /* Performs a blocking Bitcoin protocol handshake, which includes exchanging version + /** + * Performs a blocking Bitcoin protocol handshake, which includes exchanging version * messages and acks. Its purpose is to check if a local Bitcoin node is running, * and, if it is, check its advertised configuration. The returned Optional is empty, * if a local peer wasn't found, or if handshake failed for some reason. This method @@ -231,19 +228,16 @@ private Optional attemptHandshakeForVersionMessage() { return Optional.empty(); } - /* We temporarily silence BitcoinJ NioClient's and NioClientManager's loggers, - * because when a local Bitcoin node is not found they pollute console output - * with "connection refused" error messages. - */ + // We temporarily silence BitcoinJ NioClient's and NioClientManager's loggers, + // because when a local Bitcoin node is not found they pollute console output + // with "connection refused" error messages. var originalNioClientLoggerLevel = silence(NioClient.class); var originalNioClientManagerLoggerLevel = silence(NioClientManager.class); NioClient client; - try { - log.info("Initiating attempt to connect to and handshake with a local " - + "Bitcoin node (which may or may not be running) on port {}.", - port); + log.info("Initiating attempt to connect to and handshake with a local " + + "Bitcoin node (which may or may not be running) on port {}.", port); client = createClient(peer, port, CONNECTION_TIMEOUT); } catch (IOException ex) { log.error("Local bitcoin node handshake attempt was unexpectedly interrupted", ex); @@ -251,7 +245,6 @@ private Optional attemptHandshakeForVersionMessage() { } ListenableFuture peerVersionMessageFuture = getVersionMessage(peer); - Optional optionalPeerVersionMessage; // block for VersionMessage or cancellation (in case of connection failure) @@ -271,7 +264,8 @@ private Optional attemptHandshakeForVersionMessage() { return optionalPeerVersionMessage; } - /* Creates a Peer that is expected to only be used to coerce a VersionMessage out of a + /** + * Creates a Peer that is expected to only be used to coerce a VersionMessage out of a * local Bitcoin node and be closed right after. */ private Peer createLocalPeer(int port) throws UnknownHostException { @@ -288,16 +282,15 @@ private Peer createLocalPeer(int port) throws UnknownHostException { return new Peer(networkParameters, ourVersionMessage, localPeerAddress, null); } - /* Creates an NioClient that is expected to only be used to coerce a VersionMessage + /** + * Creates an NioClient that is expected to only be used to coerce a VersionMessage * out of a local Bitcoin node and be closed right after. */ private static NioClient createClient(Peer peer, int port, int connectionTimeout) throws IOException { - InetSocketAddress serverAddress = - new InetSocketAddress(InetAddress.getLocalHost(), port); + InetSocketAddress serverAddress = new InetSocketAddress(InetAddress.getLocalHost(), port); // This initiates the handshake procedure, which, if successful, will complete // the peerVersionMessageFuture, or be cancelled, in case of failure. - return new NioClient(serverAddress, peer, connectionTimeout); } @@ -324,19 +317,17 @@ private ListenableFuture getVersionMessage(Peer peer) { public void onSuccess(Peer peer) { peerVersionMessageFuture.set(peer.getPeerVersionMessage()); } + public void onFailure(@NotNull Throwable thr) { // No action } }; - Futures.addCallback( - versionHandshakeDone, - fetchPeerVersionMessage - ); + Futures.addCallback(versionHandshakeDone, fetchPeerVersionMessage); PeerDisconnectedEventListener cancelIfConnectionFails = (Peer disconnectedPeer, int peerCount) -> { var peerVersionMessageAlreadyReceived = - peerVersionMessageFuture.isDone(); + peerVersionMessageFuture.isDone(); if (peerVersionMessageAlreadyReceived) { // This method is called whether or not the handshake was // successful. In case it was successful, we don't want to do @@ -348,8 +339,8 @@ public void onFailure(@NotNull Throwable thr) { // In such a case, we want to retrieve the VersionMessage. var peerVersionMessage = disconnectedPeer.getPeerVersionMessage(); if (peerVersionMessage != null) { - log.info("Handshake attempt was interrupted;" - + " however, the local node's version message was coerced."); + log.info("Handshake attempt was interrupted; " + + "however, the local node's version message was coerced."); peerVersionMessageFuture.set(peerVersionMessage); } else { log.info("Handshake attempt did not result in a version message exchange."); @@ -362,5 +353,4 @@ public void onFailure(@NotNull Throwable thr) { return peerVersionMessageFuture; } - } diff --git a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java index a2035647258..ad6870ef12e 100644 --- a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java +++ b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java @@ -307,14 +307,14 @@ private void setupHandlers() { }); bisqSetup.setDisplayLocalNodeMisconfigurationHandler( (Runnable continueWithoutLocalNode) -> - new Popup() - .hideCloseButton() - .warning(Res.get("popup.warning.localNodeMisconfigured.explanation")) - .useShutDownButton() - .secondaryActionButtonText(Res.get("popup.warning.localNodeMisconfigured.continueWithoutLocalNode")) - .onSecondaryAction(continueWithoutLocalNode) - .show() - ); + new Popup() + .hideCloseButton() + .warning(Res.get("popup.warning.localNodeMisconfigured.explanation")) + .useShutDownButton() + .secondaryActionButtonText(Res.get("popup.warning.localNodeMisconfigured.continueWithoutLocalNode")) + .onSecondaryAction(continueWithoutLocalNode) + .show() + ); bisqSetup.setSpvFileCorruptedHandler(msg -> new Popup().warning(msg) .actionButtonText(Res.get("settings.net.reSyncSPVChainButton")) .onAction(() -> GUIUtil.reSyncSPVChain(preferences)) @@ -453,10 +453,12 @@ private void setupBtcNumPeersWatcher() { if (walletsSetup.numPeersProperty().get() == 0) { if (localBitcoinNode.willUse()) getWalletServiceErrorMsg().set( - Res.get("mainView.networkWarning.localhostBitcoinLost", Res.getBaseCurrencyName().toLowerCase())); + Res.get("mainView.networkWarning.localhostBitcoinLost", + Res.getBaseCurrencyName().toLowerCase())); else getWalletServiceErrorMsg().set( - Res.get("mainView.networkWarning.allConnectionsLost", Res.getBaseCurrencyName().toLowerCase())); + Res.get("mainView.networkWarning.allConnectionsLost", + Res.getBaseCurrencyName().toLowerCase())); } else { getWalletServiceErrorMsg().set(null); } From c1a99ccc55b57cc784f3b5c28c6dd48e99ad1291 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Thu, 27 Feb 2020 09:35:36 +0100 Subject: [PATCH 18/20] Polish LocalBitcoinNode method names and visibility - Rename #willUse => #shouldBeUsed - Rename #willIgnore => #shouldBeIgnored - Reduce visibility of methods where appropriate - Edit Javadoc typos and use @link syntax where appropriate --- .../main/java/bisq/core/app/BisqSetup.java | 7 ++-- .../bisq/core/btc/nodes/LocalBitcoinNode.java | 42 +++++++++---------- .../bisq/core/btc/setup/WalletConfig.java | 2 +- .../bisq/core/btc/setup/WalletsSetup.java | 2 +- .../main/java/bisq/core/user/Preferences.java | 2 +- .../java/bisq/desktop/main/MainViewModel.java | 2 +- .../settings/network/NetworkSettingsView.java | 20 ++++----- 7 files changed, 37 insertions(+), 40 deletions(-) diff --git a/core/src/main/java/bisq/core/app/BisqSetup.java b/core/src/main/java/bisq/core/app/BisqSetup.java index 176bccb0195..42316e88726 100644 --- a/core/src/main/java/bisq/core/app/BisqSetup.java +++ b/core/src/main/java/bisq/core/app/BisqSetup.java @@ -76,7 +76,6 @@ import bisq.common.UserThread; import bisq.common.app.DevEnv; import bisq.common.app.Log; -import bisq.common.config.BaseCurrencyNetwork; import bisq.common.config.Config; import bisq.common.crypto.CryptoException; import bisq.common.crypto.KeyRing; @@ -483,7 +482,7 @@ private void maybeShowTac() { } private void maybeCheckLocalBitcoinNode(Runnable nextStep) { - if (localBitcoinNode.willIgnore()) { + if (localBitcoinNode.shouldBeIgnored()) { nextStep.run(); return; } @@ -570,7 +569,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. // TODO encapsulate below conditional inside getUseTorForBitcoinJ - if (!preferences.getUseTorForBitcoinJ() || localBitcoinNode.willUse()) { + if (!preferences.getUseTorForBitcoinJ() || localBitcoinNode.shouldBeUsed()) { initWallet(); } @@ -873,7 +872,7 @@ private void maybeShowSecurityRecommendation() { } private void maybeShowLocalhostRunningInfo() { - maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.willUse()); + maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.shouldBeUsed()); } private void maybeShowAccountSigningStateInfo() { diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index 2b22c2a9438..414d1d2c20b 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -41,7 +41,7 @@ * Detects whether a Bitcoin node is running on localhost and whether it is well * configured (meaning it's not pruning and has bloom filters enabled). The public * methods automatically trigger detection and (if detected) configuration checks, - * and cache the results, and consequent queries to `LocalBitcoinNode` will always + * and cache the results, and subsequent queries to {@link LocalBitcoinNode} will always * return the cached results. * @see bisq.common.config.Config#ignoreLocalBtcNode */ @@ -66,35 +66,35 @@ public LocalBitcoinNode(Config config, @Named(LOCAL_BITCOIN_NODE_PORT) int port) } /** - * Returns whether Bisq will use a local Bitcoin node. Meaning a usable node was found - * and conditions under which we would ignore it are not met. If we're ignoring the - * local node, a call to this method will not trigger an unnecessary detection - * attempt. + * Returns whether Bisq should use a local Bitcoin node, meaning that a usable node + * was detected and conditions under which it should be ignored have not been met. If + * the local node should be ignored, a call to this method will not trigger an + * unnecessary detection attempt. */ - public boolean willUse() { - return !willIgnore() && isUsable(); + public boolean shouldBeUsed() { + return !shouldBeIgnored() && isUsable(); } /** * Returns whether Bisq will ignore a local Bitcoin node even if it is usable. */ - public boolean willIgnore() { + public boolean shouldBeIgnored() { BaseCurrencyNetwork baseCurrencyNetwork = config.baseCurrencyNetwork; - // For dao testnet (server side regtest) we disable the use of local bitcoin node to - // avoid confusion if local btc node is not synced with our dao testnet master node. - // Note: above comment was previously in WalletConfig::createPeerGroup. + // For dao testnet (server side regtest) we disable the use of local bitcoin node + // to avoid confusion if local btc node is not synced with our dao testnet master + // node. Note: above comment was previously in WalletConfig::createPeerGroup. return config.ignoreLocalBtcNode || baseCurrencyNetwork.isDaoRegTest() || baseCurrencyNetwork.isDaoTestNet(); } /** - * Returns whether or not a local Bitcion node was detected and was well configured + * Returns whether or not a local Bitcoin node was detected and was well-configured * at the time the checks were performed. All checks are triggered in case they have * not been performed. */ - public boolean isUsable() { + private boolean isUsable() { // If a node is found to be well configured, it implies that it was also detected, // so this is query is enough to show if the relevant checks were performed and if // their results are positive. @@ -102,8 +102,7 @@ public boolean isUsable() { } /** - * Returns whether the local node was detected, but misconfigured. Combination of - * methods isDetected and isWellConfigured. + * Returns whether a local node was detected but misconfigured. */ public boolean isDetectedButMisconfigured() { return isDetected() && !isWellConfigured(); @@ -116,7 +115,7 @@ public boolean isDetectedButMisconfigured() { * value. See {@code MainViewModel#setupBtcNumPeersWatcher} to understand how * disconnection and reconnection of the local Bitcoin node is actually handled. */ - public boolean isDetected() { + private boolean isDetected() { if (detected == null) { performChecks(); } @@ -128,7 +127,7 @@ public boolean isDetected() { * they were performed. All checks are triggered in case they have not been performed. * We check if the local node is not pruning and has bloom filters enabled. */ - public boolean isWellConfigured() { + private boolean isWellConfigured() { if (wellConfigured == null) { performChecks(); } @@ -144,7 +143,7 @@ private void performChecks() { /** * Initiates detection and configuration checks. The results are cached so that the - * public methods isUsable, isDetected, etc. don't trigger a recheck. + * {@link #isUsable()}, {@link #isDetected()} et al don't trigger a recheck. */ private void checkUsable() { var optionalVersionMessage = attemptHandshakeForVersionMessage(); @@ -183,12 +182,11 @@ private static boolean checkWellConfigured(VersionMessage versionMessage) { /** * 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: + * 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 + * and core/src/main/java/org/bitcoinj/core/NetworkParameters.java */ + @SuppressWarnings("UnnecessaryLocalVariable") 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. diff --git a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java index 1aa79bcb70d..5c105f9ed6e 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletConfig.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletConfig.java @@ -239,7 +239,7 @@ private PeerGroup createPeerGroup() { peerGroup.setConnectTimeoutMillis(TOR_VERSION_EXCHANGE_TIMEOUT); } - if (!localBitcoinNode.willUse()) + if (!localBitcoinNode.shouldBeUsed()) peerGroup.setUseLocalhostPeerWhenPossible(false); return peerGroup; diff --git a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java index acc14699746..722799f451a 100644 --- a/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java +++ b/core/src/main/java/bisq/core/btc/setup/WalletsSetup.java @@ -278,7 +278,7 @@ protected void onSetupCompleted() { return; } } - } else if (localBitcoinNode.willUse()) { + } else if (localBitcoinNode.shouldBeUsed()) { walletConfig.setMinBroadcastConnections(1); walletConfig.setPeerNodesForLocalHost(); } else { diff --git a/core/src/main/java/bisq/core/user/Preferences.java b/core/src/main/java/bisq/core/user/Preferences.java index f353c22d076..077d221bc6c 100644 --- a/core/src/main/java/bisq/core/user/Preferences.java +++ b/core/src/main/java/bisq/core/user/Preferences.java @@ -742,7 +742,7 @@ public boolean getUseTorForBitcoinJ() { // nodes and we don't provide tor nodes. if ((!Config.baseCurrencyNetwork().isMainnet() - || localBitcoinNode.willUse()) + || localBitcoinNode.shouldBeUsed()) && !config.useTorForBtcOptionSetExplicitly) return false; else diff --git a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java index ad6870ef12e..0766f96c43c 100644 --- a/desktop/src/main/java/bisq/desktop/main/MainViewModel.java +++ b/desktop/src/main/java/bisq/desktop/main/MainViewModel.java @@ -451,7 +451,7 @@ private void setupBtcNumPeersWatcher() { checkNumberOfBtcPeersTimer = UserThread.runAfter(() -> { // check again numPeers if (walletsSetup.numPeersProperty().get() == 0) { - if (localBitcoinNode.willUse()) + if (localBitcoinNode.shouldBeUsed()) getWalletServiceErrorMsg().set( Res.get("mainView.networkWarning.localhostBitcoinLost", Res.getBaseCurrencyName().toLowerCase())); diff --git a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java index 5482f4d0dde..416edc6a02d 100644 --- a/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java +++ b/desktop/src/main/java/bisq/desktop/main/settings/network/NetworkSettingsView.java @@ -165,7 +165,7 @@ public void initialize() { bitcoinPeerSubVersionColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.subVersionColumn"))); bitcoinPeerHeightColumn.setGraphic(new AutoTooltipLabel(Res.get("settings.net.heightColumn"))); localhostBtcNodeInfoLabel.setText(Res.get("settings.net.localhostBtcNodeInfo")); - if (!localBitcoinNode.willUse()) { + if (localBitcoinNode.shouldBeIgnored()) { localhostBtcNodeInfoLabel.setVisible(false); } useProvidedNodesRadio.setText(Res.get("settings.net.useProvidedNodesRadio")); @@ -380,14 +380,14 @@ private void showShutDownPopup() { } private void onBitcoinPeersToggleSelected(boolean calledFromUser) { - boolean bitcoinLocalhostNodeBeingUsed = localBitcoinNode.willUse(); - useTorForBtcJCheckBox.setDisable(bitcoinLocalhostNodeBeingUsed); - bitcoinNodesLabel.setDisable(bitcoinLocalhostNodeBeingUsed); - btcNodesLabel.setDisable(bitcoinLocalhostNodeBeingUsed); - btcNodesInputTextField.setDisable(bitcoinLocalhostNodeBeingUsed); - useProvidedNodesRadio.setDisable(bitcoinLocalhostNodeBeingUsed || !btcNodes.useProvidedBtcNodes()); - useCustomNodesRadio.setDisable(bitcoinLocalhostNodeBeingUsed); - usePublicNodesRadio.setDisable(bitcoinLocalhostNodeBeingUsed || isPreventPublicBtcNetwork()); + boolean localBitcoinNodeShouldBeUsed = localBitcoinNode.shouldBeUsed(); + useTorForBtcJCheckBox.setDisable(localBitcoinNodeShouldBeUsed); + bitcoinNodesLabel.setDisable(localBitcoinNodeShouldBeUsed); + btcNodesLabel.setDisable(localBitcoinNodeShouldBeUsed); + btcNodesInputTextField.setDisable(localBitcoinNodeShouldBeUsed); + useProvidedNodesRadio.setDisable(localBitcoinNodeShouldBeUsed || !btcNodes.useProvidedBtcNodes()); + useCustomNodesRadio.setDisable(localBitcoinNodeShouldBeUsed); + usePublicNodesRadio.setDisable(localBitcoinNodeShouldBeUsed || isPreventPublicBtcNetwork()); BtcNodes.BitcoinNodesOption currentBitcoinNodesOption = BtcNodes.BitcoinNodesOption.values()[preferences.getBitcoinNodesOptionOrdinal()]; @@ -454,7 +454,7 @@ private void onBitcoinPeersToggleSelected(boolean calledFromUser) { private void applyPreventPublicBtcNetwork() { final boolean preventPublicBtcNetwork = isPreventPublicBtcNetwork(); - usePublicNodesRadio.setDisable(localBitcoinNode.willUse() || preventPublicBtcNetwork); + usePublicNodesRadio.setDisable(localBitcoinNode.shouldBeUsed() || preventPublicBtcNetwork); if (preventPublicBtcNetwork && selectedBitcoinNodesOption == BtcNodes.BitcoinNodesOption.PUBLIC) { selectedBitcoinNodesOption = BtcNodes.BitcoinNodesOption.PROVIDED; preferences.setBitcoinNodesOptionOrdinal(selectedBitcoinNodesOption.ordinal()); From 57b7041dfefba2e0c370468e150ca6e042df35c4 Mon Sep 17 00:00:00 2001 From: Chris Beams Date: Thu, 27 Feb 2020 09:53:40 +0100 Subject: [PATCH 19/20] Remove unnecessary LOCAL_BITCOIN_NODE_PORT constant 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. --- core/src/main/java/bisq/core/app/CoreModule.java | 5 ----- .../main/java/bisq/core/app/misc/ModuleForAppWithP2p.java | 3 --- .../main/java/bisq/core/btc/nodes/LocalBitcoinNode.java | 7 ++----- core/src/test/java/bisq/core/user/PreferencesTest.java | 2 +- 4 files changed, 3 insertions(+), 14 deletions(-) diff --git a/core/src/main/java/bisq/core/app/CoreModule.java b/core/src/main/java/bisq/core/app/CoreModule.java index 33e94eebd07..1f058e123a7 100644 --- a/core/src/main/java/bisq/core/app/CoreModule.java +++ b/core/src/main/java/bisq/core/app/CoreModule.java @@ -47,7 +47,6 @@ import java.io.File; import static bisq.common.config.Config.*; -import static bisq.core.btc.nodes.LocalBitcoinNode.LOCAL_BITCOIN_NODE_PORT; import static com.google.inject.name.Names.named; public class CoreModule extends AppModule { @@ -78,10 +77,6 @@ protected void configure() { bindConstant().annotatedWith(named(USE_DEV_MODE)).to(config.useDevMode); bindConstant().annotatedWith(named(REFERRAL_ID)).to(config.referralId); - bindConstant().annotatedWith(named(LOCAL_BITCOIN_NODE_PORT)) - .to(config.baseCurrencyNetworkParameters.getPort()); - - // ordering is used for shut down sequence install(new TradeModule(config)); install(new EncryptionServiceModule(config)); diff --git a/core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java b/core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java index f95d8548e52..86e49bbee22 100644 --- a/core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java +++ b/core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java @@ -82,9 +82,6 @@ protected void configure() { bindConstant().annotatedWith(named(USE_DEV_MODE)).to(config.useDevMode); bindConstant().annotatedWith(named(REFERRAL_ID)).to(config.referralId); - bindConstant().annotatedWith(named(LOCAL_BITCOIN_NODE_PORT)) - .to(config.baseCurrencyNetworkParameters.getPort()); - // ordering is used for shut down sequence install(new TradeModule(config)); install(new EncryptionServiceModule(config)); diff --git a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java index 414d1d2c20b..fcb250a07ae 100644 --- a/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java +++ b/core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java @@ -12,7 +12,6 @@ import org.bitcoinj.net.NioClientManager; import javax.inject.Inject; -import javax.inject.Named; import javax.inject.Singleton; import com.google.common.util.concurrent.FutureCallback; @@ -48,8 +47,6 @@ @Singleton public class LocalBitcoinNode { - public static final String LOCAL_BITCOIN_NODE_PORT = "localBitcoinNodePort"; - private static final Logger log = LoggerFactory.getLogger(LocalBitcoinNode.class); private static final int CONNECTION_TIMEOUT = 5000; @@ -60,9 +57,9 @@ public class LocalBitcoinNode { private Boolean wellConfigured; @Inject - public LocalBitcoinNode(Config config, @Named(LOCAL_BITCOIN_NODE_PORT) int port) { + public LocalBitcoinNode(Config config) { this.config = config; - this.port = port; + this.port = config.baseCurrencyNetworkParameters.getPort(); } /** diff --git a/core/src/test/java/bisq/core/user/PreferencesTest.java b/core/src/test/java/bisq/core/user/PreferencesTest.java index fcde9455f59..b29ea9520c1 100644 --- a/core/src/test/java/bisq/core/user/PreferencesTest.java +++ b/core/src/test/java/bisq/core/user/PreferencesTest.java @@ -60,7 +60,7 @@ public void setUp() { storage = mock(Storage.class); Config config = new Config(); - LocalBitcoinNode localBitcoinNode = new LocalBitcoinNode(config, config.baseCurrencyNetworkParameters.getPort()); + LocalBitcoinNode localBitcoinNode = new LocalBitcoinNode(config); preferences = new Preferences( storage, config, localBitcoinNode, null, null, Config.DEFAULT_FULL_DAO_NODE, null, null, Config.UNSPECIFIED_PORT); From 85e4515f53116c0fbec27e4ecee0eaaf9313ef4c Mon Sep 17 00:00:00 2001 From: Dominykas Mostauskis Date: Thu, 27 Feb 2020 12:14:32 +0100 Subject: [PATCH 20/20] Remove reference to removed constant --- core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java b/core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java index 86e49bbee22..aa76144c0df 100644 --- a/core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java +++ b/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; import static com.google.inject.name.Names.named; public class ModuleForAppWithP2p extends AppModule {