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

Mute unnecessary logging and minor improvements #4062

Conversation

dmos62
Copy link
Contributor

@dmos62 dmos62 commented Mar 14, 2020

Replaces NioClient and NioClientManager with BlockingClient, which
gets rid of misguiding log entries as reported in #4058 PR's comments by
@ripcurlx. Also introduces a few minor changes including better logs and
clearer code.

Replaces NioClient and NioClientManager with BlockingClient, which
gets rid of misguiding log entries as reported in bisq-network#4058 PR's comments by
@ripcurlx. Also introduces a few minor changes including better logs and
clearer code.
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.

NACK

I just tested it on Mainnet with and without a local Bitcoin Core.
Running a well configured Bitcoin Core everything looks fine, but when I start the node as a light client without a Bitcoin Core running I'll always get this warning log:

März-15 11:44:21.452 [JavaFX Application Thread] ERROR b.c.btc.nodes.LocalBitcoinNode: Exploratory handshake attempt with a local Bitcoin node (that may not be there) unexpectedly timed out. This should never happen; please report this. Continuing as if a local BTC node was not found. 

If that will be triggered all the time no matter if there is a local bitcoin core node running or not, I think the text should be changed, that this is expected if no local BTC node is running on this system.

@ripcurlx
Copy link
Contributor

I've now also tested the two configuration errors again on Regtest and it is working as before 👍

@dmos62
Copy link
Contributor Author

dmos62 commented Mar 15, 2020

@ripcurlx that log error is about BitcoinJ hanging, which is not normal. I've not experienced this. @wiz reported this first, but in his case it was when a local BTC node wasn't running. You experiencing it when there is a local BTC node running is troubling.

Could you clarify your second comment? Did the bug go away on its own?

Starting to think this implementation is cursed. The bright side is that if we generalize configuration checking to remote nodes as well, we'll be able to do away with all this additional IO. I've realised that this implementation turned out like an awkward appendage, because I felt intimidated hooking into Bisq's regular BTC networking. A lot would have been simpler.

What's kind of troubling is that if LocalBitcoinNode's usage of BitcoinJ is not somehow incorrect, these weird BitcoinJ hangs might be happening in other parts of Bisq too.

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.

2 participants