<p><b>@julianknutsen</b> commented on this pull request.</p>

<hr>

<p>In <a href="https://github.com/bisq-network/bisq/pull/3556#discussion_r342737543">p2p/src/main/java/bisq/network/p2p/P2PService.java</a>:</p>
<pre style='color:#555'>> @@ -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);
                     }
</pre>
<p>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.</p>
<p>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.</p>
<p>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.</p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/bisq-network/bisq/pull/3556?email_source=notifications&email_token=AJFFTNV6FZ2IUEJHSYKZLZTQSG6PDA5CNFSM4JIYAKSKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCKMDABI#discussion_r342737543">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AJFFTNV624FXFFCVMKFVIWLQSG6PDANCNFSM4JIYAKSA">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AJFFTNXVQ3REOTD4R5HSYDLQSG6PDA5CNFSM4JIYAKSKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCKMDABI.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/bisq-network/bisq/pull/3556?email_source=notifications\u0026email_token=AJFFTNV6FZ2IUEJHSYKZLZTQSG6PDA5CNFSM4JIYAKSKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCKMDABI#discussion_r342737543",
"url": "https://github.com/bisq-network/bisq/pull/3556?email_source=notifications\u0026email_token=AJFFTNV6FZ2IUEJHSYKZLZTQSG6PDA5CNFSM4JIYAKSKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCKMDABI#discussion_r342737543",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>