diff --git a/p2p/src/main/java/bisq/network/p2p/P2PService.java b/p2p/src/main/java/bisq/network/p2p/P2PService.java index f0a1fcddb75..f900a88c7ae 100644 --- 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 index 1d911896025..3ecc098b686 100644 --- 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 index 41964e2715c..51b8879c3ae 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -1162,12 +1162,7 @@ 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); @@ -1175,38 +1170,9 @@ public void remove_canCallWrongRemoveAndFail() throws CryptoException { 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 #)