Skip to content

Commit

Permalink
Combine remove() and removeMailboxData()
Browse files Browse the repository at this point in the history 
Now that all the code is abstracted and tested, the remove()
and removeMailboxData() functions are identical. Combine them and update
callers appropriately.

Now, any caller don't need to know the difference and it removes the
sharp edge originally found in bisq-network#3556
  • Loading branch information
@julianknutsen
julianknutsen committed Nov 13, 2019
1 parent 53b5feb commit 5ae9dd1
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 65 deletions.
2 changes: 1 addition & 1 deletion p2p/src/main/java/bisq/network/p2p/P2PService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -759,7 +759,7 @@ private void delayedRemoveEntryFromMailbox(DecryptedMessageWithPubKey decryptedM
expirableMailboxStoragePayload,
keyRing.getSignatureKeyPair(),
receiversPubKey);
p2PDataStorage.removeMailboxData(protectedMailboxStorageEntry, networkNode.getNodeAddress(), true);
p2PDataStorage.remove(protectedMailboxStorageEntry, networkNode.getNodeAddress(), true);
} catch (CryptoException e) {
log.error("Signing at getDataWithSignedSeqNr failed. That should never happen.");
}
Expand Down
53 changes: 7 additions & 46 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 @@ -240,7 +240,7 @@ public void onMessage(NetworkEnvelope networkEnvelope, Connection connection) {
} else if (networkEnvelope instanceof RemoveDataMessage) {
remove(((RemoveDataMessage) networkEnvelope).getProtectedStorageEntry(), peersNodeAddress, false);
} else if (networkEnvelope instanceof RemoveMailboxDataMessage) {
removeMailboxData(((RemoveMailboxDataMessage) networkEnvelope).getProtectedMailboxStorageEntry(), peersNodeAddress, false);
remove(((RemoveMailboxDataMessage) networkEnvelope).getProtectedMailboxStorageEntry(), peersNodeAddress, false);
} else if (networkEnvelope instanceof RefreshOfferMessage) {
refreshTTL((RefreshOfferMessage) networkEnvelope, peersNodeAddress, false);
} else if (networkEnvelope instanceof AddPersistableNetworkPayloadMessage) {
Expand Down Expand Up @@ -485,8 +485,6 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage,
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);

Expand Down Expand Up @@ -519,7 +517,11 @@ public boolean remove(ProtectedStorageEntry protectedStorageEntry,

maybeAddToRemoveAddOncePayloads(protectedStoragePayload, hashOfPayload);

broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner);
if (protectedStorageEntry instanceof ProtectedMailboxStorageEntry) {
broadcast(new RemoveMailboxDataMessage((ProtectedMailboxStorageEntry) protectedStorageEntry), sender, null, isDataOwner);
} else {
broadcast(new RemoveDataMessage(protectedStorageEntry), sender, null, isDataOwner);
}

removeFromProtectedDataStore(protectedStorageEntry);

Expand Down Expand Up @@ -570,48 +572,7 @@ private void removeFromProtectedDataStore(ProtectedStorageEntry protectedStorage
}
}
}

@SuppressWarnings("UnusedReturnValue")
public boolean removeMailboxData(ProtectedMailboxStorageEntry protectedMailboxStorageEntry,
@Nullable NodeAddress sender,
boolean isDataOwner) {
ProtectedStoragePayload protectedStoragePayload = protectedMailboxStorageEntry.getProtectedStoragePayload();
ByteArray hashOfPayload = get32ByteHashAsByteArray(protectedStoragePayload);

ProtectedStorageEntry storedEntry = map.get(hashOfPayload);
if (storedEntry == null) {
log.debug("removeMailboxData failed due to unknown entry");

return false;
}

int sequenceNumber = protectedMailboxStorageEntry.getSequenceNumber();

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

if (!protectedMailboxStorageEntry.isValidForRemoveOperation())
return false;

// Verify the metadata between the stored entry and the new remove entry are the same
if (!protectedMailboxStorageEntry.isMetadataEquals(storedEntry))
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,
ByteArray hashOfData) {
if (protectedStoragePayload instanceof AddOncePayload) {
Expand Down
22 changes: 4 additions & 18 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 @@ -1130,7 +1130,7 @@ boolean doRemove(ProtectedStorageEntry entry) {
return true;
} else {
// XXX: All external callers just pass in true, a future patch can remove the argument.
return testState.mockedStorage.removeMailboxData((ProtectedMailboxStorageEntry) entry, getTestNodeAddress(), true);
return testState.mockedStorage.remove(entry, getTestNodeAddress(), true);
}
}

Expand Down Expand Up @@ -1280,20 +1280,6 @@ public void remove_receiversPubKeyChanged() throws NoSuchAlgorithmException, Cry
doProtectedStorageRemoveAndVerify(this.getProtectedStorageEntryForRemove(2), false, false);
}


@Test(expected = IllegalArgumentException.class)
public void remove_canCallWrongRemoveAndFail() throws CryptoException {

ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);
ProtectedStorageEntry entryForRemove = this.getProtectedStorageEntryForRemove(1);

doProtectedStorageAddAndVerify(entryForAdd, true, true);

// Call remove(ProtectedStorageEntry) instead of removeFromMailbox(ProtectedMailboxStorageEntry) and verify
// it fails spectacularly
super.doRemove(entryForRemove);
}

// TESTCASE: Add after removed when add-once required (greater seq #)
@Override
@Test
Expand Down Expand Up @@ -1445,7 +1431,7 @@ public void getMailboxDataWithSignedSeqNr_RemoveNoExist() throws NoSuchAlgorithm
this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic());

SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry);
Assert.assertFalse(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true));
Assert.assertFalse(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, getTestNodeAddress(), true));

verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, false, true, true, true);
}
Expand All @@ -1467,7 +1453,7 @@ public void getMailboxDataWithSignedSeqNr_AddThenRemove() throws NoSuchAlgorithm
this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic());

SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry);
Assert.assertTrue(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true));
Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, getTestNodeAddress(), true));

verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true, true,true);
}
Expand All @@ -1493,7 +1479,7 @@ public void getMailboxDataWithSignedSeqNr_ValidRemoveAddFromMessage() throws NoS
this.testState.mockedStorage.getMailboxDataWithSignedSeqNr(mailboxStoragePayload, receiverKeys, receiverKeys.getPublic());

SavedTestState beforeState = new SavedTestState(this.testState, protectedMailboxStorageEntry);
Assert.assertTrue(this.testState.mockedStorage.removeMailboxData(protectedMailboxStorageEntry, getTestNodeAddress(), true));
Assert.assertTrue(this.testState.mockedStorage.remove(protectedMailboxStorageEntry, getTestNodeAddress(), true));

verifyProtectedStorageRemove(this.testState, beforeState, protectedMailboxStorageEntry, true, true, true,true);
}
Expand Down

0 comments on commit 5ae9dd1

Please sign in to comment.