[bisq-network/bisq] (3/4) [REFACTOR] P2PDataStore::add/remove/refresh (#3557)

[chimp1984]

chimp1984 commented on this pull request.



>  
-                    return true;
-                } else {
-                    log.warn("Publish date of payload is not matching our current time and outside of our tolerance.\n" +
-                            "Payload={}; now={}", payload.toString(), new Date());
-                    return false;
-                }
-            } else {
-                log.trace("We have that payload already in our map.");
-                return false;
-            }
-        } else {
+        // Payload hash size does not match expectation for that type of message.
+        if (!payload.verifyHashSize()) {
             log.warn("We got a hash exceeding our permitted size");
             return false;

I think we do not check for exceeding, just if hash size is as expected.

> -        boolean result = sequenceNrValid &&
-                checkPublicKeys(protectedStorageEntry, true)
-                && checkSignature(protectedStorageEntry);
+        // TODO: Combine with hasSequenceNrIncreased check, but keep existing behavior for now
+        if(!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload) ) {
+            log.trace("addProtectedStorageEntry failed due to invalid sequence number");
+
+            return false;
+        }
+
+        // Verify the ProtectedStorageEntry is well formed and valid for the add operation
+        if (!checkPublicKeys(protectedStorageEntry, true) ||
+                !checkSignature(protectedStorageEntry)) {
+
+            log.trace("addProtectedStorageEntry failed due to invalid entry");
+            return false;

Maybe better to split that to 2 checks and more concrete log messages.

>  
         boolean containsKey = map.containsKey(hashOfPayload);
-        if (containsKey) {
-            result = result && checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload);
+
+        // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten
+        if (containsKey &&

> In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten
       
I think that comment is misleading. The checkIfStoredDataPubKeyMatchesNewDataPubKey methods just checks if the pubKey is the same and if the hash is matching. Hash collusion is not an issue as we use cryptographic hashes as keys. 

>  
         boolean containsKey = map.containsKey(hashOfPayload);
-        if (containsKey) {
-            result = result && checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload);
+
+        // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten
+        if (containsKey &&
+                !checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) {
+
+            log.trace("addProtectedStorageEntry failed due to hash collision");

See above comment...

>              return false;
         }
+
+        int sequenceNumber = refreshTTLMessage.getSequenceNumber();
+
+        // If we have seen a more recent operation for this payload, we ignore the current one
+        // TODO: I think we can return false here. All callers use the get() public API which increments the sequence number
+        // leaving only the onMessage() handler which doesn't look at the return value. It makes more intuitive sense that operations that don't
+        // change state return false.
+        if (sequenceNumberMap.containsKey(hashOfPayload) && sequenceNumberMap.get(hashOfPayload).sequenceNr == sequenceNumber) {

In OfferBookService it is used and would cause a error message in that case (thought I think its an impossible case). Have not followed further if that error message would cause more troubles but I think it would require more checks of all client usage code to be sure we can change return value.  Conceptually I agree. 

>  
-            if (!containsKey || hasSequenceNrIncreased) {
-                // At startup we don't have the item so we store it. At updates of the seq nr we store as well.
-                map.put(hashOfPayload, protectedStorageEntry);
-                hashMapChangedListeners.forEach(e -> e.onAdded(protectedStorageEntry));
-                // printData("after add");
-            } else {
-                log.trace("We got that version of the data already, so we don't store it.");
-            }
+        // If we have seen a more recent operation for this payload, we ignore the current one
+        // TODO: I think we can return false here. All callers use the get() public API which increments the sequence number

There is quite a bit of client code dealing with the return value. Would require more analysis if we can safely change it. Conceptually I agree.

-- 
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/3557#pullrequestreview-311469128
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191104/e0729802/attachment.html>