[bisq-network/bisq] Improve getBlocks request handling (#4851)

[sqrrm]

@sqrrm commented on this pull request.



> +        } else {
+            if (blockDownloadListener == null) {
+                blockDownloadListener = (observable, oldValue, newValue) -> {
+                    if ((double) newValue == 1) {
+                        setupWalletBestBlockListener();
+                        UserThread.execute(() -> walletsSetup.downloadPercentageProperty().removeListener(blockDownloadListener));
+                    }
+                };
+                walletsSetup.downloadPercentageProperty().addListener(blockDownloadListener);
+            }
+        }

```suggestion
        } else if (blockDownloadListener == null) {
            blockDownloadListener = (observable, oldValue, newValue) -> {
                if ((double) newValue == 1) {
                    setupWalletBestBlockListener();
                    UserThread.execute(() -> walletsSetup.downloadPercentageProperty().removeListener(blockDownloadListener));
                }
            };
            walletsSetup.downloadPercentageProperty().addListener(blockDownloadListener);
        }
```

> @@ -97,20 +122,20 @@ public void start() {
                 checkForBlockReceivedTimer.stop();
             }
 
-            int height = block.getHeight();
-            log.info("New block at height {} from bsqWalletService", height);
+            int btcWalletHeight = btcBlock.getHeight();
+            log.error("New block at height {} from bsqWalletService", btcWalletHeight);

Why error log level?

>                  }
-            }, CHECK_FOR_BLOCK_RECEIVED_DELAY_SEC);
+            }, CHECK_FOR_BLOCK_RECEIVED_DELAY_SEC, TimeUnit.MILLISECONDS);

Timeout unit should be sec, is this a test artefact together with the error log level?

> @@ -229,11 +257,12 @@ private void runDelayedBatchProcessing(List<RawBlock> blocks, Runnable resultHan
     // We received a new block
     private void onNewBlockReceived(RawBlock block) {
         int blockHeight = block.getHeight();
-        log.debug("onNewBlockReceived: block at height {}, hash={}", blockHeight, block.getHash());
+        log.info("onNewBlockReceived: block at height {}, hash={}. Our DAO chainHeight={}", blockHeight, block.getHash(), chainTipHeight);

That's quite a bit of logging but perhaps ok since it's once per received block

>                      } else {
-                        log.trace("We have stopped already. We ignore that networkNode.sendMessage.onFailure call. " +
-                                "Might be caused by a previous timeout.");
+                        log.warn("We have stopped already. We ignore that timeoutTimer.run call. " +
+                                "Might be caused by an previous networkNode.sendMessage.onFailure.");

```suggestion
                                "Might be caused by a previous networkNode.sendMessage.onFailure.");
```

> @@ -168,31 +167,36 @@ public void onFailure(@NotNull Throwable throwable) {
     @Override
     public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) {
         if (networkEnvelope instanceof GetBlocksResponse) {

```suggestion
        if (!(networkEnvelope instanceof GetBlocksResponse)) return; // Also need to remove other brace
```

>              }
+
+            GetBlocksResponse getBlocksResponse = (GetBlocksResponse) networkEnvelope;
+            if (getBlocksResponse.getRequestNonce() != nonce) {
+                log.warn("Nonce not matching. That can happen rarely if we get a response after a canceled " +
+                                "handshake (timeout causes connection close but peer might have sent a msg before " +
+                                "connection was closed).\n\t" +
+                                "We drop that message. nonce={} / requestNonce={}",
+                        nonce, getBlocksResponse.getRequestNonce());
+                return;
+            }
+
+            cleanup();

Doesn't timeoutTimer need to be stopped here like before?
```
        stopTimeoutTimer();
```

-- 
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/4851#pullrequestreview-545003484
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20201204/75d6d4f4/attachment-0001.htm>