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 c1f54bb429..7f1a20e41d 100644 --- a/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java +++ b/p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java @@ -390,28 +390,19 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers return false; } - // TODO: Combine with hasSequenceNrIncreased check, but keep existing behavior for now - if(!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + // If we have seen a more recent operation for this payload, we ignore the current one + if(!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) return false; // Verify the ProtectedStorageEntry is well formed and valid for the add operation if (!checkPublicKeys(protectedStorageEntry, true) || !checkSignature(protectedStorageEntry)) return false; - boolean containsKey = map.containsKey(hashOfPayload); - - // If we have already seen an Entry with the same hash, verify the new Entry has the same owner - if (containsKey && !checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) + // In a hash collision between two well formed ProtectedStorageEntry, the first item wins and will not be overwritten + if (map.containsKey(hashOfPayload) && + !checkIfStoredDataPubKeyMatchesNewDataPubKey(protectedStorageEntry.getOwnerPubKey(), hashOfPayload)) { return false; - - boolean hasSequenceNrIncreased = hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload); - - // If we have seen a more recent operation for this payload, we ignore the current one - // TODO: I think we can return false here. All callers use the Client API (addProtectedStorageEntry(getProtectedStorageEntry()) - // leaving only the onMessage() handler which doesn't look at the return value. It makes more intuitive sense that adds() that don't - // change state return false. - if (!hasSequenceNrIncreased) - return true; + } // This is an updated entry. Record it and signal listeners. map.put(hashOfPayload, protectedStorageEntry); @@ -456,17 +447,6 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers 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; @@ -517,7 +497,7 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers } // If we have seen a more recent operation for this payload, ignore this one - if (!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) + if (!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload)) return false; // Verify the ProtectedStorageEntry is well formed and valid for the remove operation @@ -605,7 +585,7 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers int sequenceNumber = protectedMailboxStorageEntry.getSequenceNumber(); - if (!isSequenceNrValid(sequenceNumber, hashOfPayload)) + if (!hasSequenceNrIncreased(sequenceNumber, hashOfPayload)) return false; PublicKey receiversPubKey = protectedMailboxStorageEntry.getReceiversPubKey(); @@ -730,24 +710,6 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers hashMapChangedListeners.forEach(e -> e.onRemoved(protectedStorageEntry)); } - private boolean isSequenceNrValid(int newSequenceNumber, ByteArray hashOfData) { - if (sequenceNumberMap.containsKey(hashOfData)) { - int storedSequenceNumber = sequenceNumberMap.get(hashOfData).sequenceNr; - if (newSequenceNumber >= storedSequenceNumber) { - log.trace("Sequence number is valid (>=). sequenceNumber = " - + newSequenceNumber + " / storedSequenceNumber=" + storedSequenceNumber); - return true; - } else { - log.debug("Sequence number is invalid. sequenceNumber = " - + newSequenceNumber + " / storedSequenceNumber=" + storedSequenceNumber + "\n" + - "That can happen if the data owner gets an old delayed data storage message."); - return false; - } - } else { - return true; - } - } - private boolean hasSequenceNrIncreased(int newSequenceNumber, ByteArray hashOfData) { if (sequenceNumberMap.containsKey(hashOfData)) { int storedSequenceNumber = sequenceNumberMap.get(hashOfData).sequenceNr; diff --git a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java index 51b8879c3a..02073b06d5 100644 --- a/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java +++ b/p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java @@ -737,12 +737,11 @@ public class P2PDataStorageTest { } // TESTCASE: Adding duplicate payload w/ same sequence number - // TODO: Should adds() of existing sequence #s return false since they don't update state? @Test public void addProtectedStorageEntry_duplicateSeqNrGt0() throws CryptoException { ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1); doProtectedStorageAddAndVerify(entryForAdd, true, true); - doProtectedStorageAddAndVerify(entryForAdd, true, false); + doProtectedStorageAddAndVerify(entryForAdd, false, false); } // TESTCASE: Adding duplicate payload w/ 0 sequence number (special branch in code for logging) @@ -750,7 +749,7 @@ public class P2PDataStorageTest { 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 +771,17 @@ public class P2PDataStorageTest { } // 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 @@ -819,8 +814,6 @@ public class P2PDataStorageTest { }*/ // 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 +821,7 @@ public class P2PDataStorageTest { 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 +858,7 @@ public class P2PDataStorageTest { 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 +873,7 @@ public class P2PDataStorageTest { // For standard ProtectedStorageEntrys the entry owner must match the payload owner for removes ProtectedStorageEntry entryForRemove = buildProtectedStorageEntry( - this.protectedStoragePayload, notOwner, notOwner, 1); + this.protectedStoragePayload, notOwner, notOwner, 2); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -977,7 +969,7 @@ public class P2PDataStorageTest { 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 #) @@ -987,7 +979,7 @@ public class P2PDataStorageTest { doProtectedStorageAddAndVerify(entry, true, true); doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), true, true); - doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), true, false); + doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys, 2), false, false); } // TESTCASE: Duplicate refresh message (greater seq #) @@ -1013,11 +1005,13 @@ public class P2PDataStorageTest { // 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); - doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,3), false, false); + doProtectedStorageAddAndVerify(entryForAdd, true, true); + doProtectedStorageRemoveAndVerify(entryForRemove, true, true); + + doRefreshTTLAndVerify(buildRefreshOfferMessage(entryForAdd, this.payloadOwnerKeys,3), false, false); } // TESTCASE: Refresh an entry, but owner doesn't match PubKey of original add owner @@ -1129,7 +1123,7 @@ public class P2PDataStorageTest { KeyPair notReceiver = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 1); + ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 2); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); } @@ -1142,7 +1136,7 @@ public class P2PDataStorageTest { KeyPair notSender = TestUtils.generateKeyPair(); - ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 1); + ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 2); doProtectedStorageRemoveAndVerify(entryForRemove, false, false); }