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

Revert to only check local BTC node's port #4067

Merged

Conversation

dmos62
Copy link
Contributor

@dmos62 dmos62 commented Mar 16, 2020

Revert to only check local BTC node's port

Testing showed that the new mechanic for checking a local BTC node's
configuration is unstable. This commit reverts to just checking if the
relevant port is open. The recent refactoring and centralization of
logic is still in place.

Supersedes PR #4062.

dmos62 added 2 commits March 16, 2020 11:57
Testing showed that the new mechanic for checking a local BTC node's
configuration is unstable. This commit reverts to just checking if the
relevant port is open. The recent refactoring and centralization of
logic is still in place.
@ripcurlx ripcurlx requested a review from wiz March 16, 2020 13:04
ripcurlx
ripcurlx previously approved these changes Mar 16, 2020
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK

Works for me with and without a Bitcoin Core running locally

@ripcurlx
Copy link
Contributor

Waiting for another ACK by @wiz until I merge and cherry-pick it into the v1.2.9 release branch

wiz
wiz previously approved these changes Mar 16, 2020
Copy link
Contributor

@wiz wiz left a comment

Choose a reason for hiding this comment

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

ACK
tested OK on 4 node linux regtest setup 👍

popup.warning.localNodeMisconfigured.explanation=Bisq detected a locally running Bitcoin Core node (at localhost); however, its configuration is incompatible with Bisq.\n\n\
For Bisq to use a local Bitcoin node, the node has to have pruning disabled and bloom filters enabled. Starting with Bitcoin Core v0.19 you have to manually enable bloom filters by setting peerbloomfilters=1 in your bitcoin.conf configuration file.
popup.warning.localNodeMisconfigured.continueWithoutLocalNode=Continue without using local node

popup.bitcoinLocalhostNode.msg=Bisq detected a locally running Bitcoin Core node (at localhost) and is using it.\n\
Please make sure that this node is fully synced before you start Bisq.
popup.bitcoinLocalhostNode.additionalRequirements=\n\nThe node was found to be well configured. For reference, the requirements are for the node to have pruning disabled and bloom filters enabled.
Copy link
Member

Choose a reason for hiding this comment

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

Should this message remain without the proper check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't notice that. Should be changed. Have to go out for an hour; will change it when I come back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch - this should be in that case

Suggested change
popup.bitcoinLocalhostNode.additionalRequirements=\n\nThe node was found to be well configured. For reference, the requirements are for the node to have pruning disabled and bloom filters enabled.
popup.bitcoinLocalhostNode.additionalRequirements=\n\nStarting with Bitcoin Core v0.19 you have to manually enable bloom filters by setting peerbloomfilters=1 in your bitcoin.conf configuration file.

@ripcurlx ripcurlx dismissed stale reviews from wiz and themself via 253ec9c March 16, 2020 13:54
@ripcurlx
Copy link
Contributor

@dmos62 I already committed the change suggestions

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

ACK

Committed the suggested text changes

@ripcurlx ripcurlx merged commit e13ffe6 into bisq-network:master Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants