From cf87a06efb576415ab953e01d93aa574f0d2e2cf Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Mon, 7 Sep 2026 13:26:06 +0200 Subject: [PATCH 1/3] Qt: defer Spark updates while validation is busy Use try-locks in the queued incoming-fund scan, automatic mint check and Spark address-book refresh so they do not block the GUI on cs_main. Retry the startup scan and retain address choices while refresh is busy. Keep Spark proof verification serialized and preserve historical batching. Add Qt regressions for lock contention and the deferred scan retry. Alternative to #1940 for the GUI responsiveness issue. --- src/qt/addressbookpage.cpp | 10 ++- src/qt/automintmodel.cpp | 14 +++- src/qt/test/CMakeLists.txt | 1 + src/qt/test/sparkmodeltests.cpp | 118 ++++++++++++++++++++++++++++++++ src/qt/test/sparkmodeltests.h | 17 +++++ src/qt/test/test_main.cpp | 5 ++ 6 files changed, 162 insertions(+), 3 deletions(-) create mode 100644 src/qt/test/sparkmodeltests.cpp create mode 100644 src/qt/test/sparkmodeltests.h diff --git a/src/qt/addressbookpage.cpp b/src/qt/addressbookpage.cpp index ee4d891a57..4a3385861a 100644 --- a/src/qt/addressbookpage.cpp +++ b/src/qt/addressbookpage.cpp @@ -16,6 +16,7 @@ #include "createsparknamepage.h" #include "guiutil.h" #include "platformstyle.h" +#include "validation.h" #include "bip47/paymentcode.h" #include "bip47/paymentchannel.h" @@ -219,7 +220,14 @@ void AddressBookPage::setModel(AddressTableModel *_model) bool AddressBookPage::updateSpark() { - const bool sparkAllowed = model && model->IsSparkAllowed(); + bool sparkAllowed; + { + // Leave the current choices intact; the next block-tip update retries. + TRY_LOCK(cs_main, lockMain); + if (!lockMain) + return false; + sparkAllowed = model && model->IsSparkAllowed(); + } populateAddressTypes(sparkAllowed); chooseAddressType(0); diff --git a/src/qt/automintmodel.cpp b/src/qt/automintmodel.cpp index 670d4f86ba..380faa61f4 100644 --- a/src/qt/automintmodel.cpp +++ b/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; diff --git a/src/qt/test/CMakeLists.txt b/src/qt/test/CMakeLists.txt index 486dc4de33..96aa322409 100644 --- a/src/qt/test/CMakeLists.txt +++ b/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 ) diff --git a/src/qt/test/sparkmodeltests.cpp b/src/qt/test/sparkmodeltests.cpp new file mode 100644 index 0000000000..f763dd919b --- /dev/null +++ b/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 +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +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& callback) +{ + std::promise 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("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(); + 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(¬ifier, 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 style(PlatformStyle::instantiate("other")); + QVERIFY(style); + AddressBookPage page(style.get(), AddressBookPage::ForEditing, AddressBookPage::SendingTab); + page.setModel(&model); + QComboBox* types = page.findChild("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); + + page.updateSpark(); + QCOMPARE(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()); +} diff --git a/src/qt/test/sparkmodeltests.h b/src/qt/test/sparkmodeltests.h new file mode 100644 index 0000000000..39965a2835 --- /dev/null +++ b/src/qt/test/sparkmodeltests.h @@ -0,0 +1,17 @@ +#ifndef FIRO_QT_TEST_SPARKMODELTESTS_H +#define FIRO_QT_TEST_SPARKMODELTESTS_H + +#include + +class SparkModelTests : public QObject +{ + Q_OBJECT + +private Q_SLOTS: + void importRetries_data(); + void importRetries(); + void addressBookDefers(); + void autoMintDefers(); +}; + +#endif // FIRO_QT_TEST_SPARKMODELTESTS_H diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp index 5d0d720375..6827edfbe0 100644 --- a/src/qt/test/test_main.cpp +++ b/src/qt/test/test_main.cpp @@ -13,6 +13,7 @@ #include "uritests.h" #include "compattests.h" #include "test_sendcoinsentry.h" +#include "sparkmodeltests.h" #include #include #include @@ -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; } From e53d266f0c5aaf8d1a6167bad2b69409cb2f1e62 Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Tue, 8 Sep 2026 04:42:23 +0200 Subject: [PATCH 2/3] Tests: pass the address-book parent explicitly AddressBookPage requires a QWidget parent argument. Supply nullptr in the Spark GUI contention test so the Linux Qt test target compiles. --- src/qt/test/sparkmodeltests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qt/test/sparkmodeltests.cpp b/src/qt/test/sparkmodeltests.cpp index f763dd919b..b92d43f511 100644 --- a/src/qt/test/sparkmodeltests.cpp +++ b/src/qt/test/sparkmodeltests.cpp @@ -79,7 +79,7 @@ void SparkModelTests::addressBookDefers() AddressTableModel model(&wallet); const std::unique_ptr style(PlatformStyle::instantiate("other")); QVERIFY(style); - AddressBookPage page(style.get(), AddressBookPage::ForEditing, AddressBookPage::SendingTab); + AddressBookPage page(style.get(), AddressBookPage::ForEditing, AddressBookPage::SendingTab, nullptr); page.setModel(&model); QComboBox* types = page.findChild("addressType"); QVERIFY(types); From 82c9f8f5c628d7b9f16a54eee15ce698b3ff91cf Mon Sep 17 00:00:00 2001 From: Reuben Yap Date: Tue, 8 Sep 2026 04:50:04 +0200 Subject: [PATCH 3/3] Qt: retry deferred address-book pages A shared refresh can finish when another page succeeds. Schedule the busy page's retry directly so its address choices still update, and verify the retry without another block notification. --- src/qt/addressbookpage.cpp | 8 ++++++-- src/qt/test/sparkmodeltests.cpp | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/qt/addressbookpage.cpp b/src/qt/addressbookpage.cpp index 4a3385861a..ad4e9704ef 100644 --- a/src/qt/addressbookpage.cpp +++ b/src/qt/addressbookpage.cpp @@ -14,6 +14,7 @@ #include "csvmodelwriter.h" #include "editaddressdialog.h" #include "createsparknamepage.h" +#include "guiconstants.h" #include "guiutil.h" #include "platformstyle.h" #include "validation.h" @@ -24,6 +25,7 @@ #include #include #include +#include AddressBookPage::AddressBookPage(const PlatformStyle *_platformStyle, Mode _mode, Tabs _tab, QWidget *parent, bool isReused) : QDialog(parent), @@ -222,10 +224,12 @@ bool AddressBookPage::updateSpark() { bool sparkAllowed; { - // Leave the current choices intact; the next block-tip update retries. TRY_LOCK(cs_main, lockMain); - if (!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); diff --git a/src/qt/test/sparkmodeltests.cpp b/src/qt/test/sparkmodeltests.cpp index b92d43f511..32e87668d5 100644 --- a/src/qt/test/sparkmodeltests.cpp +++ b/src/qt/test/sparkmodeltests.cpp @@ -94,8 +94,8 @@ void SparkModelTests::addressBookDefers() QCOMPARE(types->count(), originalCount + 1); QCOMPARE(types->currentIndex(), originalCount); - page.updateSpark(); - QCOMPARE(types->count(), originalCount); + // Retry independently of another page or block-tip notification. + QTRY_COMPARE(types->count(), originalCount); } void SparkModelTests::autoMintDefers()