[bisq-network/bisq] Add local Bitcoin node configuration detection (#3982)
Christoph Atteneder
notifications at github.com
Wed Feb 26 10:42:31 UTC 2020
ripcurlx requested changes on this pull request.
NACK
Please see my remarks in the code.
I tested the functionality on Regtest with a local Bitcoin Core and I don't get the informational popup anymore when running the client with a properly configured client
Having peer bloom filters not configured and I also don't get the warning. Doesn't it work with the local Regtest setup?
> 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.
+ var detectedButMisconfigured = localBitcoinNode.isDetectedButMisconfigured();
+ if (detectedButMisconfigured) {
+ displayLocalNodeMisconfigurationHandler.accept(nextStep);
```suggestion
if (displayLocalNodeMisconfigurationHandler != null) {
displayLocalNodeMisconfigurationHandler.accept(nextStep);
}
```
Add a check to prevent possible NullPointerException.
> @@ -559,7 +576,7 @@ 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()) {
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.
> + }
+
+ /* Performs a blocking Bitcoin protocol handshake, which includes exchanging version
+ * messages and acks. Its purpose is to check if a local Bitcoin node is running,
+ * and, if it is, check its advertised configuration. The returned Optional is empty,
+ * if a local peer wasn't found, or if handshake failed for some reason. This method
+ * could be noticably simplified, by turning connection failure callback into a
+ * future and using a first-future-to-complete type of construct, but I couldn't find
+ * a ready-made implementation.
+ */
+ private Optional<VersionMessage> attemptHandshakeForVersionMessage() {
+ Peer peer;
+ try {
+ peer = createLocalPeer(port);
+ } catch (UnknownHostException ex) {
+ log.error("Local bitcoin node handshake attempt unexpectedly threw: {}", ex);
Using it like that the placeholder will never be filled. Maybe something like:
```suggestion
log.error("Local bitcoin node handshake attempt was unexpectedly interrupted", ex);
```
> + /* We temporarily silence BitcoinJ NioClient's and NioClientManager's loggers,
+ * because when a local Bitcoin node is not found they pollute console output
+ * with "connection refused" error messages.
+ */
+ var originalNioClientLoggerLevel = silence(NioClient.class);
+ var originalNioClientManagerLoggerLevel = silence(NioClientManager.class);
+
+ NioClient client;
+
+ try {
+ log.info("Initiating attempt to connect to and handshake with a local "
+ + "Bitcoin node (which may or may not be running) on port {}.",
+ port);
+ client = createClient(peer, port, CONNECTION_TIMEOUT);
+ } catch (IOException ex) {
+ log.error("Local bitcoin node handshake attempt unexpectedly threw: {}", ex);
Using it like that the placeholder will never be filled. Maybe something like:
```suggestion
log.error("Local bitcoin node handshake attempt was unexpectedly interrupted", ex);
```
> + var peer = new Peer(networkParameters, ourVersionMessage, localPeerAddress, blockchain);
+ return peer;
Not sure if that makes it so much more readable than simply:
```
return new Peer(networkParameters, ourVersionMessage, localPeerAddress, blockchain);
```
> + NioClient client = new NioClient(serverAddress, peer, connectionTimeout);
+
+ return client;
Same here:
```
return new NioClient(serverAddress, peer, connectionTimeout);
```
> + AbstractBlockChain blockchain = null;
+
+ var peer = new Peer(networkParameters, ourVersionMessage, localPeerAddress, blockchain);
I know that you do this because you use Vim, but everyone with a more sophisticated editor has this parameter information next to the null value in the constructor instantiation
> +
+ /* Creates an NioClient that is expected to only be used to coerce a VersionMessage
+ * out of a local Bitcoin node and be closed right after.
+ */
+ private static NioClient createClient(Peer peer, int port, int connectionTimeout) throws IOException {
+ InetSocketAddress serverAddress =
+ new InetSocketAddress(InetAddress.getLocalHost(), port);
+
+ // This initiates the handshake procedure, which, if successful, will complete
+ // the peerVersionMessageFuture, or be cancelled, in case of failure.
+ NioClient client = new NioClient(serverAddress, peer, connectionTimeout);
+
+ return client;
+ }
+
+ private static Level silence(Class klass) {
```suggestion
private static Level silence(Class<?> klass) {
```
> +
+ // This initiates the handshake procedure, which, if successful, will complete
+ // the peerVersionMessageFuture, or be cancelled, in case of failure.
+ NioClient client = new NioClient(serverAddress, peer, connectionTimeout);
+
+ return client;
+ }
+
+ private static Level silence(Class klass) {
+ var logger = getLogger(klass);
+ var originalLevel = logger.getLevel();
+ logger.setLevel(Level.OFF);
+ return originalLevel;
+ }
+
+ private static void restoreLoggerLevel(Class klass, Level originalLevel) {
```suggestion
private static void restoreLoggerLevel(Class<?> klass, Level originalLevel) {
```
> +
+ return client;
+ }
+
+ private static Level silence(Class klass) {
+ var logger = getLogger(klass);
+ var originalLevel = logger.getLevel();
+ logger.setLevel(Level.OFF);
+ return originalLevel;
+ }
+
+ private static void restoreLoggerLevel(Class klass, Level originalLevel) {
+ getLogger(klass).setLevel(originalLevel);
+ }
+
+ private static ch.qos.logback.classic.Logger getLogger(Class klass) {
```suggestion
private static ch.qos.logback.classic.Logger getLogger(Class<?> klass) {
```
> + return originalLevel;
+ }
+
+ private static void restoreLoggerLevel(Class klass, Level originalLevel) {
+ getLogger(klass).setLevel(originalLevel);
+ }
+
+ private static ch.qos.logback.classic.Logger getLogger(Class klass) {
+ return (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(klass);
+ }
+
+ private ListenableFuture<VersionMessage> getVersionMessage(Peer peer) {
+ SettableFuture<VersionMessage> peerVersionMessageFuture = SettableFuture.create();
+
+ var versionHandshakeDone = peer.getVersionHandshakeFuture();
+ FutureCallback<Peer> fetchPeerVersionMessage = new FutureCallback<Peer>() {
```suggestion
FutureCallback<Peer> fetchPeerVersionMessage = new FutureCallback<>() {
```
> + getLogger(klass).setLevel(originalLevel);
+ }
+
+ private static ch.qos.logback.classic.Logger getLogger(Class klass) {
+ return (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(klass);
+ }
+
+ private ListenableFuture<VersionMessage> getVersionMessage(Peer peer) {
+ SettableFuture<VersionMessage> peerVersionMessageFuture = SettableFuture.create();
+
+ var versionHandshakeDone = peer.getVersionHandshakeFuture();
+ FutureCallback<Peer> fetchPeerVersionMessage = new FutureCallback<Peer>() {
+ public void onSuccess(Peer peer) {
+ peerVersionMessageFuture.set(peer.getPeerVersionMessage());
+ }
+ public void onFailure(Throwable thr) {
```suggestion
public void onFailure(@NotNull Throwable thr) {
```
> + new PeerDisconnectedEventListener() {
+ public void onPeerDisconnected(Peer peer, int peerCount) {
Can be replaced with:
```
(disconnectedPeer, peerCount) -> {
```
> +
+ PeerDisconnectedEventListener cancelIfConnectionFails =
+ new PeerDisconnectedEventListener() {
+ public void onPeerDisconnected(Peer peer, int peerCount) {
+ var peerVersionMessageAlreadyReceived =
+ peerVersionMessageFuture.isDone();
+ if (peerVersionMessageAlreadyReceived) {
+ // This method is called whether or not the handshake was
+ // successful. In case it was successful, we don't want to do
+ // anything here.
+ return;
+ }
+ // In some cases Peer will self-disconnect after receiving
+ // node's VersionMessage, but before completing the handshake.
+ // In such a case, we want to retrieve the VersionMessage.
+ var peerVersionMessage = peer.getPeerVersionMessage();
That would need also to change this to:
```suggestion
var peerVersionMessage = disconnectedPeer.getPeerVersionMessage();
```
to prevent scope issues with the other peer passed as parameter. This makes it also easier to distinguish that they are different peers.
> + var mayInterruptWhileRunning = true;
+ peerVersionMessageFuture.cancel(mayInterruptWhileRunning);
Here again: This is something that helps only someone not using code editors like IntelliJ.
> + public boolean willUse() {
+ return !willIgnore() && isUsable();
+ }
+
+ /**
+ * Returns whether Bisq will ignore a local Bitcoin node even if it is usable.
+ */
+ public boolean willIgnore() {
+ BaseCurrencyNetwork baseCurrencyNetwork = config.baseCurrencyNetwork;
+
+ // For dao testnet (server side regtest) we disable the use of local bitcoin node to
+ // avoid confusion if local btc node is not synced with our dao testnet master node.
+ // Note: above comment was previously in WalletConfig::createPeerGroup.
+
+ return config.ignoreLocalBtcNode
+ || baseCurrencyNetwork.isDaoRegTest()
```suggestion
|| baseCurrencyNetwork.isDaoRegTest()
```
> + return !willIgnore() && isUsable();
+ }
+
+ /**
+ * Returns whether Bisq will ignore a local Bitcoin node even if it is usable.
+ */
+ public boolean willIgnore() {
+ BaseCurrencyNetwork baseCurrencyNetwork = config.baseCurrencyNetwork;
+
+ // For dao testnet (server side regtest) we disable the use of local bitcoin node to
+ // avoid confusion if local btc node is not synced with our dao testnet master node.
+ // Note: above comment was previously in WalletConfig::createPeerGroup.
+
+ return config.ignoreLocalBtcNode
+ || baseCurrencyNetwork.isDaoRegTest()
+ || baseCurrencyNetwork.isDaoTestNet();
```suggestion
|| baseCurrencyNetwork.isDaoTestNet();
```
> + }
+
+ /* Initiates detection and configuration checks. The results are cached so that the
+ * public methods isUsable, isDetected, etc. don't trigger a recheck.
+ */
+ private void checkUsable() {
+ var optionalVersionMessage = attemptHandshakeForVersionMessage();
+ handleHandshakeAttempt(optionalVersionMessage);
+ }
+
+ private void handleHandshakeAttempt(Optional<VersionMessage> optionalVersionMessage) {
+ if (!optionalVersionMessage.isPresent()) {
+ detected = false;
+ wellConfigured = false;
+ log.info("No local Bitcoin node detected on port {},"
+ + " or the connection was prematurely closed"
```suggestion
+ " or the connection was prematurely closed"
```
> +
+ /* Initiates detection and configuration checks. The results are cached so that the
+ * public methods isUsable, isDetected, etc. don't trigger a recheck.
+ */
+ private void checkUsable() {
+ var optionalVersionMessage = attemptHandshakeForVersionMessage();
+ handleHandshakeAttempt(optionalVersionMessage);
+ }
+
+ private void handleHandshakeAttempt(Optional<VersionMessage> optionalVersionMessage) {
+ if (!optionalVersionMessage.isPresent()) {
+ detected = false;
+ wellConfigured = false;
+ log.info("No local Bitcoin node detected on port {},"
+ + " or the connection was prematurely closed"
+ + " (before a version messages could be coerced)",
```suggestion
+ " (before a version messages could be coerced)",
```
> + log.error("Local bitcoin node handshake attempt unexpectedly threw: {}", ex);
+ return Optional.empty();
+ }
+
+ /* We temporarily silence BitcoinJ NioClient's and NioClientManager's loggers,
+ * because when a local Bitcoin node is not found they pollute console output
+ * with "connection refused" error messages.
+ */
+ var originalNioClientLoggerLevel = silence(NioClient.class);
+ var originalNioClientManagerLoggerLevel = silence(NioClientManager.class);
+
+ NioClient client;
+
+ try {
+ log.info("Initiating attempt to connect to and handshake with a local "
+ + "Bitcoin node (which may or may not be running) on port {}.",
```suggestion
+ "Bitcoin node (which may or may not be running) on port {}.",
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3982#pullrequestreview-364736228
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200226/08a47bc2/attachment-0001.html>
More information about the bisq-github
mailing list