Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add local Bitcoin node configuration detection #3982

Merged
merged 20 commits into from Feb 27, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 4 additions & 8 deletions core/src/main/java/bisq/core/app/BisqSetup.java
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
displayLocalNodeMisconfigurationHandler.accept(nextStep);
if (displayLocalNodeMisconfigurationHandler != null) {
displayLocalNodeMisconfigurationHandler.accept(nextStep);
}

Add a check to prevent possible NullPointerException.

return;
Expand Down Expand Up @@ -879,7 +875,7 @@ private void maybeShowSecurityRecommendation() {
}

private void maybeShowLocalhostRunningInfo() {
maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.safeIsUsable());
maybeTriggerDisplayHandler("bitcoinLocalhostNode", displayLocalhostHandler, localBitcoinNode.isUsable());
}

private void maybeShowAccountSigningStateInfo() {
Expand Down
122 changes: 45 additions & 77 deletions core/src/main/java/bisq/core/btc/nodes/LocalBitcoinNode.java
Expand Up @@ -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<Boolean>, 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
Expand All @@ -54,8 +51,9 @@ public class LocalBitcoinNode {
private static final int CONNECTION_TIMEOUT = 5000;

private final int port;
private Optional<Boolean> detected = Optional.empty();
private Optional<Boolean> wellConfigured = Optional.empty();

private Boolean detected;
private Boolean wellConfigured;

@Inject
public LocalBitcoinNode(@Named(LOCAL_BITCOIN_NODE_PORT) int port) {
Expand Down Expand Up @@ -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<VersionMessage> 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"
dmos62 marked this conversation as resolved.
Show resolved Hide resolved
+ " (before a version messages could be coerced)",
dmos62 marked this conversation as resolved.
Show resolved Hide resolved
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);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blank line

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)");
}
Expand Down Expand Up @@ -293,82 +294,49 @@ 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<Boolean> 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.
return isWellConfigured();
}

/**
* Returns an Optional<Boolean> 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<Boolean> 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<Boolean> 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<Boolean> 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<Boolean> 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;
});
}
}
2 changes: 1 addition & 1 deletion core/src/main/java/bisq/core/btc/setup/WalletConfig.java
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/bisq/core/btc/setup/WalletsSetup.java
Expand Up @@ -278,7 +278,7 @@ protected void onSetupCompleted() {
return;
}
}
} else if (localBitcoinNode.safeIsUsable()) {
} else if (localBitcoinNode.isUsable()) {
walletConfig.setMinBroadcastConnections(1);
walletConfig.setPeerNodesForLocalHost();
} else {
Expand Down
10 changes: 1 addition & 9 deletions core/src/main/java/bisq/core/user/Preferences.java
Expand Up @@ -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<Boolean> 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
Expand Down
2 changes: 1 addition & 1 deletion desktop/src/main/java/bisq/desktop/main/MainViewModel.java
Expand Up @@ -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
Expand Down
Expand Up @@ -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"));
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());
Expand Down