Skip to content

Commit

Permalink
Introduce LocalBitcoinNode and tests
Browse files Browse the repository at this point in the history
This new class encapsulates all functionality related to detecting a
local Bitcoin node and reporting whether or not it was detected.
Previously this functionality was spread across the Config class
(formerly BisqEnvironment) with its mutable static
isLocalBitcoinNodeRunning property and the BisqSetup class with its
checkIfLocalHostNodeIsRunning method. All of this functionality now
lives within the LocalBitcoinNode class, an instance of which is wired
up via Guice and injected wherever necessary.

Note that the code for detecting whether the node is running has been
simplified, in that it is no longer wrapped in its own dedicated Thread.
There appears to be no performance benefit from doing so, and leaving it
in place would have made testing more difficult than necessary.

Several methods in BisqSetup have also been refactored to accept
callbacks indicating which step should be run next. This has the effect
of clarifying when the step2()-step5() methods will be called.
  • Loading branch information
cbeams committed Jan 10, 2020
1 parent 5014377 commit 30bef16
Show file tree
Hide file tree
Showing 13 changed files with 181 additions and 58 deletions.
11 changes: 0 additions & 11 deletions common/src/main/java/bisq/common/config/Config.java
Expand Up @@ -168,9 +168,6 @@ public class Config {

private final OptionParser parser = new OptionParser();

// legacy mutable property TODO: move to new LocalBitcoinNode class
private boolean localBitcoinNodeIsRunning = false;

public Config(String... args) {
this(tempAppName(), tempUserDataDir(), args);
}
Expand Down Expand Up @@ -854,14 +851,6 @@ public String getLogLevel() {
return logLevel;
}

public boolean isLocalBitcoinNodeIsRunning() {
return localBitcoinNodeIsRunning;
}

public void setLocalBitcoinNodeIsRunning(boolean value) {
this.localBitcoinNodeIsRunning = value;
}

public String getReferralId() {
return referralId;
}
Expand Down
4 changes: 4 additions & 0 deletions core/src/main/java/bisq/core/CoreModule.java
Expand Up @@ -47,6 +47,7 @@
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 {
Expand Down Expand Up @@ -77,6 +78,9 @@ protected void configure() {
bindConstant().annotatedWith(named(USE_DEV_MODE)).to(config.isUseDevMode());
bindConstant().annotatedWith(named(REFERRAL_ID)).to(config.getReferralId());

bindConstant().annotatedWith(named(LOCAL_BITCOIN_NODE_PORT))
.to(config.getBaseCurrencyNetworkParameters().getPort());


// ordering is used for shut down sequence
install(new TradeModule(config));
Expand Down
57 changes: 22 additions & 35 deletions core/src/main/java/bisq/core/app/BisqSetup.java
Expand Up @@ -26,6 +26,7 @@
import bisq.core.alert.PrivateNotificationPayload;
import bisq.core.btc.Balances;
import bisq.core.btc.model.AddressEntry;
import bisq.core.btc.nodes.LocalBitcoinNode;
import bisq.core.btc.setup.WalletsSetup;
import bisq.core.btc.wallet.BtcWalletService;
import bisq.core.btc.wallet.WalletsManager;
Expand Down Expand Up @@ -90,8 +91,6 @@
import javax.inject.Named;
import javax.inject.Singleton;

import com.google.common.net.InetAddresses;

import org.fxmisc.easybind.EasyBind;
import org.fxmisc.easybind.monadic.MonadicBinding;

Expand All @@ -106,9 +105,6 @@

import org.spongycastle.crypto.params.KeyParameter;

import java.net.InetSocketAddress;
import java.net.Socket;

import java.io.IOException;

import java.util.ArrayList;
Expand All @@ -130,6 +126,7 @@
@Slf4j
@Singleton
public class BisqSetup {

public interface BisqSetupListener {
default void onInitP2pNetwork() {
log.info("onInitP2pNetwork");
Expand Down Expand Up @@ -192,6 +189,8 @@ default void onRequestWalletPassword() {
private final TorSetup torSetup;
private final TradeLimits tradeLimits;
private final CoinFormatter formatter;
private final LocalBitcoinNode localBitcoinNode;

@Setter
@Nullable
private Consumer<Runnable> displayTacHandler;
Expand Down Expand Up @@ -278,8 +277,8 @@ public BisqSetup(P2PNetworkSetup p2PNetworkSetup,
AssetService assetService,
TorSetup torSetup,
TradeLimits tradeLimits,
@Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormatter formatter) {

@Named(FormattingUtils.BTC_FORMATTER_KEY) CoinFormatter formatter,
LocalBitcoinNode localBitcoinNode) {

this.p2PNetworkSetup = p2PNetworkSetup;
this.walletAppSetup = walletAppSetup;
Expand Down Expand Up @@ -326,6 +325,7 @@ public BisqSetup(P2PNetworkSetup p2PNetworkSetup,
this.torSetup = torSetup;
this.tradeLimits = tradeLimits;
this.formatter = formatter;
this.localBitcoinNode = localBitcoinNode;
}


Expand All @@ -345,18 +345,18 @@ public void start() {
}

private void step2() {
checkIfLocalHostNodeIsRunning();
detectLocalBitcoinNode(this::step3);
}

private void step3() {
torSetup.cleanupTorFiles();
readMapsFromResources();
readMapsFromResources(this::step4);
checkCryptoSetup();
checkForCorrectOSArchitecture();
}

private void step4() {
startP2pNetworkAndWallet();
startP2pNetworkAndWallet(this::step5);
}

private void step5() {
Expand Down Expand Up @@ -479,33 +479,20 @@ private void maybeShowTac() {
}
}

private void checkIfLocalHostNodeIsRunning() {
private void detectLocalBitcoinNode(Runnable nextStep) {
BaseCurrencyNetwork baseCurrencyNetwork = config.getBaseCurrencyNetwork();
// For DAO testnet we ignore local btc node
if (baseCurrencyNetwork.isDaoRegTest() || baseCurrencyNetwork.isDaoTestNet() ||
config.isIgnoreLocalBtcNode()) {
step3();
} else {
new Thread(() -> {
try (Socket socket = new Socket()) {
socket.connect(new InetSocketAddress(InetAddresses.forString("127.0.0.1"),
baseCurrencyNetwork.getParameters().getPort()), 5000);
log.info("Localhost Bitcoin node detected.");
UserThread.execute(() -> {
config.setLocalBitcoinNodeIsRunning(true);
step3();
});
} catch (Throwable e) {
UserThread.execute(BisqSetup.this::step3);
}
}, "checkIfLocalHostNodeIsRunningThread").start();
if (config.isIgnoreLocalBtcNode() || baseCurrencyNetwork.isDaoRegTest() || baseCurrencyNetwork.isDaoTestNet()) {
nextStep.run();
return;
}

localBitcoinNode.detectAndRun(nextStep);

This comment has been minimized.

Copy link
@ripcurlx

ripcurlx Jan 20, 2020

Member

@ManfredKarrer Just to be on the safe side. Was there a reason why you did the node detection in a separate thread?

}

private void readMapsFromResources() {
private void readMapsFromResources(Runnable nextStep) {
SetupUtils.readFromResources(p2PService.getP2PDataStorage(), config).addListener((observable, oldValue, newValue) -> {
if (newValue)
step4();
nextStep.run();
});
}

Expand Down Expand Up @@ -539,7 +526,7 @@ private void checkCryptoSetup() {
}, "checkCryptoThread").start();
}

private void startP2pNetworkAndWallet() {
private void startP2pNetworkAndWallet(Runnable nextStep) {
ChangeListener<Boolean> walletInitializedListener = (observable, oldValue, newValue) -> {
// TODO that seems to be called too often if Tor takes longer to start up...
if (newValue && !p2pNetworkReady.get() && displayTorNetworkSettingsHandler != null)
Expand Down Expand Up @@ -569,7 +556,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() || config.isLocalBitcoinNodeIsRunning()) {
if (!preferences.getUseTorForBitcoinJ() || localBitcoinNode.isDetected()) {
initWallet();
}

Expand All @@ -585,7 +572,7 @@ else if (displayTorNetworkSettingsHandler != null)
walletInitialized.removeListener(walletInitializedListener);
if (displayTorNetworkSettingsHandler != null)
displayTorNetworkSettingsHandler.accept(false);
step5();
nextStep.run();
}
});
}
Expand Down Expand Up @@ -871,7 +858,7 @@ private void maybeShowSecurityRecommendation() {
}

private void maybeShowLocalhostRunningInfo() {
maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, config.isLocalBitcoinNodeIsRunning());
maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.isDetected());
}

private void maybeShowAccountSigningStateInfo() {
Expand Down
Expand Up @@ -50,6 +50,7 @@
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 {
Expand Down Expand Up @@ -81,6 +82,9 @@ protected void configure() {
bindConstant().annotatedWith(named(USE_DEV_MODE)).to(config.isUseDevMode());
bindConstant().annotatedWith(named(REFERRAL_ID)).to(config.getReferralId());

bindConstant().annotatedWith(named(LOCAL_BITCOIN_NODE_PORT))
.to(config.getBaseCurrencyNetworkParameters().getPort());

// ordering is used for shut down sequence
install(new TradeModule(config));
install(new EncryptionServiceModule(config));
Expand Down
61 changes: 61 additions & 0 deletions core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java
@@ -0,0 +1,61 @@
package bisq.core.btc.nodes;

import javax.inject.Inject;
import javax.inject.Named;

import java.net.InetSocketAddress;
import java.net.Socket;

import java.io.IOException;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Detects whether a Bitcoin node is running on localhost.
* @see bisq.common.config.Config#isIgnoreLocalBtcNode()
*/
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;

private final int port;
private boolean detected = false;

@Inject
public LocalBitcoinNode(@Named(LOCAL_BITCOIN_NODE_PORT) int port) {
this.port = 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
*/
public void detectAndRun(Runnable callback) {
try (Socket socket = new Socket()) {
socket.connect(new InetSocketAddress("127.0.0.1", port), CONNECTION_TIMEOUT);
log.info("Local Bitcoin node detected on port {}", port);
detected = true;
} catch (IOException ex) {
log.info("No local Bitcoin node detected on port {}.", port);
}
callback.run();
}

/**
* 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.
*/
public boolean isDetected() {
return detected;
}
}
6 changes: 5 additions & 1 deletion core/src/main/java/bisq/core/btc/setup/WalletConfig.java
Expand Up @@ -17,6 +17,7 @@

package bisq.core.btc.setup;

import bisq.core.btc.nodes.LocalBitcoinNode;
import bisq.core.btc.nodes.ProxySocketFactory;
import bisq.core.btc.wallet.BisqRiskAnalysis;

Expand Down Expand Up @@ -115,6 +116,7 @@ public interface BisqWalletFactory extends WalletProtobufSerializer.WalletFactor
private final Socks5Proxy socks5Proxy;
private final BisqWalletFactory walletFactory;
private final Config config;
private final LocalBitcoinNode localBitcoinNode;
private final String userAgent;
private int numConnectionsForBtc;

Expand Down Expand Up @@ -152,12 +154,14 @@ public WalletConfig(NetworkParameters params,
Socks5Proxy socks5Proxy,
File directory,
Config config,
LocalBitcoinNode localBitcoinNode,
String userAgent,
int numConnectionsForBtc,
@SuppressWarnings("SameParameterValue") String btcWalletFileName,
@SuppressWarnings("SameParameterValue") String bsqWalletFileName,
@SuppressWarnings("SameParameterValue") String spvChainFileName) {
this.config = config;
this.localBitcoinNode = localBitcoinNode;
this.userAgent = userAgent;
this.numConnectionsForBtc = numConnectionsForBtc;
this.context = new Context(params);
Expand Down Expand Up @@ -239,7 +243,7 @@ private PeerGroup createPeerGroup() {
// if local btc node is not synced with our dao testnet master node.
if (Config.baseCurrencyNetwork().isDaoRegTest() ||
Config.baseCurrencyNetwork().isDaoTestNet() ||
!config.isLocalBitcoinNodeIsRunning())
!localBitcoinNode.isDetected())
peerGroup.setUseLocalhostPeerWhenPossible(false);

return peerGroup;
Expand Down
7 changes: 6 additions & 1 deletion core/src/main/java/bisq/core/btc/setup/WalletsSetup.java
Expand Up @@ -17,6 +17,7 @@

package bisq.core.btc.setup;

import bisq.core.btc.nodes.LocalBitcoinNode;
import bisq.core.btc.exceptions.RejectedTxException;
import bisq.core.btc.model.AddressEntry;
import bisq.core.btc.model.AddressEntryList;
Expand Down Expand Up @@ -113,6 +114,7 @@ public class WalletsSetup {
private final Preferences preferences;
private final Socks5ProxyProvider socks5ProxyProvider;
private final Config config;
private final LocalBitcoinNode localBitcoinNode;
private final BtcNodes btcNodes;
private final String btcWalletFileName;
private final int numConnectionsForBtc;
Expand Down Expand Up @@ -140,6 +142,7 @@ public WalletsSetup(RegTestHost regTestHost,
Preferences preferences,
Socks5ProxyProvider socks5ProxyProvider,
Config config,
LocalBitcoinNode localBitcoinNode,
BtcNodes btcNodes,
@Named(Config.USER_AGENT) String userAgent,
@Named(Config.WALLET_DIR) File walletDir,
Expand All @@ -151,6 +154,7 @@ public WalletsSetup(RegTestHost regTestHost,
this.preferences = preferences;
this.socks5ProxyProvider = socks5ProxyProvider;
this.config = config;
this.localBitcoinNode = localBitcoinNode;
this.btcNodes = btcNodes;
this.numConnectionsForBtc = numConnectionsForBtc;
this.useAllProvidedNodes = useAllProvidedNodes;
Expand Down Expand Up @@ -191,6 +195,7 @@ public void initialize(@Nullable DeterministicSeed seed,
socks5Proxy,
walletDir,
config,
localBitcoinNode,
userAgent,
numConnectionsForBtc,
btcWalletFileName,
Expand Down Expand Up @@ -261,7 +266,7 @@ protected void onSetupCompleted() {
} else {
configPeerNodes(socks5Proxy);
}
} else if (config.isLocalBitcoinNodeIsRunning()) {
} else if (localBitcoinNode.isDetected()) {
walletConfig.setMinBroadcastConnections(1);
walletConfig.setPeerNodesForLocalHost();
} else {
Expand Down

0 comments on commit 30bef16

Please sign in to comment.