[bisq-network/bisq] Add local Bitcoin node configuration detection (#3982)
sqrrm
notifications at github.com
Sat Feb 22 23:05:59 UTC 2020
sqrrm commented on this pull request.
I added a comment on some of the blank lines I would remove. Personally I would probably make it even tighter though.
I understand the motivation of separating some of the concepts with blank lines, but it doesn't really help with readability since the methods themselves already contain the scope of activity.
>
@Inject
public LocalBitcoinNode(@Named(LOCAL_BITCOIN_NODE_PORT) int port) {
this.port = port;
}
+ /* 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);
+
Blank line
> +
+ return client;
+ }
+
+ /* Creates a Peer that is expected to only be used to coerce a VersionMessage out of a
+ * local Bitcoin node and be closed right after.
+ */
+ private static Peer createLocalPeer(int port) throws UnknownHostException {
+ // TODO: what's the effect of NetworkParameters on handshake?
+ // i.e. is it fine to just always use MainNetParams?
+ var networkParameters = new MainNetParams();
+
+ // We must construct a BitcoinJ Context before using BitcoinJ. We don't keep a
+ // reference, because it's automatically kept in a thread local storage.
+ new Context(networkParameters);
+
Blank line
> + }
+
+ /* Creates a Peer that is expected to only be used to coerce a VersionMessage out of a
+ * local Bitcoin node and be closed right after.
+ */
+ private static Peer createLocalPeer(int port) throws UnknownHostException {
+ // TODO: what's the effect of NetworkParameters on handshake?
+ // i.e. is it fine to just always use MainNetParams?
+ var networkParameters = new MainNetParams();
+
+ // We must construct a BitcoinJ Context before using BitcoinJ. We don't keep a
+ // reference, because it's automatically kept in a thread local storage.
+ new Context(networkParameters);
+
+ var ourVersionMessage = new VersionMessage(networkParameters, 0);
+
Blank line
> + /* Creates a Peer that is expected to only be used to coerce a VersionMessage out of a
+ * local Bitcoin node and be closed right after.
+ */
+ private static Peer createLocalPeer(int port) throws UnknownHostException {
+ // TODO: what's the effect of NetworkParameters on handshake?
+ // i.e. is it fine to just always use MainNetParams?
+ var networkParameters = new MainNetParams();
+
+ // We must construct a BitcoinJ Context before using BitcoinJ. We don't keep a
+ // reference, because it's automatically kept in a thread local storage.
+ new Context(networkParameters);
+
+ var ourVersionMessage = new VersionMessage(networkParameters, 0);
+
+ var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port);
+
Blank line
> + */
+ private static Peer createLocalPeer(int port) throws UnknownHostException {
+ // TODO: what's the effect of NetworkParameters on handshake?
+ // i.e. is it fine to just always use MainNetParams?
+ var networkParameters = new MainNetParams();
+
+ // We must construct a BitcoinJ Context before using BitcoinJ. We don't keep a
+ // reference, because it's automatically kept in a thread local storage.
+ new Context(networkParameters);
+
+ var ourVersionMessage = new VersionMessage(networkParameters, 0);
+
+ var localPeerAddress = new PeerAddress(InetAddress.getLocalHost(), port);
+
+ AbstractBlockChain blockchain = null;
+
Blank line
> + 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
+ */
+ 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;
+
Blank line
> + * 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
+ */
+ 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;
+
Blank line
> +
+ /* 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
+ */
+ 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;
+
Blank line
> log.info("Local Bitcoin node detected on port {}", port);
- detected = true;
+
+ var versionMessage = optionalVersionMessage.get();
+ var configurationCheckResult = checkWellConfigured(versionMessage);
+
Blank line
> + try {
+ peer = createLocalPeer(port);
+ } catch (UnknownHostException ex) {
+ 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;
+
Blank line
> }
- callback.run();
+
+ ListenableFuture<VersionMessage> peerVersionMessageFuture = getVersionMessage(peer);
+
Blank line
> +
+ ListenableFuture<VersionMessage> peerVersionMessageFuture = getVersionMessage(peer);
+
+ Optional<VersionMessage> optionalPeerVersionMessage;
+
+ // block for VersionMessage or cancellation (in case of connection failure)
+ try {
+ var peerVersionMessage = peerVersionMessageFuture.get();
+ optionalPeerVersionMessage = Optional.of(peerVersionMessage);
+ } catch (ExecutionException | InterruptedException | CancellationException ex) {
+ optionalPeerVersionMessage = Optional.empty();
+ }
+
+ peer.close();
+ client.closeConnection();
+
Blank line
> + 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) {
+ return (ch.qos.logback.classic.Logger) LoggerFactory.getLogger(klass);
+ }
+
+ private ListenableFuture<VersionMessage> getVersionMessage(Peer peer) {
+ SettableFuture<VersionMessage> peerVersionMessageFuture = SettableFuture.create();
+
Blank line
> + return handleUnsafeQuery(isUsable());
+ }
+
+ private boolean handleUnsafeQuery(Optional<Boolean> opt) {
+ return opt.orElseGet(() -> {
+ /* Returning false when checks haven't been performed yet is what the behaviour
+ * was before we switched to using Optionals. More specifically, the only query
+ * method at the time, isDetected(), would return false in such a case. We are
+ * relatively confident that the previous behaviour doesn't cause fatal bugs,
+ * so, in case LocalBitcoinNode is queried too early, we revert to it, instead
+ * of letting Optional.empty().get() throw an exception. The advantage over
+ * plain booleans then is that we can log the below error message (with
+ * stacktrace).
+ */
+ var whenChecksNotFinished = false;
+
Blank line
> +
+ private boolean handleUnsafeQuery(Optional<Boolean> opt) {
+ return opt.orElseGet(() -> {
+ /* Returning false when checks haven't been performed yet is what the behaviour
+ * was before we switched to using Optionals. More specifically, the only query
+ * method at the time, isDetected(), would return false in such a case. We are
+ * relatively confident that the previous behaviour doesn't cause fatal bugs,
+ * so, in case LocalBitcoinNode is queried too early, we revert to it, instead
+ * of letting Optional.empty().get() throw an exception. The advantage over
+ * plain booleans then is that we can log the below error message (with
+ * stacktrace).
+ */
+ var whenChecksNotFinished = false;
+
+ var throwable = new Throwable("LocalBitcoinNode was queried before it was ready");
+
Blank line
> // unless the useTorForBtc parameter is explicitly provided.
// On testnet there are very few Bitcoin tor nodes and we don't provide tor nodes.
+
+ // TODO bug. Non-critical, apparently.
+ // Sometimes this method, which queries LocalBitcoinNode for whether or not there's a
+ // usable local Bitcoin node, is called before LocalBitcoinNode has performed its
+ // checks. This was noticed when LocalBitcoinNode was refactored to return
+ // Optional<Boolean> istead of boolean, an empty Optional signifying that the relevant
+ // check has not yet been performed.
+ var usableLocalNodePresent = localBitcoinNode.safeIsUsable();
+
Blank line
--
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-363055405
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20200222/6c82d87d/attachment-0001.html>
More information about the bisq-github
mailing list