[bisq-network/bisq] (5/6) Broadcast Remove messages seen before Adds (#3664)

[Julian Knutsen]

julianknutsen commented on this pull request.



> @@ -501,14 +501,14 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry,
 
         maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload);
 
-        // This means the RemoveData or RemoveMailboxData was seen prior to the AddData. We have already updated
-        // the SequenceNumberMap appropriately so the stale Add will not pass validation, but we don't want to
-        // signal listeners for state changes since no original state existed.
-        if (storedEntry == null)
-            return false;
-
-        // Valid remove entry, do the remove and signal listeners
-        removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload);
+        if (storedEntry != null) {
+            // Valid remove entry, do the remove and signal listeners
+            removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload);
+        } /* else {
+            // This means the RemoveData or RemoveMailboxData was seen prior to the AddData. We have already updated
+            // the SequenceNumberMap appropriately so the stale Add will not pass validation, but we still want to
+            // broadcast the remove to peers so they can update their state appropriately

The original change from true->false was in the equivalent sequence number case so it isn't related to this change.

In this case, the contract of the return value has stayed the same since it is based on the internal state changing. Previously, nothing changed so we returned false, but now that the SequenceNumberMap is changed in some cases it needs to return true.

In practice, this branch of the code is not relevant for return values. The `onMessage()` handler is the only caller we expect to receive `RemoveData` messages before `AddData` messages since the race is related to when the connections happen in relation to the original add/removes. It doesn't even look at the return values.

The P2PService caller is the second user and will only remove `ProtectedStorageEntry` items that it knows about which means they must have existed locally so the `remove()` call will always return true. Personally, I would like to add asserts around this to guarantee it, but they are risky for this type of legacy code where there are strange interactions I am not familiar with.

I also don't like returning `false` here in the add-remove-case because there is a guarantee being added "all future adds older than this operations will fail" and returning false is confusing because it makes it seem like nothing happened.

-- 
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/3664#discussion_r350298335
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191125/58de3753/attachment.html>