[bisq-network/bisq] Add transient tx map to DaoState to speed up getTx queries (#3773)

[chimp1984]

chimp1984 requested changes on this pull request.



> @@ -145,6 +154,10 @@ private DaoState(int chainHeight,
         this.paramChangeList = paramChangeList;
         this.evaluatedProposalList = evaluatedProposalList;
         this.decryptedBallotsWithMeritsList = decryptedBallotsWithMeritsList;
+
+        txMap = blocks.stream()
+                .flatMap(block -> block.getTxs().stream())
+                .collect(Collectors.toMap(Tx::getId, Function.identity(), (x, y) -> y, HashMap::new));

I assume the mergeFunction is only passed as you want to have the mapFactory. Not sure what the mergeFunction really should do as conflicts are not expected and not clear how to handle it. Maybe throwing an exception would be more appropriate here? Or maybe just add a comment.

> @@ -226,7 +232,16 @@ public void onNewBlockWithEmptyTxs(Block block) {
         }
     }
 
-    // Third we get the onParseBlockComplete called after all rawTxs of blocks have been parsed
+    // Third we add each successfully parsed BSQ tx to the last block
+    public void onNewTxForLastBlock(Block block, Tx tx) {
+        // At least one block must be present else no rawTx would have been recognised as a BSQ tx.

We should add here the `assertDaoStateChange();` check for conistency. All methods which alter the DAO state call that method to guard against changes outside of the permitted process. Only at parsing the DAO state must be changed.

> @@ -226,7 +232,16 @@ public void onNewBlockWithEmptyTxs(Block block) {
         }
     }
 
-    // Third we get the onParseBlockComplete called after all rawTxs of blocks have been parsed
+    // Third we add each successfully parsed BSQ tx to the last block
+    public void onNewTxForLastBlock(Block block, Tx tx) {
+        // At least one block must be present else no rawTx would have been recognised as a BSQ tx.
+        Preconditions.checkArgument(block == getLastBlock().orElseThrow());

This would change the original behaviour.

We used the onNewBlockWithEmptyTxs to add a block, but in case we got into the `if` path we did not add the block but only log a warning. Adding of transactions would had no effect as it was a local list in the original code. Now we throw an exception in such a case as we expect that the block has been added.

```
public void onNewBlockWithEmptyTxs(Block block) {
        assertDaoStateChange();
        if (daoState.getBlocks().isEmpty() && block.getHeight() != getGenesisBlockHeight()) {
            log.warn("We don't have any blocks yet and we received a block which is not the genesis block. " +
                    "We ignore that block as the first block need to be the genesis block. " +
                    "That might happen in edge cases at reorgs. Received block={}", block);
        } else {
            daoState.getBlocks().add(block);

            if (parseBlockChainComplete)
                log.info("New Block added at blockHeight {}", block.getHeight());
        }
    }
```

I am not sure if that case is valid and can happen, but as the log suggests there might be tricky edge cases in re-org scenarious where this was a possible scenario. Testing those edge cases is pretty tricky and it can be that it was during development an issue which disappeared later and is not present anymore. But I would prefer to stay very conservative/restrictive in the DAO domain as a consensus bug can have severe consequences and the DAO has a very deep level of complexit. If we are not 100% sure that existing code is wrong I prefer to stick with it, as this code base has been tested excessively and is in production since April.



> @@ -226,7 +232,16 @@ public void onNewBlockWithEmptyTxs(Block block) {
         }
     }
 
-    // Third we get the onParseBlockComplete called after all rawTxs of blocks have been parsed
+    // Third we add each successfully parsed BSQ tx to the last block
+    public void onNewTxForLastBlock(Block block, Tx tx) {
+        // At least one block must be present else no rawTx would have been recognised as a BSQ tx.
+        Preconditions.checkArgument(block == getLastBlock().orElseThrow());

NACK for that Preconditions check, otherwise it looks good. Thanks for working on that.
I need a bit more time to go again over all and check if anything might be missing.

Could you provide a comparision of performance gains from that PR?


-- 
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/3773#pullrequestreview-330666614
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191211/2cee4c9a/attachment-0001.html>