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/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..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; @@ -93,12 +91,15 @@ import javax.annotation.Nullable; +import static com.google.common.base.Preconditions.checkArgument; + @Slf4j public class P2PDataStorage implements MessageListener, ConnectionListener, PersistedDataHost { /** * 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; @@ -121,6 +122,8 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers private final Set protectedDataStoreListeners = new CopyOnWriteArraySet<>(); private final Clock clock; + protected int maxSequenceNumberMapSizeBeforePurge; + /////////////////////////////////////////////////////////////////////////////////////////// // Constructor /////////////////////////////////////////////////////////////////////////////////////////// @@ -145,6 +148,7 @@ public P2PDataStorage(NetworkNode networkNode, this.sequenceNumberMapStorage = sequenceNumberMapStorage; sequenceNumberMapStorage.setNumMaxBackupFiles(5); + this.maxSequenceNumberMapSizeBeforePurge = 1000; } @Override @@ -175,40 +179,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()) - .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() > this.maxSequenceNumberMapSizeBeforePurge) + 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() { @@ -283,7 +290,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); @@ -314,33 +321,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 { - log.warn("We got a hash exceeding our permitted size"); + // Payload hash size does not match expectation for that type of message. + if (!payload.verifyHashSize()) { + log.warn("addPersistableNetworkPayload failed due to unexpected hash 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("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("addPersistableNetworkPayload failed due to payload time outside 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 @@ -380,50 +395,41 @@ public boolean addProtectedStorageEntry(ProtectedStorageEntry protectedStorageEn return false; } - boolean sequenceNrValid = isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload); - boolean result = sequenceNrValid && - checkPublicKeys(protectedStorageEntry, true) - && checkSignature(protectedStorageEntry); + // If we have seen a more recent operation for this payload, we ignore the current one + if(!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + return false; - boolean containsKey = map.containsKey(hashOfPayload); - if (containsKey) { - result = result && checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload); - } + // Verify the ProtectedStorageEntry is well formed and valid for the add operation + if (!checkPublicKeys(protectedStorageEntry, true) || !checkSignature(protectedStorageEntry)) + return false; - // printData("before add"); - if (result) { - boolean hasSequenceNrIncreased = hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), 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; + } - 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."); - } + // This is an updated entry. Record it and signal listeners. + map.put(hashOfPayload, protectedStorageEntry); + hashMapChangedListeners.forEach(e -> e.onAdded(protectedStorageEntry)); - 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); + // Record the updated sequence number and persist it. Higher delay so we can batch more items. + sequenceNumberMap.put(hashOfPayload, new MapValue(protectedStorageEntry.getSequenceNumber(), this.clock.millis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 2000); - if (allowBroadcast) - broadcastProtectedStorageEntry(protectedStorageEntry, sender, listener, isDataOwner); - } else { - log.trace("We got that version of the data already, so we don't broadcast it."); - } + // Optionally, broadcast the add/update depending on the calling environment + if (allowBroadcast) + broadcastProtectedStorageEntry(protectedStorageEntry, sender, listener, isDataOwner); - 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"); + // 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, @@ -436,73 +442,92 @@ 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)) { - 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; - } + ByteArray hashOfPayload = new ByteArray(refreshTTLMessage.getHashOfPayload()); - 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; } + + ProtectedStorageEntry storedEntry = map.get(hashOfPayload); + ProtectedStorageEntry updatedEntry = new ProtectedStorageEntry( + storedEntry.getProtectedStoragePayload(), + storedEntry.getOwnerPubKey(), + refreshTTLMessage.getSequenceNumber(), + refreshTTLMessage.getSignature(), + this.clock); + + + // 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 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)) + return false; + + // 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(updatedEntry.getSequenceNumber(), this.clock.millis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 1000); + + // Always broadcast refreshes + broadcast(refreshTTLMessage, sender, null, isDataOwner); + + return true; } 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); - 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); + return false; + } - // 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); + // If we have seen a more recent operation for this payload, ignore this one + if (!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + return false; - maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); + // Verify the ProtectedStorageEntry is well formed and valid for the remove operation + if (!checkPublicKeys(protectedStorageEntry, false) || !checkSignature(protectedStorageEntry)) + return false; - broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner); + // 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; - removeFromProtectedDataStore(protectedStorageEntry); - } else { - log.debug("remove failed"); - } - return result; - } + // 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(), this.clock.millis())); + sequenceNumberMapStorage.queueUpForSave(SequenceNumberMap.clone(sequenceNumberMap), 300); + + maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload); + + broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner); + + removeFromProtectedDataStore(protectedStorageEntry); + + return true; +} /** @@ -555,33 +580,46 @@ 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 (!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) + 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) || !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; } - return result; + + // 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); + 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, @@ -603,7 +641,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, @@ -635,7 +673,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) { @@ -676,24 +714,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; @@ -823,7 +843,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); 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..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,11 +42,38 @@ public ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, PublicKey ownerPubKey, int sequenceNumber, byte[] signature, - PublicKey receiversPubKey) { - super(mailboxStoragePayload, ownerPubKey, sequenceNumber, signature); + PublicKey receiversPubKey, + Clock clock) { + this(mailboxStoragePayload, + Sig.getPublicKeyBytes(ownerPubKey), + ownerPubKey, + sequenceNumber, + signature, + Sig.getPublicKeyBytes(receiversPubKey), + receiversPubKey, + clock.millis(), + clock); + } + + private ProtectedMailboxStorageEntry(MailboxStoragePayload mailboxStoragePayload, + byte[] ownerPubKeyBytes, + PublicKey ownerPubKey, + int sequenceNumber, + byte[] signature, + byte[] receiversPubKeyBytes, + PublicKey receiversPubKey, + long creationTimeStamp, + Clock clock) { + super(mailboxStoragePayload, + ownerPubKeyBytes, + ownerPubKey, + sequenceNumber, + signature, + creationTimeStamp, + clock); this.receiversPubKey = receiversPubKey; - receiversPubKeyBytes = Sig.getPublicKeyBytes(receiversPubKey); + this.receiversPubKeyBytes = receiversPubKeyBytes; } public MailboxStoragePayload getMailboxStoragePayload() { @@ -56,22 +85,22 @@ 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, + Clock clock) { + this(mailboxStoragePayload, + ownerPubKeyBytes, + Sig.getPublicKeyFromBytes(ownerPubKeyBytes), sequenceNumber, - signature); - - this.receiversPubKeyBytes = receiversPubKeyBytes; - receiversPubKey = Sig.getPublicKeyFromBytes(receiversPubKeyBytes); - - maybeAdjustCreationTimeStamp(); + signature, + receiversPubKeyBytes, + Sig.getPublicKeyFromBytes(receiversPubKeyBytes), + creationTimeStamp, + clock); } public protobuf.ProtectedMailboxStorageEntry toProtoMessage() { @@ -85,12 +114,13 @@ 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(), + 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 b967caf594f..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 @@ -27,6 +27,8 @@ import java.security.PublicKey; +import java.time.Clock; + import lombok.EqualsAndHashCode; import lombok.Getter; import lombok.extern.slf4j.Slf4j; @@ -38,42 +40,60 @@ 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; public ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, + PublicKey ownerPubKey, + int sequenceNumber, + byte[] signature, + Clock clock) { + this(protectedStoragePayload, + Sig.getPublicKeyBytes(ownerPubKey), + ownerPubKey, + sequenceNumber, + signature, + clock.millis(), + clock); + } + + protected ProtectedStorageEntry(ProtectedStoragePayload protectedStoragePayload, + byte[] ownerPubKeyBytes, PublicKey ownerPubKey, int sequenceNumber, - byte[] signature) { + byte[] signature, + long creationTimeStamp, + Clock clock) { + 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(clock); + } /////////////////////////////////////////////////////////////////////////////////////////// // 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, + Clock clock) { + this(protectedStoragePayload, + ownerPubKeyBytes, + Sig.getPublicKeyFromBytes(ownerPubKeyBytes), + sequenceNumber, + signature, + creationTimeStamp, + clock); } public Message toProtoMessage() { @@ -93,11 +113,13 @@ 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(), + resolver.getClock()); } @@ -105,14 +127,10 @@ 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(); - } - - public void refreshTTL() { - creationTimeStamp = System.currentTimeMillis(); + if (creationTimeStamp > clock.millis()) + creationTimeStamp = clock.millis(); } public void backDate() { @@ -120,16 +138,13 @@ public void backDate() { 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; } - 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 41964e2715c..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; @@ -67,13 +69,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 +90,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 { @@ -98,16 +111,36 @@ static class TestState { final ProtectedDataStoreListener protectedDataStoreListener; final HashMapChangedListener hashMapChangedListener; 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), - this.mockSeqNrStorage, Clock.systemUTC()); + this.mockSeqNrStorage, this.clockFake); this.appendOnlyDataStoreListener = mock(AppendOnlyDataStoreListener.class); this.protectedDataStoreListener = mock(ProtectedDataStoreListener.class); @@ -125,6 +158,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 +224,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); + return new ProtectedStorageEntry(protectedStoragePayload, entryOwnerKeys.getPublic(), sequenceNumber, signature, clock); } private static MailboxStoragePayload buildMailboxStoragePayload(PublicKey payloadSenderPubKeyForAddOperation, @@ -213,14 +251,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); + entryOwnerPubKey, sequenceNumber, signature, entryReceiversPubKey, clock); } private static RefreshOfferMessage buildRefreshOfferMessage(ProtectedStoragePayload protectedStoragePayload, @@ -333,6 +372,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()); @@ -348,9 +389,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)); @@ -689,7 +733,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 +741,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, @@ -725,7 +769,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 @@ -737,12 +781,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 +793,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 @@ -772,21 +815,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 @@ -804,7 +843,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); } @@ -819,8 +858,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); @@ -828,8 +865,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 #) @@ -866,7 +902,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); } @@ -881,7 +917,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, this.testState.clockFake); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -977,7 +1013,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,8 +1022,13 @@ 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); - doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), true, false); + + this.testState.incrementClock(); + + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), false, false); } // TESTCASE: Duplicate refresh message (greater seq #) @@ -996,7 +1037,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); } @@ -1006,18 +1052,25 @@ 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); } // 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 @@ -1032,17 +1085,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()); } } @@ -1086,12 +1132,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 @@ -1104,7 +1150,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); } @@ -1116,7 +1162,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); } @@ -1129,7 +1175,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, this.testState.clockFake); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -1142,7 +1188,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, this.testState.clockFake); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -1155,19 +1201,14 @@ 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); } - // 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 +1216,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 #) @@ -1241,7 +1253,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); @@ -1255,7 +1267,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)); @@ -1272,7 +1284,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); @@ -1292,7 +1304,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); @@ -1307,7 +1319,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); @@ -1316,6 +1328,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)); @@ -1327,7 +1341,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); @@ -1338,6 +1352,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)); @@ -1358,7 +1374,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 @@ -1380,7 +1396,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 @@ -1391,7 +1407,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())); @@ -1406,7 +1422,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); } } @@ -1416,7 +1432,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); @@ -1518,9 +1534,152 @@ 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); } } + + 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 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 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 < 4; ++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 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)); + + // 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); + } + } } 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(); 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); + } +}