Fix loss of mailbox messages during SPV resync

[ghost]

Fixes #6367

When SPV resync is performed from the UI, the app is started but the wallet does a resync and then needs another restart. Unfortunately per #6367, during SPV resync mailbox messages can still be received but are tossed by the recipients due to the app not being ready. The MailboxMessageService was unaware of this and deleted the unprocessed messages from the network. The fix is to pass a widely used onAllServicesInitialized flag so that the MailboxMessageService can make the decision to not remove messages before the app is ready to process them.

During testing, observed an NPE when the NetworkSettingsView is the first screen shown (happens after clicking the Delete SPV file and resync button and restarting). Included a fix for that NPE here.

---

Test case:

• Start a trade.
• Alice or Bob opens mediation.
• Alice or Bob (or both) requests an SPV resync from NetworkSettingsView and shuts down.
• Mediator proposes payout.
• Alice or Bob (or both) start Bisq to resync.
• Spv resync completes and Bisq shuts down again.
• Alice or Bob (or both) start Bisq and sees that a mediation proposal has been issued.
• Alice and Bob can both accept the mediation proposal and that finalizes the trade.

[HenrikJannsen]

I think its better and more safe to protect the init method as otherwise the mailbox could be applied with unintended consequences.

Here is a patch of my suggestion (does not cover the UI change):

Index: p2p/src/main/java/bisq/network/p2p/mailbox/MailboxMessageService.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/p2p/src/main/java/bisq/network/p2p/mailbox/MailboxMessageService.java b/p2p/src/main/java/bisq/network/p2p/mailbox/MailboxMessageService.java
--- a/p2p/src/main/java/bisq/network/p2p/mailbox/MailboxMessageService.java	(revision 84a1cd626d246726303cbc4fe595fec3473015c2)
+++ b/p2p/src/main/java/bisq/network/p2p/mailbox/MailboxMessageService.java	(date 1669326934458)
@@ -128,6 +128,8 @@
     private final Map<String, MailboxItem> mailboxItemsByUid = new HashMap<>();
 
     private boolean isBootstrapped;
+    private boolean allServicesInitialized;
+    private boolean initAfterBootstrapped;
 
     @Inject
     public MailboxMessageService(NetworkNode networkNode,
@@ -215,6 +217,12 @@
     // API
     ///////////////////////////////////////////////////////////////////////////////////////////
 
+    // We wait until all services are ready to avoid some edge cases as in https://github.com/bisq-network/bisq/issues/6367
+    public void onAllServicesInitialized() {
+        allServicesInitialized = true;
+        init();
+    }
+
     // We don't listen on requestDataManager directly as we require the correct
     // order of execution. The p2pService is handling the correct order of execution and we get called
     // directly from there.
@@ -226,11 +234,18 @@
 
     // second stage starup for MailboxMessageService ... apply existing messages to their modules
     public void initAfterBootstrapped() {
-        // Only now we start listening and processing. The p2PDataStorage is our cache for data we have received
-        // after the hidden service was ready.
-        addHashMapChangedListener();
-        onAdded(p2PDataStorage.getMap().values());
-        maybeRepublishMailBoxMessages();
+        initAfterBootstrapped = true;
+        init();
+    }
+
+    private void init() {
+        if (allServicesInitialized && initAfterBootstrapped) {
+            // Only now we start listening and processing. The p2PDataStorage is our cache for data we have received
+            // after the hidden service was ready.
+            addHashMapChangedListener();
+            onAdded(p2PDataStorage.getMap().values());
+            maybeRepublishMailBoxMessages();
+        }
     }
 
 
Index: core/src/main/java/bisq/core/app/DomainInitialisation.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/core/src/main/java/bisq/core/app/DomainInitialisation.java b/core/src/main/java/bisq/core/app/DomainInitialisation.java
--- a/core/src/main/java/bisq/core/app/DomainInitialisation.java	(revision 84a1cd626d246726303cbc4fe595fec3473015c2)
+++ b/core/src/main/java/bisq/core/app/DomainInitialisation.java	(date 1669326857062)
@@ -58,6 +58,7 @@
 import bisq.core.user.User;
 
 import bisq.network.p2p.P2PService;
+import bisq.network.p2p.mailbox.MailboxMessageService;
 
 import bisq.common.ClockWatcher;
 import bisq.common.persistence.PersistenceManager;
@@ -115,6 +116,7 @@
     private final TriggerPriceService triggerPriceService;
     private final MempoolService mempoolService;
     private final OpenBsqSwapOfferService openBsqSwapOfferService;
+    private final MailboxMessageService mailboxMessageService;
 
     @Inject
     public DomainInitialisation(ClockWatcher clockWatcher,
@@ -154,7 +156,8 @@
                                 DaoStateSnapshotService daoStateSnapshotService,
                                 TriggerPriceService triggerPriceService,
                                 MempoolService mempoolService,
-                                OpenBsqSwapOfferService openBsqSwapOfferService) {
+                                OpenBsqSwapOfferService openBsqSwapOfferService,
+                                MailboxMessageService mailboxMessageService) {
         this.clockWatcher = clockWatcher;
         this.tradeLimits = tradeLimits;
         this.arbitrationManager = arbitrationManager;
@@ -193,6 +196,7 @@
         this.triggerPriceService = triggerPriceService;
         this.mempoolService = mempoolService;
         this.openBsqSwapOfferService = openBsqSwapOfferService;
+        this.mailboxMessageService = mailboxMessageService;
     }
 
     public void initDomainServices(Consumer<String> rejectedTxErrorMessageHandler,
@@ -277,6 +281,8 @@
         triggerPriceService.onAllServicesInitialized();
         mempoolService.onAllServicesInitialized();
 
+        mailboxMessageService.onAllServicesInitialized();
+
         if (revolutAccountsUpdateHandler != null) {
             revolutAccountsUpdateHandler.accept(user.getPaymentAccountsAsObservable().stream()
                     .filter(paymentAccount -> paymentAccount instanceof RevolutAccount)

[ghost]

Applied the code changes from @HenrikJannsen and re-tested.

[HenrikJannsen]

utACK

[alejandrogarcia83]

utACK

[pazza83]

Hi @jmacxx

I had a user that appears to have lost a mediation ticket during an SPV resync.

• BTC buyer was doing an SPV resync (was taking a while)
• BTC seller opened mediation
• BTC buyer completed SPV resync but there was no mediation ticket in their Bisq instance
• Mediator unable to communicate with BTC buyer though ticket (receiver could not process message).

Does this feature enable mediations and arbitrations opened during an SPV resync not to get lost?

I have the logs if helpful.

[ghost]

• First conclusion upon checking logs was dropped message but now realized provided client logs began after the mediation started, so requesting earlier client logs to see if they did receive a PeerOpenedDisputeMessage, why it was not ack'd.

• Earlier client logs showed that an SPV resync was well underway at the time mediator sent PeerOpenedDisputeMessage, and there is no indication of ever receiving that message. But at SPV resync completion, there is a log message showing that a connection from the mediator instance had been established, so I'm wondering if the direct message is silently tossed during that time.

[ghost]

I did a few tests with the following results:

Alice and Bob have a trade in process.
Bob does a SPV resync, at the same time Alice opens Mediation.
The mediator sends PeerOpenedDisputeMessage to Bob. This can go one of two ways:

• (a) the message is stored as a MailboxMessage until Bob comes back online. Bob sees the mediation ticket after the SPV resync completes (what this PR achieved).
• (b) the message is sent, and Bob never receives the mediation ticket message. It does not appear in his logs at all.

Presumably the messaging layer decides to store the message in a mailbox if it cannot make a connection. I'm not sure how scenario (b) can be solved.

[pazza83]

Presumably the messaging layer decides to store the message in a mailbox if it cannot make a connection. I'm not sure how scenario (b) can be solved.

Mediator will always try to send a message again if a trader does not reply.

Could something be done on the subsequent attempts? Maybe checking to see if a mediation ticket exists on the traders instance and opening one if that is not the case.