Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(2/8) [BUGFIX] Don't try and remove() if addMailboxData() fails #3556

Merged
merged 1 commit into from
Nov 18, 2019
Merged

(2/8) [BUGFIX] Don't try and remove() if addMailboxData() fails #3556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions p2p/src/main/java/bisq/network/p2p/P2PService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -700,12 +700,12 @@ public void onBroadcastFailed(String errorMessage) {
};
boolean result = p2PDataStorage.addProtectedStorageEntry(protectedMailboxStorageEntry, networkNode.getNodeAddress(), listener, true);
if (!result) {
//TODO remove and add again with a delay to ensure the data will be broadcasted
// The p2PDataStorage.remove makes probably sense but need to be analysed more.
// Don't change that if it is not 100% clear.
sendMailboxMessageListener.onFault("Data already exists in our local database");
boolean removeResult = p2PDataStorage.remove(protectedMailboxStorageEntry, networkNode.getNodeAddress(), true);
log.debug("remove result=" + removeResult);

// This should only fail if there are concurrent calls to addProtectedStorageEntry with the
// same ProtectedMailboxStorageEntry. This is an unexpected use case so if it happens we
// want to see it, but it is not worth throwing an exception.
log.error("Unexpected state: adding mailbox message that already exists.");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the result from addProtectedStorageEntry is false we have not added data to the map or the sequence nr map. From my understanding I think the remove is pointless as there is nothing to remove anyway in case of a failed add. But maybe I am missing something. Do you see any case where this would make sense? Beside that your code looks correct to me. So if you agree that the whole result=false path is pointless, I would suggest to remove it instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After more testing, it is even simpler than that. Even if the add() did update some state and still return false (which isn't the case), only the receiver of a mailbox entry can remove it so the sender shouldn't try.

This brings up one of my TODO list items for this module which is the fact that concurrent calls through the client API (getProtectedStorageEntry) can result in interleaved sequence numbers that cause unexpected failures. Moving to a contract where client API calls never fail due to stale sequence numbers will make the system easier to reason about. This could be done by synchronizing the client API paths to not hand out duplicate seq numbers.

I've added multiple tests in the normal add() and mailbox add() paths to document the current behavior to ensure my future patches don't change it.

} catch (CryptoException e) {
log.error("Signing at getDataWithSignedSeqNr failed. That should never happen.");
Expand Down
4 changes: 4 additions & 0 deletions p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@

import javax.annotation.Nullable;

import static com.google.common.base.Preconditions.checkArgument;

@Slf4j
public class P2PDataStorage implements MessageListener, ConnectionListener, PersistedDataHost {
/**
Expand Down Expand Up @@ -475,6 +477,8 @@ 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);
boolean containsKey = map.containsKey(hashOfPayload);
Expand Down
40 changes: 3 additions & 37 deletions p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1162,51 +1162,17 @@ public void remove_receiversPubKeyChanged() throws NoSuchAlgorithmException, Cry
}


// XXXBUGXXX: The P2PService calls remove() instead of removeFromMailbox() in the addMailboxData() path.
// This test shows it will always fail even with a valid remove entry. Future work should be able to
// combine the remove paths in the same way the add() paths are combined. This will require deprecating
// the receiversPubKey field which is a duplicate of the ownerPubKey in the MailboxStoragePayload.
// More investigation is needed.
@Test
@Test(expected = IllegalArgumentException.class)
public void remove_canCallWrongRemoveAndFail() throws CryptoException {

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

doProtectedStorageAddAndVerify(entryForAdd, true, true);

SavedTestState beforeState = new SavedTestState(this.testState, entryForRemove);

// Call remove(ProtectedStorageEntry) instead of removeFromMailbox(ProtectedMailboxStorageEntry) and verify
// it fails
boolean addResult = super.doRemove(entryForRemove);

if (!this.useMessageHandler)
Assert.assertFalse(addResult);

// should succeed with expectedStatechange==true when remove paths are combined
verifyProtectedStorageRemove(this.testState, beforeState, entryForRemove, false, this.expectIsDataOwner());
}

// TESTCASE: Verify misuse of the API (calling remove() instead of removeFromMailbox correctly errors with
// a payload that is valid for remove of a non-mailbox entry.
@Test
public void remove_canCallWrongRemoveAndFailInvalidPayload() throws CryptoException {

ProtectedStorageEntry entryForAdd = this.getProtectedStorageEntryForAdd(1);

doProtectedStorageAddAndVerify(entryForAdd, true, true);

SavedTestState beforeState = new SavedTestState(this.testState, entryForAdd);

// Call remove(ProtectedStorageEntry) instead of removeFromMailbox(ProtectedMailboxStorageEntry) and verify
// it fails with a payload that isn't signed by payload.ownerPubKey
boolean addResult = super.doRemove(entryForAdd);

if (!this.useMessageHandler)
Assert.assertFalse(addResult);

verifyProtectedStorageRemove(this.testState, beforeState, entryForAdd, false, this.expectIsDataOwner());
// it fails spectacularly
super.doRemove(entryForRemove);
}

// TESTCASE: Add after removed when add-once required (greater seq #)
Expand Down