Implement remove-before-add message sequence behavior

It is possible to receive a RemoveData or RemoveMailboxData message
before the relevant AddData, but the current code does not handle
it.

This results in internal state updates and signal handler's being called
when an Add is received with a lower sequence number than a previously
seen Remove.

Minor test validation changes to allow tests to specify that only the
SequenceNumberMap should be written during an operation.

p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java

@@ -482,13 +482,6 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers
         ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload();
         ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload);
 
-        // If we don't know about the target of this remove, ignore it
-        ProtectedStorageEntry storedEntry = map.get(hashOfPayload);
-        if (storedEntry == null) {
-            log.debug("Remove data ignored as we don't have an entry for that data.");
-            return false;
-        }
-
         // If we have seen a more recent operation for this payload, ignore this one
         if (!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload))
             return false;
@@ -498,19 +491,26 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers
             return false;
 
         // If we have already seen an Entry with the same hash, verify the metadata is the same
-        if (!protectedStorageEntry.matchesRelevantPubKey(storedEntry))
+        ProtectedStorageEntry storedEntry = map.get(hashOfPayload);
+        if (storedEntry != null && !protectedStorageEntry.matchesRelevantPubKey(storedEntry))
             return false;
 
-        // Valid remove entry, do the remove and signal listeners
-        removeFromMapAndDataStore(protectedStorageEntry, hashOfPayload);
-        printData("after remove");
-
         // Record the latest sequence number and persist it
         sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), this.clock.millis()));
         sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300);
 
         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);
+        printData("after remove");
+
         if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) {
             broadcast(new RemoveMailboxDataMessage((ProtectedMailboxStorageEntry) protectedStorageEntry), sender, null, isDataOwner);
         } else {

p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java

@@ -188,7 +188,7 @@ public class P2PDataStorageClientAPITest {
         SavedTestState beforeState = this.testState.saveTestState(protectedMailboxStorageEntry);
         Assert.assertFalse(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, TestState.getTestNodeAddress(), true));
 
-        this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, true, false, true);
+        this.testState.verifyProtectedStorageRemove(beforeState, protectedMailboxStorageEntry, false, true, true, true);
     }
 
     // TESTCASE: Adding, then removing a mailbox message from the getMailboxDataWithSignedSeqNr API

p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java

@@ -204,6 +204,14 @@ public class P2PDataStorageProtectedStorageEntryTest {
                                                boolean expectedReturnValue,
                                                boolean expectInternalStateChange) {
 
+            doProtectedStorageRemoveAndVerify(entry, expectedReturnValue, expectInternalStateChange, expectInternalStateChange);
+        }
+
+        void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry,
+                                               boolean expectedReturnValue,
+                                               boolean expectInternalStateChange,
+                                               boolean expectSeqNrWrite) {
+
             SavedTestState beforeState = this.testState.saveTestState(entry);
 
             boolean addResult = this.doRemove(entry);
@@ -211,7 +219,7 @@ public class P2PDataStorageProtectedStorageEntryTest {
             if (!this.useMessageHandler)
                 Assert.assertEquals(expectedReturnValue, addResult);
 
-            this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, true, expectInternalStateChange, this.expectIsDataOwner());
+            this.testState.verifyProtectedStorageRemove(beforeState, entry, expectInternalStateChange, true, expectSeqNrWrite, this.expectIsDataOwner());
         }
 
         /// Valid Add Tests (isValidForAdd() and matchesRelevantPubKey() return true)
@@ -325,12 +333,12 @@ public class P2PDataStorageProtectedStorageEntryTest {
             doProtectedStorageRemoveAndVerify(entryForRemove, true, true);
         }
 
-        // TESTCASE: Removing an item before it was added
+        // TESTCASE: Removing an item before it was added. This triggers a SequenceNumberMap write, but nothing else
         @Test
         public void remove_notExists() {
             ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1);
 
-            doProtectedStorageRemoveAndVerify(entryForRemove, false, false);
+            doProtectedStorageRemoveAndVerify(entryForRemove, false, false, true);
         }
 
         // TESTCASE: Removing an item after successfully adding (remove seq # < add seq #)
@@ -415,17 +423,35 @@ public class P2PDataStorageProtectedStorageEntryTest {
         }
 
         // TESTCASE: Received remove for nonexistent item that was later received
-        // XXXBUGXXX: There may be cases where removes are reordered with adds (remove during pending GetDataRequest?).
-        // The proper behavior may be to not add the late messages, but the current code will successfully add them
-        // even in the AddOncePayload (mailbox) case.
         @Test
         public void remove_lateAdd() {
             ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);
             ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2);
 
-            doProtectedStorageRemoveAndVerify(entryForRemove, false, false);
+            this.doRemove(entryForRemove);
+
+            doProtectedStorageAddAndVerify(entryForAdd, false, false);
+        }
+
+        // TESTCASE: Invalid remove doesn't block a valid add (isValidForRemove == false | matchesRelevantPubKey == false)
+        @Test
+        public void remove_entryNotIsValidForRemoveDoesntBlockAdd1() {
+            ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);
+            ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1, false, false);
+
+            this.doRemove(entryForRemove);
+
+            doProtectedStorageAddAndVerify(entryForAdd, true, true);
+        }
+
+        // TESTCASE: Invalid remove doesn't block a valid add (isValidForRemove == false | matchesRelevantPubKey == true)
+        @Test
+        public void remove_entryNotIsValidForRemoveDoesntBlockAdd2() {
+            ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);
+            ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1, false, true);
+
+            this.doRemove(entryForRemove);
 
-            // should be (false, false)
             doProtectedStorageAddAndVerify(entryForAdd, true, true);
         }
     }