Qt: defer Spark GUI callbacks during validation

src/qt/addressbookpage.cpp

@@ -14,15 +14,18 @@
 #include "csvmodelwriter.h"
 #include "editaddressdialog.h"
 #include "createsparknamepage.h"
+#include "guiconstants.h"
 #include "guiutil.h"
 #include "platformstyle.h"
+#include "validation.h"
 #include "bip47/paymentcode.h"
 #include "bip47/paymentchannel.h"
 
 #include <QIcon>
 #include <QMenu>
 #include <QMessageBox>
 #include <QSortFilterProxyModel>
+#include <QTimer>
 
 AddressBookPage::AddressBookPage(const PlatformStyle *_platformStyle, Mode _mode, Tabs _tab, QWidget *parent, bool isReused) :
     QDialog(parent),
@@ -219,7 +222,16 @@ void AddressBookPage::setModel(AddressTableModel *_model)
 
 bool AddressBookPage::updateSpark()
 {
-    const bool sparkAllowed = model && model->IsSparkAllowed();
+    bool sparkAllowed;
+    {
+        TRY_LOCK(cs_main, lockMain);
+        if (!lockMain) {
+            // Retry this page even if another page completes the shared refresh.
+            QTimer::singleShot(MODEL_UPDATE_DELAY, this, &AddressBookPage::updateSpark);
+            return false;
+        }
+        sparkAllowed = model && model->IsSparkAllowed();
+    }
     populateAddressTypes(sparkAllowed);
 
     chooseAddressType(0);

src/qt/automintmodel.cpp

@@ -111,8 +111,14 @@ void IncomingFundNotifier::check()
 
 void IncomingFundNotifier::importTransactions()
 {
-    LOCK2(cs_main, cs);
-    LOCK(wallet->cs_wallet);
+    TRY_LOCK(cs_main, lockMain);
+    TRY_LOCK(cs, lock);
+    TRY_LOCK(wallet->cs_wallet, lockWallet);
+    if (!lockMain || !lock || !lockWallet) {
+        // This queued startup scan must retry after long-running validation.
+        QTimer::singleShot(MODEL_UPDATE_DELAY, this, &IncomingFundNotifier::importTransactions);
+        return;
+    }
 
     for (auto const &tx : wallet->mapWallet) {
         if (tx.second.GetAvailableCredit() > 0 || tx.second.GetImmatureCredit() > 0) {
@@ -247,6 +253,10 @@ void AutoMintSparkModel::checkAutoMintSpark(bool force)
             return;
         }
 
+        // The periodic check can try again on its next timer tick.
+        TRY_LOCK(cs_main, lockMain);
+        if (!lockMain)
+            return;
         bool allowed = spark::IsSparkAllowed();
         if (!allowed) {
             return;

src/qt/test/CMakeLists.txt

@@ -14,6 +14,7 @@ add_executable(test_firo-qt
   ${CMAKE_CURRENT_SOURCE_DIR}/test_main.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/uritests.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_sendcoinsentry.cpp
+  ${CMAKE_CURRENT_SOURCE_DIR}/sparkmodeltests.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/../../test/test_bitcoin.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/../../test/testutil.cpp
 )

src/qt/test/sparkmodeltests.cpp

@@ -0,0 +1,118 @@
+#include "sparkmodeltests.h"
+
+#include "addressbookpage.h"
+#include "addresstablemodel.h"
+#include "automintmodel.h"
+#include "optionsmodel.h"
+#include "platformstyle.h"
+#include "sparkmodel.h"
+
+#include "masternode-sync.h"
+#include "validation.h"
+#include "wallet/wallet.h"
+
+#include <QComboBox>
+#include <QCoreApplication>
+#include <QEvent>
+#include <QTest>
+#include <QTimer>
+
+#include <chrono>
+#include <functional>
+#include <future>
+#include <memory>
+#include <thread>
+
+namespace {
+
+// Exercise the GUI callback while another thread owns the real core lock.
+// The timeout lets a blocking regression fail instead of hanging the suite.
+bool WithContendedLock(CCriticalSection& mutex, const std::function<void()>& callback)
+{
+    std::promise<void> locked, release;
+    auto released = release.get_future();
+    bool timedOut = false;
+    std::jthread worker([&] {
+        LOCK(mutex);
+        locked.set_value();
+        timedOut = released.wait_for(std::chrono::seconds(5)) != std::future_status::ready;
+    });
+    locked.get_future().wait();
+    callback();
+    release.set_value();
+    worker.join();
+    return !timedOut;
+}
+
+} // namespace
+
+void SparkModelTests::importRetries_data()
+{
+    QTest::addColumn<bool>("walletLock");
+    QTest::newRow("chain-lock") << false;
+    QTest::newRow("wallet-lock") << true;
+}
+
+void SparkModelTests::importRetries()
+{
+    QFETCH(bool, walletLock);
+    CWallet wallet;
+    IncomingFundNotifier notifier(&wallet);
+    QTimer* timer = notifier.findChild<QTimer*>();
+    QVERIFY(timer);
+
+    bool deferred = false;
+    const bool responsive = WithContendedLock(walletLock ? wallet.cs_wallet : cs_main, [&] {
+        // Deliver the constructor's queued startup scan on the GUI thread.
+        QCoreApplication::sendPostedEvents(&notifier, QEvent::MetaCall);
+        deferred = !timer->isActive();
+    });
+    QVERIFY(responsive);
+    QVERIFY(deferred);
+    // No new block or transaction notification is needed to retry the scan.
+    QTRY_VERIFY(timer->isActive());
+}
+
+void SparkModelTests::addressBookDefers()
+{
+    CWallet wallet;
+    AddressTableModel model(&wallet);
+    const std::unique_ptr<const PlatformStyle> style(PlatformStyle::instantiate("other"));
+    QVERIFY(style);
+    AddressBookPage page(style.get(), AddressBookPage::ForEditing, AddressBookPage::SendingTab, nullptr);
+    page.setModel(&model);
+    QComboBox* types = page.findChild<QComboBox*>("addressType");
+    QVERIFY(types);
+    const int originalCount = types->count();
+    types->addItem("Retain this selection while busy");
+    types->setCurrentIndex(originalCount);
+
+    bool updated = true;
+    const bool responsive = WithContendedLock(cs_main, [&] { updated = page.updateSpark(); });
+    QVERIFY(responsive);
+    QVERIFY(!updated);
+    QCOMPARE(types->count(), originalCount + 1);
+    QCOMPARE(types->currentIndex(), originalCount);
+
+    // Retry independently of another page or block-tip notification.
+    QTRY_COMPARE(types->count(), originalCount);
+}
+
+void SparkModelTests::autoMintDefers()
+{
+    CWallet wallet;
+    OptionsModel options;
+    SparkModel model(nullptr, &wallet, &options);
+    // Reach the activation check even though this test has no network peers.
+    const CMasternodeSync savedSync = masternodeSync;
+    CConnman connman(0, 0);
+    masternodeSync.Reset();
+    masternodeSync.SwitchToNextAsset(connman);
+    masternodeSync.SwitchToNextAsset(connman);
+    const bool responsive = WithContendedLock(cs_main, [&] {
+        model.getAutoMintSparkModel()->checkAutoMintSpark();
+    });
+    masternodeSync = savedSync;
+    QVERIFY(responsive);
+    QVERIFY(!model.getAutoMintSparkModel()->isSparkAnonymizing());
+}

src/qt/test/sparkmodeltests.h

@@ -0,0 +1,17 @@
+#ifndef FIRO_QT_TEST_SPARKMODELTESTS_H
+#define FIRO_QT_TEST_SPARKMODELTESTS_H
+
+#include <QObject>
+
+class SparkModelTests : public QObject
+{
+    Q_OBJECT
+
+private Q_SLOTS:
+    void importRetries_data();
+    void importRetries();
+    void addressBookDefers();
+    void autoMintDefers();
+};
+
+#endif // FIRO_QT_TEST_SPARKMODELTESTS_H

src/qt/test/test_main.cpp

@@ -13,6 +13,7 @@
 #include "uritests.h"
 #include "compattests.h"
 #include "test_sendcoinsentry.h"
+#include "sparkmodeltests.h"
 #include <QApplication>
 #include <QObject>
 #include <openssl/ssl.h>
@@ -59,6 +60,10 @@ int main(int argc, char *argv[])
     if (QTest::qExec(&test4) != 0)
         fInvalid = true;
 
+    SparkModelTests sparkModelTests;
+    if (QTest::qExec(&sparkModelTests) != 0)
+        fInvalid = true;
+
     ECC_Stop();
     return fInvalid;
 }