From de5ffd43e3742e541cbe9130edaaec382a482fa0 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 09:09:47 -0800 Subject: [PATCH] [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. --- .../java/bisq/network/p2p/P2PService.java | 10 ++--- .../network/p2p/storage/P2PDataStorage.java | 4 ++ .../p2p/storage/P2PDataStorageTest.java | 40 ++----------------- 3 files changed, 12 insertions(+), 42 deletions(-) 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 #)