From de5ffd43e3742e541cbe9130edaaec382a482fa0 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sun, 3 Nov 2019 09:09:47 -0800 Subject: [PATCH 01/30] [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/30] [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/30] [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/30] [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/30] [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/30] [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/30] [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/30] 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/30] 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/30] 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/30] 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/30] [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/30] 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/30] [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/30] [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/30] [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/30] [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; } From 54b4b4df1531672cf23768209aa38a1388cfb446 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 12:07:00 -0800 Subject: [PATCH 18/30] [TESTS] Add more tests around mis-wrapped ProtectedStorageEntrys The code around validating MailboxStoragePayloads is subtle when a MailboxStoragePayload is wrapped in a ProtectedStorageEntry. Add tests to document the current behavior. --- .../p2p/storage/P2PDataStorageTest.java | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) 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 93ba016d6cf..c06bbfad57e 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -1145,6 +1145,75 @@ protected ProtectedStoragePayload createInstance(KeyPair payloadOwnerKeys) { return null; } + // TESTCASE: adding a MailboxStoragePayload wrapped in a ProtectedStorageEntry owned by the receiver should fail + @Test + public void addProtectedStorageEntry_badWrappingReceiverEntry() throws CryptoException, NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + ProtectedStorageEntry entryForAdd = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys, receiverKeys, 1, this.testState.clockFake); + + doProtectedStorageAddAndVerify(entryForAdd, false, false); + } + + // TESTCASE: adding a MailboxStoragePayload wrapped in a ProtectedStorageEntry owned by the sender should fail + // XXXBUGXXX Miswrapped MailboxStoragePayload objects go through non-mailbox validation. This circumvents the + // Entry.ownerPubKey == Payload.senderPubKeyForAddOperation checks. + @Test + public void addProtectedStorageEntry_badWrappingSenderEntry() throws CryptoException, NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + ProtectedStorageEntry entryForAdd = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys, senderKeys, 1, this.testState.clockFake); + + // should be (false, false) + doProtectedStorageAddAndVerify(entryForAdd, true, true); + } + + // TESTCASE: removing a MailboxStoragePayload wrapped in a ProtectedStorageEntry owned by the receiver should fail + // XXX: The reason this fails correctly is extremely subtle. checkIfStoredDataPubKeyMatchesNewDataPubKey() + // in the non-mailbox path sees the the Entry owner changed from the initial add request and fails. If we see + // this invalid wrapper we should fail more explicitly. + @Test + public void remove_badWrappingReceiverEntry() throws CryptoException { + ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); + doProtectedStorageAddAndVerify(entryForAdd, true, true); + + ProtectedStorageEntry entryForRemove = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys, receiverKeys, 2, this.testState.clockFake); + + SavedTestState beforeState = new SavedTestState(this.testState, entryForRemove); + + boolean addResult = super.doRemove(entryForRemove); + + if (!this.useMessageHandler) + Assert.assertEquals(false, addResult); + + verifyProtectedStorageRemove(this.testState, beforeState, entryForRemove, false, false, false, this.expectIsDataOwner()); + } + + // TESTCASE: removing a MailboxStoragePayload wrapped in a ProtectedStorageEntry owned by the sender should fail + @Test + public void remove_badWrappingSenderEntry() throws CryptoException { + ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); + doProtectedStorageAddAndVerify(entryForAdd, true, true); + + ProtectedStorageEntry entryForRemove = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys, senderKeys, 2, this.testState.clockFake); + + SavedTestState beforeState = new SavedTestState(this.testState, entryForRemove); + + boolean addResult = super.doRemove(entryForRemove); + + // should be (false, false) + if (!this.useMessageHandler) + Assert.assertEquals(true, addResult); + + verifyProtectedStorageRemove(this.testState, beforeState, entryForRemove, true, true, true, this.expectIsDataOwner()); + } + // TESTCASE: Adding fails when Entry owner is different from sender @Test public void addProtectedStorageEntry_payloadOwnerEntryOwnerNotCompatible() throws CryptoException, NoSuchAlgorithmException { From ebf33e2ff1fd217724d6f31ad36fd3fda1cfa50d Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 09:57:07 -0800 Subject: [PATCH 19/30] [REFACTOR] ProtectedStorageEntry::validForAddOperation() Method bodies are copied from P2PDataStore to separate refactoring efforts and behavior changes. Identified a bug where a ProtectedMailboxStorageEntry mailbox entry could be added, but never removed. --- .../network/p2p/storage/P2PDataStorage.java | 7 +- .../payload/ProtectedMailboxStorageEntry.java | 15 +++ .../payload/ProtectedStorageEntry.java | 18 ++++ .../ProtectedMailboxStorageEntryTest.java | 84 ++++++++++++++++ .../payload/ProtectedStorageEntryTest.java | 97 +++++++++++++++++++ 5 files changed, 216 insertions(+), 5 deletions(-) create mode 100644 p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java create mode 100644 p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java 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 613f9bcdc91..d4553a7d811 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -770,15 +770,12 @@ private boolean checkPublicKeys(ProtectedStorageEntry protectedStorageEntry, boo if (protectedStoragePayload instanceof MailboxStoragePayload) { MailboxStoragePayload payload = (MailboxStoragePayload) protectedStoragePayload; if (isAddOperation) - result = payload.getSenderPubKeyForAddOperation() != null && - payload.getSenderPubKeyForAddOperation().equals(protectedStorageEntry.getOwnerPubKey()); + result = protectedStorageEntry.isValidForAddOperation(); else result = payload.getOwnerPubKey() != null && payload.getOwnerPubKey().equals(protectedStorageEntry.getOwnerPubKey()); } else { - result = protectedStorageEntry.getOwnerPubKey() != null && - protectedStoragePayload != null && - protectedStorageEntry.getOwnerPubKey().equals(protectedStoragePayload.getOwnerPubKey()); + result = protectedStorageEntry.isValidForAddOperation(); } if (!result) { 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 e0fe2ccc766..d1f281881e9 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 @@ -76,10 +76,25 @@ private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload this.receiversPubKeyBytes = receiversPubKeyBytes; } + /////////////////////////////////////////////////////////////////////////////////////////// + // API + /////////////////////////////////////////////////////////////////////////////////////////// + public MailboxStoragePayload getMailboxStoragePayload() { return (MailboxStoragePayload) getProtectedStoragePayload(); } + /* + * Returns true if this Entry is valid for an add operation. For mailbox Entrys, the entry owner must + * match the valid sender Public Key specified in the payload. + */ + @Override + public boolean isValidForAddOperation() { + MailboxStoragePayload mailboxStoragePayload = this.getMailboxStoragePayload(); + return mailboxStoragePayload.getSenderPubKeyForAddOperation() != null && + mailboxStoragePayload.getSenderPubKeyForAddOperation().equals(this.getOwnerPubKey()); + } + /////////////////////////////////////////////////////////////////////////////////////////// // PROTO BUFFER 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 71f264d386e..756194d1996 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 @@ -147,4 +147,22 @@ public boolean isExpired(Clock clock) { return protectedStoragePayload instanceof ExpirablePayload && (clock.millis() - creationTimeStamp) > ((ExpirablePayload) protectedStoragePayload).getTTL(); } + + /* + * Returns true if the Entry is valid for an add operation. For non-mailbox Entrys, the entry owner must + * match the payload owner. + */ + public boolean isValidForAddOperation() { + // TODO: The code currently supports MailboxStoragePayload objects inside ProtectedStorageEntry. Fix this. + if (protectedStoragePayload instanceof MailboxStoragePayload) { + MailboxStoragePayload mailboxStoragePayload = (MailboxStoragePayload) this.getProtectedStoragePayload(); + return mailboxStoragePayload.getSenderPubKeyForAddOperation() != null && + mailboxStoragePayload.getSenderPubKeyForAddOperation().equals(this.getOwnerPubKey()); + + } else { + return this.ownerPubKey != null && + this.protectedStoragePayload != null && + this.ownerPubKey.equals(protectedStoragePayload.getOwnerPubKey()); + } + } } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java new file mode 100644 index 00000000000..463dce425d1 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java @@ -0,0 +1,84 @@ +/* + * 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.payload; + +import bisq.network.p2p.PrefixedSealedAndSignedMessage; +import bisq.network.p2p.TestUtils; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; +import java.security.PublicKey; + +import java.time.Clock; + +import org.junit.Assert; +import org.junit.Test; + +import static org.mockito.Mockito.*; + +public class ProtectedMailboxStorageEntryTest { + + private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloadSenderPubKeyForAddOperation, + PublicKey payloadOwnerPubKey) { + return new MailboxStoragePayload( + mock(PrefixedSealedAndSignedMessage.class), payloadSenderPubKeyForAddOperation, payloadOwnerPubKey); + } + + private static ProtectedMailboxStorageEntry buildProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, PublicKey ownerKey, PublicKey receiverKey) { + return new ProtectedMailboxStorageEntry(mailboxStoragePayload, ownerKey, 1, new byte[] { 0 }, receiverKey, Clock.systemDefaultZone()); + } + + // TESTCASE: validForAddOperation() should return true if the Entry owner and sender key specified in payload match + @Test + public void isValidForAddOperation() throws NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys.getPublic(), receiverKeys.getPublic()); + + Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should return false if the Entry owner and sender key specified in payload don't match + @Test + public void isValidForAddOperation_EntryOwnerPayloadReceiverMismatch() throws NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys.getPublic(), receiverKeys.getPublic()); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should fail if Entry.receiversPubKey and Payload.ownerPubKey don't match + // XXXBUGXXX: The current code doesn't validate this mismatch, but it would create an added payload that could never + // be removed since the remove code requires Entry.receiversPubKey == Payload.ownerPubKey + @Test + public void isValidForAddOperation_EntryReceiverPayloadReceiverMismatch() throws NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys.getPublic(), senderKeys.getPublic()); + + // should be assertFalse + Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); + } +} diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java new file mode 100644 index 00000000000..a9a9e634473 --- /dev/null +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java @@ -0,0 +1,97 @@ +/* + * 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.payload; + +import bisq.network.p2p.PrefixedSealedAndSignedMessage; +import bisq.network.p2p.TestUtils; +import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; + +import java.security.KeyPair; +import java.security.NoSuchAlgorithmException; + +import java.time.Clock; + +import org.junit.Assert; +import org.junit.Test; + +import static org.mockito.Mockito.*; + +public class ProtectedStorageEntryTest { + + private static ProtectedStorageEntry buildProtectedStorageEntry(KeyPair payloadOwner, KeyPair entryOwner) { + return buildProtectedStorageEntry(new ProtectedStoragePayloadStub(payloadOwner.getPublic()), entryOwner); + } + + private static ProtectedStorageEntry buildProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, + KeyPair entryOwner) { + return new ProtectedStorageEntry(protectedStoragePayload, entryOwner.getPublic(), 1, + new byte[] { 0 }, Clock.systemDefaultZone()); + } + + // TESTCASE: validForAddOperation() should return true if the Entry owner and payload owner match + @Test + public void isValidForAddOperation() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); + + Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should return false if the Entry owner and payload owner don't match + @Test + public void isValidForAddOperation_Mismatch() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + KeyPair notOwnerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should fail if the entry is a MailboxStoragePayload wrapped in a + // ProtectedStorageEntry and the Entry is owned by the sender + // XXXBUGXXX: Currently, a mis-wrapped MailboxStorageEntry will circumvent the senderPubKeyForAddOperation checks + @Test + public void isValidForAddOperation_invalidMailboxPayloadSender() throws NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload( + mock(PrefixedSealedAndSignedMessage.class), senderKeys.getPublic(), receiverKeys.getPublic()); + + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(mailboxStoragePayload, senderKeys); + + // should be assertFalse + Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should fail if the entry is a MailboxStoragePayload wrapped in a + // ProtectedStorageEntry and the Entry is owned by the receiver + @Test + public void isValidForAddOperation_invalidMailboxPayloadReceiver() throws NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload( + mock(PrefixedSealedAndSignedMessage.class), senderKeys.getPublic(), receiverKeys.getPublic()); + + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(mailboxStoragePayload, receiverKeys); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + +} From a6317779edf4b6def41884df954d98190b4e6d3c Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 10:23:22 -0800 Subject: [PATCH 20/30] [REFACTOR] ProtectedStorageEntry::isValidForRemoveOperation() Extract code from P2PDataStore, test it, and use new methods. --- .../network/p2p/storage/P2PDataStorage.java | 3 +- .../payload/ProtectedMailboxStorageEntry.java | 13 ++++- .../payload/ProtectedStorageEntry.java | 10 ++++ .../ProtectedMailboxStorageEntryTest.java | 24 ++++++++++ .../payload/ProtectedStorageEntryTest.java | 48 +++++++++++++++++++ 5 files changed, 95 insertions(+), 3 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 d4553a7d811..934b359c722 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -772,8 +772,7 @@ private boolean checkPublicKeys(ProtectedStorageEntry protectedStorageEntry, boo if (isAddOperation) result = protectedStorageEntry.isValidForAddOperation(); else - result = payload.getOwnerPubKey() != null && - payload.getOwnerPubKey().equals(protectedStorageEntry.getOwnerPubKey()); + result = protectedStorageEntry.isValidForRemoveOperation(); } else { result = protectedStorageEntry.isValidForAddOperation(); } 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 d1f281881e9..df9d28524ab 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 @@ -86,7 +86,7 @@ public MailboxStoragePayload getMailboxStoragePayload() { /* * Returns true if this Entry is valid for an add operation. For mailbox Entrys, the entry owner must - * match the valid sender Public Key specified in the payload. + * match the valid sender Public Key specified in the payload. (Only sender can add) */ @Override public boolean isValidForAddOperation() { @@ -95,6 +95,17 @@ public boolean isValidForAddOperation() { mailboxStoragePayload.getSenderPubKeyForAddOperation().equals(this.getOwnerPubKey()); } + /* + * Returns true if the Entry is valid for a remove operation. For mailbox Entrys, the entry owner must + * match the payload owner. (Only receiver can remove) + */ + @Override + public boolean isValidForRemoveOperation() { + MailboxStoragePayload mailboxStoragePayload = this.getMailboxStoragePayload(); + return mailboxStoragePayload.getOwnerPubKey() != null && + mailboxStoragePayload.getOwnerPubKey().equals(this.getOwnerPubKey()); + } + /////////////////////////////////////////////////////////////////////////////////////////// // PROTO BUFFER 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 756194d1996..364b7674e08 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 @@ -165,4 +165,14 @@ public boolean isValidForAddOperation() { this.ownerPubKey.equals(protectedStoragePayload.getOwnerPubKey()); } } + + /* + * Returns true if the Entry is valid for a remove operation. For non-mailbox Entrys, the entry owner must + * match the payload owner. + */ + public boolean isValidForRemoveOperation() { + + // Same requirements as add() + return this.isValidForAddOperation(); + } } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java index 463dce425d1..84332215d1a 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java @@ -81,4 +81,28 @@ public void isValidForAddOperation_EntryReceiverPayloadReceiverMismatch() throws // should be assertFalse Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); } + + // TESTCASE: validForRemoveOperation() should return true if the Entry owner and payload owner match + @Test + public void validForRemove() throws NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys.getPublic(), receiverKeys.getPublic()); + + Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: validForRemoveOperation() should return false if the Entry owner and payload owner don't match + @Test + public void validForRemoveEntryOwnerPayloadOwnerMismatch() throws NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys.getPublic(), receiverKeys.getPublic()); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java index a9a9e634473..a434aa7b7c3 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java @@ -94,4 +94,52 @@ public void isValidForAddOperation_invalidMailboxPayloadReceiver() throws NoSuch Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); } + // TESTCASE: validForRemoveOperation() should return true if the Entry owner and payload owner match + @Test + public void isValidForRemoveOperation() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); + + Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: validForRemoveOperation() should return false if the Entry owner and payload owner don't match + @Test + public void isValidForRemoveOperation_Mismatch() throws NoSuchAlgorithmException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + KeyPair notOwnerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } + + // TESTCASE: validForRemoveOperation() should fail if the entry is a MailboxStoragePayload wrapped in a + // ProtectedStorageEntry and the Entry is owned by the sender + // XXXBUGXXX: Currently, a mis-wrapped MailboxStoragePayload will succeed + @Test + public void isValidForRemoveOperation_invalidMailboxPayloadSender() throws NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload( + mock(PrefixedSealedAndSignedMessage.class), senderKeys.getPublic(), receiverKeys.getPublic()); + + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(mailboxStoragePayload, senderKeys); + + // should be assertFalse + Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); + } + + @Test + public void isValidForRemoveOperation_invalidMailboxPayloadReceiver() throws NoSuchAlgorithmException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload( + mock(PrefixedSealedAndSignedMessage.class), senderKeys.getPublic(), receiverKeys.getPublic()); + + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(mailboxStoragePayload, receiverKeys); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } } From 217a321bbe55f21fa21a016eadc188f03f5e8b24 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 15:44:54 -0800 Subject: [PATCH 21/30] [REFACTOR] Remove checkPublicKeys() Now that the objects can answer questions about valid conditions for add/remove, ask them directly. This also pushes the logging down into the ProtectedStorageEntry and ProtectedMailboxStorageEntry and cleans up the message. --- .../network/p2p/storage/P2PDataStorage.java | 34 ++----------------- .../payload/ProtectedMailboxStorageEntry.java | 30 ++++++++++++++-- .../payload/ProtectedStorageEntry.java | 29 ++++++++++++++-- 3 files changed, 58 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 934b359c722..b82d7aaf503 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -400,7 +400,7 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn return false; // Verify the ProtectedStorageEntry is well formed and valid for the add operation - if (!checkPublicKeys(protectedStorageEntry, true) || !checkSignature(protectedStorageEntry)) + if (!protectedStorageEntry.isValidForAddOperation() || !checkSignature(protectedStorageEntry)) return false; // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten @@ -505,7 +505,7 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, return false; // Verify the ProtectedStorageEntry is well formed and valid for the remove operation - if (!checkPublicKeys(protectedStorageEntry, false) || !checkSignature(protectedStorageEntry)) + if (!protectedStorageEntry.isValidForRemoveOperation() || !checkSignature(protectedStorageEntry)) return false; // If we have already seen an Entry with the same hash, verify the new Entry has the same owner @@ -594,7 +594,7 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt PublicKey receiversPubKey = protectedMailboxStorageEntry.getReceiversPubKey(); - if (!checkPublicKeys(protectedMailboxStorageEntry, false) || !checkSignature(protectedMailboxStorageEntry)) + if (!protectedMailboxStorageEntry.isValidForRemoveOperation() || !checkSignature(protectedMailboxStorageEntry)) return false; // Verify the Entry has the correct receiversPubKey for removal. @@ -762,34 +762,6 @@ private boolean checkSignature(ProtectedStorageEntry protectedStorageEntry) { return checkSignature(protectedStorageEntry.getOwnerPubKey(), hashOfDataAndSeqNr, protectedStorageEntry.getSignature()); } - // Check that the pubkey of the storage entry matches the allowed pubkey for the addition or removal operation - // in the contained mailbox message, or the pubKey of other kinds of network_messages. - private boolean checkPublicKeys(ProtectedStorageEntry protectedStorageEntry, boolean isAddOperation) { - boolean result; - ProtectedStoragePayload protectedStoragePayload = protectedStorageEntry.getProtectedStoragePayload(); - if (protectedStoragePayload instanceof MailboxStoragePayload) { - MailboxStoragePayload payload = (MailboxStoragePayload) protectedStoragePayload; - if (isAddOperation) - result = protectedStorageEntry.isValidForAddOperation(); - else - result = protectedStorageEntry.isValidForRemoveOperation(); - } else { - result = protectedStorageEntry.isValidForAddOperation(); - } - - if (!result) { - String res1 = protectedStorageEntry.toString(); - String res2 = "null"; - if (protectedStoragePayload != null && - protectedStoragePayload.getOwnerPubKey() != null) - res2 = Utilities.encodeToHex(protectedStoragePayload.getOwnerPubKey().getEncoded(), true); - - log.warn("PublicKey of payload data and ProtectedStorageEntry are not matching. protectedStorageEntry=" + res1 + - "protectedStorageEntry.getStoragePayload().getOwnerPubKey()=" + res2); - } - return result; - } - private boolean checkIfStoredDataPubKeyMatchesNewDataPubKey(PublicKey ownerPubKey, ByteArray hashOfData) { ProtectedStorageEntry storedData = map.get(hashOfData); boolean result = storedData.getOwnerPubKey() != null && storedData.getOwnerPubKey().equals(ownerPubKey); 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 df9d28524ab..80bce135df6 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 @@ -91,8 +91,21 @@ public MailboxStoragePayload getMailboxStoragePayload() { @Override public boolean isValidForAddOperation() { MailboxStoragePayload mailboxStoragePayload = this.getMailboxStoragePayload(); - return mailboxStoragePayload.getSenderPubKeyForAddOperation() != null && + boolean result = mailboxStoragePayload.getSenderPubKeyForAddOperation() != null && mailboxStoragePayload.getSenderPubKeyForAddOperation().equals(this.getOwnerPubKey()); + + if (!result) { + String res1 = this.toString(); + String res2 = "null"; + if (mailboxStoragePayload != null && mailboxStoragePayload.getOwnerPubKey() != null) + res2 = Utilities.encodeToHex(mailboxStoragePayload.getSenderPubKeyForAddOperation().getEncoded(),true); + + log.warn(String.format("ProtectedMailboxStorageEntry::isValidForAddOperation() failed. " + + "Entry owner does not match sender key in payload:\nProtectedStorageEntry=%s\n" + + "SenderPubKeyForAddOperation=%s", res1, res2)); + } + + return result; } /* @@ -102,8 +115,21 @@ public boolean isValidForAddOperation() { @Override public boolean isValidForRemoveOperation() { MailboxStoragePayload mailboxStoragePayload = this.getMailboxStoragePayload(); - return mailboxStoragePayload.getOwnerPubKey() != null && + boolean result = mailboxStoragePayload.getOwnerPubKey() != null && mailboxStoragePayload.getOwnerPubKey().equals(this.getOwnerPubKey()); + + if (!result) { + String res1 = this.toString(); + String res2 = "null"; + if (mailboxStoragePayload != null && mailboxStoragePayload.getOwnerPubKey() != null) + res2 = Utilities.encodeToHex(mailboxStoragePayload.getOwnerPubKey().getEncoded(), true); + + log.warn(String.format("ProtectedMailboxStorageEntry::isValidForRemoveOperation() failed. " + + "Entry owner does not match Payload owner:\nProtectedStorageEntry=%s\n" + + "PayloadOwner=%s", res1, res2)); + } + + return result; } 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 364b7674e08..e19d2cf0680 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 @@ -21,6 +21,7 @@ import bisq.common.proto.network.NetworkPayload; import bisq.common.proto.network.NetworkProtoResolver; import bisq.common.proto.persistable.PersistablePayload; +import bisq.common.util.Utilities; import com.google.protobuf.ByteString; import com.google.protobuf.Message; @@ -160,9 +161,21 @@ public boolean isValidForAddOperation() { mailboxStoragePayload.getSenderPubKeyForAddOperation().equals(this.getOwnerPubKey()); } else { - return this.ownerPubKey != null && + boolean result = this.ownerPubKey != null && this.protectedStoragePayload != null && this.ownerPubKey.equals(protectedStoragePayload.getOwnerPubKey()); + + if (!result) { + String res1 = this.toString(); + String res2 = "null"; + if (protectedStoragePayload != null && protectedStoragePayload.getOwnerPubKey() != null) + res2 = Utilities.encodeToHex(protectedStoragePayload.getOwnerPubKey().getEncoded(), true); + + log.warn(String.format("ProtectedStorageEntry::isValidForAddOperation() failed. Entry owner does not match Payload owner:\n" + + "ProtectedStorageEntry=%s\nPayloadOwner=%s", res1, res2)); + } + + return result; } } @@ -173,6 +186,18 @@ public boolean isValidForAddOperation() { public boolean isValidForRemoveOperation() { // Same requirements as add() - return this.isValidForAddOperation(); + boolean result = this.isValidForAddOperation(); + + if (!result) { + String res1 = this.toString(); + String res2 = "null"; + if (protectedStoragePayload != null && protectedStoragePayload.getOwnerPubKey() != null) + res2 = Utilities.encodeToHex(protectedStoragePayload.getOwnerPubKey().getEncoded(), true); + + log.warn(String.format("ProtectedStorageEntry::isValidForRemoveOperation() failed. Entry owner does not match Payload owner:\n" + + "ProtectedStorageEntry=%s\nPayloadOwner=%s", res1, res2)); + } + + return result; } } From 40337ff1b8fcde56824bc597bd09da70101641b0 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 16:01:05 -0800 Subject: [PATCH 22/30] Clean up toString() methods Add toString() for ProtectedStorageEntry so log messages have useful information and clean up the formatting. --- .../storage/payload/ProtectedMailboxStorageEntry.java | 5 ++--- .../p2p/storage/payload/ProtectedStorageEntry.java | 11 +++++++++++ 2 files changed, 13 insertions(+), 3 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 80bce135df6..0173fb6b49e 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 @@ -179,8 +179,7 @@ public static ProtectedMailboxStorageEntry fromProto(protobuf.ProtectedMailboxSt @Override public String toString() { return "ProtectedMailboxStorageEntry{" + - "\n receiversPubKeyBytes=" + Utilities.bytesAsHexString(receiversPubKeyBytes) + - ",\n receiversPubKey=" + receiversPubKey + - "\n} " + super.toString(); + "\n\tReceivers Public Key: " + Utilities.bytesAsHexString(receiversPubKeyBytes) + + "\n" + super.toString(); } } 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 e19d2cf0680..755f4bfc920 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 @@ -200,4 +200,15 @@ public boolean isValidForRemoveOperation() { return result; } + + @Override + public String toString() { + return "ProtectedStorageEntry {" + + "\n\tPayload: " + protectedStoragePayload + + "\n\tOwner Public Key: " + Utilities.bytesAsHexString(this.ownerPubKeyBytes) + + "\n\tSequence Number: " + this.sequenceNumber + + "\n\tSignature: " + Utilities.bytesAsHexString(this.signature) + + "\n\tTimestamp: " + this.creationTimeStamp + + "\n} "; + } } From 9c7dc0c1ad29958b7b3f39ff68f29449071e6e38 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 16:52:55 -0800 Subject: [PATCH 23/30] [REFACTOR] Move signature validation behind isValidForAddOperation() Move the signature checks into the objects to clean up the calling code and make it more testable. The testing now has to take real hashes so some work was done in the fixtures to create valid hashable objects. --- .../network/p2p/storage/P2PDataStorage.java | 2 +- .../payload/ProtectedMailboxStorageEntry.java | 3 + .../payload/ProtectedStorageEntry.java | 26 +++++ .../ProtectedMailboxStorageEntryTest.java | 67 ++++++++++--- .../payload/ProtectedStorageEntryTest.java | 96 +++++++++++++------ 5 files changed, 151 insertions(+), 43 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 b82d7aaf503..902609e1416 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -400,7 +400,7 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn return false; // Verify the ProtectedStorageEntry is well formed and valid for the add operation - if (!protectedStorageEntry.isValidForAddOperation() || !checkSignature(protectedStorageEntry)) + if (!protectedStorageEntry.isValidForAddOperation()) return false; // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten 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 0173fb6b49e..d9b2fa83e00 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 @@ -90,6 +90,9 @@ public MailboxStoragePayload getMailboxStoragePayload() { */ @Override public boolean isValidForAddOperation() { + if (!this.isSignatureValid()) + return false; + MailboxStoragePayload mailboxStoragePayload = this.getMailboxStoragePayload(); boolean result = mailboxStoragePayload.getSenderPubKeyForAddOperation() != null && mailboxStoragePayload.getSenderPubKeyForAddOperation().equals(this.getOwnerPubKey()); 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 755f4bfc920..b11876b1bed 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 @@ -17,6 +17,9 @@ package bisq.network.p2p.storage.payload; +import bisq.network.p2p.storage.P2PDataStorage; + +import bisq.common.crypto.CryptoException; import bisq.common.crypto.Sig; import bisq.common.proto.network.NetworkPayload; import bisq.common.proto.network.NetworkProtoResolver; @@ -154,6 +157,9 @@ public boolean isExpired(Clock clock) { * match the payload owner. */ public boolean isValidForAddOperation() { + if (!this.isSignatureValid()) + return false; + // TODO: The code currently supports MailboxStoragePayload objects inside ProtectedStorageEntry. Fix this. if (protectedStoragePayload instanceof MailboxStoragePayload) { MailboxStoragePayload mailboxStoragePayload = (MailboxStoragePayload) this.getProtectedStoragePayload(); @@ -201,6 +207,26 @@ public boolean isValidForRemoveOperation() { return result; } + /* + * Returns true if the signature for the Entry is valid for the payload, sequence number, and ownerPubKey + */ + boolean isSignatureValid() { + try { + byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash( + new P2PDataStorage.DataAndSeqNrPair(this.protectedStoragePayload, this.sequenceNumber)); + + boolean result = Sig.verify(this.ownerPubKey, hashOfDataAndSeqNr, this.signature); + + if (!result) + log.warn(String.format("ProtectedStorageEntry::isSignatureValid() failed.\n%s", this)); + + return result; + } catch (CryptoException e) { + log.error(String.format("ProtectedStorageEntry::isSignatureValid() exception %s", e.toString())); + return false; + } + } + @Override public String toString() { return "ProtectedStorageEntry {" + diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java index 84332215d1a..b7408ab93a6 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java @@ -19,6 +19,11 @@ import bisq.network.p2p.PrefixedSealedAndSignedMessage; import bisq.network.p2p.TestUtils; +import bisq.network.p2p.storage.P2PDataStorage; + +import bisq.common.app.Version; +import bisq.common.crypto.CryptoException; +import bisq.common.crypto.Sig; import java.security.KeyPair; import java.security.NoSuchAlgorithmException; @@ -27,6 +32,7 @@ import java.time.Clock; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; import static org.mockito.Mockito.*; @@ -35,34 +41,55 @@ public class ProtectedMailboxStorageEntryTest { private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloadSenderPubKeyForAddOperation, PublicKey payloadOwnerPubKey) { + + // Mock out the PrefixedSealedAndSignedMessage with a version that just serializes to the DEFAULT_INSTANCE + // in protobuf. This object is never validated in the test, but needs to be hashed as part of the testing path. + PrefixedSealedAndSignedMessage prefixedSealedAndSignedMessageMock = mock(PrefixedSealedAndSignedMessage.class); + protobuf.NetworkEnvelope networkEnvelopeMock = mock(protobuf.NetworkEnvelope.class); + when(networkEnvelopeMock.getPrefixedSealedAndSignedMessage()).thenReturn( + protobuf.PrefixedSealedAndSignedMessage.getDefaultInstance()); + when(prefixedSealedAndSignedMessageMock.toProtoNetworkEnvelope()).thenReturn(networkEnvelopeMock); + return new MailboxStoragePayload( - mock(PrefixedSealedAndSignedMessage.class), payloadSenderPubKeyForAddOperation, payloadOwnerPubKey); + prefixedSealedAndSignedMessageMock, payloadSenderPubKeyForAddOperation, payloadOwnerPubKey); } - private static ProtectedMailboxStorageEntry buildProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, PublicKey ownerKey, PublicKey receiverKey) { - return new ProtectedMailboxStorageEntry(mailboxStoragePayload, ownerKey, 1, new byte[] { 0 }, receiverKey, Clock.systemDefaultZone()); + private static ProtectedMailboxStorageEntry buildProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, KeyPair ownerKey, PublicKey receiverKey) throws CryptoException { + int sequenceNumber = 1; + + byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(mailboxStoragePayload, sequenceNumber)); + byte[] signature = Sig.sign(ownerKey.getPrivate(), hashOfDataAndSeqNr); + + return new ProtectedMailboxStorageEntry(mailboxStoragePayload, ownerKey.getPublic(), sequenceNumber, signature, receiverKey, Clock.systemDefaultZone()); + } + + @Before + public void SetUp() { + // 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: validForAddOperation() should return true if the Entry owner and sender key specified in payload match @Test - public void isValidForAddOperation() throws NoSuchAlgorithmException { + public void isValidForAddOperation() throws NoSuchAlgorithmException, CryptoException { KeyPair senderKeys = TestUtils.generateKeyPair(); KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic()); Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); } // TESTCASE: validForAddOperation() should return false if the Entry owner and sender key specified in payload don't match @Test - public void isValidForAddOperation_EntryOwnerPayloadReceiverMismatch() throws NoSuchAlgorithmException { + public void isValidForAddOperation_EntryOwnerPayloadReceiverMismatch() throws NoSuchAlgorithmException, CryptoException { KeyPair senderKeys = TestUtils.generateKeyPair(); KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); } @@ -71,37 +98,51 @@ public void isValidForAddOperation_EntryOwnerPayloadReceiverMismatch() throws No // XXXBUGXXX: The current code doesn't validate this mismatch, but it would create an added payload that could never // be removed since the remove code requires Entry.receiversPubKey == Payload.ownerPubKey @Test - public void isValidForAddOperation_EntryReceiverPayloadReceiverMismatch() throws NoSuchAlgorithmException { + public void isValidForAddOperation_EntryReceiverPayloadReceiverMismatch() throws NoSuchAlgorithmException, CryptoException { KeyPair senderKeys = TestUtils.generateKeyPair(); KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys.getPublic(), senderKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, senderKeys.getPublic()); // should be assertFalse Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); } + // TESTCASE: validForAddOperation() should fail if the signature isn't valid + @Test + public void isValidForAddOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic()); + + protectedStorageEntry.updateSignature( new byte[] { 0 }); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + // TESTCASE: validForRemoveOperation() should return true if the Entry owner and payload owner match @Test - public void validForRemove() throws NoSuchAlgorithmException { + public void validForRemove() throws NoSuchAlgorithmException, CryptoException { KeyPair senderKeys = TestUtils.generateKeyPair(); KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); } // TESTCASE: validForRemoveOperation() should return false if the Entry owner and payload owner don't match @Test - public void validForRemoveEntryOwnerPayloadOwnerMismatch() throws NoSuchAlgorithmException { + public void validForRemoveEntryOwnerPayloadOwnerMismatch() throws NoSuchAlgorithmException, CryptoException { KeyPair senderKeys = TestUtils.generateKeyPair(); KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic()); Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java index a434aa7b7c3..236bc400cc2 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java @@ -19,33 +19,68 @@ import bisq.network.p2p.PrefixedSealedAndSignedMessage; import bisq.network.p2p.TestUtils; +import bisq.network.p2p.storage.P2PDataStorage; import bisq.network.p2p.storage.mocks.ProtectedStoragePayloadStub; +import bisq.common.app.Version; +import bisq.common.crypto.CryptoException; +import bisq.common.crypto.Sig; + import java.security.KeyPair; import java.security.NoSuchAlgorithmException; +import java.security.PublicKey; import java.time.Clock; import org.junit.Assert; +import org.junit.Before; import org.junit.Test; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; public class ProtectedStorageEntryTest { - - private static ProtectedStorageEntry buildProtectedStorageEntry(KeyPair payloadOwner, KeyPair entryOwner) { + private static ProtectedStorageEntry buildProtectedStorageEntry(KeyPair payloadOwner, KeyPair entryOwner) throws CryptoException { return buildProtectedStorageEntry(new ProtectedStoragePayloadStub(payloadOwner.getPublic()), entryOwner); } private static ProtectedStorageEntry buildProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, - KeyPair entryOwner) { - return new ProtectedStorageEntry(protectedStoragePayload, entryOwner.getPublic(), 1, - new byte[] { 0 }, Clock.systemDefaultZone()); + KeyPair entryOwner) throws CryptoException { + + int sequenceNumber = 1; + + byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(protectedStoragePayload, sequenceNumber)); + byte[] signature = Sig.sign(entryOwner.getPrivate(), hashOfDataAndSeqNr); + + return new ProtectedStorageEntry(protectedStoragePayload, entryOwner.getPublic(), sequenceNumber, + signature, Clock.systemDefaultZone()); + } + + private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloadSenderPubKeyForAddOperation, + PublicKey payloadOwnerPubKey) { + + // Mock out the PrefixedSealedAndSignedMessage with a version that just serializes to the DEFAULT_INSTANCE + // in protobuf. This object is never validated in the test, but needs to be hashed as part of the testing path. + PrefixedSealedAndSignedMessage prefixedSealedAndSignedMessageMock = mock(PrefixedSealedAndSignedMessage.class); + protobuf.NetworkEnvelope networkEnvelopeMock = mock(protobuf.NetworkEnvelope.class); + when(networkEnvelopeMock.getPrefixedSealedAndSignedMessage()).thenReturn( + protobuf.PrefixedSealedAndSignedMessage.getDefaultInstance()); + when(prefixedSealedAndSignedMessageMock.toProtoNetworkEnvelope()).thenReturn(networkEnvelopeMock); + + return new MailboxStoragePayload( + prefixedSealedAndSignedMessageMock, payloadSenderPubKeyForAddOperation, payloadOwnerPubKey); + } + + @Before + public void SetUp() { + // 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: validForAddOperation() should return true if the Entry owner and payload owner match @Test - public void isValidForAddOperation() throws NoSuchAlgorithmException { + public void isValidForAddOperation() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); @@ -54,7 +89,7 @@ public void isValidForAddOperation() throws NoSuchAlgorithmException { // TESTCASE: validForAddOperation() should return false if the Entry owner and payload owner don't match @Test - public void isValidForAddOperation_Mismatch() throws NoSuchAlgorithmException { + public void isValidForAddOperation_Mismatch() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); KeyPair notOwnerKeys = TestUtils.generateKeyPair(); ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys); @@ -66,14 +101,12 @@ public void isValidForAddOperation_Mismatch() throws NoSuchAlgorithmException { // ProtectedStorageEntry and the Entry is owned by the sender // XXXBUGXXX: Currently, a mis-wrapped MailboxStorageEntry will circumvent the senderPubKeyForAddOperation checks @Test - public void isValidForAddOperation_invalidMailboxPayloadSender() throws NoSuchAlgorithmException { + public void isValidForAddOperation_invalidMailboxPayloadSender() throws NoSuchAlgorithmException, CryptoException { KeyPair senderKeys = TestUtils.generateKeyPair(); KeyPair receiverKeys = TestUtils.generateKeyPair(); - MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload( - mock(PrefixedSealedAndSignedMessage.class), senderKeys.getPublic(), receiverKeys.getPublic()); - - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(mailboxStoragePayload, senderKeys); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys); // should be assertFalse Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); @@ -82,21 +115,30 @@ public void isValidForAddOperation_invalidMailboxPayloadSender() throws NoSuchAl // TESTCASE: validForAddOperation() should fail if the entry is a MailboxStoragePayload wrapped in a // ProtectedStorageEntry and the Entry is owned by the receiver @Test - public void isValidForAddOperation_invalidMailboxPayloadReceiver() throws NoSuchAlgorithmException { + public void isValidForAddOperation_invalidMailboxPayloadReceiver() throws NoSuchAlgorithmException, CryptoException { KeyPair senderKeys = TestUtils.generateKeyPair(); KeyPair receiverKeys = TestUtils.generateKeyPair(); - MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload( - mock(PrefixedSealedAndSignedMessage.class), senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys); + + Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); + } + + // TESTCASE: validForAddOperation() should fail if the signature isn't valid + @Test + public void isValidForAddOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(mailboxStoragePayload, receiverKeys); + protectedStorageEntry.updateSignature( new byte[] { 0 }); Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); } // TESTCASE: validForRemoveOperation() should return true if the Entry owner and payload owner match @Test - public void isValidForRemoveOperation() throws NoSuchAlgorithmException { + public void isValidForRemoveOperation() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); @@ -105,7 +147,7 @@ public void isValidForRemoveOperation() throws NoSuchAlgorithmException { // TESTCASE: validForRemoveOperation() should return false if the Entry owner and payload owner don't match @Test - public void isValidForRemoveOperation_Mismatch() throws NoSuchAlgorithmException { + public void isValidForRemoveOperation_Mismatch() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); KeyPair notOwnerKeys = TestUtils.generateKeyPair(); ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys); @@ -117,28 +159,24 @@ public void isValidForRemoveOperation_Mismatch() throws NoSuchAlgorithmException // ProtectedStorageEntry and the Entry is owned by the sender // XXXBUGXXX: Currently, a mis-wrapped MailboxStoragePayload will succeed @Test - public void isValidForRemoveOperation_invalidMailboxPayloadSender() throws NoSuchAlgorithmException { + public void isValidForRemoveOperation_invalidMailboxPayloadSender() throws NoSuchAlgorithmException, CryptoException { KeyPair senderKeys = TestUtils.generateKeyPair(); KeyPair receiverKeys = TestUtils.generateKeyPair(); - MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload( - mock(PrefixedSealedAndSignedMessage.class), senderKeys.getPublic(), receiverKeys.getPublic()); - - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(mailboxStoragePayload, senderKeys); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys); // should be assertFalse Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); } @Test - public void isValidForRemoveOperation_invalidMailboxPayloadReceiver() throws NoSuchAlgorithmException { + public void isValidForRemoveOperation_invalidMailboxPayloadReceiver() throws NoSuchAlgorithmException, CryptoException { KeyPair senderKeys = TestUtils.generateKeyPair(); KeyPair receiverKeys = TestUtils.generateKeyPair(); - MailboxStoragePayload mailboxStoragePayload = new MailboxStoragePayload( - mock(PrefixedSealedAndSignedMessage.class), senderKeys.getPublic(), receiverKeys.getPublic()); - - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(mailboxStoragePayload, receiverKeys); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys); Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } From f915a03ff9b9e93c82df35e3eb2c1bd4b80c8351 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 16:56:32 -0800 Subject: [PATCH 24/30] [REFACTOR] Move signature validation behind isValidForRemoveOperation() Move the signature checks into the objects to clean up the calling code and make it more testable. --- .../bisq/network/p2p/storage/P2PDataStorage.java | 4 ++-- .../payload/ProtectedMailboxStorageEntry.java | 3 +++ .../payload/ProtectedMailboxStorageEntryTest.java | 14 ++++++++++++++ .../storage/payload/ProtectedStorageEntryTest.java | 11 +++++++++++ 4 files changed, 30 insertions(+), 2 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 902609e1416..524514f2ea6 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -505,7 +505,7 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, return false; // Verify the ProtectedStorageEntry is well formed and valid for the remove operation - if (!protectedStorageEntry.isValidForRemoveOperation() || !checkSignature(protectedStorageEntry)) + if (!protectedStorageEntry.isValidForRemoveOperation()) return false; // If we have already seen an Entry with the same hash, verify the new Entry has the same owner @@ -594,7 +594,7 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt PublicKey receiversPubKey = protectedMailboxStorageEntry.getReceiversPubKey(); - if (!protectedMailboxStorageEntry.isValidForRemoveOperation() || !checkSignature(protectedMailboxStorageEntry)) + if (!protectedMailboxStorageEntry.isValidForRemoveOperation()) return false; // Verify the Entry has the correct receiversPubKey for removal. 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 d9b2fa83e00..0cb364c6d5e 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 @@ -117,6 +117,9 @@ public boolean isValidForAddOperation() { */ @Override public boolean isValidForRemoveOperation() { + if (!this.isSignatureValid()) + return false; + MailboxStoragePayload mailboxStoragePayload = this.getMailboxStoragePayload(); boolean result = mailboxStoragePayload.getOwnerPubKey() != null && mailboxStoragePayload.getOwnerPubKey().equals(this.getOwnerPubKey()); diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java index b7408ab93a6..ca81448c156 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java @@ -146,4 +146,18 @@ public void validForRemoveEntryOwnerPayloadOwnerMismatch() throws NoSuchAlgorith Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } + + // TESTCASE: isValidForRemoveOperation() should fail if the signature is bad + @Test + public void isValidForRemoveOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); + + protectedStorageEntry.updateSignature(new byte[] { 0 }); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java index 236bc400cc2..05f27d4809c 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java @@ -180,4 +180,15 @@ public void isValidForRemoveOperation_invalidMailboxPayloadReceiver() throws NoS Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } + + // TESTCASE: isValidForRemoveOperation() should fail if the signature is bad + @Test + public void isValidForRemoveOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); + + protectedStorageEntry.updateSignature(new byte[] { 0 }); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } } From 28a7bc887ca9bc20778fba5c2ea7c56a37e26b68 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 17:03:30 -0800 Subject: [PATCH 25/30] [REFACTOR] Move receiversPubKey check behind isValidForRemoveOperation() This mailbox-only check can now exist inside the object for which it belongs. This makes it easier to test and moves closer to allowing the deduplication of the remove() methods. --- .../bisq/network/p2p/storage/P2PDataStorage.java | 10 ---------- .../payload/ProtectedMailboxStorageEntry.java | 7 +++++++ .../payload/ProtectedMailboxStorageEntryTest.java | 12 ++++++++++++ 3 files changed, 19 insertions(+), 10 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 524514f2ea6..b5544d767b2 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -597,11 +597,6 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt if (!protectedMailboxStorageEntry.isValidForRemoveOperation()) 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; - } // If we have already seen an Entry with the same hash, verify the new Entry has the same owner if (!checkIfStoredMailboxDataMatchesNewMailboxData(receiversPubKey, hashOfPayload)) @@ -757,11 +752,6 @@ private boolean checkSignature(PublicKey ownerPubKey, byte[] hashOfDataAndSeqNr, } } - private boolean checkSignature(ProtectedStorageEntry protectedStorageEntry) { - byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new DataAndSeqNrPair(protectedStorageEntry.getProtectedStoragePayload(), protectedStorageEntry.getSequenceNumber())); - return checkSignature(protectedStorageEntry.getOwnerPubKey(), hashOfDataAndSeqNr, protectedStorageEntry.getSignature()); - } - private boolean checkIfStoredDataPubKeyMatchesNewDataPubKey(PublicKey ownerPubKey, ByteArray hashOfData) { ProtectedStorageEntry storedData = map.get(hashOfData); boolean result = storedData.getOwnerPubKey() != null && storedData.getOwnerPubKey().equals(ownerPubKey); 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 0cb364c6d5e..86b8673f937 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 @@ -121,6 +121,13 @@ public boolean isValidForRemoveOperation() { return false; MailboxStoragePayload mailboxStoragePayload = this.getMailboxStoragePayload(); + + // Verify the Entry has the correct receiversPubKey for removal + if (!mailboxStoragePayload.getOwnerPubKey().equals(this.receiversPubKey)) { + log.debug("Entry receiversPubKey does not match payload owner which is a requirement for removing MailboxStoragePayloads"); + return false; + } + boolean result = mailboxStoragePayload.getOwnerPubKey() != null && mailboxStoragePayload.getOwnerPubKey().equals(this.getOwnerPubKey()); diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java index ca81448c156..e820ba7ef0d 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java @@ -160,4 +160,16 @@ public void isValidForRemoveOperation_BadSignature() throws NoSuchAlgorithmExcep Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } + + // TESTCASE: isValidForRemoveOperation() should fail if the receiversPubKey does not match the Entry owner + @Test + public void isValidForRemoveOperation_ReceiversPubKeyMismatch() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, senderKeys.getPublic()); + + Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); + } } From 0c07883c1797f06e67ef8cb217221281e0a33406 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 18:02:42 -0800 Subject: [PATCH 26/30] Remove duplicate check in refreshTTL The current check verifies that the stored Payload.ownerPubKey == stored Entry.ownerPubKey. This is the same check that was done when the item was originally added and there is no reason to do it again. --- .../main/java/bisq/network/p2p/storage/P2PDataStorage.java | 7 +------ 1 file changed, 1 insertion(+), 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 b5544d767b2..fbdf2bc7c8a 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -465,12 +465,7 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, return false; // 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(updatedEntry.getOwnerPubKey(), hashOfPayload)) + if (!updatedEntry.isValidForAddOperation()) return false; // Update the hash map with the updated entry From 289788e374fd4f24791a3657161a49aac84433d2 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 18:41:27 -0800 Subject: [PATCH 27/30] Introduce isMetadataEquals and use it Let the objects compare their metadata instead of doing it for them. This allows for actual unit testing and paves the way for deduplicating the remove code paths. This patch also removes an unnecessary check around comparing the hash of the stored data to the new data's hash. That check can't fail since the hash was a requirement for the map lookup in the first place. --- .../network/p2p/storage/P2PDataStorage.java | 57 +++++-------------- .../payload/ProtectedMailboxStorageEntry.java | 25 ++++++++ .../payload/ProtectedStorageEntry.java | 16 ++++++ .../ProtectedMailboxStorageEntryTest.java | 49 ++++++++++++---- .../payload/ProtectedStorageEntryTest.java | 52 ++++++++++++----- 5 files changed, 130 insertions(+), 69 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 fbdf2bc7c8a..b2542412dc3 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -403,11 +403,11 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn if (!protectedStorageEntry.isValidForAddOperation()) return false; - // 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)) { + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + + // If we have already seen an Entry with the same hash, verify the metadata is equal + if (storedEntry != null && !protectedStorageEntry.isMetadataEquals(storedEntry)) return false; - } // This is an updated entry. Record it and signal listeners. map.put(hashOfPayload, protectedStorageEntry); @@ -444,8 +444,9 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage, boolean isDataOwner) { ByteArray hashOfPayload = new ByteArray(refreshTTLMessage.getHashOfPayload()); + ProtectedStorageEntry storedData = map.get(hashOfPayload); - if (!map.containsKey((hashOfPayload))) { + if (storedData == null) { 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; @@ -490,7 +491,8 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload); // If we don't know about the target of this remove, ignore it - if (!map.containsKey(hashOfPayload)) { + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + if (storedEntry == null) { log.debug("Remove data ignored as we don't have an entry for that data."); return false; } @@ -503,8 +505,8 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, if (!protectedStorageEntry.isValidForRemoveOperation()) return false; - // If we have already seen an Entry with the same hash, verify the new Entry has the same owner - if (!checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) + // If we have already seen an Entry with the same hash, verify the metadata is the same + if (!protectedStorageEntry.isMetadataEquals(storedEntry)) return false; // Valid remove entry, do the remove and signal listeners @@ -576,7 +578,8 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt ProtectedStoragePayload protectedStoragePayload = protectedMailboxStorageEntry.getProtectedStoragePayload(); ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload); - if (!map.containsKey(hashOfPayload)) { + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + if (storedEntry == null) { log.debug("removeMailboxData failed due to unknown entry"); return false; @@ -587,14 +590,11 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt if (!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) return false; - PublicKey receiversPubKey = protectedMailboxStorageEntry.getReceiversPubKey(); - if (!protectedMailboxStorageEntry.isValidForRemoveOperation()) return false; - - // If we have already seen an Entry with the same hash, verify the new Entry has the same owner - if (!checkIfStoredMailboxDataMatchesNewMailboxData(receiversPubKey, hashOfPayload)) + // Verify the metadata between the stored entry and the new remove entry are the same + if (!protectedMailboxStorageEntry.isMetadataEquals(storedEntry)) return false; // Valid remove ProtectedMailboxStorageEntry, do the remove and signal listeners @@ -747,35 +747,6 @@ private boolean checkSignature(PublicKey ownerPubKey, byte[] hashOfDataAndSeqNr, } } - private boolean checkIfStoredDataPubKeyMatchesNewDataPubKey(PublicKey ownerPubKey, ByteArray hashOfData) { - ProtectedStorageEntry storedData = map.get(hashOfData); - boolean result = storedData.getOwnerPubKey() != null && storedData.getOwnerPubKey().equals(ownerPubKey); - if (!result) - log.warn("New data entry does not match our stored data. storedData.ownerPubKey=" + - (storedData.getOwnerPubKey() != null ? storedData.getOwnerPubKey().toString() : "null") + - ", ownerPubKey=" + ownerPubKey); - - return result; - } - - private boolean checkIfStoredMailboxDataMatchesNewMailboxData(PublicKey receiversPubKey, ByteArray hashOfData) { - ProtectedStorageEntry storedData = map.get(hashOfData); - if (storedData instanceof ProtectedMailboxStorageEntry) { - ProtectedMailboxStorageEntry entry = (ProtectedMailboxStorageEntry) storedData; - // publicKey is not the same (stored: sender, new: receiver) - boolean result = entry.getReceiversPubKey().equals(receiversPubKey) - && get32ByteHashAsByteArray(entry.getProtectedStoragePayload()).equals(hashOfData); - if (!result) - log.warn("New data entry does not match our stored data. entry.receiversPubKey=" + entry.getReceiversPubKey() - + ", receiversPubKey=" + receiversPubKey); - - return result; - } else { - log.error("We expected a MailboxData but got other type. That must never happen. storedData=" + storedData); - return false; - } - } - private void broadcast(BroadcastMessage message, @Nullable NodeAddress sender, @Nullable BroadcastHandler.Listener listener, boolean isDataOwner) { broadcaster.broadcast(message, sender, listener, isDataOwner); 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 86b8673f937..354d3c93479 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 @@ -145,6 +145,31 @@ public boolean isValidForRemoveOperation() { return result; } + @Override + /* + * Returns true if the Entry metadata that is expected to stay constant between different versions of the same object + * matches. For ProtectedMailboxStorageEntry, the receiversPubKey must stay the same. + */ + public boolean isMetadataEquals(ProtectedStorageEntry protectedStorageEntry) { + if (!(protectedStorageEntry instanceof ProtectedMailboxStorageEntry)) { + log.error("ProtectedMailboxStorageEntry::isMetadataEquals() failed due to object type mismatch. " + + "ProtectedMailboxStorageEntry required, but got\n" + protectedStorageEntry); + + return false; + } + + ProtectedMailboxStorageEntry protectedMailboxStorageEntry = (ProtectedMailboxStorageEntry) protectedStorageEntry; + + boolean result = protectedMailboxStorageEntry.getReceiversPubKey().equals(this.receiversPubKey); + if (!result) { + log.warn("ProtectedMailboxStorageEntry::isMetadataEquals() failed due to metadata mismatch. " + + "new.receiversPubKey=" + Utilities.bytesAsHexString(protectedMailboxStorageEntry.getReceiversPubKeyBytes()) + + "stored.receiversPubKey=" + Utilities.bytesAsHexString(this.getReceiversPubKeyBytes())); + } + + return result; + } + /////////////////////////////////////////////////////////////////////////////////////////// // PROTO BUFFER 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 b11876b1bed..0daff40beb4 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 @@ -227,6 +227,22 @@ boolean isSignatureValid() { } } + /* + * Returns true if the Entry metadata that is expected to stay constant between different versions of the same object + * matches. + */ + public boolean isMetadataEquals(ProtectedStorageEntry protectedStorageEntry) { + boolean result = protectedStorageEntry.getOwnerPubKey().equals(this.ownerPubKey); + + if (!result) { + log.warn("New data entry does not match our stored data. storedData.ownerPubKey=" + + (protectedStorageEntry.getOwnerPubKey() != null ? protectedStorageEntry.getOwnerPubKey().toString() : "null") + + ", ownerPubKey=" + this.ownerPubKey); + } + + return result; + } + @Override public String toString() { return "ProtectedStorageEntry {" + diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java index e820ba7ef0d..9be1730e9a8 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java @@ -54,9 +54,8 @@ private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloa prefixedSealedAndSignedMessageMock, payloadSenderPubKeyForAddOperation, payloadOwnerPubKey); } - private static ProtectedMailboxStorageEntry buildProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, KeyPair ownerKey, PublicKey receiverKey) throws CryptoException { - int sequenceNumber = 1; - + private static ProtectedMailboxStorageEntry buildProtectedMailboxStorageEntry( + MailboxStoragePayload mailboxStoragePayload, KeyPair ownerKey, PublicKey receiverKey, int sequenceNumber) throws CryptoException { byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(mailboxStoragePayload, sequenceNumber)); byte[] signature = Sig.sign(ownerKey.getPrivate(), hashOfDataAndSeqNr); @@ -77,7 +76,7 @@ public void isValidForAddOperation() throws NoSuchAlgorithmException, CryptoExce KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); } @@ -89,7 +88,7 @@ public void isValidForAddOperation_EntryOwnerPayloadReceiverMismatch() throws No KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic(), 1); Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); } @@ -103,7 +102,7 @@ public void isValidForAddOperation_EntryReceiverPayloadReceiverMismatch() throws KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, senderKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, senderKeys.getPublic(), 1); // should be assertFalse Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); @@ -116,7 +115,7 @@ public void isValidForAddOperation_BadSignature() throws NoSuchAlgorithmExceptio KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); protectedStorageEntry.updateSignature( new byte[] { 0 }); @@ -130,7 +129,7 @@ public void validForRemove() throws NoSuchAlgorithmException, CryptoException { KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic(), 1); Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); } @@ -142,7 +141,7 @@ public void validForRemoveEntryOwnerPayloadOwnerMismatch() throws NoSuchAlgorith KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } @@ -154,7 +153,7 @@ public void isValidForRemoveOperation_BadSignature() throws NoSuchAlgorithmExcep KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic(), 1); protectedStorageEntry.updateSignature(new byte[] { 0 }); @@ -168,8 +167,36 @@ public void isValidForRemoveOperation_ReceiversPubKeyMismatch() throws NoSuchAlg KeyPair receiverKeys = TestUtils.generateKeyPair(); MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); - ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, senderKeys.getPublic()); + ProtectedStorageEntry protectedStorageEntry = buildProtectedMailboxStorageEntry(mailboxStoragePayload, receiverKeys, senderKeys.getPublic(), 1); Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } + + // TESTCASE: isMetadataEquals() should succeed if the sequence number changes + @Test + public void isMetadataEquals() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry seqNrOne = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); + + ProtectedStorageEntry seqNrTwo = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 2); + + Assert.assertTrue(seqNrOne.isMetadataEquals(seqNrTwo)); + } + + // TESTCASE: isMetadataEquals() should fail if the receiversPubKey changes + @Test + public void isMetadataEquals_receiverPubKeyChanged() throws NoSuchAlgorithmException, CryptoException { + KeyPair senderKeys = TestUtils.generateKeyPair(); + KeyPair receiverKeys = TestUtils.generateKeyPair(); + + MailboxStoragePayload mailboxStoragePayload = buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()); + ProtectedStorageEntry seqNrOne = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 1); + + ProtectedStorageEntry seqNrTwo = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, senderKeys.getPublic(), 1); + + Assert.assertFalse(seqNrOne.isMetadataEquals(seqNrTwo)); + } } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java index 05f27d4809c..fabacc3e261 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java @@ -40,14 +40,12 @@ import static org.mockito.Mockito.when; public class ProtectedStorageEntryTest { - private static ProtectedStorageEntry buildProtectedStorageEntry(KeyPair payloadOwner, KeyPair entryOwner) throws CryptoException { - return buildProtectedStorageEntry(new ProtectedStoragePayloadStub(payloadOwner.getPublic()), entryOwner); + private static ProtectedStorageEntry buildProtectedStorageEntry(KeyPair payloadOwner, KeyPair entryOwner, int sequenceNumber) throws CryptoException { + return buildProtectedStorageEntry(new ProtectedStoragePayloadStub(payloadOwner.getPublic()), entryOwner, sequenceNumber); } private static ProtectedStorageEntry buildProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, - KeyPair entryOwner) throws CryptoException { - - int sequenceNumber = 1; + KeyPair entryOwner, int sequenceNumber) throws CryptoException { byte[] hashOfDataAndSeqNr = P2PDataStorage.get32ByteHash(new P2PDataStorage.DataAndSeqNrPair(protectedStoragePayload, sequenceNumber)); byte[] signature = Sig.sign(entryOwner.getPrivate(), hashOfDataAndSeqNr); @@ -82,7 +80,7 @@ public void SetUp() { @Test public void isValidForAddOperation() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); } @@ -92,7 +90,7 @@ public void isValidForAddOperation() throws NoSuchAlgorithmException, CryptoExce public void isValidForAddOperation_Mismatch() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); KeyPair notOwnerKeys = TestUtils.generateKeyPair(); - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys, 1); Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); } @@ -106,7 +104,7 @@ public void isValidForAddOperation_invalidMailboxPayloadSender() throws NoSuchAl KeyPair receiverKeys = TestUtils.generateKeyPair(); ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( - buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys); + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys, 1); // should be assertFalse Assert.assertTrue(protectedStorageEntry.isValidForAddOperation()); @@ -120,7 +118,7 @@ public void isValidForAddOperation_invalidMailboxPayloadReceiver() throws NoSuch KeyPair receiverKeys = TestUtils.generateKeyPair(); ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( - buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys); + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys, 1); Assert.assertFalse(protectedStorageEntry.isValidForAddOperation()); } @@ -129,7 +127,7 @@ public void isValidForAddOperation_invalidMailboxPayloadReceiver() throws NoSuch @Test public void isValidForAddOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); protectedStorageEntry.updateSignature( new byte[] { 0 }); @@ -140,7 +138,7 @@ public void isValidForAddOperation_BadSignature() throws NoSuchAlgorithmExceptio @Test public void isValidForRemoveOperation() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); } @@ -150,7 +148,7 @@ public void isValidForRemoveOperation() throws NoSuchAlgorithmException, CryptoE public void isValidForRemoveOperation_Mismatch() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); KeyPair notOwnerKeys = TestUtils.generateKeyPair(); - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, notOwnerKeys, 1); Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } @@ -164,7 +162,7 @@ public void isValidForRemoveOperation_invalidMailboxPayloadSender() throws NoSuc KeyPair receiverKeys = TestUtils.generateKeyPair(); ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( - buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys); + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), senderKeys, 1); // should be assertFalse Assert.assertTrue(protectedStorageEntry.isValidForRemoveOperation()); @@ -176,7 +174,7 @@ public void isValidForRemoveOperation_invalidMailboxPayloadReceiver() throws NoS KeyPair receiverKeys = TestUtils.generateKeyPair(); ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry( - buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys); + buildMailboxStoragePayload(senderKeys.getPublic(), receiverKeys.getPublic()), receiverKeys, 1); Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } @@ -185,10 +183,34 @@ public void isValidForRemoveOperation_invalidMailboxPayloadReceiver() throws NoS @Test public void isValidForRemoveOperation_BadSignature() throws NoSuchAlgorithmException, CryptoException { KeyPair ownerKeys = TestUtils.generateKeyPair(); - ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys); + ProtectedStorageEntry protectedStorageEntry = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); protectedStorageEntry.updateSignature(new byte[] { 0 }); Assert.assertFalse(protectedStorageEntry.isValidForRemoveOperation()); } + + // TESTCASE: isMetadataEquals() should succeed if the sequence number changes + @Test + public void isMetadataEquals() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + ProtectedStorageEntry seqNrOne = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); + + ProtectedStorageEntry seqNrTwo = buildProtectedStorageEntry(ownerKeys, ownerKeys, 2); + + Assert.assertTrue(seqNrOne.isMetadataEquals(seqNrTwo)); + } + + // TESTCASE: isMetadataEquals() should fail if the OwnerPubKey changes + @Test + public void isMetadataEquals_OwnerPubKeyChanged() throws NoSuchAlgorithmException, CryptoException { + KeyPair ownerKeys = TestUtils.generateKeyPair(); + KeyPair notOwner = TestUtils.generateKeyPair(); + + ProtectedStorageEntry protectedStorageEntryOne = buildProtectedStorageEntry(ownerKeys, ownerKeys, 1); + + ProtectedStorageEntry protectedStorageEntryTwo = buildProtectedStorageEntry(ownerKeys, notOwner, 1); + + Assert.assertFalse(protectedStorageEntryOne.isMetadataEquals(protectedStorageEntryTwo)); + } } From 53b5feb7a0c1949ded6ffee597abf9a6f7dacf92 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 20:31:22 -0800 Subject: [PATCH 28/30] [TESTS] Update remove validation with BroadcastMessage type Make the remove validation more robust by asserting that the correct remove message is broadcast. This will provide a better safety net when combining the remove functions. --- .../java/bisq/network/p2p/storage/P2PDataStorageTest.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 c06bbfad57e..5b3dd4abaea 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -392,8 +392,12 @@ private static void verifyProtectedStorageRemove(TestState currentState, 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)); + if (expectedBroadcastOnStateChange) { + if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) + verify(currentState.mockBroadcaster).broadcast(any(RemoveMailboxDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); + else + verify(currentState.mockBroadcaster).broadcast(any(RemoveDataMessage.class), any(NodeAddress.class), eq(null), eq(expectedIsDataOwner)); + } } else { Assert.assertEquals(beforeState.protectedStorageEntryBeforeOp, currentState.mockedStorage.getMap().get(hashMapHash)); From 5ae9dd1e17ab4f6fda047729cb7724e8415f35d9 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Thu, 7 Nov 2019 20:33:11 -0800 Subject: [PATCH 29/30] Combine remove() and removeMailboxData() Now that all the code is abstracted and tested, the remove() and removeMailboxData() functions are identical. Combine them and update callers appropriately. Now, any caller don't need to know the difference and it removes the sharp edge originally found in #3556 --- .../java/bisq/network/p2p/P2PService.java | 2 +- .../network/p2p/storage/P2PDataStorage.java | 53 +++---------------- .../p2p/storage/P2PDataStorageTest.java | 22 ++------ 3 files changed, 12 insertions(+), 65 deletions(-) diff --git a/p2p/src/main/java/bisq/network/p2p/P2PService.java b/p2p/src/main/java/bisq/network/p2p/P2PService.java index f900a88c7ae..d90fdeb2565 100644 --- a/p2p/src/main/java/bisq/network/p2p/P2PService.java +++ b/p2p/src/main/java/bisq/network/p2p/P2PService.java @@ -759,7 +759,7 @@ private void delayedRemoveEntryFromMailbox(DecryptedMessageWithPubKey decryptedM expirableMailboxStoragePayload, keyRing.getSignatureKeyPair(), receiversPubKey); - p2PDataStorage.removeMailboxData(protectedMailboxStorageEntry, networkNode.getNodeAddress(), true); + p2PDataStorage.remove(protectedMailboxStorageEntry, networkNode.getNodeAddress(), true); } 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 b2542412dc3..d926e7f31d1 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -240,7 +240,7 @@ public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) { } else if (networkEnvelope instanceof RemoveDataMessage) { remove(((RemoveDataMessage) networkEnvelope).getProtectedStorageEntry(), peersNodeAddress, false); } else if (networkEnvelope instanceof RemoveMailboxDataMessage) { - removeMailboxData(((RemoveMailboxDataMessage) networkEnvelope).getProtectedMailboxStorageEntry(), peersNodeAddress, false); + remove(((RemoveMailboxDataMessage) networkEnvelope).getProtectedMailboxStorageEntry(), peersNodeAddress, false); } else if (networkEnvelope instanceof RefreshOfferMessage) { refreshTTL((RefreshOfferMessage) networkEnvelope, peersNodeAddress, false); } else if (networkEnvelope instanceof AddPersistableNetworkPayloadMessage) { @@ -485,8 +485,6 @@ 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); @@ -519,7 +517,11 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); - broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner); + if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) { + broadcast(new RemoveMailboxDataMessage((ProtectedMailboxStorageEntry) protectedStorageEntry), sender, null, isDataOwner); + } else { + broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner); + } removeFromProtectedDataStore(protectedStorageEntry); @@ -570,48 +572,7 @@ private void removeFromProtectedDataStore(ProtectedStorageEntry protectedStorage } } } - - @SuppressWarnings("UnusedReturnValue") - public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxStorageEntry, - @Nullable NodeAddress sender, - boolean isDataOwner) { - ProtectedStoragePayload protectedStoragePayload = protectedMailboxStorageEntry.getProtectedStoragePayload(); - ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload); - - ProtectedStorageEntry storedEntry = map.get(hashOfPayload); - if (storedEntry == null) { - log.debug("removeMailboxData failed due to unknown entry"); - - return false; - } - - int sequenceNumber = protectedMailboxStorageEntry.getSequenceNumber(); - - if (!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) - return false; - - if (!protectedMailboxStorageEntry.isValidForRemoveOperation()) - return false; - - // Verify the metadata between the stored entry and the new remove entry are the same - if (!protectedMailboxStorageEntry.isMetadataEquals(storedEntry)) - 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, this.clock.millis())); - sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); - - maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); - - broadcast(new RemoveMailboxDataMessage(protectedMailboxStorageEntry), sender, null, isDataOwner); - - return true; - } - + private void maybeAddToRemoveAddOncePayloads(ProtectedStoragePayload protectedStoragePayload, ByteArray hashOfData) { if (protectedStoragePayload instanceof AddOncePayload) { 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 5b3dd4abaea..0586350296c 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -1130,7 +1130,7 @@ boolean doRemove(ProtectedStorageEntry entry) { return true; } else { // XXX: All external callers just pass in true, a future patch can remove the argument. - return testState.mockedStorage.removeMailboxData((ProtectedMailboxStorageEntry) entry, getTestNodeAddress(), true); + return testState.mockedStorage.remove(entry, getTestNodeAddress(), true); } } @@ -1280,20 +1280,6 @@ public void remove_receiversPubKeyChanged() throws NoSuchAlgorithmException, Cry doProtectedStorageRemoveAndVerify(this.getProtectedStorageEntryForRemove(2), false, false); } - - @Test(expected = IllegalArgumentException.class) - public void remove_canCallWrongRemoveAndFail() throws CryptoException { - - ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); - ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1); - - doProtectedStorageAddAndVerify(entryForAdd, true, true); - - // Call remove(ProtectedStorageEntry) instead of removeFromMailbox(ProtectedMailboxStorageEntry) and verify - // it fails spectacularly - super.doRemove(entryForRemove); - } - // TESTCASE: Add after removed when add-once required (greater seq #) @Override @Test @@ -1445,7 +1431,7 @@ public void getMailboxDataWithSignedSeqNr_RemoveNoExist() throws NoSuchAlgorithm this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry); - Assert.assertFalse(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true)); + Assert.assertFalse(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, getTestNodeAddress(), true)); verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, false, true, true, true); } @@ -1467,7 +1453,7 @@ public void getMailboxDataWithSignedSeqNr_AddThenRemove() throws NoSuchAlgorithm this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry); - Assert.assertTrue(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true)); + Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, getTestNodeAddress(), true)); verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true, true,true); } @@ -1493,7 +1479,7 @@ public void getMailboxDataWithSignedSeqNr_ValidRemoveAddFromMessage() throws NoS this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic()); SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry); - Assert.assertTrue(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true)); + Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, getTestNodeAddress(), true)); verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true, true,true); } From 5d35d08b00f3a39450bd3595dc9eead6c5e66807 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Tue, 12 Nov 2019 16:28:13 -0800 Subject: [PATCH 30/30] [PR COMMENTS] Logging format and function rename Use a more compact version of string formatting in log messages Rename isMetadataEquals to matchesRelevantPubKey which is more descriptive of the actual check --- .../bisq/network/p2p/storage/P2PDataStorage.java | 4 ++-- .../payload/ProtectedMailboxStorageEntry.java | 14 +++++++------- .../p2p/storage/payload/ProtectedStorageEntry.java | 14 +++++++------- .../payload/ProtectedMailboxStorageEntryTest.java | 4 ++-- .../storage/payload/ProtectedStorageEntryTest.java | 4 ++-- 5 files changed, 20 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 d926e7f31d1..98e7671b305 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -406,7 +406,7 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn ProtectedStorageEntry storedEntry = map.get(hashOfPayload); // If we have already seen an Entry with the same hash, verify the metadata is equal - if (storedEntry != null && !protectedStorageEntry.isMetadataEquals(storedEntry)) + if (storedEntry != null && !protectedStorageEntry.matchesRelevantPubKey(storedEntry)) return false; // This is an updated entry. Record it and signal listeners. @@ -504,7 +504,7 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry, return false; // If we have already seen an Entry with the same hash, verify the metadata is the same - if (!protectedStorageEntry.isMetadataEquals(storedEntry)) + if (!protectedStorageEntry.matchesRelevantPubKey(storedEntry)) return false; // Valid remove entry, do the remove and signal listeners 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 354d3c93479..f0d51ef6108 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 @@ -103,9 +103,9 @@ public boolean isValidForAddOperation() { if (mailboxStoragePayload != null && mailboxStoragePayload.getOwnerPubKey() != null) res2 = Utilities.encodeToHex(mailboxStoragePayload.getSenderPubKeyForAddOperation().getEncoded(),true); - log.warn(String.format("ProtectedMailboxStorageEntry::isValidForAddOperation() failed. " + - "Entry owner does not match sender key in payload:\nProtectedStorageEntry=%s\n" + - "SenderPubKeyForAddOperation=%s", res1, res2)); + log.warn("ProtectedMailboxStorageEntry::isValidForAddOperation() failed. " + + "Entry owner does not match sender key in payload:\nProtectedStorageEntry=%{}\n" + + "SenderPubKeyForAddOperation=%{}", res1, res2); } return result; @@ -137,9 +137,9 @@ public boolean isValidForRemoveOperation() { if (mailboxStoragePayload != null && mailboxStoragePayload.getOwnerPubKey() != null) res2 = Utilities.encodeToHex(mailboxStoragePayload.getOwnerPubKey().getEncoded(), true); - log.warn(String.format("ProtectedMailboxStorageEntry::isValidForRemoveOperation() failed. " + - "Entry owner does not match Payload owner:\nProtectedStorageEntry=%s\n" + - "PayloadOwner=%s", res1, res2)); + log.warn("ProtectedMailboxStorageEntry::isValidForRemoveOperation() failed. " + + "Entry owner does not match Payload owner:\nProtectedStorageEntry={}\n" + + "PayloadOwner={}", res1, res2); } return result; @@ -150,7 +150,7 @@ public boolean isValidForRemoveOperation() { * Returns true if the Entry metadata that is expected to stay constant between different versions of the same object * matches. For ProtectedMailboxStorageEntry, the receiversPubKey must stay the same. */ - public boolean isMetadataEquals(ProtectedStorageEntry protectedStorageEntry) { + public boolean matchesRelevantPubKey(ProtectedStorageEntry protectedStorageEntry) { if (!(protectedStorageEntry instanceof ProtectedMailboxStorageEntry)) { log.error("ProtectedMailboxStorageEntry::isMetadataEquals() failed due to object type mismatch. " + "ProtectedMailboxStorageEntry required, but got\n" + protectedStorageEntry); 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 0daff40beb4..a055e101018 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 @@ -177,8 +177,8 @@ public boolean isValidForAddOperation() { if (protectedStoragePayload != null && protectedStoragePayload.getOwnerPubKey() != null) res2 = Utilities.encodeToHex(protectedStoragePayload.getOwnerPubKey().getEncoded(), true); - log.warn(String.format("ProtectedStorageEntry::isValidForAddOperation() failed. Entry owner does not match Payload owner:\n" + - "ProtectedStorageEntry=%s\nPayloadOwner=%s", res1, res2)); + log.warn("ProtectedStorageEntry::isValidForAddOperation() failed. Entry owner does not match Payload owner:\n" + + "ProtectedStorageEntry={}\nPayloadOwner={}", res1, res2); } return result; @@ -200,8 +200,8 @@ public boolean isValidForRemoveOperation() { if (protectedStoragePayload != null && protectedStoragePayload.getOwnerPubKey() != null) res2 = Utilities.encodeToHex(protectedStoragePayload.getOwnerPubKey().getEncoded(), true); - log.warn(String.format("ProtectedStorageEntry::isValidForRemoveOperation() failed. Entry owner does not match Payload owner:\n" + - "ProtectedStorageEntry=%s\nPayloadOwner=%s", res1, res2)); + log.warn("ProtectedStorageEntry::isValidForRemoveOperation() failed. Entry owner does not match Payload owner:\n" + + "ProtectedStorageEntry={}\nPayloadOwner={}", res1, res2); } return result; @@ -218,11 +218,11 @@ boolean isSignatureValid() { boolean result = Sig.verify(this.ownerPubKey, hashOfDataAndSeqNr, this.signature); if (!result) - log.warn(String.format("ProtectedStorageEntry::isSignatureValid() failed.\n%s", this)); + log.warn("ProtectedStorageEntry::isSignatureValid() failed.\n{}}", this); return result; } catch (CryptoException e) { - log.error(String.format("ProtectedStorageEntry::isSignatureValid() exception %s", e.toString())); + log.error("ProtectedStorageEntry::isSignatureValid() exception {}", e.toString()); return false; } } @@ -231,7 +231,7 @@ boolean isSignatureValid() { * Returns true if the Entry metadata that is expected to stay constant between different versions of the same object * matches. */ - public boolean isMetadataEquals(ProtectedStorageEntry protectedStorageEntry) { + public boolean matchesRelevantPubKey(ProtectedStorageEntry protectedStorageEntry) { boolean result = protectedStorageEntry.getOwnerPubKey().equals(this.ownerPubKey); if (!result) { diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java index 9be1730e9a8..b97850fc381 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedMailboxStorageEntryTest.java @@ -183,7 +183,7 @@ public void isMetadataEquals() throws NoSuchAlgorithmException, CryptoException ProtectedStorageEntry seqNrTwo = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, receiverKeys.getPublic(), 2); - Assert.assertTrue(seqNrOne.isMetadataEquals(seqNrTwo)); + Assert.assertTrue(seqNrOne.matchesRelevantPubKey(seqNrTwo)); } // TESTCASE: isMetadataEquals() should fail if the receiversPubKey changes @@ -197,6 +197,6 @@ public void isMetadataEquals_receiverPubKeyChanged() throws NoSuchAlgorithmExcep ProtectedStorageEntry seqNrTwo = buildProtectedMailboxStorageEntry(mailboxStoragePayload, senderKeys, senderKeys.getPublic(), 1); - Assert.assertFalse(seqNrOne.isMetadataEquals(seqNrTwo)); + Assert.assertFalse(seqNrOne.matchesRelevantPubKey(seqNrTwo)); } } diff --git a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java index fabacc3e261..b32ee34c943 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/payload/ProtectedStorageEntryTest.java @@ -198,7 +198,7 @@ public void isMetadataEquals() throws NoSuchAlgorithmException, CryptoException ProtectedStorageEntry seqNrTwo = buildProtectedStorageEntry(ownerKeys, ownerKeys, 2); - Assert.assertTrue(seqNrOne.isMetadataEquals(seqNrTwo)); + Assert.assertTrue(seqNrOne.matchesRelevantPubKey(seqNrTwo)); } // TESTCASE: isMetadataEquals() should fail if the OwnerPubKey changes @@ -211,6 +211,6 @@ public void isMetadataEquals_OwnerPubKeyChanged() throws NoSuchAlgorithmExceptio ProtectedStorageEntry protectedStorageEntryTwo = buildProtectedStorageEntry(ownerKeys, notOwner, 1); - Assert.assertFalse(protectedStorageEntryOne.isMetadataEquals(protectedStorageEntryTwo)); + Assert.assertFalse(protectedStorageEntryOne.matchesRelevantPubKey(protectedStorageEntryTwo)); } }