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

Check BTC node config without IO overhead #4081

Closed
wants to merge 2 commits into from
Closed

Check BTC node config without IO overhead #4081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5811f33
Setup Bisq to use the locally installed BitcoinJ
dmos62 Feb 2, 2021
c1203c9
Use the new VersionMessage receival hook to check BTC node configuration
dmos62 Feb 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ configure(subprojects) {

ext { // in alphabetical order
bcVersion = '1.63'
bitcoinjVersion = '2a80db4'
bitcoinjVersion = '0.15.8.bisq.14'
btcdCli4jVersion = '27b94333'
codecVersion = '1.13'
easybindVersion = '1.0.3'
Expand Down Expand Up @@ -79,6 +79,7 @@ configure(subprojects) {
repositories {
mavenCentral()
maven { url 'https://jitpack.io' }
mavenLocal()
}

dependencies {
Expand Down Expand Up @@ -212,7 +213,7 @@ configure(project(':proto')) {

configure(project(':assets')) {
dependencies {
compile("com.github.bisq-network:bitcoinj:$bitcoinjVersion") {
compile("org.bitcoinj:bitcoinj-core:$bitcoinjVersion") {
exclude(module: 'jsr305')
exclude(module: 'slf4j-api')
exclude(module: 'guava')
Expand Down Expand Up @@ -245,7 +246,7 @@ configure(project(':common')) {
compile("com.google.inject:guice:$guiceVersion") {
exclude(module: 'guava')
}
compile("com.github.bisq-network:bitcoinj:$bitcoinjVersion") {
compile("org.bitcoinj:bitcoinj-core:$bitcoinjVersion") {
exclude(module: 'jsr305')
exclude(module: 'slf4j-api')
exclude(module: 'guava')
Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/bisq/core/app/WalletAppSetup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ void init(@Nullable Consumer<String> chainFileLockedExceptionHandler,
Runnable downloadCompleteHandler,
Runnable walletInitializedHandler) {
log.info("Initialize WalletAppSetup with BitcoinJ version {} and hash of BitcoinJ commit {}",
VersionMessage.BITCOINJ_VERSION, "2a80db4");
VersionMessage.BITCOINJ_VERSION, "-"); // TODO what's the purpose of specifying the commit hash here?

ObjectProperty<Throwable> walletServiceException = new SimpleObjectProperty<>();
btcInfoBinding = EasyBind.combine(walletsSetup.downloadPercentageProperty(),
Expand Down
69 changes: 69 additions & 0 deletions core/src/main/java/bisq/core/btc/setup/WalletsSetup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.bitcoinj.core.NetworkParameters;
import org.bitcoinj.core.Peer;
import org.bitcoinj.core.PeerAddress;
import org.bitcoinj.core.VersionMessage;
import org.bitcoinj.core.PeerGroup;
import org.bitcoinj.core.RejectMessage;
import org.bitcoinj.core.listeners.DownloadProgressTracker;
Expand Down Expand Up @@ -91,10 +92,12 @@
import java.util.List;
import java.util.Set;
import java.util.concurrent.TimeUnit;
import java.util.function.Consumer;
import java.util.concurrent.TimeoutException;
import java.util.stream.Collectors;

import lombok.Getter;
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;

import org.jetbrains.annotations.NotNull;
Expand Down Expand Up @@ -141,6 +144,10 @@ public class WalletsSetup {
private final boolean useAllProvidedNodes;
private WalletConfig walletConfig;

@Setter
@Nullable
private Consumer<String> displayUserBtcNodeMisconfigurationHandler;

///////////////////////////////////////////////////////////////////////////////////////////
// Constructor
///////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -215,6 +222,10 @@ protected void onSetupCompleted() {
if (preferences.getBitcoinNodes() != null && !preferences.getBitcoinNodes().isEmpty())
peerGroup.setAddPeersFromAddressMessage(false);

peerGroup.addVersionMessageReceivedEventListener((peer, versionMessage) -> {
UserThread.execute(() -> handleConfiguration(peer, versionMessage));
});

peerGroup.addConnectedEventListener((peer, peerCount) -> {
// We get called here on our user thread
numPeers.set(peerCount);
Expand Down Expand Up @@ -409,6 +420,64 @@ private void configPeerNodes(@Nullable Socks5Proxy proxy) {
networkConfig.proposePeers(peers);
}

///////////////////////////////////////////////////////////////////////////////////////////
// Peer configuration check
///////////////////////////////////////////////////////////////////////////////////////////

/* Provided a Peer and its VersionMessage, will check and handle Peer's advertised
* configuration. The VersionMessage is provided separately, because it is not
* always available and this is meant to be called from a listener that is triggered
* when the VersionMessage becomes available.
*/
private void handleConfiguration(Peer peer, VersionMessage versionMessage) {
var wellConfigured = isWellConfigured(versionMessage);
if (wellConfigured) {
Comment on lines +433 to +434
Copy link

Choose a reason for hiding this comment

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

A temp variable is not necessary imo, the method call is already very concise and readable

Suggested change
var wellConfigured = isWellConfigured(versionMessage);
if (wellConfigured) {
if (isWellConfigured(versionMessage)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A variable defining the condition is a style I use (and propagate) and, even if the called method is descriptive as well, I still use it. One reason is consistency of style. Another, the if statement doesn't care that the node being well configured is predicated on the version message (that's an implemenation detail), so there's no reason to put that in the if statement. I would be against your suggestion, because it optimizes for terseness at the cost of readability.

log.info("Peer well configured: {}", peer);
} else {
Comment on lines +434 to +436
Copy link

Choose a reason for hiding this comment

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

Redundant else clause bc the if can return. Makes it more readable

Suggested change
if (wellConfigured) {
log.info("Peer well configured: {}", peer);
} else {
if (wellConfigured) {
log.info("Peer well configured: {}", peer);
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't remove the wellConfigured intermediary variable as you suggested above (which I am against), this originally reads "if the node is well configured log that, else perform following steps", which is favourable over your suggetion: "if the node is well configured, given provided version message, exit the method, foregoing steps that would only be executed in case the node were not well configured." Your edits have a negative effect on readability and a very minimal effect on terseness, which, in my opinion, should not be a consideration anyway (but that's besides the point).

log.error("Peer _badly_ configured (pruning and/or bloom filters disabled): {}", peer);
var usingLocalNode = localBitcoinNode.shouldBeUsed();
var usingCustomNodes =
BtcNodes.BitcoinNodesOption.CUSTOM.ordinal() == preferences.getBitcoinNodesOptionOrdinal();
if (usingLocalNode || usingCustomNodes) {
displayUserBtcNodeMisconfigurationHandler.accept(peer.toString());
}
}
}

private static boolean isWellConfigured(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
*/
@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.
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;
}

///////////////////////////////////////////////////////////////////////////////////////////
// Backup
Expand Down
1 change: 1 addition & 0 deletions core/src/main/resources/i18n/displayStrings.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2786,6 +2786,7 @@ Please check if you have the latest version of Bisq installed.\n\
You can download it at: [HYPERLINK:https://bisq.network/downloads].\n\n\
Please restart the application.
popup.warning.startupFailed.twoInstances=Bisq is already running. You cannot run two instances of Bisq.
popup.warning.userBtcNodeMisconfigured.explanation=The user-provided Bitcoin node {0} was found to be misconfigured. A user-provided BTC node is either a locally running node that was automatically detected and used, or a node explicitly specified via the --btcNodes command-line option. Bisq requires BTC nodes to have bloom filters enabled and pruning disabled. Automatic use of a local Bitcoin node can be disabled by using the --ignoreLocalBtcNode option.
popup.warning.tradePeriod.halfReached=Your trade with ID {0} has reached the half of the max. allowed trading period and is still not completed.\n\nThe trade period ends on {1}\n\nPlease check your trade state at \"Portfolio/Open trades\" for further information.
popup.warning.tradePeriod.ended=Your trade with ID {0} has reached the max. allowed trading period and is not completed.\n\n\
The trade period ended on {1}\n\n\
Expand Down
9 changes: 9 additions & 0 deletions desktop/src/main/java/bisq/desktop/main/MainViewModel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -474,6 +474,15 @@ public void onUpdatedDataReceived() {
}
});
}

walletsSetup.setDisplayUserBtcNodeMisconfigurationHandler(
(String peer) ->
new Popup()
.hideCloseButton()
.warning(Res.get("popup.warning.userBtcNodeMisconfigured.explanation", peer))
.useShutDownButton()
.show()
);
}

private void showRevolutAccountUpdateWindow(List<RevolutAccount> revolutAccountList) {
Expand Down
2 changes: 1 addition & 1 deletion gradle/witness/gradle-witness.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ dependencyVerification {
'com.github.bisq-network.netlayer:tor.external:a3606a592d6b6caa6a2fb7db224eaf43c6874c6730da4815bd37ad686b283dcb',
'com.github.bisq-network.netlayer:tor.native:b15aba7fe987185037791c7ec7c529cb001b90d723d047d54aab87aceb3b3d45',
'com.github.bisq-network.netlayer:tor:a974190aa3a031067ccd1dda28a3ae58cad14060792299d86ea38a05fb21afc5',
'com.github.bisq-network:bitcoinj:65ed08fa5777ea4a08599bdd575e7dc1f4ba2d4d5835472551439d6f6252e68a',
//'com.github.bisq-network:bitcoinj:65ed08fa5777ea4a08599bdd575e7dc1f4ba2d4d5835472551439d6f6252e68a',
'com.github.cd2357.tor-binary:tor-binary-geoip:ae27b6aca1a3a50a046eb11e38202b6d21c2fcd2b8643bbeb5ea85e065fbc1be',
'com.github.cd2357.tor-binary:tor-binary-linux32:7b5d6770aa442ef6d235e8a9bfbaa7c62560690f9fe69ff03c7a752eae84f7dc',
'com.github.cd2357.tor-binary:tor-binary-linux64:24111fa35027599a750b0176392dc1e9417d919414396d1b221ac2e707eaba76',
Expand Down