[bisq-network/bisq] Add hash of dao state (#2532)

[sqrrm]

sqrrm commented on this pull request.



> @@ -45,9 +45,10 @@
     protected final long value;
     protected final String txId;
 
-    // Only set if dumpBlockchainData is true
+    // Before v0.9.6 it was only set if dumpBlockchainData was set to true but we changed that with 0.9.6
+    // so that is is always set. We still need to support it because of backward compatibility.

Perhaps we should do a cleanup before starting testing on mainnet. That would break the regtestnet we have running now though.

> @@ -162,6 +173,11 @@ public int getChainHeight() {
         return daoState.getCycles();
     }
 
+    public void addCycle(Cycle firstCycle) {

>From what I understand this is to add any cycle, not just the first one, would be better to call the argument cycle rather than firstCycle

> +
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Optional;
+import java.util.Random;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
+
+import lombok.Getter;
+import lombok.extern.slf4j.Slf4j;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+/**
+ * Monitors the DaoState with using a hash fo the complete daoState and make it accessible to the network for

```suggestion
 * Monitors the DaoState with using a hash fo the complete daoState and make it accessible to the network
```

> +import java.util.List;
+import java.util.Optional;
+import java.util.Random;
+import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.stream.Collectors;
+
+import lombok.Getter;
+import lombok.extern.slf4j.Slf4j;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+/**
+ * Monitors the DaoState with using a hash fo the complete daoState and make it accessible to the network for
+ * so we can detect quickly if any consensus issue arise. The data does not contain any private user
+ * data so sharing it on demand has no privacy concerns.

Could this be an attack vector, allowing too many requests as a DOS?

> +        daoStateNetworkService.addListener(this);
+    }
+
+    @Override
+    public void start() {
+    }
+
+
+    ///////////////////////////////////////////////////////////////////////////////////////////
+    // DaoStateListener
+    ///////////////////////////////////////////////////////////////////////////////////////////
+
+    @Override
+    public void onParseBlockChainComplete() {
+        parseBlockChainComplete = true;
+        daoStateNetworkService.addListeners();

Won't this add a listener every time a snapshot is reapplied. So after reorgs there would be duplicate listeners?

> +        //TODO handle reorgs TODO need to start from gen
+
+        byte[] prevHash;
+        int height = block.getHeight();
+        if (daoStateBlockchain.isEmpty()) {
+            // Only at genesis we allow an empty prevHash
+            if (height == genesisTxInfo.getGenesisBlockHeight()) {
+                prevHash = new byte[0];
+            } else {
+                log.warn("DaoStateBlockchain is empty but we received the block which was not the genesis block. " +
+                        "We stop execution here.");
+                return;
+            }
+        } else {
+            // TODO check if in reorg cases it might be a valid case
+            checkArgument(height > daoStateBlockchain.getLast().getBlockHeight(),

Shouldn't this check be `height == daoStateBlockchain.getLast().getBlockHeight() + 1`

> +        // match as well.
+        byte[] combined = ArrayUtils.addAll(prevHash, stateHash);
+        byte[] hash = Hash.getRipemd160hash(combined);
+
+        DaoStateHash myDaoStateHash = new DaoStateHash(height, hash, prevHash);
+        DaoStateBlock daoStateBlock = new DaoStateBlock(myDaoStateHash);
+        daoStateBlockchain.add(daoStateBlock);
+        listeners.forEach(Listener::onDaoStateBlockchainChanged);
+
+        log.info("Add daoStateBlock at processNewBlock:\n{}", daoStateBlock);
+
+        // We only broadcast after parsing of blockchain is complete
+        if (parseBlockChainComplete) {
+            // We delay broadcast to give peers enough time to have received the block.
+            // Otherwise they would ignore our data if received block is in future to their local blockchain.
+            int delayInSec = 1 + new Random().nextInt(5);

Is this a reasonable assumption, that most peers get the block within 1s of each other?

-- 
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/2532#pullrequestreview-213460845
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20190314/829af0f3/attachment.html>