[bisq-network/bisq] Dao fix reorg issues (#2056)

[sqrrm]

sqrrm commented on this pull request.

Please have a look at my comments on recursion

> +        }
+
+        try {
+            Block block = blockParser.parseBlock(rawBlock);
+
+            if (pendingBlocks.contains(rawBlock))
+                pendingBlocks.remove(rawBlock);
+
+            // After parsing we check if we have pending blocks we might have received earlier but which have been
+            // not connecting from the latest height we had. The list is sorted by height
+            if (!pendingBlocks.isEmpty()) {
+                // To avoid ConcurrentModificationException we copy the list. It might be altered in the method call
+                ArrayList<RawBlock> tempPendingBlocks = new ArrayList<>(pendingBlocks);
+                for (RawBlock tempPendingBlock : tempPendingBlocks) {
+                    try {
+                        doParseBlock(tempPendingBlock);

This recursion seems a bit strange, it has to copy pendingBlocks into a temporary array for each recursion level, probably ok but a bit wasteful.

> +            if (rawBlock.getHeight() > heightForNextBlock) {
+                pendingBlocks.add(rawBlock);
+                pendingBlocks.sort(Comparator.comparing(RawBlock::getHeight));
+                log.info("We received an block with a future block height. We store it as pending and try to apply " +
+                        "it at the next block. rawBlock: height/hash={}/{}", rawBlock.getHeight(), rawBlock.getHash());
+            } else if (rawBlock.getHeight() >= daoStateService.getGenesisBlockHeight()) {
+                // We received an older block. We compare if we have it in our chain.
+                Optional<Block> optionalBlock = daoStateService.getBlockAtHeight(rawBlock.getHeight());
+                if (optionalBlock.isPresent()) {
+                    if (optionalBlock.get().getHash().equals(rawBlock.getPreviousBlockHash())) {
+                        log.info("We received an old block we have already parsed and added. We ignore it.");
+                    } else {
+                        log.info("We received an old block with a different hash. We ignore it. Hash={}", rawBlock.getHash());
+                    }
+                } else {
+                    log.info("In case we have reset from genesis height we would not find the block");

```suggestion
                    log.info("Older block received but nothing found at that height, this is normal if there was a reset from genesis height");
```

> +
+        try {
+            Block block = blockParser.parseBlock(rawBlock);
+
+            if (pendingBlocks.contains(rawBlock))
+                pendingBlocks.remove(rawBlock);
+
+            // After parsing we check if we have pending blocks we might have received earlier but which have been
+            // not connecting from the latest height we had. The list is sorted by height
+            if (!pendingBlocks.isEmpty()) {
+                // To avoid ConcurrentModificationException we copy the list. It might be altered in the method call
+                ArrayList<RawBlock> tempPendingBlocks = new ArrayList<>(pendingBlocks);
+                for (RawBlock tempPendingBlock : tempPendingBlocks) {
+                    try {
+                        doParseBlock(tempPendingBlock);
+                    } catch (RequiredReorgFromSnapshotException e1) {

Catching the RequiredReorgFromSnapshotException here and braking will stop this loop but if there was a loop in a previous level of recursion that would continue running and parsing in a now known bad state. I think the exception has to be rethrown here.

> @@ -245,16 +240,13 @@ private void parseBlock(int blockHeight, int chainHeight,
     private void handleError(Throwable throwable) {
         String errorMessage = "An error occurred: Error=" + throwable.toString();
         log.error(errorMessage);
-
-        if (throwable instanceof BlockNotConnectingException) {
-            startReOrgFromLastSnapshot();
-        } else if (throwable instanceof RpcException &&
+        if (throwable instanceof RpcException &&
                 throwable.getCause() != null &&
                 throwable.getCause() instanceof HttpLayerException &&
                 ((HttpLayerException) throwable.getCause()).getCode() == 1004004) {
             errorMessage = "You have configured Bisq to run as DAO full node but there is not " +

```suggestion
            errorMessage = "You have configured Bisq to run as DAO full node but there is no " +
```

-- 
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/2056#pullrequestreview-181757903
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20181205/b073718e/attachment-0001.html>