Skip to content

Commit

Permalink
Update behavior of P2PDataStorage::refreshTTL() on duplicates
Browse files Browse the repository at this point in the history
Now returns false if the sequence number of the refresh matches
the last operation seen for the specified hash. This is a more expected
return value when no state change occurs.

The only callers are either P2PService users that always increment the
sequence number or the onMessage() handler which doesn't verify the return
so there will be no visible change other than the increased readability
of the code and deduplication of the code paths.
  • Loading branch information
julianknutsen committed Nov 11, 2019
1 parent c1ad6b4 commit cbda653
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 13 deletions.
11 changes: 0 additions & 11 deletions p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
Original file line number Diff line number Diff line change
Expand Up @@ -447,17 +447,6 @@ public boolean refreshTTL(RefreshOfferMessage refreshTTLMessage,

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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -976,7 +976,7 @@ public void refreshTTL_existingEntry() throws CryptoException {
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 #)
Expand All @@ -986,7 +986,7 @@ public void refreshTTL_duplicateRefreshSeqNrEqual() throws CryptoException {
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 #)
Expand Down

0 comments on commit cbda653

Please sign in to comment.