From 3d571c4ca30693f59cb2b3fc2aa81103fec23b00 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 22 Nov 2019 15:05:17 -0800 Subject: [PATCH] [BUGFIX] Fix duplicate sequence number use case (startup) Fix a bug introduced in d484617385276d033223ad8f36e821504e116f96 that did not properly handle a valid use case for duplicate sequence numbers. For in-memory-only ProtectedStoragePayloads, the client nodes need a way to reconstruct the Payloads after startup from peer and seed nodes. This involves sending a ProtectedStorageEntry with a sequence number that is equal to the last one the client had already seen. This patch adds tests to confirm the bug and fix as well as the changes necessary to allow adding of Payloads that were previously seen, but removed during a restart. --- .../network/p2p/storage/P2PDataStorage.java | 18 +++++++-- ...PDataStorageProtectedStorageEntryTest.java | 40 +++++++++++++++++++ .../P2PDataStorageRemoveExpiredTest.java | 10 ----- 3 files changed, 54 insertions(+), 14 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 0aae51e3e84..9e2f296dfb6 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -390,16 +390,26 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn return false; } - // If we have seen a more recent operation for this payload, we ignore the current one - if(!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + + // If we have seen a more recent operation for this payload and we have a payload locally, ignore it + if (storedEntry != null && + !hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) { return false; + } + + // We want to allow add operations for equal sequence numbers if we don't have the payload locally. This is + // the case for non-persistent Payloads that need to be reconstructed from peer and seed nodes each startup. + MapValue sequenceNumberMapValue = sequenceNumberMap.get(hashOfPayload); + if (sequenceNumberMapValue != null && + protectedStorageEntry.getSequenceNumber() < sequenceNumberMapValue.sequenceNr) { + return false; + } // Verify the ProtectedStorageEntry is well formed and valid for the add operation if (!protectedStorageEntry.isValidForAddOperation()) return false; - ProtectedStorageEntry storedEntry = map.get(hashOfPayload); - // If we have already seen an Entry with the same hash, verify the metadata is equal if (storedEntry != null && !protectedStorageEntry.matchesRelevantPubKey(storedEntry)) return false; 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 e457fd6c84f..9f5bd539234 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageProtectedStorageEntryTest.java @@ -576,6 +576,34 @@ public void refreshTTL_refreshEntryOwnerOriginalOwnerMismatch() throws CryptoExc KeyPair notOwner = TestUtils.generateKeyPair(); doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, notOwner, 2), false, false); } + + // TESTCASE: After restart, identical sequence numbers are accepted ONCE. We need a way to reconstruct + // in-memory ProtectedStorageEntrys from seed and peer nodes around startup time. + @Test + public void addProtectedStorageEntry_afterRestartCanAddDuplicateSeqNr() { + ProtectedStorageEntry toAdd1 = this.getProtectedStorageEntryForAdd(1); + doProtectedStorageAddAndVerify(toAdd1, true, true); + + this.testState.simulateRestart(); + + // Can add equal seqNr only once + doProtectedStorageAddAndVerify(toAdd1, true, true); + + // Can't add equal seqNr twice + doProtectedStorageAddAndVerify(toAdd1, false, false); + } + + // TESTCASE: After restart, old sequence numbers are not accepted + @Test + public void addProtectedStorageEntry_afterRestartCanNotAddLowerSeqNr() { + ProtectedStorageEntry toAdd1 = this.getProtectedStorageEntryForAdd(1); + ProtectedStorageEntry toAdd2 = this.getProtectedStorageEntryForAdd(2); + doProtectedStorageAddAndVerify(toAdd2, true, true); + + this.testState.simulateRestart(); + + doProtectedStorageAddAndVerify(toAdd1, false, false); + } } /** @@ -608,6 +636,18 @@ public void addProtectedStorageEntry_afterReadFromResourcesWithDuplicate_3629Reg Assert.assertEquals(beforeRestart, this.testState.mockedStorage.getMap()); } + + // TESTCASE: After restart, identical sequence numbers are not accepted for persistent payloads + @Test + public void addProtectedStorageEntry_afterRestartCanNotAddDuplicateSeqNr() { + ProtectedStorageEntry toAdd1 = this.getProtectedStorageEntryForAdd(1); + doProtectedStorageAddAndVerify(toAdd1, true, true); + + this.testState.simulateRestart(); + + // Can add equal seqNr only once + doProtectedStorageAddAndVerify(toAdd1, false, false); + } } /** diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java index f365abdf7d9..454799ed515 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageRemoveExpiredTest.java @@ -188,15 +188,5 @@ public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchA SavedTestState beforeState = this.testState.saveTestState(purgedProtectedStorageEntry); this.testState.mockedStorage.removeExpiredEntries(); this.testState.verifyProtectedStorageRemove(beforeState, expectedRemoves, true, true, false, false, false); - - // Which means that an addition of a purged entry should succeed. - beforeState = this.testState.saveTestState(purgedProtectedStorageEntry); - Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, TestState.getTestNodeAddress(), null, false)); - this.testState.verifyProtectedStorageAdd(beforeState, purgedProtectedStorageEntry, true, false); - - // The last entry (5 days old) should still exist in the SequenceNumberMap which means trying to add it again should fail. - beforeState = this.testState.saveTestState(keepProtectedStorageEntry); - Assert.assertFalse(this.testState.mockedStorage.addProtectedStorageEntry(keepProtectedStorageEntry, TestState.getTestNodeAddress(), null, false)); - this.testState.verifyProtectedStorageAdd(beforeState, keepProtectedStorageEntry, false, false); } }