From 5512f34566418f4b273becdbf4387348666a1dd7 Mon Sep 17 00:00:00 2001 From: Julian Knutsen Date: Sat, 26 Oct 2019 00:06:34 +0000 Subject: [PATCH 1/6] [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 2/6] [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 3/6] [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 4/6] [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 5/6] [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 6/6] [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);