Skip to content

Commit

Permalink
Update behavior of P2PDataStorage::remove() & removeMailboxData() on …
Browse files Browse the repository at this point in the history 
…duplicate sequence #s

Remove operations are now only processed if the sequence number
is greater than the last operation seen for a specific payload.

The only creator of new remove entrys is the P2PService layer that always increments
the sequence number. So, this is either left around from a time where
removes needed to work with non-incrementing sequence numbers or just
a longstanding bug.

With the completion of this patch, all operations now require increasing
sequence numbers so it should be easier to reason about the behavior in
future debugging.
  • Loading branch information
@julianknutsen
julianknutsen committed Nov 11, 2019
1 parent cbda653 commit e5f9261
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 40 deletions.
22 changes: 2 additions & 20 deletions p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry,
}

// If we have seen a more recent operation for this payload, ignore this one
if (!isSequenceNrValid(protectedStorageEntry.getSequenceNumber(), hashOfPayload))
if (!hasSequenceNrIncreased(protectedStorageEntry.getSequenceNumber(), hashOfPayload))
return false;

// Verify the ProtectedStorageEntry is well formed and valid for the remove operation
Expand Down Expand Up @@ -585,7 +585,7 @@ public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxSt

int sequenceNumber = protectedMailboxStorageEntry.getSequenceNumber();

if (!isSequenceNrValid(sequenceNumber, hashOfPayload))
if (!hasSequenceNrIncreased(sequenceNumber, hashOfPayload))
return false;

PublicKey receiversPubKey = protectedMailboxStorageEntry.getReceiversPubKey();
Expand Down Expand Up @@ -710,24 +710,6 @@ private void doRemoveProtectedExpirableData(ProtectedStorageEntry protectedStora
hashMapChangedListeners.forEach(e -> e.onRemoved(protectedStorageEntry));
}

private boolean isSequenceNrValid(int newSequenceNumber, ByteArray hashOfData) {
if (sequenceNumberMap.containsKey(hashOfData)) {
int storedSequenceNumber = sequenceNumberMap.get(hashOfData).sequenceNr;
if (newSequenceNumber >= storedSequenceNumber) {
log.trace("Sequence number is valid (>=). sequenceNumber = "
+ newSequenceNumber + " / storedSequenceNumber=" + storedSequenceNumber);
return true;
} else {
log.debug("Sequence number is invalid. sequenceNumber = "
+ newSequenceNumber + " / storedSequenceNumber=" + storedSequenceNumber + "\n" +
"That can happen if the data owner gets an old delayed data storage message.");
return false;
}
} else {
return true;
}
}

private boolean hasSequenceNrIncreased(int newSequenceNumber, ByteArray hashOfData) {
if (sequenceNumberMap.containsKey(hashOfData)) {
int storedSequenceNumber = sequenceNumberMap.get(hashOfData).sequenceNr;
Expand Down
35 changes: 15 additions & 20 deletions p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -771,21 +771,17 @@ public void addProtectedStorageEntry_greaterSeqNr() throws CryptoException {
}

// TESTCASE: Add w/ same sequence number after remove of sequence number
// XXXBUGXXX: Since removes aren't required to increase the sequence number, duplicate adds
// can occur that will cause listeners to be signaled. Any well-intentioned nodes will create remove messages
// that increment the seq #, but this may just fall into a larger effort to protect against malicious nodes.
/* @Test
// Regression test for old remove() behavior that succeeded if add.seq# == remove.seq#
@Test
public void addProtectectedStorageEntry_afterRemoveSameSeqNr() throws CryptoException {
ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);
ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1);

doProtectedStorageAddAndVerify(entryForAdd, true, true);
doProtectedStorageRemoveAndVerify(entryForRemove, true, true);
doProtectedStorageRemoveAndVerify(entryForRemove, false, false);

// Should be false, false. Instead, the hashmap is updated and hashmap listeners are signaled.
// Broadcast isn't called
doProtectedStorageAddAndVerify(entryForAdd, false, false);
}*/
}

// TESTCASE: Entry signature does not match entry owner
@Test
Expand Down Expand Up @@ -818,17 +814,14 @@ 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);
ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1);

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 #)
Expand Down Expand Up @@ -865,7 +858,7 @@ public void remove_invalidEntrySig() throws CryptoException {
ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);
doProtectedStorageAddAndVerify(entryForAdd, true, true);

ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1);
ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(2);
entryForRemove.updateSignature(new byte[] { 0 });
doProtectedStorageRemoveAndVerify(entryForRemove, false, false);
}
Expand All @@ -880,7 +873,7 @@ public void remove_payloadOwnerEntryOwnerNotCompatible() throws NoSuchAlgorithmE

// For standard ProtectedStorageEntrys the entry owner must match the payload owner for removes
ProtectedStorageEntry entryForRemove = buildProtectedStorageEntry(
this.protectedStoragePayload, notOwner, notOwner, 1);
this.protectedStoragePayload, notOwner, notOwner, 2);

doProtectedStorageRemoveAndVerify(entryForRemove, false, false);
}
Expand Down Expand Up @@ -1012,11 +1005,13 @@ public void refreshTTL_duplicateRefreshSeqNrLower() throws CryptoException {
// TESTCASE: Refresh previously removed entry
@Test
public void refreshTTL_refreshAfterRemove() throws CryptoException {
ProtectedStorageEntry entry = this.getProtectedStorageEntryForAdd(1);
doProtectedStorageAddAndVerify(entry, true, true);
doProtectedStorageRemoveAndVerify(entry, true, true);
ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);
ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForAdd(2);

doProtectedStorageAddAndVerify(entryForAdd, true, true);
doProtectedStorageRemoveAndVerify(entryForRemove, true, true);

doRefreshTTLAndVerify(buildRefreshOfferMessage(entry, this.payloadOwnerKeys,3), false, false);
doRefreshTTLAndVerify(buildRefreshOfferMessage(entryForAdd, this.payloadOwnerKeys,3), false, false);
}

// TESTCASE: Refresh an entry, but owner doesn't match PubKey of original add owner
Expand Down Expand Up @@ -1128,7 +1123,7 @@ public void remove_payloadOwnerEntryOwnerNotCompatible() throws NoSuchAlgorithmE

KeyPair notReceiver = TestUtils.generateKeyPair();

ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 1);
ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), notReceiver.getPrivate(), notReceiver.getPublic(), receiverKeys.getPublic(), 2);

doProtectedStorageRemoveAndVerify(entryForRemove, false, false);
}
Expand All @@ -1141,7 +1136,7 @@ public void remove_payloadOwnerEntryReceiversPubKeyNotCompatible() throws NoSuch

KeyPair notSender = TestUtils.generateKeyPair();

ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 1);
ProtectedStorageEntry entryForRemove = buildProtectedMailboxStorageEntry(senderKeys.getPublic(), receiverKeys.getPublic(), receiverKeys.getPrivate(), receiverKeys.getPublic(), notSender.getPublic(), 2);

doProtectedStorageRemoveAndVerify(entryForRemove, false, false);
}
Expand Down

0 comments on commit e5f9261

Please sign in to comment.