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.

Sign up for GitHub

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

Jump to bottom

Add local Bitcoin node configuration detection #3982

Merged
merged 20 commits into from
Feb 27, 2020
Merged

Add local Bitcoin node configuration detection #3982

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
f895da4
Add local Bitcoin node configuration detection
dmos62 Feb 13, 2020
18478d9
Downgrade Optional usage to Java 10
dmos62 Feb 18, 2020
74c946a
Remove defunct test suite
dmos62 Feb 18, 2020
daa1b0b
Minor changes to satisfy Codacy or clarify why it fails
dmos62 Feb 18, 2020
e6dea3d
Improve marking that method is empty
dmos62 Feb 18, 2020
65177fc
Fix unchecked usage of LocalBitcoinNode.isUsable()
dmos62 Feb 21, 2020
08cd31b
Silence NioClient and NioClientManager loggers
dmos62 Feb 21, 2020
7848836
Formating changes
dmos62 Feb 21, 2020
0bbbe8c
Perform checks automatically on first query
dmos62 Feb 25, 2020
aceb608
Reorder methods
dmos62 Feb 25, 2020
6b4878a
Centralize some of local BTC node logic
dmos62 Feb 25, 2020
2a57ecd
Fix failing test
dmos62 Feb 25, 2020
6dec780
Minor requested changes (github batch)
dmos62 Feb 26, 2020
a92b6ad
Minor requested changes (non-github batch)
dmos62 Feb 26, 2020
30578bf
Have detection work on other network modes
dmos62 Feb 26, 2020
fdaced4
Changes to Background information popup
dmos62 Feb 26, 2020
b93ca2b
Polish formatting
cbeams Feb 26, 2020
c1a99cc
Polish LocalBitcoinNode method names and visibility
cbeams Feb 27, 2020
57b7041
Remove unnecessary LOCAL_BITCOIN_NODE_PORT constant
cbeams Feb 27, 2020
85e4515
Remove reference to removed constant
dmos62 Feb 27, 2020
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
28 changes: 19 additions & 9 deletions core/src/main/java/bisq/core/app/BisqSetup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -193,7 +192,7 @@ default void onRequestWalletPassword() {

@Setter
@Nullable
private Consumer<Runnable> displayTacHandler;
private Consumer<Runnable> displayTacHandler, displayLocalNodeMisconfigurationHandler;
@Setter
@Nullable
private Consumer<String> cryptoSetupFailedHandler, chainFileLockedExceptionHandler,
Expand Down Expand Up @@ -348,7 +347,7 @@ public void start() {
}

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

private void step3() {
Expand Down Expand Up @@ -482,14 +481,24 @@ private void maybeShowTac() {
}
}

private void detectLocalBitcoinNode(Runnable nextStep) {
BaseCurrencyNetwork baseCurrencyNetwork = config.baseCurrencyNetwork;
if (config.ignoreLocalBtcNode || baseCurrencyNetwork.isDaoRegTest() || baseCurrencyNetwork.isDaoTestNet()) {
private void maybeCheckLocalBitcoinNode(Runnable nextStep) {
if (localBitcoinNode.shouldBeIgnored()) {
nextStep.run();
return;
}

localBitcoinNode.detectAndRun(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.
if (localBitcoinNode.isDetectedButMisconfigured()) {
if (displayLocalNodeMisconfigurationHandler != null) {
displayLocalNodeMisconfigurationHandler.accept(nextStep);
return;
} else {
log.error("displayLocalNodeMisconfigurationHandler undefined", new RuntimeException());
}
}

nextStep.run();
}

private void readMapsFromResources(Runnable nextStep) {
Expand Down Expand Up @@ -559,7 +568,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() || localBitcoinNode.isDetected()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@dmos62 dmos62 Feb 26, 2020 •
edited

Choose a reason for hiding this comment

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

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

// TODO encapsulate below conditional inside getUseTorForBitcoinJ
if (!preferences.getUseTorForBitcoinJ() || localBitcoinNode.shouldBeUsed()) {
initWallet();
}

Expand Down Expand Up @@ -862,7 +872,7 @@ private void maybeShowSecurityRecommendation() {
}

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

private void maybeShowAccountSigningStateInfo() {
Expand Down
5 changes: 0 additions & 5 deletions core/src/main/java/bisq/core/app/CoreModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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));
Expand Down
4 changes: 0 additions & 4 deletions core/src/main/java/bisq/core/app/misc/ModuleForAppWithP2p.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

Good catch, thanks @dmos62.

import static com.google.inject.name.Names.named;

public class ModuleForAppWithP2p extends AppModule {
Expand Down Expand Up @@ -82,9 +81,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));
Expand Down