[BUGFIX] Don't try and remove() if addMailboxData() fails

Fix a bug where remove() was called in the addMailboxData()
failure path.

1. Sender's can't remove mailbox entries. Only
   the receiver can remove it so even if the previous add() failed and
   left partial state, the remove() can never succeed.

2. Even if the sender could remove, this path used remove() instead
   of removeMailboxData() so it wouldn't have succeed anyway.

This patch cleans up the failure path as well as adds a precondition
for the remove() function to ensure future callers don't use them for
ProtectedMailboxStorageEntrys.

p2p/src/main/java/bisq/network/p2p/P2PService.java

@@ -700,12 +700,12 @@ public void onBroadcastFailed(String errorMessage) {
                     };
                     boolean result = p2PDataStorage.addProtectedStorageEntry(protectedMailboxStorageEntry, networkNode.getNodeAddress(), listener, true);
                     if (!result) {
-                        //TODO remove and add again with a delay to ensure the data will be broadcasted
-                        // The p2PDataStorage.remove makes probably sense but need to be analysed more.
-                        // Don't change that if it is not 100% clear.
                         sendMailboxMessageListener.onFault("Data already exists in our local database");
-                        boolean removeResult = p2PDataStorage.remove(protectedMailboxStorageEntry, networkNode.getNodeAddress(), true);
-                        log.debug("remove result=" + removeResult);
+
+                        // This should only fail if there are concurrent calls to addProtectedStorageEntry with the
+                        // same ProtectedMailboxStorageEntry. This is an unexpected use case so if it happens we
+                        // want to see it, but it is not worth throwing an exception.
+                        log.error("Unexpected state: adding mailbox message that already exists.");
                     }
                 } catch (CryptoException e) {
                     log.error("Signing at getDataWithSignedSeqNr failed. That should never happen.");

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

@@ -93,6 +93,8 @@
 
 import javax.annotation.Nullable;
 
+import static com.google.common.base.Preconditions.checkArgument;
+
 @Slf4j
 public class P2PDataStorage implements MessageListener, ConnectionListener, PersistedDataHost {
     /**
@@ -475,6 +477,8 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage,
     public boolean remove(ProtectedStorageEntry protectedStorageEntry,
                           @Nullable NodeAddress sender,
                           boolean isDataOwner) {
+        checkArgument(!(protectedStorageEntry instanceof ProtectedMailboxStorageEntry), "Use removeMailboxData for ProtectedMailboxStorageEntry");
+
         ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload();
         ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload);
         boolean containsKey = map.containsKey(hashOfPayload);

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

@@ -1162,51 +1162,17 @@ public void remove_receiversPubKeyChanged() throws NoSuchAlgorithmException, Cry
         }
 
 
-        // XXXBUGXXX: The P2PService calls remove() instead of removeFromMailbox() in the addMailboxData() path.
-        // This test shows it will always fail even with a valid remove entry. Future work should be able to
-        // combine the remove paths in the same way the add() paths are combined. This will require deprecating
-        // the receiversPubKey field which is a duplicate of the ownerPubKey in the MailboxStoragePayload.
-        // More investigation is needed.
-        @Test
+        @Test(expected = IllegalArgumentException.class)
         public void remove_canCallWrongRemoveAndFail() throws CryptoException {
 
             ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);
             ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1);
 
             doProtectedStorageAddAndVerify(entryForAdd, true, true);
 
-            SavedTestState beforeState = new SavedTestState(this.testState, entryForRemove);
-
-            // Call remove(ProtectedStorageEntry) instead of removeFromMailbox(ProtectedMailboxStorageEntry) and verify
-            // it fails
-            boolean addResult = super.doRemove(entryForRemove);
-
-            if (!this.useMessageHandler)
-                Assert.assertFalse(addResult);
-
-            // should succeed with expectedStatechange==true when remove paths are combined
-            verifyProtectedStorageRemove(this.testState, beforeState, entryForRemove, false, this.expectIsDataOwner());
-        }
-
-        // TESTCASE: Verify misuse of the API (calling remove() instead of removeFromMailbox correctly errors with
-        // a payload that is valid for remove of a non-mailbox entry.
-        @Test
-        public void remove_canCallWrongRemoveAndFailInvalidPayload() throws CryptoException {
-
-            ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);
-
-            doProtectedStorageAddAndVerify(entryForAdd, true, true);
-
-            SavedTestState beforeState = new SavedTestState(this.testState, entryForAdd);
-
             // Call remove(ProtectedStorageEntry) instead of removeFromMailbox(ProtectedMailboxStorageEntry) and verify
-            // it fails with a payload that isn't signed by payload.ownerPubKey
-            boolean addResult = super.doRemove(entryForAdd);
-
-            if (!this.useMessageHandler)
-                Assert.assertFalse(addResult);
-
-            verifyProtectedStorageRemove(this.testState, beforeState, entryForAdd, false, this.expectIsDataOwner());
+            // it fails spectacularly
+            super.doRemove(entryForRemove);
         }
 
         // TESTCASE: Add after removed when add-once required (greater seq #)