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

Check BTC node config without IO overhead #4081

Closed

Conversation

dmos62
Copy link
Contributor

@dmos62 dmos62 commented Mar 21, 2020

Edit: not proof of concept anymore; this initial post is outdated. TLDR is that this PR has Bisq check if nodes it connects to are suitable for use by Bisq (e.g. that the node isn't pruning). That's currently most useful with a local BTC node or custom nodes (because otherwise Bisq uses our own federated nodes that are known to be well configured), but it checks all BTC nodes it connects to, with negligible overhead. This PR is currently stagnating in large part because it depends on a (minor) BitcoinJ PR.

This is a proof of concept for checking BTC node's configuration. It's an alternative approach to what I had proposed earlier (that was merged and reverted due to reliability issues). In contrast to the previous attempt, this approach doesn't incur additional IO and doesn't use blocking. Whereas the previous attempt initiated a short lived BitcoinJ connection to a node, this approach just listens into the connections we are already establishing. Also, while previously the checking applied only to a local BTC node, this approach generalizes checking to all BTC nodes we connect to.

Currently, it has two problems:

a) connecting to a pruning node, on my setup at least, causes the connection to crash, and the listener never gets triggered; this is a rare BitcoinJ edge case bug and I opened an issue for it #4080 ;

and, b) this WIP implementation just does peer.close(), which causes the PeerGroup to just retry the connection, in case we're in local mode, or possibly retry later in case we're in non-local mode (didn't explore non-local behaviour yet); I'm pondering if this is indeed undesirable or if we would want to use this to get free auto-retry.

Another open question is what to do in case a remote node is found to be misconfigured. Just an error log?

@ripcurlx
Copy link
Contributor

@dmos62 Your build is broken right now: https://travis-ci.org/github/bisq-network/bisq/builds/665235608?utm_source=github_status&utm_medium=notification

a) connecting to a pruning node, on my setup at least, causes the connection to crash, and the listener never gets triggered; this is a rare BitcoinJ edge case bug and I opened an issue for it #4080 ;

Yes, I get following exception in the logs

März-23 15:46:57.444 [NioClientManager] WARN  o.b.core.PeerSocketHandler: [127.0.0.1]:18444 -  org.bitcoinj.core.ProtocolException: Received AlertMessage before version handshake is complete.
	at org.bitcoinj.core.Peer.processMessage(Peer.java:478)
	at org.bitcoinj.core.PeerSocketHandler.receiveBytes(PeerSocketHandler.java:184)
	at org.bitcoinj.net.ConnectionHandler.handleKey(ConnectionHandler.java:223)
	at org.bitcoinj.net.NioClientManager.handleKey(NioClientManager.java:86)
	at org.bitcoinj.net.NioClientManager.run(NioClientManager.java:122)
	at com.google.common.util.concurrent.AbstractExecutionThreadService$1$2.run(AbstractExecutionThreadService.java:66)
	at com.google.common.util.concurrent.Callables$4.run(Callables.java:122)
	at org.bitcoinj.utils.ContextPropagatingThreadFactory$1.run(ContextPropagatingThreadFactory.java:49)
	at java.base/java.lang.Thread.run(Thread.java:844)

and, b) this WIP implementation just does peer.close(), which causes the PeerGroup to just retry the connection, in case we're in local mode, or possibly retry later in case we're in non-local mode (didn't explore non-local behaviour yet); I'm pondering if this is indeed undesirable or if we would want to use this to get free auto-retry.

You mean a retry to the same peer?

Another open question is what to do in case a remote node is found to be misconfigured. Just an error log?

It depends. If it would be a misconfigured provided node/public node I would just ignore it and not connect to it. If it is a node the user has configured by using a config/start parameter I think we should display a warning to the user. Also if it is a misconfigured local node we want to notify the user that something is not correct.

In general if we do this check after the connection I think it should be fine if we block the setup process at that point (e.g. when a local bitcoin node is misconfigured) and do it as with your previous implementation to allow the user to shutdown and fix this issue.

@dmos62
Copy link
Contributor Author

dmos62 commented Mar 23, 2020

and, b) this WIP implementation just does peer.close(), which causes the PeerGroup to just retry the connection, in case we're in local mode, or possibly retry later in case we're in non-local mode (didn't explore non-local behaviour yet); I'm pondering if this is indeed undesirable or if we would want to use this to get free auto-retry.

You mean a retry to the same peer?

Yes. I didn't double-check, but I think BitcoinJ uses a FILO queue for keeping addresses of nodes. It pops and connects to them until wanted number of connections is established. In case we're in local-mode, then that queue has size 1 and contains localhost. peer.close() probably causes the node to go to the back of the queue.

block the setup process [when a local bitcoin node is misconfigured] and do it as with your previous implementation to allow the user to shutdown and fix this issue

I'd be in favor of letting an unclosable error popup be displayed asynchroniously. That would involve less fiddling with the setup process (less likely to break something).

@ripcurlx
Copy link
Contributor

I'd be in favor of letting an unclosable error popup be displayed asynchroniously. That would involve less fiddling with the setup process (less likely to break something).

I think this should be fine as well.

@dmos62 dmos62 changed the title [WIP] Check BTC node config without IO overhead Check BTC node config without IO overhead Mar 30, 2020
@dmos62
Copy link
Contributor Author

dmos62 commented Mar 30, 2020

This PR now depends on this PR on our BitcoinJ fork: bisq-network/bitcoinj#35. For development I used a BitcoinJ build from a local Maven repository. When that PR for BitcoinJ is merged, relevant changes to build.gradle should be made here.

Screenshot:

user provided node misconfigured

As seen in the screenshot, we're not providing a button to revert to not using a user-provided node (don't see how that can be done easily). The reason why before we could offer that option before is that we were blocking on the check and we were only handling the local bitcoin node.

It can also be seen that Bisq is actually loaded, and after you click "shutdown" the informational local BTC node popup will appear briefly before Bisq closes.

In the background, the misconfigured peers are not explicitly closed (BitcoinJ closes them automatically if they're pruning). The errors are logged and if custom BTC nodes are used, or a local BTC node, the above error popup locks the user out.

In other words, advantage over previous tries at getting this implemented, in terms of features, is that now we get errors when remote nodes are misconfigured, and the user is locked out if the misconfigured node is local or custom. Before we were only checking the local Bitcoin node.

@ripcurlx ripcurlx added this to the v1.3.0 milestone Apr 1, 2020
@ripcurlx ripcurlx modified the milestones: v1.3.2, v1.3.3 Apr 10, 2020
@stale
Copy link

stale bot commented May 10, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the was:dropped label May 10, 2020
@dmos62
Copy link
Contributor Author

dmos62 commented May 11, 2020

Bump

@stale stale bot removed the was:dropped label May 11, 2020
@ripcurlx ripcurlx modified the milestones: v1.3.4, v1.3.6 Jun 4, 2020
@stale
Copy link

stale bot commented Jul 4, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@chimp1984
Copy link
Contributor

@dmos62 Sorry that the PR got delayed so long... I asked @oscarguindzberg to review your BitcoinJ PR. If that is done I think we should get that PR in as well asap.

@@ -224,11 +228,12 @@ protected void onSetupCompleted() {
if (preferences.getBitcoinNodes() != null && !preferences.getBitcoinNodes().isEmpty())
peerGroup.setAddPeersFromAddressMessage(false);

peerGroup.addVersionMessageReceivedEventListener(WalletsSetup.this::handleConfiguration);
Copy link
Contributor

Choose a reason for hiding this comment

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

We get called from the BitcoinJ thread. We should map it to our userThread to avoid threading issues.

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

@@ -2527,6 +2527,7 @@ It will make debugging easier if you include the bisq.log file by pressing "Open

popup.error.tryRestart=Please try to restart your application and check your network connection to see if you can resolve the issue.
popup.error.takeOfferRequestFailed=An error occurred when someone tried to take one of your offers:\n{0}
popup.error.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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not show an Error popup as it comes with the bug-report text and is confusing for that use case, even it is somehow an error as Bisq cannot be used in that case. I would suggest to use warning popup and to reflect that in the display string we should use popup.warning.userBtcNodeMisConfigured.explanation. Also changed to camelCase as IDEA static code inspector complains about a typo otherwise. I think misconfigured should be mis-configured in the text.

Copy link
Contributor Author

@dmos62 dmos62 Sep 10, 2020

Choose a reason for hiding this comment

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

Warning doesn't seem entirely appropriate, giving the event a milder air than it might deserve (event being not something to keep in mind, but something to fix), but it might be more appropriate than Error, which seems to be dedicated to events that should be bug-reported. I'll switch to Warning.

I think the spelling of "misconfigure" is correct. "Mis" + "construct" is a rarer combination, so it's not found on many dictionaries, but you can find more common uses of the prefix mis in entries like "misplace" [0] or "misconduct" [1]. If you take a look at the descriptions of the prefix mis- in popular dictionaries [2], it's normal to use it without a hyphen. I think a hyphen is used uniquely when connecting entire words (something like the hyphenation in post-modernism [3] is a discouraged spelling: redirects to postmodernism).

IDEA static code inspector complains about a typo

Can words be added to its dictionary, like you would in a spellchecking text editor? It would be awkward to alter writing to match a spellchecker.

[0] https://www.thefreedictionary.com/misplace
[1] https://www.thefreedictionary.com/misconduct
[2] https://www.thefreedictionary.com/mis-
[3] https://www.thefreedictionary.com/post-modernism

Copy link
Contributor

Choose a reason for hiding this comment

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

You can set the header message as well, so the title is not warning but something more appropriate. The error type popup carries the automatic support for bug report...

Copy link
Contributor

Choose a reason for hiding this comment

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

Re misconfigure: Ah ok. I am not native speaker and IDEA suggestions is my teacher ;-).

Can words be added to its dictionary
Yes.

(String peer) ->
new Popup()
.hideCloseButton()
.error(Res.get("popup.error.userBtcNodeMisconfigured.explanation", peer))
Copy link
Contributor

Choose a reason for hiding this comment

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

As per prev. comment:
-> .warning(Res.get("popup.warning.userBtcNodeMisConfigured.explanation", peer))

* 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The uppercase Void is probably a leftover from the previous code. Should be void.

BtcNodes.BitcoinNodesOption.CUSTOM.ordinal() == preferences.getBitcoinNodesOptionOrdinal();
if (usingLocalNode || usingCustomNodes) {
displayUserBtcNodeMisconfigurationHandler.accept(peer.toString());
}
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

If void is used above the return need to be removed.

chimp1984
chimp1984 previously approved these changes Sep 10, 2020
Copy link
Contributor

@chimp1984 chimp1984 left a comment

Choose a reason for hiding this comment

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

utACK

Thanks for the update!

@dmos62 dmos62 force-pushed the different-peer-configuration-checking branch from 8415c04 to c1203c9 Compare February 3, 2021 15:56
@dmos62
Copy link
Contributor Author

dmos62 commented Feb 4, 2021

I've ported this PR and a BitcoinJ PR that this depends on to BitcoinJ 15.8. No noteworthy changes required.

This PR currently needs a locally installed BitcoinJ library instance, for development purposes (you have to git checkout the above mentioned BitcoinJ PR and run gradle install on it). Hence Travis failing. Once the BitcoinJ PR is merged, we can update this PR to use our Github hosted fork.

@ripcurlx ripcurlx modified the milestones: v1.5.7, v1.6.0 Mar 1, 2021
@ripcurlx ripcurlx modified the milestones: v1.6.0, v1.6.1, v1.6.2 Mar 27, 2021
@ripcurlx ripcurlx modified the milestones: v1.6.3, v1.6.4 Apr 22, 2021
@ripcurlx ripcurlx modified the milestones: v1.6.5, v1.6.6 May 21, 2021
Comment on lines +433 to +434
var wellConfigured = isWellConfigured(versionMessage);
if (wellConfigured) {

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.

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

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).

@ripcurlx ripcurlx modified the milestones: v1.7.0, v1.7.1 Jun 24, 2021
@ripcurlx ripcurlx modified the milestones: v1.7.2, v1.7.3 Jul 16, 2021
@ripcurlx ripcurlx modified the milestones: v1.7.3, v1.7.4 Aug 23, 2021
@ripcurlx ripcurlx modified the milestones: v1.7.4, v1.7.5 Sep 21, 2021
@ripcurlx ripcurlx modified the milestones: v1.7.5, v1.8.0 Oct 19, 2021
@ripcurlx ripcurlx modified the milestones: v1.8.0, v1.8.1 Dec 9, 2021
@ripcurlx ripcurlx modified the milestones: v1.8.1, v1.8.2 Jan 12, 2022
@ripcurlx ripcurlx modified the milestones: v1.8.3, v1.8.4 Feb 24, 2022
@dmos62 dmos62 closed this Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants