From de5ffd43e3742e541cbe9130edaaec382a482fa0 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 09:09:47 -0800 Subject: [PATCH 01/17] [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 #) From 5512f34566418f4b273becdbf4387348666a1dd7 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sat, 26 Oct 2019 00:06:34 +0000 Subject: [PATCH 02/17] [REFACTOR] P2PDataStorage::addPersistableNetworkPayload() Add comments and refactor the body in order to make it easier to understand. --- .../network/p2p/storage/P2PDataStorage.java | 54 +++++++++++-------- 1 file changed, 31 insertions(+), 23 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 3ecc098b686..6d587bf7e7f 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -316,33 +316,41 @@ public boolean addPersistableNetworkPayload(PersistableNetworkPayload payload, boolean reBroadcast, boolean checkDate) { log.trace("addPersistableNetworkPayload payload={}", payload); - byte[] hash = payload.getHash(); - if (payload.verifyHashSize()) { - ByteArray hashAsByteArray = new ByteArray(hash); - boolean containsKey = getAppendOnlyDataStoreMap().containsKey(hashAsByteArray); - if (!containsKey || reBroadcast) { - if (!(payload instanceof DateTolerantPayload) || !checkDate || ((DateTolerantPayload) payload).isDateInTolerance(clock)) { - if (!containsKey) { - appendOnlyDataStoreService.put(hashAsByteArray, payload); - appendOnlyDataStoreListeners.forEach(e -> e.onAdded(payload)); - } - if (allowBroadcast) - broadcaster.broadcast(new AddPersistableNetworkPayloadMessage(payload), sender, null, isDataOwner); - return true; - } else { - log.warn("Publish date of payload is not matching our current time and outside of our tolerance.\n" + - "Payload={}; now={}", payload.toString(), new Date()); - return false; - } - } else { - log.trace("We have that payload already in our map."); - return false; - } - } else { + // Payload hash size does not match expectation for that type of message. + if (!payload.verifyHashSize()) { log.warn("We got a hash exceeding our permitted size"); return false; } + + ByteArray hashAsByteArray = new ByteArray(payload.getHash()); + boolean payloadHashAlreadyInStore = getAppendOnlyDataStoreMap().containsKey(hashAsByteArray); + + // Store already knows about this payload. Ignore it unless the caller specifically requests a republish. + if (payloadHashAlreadyInStore && !reBroadcast) { + log.trace("We have that payload already in our map."); + return false; + } + + // DateTolerantPayloads are only checked for tolerance from the onMessage handler (checkDate == true). If not in + // tolerance, ignore it. + if (checkDate && payload instanceof DateTolerantPayload && !((DateTolerantPayload) payload).isDateInTolerance((clock))) { + log.warn("Publish date of payload is not matching our current time and outside of our tolerance.\n" + + "Payload={}; now={}", payload.toString(), new Date()); + return false; + } + + // Add the payload and publish the state update to the appendOnlyDataStoreListeners + if (!payloadHashAlreadyInStore) { + appendOnlyDataStoreService.put(hashAsByteArray, payload); + appendOnlyDataStoreListeners.forEach(e -> e.onAdded(payload)); + } + + // Broadcast the payload if requested by caller + if (allowBroadcast) + broadcaster.broadcast(new AddPersistableNetworkPayloadMessage(payload), sender, null, isDataOwner); + + return true; } // When we receive initial data we skip several checks to improve performance. We requested only missing entries so we From f2f6399cac697159f4cba7871fa3e9c39a1e3a9e Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 17:46:12 -0800 Subject: [PATCH 03/17] [REFACTOR] P2PDataStorage::addProtectedStorageEntry() Refactor addProtectedStorageEntry for more readability and add comments to help future readers. --- .../network/p2p/storage/P2PDataStorage.java | 84 +++++++++++-------- 1 file changed, 49 insertions(+), 35 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 6d587bf7e7f..7aaae82fc91 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -390,50 +390,64 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn return false; } - boolean sequenceNrValid = isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload); - boolean result = sequenceNrValid && - checkPublicKeys(protectedStorageEntry, true) - && checkSignature(protectedStorageEntry); + // TODO: Combine with hasSequenceNrIncreased check, but keep existing behavior for now + if(!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload) ) { + log.trace("addProtectedStorageEntry failed due to invalid sequence number"); + + return false; + } + + // Verify the ProtectedStorageEntry is well formed and valid for the add operation + if (!checkPublicKeys(protectedStorageEntry, true) || + !checkSignature(protectedStorageEntry)) { + + log.trace("addProtectedStorageEntry failed due to invalid entry"); + return false; + } boolean containsKey = map.containsKey(hashOfPayload); - if (containsKey) { - result = result && checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload); + + // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten + if (containsKey && + !checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) { + + log.trace("addProtectedStorageEntry failed due to hash collision"); + return false; } - // printData("before add"); - if (result) { - boolean hasSequenceNrIncreased = hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload); + boolean hasSequenceNrIncreased = hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload); - if (!containsKey || hasSequenceNrIncreased) { - // At startup we don't have the item so we store it. At updates of the seq nr we store as well. - map.put(hashOfPayload, protectedStorageEntry); - hashMapChangedListeners.forEach(e -> e.onAdded(protectedStorageEntry)); - // printData("after add"); - } else { - log.trace("We got that version of the data already, so we don't store it."); - } + // If we have seen a more recent operation for this payload, we ignore the current one + // TODO: I think we can return false here. All callers use the Client API (addProtectedStorageEntry(getProtectedStorageEntry()) + // leaving only the onMessage() handler which doesn't look at the return value. It makes more intuitive sense that adds() that don't + // change state return false. + if (!hasSequenceNrIncreased) { + log.trace("addProtectedStorageEntry failed due to old sequence number"); + + return true; + } - if (hasSequenceNrIncreased) { - sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), System.currentTimeMillis())); - // We set the delay higher as we might receive a batch of items - sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 2000); + // This is an updated entry. Record it and signal listeners. + map.put(hashOfPayload, protectedStorageEntry); + hashMapChangedListeners.forEach(e -> e.onAdded(protectedStorageEntry)); - if (allowBroadcast) - broadcastProtectedStorageEntry(protectedStorageEntry, sender, listener, isDataOwner); - } else { - log.trace("We got that version of the data already, so we don't broadcast it."); - } + // Record the updated sequence number and persist it. Higher delay so we can batch more items. + sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), System.currentTimeMillis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 2000); - if (protectedStoragePayload instanceof PersistablePayload) { - ByteArray compactHash = getCompactHashAsByteArray(protectedStoragePayload); - ProtectedStorageEntry previous = protectedDataStoreService.putIfAbsent(compactHash, protectedStorageEntry); - if (previous == null) - protectedDataStoreListeners.forEach(e -> e.onAdded(protectedStorageEntry)); - } - } else { - log.trace("add failed"); + // Optionally, broadcast the add/update depending on the calling environment + if (allowBroadcast) + broadcastProtectedStorageEntry(protectedStorageEntry, sender, listener, isDataOwner); + + // Persist ProtectedStorageEntrys carrying PersistablePayload payloads and signal listeners on changes + if (protectedStoragePayload instanceof PersistablePayload) { + ByteArray compactHash = P2PDataStorage.getCompactHashAsByteArray(protectedStoragePayload); + ProtectedStorageEntry previous = protectedDataStoreService.putIfAbsent(compactHash, protectedStorageEntry); + if (previous == null) + protectedDataStoreListeners.forEach(e -> e.onAdded(protectedStorageEntry)); } - return result; + + return true; } private void broadcastProtectedStorageEntry(ProtectedStorageEntry protectedStorageEntry, From ae502709eeaf6661cb61fbdc6dc5d583aff1271c Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 18:18:20 -0800 Subject: [PATCH 04/17] [REFACTOR] P2PDataStorage::refreshTTL() Refactor for readability and add comments for future readers. --- .../network/p2p/storage/P2PDataStorage.java | 83 ++++++++++++------- 1 file changed, 54 insertions(+), 29 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 7aaae82fc91..c1070941310 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -461,39 +461,64 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, @Nullable NodeAddress sender, boolean isDataOwner) { ByteArray hashOfPayload = new ByteArray(refreshTTLMessage.getHashOfPayload()); - if (map.containsKey(hashOfPayload)) { - ProtectedStorageEntry storedData = map.get(hashOfPayload); - int sequenceNumber = refreshTTLMessage.getSequenceNumber(); - if (sequenceNumberMap.containsKey(hashOfPayload) && sequenceNumberMap.get(hashOfPayload).sequenceNr == sequenceNumber) { - log.trace("We got that message with that seq nr already from another peer. We ignore that message."); - return true; - } else { - PublicKey ownerPubKey = storedData.getProtectedStoragePayload().getOwnerPubKey(); - byte[] hashOfDataAndSeqNr = refreshTTLMessage.getHashOfDataAndSeqNr(); - byte[] signature = refreshTTLMessage.getSignature(); - // printData("before refreshTTL"); - if (hasSequenceNrIncreased(sequenceNumber, hashOfPayload) && - checkIfStoredDataPubKeyMatchesNewDataPubKey(ownerPubKey, hashOfPayload) && - checkSignature(ownerPubKey, hashOfDataAndSeqNr, signature)) { - log.debug("refreshDate called for storedData:\n\t" + StringUtils.abbreviate(storedData.toString(), 100)); - storedData.refreshTTL(); - storedData.updateSequenceNumber(sequenceNumber); - storedData.updateSignature(signature); - printData("after refreshTTL"); - sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, System.currentTimeMillis())); - sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 1000); - - broadcast(refreshTTLMessage, sender, null, isDataOwner); - return true; - } - - return false; - } - } else { + if (!map.containsKey((hashOfPayload))) { log.debug("We don't have data for that refresh message in our map. That is expected if we missed the data publishing."); + + return false; + } + + int sequenceNumber = refreshTTLMessage.getSequenceNumber(); + + // If we have seen a more recent operation for this payload, we ignore the current one + // TODO: I think we can return false here. All callers use the Client API (refreshTTL(getRefreshTTLMessage()) which increments the sequence number + // leaving only the onMessage() handler which doesn't look at the return value. It makes more intuitive sense that operations that don't + // change state return false. + if (sequenceNumberMap.containsKey(hashOfPayload) && sequenceNumberMap.get(hashOfPayload).sequenceNr == sequenceNumber) { + log.trace("We got that message with that seq nr already from another peer. We ignore that message."); + + return true; + } + + // TODO: Combine with above in future work, but preserve existing behavior for now + if(!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) { + return false; + } + + ProtectedStorageEntry storedData = map.get(hashOfPayload); + PublicKey ownerPubKey = storedData.getProtectedStoragePayload().getOwnerPubKey(); + byte[] hashOfDataAndSeqNr = refreshTTLMessage.getHashOfDataAndSeqNr(); + byte[] signature = refreshTTLMessage.getSignature(); + + // Verify the RefreshOfferMessage is well formed and valid for the refresh operation + if (!checkSignature(ownerPubKey, hashOfDataAndSeqNr, signature)) { + log.trace("refreshTTL failed due to invalid entry"); + + return false; + } + + // In a hash collision between two well formed RefreshOfferMessage, the first item wins and will not be overwritten + if (!checkIfStoredDataPubKeyMatchesNewDataPubKey(ownerPubKey, hashOfPayload)) { + log.trace("refreshTTL failed due to hash collision"); + return false; } + + // This is a valid refresh, update the payload for it + log.debug("refreshDate called for storedData:\n\t" + StringUtils.abbreviate(storedData.toString(), 100)); + storedData.refreshTTL(); + storedData.updateSequenceNumber(sequenceNumber); + storedData.updateSignature(signature); + printData("after refreshTTL"); + + // Record the latest sequence number and persist it + sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, System.currentTimeMillis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 1000); + + // Always broadcast refreshes + broadcast(refreshTTLMessage, sender, null, isDataOwner); + + return true; } public boolean remove(ProtectedStorageEntry protectedStorageEntry, From a569852524410723ee2d81c1059521b2bd424f26 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 18:28:33 -0800 Subject: [PATCH 05/17] [REFACTOR] P2PDataStorage::remove() Refactor for readability and add comments to help future readers. --- .../network/p2p/storage/P2PDataStorage.java | 60 ++++++++++++------- 1 file changed, 40 insertions(+), 20 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 c1070941310..843f44ae10d 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -528,32 +528,52 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload); - boolean containsKey = map.containsKey(hashOfPayload); - if (!containsKey) + + // If we don't know about the target of this remove, ignore it + if (!map.containsKey(hashOfPayload)) { log.debug("Remove data ignored as we don't have an entry for that data."); - boolean result = containsKey - && checkPublicKeys(protectedStorageEntry, false) - && isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload) - && checkSignature(protectedStorageEntry) - && checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload); - // printData("before remove"); - if (result) { - doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload); - printData("after remove"); - sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), System.currentTimeMillis())); - sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); + return false; + } - maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); + // If we have seen a more recent operation for this payload, ignore this one + if (!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) { + log.trace("remove failed due to old sequence number"); - broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner); + return false; + } - removeFromProtectedDataStore(protectedStorageEntry); - } else { - log.debug("remove failed"); + // Verify the ProtectedStorageEntry is well formed and valid for the remove operation + if (!checkPublicKeys(protectedStorageEntry, false) || + !checkSignature(protectedStorageEntry)) { + log.trace("remove failed due to invalid entry"); + + return false; } - return result; - } + + // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten + if (!checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) { + log.trace("remove failed due to hash collision"); + + return false; + } + + // Valid remove entry, do the remove and signal listeners + doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload); + printData("after remove"); + + // Record the latest sequence number and persist it + sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), System.currentTimeMillis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); + + maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); + + broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner); + + removeFromProtectedDataStore(protectedStorageEntry); + + return true; +} /** From 66e3ece63e105b03366a9a5517063df346cd8db7 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 18:42:12 -0800 Subject: [PATCH 06/17] [REFACTOR] P2PDataStorage::removeMailboxData() Refactor for readability and add comments for future readers. --- .../network/p2p/storage/P2PDataStorage.java | 64 ++++++++++++------- 1 file changed, 41 insertions(+), 23 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 843f44ae10d..81cbe242e19 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -626,33 +626,51 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt boolean isDataOwner) { ProtectedStoragePayload protectedStoragePayload = protectedMailboxStorageEntry.getProtectedStoragePayload(); ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload); - boolean containsKey = map.containsKey(hashOfPayload); - if (!containsKey) - log.debug("Remove data ignored as we don't have an entry for that data."); + + if (!map.containsKey(hashOfPayload)) { + log.debug("removeMailboxData failed due to unknown entry"); + + return false; + } int sequenceNumber = protectedMailboxStorageEntry.getSequenceNumber(); + + if (!isSequenceNrValid(sequenceNumber, hashOfPayload)) { + log.trace("removeMailboxData failed due to old sequence number"); + + return false; + } + PublicKey receiversPubKey = protectedMailboxStorageEntry.getReceiversPubKey(); - boolean result = containsKey && - isSequenceNrValid(sequenceNumber, hashOfPayload) && - checkPublicKeys(protectedMailboxStorageEntry, false) && - protectedMailboxStorageEntry.getMailboxStoragePayload().getOwnerPubKey().equals(receiversPubKey) && // at remove both keys are the same (only receiver is able to remove data) - checkSignature(protectedMailboxStorageEntry) && - checkIfStoredMailboxDataMatchesNewMailboxData(receiversPubKey, hashOfPayload); - - // printData("before removeMailboxData"); - if (result) { - doRemoveProtectedExpirableData(protectedMailboxStorageEntry, hashOfPayload); - printData("after removeMailboxData"); - sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, System.currentTimeMillis())); - sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); - - maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); - - broadcast(new RemoveMailboxDataMessage(protectedMailboxStorageEntry), sender, null, isDataOwner); - } else { - log.debug("removeMailboxData failed"); + + if (!checkPublicKeys(protectedMailboxStorageEntry, false) || + !protectedMailboxStorageEntry.getMailboxStoragePayload().getOwnerPubKey().equals(receiversPubKey) || // at remove both keys are the same (only receiver is able to remove data) + !checkSignature(protectedMailboxStorageEntry)) { + log.trace("removeMailboxData failed due to invalid entry"); + + return false; } - return result; + + // In a hash collision between two well formed ProtectedMailboxStorageEntry, the first item wins and will not be overwritten + if (!checkIfStoredMailboxDataMatchesNewMailboxData(receiversPubKey, hashOfPayload)) { + log.trace("removeMailboxData failed due to hash collision"); + + return false; + } + + // Valid remove ProtectedMailboxStorageEntry, do the remove and signal listeners + doRemoveProtectedExpirableData(protectedMailboxStorageEntry, hashOfPayload); + printData("after removeMailboxData"); + + // Record the latest sequence number and persist it + sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, System.currentTimeMillis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); + + maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); + + broadcast(new RemoveMailboxDataMessage(protectedMailboxStorageEntry), sender, null, isDataOwner); + + return true; } private void maybeAddToRemoveAddOncePayloads(ProtectedStoragePayload protectedStoragePayload, From 86c8c839d14b906321e2247dbf1f1a04a04cb835 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 5 Nov 2019 15:45:31 -0800 Subject: [PATCH 07/17] [PR COMMENTS] Clean up logging messages Removed duplicate log messages that are handled inside the various helper methods and print more verbose state useful for debugging. Updated potentially misleading comments around hashing collisions --- .../network/p2p/storage/P2PDataStorage.java | 85 ++++++------------- 1 file changed, 25 insertions(+), 60 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 81cbe242e19..c1f54bb4297 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -319,7 +319,7 @@ public boolean addPersistableNetworkPayload(PersistableNetworkPayload payload, // Payload hash size does not match expectation for that type of message. if (!payload.verifyHashSize()) { - log.warn("We got a hash exceeding our permitted size"); + log.warn("addPersistableNetworkPayload failed due to unexpected hash size"); return false; } @@ -328,14 +328,14 @@ public boolean addPersistableNetworkPayload(PersistableNetworkPayload payload, // Store already knows about this payload. Ignore it unless the caller specifically requests a republish. if (payloadHashAlreadyInStore && !reBroadcast) { - log.trace("We have that payload already in our map."); + log.trace("addPersistableNetworkPayload failed due to duplicate payload"); return false; } // DateTolerantPayloads are only checked for tolerance from the onMessage handler (checkDate == true). If not in // tolerance, ignore it. if (checkDate && payload instanceof DateTolerantPayload && !((DateTolerantPayload) payload).isDateInTolerance((clock))) { - log.warn("Publish date of payload is not matching our current time and outside of our tolerance.\n" + + log.warn("addPersistableNetworkPayload failed due to payload time outside tolerance.\n" + "Payload={}; now={}", payload.toString(), new Date()); return false; } @@ -391,29 +391,18 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn } // TODO: Combine with hasSequenceNrIncreased check, but keep existing behavior for now - if(!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload) ) { - log.trace("addProtectedStorageEntry failed due to invalid sequence number"); - + if(!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) return false; - } // Verify the ProtectedStorageEntry is well formed and valid for the add operation - if (!checkPublicKeys(protectedStorageEntry, true) || - !checkSignature(protectedStorageEntry)) { - - log.trace("addProtectedStorageEntry failed due to invalid entry"); + if (!checkPublicKeys(protectedStorageEntry, true) || !checkSignature(protectedStorageEntry)) return false; - } boolean containsKey = map.containsKey(hashOfPayload); - // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten - if (containsKey && - !checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) { - - log.trace("addProtectedStorageEntry failed due to hash collision"); + // If we have already seen an Entry with the same hash, verify the new Entry has the same owner + if (containsKey && !checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) return false; - } boolean hasSequenceNrIncreased = hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload); @@ -421,11 +410,8 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn // TODO: I think we can return false here. All callers use the Client API (addProtectedStorageEntry(getProtectedStorageEntry()) // leaving only the onMessage() handler which doesn't look at the return value. It makes more intuitive sense that adds() that don't // change state return false. - if (!hasSequenceNrIncreased) { - log.trace("addProtectedStorageEntry failed due to old sequence number"); - + if (!hasSequenceNrIncreased) return true; - } // This is an updated entry. Record it and signal listeners. map.put(hashOfPayload, protectedStorageEntry); @@ -481,9 +467,8 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, } // TODO: Combine with above in future work, but preserve existing behavior for now - if(!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) { + if(!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) return false; - } ProtectedStorageEntry storedData = map.get(hashOfPayload); PublicKey ownerPubKey = storedData.getProtectedStoragePayload().getOwnerPubKey(); @@ -491,18 +476,13 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, byte[] signature = refreshTTLMessage.getSignature(); // Verify the RefreshOfferMessage is well formed and valid for the refresh operation - if (!checkSignature(ownerPubKey, hashOfDataAndSeqNr, signature)) { - log.trace("refreshTTL failed due to invalid entry"); - + if (!checkSignature(ownerPubKey, hashOfDataAndSeqNr, signature)) return false; - } - - // In a hash collision between two well formed RefreshOfferMessage, the first item wins and will not be overwritten - if (!checkIfStoredDataPubKeyMatchesNewDataPubKey(ownerPubKey, hashOfPayload)) { - log.trace("refreshTTL failed due to hash collision"); + // Verify the Payload owner and the Entry owner for the stored Entry are the same + // TODO: This is also checked in the validation for the original add(), investigate if this can be removed + if (!checkIfStoredDataPubKeyMatchesNewDataPubKey(ownerPubKey, hashOfPayload)) return false; - } // This is a valid refresh, update the payload for it log.debug("refreshDate called for storedData:\n\t" + StringUtils.abbreviate(storedData.toString(), 100)); @@ -537,26 +517,16 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, } // If we have seen a more recent operation for this payload, ignore this one - if (!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) { - log.trace("remove failed due to old sequence number"); - + if (!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) return false; - } // Verify the ProtectedStorageEntry is well formed and valid for the remove operation - if (!checkPublicKeys(protectedStorageEntry, false) || - !checkSignature(protectedStorageEntry)) { - log.trace("remove failed due to invalid entry"); - + if (!checkPublicKeys(protectedStorageEntry, false) || !checkSignature(protectedStorageEntry)) return false; - } - - // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten - if (!checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) { - log.trace("remove failed due to hash collision"); + // If we have already seen an Entry with the same hash, verify the new Entry has the same owner + if (!checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) return false; - } // Valid remove entry, do the remove and signal listeners doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload); @@ -635,28 +605,23 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt int sequenceNumber = protectedMailboxStorageEntry.getSequenceNumber(); - if (!isSequenceNrValid(sequenceNumber, hashOfPayload)) { - log.trace("removeMailboxData failed due to old sequence number"); - + if (!isSequenceNrValid(sequenceNumber, hashOfPayload)) return false; - } PublicKey receiversPubKey = protectedMailboxStorageEntry.getReceiversPubKey(); - if (!checkPublicKeys(protectedMailboxStorageEntry, false) || - !protectedMailboxStorageEntry.getMailboxStoragePayload().getOwnerPubKey().equals(receiversPubKey) || // at remove both keys are the same (only receiver is able to remove data) - !checkSignature(protectedMailboxStorageEntry)) { - log.trace("removeMailboxData failed due to invalid entry"); + if (!checkPublicKeys(protectedMailboxStorageEntry, false) || !checkSignature(protectedMailboxStorageEntry)) + return false; + // Verify the Entry has the correct receiversPubKey for removal. + if (!protectedMailboxStorageEntry.getMailboxStoragePayload().getOwnerPubKey().equals(receiversPubKey)) { + log.debug("Entry receiversPubKey does not match payload owner which is a requirement for removing MailboxStoragePayloads"); return false; } - // In a hash collision between two well formed ProtectedMailboxStorageEntry, the first item wins and will not be overwritten - if (!checkIfStoredMailboxDataMatchesNewMailboxData(receiversPubKey, hashOfPayload)) { - log.trace("removeMailboxData failed due to hash collision"); - + // If we have already seen an Entry with the same hash, verify the new Entry has the same owner + if (!checkIfStoredMailboxDataMatchesNewMailboxData(receiversPubKey, hashOfPayload)) return false; - } // Valid remove ProtectedMailboxStorageEntry, do the remove and signal listeners doRemoveProtectedExpirableData(protectedMailboxStorageEntry, hashOfPayload); From c1ad6b408b24d9e93597bf937cfab9ac63c7bf09 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 18:58:05 -0800 Subject: [PATCH 08/17] Update behavior of P2PDataStorage::addProtectedStorageEntry() on duplicates Now returns false on duplicate sequence numbers. This matches more of the expected behavior for an add() function when the element previously exists. The only callers are either P2PService users that always increment the sequence number or the onMessage() handler which doesn't verify the return so there will be no visible change other than the increased readability of the code and deduplication of the code paths. --- .../network/p2p/storage/P2PDataStorage.java | 21 ++++++------------- .../p2p/storage/P2PDataStorageTest.java | 5 ++--- 2 files changed, 8 insertions(+), 18 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 c1f54bb4297..813e9e32bcd 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -390,28 +390,19 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn return false; } - // TODO: Combine with hasSequenceNrIncreased check, but keep existing behavior for now - if(!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + // If we have seen a more recent operation for this payload, we ignore the current one + if(!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) return false; // Verify the ProtectedStorageEntry is well formed and valid for the add operation if (!checkPublicKeys(protectedStorageEntry, true) || !checkSignature(protectedStorageEntry)) return false; - boolean containsKey = map.containsKey(hashOfPayload); - - // If we have already seen an Entry with the same hash, verify the new Entry has the same owner - if (containsKey && !checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) + // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten + if (map.containsKey(hashOfPayload) && + !checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) { return false; - - boolean hasSequenceNrIncreased = hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload); - - // If we have seen a more recent operation for this payload, we ignore the current one - // TODO: I think we can return false here. All callers use the Client API (addProtectedStorageEntry(getProtectedStorageEntry()) - // leaving only the onMessage() handler which doesn't look at the return value. It makes more intuitive sense that adds() that don't - // change state return false. - if (!hasSequenceNrIncreased) - return true; + } // This is an updated entry. Record it and signal listeners. map.put(hashOfPayload, protectedStorageEntry); 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 51b8879c3ae..4fc3d924e80 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -737,12 +737,11 @@ public void addProtectedStorageEntry() throws CryptoException { } // TESTCASE: Adding duplicate payload w/ same sequence number - // TODO: Should adds() of existing sequence #s return false since they don't update state? @Test public void addProtectedStorageEntry_duplicateSeqNrGt0() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageAddAndVerify(entryForAdd, true, false); + doProtectedStorageAddAndVerify(entryForAdd, false, false); } // TESTCASE: Adding duplicate payload w/ 0 sequence number (special branch in code for logging) @@ -750,7 +749,7 @@ public void addProtectedStorageEntry_duplicateSeqNrGt0() throws CryptoException public void addProtectedStorageEntry_duplicateSeqNrEq0() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(0); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageAddAndVerify(entryForAdd, true, false); + doProtectedStorageAddAndVerify(entryForAdd, false, false); } // TESTCASE: Adding duplicate payload for w/ lower sequence number From cbda653aba9e295167f9d1dffb76afec85b705db Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 19:04:50 -0800 Subject: [PATCH 09/17] Update behavior of P2PDataStorage::refreshTTL() on duplicates Now returns false if the sequence number of the refresh matches the last operation seen for the specified hash. This is a more expected return value when no state change occurs. The only callers are either P2PService users that always increment the sequence number or the onMessage() handler which doesn't verify the return so there will be no visible change other than the increased readability of the code and deduplication of the code paths. --- .../java/bisq/network/p2p/storage/P2PDataStorage.java | 11 ----------- .../bisq/network/p2p/storage/P2PDataStorageTest.java | 4 ++-- 2 files changed, 2 insertions(+), 13 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 813e9e32bcd..a55ec68d620 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -447,17 +447,6 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, int sequenceNumber = refreshTTLMessage.getSequenceNumber(); - // If we have seen a more recent operation for this payload, we ignore the current one - // TODO: I think we can return false here. All callers use the Client API (refreshTTL(getRefreshTTLMessage()) which increments the sequence number - // leaving only the onMessage() handler which doesn't look at the return value. It makes more intuitive sense that operations that don't - // change state return false. - if (sequenceNumberMap.containsKey(hashOfPayload) && sequenceNumberMap.get(hashOfPayload).sequenceNr == sequenceNumber) { - log.trace("We got that message with that seq nr already from another peer. We ignore that message."); - - return true; - } - - // TODO: Combine with above in future work, but preserve existing behavior for now if(!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) return false; 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 4fc3d924e80..97b6a415545 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -976,7 +976,7 @@ public void refreshTTL_existingEntry() throws CryptoException { ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entry, true, true); - doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,1), true, false); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,1), false, false); } // TESTCASE: Duplicate refresh message (same seq #) @@ -986,7 +986,7 @@ public void refreshTTL_duplicateRefreshSeqNrEqual() throws CryptoException { doProtectedStorageAddAndVerify(entry, true, true); doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), true, true); - doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), true, false); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), false, false); } // TESTCASE: Duplicate refresh message (greater seq #) From e5f9261d976fa6e242b9e55e52bb70f8686bc774 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 19:22:27 -0800 Subject: [PATCH 10/17] Update behavior of P2PDataStorage::remove() & removeMailboxData() on duplicate sequence #s Remove operations are now only processed if the sequence number is greater than the last operation seen for a specific payload. The only creator of new remove entrys is the P2PService layer that always increments the sequence number. So, this is either left around from a time where removes needed to work with non-incrementing sequence numbers or just a longstanding bug. With the completion of this patch, all operations now require increasing sequence numbers so it should be easier to reason about the behavior in future debugging. --- .../network/p2p/storage/P2PDataStorage.java | 22 ++---------- .../p2p/storage/P2PDataStorageTest.java | 35 ++++++++----------- 2 files changed, 17 insertions(+), 40 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 a55ec68d620..7f1a20e41d7 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -497,7 +497,7 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, } // If we have seen a more recent operation for this payload, ignore this one - if (!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + if (!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) return false; // Verify the ProtectedStorageEntry is well formed and valid for the remove operation @@ -585,7 +585,7 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt int sequenceNumber = protectedMailboxStorageEntry.getSequenceNumber(); - if (!isSequenceNrValid(sequenceNumber, hashOfPayload)) + if (!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) return false; PublicKey receiversPubKey = protectedMailboxStorageEntry.getReceiversPubKey(); @@ -710,24 +710,6 @@ private void doRemoveProtectedExpirableData(ProtectedStorageEntry protectedStora hashMapChangedListeners.forEach(e -> e.onRemoved(protectedStorageEntry)); } - private boolean isSequenceNrValid(int newSequenceNumber, ByteArray hashOfData) { - if (sequenceNumberMap.containsKey(hashOfData)) { - int storedSequenceNumber = sequenceNumberMap.get(hashOfData).sequenceNr; - if (newSequenceNumber >= storedSequenceNumber) { - log.trace("Sequence number is valid (>=). sequenceNumber = " - + newSequenceNumber + " / storedSequenceNumber=" + storedSequenceNumber); - return true; - } else { - log.debug("Sequence number is invalid. sequenceNumber = " - + newSequenceNumber + " / storedSequenceNumber=" + storedSequenceNumber + "\n" + - "That can happen if the data owner gets an old delayed data storage message."); - return false; - } - } else { - return true; - } - } - private boolean hasSequenceNrIncreased(int newSequenceNumber, ByteArray hashOfData) { if (sequenceNumberMap.containsKey(hashOfData)) { int storedSequenceNumber = sequenceNumberMap.get(hashOfData).sequenceNr; 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 97b6a415545..02073b06d58 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -771,21 +771,17 @@ public void addProtectedStorageEntry_greaterSeqNr() throws CryptoException { } // TESTCASE: Add w/ same sequence number after remove of sequence number - // XXXBUGXXX: Since removes aren't required to increase the sequence number, duplicate adds - // can occur that will cause listeners to be signaled. Any well-intentioned nodes will create remove messages - // that increment the seq #, but this may just fall into a larger effort to protect against malicious nodes. -/* @Test + // Regression test for old remove() behavior that succeeded if add.seq# == remove.seq# + @Test public void addProtectectedStorageEntry_afterRemoveSameSeqNr() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false); - // Should be false, false. Instead, the hashmap is updated and hashmap listeners are signaled. - // Broadcast isn't called doProtectedStorageAddAndVerify(entryForAdd, false, false); - }*/ + } // TESTCASE: Entry signature does not match entry owner @Test @@ -818,8 +814,6 @@ public void addProtectedStorageEntry_PayloadHashCollision_Fails() { }*/ // TESTCASE: Removing an item after successfully added (remove seq # == add seq #) - // XXXBUGXXX A state change shouldn't occur. Any well-intentioned nodes will create remove messages - // that increment the seq #, but this may just fall into a larger effort to protect against malicious nodes. @Test public void remove_seqNrEqAddSeqNr() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); @@ -827,8 +821,7 @@ public void remove_seqNrEqAddSeqNr() throws CryptoException { doProtectedStorageAddAndVerify(entryForAdd, true, true); - // should be (false, false) - doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } // TESTCASE: Removing an item after successfully added (remove seq # > add seq #) @@ -865,7 +858,7 @@ public void remove_invalidEntrySig() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entryForAdd, true, true); - ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); + ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2); entryForRemove.updateSignature(new byte[] { 0 }); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -880,7 +873,7 @@ public void remove_payloadOwnerEntryOwnerNotCompatible() throws NoSuchAlgorithmE // For standard ProtectedStorageEntrys the entry owner must match the payload owner for removes ProtectedStorageEntry entryForRemove = buildProtectedStorageEntry( - this.protectedStoragePayload, notOwner, notOwner, 1); + this.protectedStoragePayload, notOwner, notOwner, 2); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -1012,11 +1005,13 @@ public void refreshTTL_duplicateRefreshSeqNrLower() throws CryptoException { // TESTCASE: Refresh previously removed entry @Test public void refreshTTL_refreshAfterRemove() throws CryptoException { - ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); - doProtectedStorageAddAndVerify(entry, true, true); - doProtectedStorageRemoveAndVerify(entry, true, true); + ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); + ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForAdd(2); + + doProtectedStorageAddAndVerify(entryForAdd, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, true); - doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,3), false, false); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entryForAdd, this.payloadOwnerKeys,3), false, false); } // TESTCASE: Refresh an entry, but owner doesn't match PubKey of original add owner @@ -1128,7 +1123,7 @@ public void remove_payloadOwnerEntryOwnerNotCompatible() throws NoSuchAlgorithmE KeyPair notReceiver = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 1); + ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 2); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -1141,7 +1136,7 @@ public void remove_payloadOwnerEntryReceiversPubKeyNotCompatible() throws NoSuch KeyPair notSender = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 1); + ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 2); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } From de72d3954d44b6064ee6cb2284f09fd4b8e43c53 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 6 Nov 2019 11:04:01 -0800 Subject: [PATCH 11/17] Use dependency injected Clock in P2PDataStore Use the DI Clock object already available in P2PDataStore, instead of calling System.currentTimeMillis() directly. These two functions have the same behavior and switching over allows finer control of time in the tests. --- .../java/bisq/network/p2p/storage/P2PDataStorage.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 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 7f1a20e41d7..02323e4d97b 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -409,7 +409,7 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn hashMapChangedListeners.forEach(e -> e.onAdded(protectedStorageEntry)); // Record the updated sequence number and persist it. Higher delay so we can batch more items. - sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), System.currentTimeMillis())); + sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), this.clock.millis())); sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 2000); // Optionally, broadcast the add/update depending on the calling environment @@ -472,7 +472,7 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, printData("after refreshTTL"); // Record the latest sequence number and persist it - sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, System.currentTimeMillis())); + sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, this.clock.millis())); sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 1000); // Always broadcast refreshes @@ -492,7 +492,6 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, // If we don't know about the target of this remove, ignore it if (!map.containsKey(hashOfPayload)) { log.debug("Remove data ignored as we don't have an entry for that data."); - return false; } @@ -513,7 +512,7 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, printData("after remove"); // Record the latest sequence number and persist it - sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), System.currentTimeMillis())); + sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), this.clock.millis())); sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); @@ -608,7 +607,7 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt printData("after removeMailboxData"); // Record the latest sequence number and persist it - sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, System.currentTimeMillis())); + sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, this.clock.millis())); sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); @@ -839,7 +838,7 @@ private static byte[] getCompactHash(ProtectedStoragePayload protectedStoragePay // Get a new map with entries older than PURGE_AGE_DAYS purged from the given map. private Map getPurgedSequenceNumberMap(Map persisted) { Map purged = new HashMap<>(); - long maxAgeTs = System.currentTimeMillis() - TimeUnit.DAYS.toMillis(PURGE_AGE_DAYS); + long maxAgeTs = this.clock.millis() - TimeUnit.DAYS.toMillis(PURGE_AGE_DAYS); persisted.forEach((key, value) -> { if (value.timeStamp > maxAgeTs) purged.put(key, value); From 42680037bd16d34479d76d46fcedaaafc445d04f Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 6 Nov 2019 12:03:38 -0800 Subject: [PATCH 12/17] [REFACTOR] Clean up ProtectedStorageEntry ctor Deduplicate some code in the ProtectedStorageEntry constructors in preparation for passing in a Clock parameter. --- .../payload/ProtectedMailboxStorageEntry.java | 55 +++++++++++++------ .../payload/ProtectedStorageEntry.java | 50 +++++++++++------ 2 files changed, 70 insertions(+), 35 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java index 509f45645f3..dc969fcb743 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java @@ -41,10 +41,33 @@ public ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, int sequenceNumber, byte[] signature, PublicKey receiversPubKey) { - super(mailboxStoragePayload, ownerPubKey, sequenceNumber, signature); + this(mailboxStoragePayload, + Sig.getPublicKeyBytes(ownerPubKey), + ownerPubKey, + sequenceNumber, + signature, + Sig.getPublicKeyBytes(receiversPubKey), + receiversPubKey, + System.currentTimeMillis()); + } + + private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, + byte[] ownerPubKeyBytes, + PublicKey ownerPubKey, + int sequenceNumber, + byte[] signature, + byte[] receiversPubKeyBytes, + PublicKey receiversPubKey, + long creationTimeStamp) { + super(mailboxStoragePayload, + ownerPubKeyBytes, + ownerPubKey, + sequenceNumber, + signature, + creationTimeStamp); this.receiversPubKey = receiversPubKey; - receiversPubKeyBytes = Sig.getPublicKeyBytes(receiversPubKey); + this.receiversPubKeyBytes = receiversPubKeyBytes; } public MailboxStoragePayload getMailboxStoragePayload() { @@ -56,22 +79,20 @@ public MailboxStoragePayload getMailboxStoragePayload() { // PROTO BUFFER /////////////////////////////////////////////////////////////////////////////////////////// - private ProtectedMailboxStorageEntry(long creationTimeStamp, - MailboxStoragePayload mailboxStoragePayload, - byte[] ownerPubKey, + private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, + byte[] ownerPubKeyBytes, int sequenceNumber, byte[] signature, - byte[] receiversPubKeyBytes) { - super(creationTimeStamp, - mailboxStoragePayload, - ownerPubKey, + byte[] receiversPubKeyBytes, + long creationTimeStamp) { + this(mailboxStoragePayload, + ownerPubKeyBytes, + Sig.getPublicKeyFromBytes(ownerPubKeyBytes), sequenceNumber, - signature); - - this.receiversPubKeyBytes = receiversPubKeyBytes; - receiversPubKey = Sig.getPublicKeyFromBytes(receiversPubKeyBytes); - - maybeAdjustCreationTimeStamp(); + signature, + receiversPubKeyBytes, + Sig.getPublicKeyFromBytes(receiversPubKeyBytes), + creationTimeStamp); } public protobuf.ProtectedMailboxStorageEntry toProtoMessage() { @@ -85,12 +106,12 @@ public static ProtectedMailboxStorageEntry fromProto(protobuf.ProtectedMailboxSt NetworkProtoResolver resolver) { ProtectedStorageEntry entry = ProtectedStorageEntry.fromProto(proto.getEntry(), resolver); return new ProtectedMailboxStorageEntry( - entry.getCreationTimeStamp(), (MailboxStoragePayload) entry.getProtectedStoragePayload(), entry.getOwnerPubKey().getEncoded(), entry.getSequenceNumber(), entry.getSignature(), - proto.getReceiversPubKeyBytes().toByteArray()); + proto.getReceiversPubKeyBytes().toByteArray(), + entry.getCreationTimeStamp()); } diff --git a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java index b967caf594f..cc2dcec1835 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java @@ -43,37 +43,50 @@ public class ProtectedStorageEntry implements NetworkPayload, PersistablePayload private long creationTimeStamp; public ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, + PublicKey ownerPubKey, + int sequenceNumber, + byte[] signature) { + this(protectedStoragePayload, + Sig.getPublicKeyBytes(ownerPubKey), + ownerPubKey, + sequenceNumber, + signature, + System.currentTimeMillis()); + } + + protected ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, + byte[] ownerPubKeyBytes, PublicKey ownerPubKey, int sequenceNumber, - byte[] signature) { + byte[] signature, + long creationTimeStamp) { + this.protectedStoragePayload = protectedStoragePayload; - ownerPubKeyBytes = Sig.getPublicKeyBytes(ownerPubKey); + this.ownerPubKeyBytes = ownerPubKeyBytes; this.ownerPubKey = ownerPubKey; this.sequenceNumber = sequenceNumber; this.signature = signature; - this.creationTimeStamp = System.currentTimeMillis(); - } + this.creationTimeStamp = creationTimeStamp; + maybeAdjustCreationTimeStamp(); + } /////////////////////////////////////////////////////////////////////////////////////////// // PROTO BUFFER /////////////////////////////////////////////////////////////////////////////////////////// - protected ProtectedStorageEntry(long creationTimeStamp, - ProtectedStoragePayload protectedStoragePayload, + private ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, byte[] ownerPubKeyBytes, int sequenceNumber, - byte[] signature) { - this.protectedStoragePayload = protectedStoragePayload; - this.ownerPubKeyBytes = ownerPubKeyBytes; - ownerPubKey = Sig.getPublicKeyFromBytes(ownerPubKeyBytes); - - this.sequenceNumber = sequenceNumber; - this.signature = signature; - this.creationTimeStamp = creationTimeStamp; - - maybeAdjustCreationTimeStamp(); + byte[] signature, + long creationTimeStamp) { + this(protectedStoragePayload, + ownerPubKeyBytes, + Sig.getPublicKeyFromBytes(ownerPubKeyBytes), + sequenceNumber, + signature, + creationTimeStamp); } public Message toProtoMessage() { @@ -93,11 +106,12 @@ public protobuf.ProtectedStorageEntry toProtectedStorageEntry() { public static ProtectedStorageEntry fromProto(protobuf.ProtectedStorageEntry proto, NetworkProtoResolver resolver) { - return new ProtectedStorageEntry(proto.getCreationTimeStamp(), + return new ProtectedStorageEntry( ProtectedStoragePayload.fromProto(proto.getStoragePayload(), resolver), proto.getOwnerPubKeyBytes().toByteArray(), proto.getSequenceNumber(), - proto.getSignature().toByteArray()); + proto.getSignature().toByteArray(), + proto.getCreationTimeStamp()); } From 10eb9c0d01b54a43e763d3ef4efca5f85401d889 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 6 Nov 2019 16:25:44 -0800 Subject: [PATCH 13/17] Use Clock in ProtectedStorageEntry Switch from System.currentTimeMills() to Clock.millis() so dependency injection can be used for tests that need finer control of time. This involves attaching a Clock to the resolver so all fromProto methods have one available when they reconstruct a message. This uses the Injector for the APP and a default Clock.systemDefaultZone is used in the manual instantiations. Work was already done in #3037 to make this possible. All tests still use the default system clock for now. --- .../proto/network/NetworkProtoResolver.java | 4 +++ .../bisq/core/proto/CoreProtoResolver.java | 6 ++++ .../network/CoreNetworkProtoResolver.java | 5 ++- .../bisq/monitor/metric/P2PNetworkLoad.java | 6 ++-- .../metric/P2PSeedNodeSnapshotBase.java | 4 ++- .../network/p2p/storage/P2PDataStorage.java | 10 +++--- .../payload/ProtectedMailboxStorageEntry.java | 23 ++++++++---- .../payload/ProtectedStorageEntry.java | 36 +++++++++++-------- .../test/java/bisq/network/p2p/TestUtils.java | 5 +++ .../p2p/storage/P2PDataStorageTest.java | 4 +-- .../p2p/storage/ProtectedDataStorageTest.java | 4 ++- .../storage/messages/AddDataMessageTest.java | 4 ++- 12 files changed, 77 insertions(+), 34 deletions(-) diff --git a/common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java b/common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java index fcbe82760a2..8df36611d58 100644 --- a/common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java +++ b/common/src/main/java/bisq/common/proto/network/NetworkProtoResolver.java @@ -20,6 +20,8 @@ import bisq.common.proto.ProtoResolver; import bisq.common.proto.ProtobufferException; +import java.time.Clock; + public interface NetworkProtoResolver extends ProtoResolver { NetworkEnvelope fromProto(protobuf.NetworkEnvelope proto) throws ProtobufferException; @@ -27,4 +29,6 @@ public interface NetworkProtoResolver extends ProtoResolver { NetworkPayload fromProto(protobuf.StoragePayload proto); NetworkPayload fromProto(protobuf.StorageEntryWrapper proto); + + Clock getClock(); } diff --git a/core/src/main/java/bisq/core/proto/CoreProtoResolver.java b/core/src/main/java/bisq/core/proto/CoreProtoResolver.java index 4799786b7db..b0ce384192b 100644 --- a/core/src/main/java/bisq/core/proto/CoreProtoResolver.java +++ b/core/src/main/java/bisq/core/proto/CoreProtoResolver.java @@ -59,10 +59,16 @@ import bisq.common.proto.ProtobufferRuntimeException; import bisq.common.proto.persistable.PersistableEnvelope; +import java.time.Clock; + +import lombok.Getter; import lombok.extern.slf4j.Slf4j; @Slf4j public class CoreProtoResolver implements ProtoResolver { + @Getter + protected Clock clock; + @Override public PaymentAccountPayload fromProto(protobuf.PaymentAccountPayload proto) { if (proto != null) { diff --git a/core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java b/core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java index 91791a5ba39..c44ce44e80e 100644 --- a/core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java +++ b/core/src/main/java/bisq/core/proto/network/CoreNetworkProtoResolver.java @@ -88,6 +88,8 @@ import javax.inject.Inject; import javax.inject.Singleton; +import java.time.Clock; + import lombok.extern.slf4j.Slf4j; // TODO Use ProtobufferException instead of ProtobufferRuntimeException @@ -95,7 +97,8 @@ @Singleton public class CoreNetworkProtoResolver extends CoreProtoResolver implements NetworkProtoResolver { @Inject - public CoreNetworkProtoResolver() { + public CoreNetworkProtoResolver(Clock clock) { + this.clock = clock; } @Override diff --git a/monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java b/monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java index d72a68767c5..5ae4dc61ac1 100644 --- a/monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java +++ b/monitor/src/main/java/bisq/monitor/metric/P2PNetworkLoad.java @@ -49,6 +49,8 @@ import org.springframework.core.env.PropertySource; +import java.time.Clock; + import java.io.File; import java.util.Collections; @@ -118,7 +120,7 @@ protected void execute() { // start the network node networkNode = new TorNetworkNode(Integer.parseInt(configuration.getProperty(TOR_PROXY_PORT, "9053")), - new CoreNetworkProtoResolver(), false, + new CoreNetworkProtoResolver(Clock.systemDefaultZone()), false, new AvailableTor(Monitor.TOR_WORKING_DIR, torHiddenServiceDir.getName())); networkNode.start(this); @@ -139,7 +141,7 @@ public String getProperty(String name) { }); CorruptedDatabaseFilesHandler corruptedDatabaseFilesHandler = new CorruptedDatabaseFilesHandler(); int maxConnections = Integer.parseInt(configuration.getProperty(MAX_CONNECTIONS, "12")); - NetworkProtoResolver networkProtoResolver = new CoreNetworkProtoResolver(); + NetworkProtoResolver networkProtoResolver = new CoreNetworkProtoResolver(Clock.systemDefaultZone()); CorePersistenceProtoResolver persistenceProtoResolver = new CorePersistenceProtoResolver(null, networkProtoResolver, storageDir, corruptedDatabaseFilesHandler); DefaultSeedNodeRepository seedNodeRepository = new DefaultSeedNodeRepository(environment, null); diff --git a/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java b/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java index 2b7fb9097b3..c47820d06e5 100644 --- a/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java +++ b/monitor/src/main/java/bisq/monitor/metric/P2PSeedNodeSnapshotBase.java @@ -39,6 +39,8 @@ import com.google.common.util.concurrent.Futures; import com.google.common.util.concurrent.SettableFuture; +import java.time.Clock; + import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -90,7 +92,7 @@ public P2PSeedNodeSnapshotBase(Reporter reporter) { protected void execute() { // start the network node final NetworkNode networkNode = new TorNetworkNode(Integer.parseInt(configuration.getProperty(TOR_PROXY_PORT, "9054")), - new CoreNetworkProtoResolver(), false, + new CoreNetworkProtoResolver(Clock.systemDefaultZone()), false, new AvailableTor(Monitor.TOR_WORKING_DIR, "unused")); // we do not need to start the networkNode, as we do not need the HS //networkNode.start(this); 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 02323e4d97b..1754c766128 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -188,7 +188,7 @@ public void onBootstrapComplete() { Map temp = new HashMap<>(map); Set toRemoveSet = new HashSet<>(); temp.entrySet().stream() - .filter(entry -> entry.getValue().isExpired()) + .filter(entry -> entry.getValue().isExpired(this.clock)) .forEach(entry -> { ByteArray hashOfPayload = entry.getKey(); ProtectedStorageEntry protectedStorageEntry = map.get(hashOfPayload); @@ -285,7 +285,7 @@ public void onDisconnect(CloseConnectionReason closeConnectionReason, Connection // TODO investigate what causes the disconnections. // Usually the are: SOCKET_TIMEOUT ,TERMINATED (EOFException) protectedStorageEntry.backDate(); - if (protectedStorageEntry.isExpired()) { + if (protectedStorageEntry.isExpired(this.clock)) { log.info("We found an expired data entry which we have already back dated. " + "We remove the protectedStoragePayload:\n\t" + Utilities.toTruncatedString(protectedStorageEntry.getProtectedStoragePayload(), 100)); doRemoveProtectedExpirableData(protectedStorageEntry, hashOfPayload); @@ -466,7 +466,7 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, // This is a valid refresh, update the payload for it log.debug("refreshDate called for storedData:\n\t" + StringUtils.abbreviate(storedData.toString(), 100)); - storedData.refreshTTL(); + storedData.refreshTTL(this.clock); storedData.updateSequenceNumber(sequenceNumber); storedData.updateSignature(signature); printData("after refreshTTL"); @@ -636,7 +636,7 @@ public ProtectedStorageEntry getProtectedStorageEntry(ProtectedStoragePayload pr byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new DataAndSeqNrPair(protectedStoragePayload, sequenceNumber)); byte[] signature = Sig.sign(ownerStoragePubKey.getPrivate(), hashOfDataAndSeqNr); - return new ProtectedStorageEntry(protectedStoragePayload, ownerStoragePubKey.getPublic(), sequenceNumber, signature); + return new ProtectedStorageEntry(protectedStoragePayload, ownerStoragePubKey.getPublic(), sequenceNumber, signature, this.clock); } public RefreshOfferMessage getRefreshTTLMessage(ProtectedStoragePayload protectedStoragePayload, @@ -668,7 +668,7 @@ public ProtectedMailboxStorageEntry getMailboxDataWithSignedSeqNr(MailboxStorage byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new DataAndSeqNrPair(expirableMailboxStoragePayload, sequenceNumber)); byte[] signature = Sig.sign(storageSignaturePubKey.getPrivate(), hashOfDataAndSeqNr); return new ProtectedMailboxStorageEntry(expirableMailboxStoragePayload, - storageSignaturePubKey.getPublic(), sequenceNumber, signature, receiversPublicKey); + storageSignaturePubKey.getPublic(), sequenceNumber, signature, receiversPublicKey, this.clock); } public void addHashMapChangedListener(HashMapChangedListener hashMapChangedListener) { diff --git a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java index dc969fcb743..e0fe2ccc766 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntry.java @@ -25,6 +25,8 @@ import java.security.PublicKey; +import java.time.Clock; + import lombok.EqualsAndHashCode; import lombok.Value; import lombok.extern.slf4j.Slf4j; @@ -40,7 +42,8 @@ public ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, PublicKey ownerPubKey, int sequenceNumber, byte[] signature, - PublicKey receiversPubKey) { + PublicKey receiversPubKey, + Clock clock) { this(mailboxStoragePayload, Sig.getPublicKeyBytes(ownerPubKey), ownerPubKey, @@ -48,7 +51,8 @@ public ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, signature, Sig.getPublicKeyBytes(receiversPubKey), receiversPubKey, - System.currentTimeMillis()); + clock.millis(), + clock); } private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, @@ -58,13 +62,15 @@ private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload byte[] signature, byte[] receiversPubKeyBytes, PublicKey receiversPubKey, - long creationTimeStamp) { + long creationTimeStamp, + Clock clock) { super(mailboxStoragePayload, ownerPubKeyBytes, ownerPubKey, sequenceNumber, signature, - creationTimeStamp); + creationTimeStamp, + clock); this.receiversPubKey = receiversPubKey; this.receiversPubKeyBytes = receiversPubKeyBytes; @@ -84,7 +90,8 @@ private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload int sequenceNumber, byte[] signature, byte[] receiversPubKeyBytes, - long creationTimeStamp) { + long creationTimeStamp, + Clock clock) { this(mailboxStoragePayload, ownerPubKeyBytes, Sig.getPublicKeyFromBytes(ownerPubKeyBytes), @@ -92,7 +99,8 @@ private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload signature, receiversPubKeyBytes, Sig.getPublicKeyFromBytes(receiversPubKeyBytes), - creationTimeStamp); + creationTimeStamp, + clock); } public protobuf.ProtectedMailboxStorageEntry toProtoMessage() { @@ -111,7 +119,8 @@ public static ProtectedMailboxStorageEntry fromProto(protobuf.ProtectedMailboxSt entry.getSequenceNumber(), entry.getSignature(), proto.getReceiversPubKeyBytes().toByteArray(), - entry.getCreationTimeStamp()); + entry.getCreationTimeStamp(), + resolver.getClock()); } diff --git a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java index cc2dcec1835..67d71d5031d 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java @@ -27,6 +27,8 @@ import java.security.PublicKey; +import java.time.Clock; + import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -45,13 +47,15 @@ public class ProtectedStorageEntry implements NetworkPayload, PersistablePayload public ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, PublicKey ownerPubKey, int sequenceNumber, - byte[] signature) { + byte[] signature, + Clock clock) { this(protectedStoragePayload, Sig.getPublicKeyBytes(ownerPubKey), ownerPubKey, sequenceNumber, signature, - System.currentTimeMillis()); + clock.millis(), + clock); } protected ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, @@ -59,7 +63,8 @@ protected ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, PublicKey ownerPubKey, int sequenceNumber, byte[] signature, - long creationTimeStamp) { + long creationTimeStamp, + Clock clock) { this.protectedStoragePayload = protectedStoragePayload; this.ownerPubKeyBytes = ownerPubKeyBytes; @@ -69,7 +74,7 @@ protected ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, this.signature = signature; this.creationTimeStamp = creationTimeStamp; - maybeAdjustCreationTimeStamp(); + maybeAdjustCreationTimeStamp(clock); } /////////////////////////////////////////////////////////////////////////////////////////// @@ -80,13 +85,15 @@ private ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, byte[] ownerPubKeyBytes, int sequenceNumber, byte[] signature, - long creationTimeStamp) { + long creationTimeStamp, + Clock clock) { this(protectedStoragePayload, ownerPubKeyBytes, Sig.getPublicKeyFromBytes(ownerPubKeyBytes), sequenceNumber, signature, - creationTimeStamp); + creationTimeStamp, + clock); } public Message toProtoMessage() { @@ -111,7 +118,8 @@ public static ProtectedStorageEntry fromProto(protobuf.ProtectedStorageEntry pro proto.getOwnerPubKeyBytes().toByteArray(), proto.getSequenceNumber(), proto.getSignature().toByteArray(), - proto.getCreationTimeStamp()); + proto.getCreationTimeStamp(), + resolver.getClock()); } @@ -119,14 +127,14 @@ public static ProtectedStorageEntry fromProto(protobuf.ProtectedStorageEntry pro // API /////////////////////////////////////////////////////////////////////////////////////////// - public void maybeAdjustCreationTimeStamp() { + public void maybeAdjustCreationTimeStamp(Clock clock) { // We don't allow creation date in the future, but we cannot be too strict as clocks are not synced - if (creationTimeStamp > System.currentTimeMillis()) - creationTimeStamp = System.currentTimeMillis(); + if (creationTimeStamp > clock.millis()) + creationTimeStamp = clock.millis(); } - public void refreshTTL() { - creationTimeStamp = System.currentTimeMillis(); + public void refreshTTL(Clock clock) { + creationTimeStamp = clock.millis(); } public void backDate() { @@ -142,8 +150,8 @@ public void updateSignature(byte[] signature) { this.signature = signature; } - public boolean isExpired() { + public boolean isExpired(Clock clock) { return protectedStoragePayload instanceof ExpirablePayload && - (System.currentTimeMillis() - creationTimeStamp) > ((ExpirablePayload) protectedStoragePayload).getTTL(); + (clock.millis() - creationTimeStamp) > ((ExpirablePayload) protectedStoragePayload).getTTL(); } } diff --git a/p2p/src/test/java/bisq/network/p2p/TestUtils.java b/p2p/src/test/java/bisq/network/p2p/TestUtils.java index b483fde4b3c..87d42b20cd2 100644 --- a/p2p/src/test/java/bisq/network/p2p/TestUtils.java +++ b/p2p/src/test/java/bisq/network/p2p/TestUtils.java @@ -27,6 +27,8 @@ import java.security.KeyPairGenerator; import java.security.NoSuchAlgorithmException; +import java.time.Clock; + import java.util.Set; import java.util.concurrent.CountDownLatch; @@ -184,6 +186,9 @@ public NetworkPayload fromProto(protobuf.StoragePayload proto) { public NetworkPayload fromProto(protobuf.StorageEntryWrapper proto) { return null; } + + @Override + public Clock getClock() { return null; } }; } } 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 02073b06d58..0eb1c4a780a 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -191,7 +191,7 @@ private static ProtectedStorageEntry buildProtectedStorageEntry( byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(protectedStoragePayload, sequenceNumber)); byte[] signature = Sig.sign(entrySignerKeys.getPrivate(), hashOfDataAndSeqNr); - return new ProtectedStorageEntry(protectedStoragePayload, entryOwnerKeys.getPublic(), sequenceNumber, signature); + return new ProtectedStorageEntry(protectedStoragePayload, entryOwnerKeys.getPublic(), sequenceNumber, signature, Clock.systemDefaultZone()); } private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloadSenderPubKeyForAddOperation, @@ -220,7 +220,7 @@ private static ProtectedStorageEntry buildProtectedMailboxStorageEntry( byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(payload, sequenceNumber)); byte[] signature = Sig.sign(entrySigner, hashOfDataAndSeqNr); return new ProtectedMailboxStorageEntry(payload, - entryOwnerPubKey, sequenceNumber, signature, entryReceiversPubKey); + entryOwnerPubKey, sequenceNumber, signature, entryReceiversPubKey, Clock.systemDefaultZone()); } private static RefreshOfferMessage buildRefreshOfferMessage(ProtectedStoragePayload protectedStoragePayload, diff --git a/p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java b/p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java index f04dd02bf76..458f17528b9 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/ProtectedDataStorageTest.java @@ -42,6 +42,8 @@ import java.security.SignatureException; import java.security.cert.CertificateException; +import java.time.Clock; + import java.nio.file.Path; import java.nio.file.Paths; @@ -142,7 +144,7 @@ public void testAddAndRemove() throws InterruptedException, NoSuchAlgorithmExcep int newSequenceNumber = data.getSequenceNumber() + 1; byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(data.getProtectedStoragePayload(), newSequenceNumber)); byte[] signature = Sig.sign(storageSignatureKeyPair1.getPrivate(), hashOfDataAndSeqNr); - ProtectedStorageEntry dataToRemove = new ProtectedStorageEntry(data.getProtectedStoragePayload(), data.getOwnerPubKey(), newSequenceNumber, signature); + ProtectedStorageEntry dataToRemove = new ProtectedStorageEntry(data.getProtectedStoragePayload(), data.getOwnerPubKey(), newSequenceNumber, signature, Clock.systemDefaultZone()); Assert.assertTrue(dataStorage1.remove(dataToRemove, null, true)); Assert.assertEquals(0, dataStorage1.getMap().size()); } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java b/p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java index 43ac2b5655b..3c70ff6b7eb 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/messages/AddDataMessageTest.java @@ -36,6 +36,8 @@ import java.security.SignatureException; import java.security.cert.CertificateException; +import java.time.Clock; + import java.io.File; import java.io.IOException; @@ -72,7 +74,7 @@ public void toProtoBuf() throws Exception { MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload(prefixedSealedAndSignedMessage, keyRing1.getPubKeyRing().getSignaturePubKey(), keyRing1.getPubKeyRing().getSignaturePubKey()); ProtectedStorageEntry protectedStorageEntry = new ProtectedMailboxStorageEntry(mailboxStoragePayload, - keyRing1.getSignatureKeyPair().getPublic(), 1, RandomUtils.nextBytes(10), keyRing1.getPubKeyRing().getSignaturePubKey()); + keyRing1.getSignatureKeyPair().getPublic(), 1, RandomUtils.nextBytes(10), keyRing1.getPubKeyRing().getSignaturePubKey(), Clock.systemDefaultZone()); AddDataMessage dataMessage1 = new AddDataMessage(protectedStorageEntry); protobuf.NetworkEnvelope envelope = dataMessage1.toProtoNetworkEnvelope(); From 89ad234c4323250d5d19d09d1947174fe875afc2 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 6 Nov 2019 17:15:58 -0800 Subject: [PATCH 14/17] [TESTS] Use ClockFake in tests to control time Reduces non-deterministic failures of the refreshTTL tests that resulted from the uncontrollable System.currentTimeMillis(). Now, all tests have extremely fine control over the elapsed time between calls which makes the current and future tests much better. --- .../p2p/storage/P2PDataStorageTest.java | 64 ++++++++++++++----- .../network/p2p/storage/mocks/ClockFake.java | 49 ++++++++++++++ 2 files changed, 96 insertions(+), 17 deletions(-) create mode 100644 p2p/src/test/java/bisq/network/p2p/storage/mocks/ClockFake.java 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 0eb1c4a780a..c23067d1270 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -98,16 +98,18 @@ static class TestState { final ProtectedDataStoreListener protectedDataStoreListener; final HashMapChangedListener hashMapChangedListener; final Storage mockSeqNrStorage; + final ClockFake clockFake; TestState() { this.mockBroadcaster = mock(Broadcaster.class); this.mockSeqNrStorage = mock(Storage.class); + this.clockFake = new ClockFake(); this.mockedStorage = new P2PDataStorage(mock(NetworkNode.class), this.mockBroadcaster, new AppendOnlyDataStoreServiceFake(), new ProtectedDataStoreServiceFake(), mock(ResourceDataStoreService.class), - this.mockSeqNrStorage, Clock.systemUTC()); + this.mockSeqNrStorage, this.clockFake); this.appendOnlyDataStoreListener = mock(AppendOnlyDataStoreListener.class); this.protectedDataStoreListener = mock(ProtectedDataStoreListener.class); @@ -125,6 +127,10 @@ void resetState() { reset(this.hashMapChangedListener); reset(this.mockSeqNrStorage); } + + void incrementClock() { + this.clockFake.increment(TimeUnit.HOURS.toMillis(1)); + } } // Represents a snapshot of a TestState allowing easier verification of state before and after an operation. @@ -187,11 +193,12 @@ private static ProtectedStorageEntry buildProtectedStorageEntry( ProtectedStoragePayload protectedStoragePayload, KeyPair entryOwnerKeys, KeyPair entrySignerKeys, - int sequenceNumber) throws CryptoException { + int sequenceNumber, + Clock clock) throws CryptoException { byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(protectedStoragePayload, sequenceNumber)); byte[] signature = Sig.sign(entrySignerKeys.getPrivate(), hashOfDataAndSeqNr); - return new ProtectedStorageEntry(protectedStoragePayload, entryOwnerKeys.getPublic(), sequenceNumber, signature, Clock.systemDefaultZone()); + return new ProtectedStorageEntry(protectedStoragePayload, entryOwnerKeys.getPublic(), sequenceNumber, signature, clock); } private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloadSenderPubKeyForAddOperation, @@ -213,14 +220,15 @@ private static ProtectedStorageEntry buildProtectedMailboxStorageEntry( PrivateKey entrySigner, PublicKey entryOwnerPubKey, PublicKey entryReceiversPubKey, - int sequenceNumber) throws CryptoException { + int sequenceNumber, + Clock clock) throws CryptoException { MailboxStoragePayload payload = buildMailboxStoragePayload(payloadSenderPubKeyForAddOperation, payloadOwnerPubKey); byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(payload, sequenceNumber)); byte[] signature = Sig.sign(entrySigner, hashOfDataAndSeqNr); return new ProtectedMailboxStorageEntry(payload, - entryOwnerPubKey, sequenceNumber, signature, entryReceiversPubKey, Clock.systemDefaultZone()); + entryOwnerPubKey, sequenceNumber, signature, entryReceiversPubKey, clock); } private static RefreshOfferMessage buildRefreshOfferMessage(ProtectedStoragePayload protectedStoragePayload, @@ -689,7 +697,7 @@ boolean doRefreshTTL(RefreshOfferMessage refreshOfferMessage) { ProtectedStorageEntry getProtectedStorageEntryForAdd(int sequenceNumber) throws CryptoException { // Entry signed and owned by same owner as payload - return buildProtectedStorageEntry(this.protectedStoragePayload, this.payloadOwnerKeys, this.payloadOwnerKeys, sequenceNumber); + return buildProtectedStorageEntry(this.protectedStoragePayload, this.payloadOwnerKeys, this.payloadOwnerKeys, sequenceNumber, this.testState.clockFake); } // Return a ProtectedStorageEntry that is valid for remove. @@ -697,7 +705,7 @@ ProtectedStorageEntry getProtectedStorageEntryForAdd(int sequenceNumber) throws ProtectedStorageEntry getProtectedStorageEntryForRemove(int sequenceNumber) throws CryptoException { // Entry signed and owned by same owner as payload - return buildProtectedStorageEntry(this.protectedStoragePayload, this.payloadOwnerKeys, this.payloadOwnerKeys, sequenceNumber); + return buildProtectedStorageEntry(this.protectedStoragePayload, this.payloadOwnerKeys, this.payloadOwnerKeys, sequenceNumber, this.testState.clockFake); } void doProtectedStorageAddAndVerify(ProtectedStorageEntry protectedStorageEntry, @@ -799,7 +807,7 @@ public void addProtectedStorageEntry_payloadOwnerEntryOwnerNotCompatible() throw // For standard ProtectedStorageEntrys the entry owner must match the payload owner for adds ProtectedStorageEntry entryForAdd = buildProtectedStorageEntry( - this.protectedStoragePayload, notOwner, notOwner, 1); + this.protectedStoragePayload, notOwner, notOwner, 1, this.testState.clockFake); doProtectedStorageAddAndVerify(entryForAdd, false, false); } @@ -873,7 +881,7 @@ public void remove_payloadOwnerEntryOwnerNotCompatible() throws NoSuchAlgorithmE // For standard ProtectedStorageEntrys the entry owner must match the payload owner for removes ProtectedStorageEntry entryForRemove = buildProtectedStorageEntry( - this.protectedStoragePayload, notOwner, notOwner, 2); + this.protectedStoragePayload, notOwner, notOwner, 2, this.testState.clockFake); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -978,7 +986,12 @@ public void refreshTTL_duplicateRefreshSeqNrEqual() throws CryptoException { ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entry, true, true); + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), true, true); + + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), false, false); } @@ -988,7 +1001,12 @@ public void refreshTTL_duplicateRefreshSeqNrGreater() throws CryptoException { ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entry, true, true); + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,2), true, true); + + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,3), true, true); } @@ -998,7 +1016,12 @@ public void refreshTTL_duplicateRefreshSeqNrLower() throws CryptoException { ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entry, true, true); + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,3), true, true); + + this.testState.incrementClock(); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,2), false, false); } @@ -1080,12 +1103,12 @@ boolean doRemove(ProtectedStorageEntry entry) { @Override ProtectedStorageEntry getProtectedStorageEntryForAdd(int sequenceNumber) throws CryptoException { - return buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), sequenceNumber); + return buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), sequenceNumber, this.testState.clockFake); } @Override ProtectedStorageEntry getProtectedStorageEntryForRemove(int sequenceNumber) throws CryptoException { - return buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), receiverKeys.getPublic(), sequenceNumber); + return buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), receiverKeys.getPublic(), sequenceNumber, this.testState.clockFake); } @Override @@ -1098,7 +1121,7 @@ protected ProtectedStoragePayload createInstance(KeyPair payloadOwnerKeys) { public void addProtectedStorageEntry_payloadOwnerEntryOwnerNotCompatible() throws CryptoException, NoSuchAlgorithmException { KeyPair notSender = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForAdd = buildProtectedMailboxStorageEntry(notSender.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), 1); + ProtectedStorageEntry entryForAdd = buildProtectedMailboxStorageEntry(notSender.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), 1, this.testState.clockFake); doProtectedStorageAddAndVerify(entryForAdd, false, false); } @@ -1110,7 +1133,7 @@ public void addProtectedStorageEntry_payloadOwnerEntryOwnerNotCompatibleNoSideEf doProtectedStorageAddAndVerify(this.getProtectedStorageEntryForAdd(1), true, true); - ProtectedStorageEntry invalidEntryForAdd = buildProtectedMailboxStorageEntry(notSender.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), 1); + ProtectedStorageEntry invalidEntryForAdd = buildProtectedMailboxStorageEntry(notSender.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), receiverKeys.getPublic(), 1, this.testState.clockFake); doProtectedStorageAddAndVerify(invalidEntryForAdd, false, false); } @@ -1123,7 +1146,7 @@ public void remove_payloadOwnerEntryOwnerNotCompatible() throws NoSuchAlgorithmE KeyPair notReceiver = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 2); + ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 2, this.testState.clockFake); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -1136,7 +1159,7 @@ public void remove_payloadOwnerEntryReceiversPubKeyNotCompatible() throws NoSuch KeyPair notSender = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 2); + ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 2, this.testState.clockFake); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -1149,7 +1172,7 @@ public void remove_receiversPubKeyChanged() throws NoSuchAlgorithmException, Cry KeyPair otherKeys = TestUtils.generateKeyPair(); // Add an entry that has an invalid Entry.receiversPubKey. Unfortunately, this succeeds right now. - ProtectedStorageEntry entryForAdd = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), otherKeys.getPublic(), 1); + ProtectedStorageEntry entryForAdd = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), senderKeys.getPublic(), otherKeys.getPublic(), 1, this.testState.clockFake); doProtectedStorageAddAndVerify(entryForAdd, true, true); doProtectedStorageRemoveAndVerify(this.getProtectedStorageEntryForRemove(2), false, false); @@ -1276,6 +1299,8 @@ public void getRefreshTTLMessage() throws NoSuchAlgorithmException, CryptoExcept refreshOfferMessage = this.testState.mockedStorage.getRefreshTTLMessage(protectedStoragePayload, ownerKeys); + this.testState.incrementClock(); + SavedTestState beforeState = new SavedTestState(this.testState, refreshOfferMessage); Assert.assertTrue(this.testState.mockedStorage.refreshTTL(refreshOfferMessage, getTestNodeAddress(), true)); @@ -1298,6 +1323,8 @@ public void getRefreshTTLMessage_FirstOnMessageSecondAPI() throws NoSuchAlgorith RefreshOfferMessage refreshOfferMessage = this.testState.mockedStorage.getRefreshTTLMessage(protectedStoragePayload, ownerKeys); + this.testState.incrementClock(); + SavedTestState beforeState = new SavedTestState(this.testState, refreshOfferMessage); Assert.assertTrue(this.testState.mockedStorage.refreshTTL(refreshOfferMessage, getTestNodeAddress(), true)); @@ -1351,7 +1378,7 @@ public void getMailboxDataWithSignedSeqNr_ValidRemoveAddFromMessage() throws NoS ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), senderKeys.getPrivate(), - senderKeys.getPublic(), receiverKeys.getPublic(), 1); + senderKeys.getPublic(), receiverKeys.getPublic(), 1, this.testState.clockFake); Connection mockedConnection = mock(Connection.class); when(mockedConnection.getPeersNodeAddressOptional()).thenReturn(Optional.of(getTestNodeAddress())); @@ -1478,6 +1505,9 @@ public void connectionClosedReduceTTLAndExpireItemsFromPeer() throws NoSuchAlgor SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + // Increment the time by 1 hour which will put the protectedStorageState outside TTL + this.testState.incrementClock(); + this.testState.mockedStorage.onDisconnect(CloseConnectionReason.SOCKET_CLOSED, mockedConnection); verifyStateAfterDisconnect(this.testState, beforeState, true, false); diff --git a/p2p/src/test/java/bisq/network/p2p/storage/mocks/ClockFake.java b/p2p/src/test/java/bisq/network/p2p/storage/mocks/ClockFake.java new file mode 100644 index 00000000000..6e852615c55 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/mocks/ClockFake.java @@ -0,0 +1,49 @@ +/* + * This file is part of Bisq. + * + * Bisq is free software: you can redistribute it and/or modify it + * under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or (at + * your option) any later version. + * + * Bisq is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public + * License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with Bisq. If not, see . + */ + +package bisq.network.p2p.storage.mocks; + +import java.time.Clock; +import java.time.Instant; +import java.time.ZoneId; + +public class ClockFake extends Clock { + private Instant currentInstant; + + public ClockFake() { + this.currentInstant = Instant.now(); + } + + @Override + public ZoneId getZone() { + throw new UnsupportedOperationException("ClockFake does not support getZone"); + } + + @Override + public Clock withZone(ZoneId zoneId) { + throw new UnsupportedOperationException("ClockFake does not support withZone"); + } + + @Override + public Instant instant() { + return this.currentInstant; + } + + public void increment(long milliseconds) { + this.currentInstant = this.currentInstant.plusMillis(milliseconds); + } +} From 898d7fcd4a442e635c361c388e8a753f81705d89 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 6 Nov 2019 17:51:59 -0800 Subject: [PATCH 15/17] [TESTS] Test onBootstrapComplete() Add tests for removing expired entries and optionally purging the sequence number map. Now possible since these tests have control over time with the ClockFake. The remove validation needed to be improved since deletes through the expire path don't signal HashMap listeners or write sequence numbers. --- .../network/p2p/storage/P2PDataStorage.java | 70 +++--- .../p2p/storage/P2PDataStorageTest.java | 208 +++++++++++++++--- 2 files changed, 219 insertions(+), 59 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 1754c766128..1d51ff29cd6 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -100,7 +100,8 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers /** * How many days to keep an entry before it is purged. */ - private static final int PURGE_AGE_DAYS = 10; + @VisibleForTesting + public static final int PURGE_AGE_DAYS = 10; @VisibleForTesting public static int CHECK_TTL_INTERVAL_SEC = 60; @@ -177,40 +178,43 @@ public void shutDown() { removeExpiredEntriesTimer.stop(); } - public void onBootstrapComplete() { - removeExpiredEntriesTimer = UserThread.runPeriodically(() -> { - log.trace("removeExpiredEntries"); - // The moment when an object becomes expired will not be synchronous in the network and we could - // get add network_messages after the object has expired. To avoid repeated additions of already expired - // object when we get it sent from new peers, we don’t remove the sequence number from the map. - // That way an ADD message for an already expired data will fail because the sequence number - // is equal and not larger as expected. - Map temp = new HashMap<>(map); - Set toRemoveSet = new HashSet<>(); - temp.entrySet().stream() - .filter(entry -> entry.getValue().isExpired(this.clock)) - .forEach(entry -> { - ByteArray hashOfPayload = entry.getKey(); - ProtectedStorageEntry protectedStorageEntry = map.get(hashOfPayload); - if (!(protectedStorageEntry.getProtectedStoragePayload() instanceof PersistableNetworkPayload)) { - toRemoveSet.add(protectedStorageEntry); - log.debug("We found an expired data entry. We remove the protectedData:\n\t" + Utilities.toTruncatedString(protectedStorageEntry)); - map.remove(hashOfPayload); - } - }); + @VisibleForTesting + void removeExpiredEntries() { + log.trace("removeExpiredEntries"); + // The moment when an object becomes expired will not be synchronous in the network and we could + // get add network_messages after the object has expired. To avoid repeated additions of already expired + // object when we get it sent from new peers, we don’t remove the sequence number from the map. + // That way an ADD message for an already expired data will fail because the sequence number + // is equal and not larger as expected. + Map temp = new HashMap<>(map); + Set toRemoveSet = new HashSet<>(); + temp.entrySet().stream() + .filter(entry -> entry.getValue().isExpired(this.clock)) + .forEach(entry -> { + ByteArray hashOfPayload = entry.getKey(); + ProtectedStorageEntry protectedStorageEntry = map.get(hashOfPayload); + if (!(protectedStorageEntry.getProtectedStoragePayload() instanceof PersistableNetworkPayload)) { + toRemoveSet.add(protectedStorageEntry); + log.debug("We found an expired data entry. We remove the protectedData:\n\t" + Utilities.toTruncatedString(protectedStorageEntry)); + map.remove(hashOfPayload); + } + }); + + // Batch processing can cause performance issues, so we give listeners a chance to deal with it by notifying + // about start and end of iteration. + hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataStarted); + toRemoveSet.forEach(protectedStorageEntry -> { + hashMapChangedListeners.forEach(l -> l.onRemoved(protectedStorageEntry)); + removeFromProtectedDataStore(protectedStorageEntry); + }); + hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataCompleted); - // Batch processing can cause performance issues, so we give listeners a chance to deal with it by notifying - // about start and end of iteration. - hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataStarted); - toRemoveSet.forEach(protectedStorageEntry -> { - hashMapChangedListeners.forEach(l -> l.onRemoved(protectedStorageEntry)); - removeFromProtectedDataStore(protectedStorageEntry); - }); - hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataCompleted); + if (sequenceNumberMap.size() > 1000) + sequenceNumberMap.setMap(getPurgedSequenceNumberMap(sequenceNumberMap.getMap())); + } - if (sequenceNumberMap.size() > 1000) - sequenceNumberMap.setMap(getPurgedSequenceNumberMap(sequenceNumberMap.getMap())); - }, CHECK_TTL_INTERVAL_SEC); + public void onBootstrapComplete() { + removeExpiredEntriesTimer = UserThread.runPeriodically(this::removeExpiredEntries, CHECK_TTL_INTERVAL_SEC); } public Map getAppendOnlyDataStoreMap() { 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 c23067d1270..4ac5c14250e 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -67,13 +67,13 @@ public class P2PDataStorageTest { static class ExpirableProtectedStoragePayload extends ProtectedStoragePayloadStub implements ExpirablePayload, RequiresOwnerIsOnlinePayload { private long ttl; - ExpirableProtectedStoragePayload(KeyPair ownerKeys) { - super(ownerKeys.getPublic()); + ExpirableProtectedStoragePayload(PublicKey ownerPubKey) { + super(ownerPubKey); ttl = TimeUnit.DAYS.toMillis(90); } - ExpirableProtectedStoragePayload(KeyPair ownerKeys, long ttl) { - this(ownerKeys); + ExpirableProtectedStoragePayload(PublicKey ownerPubKey, long ttl) { + this(ownerPubKey); this.ttl = ttl; } @@ -88,6 +88,17 @@ public long getTTL() { } } + static class PersistableExpirableProtectedStoragePayload extends ExpirableProtectedStoragePayload implements PersistablePayload { + + PersistableExpirableProtectedStoragePayload(PublicKey ownerPubKey) { + super(ownerPubKey); + } + + PersistableExpirableProtectedStoragePayload(PublicKey ownerPubKey, long ttl) { + super(ownerPubKey, ttl); + } + } + // Common state for tests that initializes the P2PDataStore and mocks out the dependencies. Allows // shared state verification between all tests. static class TestState { @@ -341,6 +352,8 @@ private static void verifyProtectedStorageRemove(TestState currentState, SavedTestState beforeState, ProtectedStorageEntry protectedStorageEntry, boolean expectedStateChange, + boolean expectedBroadcastOnStateChange, + boolean expectedSeqNrWriteOnStateChange, boolean expectedIsDataOwner) { P2PDataStorage.ByteArray hashMapHash = P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); P2PDataStorage.ByteArray storageHash = P2PDataStorage.getCompactHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()); @@ -356,9 +369,12 @@ private static void verifyProtectedStorageRemove(TestState currentState, verify(currentState.hashMapChangedListener).onRemoved(protectedStorageEntry); - verify(currentState.mockBroadcaster).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); + if (expectedSeqNrWriteOnStateChange) + verifySequenceNumberMapWriteContains(currentState, P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); + + if (expectedBroadcastOnStateChange) + verify(currentState.mockBroadcaster).broadcast(any(BroadcastMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); - verifySequenceNumberMapWriteContains(currentState, P2PDataStorage.get32ByteHashAsByteArray(protectedStorageEntry.getProtectedStoragePayload()), protectedStorageEntry.getSequenceNumber()); } else { Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, currentState.mockedStorage.getMap().get(hashMapHash)); @@ -733,7 +749,7 @@ void doProtectedStorageRemoveAndVerify(ProtectedStorageEntry entry, if (!this.useMessageHandler) Assert.assertEquals(expectedReturnValue, addResult); - verifyProtectedStorageRemove(this.testState, beforeState, entry, expectInternalStateChange, this.expectIsDataOwner()); + verifyProtectedStorageRemove(this.testState, beforeState, entry, expectInternalStateChange, true, true, this.expectIsDataOwner()); } // TESTCASE: Adding a well-formed entry is successful @@ -1049,17 +1065,10 @@ public void refreshTTL_refreshEntryOwnerOriginalOwnerMismatch() throws CryptoExc } // Runs the ProtectedStorageEntryTestBase tests against the PersistablePayload marker class - public static class PersistableProtectedStoragePayloadTest extends ProtectedStorageEntryTestBase { - private static class PersistableProtectedStoragePayload extends ProtectedStoragePayloadStub implements PersistablePayload { - - PersistableProtectedStoragePayload(PublicKey ownerPubKey) { - super(ownerPubKey); - } - } - + public static class PersistableExpirableProtectedStoragePayloadTest extends ProtectedStorageEntryTestBase { @Override protected ProtectedStoragePayload createInstance(KeyPair payloadOwnerKeys) { - return new PersistableProtectedStoragePayload(payloadOwnerKeys.getPublic()); + return new PersistableExpirableProtectedStoragePayload(payloadOwnerKeys.getPublic()); } } @@ -1224,7 +1233,7 @@ public void setUp() { public void getProtectedStorageEntry_NoExist() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); @@ -1238,7 +1247,7 @@ public void getProtectedStorageEntry_NoExist() throws NoSuchAlgorithmException, public void getProtectedStorageEntry() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); @@ -1255,7 +1264,7 @@ public void getProtectedStorageEntry() throws NoSuchAlgorithmException, CryptoEx public void getProtectedStorageEntry_FirstOnMessageSecondAPI() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); Connection mockedConnection = mock(Connection.class); @@ -1275,7 +1284,7 @@ public void getProtectedStorageEntry_FirstOnMessageSecondAPI() throws NoSuchAlgo public void getRefreshTTLMessage_NoExists() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); RefreshOfferMessage refreshOfferMessage = this.testState.mockedStorage.getRefreshTTLMessage(protectedStoragePayload, ownerKeys); @@ -1290,7 +1299,7 @@ public void getRefreshTTLMessage_NoExists() throws NoSuchAlgorithmException, Cry public void getRefreshTTLMessage() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true); @@ -1312,7 +1321,7 @@ public void getRefreshTTLMessage() throws NoSuchAlgorithmException, CryptoExcept public void getRefreshTTLMessage_FirstOnMessageSecondAPI() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true); @@ -1345,7 +1354,7 @@ public void getMailboxDataWithSignedSeqNr_RemoveNoExist() throws NoSuchAlgorithm SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry); Assert.assertFalse(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true)); - verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, false, true); + verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, false, true, true, true); } // TESTCASE: Adding, then removing a mailbox message from the getMailboxDataWithSignedSeqNr API @@ -1367,7 +1376,7 @@ public void getMailboxDataWithSignedSeqNr_AddThenRemove() throws NoSuchAlgorithm SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry); Assert.assertTrue(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true)); - verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true); + verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true, true,true); } // TESTCASE: Removing a mailbox message that was added from the onMessage handler @@ -1393,7 +1402,7 @@ public void getMailboxDataWithSignedSeqNr_ValidRemoveAddFromMessage() throws NoS SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry); Assert.assertTrue(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true)); - verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true); + verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true, true,true); } } @@ -1403,7 +1412,7 @@ public static class DisconnectTest { private static ProtectedStorageEntry populateTestState(TestState testState, long ttl) throws CryptoException, NoSuchAlgorithmException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys, ttl); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic(), ttl); ProtectedStorageEntry protectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, false); @@ -1513,4 +1522,151 @@ public void connectionClosedReduceTTLAndExpireItemsFromPeer() throws NoSuchAlgor verifyStateAfterDisconnect(this.testState, beforeState, true, false); } } + + public static class RemoveExpiredTests { + TestState testState; + + @Before + public void setUp() { + this.testState = new TestState(); + + // Deep in the bowels of protobuf we grab the messageID from the version module. This is required to hash the + // full MailboxStoragePayload so make sure it is initialized. + Version.setBaseCryptoNetworkId(1); + } + + // TESTCASE: Correctly skips entries that are not expirable + @Test + public void removeExpiredEntries_SkipsNonExpirableEntries() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new ProtectedStoragePayloadStub(ownerKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, false, false, false, false); + } + + // TESTCASE: Correctly skips non-persistable entries that are not expired + @Test + public void removeExpiredEntries_SkipNonExpiredExpirableEntries() throws CryptoException, NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, false, false, false, false); + } + + // TESTCASE: Correctly expires non-persistable entries that are expired + @Test + public void removeExpiredEntries_ExpiresExpiredExpirableEntries() throws CryptoException, NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new ExpirableProtectedStoragePayload(ownerKeys.getPublic(), 0); + ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + // Increment the clock by an hour which will cause the Payloads to be outside the TTL range + this.testState.incrementClock(); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, true, false, false, false); + } + + // TESTCASE: Correctly skips persistable entries that are not expired + @Test + public void removeExpiredEntries_SkipNonExpiredPersistableExpirableEntries() throws CryptoException, NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayload(ownerKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = this.testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, false, false, false, false); + } + + // TESTCASE: Correctly expires persistable entries that are expired + @Test + public void removeExpiredEntries_ExpiresExpiredPersistableExpirableEntries() throws CryptoException, NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayload(ownerKeys.getPublic(), 0); + ProtectedStorageEntry protectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(protectedStorageEntry, getTestNodeAddress(), null, true)); + + // Increment the clock by an hour which will cause the Payloads to be outside the TTL range + this.testState.incrementClock(); + + SavedTestState beforeState = new SavedTestState(this.testState, protectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + + verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, true, false, false, false); + } + + // TESTCASE: Ensure we try to purge old entries sequence number map when size exceeds 1000 elements + // and that entries less than PURGE_AGE_DAYS remain + @Test + public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchAlgorithmException { + final int initialClockIncrement = 5; + + // Add 500 entries to our sequence number map that will be purged + KeyPair purgedOwnerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload purgedProtectedStoragePayload = new PersistableExpirableProtectedStoragePayload(purgedOwnerKeys.getPublic(), 0); + ProtectedStorageEntry purgedProtectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(purgedProtectedStoragePayload, purgedOwnerKeys); + + Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, getTestNodeAddress(), null, true)); + + for (int i = 0; i <= 499; ++i) { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayload(ownerKeys.getPublic(), 0); + ProtectedStorageEntry tmpEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(tmpEntry, getTestNodeAddress(), null, true)); + } + + // Increment the time by 5 days which is less than the purge requirement. This will allow the map to have + // some values that will be purged and others that will stay. + this.testState.clockFake.increment(TimeUnit.DAYS.toMillis(initialClockIncrement)); + + // Add another 501 entries that will not be purged + KeyPair keepOwnerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload keepProtectedStoragePayload = new PersistableExpirableProtectedStoragePayload(keepOwnerKeys.getPublic(), 0); + ProtectedStorageEntry keepProtectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(keepProtectedStoragePayload, keepOwnerKeys); + + Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(keepProtectedStorageEntry, getTestNodeAddress(), null, true)); + + for (int i = 0; i <= 500; ++i) { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayload(ownerKeys.getPublic(), 0); + ProtectedStorageEntry tmpEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); + Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(tmpEntry, getTestNodeAddress(), null, true)); + } + + // P2PDataStorage::PURGE_AGE_DAYS == 10 days + // Advance time past it so they will be valid purge targets + this.testState.clockFake.increment(TimeUnit.DAYS.toMillis(P2PDataStorage.PURGE_AGE_DAYS + 1 - initialClockIncrement)); + + // The first entry (11 days old) should be purged + SavedTestState beforeState = new SavedTestState(this.testState, purgedProtectedStorageEntry); + this.testState.mockedStorage.removeExpiredEntries(); + verifyProtectedStorageRemove(this.testState, beforeState, purgedProtectedStorageEntry, true, false, false, false); + + // Which means that an addition of a purged entry should succeed. + beforeState = new SavedTestState(this.testState, purgedProtectedStorageEntry); + Assert.assertTrue(this.testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, getTestNodeAddress(), null, false)); + verifyProtectedStorageAdd(this.testState, beforeState, purgedProtectedStorageEntry, true, false); + + // The second entry (5 days old) should still exist which means trying to add it again should fail. + beforeState = new SavedTestState(this.testState, keepProtectedStorageEntry); + Assert.assertFalse(this.testState.mockedStorage.addProtectedStorageEntry(keepProtectedStorageEntry, getTestNodeAddress(), null, false)); + verifyProtectedStorageAdd(this.testState, beforeState, keepProtectedStorageEntry, false, false); + } + } } From 454b2d79e1aee4f7f1c6e7a4a4e8354b9b86acea Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Wed, 6 Nov 2019 18:06:01 -0800 Subject: [PATCH 16/17] [TESTS] Lower entries required for purge in tests The original test would take over 5 seconds. Allow tests to set the number of required entries before purge to a lower value so the tests can run faster with the same confidence. --- .../network/p2p/storage/P2PDataStorage.java | 5 ++- .../p2p/storage/P2PDataStorageTest.java | 37 +++++++++++++------ 2 files changed, 29 insertions(+), 13 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 1d51ff29cd6..7696bd406db 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -124,6 +124,8 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers private final Set protectedDataStoreListeners = new CopyOnWriteArraySet<>(); private final Clock clock; + protected int maxSequenceNumberMapSizeBeforePurge; + /////////////////////////////////////////////////////////////////////////////////////////// // Constructor /////////////////////////////////////////////////////////////////////////////////////////// @@ -148,6 +150,7 @@ public P2PDataStorage(NetworkNode networkNode, this.sequenceNumberMapStorage = sequenceNumberMapStorage; sequenceNumberMapStorage.setNumMaxBackupFiles(5); + this.maxSequenceNumberMapSizeBeforePurge = 1000; } @Override @@ -209,7 +212,7 @@ void removeExpiredEntries() { }); hashMapChangedListeners.forEach(HashMapChangedListener::onBatchRemoveExpiredDataCompleted); - if (sequenceNumberMap.size() > 1000) + if (sequenceNumberMap.size() > this.maxSequenceNumberMapSizeBeforePurge) sequenceNumberMap.setMap(getPurgedSequenceNumberMap(sequenceNumberMap.getMap())); } 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 4ac5c14250e..93ba016d6cf 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -24,7 +24,9 @@ import bisq.network.p2p.storage.payload.ProtectedStoragePayload; import bisq.network.p2p.storage.payload.RequiresOwnerIsOnlinePayload; import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreListener; +import bisq.network.p2p.storage.persistence.AppendOnlyDataStoreService; import bisq.network.p2p.storage.persistence.ProtectedDataStoreListener; +import bisq.network.p2p.storage.persistence.ProtectedDataStoreService; import bisq.network.p2p.storage.persistence.ResourceDataStoreService; import bisq.network.p2p.storage.persistence.SequenceNumberMap; @@ -111,12 +113,30 @@ static class TestState { final Storage mockSeqNrStorage; final ClockFake clockFake; + /** + * Subclass of P2PDataStorage that allows for easier testing, but keeps all functionality + */ + static class P2PDataStorageForTest extends P2PDataStorage { + + P2PDataStorageForTest(NetworkNode networkNode, + Broadcaster broadcaster, + AppendOnlyDataStoreService appendOnlyDataStoreService, + ProtectedDataStoreService protectedDataStoreService, + ResourceDataStoreService resourceDataStoreService, + Storage sequenceNumberMapStorage, + Clock clock) { + super(networkNode, broadcaster, appendOnlyDataStoreService, protectedDataStoreService, resourceDataStoreService, sequenceNumberMapStorage, clock); + + this.maxSequenceNumberMapSizeBeforePurge = 5; + } + } + TestState() { this.mockBroadcaster = mock(Broadcaster.class); this.mockSeqNrStorage = mock(Storage.class); this.clockFake = new ClockFake(); - this.mockedStorage = new P2PDataStorage(mock(NetworkNode.class), + this.mockedStorage = new P2PDataStorageForTest(mock(NetworkNode.class), this.mockBroadcaster, new AppendOnlyDataStoreServiceFake(), new ProtectedDataStoreServiceFake(), mock(ResourceDataStoreService.class), @@ -1611,20 +1631,20 @@ public void removeExpiredEntries_ExpiresExpiredPersistableExpirableEntries() thr verifyProtectedStorageRemove(this.testState, beforeState, protectedStorageEntry, true, false, false, false); } - // TESTCASE: Ensure we try to purge old entries sequence number map when size exceeds 1000 elements + // TESTCASE: Ensure we try to purge old entries sequence number map when size exceeds the maximum size // and that entries less than PURGE_AGE_DAYS remain @Test public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchAlgorithmException { final int initialClockIncrement = 5; - // Add 500 entries to our sequence number map that will be purged + // Add 4 entries to our sequence number map that will be purged KeyPair purgedOwnerKeys = TestUtils.generateKeyPair(); ProtectedStoragePayload purgedProtectedStoragePayload = new PersistableExpirableProtectedStoragePayload(purgedOwnerKeys.getPublic(), 0); ProtectedStorageEntry purgedProtectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(purgedProtectedStoragePayload, purgedOwnerKeys); Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(purgedProtectedStorageEntry, getTestNodeAddress(), null, true)); - for (int i = 0; i <= 499; ++i) { + for (int i = 0; i < 4; ++i) { KeyPair ownerKeys = TestUtils.generateKeyPair(); ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayload(ownerKeys.getPublic(), 0); ProtectedStorageEntry tmpEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); @@ -1635,20 +1655,13 @@ public void removeExpiredEntries_PurgeSeqNrMap() throws CryptoException, NoSuchA // some values that will be purged and others that will stay. this.testState.clockFake.increment(TimeUnit.DAYS.toMillis(initialClockIncrement)); - // Add another 501 entries that will not be purged + // Add a final entry that will not be purged KeyPair keepOwnerKeys = TestUtils.generateKeyPair(); ProtectedStoragePayload keepProtectedStoragePayload = new PersistableExpirableProtectedStoragePayload(keepOwnerKeys.getPublic(), 0); ProtectedStorageEntry keepProtectedStorageEntry = testState.mockedStorage.getProtectedStorageEntry(keepProtectedStoragePayload, keepOwnerKeys); Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(keepProtectedStorageEntry, getTestNodeAddress(), null, true)); - for (int i = 0; i <= 500; ++i) { - KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStoragePayload protectedStoragePayload = new PersistableExpirableProtectedStoragePayload(ownerKeys.getPublic(), 0); - ProtectedStorageEntry tmpEntry = testState.mockedStorage.getProtectedStorageEntry(protectedStoragePayload, ownerKeys); - Assert.assertTrue(testState.mockedStorage.addProtectedStorageEntry(tmpEntry, getTestNodeAddress(), null, true)); - } - // P2PDataStorage::PURGE_AGE_DAYS == 10 days // Advance time past it so they will be valid purge targets this.testState.clockFake.increment(TimeUnit.DAYS.toMillis(P2PDataStorage.PURGE_AGE_DAYS + 1 - initialClockIncrement)); From 2c2a57ef9d8fd227278fe2c5a39d72bfc0ac2533 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Fri, 8 Nov 2019 10:32:49 -0800 Subject: [PATCH 17/17] [REFACTOR] Remove duplicated code in refreshTTL The custom code to verify the refreshTTLMessage's signature and update an entry isn't necessary. Just have the code construct an updated ProtectedStorageEntry from the existing and new data, verify it, and add it to the map. This also allows the removal of the ProtectedStorageEntry APIs that modify internal state. --- .../network/p2p/storage/P2PDataStorage.java | 36 +++++++++---------- .../payload/ProtectedStorageEntry.java | 11 ++---- 2 files changed, 19 insertions(+), 28 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 7696bd406db..613f9bcdc91 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -67,8 +67,6 @@ import com.google.common.annotations.VisibleForTesting; -import org.apache.commons.lang3.StringUtils; - import java.security.KeyPair; import java.security.PublicKey; @@ -444,6 +442,7 @@ private void broadcastProtectedStorageEntry(ProtectedStorageEntry protectedStora public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, @Nullable NodeAddress sender, boolean isDataOwner) { + ByteArray hashOfPayload = new ByteArray(refreshTTLMessage.getHashOfPayload()); if (!map.containsKey((hashOfPayload))) { @@ -452,34 +451,33 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, return false; } - int sequenceNumber = refreshTTLMessage.getSequenceNumber(); + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + ProtectedStorageEntry updatedEntry = new ProtectedStorageEntry( + storedEntry.getProtectedStoragePayload(), + storedEntry.getOwnerPubKey(), + refreshTTLMessage.getSequenceNumber(), + refreshTTLMessage.getSignature(), + this.clock); - if(!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) - return false; - ProtectedStorageEntry storedData = map.get(hashOfPayload); - PublicKey ownerPubKey = storedData.getProtectedStoragePayload().getOwnerPubKey(); - byte[] hashOfDataAndSeqNr = refreshTTLMessage.getHashOfDataAndSeqNr(); - byte[] signature = refreshTTLMessage.getSignature(); + // If we have seen a more recent operation for this payload, we ignore the current one + if(!hasSequenceNrIncreased(updatedEntry.getSequenceNumber(), hashOfPayload)) + return false; - // Verify the RefreshOfferMessage is well formed and valid for the refresh operation - if (!checkSignature(ownerPubKey, hashOfDataAndSeqNr, signature)) + // Verify the updated ProtectedStorageEntry is well formed and valid for update + if (!checkSignature(updatedEntry)) return false; // Verify the Payload owner and the Entry owner for the stored Entry are the same // TODO: This is also checked in the validation for the original add(), investigate if this can be removed - if (!checkIfStoredDataPubKeyMatchesNewDataPubKey(ownerPubKey, hashOfPayload)) + if (!checkIfStoredDataPubKeyMatchesNewDataPubKey(updatedEntry.getOwnerPubKey(), hashOfPayload)) return false; - // This is a valid refresh, update the payload for it - log.debug("refreshDate called for storedData:\n\t" + StringUtils.abbreviate(storedData.toString(), 100)); - storedData.refreshTTL(this.clock); - storedData.updateSequenceNumber(sequenceNumber); - storedData.updateSignature(signature); - printData("after refreshTTL"); + // Update the hash map with the updated entry + map.put(hashOfPayload, updatedEntry); // Record the latest sequence number and persist it - sequenceNumberMap.put(hashOfPayload, new MapValue(sequenceNumber, this.clock.millis())); + sequenceNumberMap.put(hashOfPayload, new MapValue(updatedEntry.getSequenceNumber(), this.clock.millis())); sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 1000); // Always broadcast refreshes diff --git a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java index 67d71d5031d..71f264d386e 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/payload/ProtectedStorageEntry.java @@ -40,7 +40,7 @@ public class ProtectedStorageEntry implements NetworkPayload, PersistablePayload private final ProtectedStoragePayload protectedStoragePayload; private final byte[] ownerPubKeyBytes; transient private final PublicKey ownerPubKey; - private int sequenceNumber; + private final int sequenceNumber; private byte[] signature; private long creationTimeStamp; @@ -133,19 +133,12 @@ public void maybeAdjustCreationTimeStamp(Clock clock) { creationTimeStamp = clock.millis(); } - public void refreshTTL(Clock clock) { - creationTimeStamp = clock.millis(); - } - public void backDate() { if (protectedStoragePayload instanceof ExpirablePayload) creationTimeStamp -= ((ExpirablePayload) protectedStoragePayload).getTTL() / 2; } - public void updateSequenceNumber(int sequenceNumber) { - this.sequenceNumber = sequenceNumber; - } - + // TODO: only used in tests so find a better way to test and delete public API public void updateSignature(byte[] signature) { this.signature = signature; }