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

[chimp1984]

chimp1984 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);
                     }

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.  

-- 
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#pullrequestreview-311463307
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.bisq.network/pipermail/bisq-github/attachments/20191104/341734d1/attachment-0001.html>