mirrors/bisq
1
0
Fork 0
mirror of https://github.com/bisq-network/bisq.git synced 2024-11-19 09:52:23 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #3556 from julianknutsen/fix-remove-bug

Browse Source 
(2/8) [BUGFIX] Don't try and remove() if addMailboxData() fails
This commit is contained in:
Christoph Atteneder 2019-11-18 10:21:33 +01:00 committed by GitHub
commit 60f02e9923
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 12 additions and 42 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
p2p/src/main/java/bisq/network/p2p/P2PService.java
View File

@ -700,12 +700,12 @@ public class P2PService implements SetupListener, MessageListener, ConnectionLis
};
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.");
}
} catch (CryptoException e) {
log.error("Signing at getDataWithSignedSeqNr failed. That should never happen.");

4
p2p/src/main/java/bisq/network/p2p/storage/P2PDataStorage.java
View File

@ -93,6 +93,8 @@ import lombok.extern.slf4j.Slf4j;
import javax.annotation.Nullable;
import static com.google.common.base.Preconditions.checkArgument;
@Slf4j
public class P2PDataStorage implements MessageListener, ConnectionListener, PersistedDataHost {
/**
@ -475,6 +477,8 @@ public class P2PDataStorage implements MessageListener, ConnectionListener, Pers
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);

40
p2p/src/test/java/bisq/network/p2p/storage/P2PDataStorageTest.java
View File

@ -1162,12 +1162,7 @@ public class P2PDataStorageTest {
}
// 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);
@ -1175,38 +1170,9 @@ public class P2PDataStorageTest {
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 #)