[bisq-network/bisq] (2/4) [BUGFIX] Call correct remove function in addMailboxData failure path. (#3556)

[Julian Knutsen]

julianknutsen commented on this pull request.



> @@ -704,8 +704,14 @@ public void onBroadcastFailed(String errorMessage) {
                         // 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);
+
+                        // Generate a new entry so sequence number is larger than previous add()
+                        ProtectedMailboxStorageEntry removeProtectedMailboxStorageEntry = p2PDataStorage.getMailboxDataWithSignedSeqNr(
+                                expirableMailboxStoragePayload,
+                                keyRing.getSignatureKeyPair(),
+                                receiversPublicKey);
+                        boolean removeResult = p2PDataStorage.removeMailboxData(removeProtectedMailboxStorageEntry, networkNode.getNodeAddress(), true);
+                        log.error("Unexpected state: adding mailbox message that already exists. removeMailboxData result=" + removeResult);
                     }

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.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/bisq-network/bisq/pull/3556#discussion_r342737543
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191105/44e97af1/attachment.html>