Merge pull request bisq-network#3556 from julianknutsen/fix-remove-bug

(2/8) [BUGFIX] Don't try and remove() if addMailboxData() fails
julianknutsen · Nov 18, 2019 · 60f02e9 · 60f02e9
2 parents 5fe71fa + de5ffd4
commit 60f02e9
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 42 deletions.
diff --git a/p2p/src/main/java/bisq/network/p2p/P2PService.java b/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.");

diff --git a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java b/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);

diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java b/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 #)