From 372c26de74e2b45d0fcc921d32a51f37756eac04 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 19 Nov 2019 16:22:53 -0800 Subject: [PATCH] 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. --- .../network/p2p/storage/P2PDataStorage.java | 24 +++++----- .../storage/P2PDataStorageClientAPITest.java | 2 +- ...PDataStorageProtectedStorageEntryTest.java | 44 +++++++++++++++---- 3 files changed, 48 insertions(+), 22 deletions(-) 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 fc5a2b1d19..01023019b7 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/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 { diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java index 4783cb2581..867cbea229 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageClientAPITest.java +++ b/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 diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java index 3475558610..3d39762e6b 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/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); } } @@ -584,7 +610,7 @@ public class P2PDataStorageProtectedStorageEntryTest { Map beforeRestart = this.testState.mockedStorage.getMap(); this.testState.simulateRestart(); - + Assert.assertEquals(beforeRestart, this.testState.mockedStorage.getMap()); } }