mirrors/bisq
1
0
Fork 0
mirror of https://github.com/bisq-network/bisq.git synced 2025-02-24 15:10:44 +01:00
Code Issues Projects Releases Packages Wiki Activity

Update behavior of P2PDataStorage::remove() & removeMailboxData() on duplicate sequence #s

Browse source 
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.
This commit is contained in:
Julian Knutsen 2019-11-03 19:22:27 -08:00
parent cbda653aba
commit e5f9261d97
No known key found for this signature in database
GPG key ID: D85F536DB3615B2D
2 changed files with 17 additions and 40 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
View file

@ -497,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
@ -585,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();
@ -710,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;

35
p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java
View file

@ -771,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
@ -818,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);
@ -827,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 #)
@ -865,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);
}
@ -880,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);
}
@ -1012,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
@ -1128,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);
}
@ -1141,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);
}