<p><b>@sqrrm</b> commented on this pull request.</p>
<p>Please have a look at my comments on recursion</p><hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2056#discussion_r239066459">core/src/main/java/bisq/core/dao/node/BsqNode.java</a>:</p>
<pre style='color:#555'>> + }
+
+ 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);
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2056#discussion_r239075473">core/src/main/java/bisq/core/dao/node/BsqNode.java</a>:</p>
<pre style='color:#555'>> + 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");
</pre>
⬇️ Suggested change
<pre style="color: #555">- log.info("In case we have reset from genesis height we would not find the block");
+ log.info("Older block received but nothing found at that height, this is normal if there was a reset from genesis height");
</pre>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2056#discussion_r239079582">core/src/main/java/bisq/core/dao/node/BsqNode.java</a>:</p>
<pre style='color:#555'>> +
+ 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) {
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/bisq-network/bisq/pull/2056#discussion_r239087043">core/src/main/java/bisq/core/dao/node/full/FullNode.java</a>:</p>
<pre style='color:#555'>> @@ -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 " +
</pre>
⬇️ Suggested change
<pre style="color: #555">- errorMessage = "You have configured Bisq to run as DAO full node but there is not " +
+ errorMessage = "You have configured Bisq to run as DAO full node but there is no " +
</pre>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/2056#pullrequestreview-181757903">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AkpZtnxcs823YQiQgOMZseGmCzABoqNPks5u19zNgaJpZM4ZB0Vf">mute the thread</a>.<img src="https://github.com/notifications/beacon/AkpZtikLs8E9ODuU26fZDBBhOz-HYQ-Uks5u19zNgaJpZM4ZB0Vf.gif" height="1" width="1" alt="" /></p>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/bisq-network/bisq","title":"bisq-network/bisq","subtitle":"GitHub repository","main_image_url":"https://assets-cdn.github.com/images/email/message_cards/header.png","avatar_image_url":"https://assets-cdn.github.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/bisq-network/bisq"}},"updates":{"snippets":[{"icon":"PERSON","message":"@sqrrm commented on #2056"}],"action":{"name":"View Pull Request","url":"https://github.com/bisq-network/bisq/pull/2056#pullrequestreview-181757903"}}}</script>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/2056#pullrequestreview-181757903",
"url": "https://github.com/bisq-network/bisq/pull/2056#pullrequestreview-181757903",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
},
{
"@type": "MessageCard",
"@context": "http://schema.org/extensions",
"hideOriginalBody": "false",
"originator": "AF6C5A86-E920-430C-9C59-A73278B5EFEB",
"title": "@sqrrm commented on 2056",
"sections": [
{
"text": "Please have a look at my comments on recursion",
"activityTitle": "**sqrrm**",
"activityImage": "https://assets-cdn.github.com/images/email/message_cards/avatar.png",
"activitySubtitle": "@sqrrm",
"facts": [
]
}
],
"potentialAction": [
{
"targets": [
{
"os": "default",
"uri": "https://github.com/bisq-network/bisq/pull/2056#pullrequestreview-181757903"
}
],
"@type": "OpenUri",
"name": "View on GitHub"
},
{
"name": "Unsubscribe",
"@type": "HttpPOST",
"target": "https://api.github.com",
"body": "{\n\"commandName\": \"MuteNotification\",\n\"threadId\": 419906911\n}"
}
],
"themeColor": "26292E"
}
]</script>