mirrors/bisq
1
0
Fork 0
mirror of https://github.com/bisq-network/bisq.git synced 2024-11-19 09:52:23 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #3558 from julianknutsen/update-duplicate-behavior

Browse Source 
(4/8) Update behavior of add/remove/refresh on duplicate sequence numbers
This commit is contained in:
Christoph Atteneder 2019-11-18 11:01:51 +01:00 committed by GitHub
commit fe1dd20b35
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 27 additions and 71 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
View File

@ -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;

44
p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java
View File

@ -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);
}