fix: ActivityListModel threading race and deprecated injected event params

qml/models/activitylistmodel.cpp

@@ -8,9 +8,19 @@
 #include <qml/models/walletqmlmodel.h>
 
 #include <QDateTime>
+#include <QMetaObject>
+#include <QThread>
 #include <QVariantList>
 
 #include <algorithm>
+#include <cassert>
+
+namespace {
+void assertModelThread(const QObject& model)
+{
+    assert(QThread::currentThread() == model.thread());
+}
+} // namespace
 
 ActivityListModel::ActivityListModel(WalletQmlModel *parent)
     : QAbstractListModel(parent)
@@ -35,7 +45,7 @@ int ActivityListModel::rowCount(const QModelIndex &parent) const
 
 void ActivityListModel::updateTransactionStatus(QSharedPointer<Transaction> tx) const
 {
-    if (m_wallet_model == nullptr || tx->isPendingRequest) {
+    if (m_wallet_model == nullptr || tx.isNull() || tx->isPendingRequest) {
         return;
     }
     interfaces::WalletTxStatus wtx;
@@ -50,7 +60,7 @@ void ActivityListModel::updateTransactionStatus(QSharedPointer<Transaction> tx)
 
 void ActivityListModel::updateTransactionLabel(QSharedPointer<Transaction> tx) const
 {
-    if (m_wallet_model == nullptr) {
+    if (m_wallet_model == nullptr || tx.isNull()) {
         return;
     }
 
@@ -63,6 +73,9 @@ QVariant ActivityListModel::data(const QModelIndex &index, int role) const
         return QVariant();
 
     QSharedPointer<Transaction> tx = m_transactions.at(index.row());
+    if (tx.isNull())
+        return QVariant();
+
     updateTransactionStatus(tx);
 
     switch (role) {
@@ -128,6 +141,7 @@ QHash<int, QByteArray> ActivityListModel::roleNames() const
 
 void ActivityListModel::reload()
 {
+    assertModelThread(*this);
     beginResetModel();
     m_transactions.clear();
     refreshWallet();
@@ -192,6 +206,7 @@ QVariantMap ActivityListModel::transactionDetails(const QSharedPointer<Transacti
 
 void ActivityListModel::setDisplayUnit(int unit)
 {
+    assertModelThread(*this);
     if (unit != m_display_unit) {
         m_display_unit = unit;
         if (!m_transactions.isEmpty()) {
@@ -252,6 +267,7 @@ void ActivityListModel::addPendingReceiveRequests()
 void ActivityListModel::addReceiveRequest(const QString& address, const QString& label,
                                           CAmount amount, qint64 timestamp, const QString& requestId)
 {
+    assertModelThread(*this);
     uint256 zero_hash;
     auto tx = QSharedPointer<Transaction>::create(zero_hash, timestamp,
         Transaction::RecvWithAddress, address, CAmount{0}, amount);
@@ -269,6 +285,7 @@ void ActivityListModel::addReceiveRequest(const QString& address, const QString&
 
 void ActivityListModel::updateReceiveRequest(const QString& requestId, const QString& label, CAmount amount)
 {
+    assertModelThread(*this);
     for (int i = 0; i < m_transactions.size(); ++i) {
         if (m_transactions[i]->isPendingRequest && m_transactions[i]->requestId == requestId) {
             m_transactions[i]->label = label.isEmpty() ? tr("Payment request") : label;
@@ -281,6 +298,7 @@ void ActivityListModel::updateReceiveRequest(const QString& requestId, const QSt
 
 void ActivityListModel::removePendingReceiveRequest(const QString& requestId)
 {
+    assertModelThread(*this);
     for (int i = 0; i < m_transactions.size(); ++i) {
         if (!m_transactions[i]->isPendingRequest || m_transactions[i]->requestId != requestId) {
             continue;
@@ -306,6 +324,7 @@ void ActivityListModel::removePendingReceiveRequest(const QString& requestId)
 
 void ActivityListModel::updateTransaction(const uint256& hash, const interfaces::WalletTxStatus& tx_status, int num_blocks, int64_t block_time)
 {
+    assertModelThread(*this);
     int index = findTransactionIndex(hash);
 
     if (index != -1) {
@@ -389,9 +408,12 @@ void ActivityListModel::subscribeToCoreSignals()
         interfaces::WalletTxStatus wtx;
         int num_blocks;
         int64_t block_time;
-        if (m_wallet_model->tryGetTxStatus(hash, wtx, num_blocks, block_time)) {
-            updateTransaction(hash, wtx, num_blocks, block_time);
+        if (!m_wallet_model->tryGetTxStatus(hash, wtx, num_blocks, block_time)) {
+            return;
         }
+        QMetaObject::invokeMethod(this, [this, hash, wtx, num_blocks, block_time] {
+            updateTransaction(hash, wtx, num_blocks, block_time);
+        }, Qt::QueuedConnection);
     });
 }
 

qml/pages/node/CommandConsole.qml

@@ -329,8 +329,8 @@ Page {
                 // popup is open; otherwise it submits the command. This mirrors the
                 // Tab behaviour and avoids submitting the raw half-typed text while a
                 // completion is highlighted.
-                Keys.onReturnPressed: root.acceptHighlightedOrSubmit(event)
-                Keys.onEnterPressed: root.acceptHighlightedOrSubmit(event)
+                Keys.onReturnPressed: (event) => root.acceptHighlightedOrSubmit(event)
+                Keys.onEnterPressed: (event) => root.acceptHighlightedOrSubmit(event)
 
                 // Up/Down: navigate autocomplete when popup is open,
                 // otherwise browse command history.
@@ -362,7 +362,7 @@ Page {
                 }
 
                 // Tab key: accept the top autocomplete suggestion.
-                Keys.onTabPressed: {
+                Keys.onTabPressed: (event) => {
                     if (!root.searchMode && autocompletePopup.visible && filteredCommands.length > 0) {
                         applySuggestion(filteredCommands[autocompleteIndex])
                         event.accepted = true

test/CMakeLists.txt

@@ -38,6 +38,7 @@ add_executable(bitcoinqml_unit_tests
   test_transaction.cpp
   test_activityfilterproxymodel.cpp
   test_qtinfolog.cpp
+  test_activitylistmodel.cpp
   test_qtmessagefilter.cpp
   test_rpccommandexecutor.cpp
   test_rpcconsolemodel.cpp

test/mocks/mockwallet.h

@@ -153,6 +153,8 @@ class MockWallet : public StubWallet
     std::function<CoinsList()> list_coins_fn;
     std::function<OutputType()> get_default_address_type_fn;
     std::function<std::unique_ptr<interfaces::Handler>(TransactionChangedFn)> handle_transaction_changed_fn;
+    std::function<interfaces::WalletTx(const Txid&)> get_wallet_tx_fn;
+    std::function<bool(const Txid&, interfaces::WalletTxStatus&, int&, int64_t&)> try_get_tx_status_fn;
     std::function<bool(const Txid&)> transaction_can_be_bumped_fn;
     std::function<bool(const Txid&, const wallet::CCoinControl&, std::vector<bilingual_str>&, CAmount&, CAmount&, CMutableTransaction&)> create_bump_transaction_fn;
     std::function<bool(CMutableTransaction&)> sign_bump_transaction_fn;
@@ -162,6 +164,8 @@ class MockWallet : public StubWallet
         CallCounter getNewDestination{"getNewDestination"};
         CallCounter createTransaction{"createTransaction"};
         CallCounter getWalletTxs{"getWalletTxs"};
+        CallCounter getWalletTx{"getWalletTx"};
+        CallCounter tryGetTxStatus{"tryGetTxStatus"};
         CallCounter getBalance{"getBalance"};
         CallCounter getAvailableBalance{"getAvailableBalance"};
         CallCounter getRequiredFee{"getRequiredFee"};
@@ -209,6 +213,18 @@ class MockWallet : public StubWallet
         return get_wallet_txs_fn ? get_wallet_txs_fn() : std::set<interfaces::WalletTx>{};
     }
 
+    interfaces::WalletTx getWalletTx(const Txid& txid) override
+    {
+        ++calls.getWalletTx;
+        return get_wallet_tx_fn ? get_wallet_tx_fn(txid) : interfaces::WalletTx{};
+    }
+
+    bool tryGetTxStatus(const Txid& txid, interfaces::WalletTxStatus& tx_status, int& num_blocks, int64_t& block_time) override
+    {
+        ++calls.tryGetTxStatus;
+        return try_get_tx_status_fn ? try_get_tx_status_fn(txid, tx_status, num_blocks, block_time) : false;
+    }
+
     CAmount getBalance() override
     {
         ++calls.getBalance;

test/qml/bitcoin_qmltests.qrc

@@ -4,6 +4,7 @@
         <file>tst_activity.qml</file>
         <file>tst_address_components.qml</file>
         <file>tst_blockclock.qml</file>
+        <file>tst_commandconsole.qml</file>
         <file>tst_contextmenubutton.qml</file>
         <file>tst_contextmenudivider.qml</file>
         <file>tst_contextmenupicker.qml</file>

test/qml/qml_tests_main.cpp

@@ -5,6 +5,7 @@
 #include <QtQuickTest/quicktest.h>
 
 #include <QAbstractListModel>
+#include <QColor>
 #include <QDateTime>
 #include <QFont>
 #include <QHash>
@@ -3377,6 +3378,160 @@ class MockDebugLogModel : public QAbstractListModel
     int m_next_old_row{0};
 };
 
+class MockRpcOutputListModel : public QAbstractListModel
+{
+    Q_OBJECT
+    Q_PROPERTY(int count READ rowCount NOTIFY countChanged)
+
+public:
+    enum Role {
+        TimestampRole = Qt::UserRole + 1,
+        ContentRole,
+        CategoryRole,
+    };
+    Q_ENUM(Role)
+
+    int rowCount(const QModelIndex& parent = QModelIndex()) const override
+    {
+        return parent.isValid() ? 0 : m_rows.size();
+    }
+
+    QVariant data(const QModelIndex& index, int role = Qt::DisplayRole) const override
+    {
+        if (!index.isValid() || index.row() < 0 || index.row() >= m_rows.size()) return {};
+        const Row& row = m_rows.at(index.row());
+        switch (role) {
+        case TimestampRole: return row.timestamp;
+        case ContentRole: return row.content;
+        case CategoryRole: return row.category;
+        default: return {};
+        }
+    }
+
+    QHash<int, QByteArray> roleNames() const override
+    {
+        return {
+            {TimestampRole, "timestamp"},
+            {ContentRole, "content"},
+            {CategoryRole, "category"},
+        };
+    }
+
+    void appendRow(const QString& content, int category)
+    {
+        beginInsertRows(QModelIndex(), m_rows.size(), m_rows.size());
+        m_rows.append(Row{QStringLiteral("00:00:00"), content, category});
+        endInsertRows();
+        Q_EMIT countChanged();
+    }
+
+    void resetAll()
+    {
+        if (m_rows.isEmpty()) return;
+        beginResetModel();
+        m_rows.clear();
+        endResetModel();
+        Q_EMIT countChanged();
+    }
+
+Q_SIGNALS:
+    void countChanged();
+
+private:
+    struct Row {
+        QString timestamp;
+        QString content;
+        int category{0};
+    };
+
+    QList<Row> m_rows;
+};
+
+class MockRpcConsoleModel : public QObject
+{
+    Q_OBJECT
+    Q_PROPERTY(bool executing READ executing NOTIFY executingChanged)
+    Q_PROPERTY(QStringList availableCommands READ availableCommands NOTIFY availableCommandsChanged)
+    Q_PROPERTY(QAbstractListModel* outputModel READ outputModel CONSTANT)
+    Q_PROPERTY(QColor requestColor MEMBER m_request_color)
+    Q_PROPERTY(QColor replyColor MEMBER m_reply_color)
+    Q_PROPERTY(QColor errorColor MEMBER m_error_color)
+    Q_PROPERTY(QColor keyColor MEMBER m_key_color)
+    // Test-only observation point: every command the console accepted.
+    Q_PROPERTY(QStringList submittedCommands READ submittedCommands NOTIFY submittedCommandsChanged)
+
+public:
+    bool executing() const { return m_executing; }
+    QStringList availableCommands() const { return m_available_commands; }
+    QAbstractListModel* outputModel() { return &m_output_model; }
+    QStringList submittedCommands() const { return m_submitted_commands; }
+
+    Q_INVOKABLE bool submitCommand(const QString& command, const QString& /*wallet_name*/ = {})
+    {
+        if (!m_accept_commands) return false;
+        m_submitted_commands.append(command);
+        m_output_model.appendRow(command, 0);
+        Q_EMIT submittedCommandsChanged();
+        return true;
+    }
+
+    Q_INVOKABLE void ensureWelcomeMessage()
+    {
+        if (m_output_model.rowCount() > 0) return;
+        m_output_model.appendRow(QStringLiteral("Welcome"), 0);
+    }
+
+    // No history is replayed; Up/Down leave the typed text as it is.
+    Q_INVOKABLE QString browseHistory(int /*direction*/, const QString& current_text) { return current_text; }
+
+    Q_INVOKABLE void resetHistoryNavigation() {}
+
+    Q_INVOKABLE void clear()
+    {
+        m_output_model.resetAll();
+        ensureWelcomeMessage();
+    }
+
+    // Reset every observable back to a known state between test functions.
+    Q_INVOKABLE void resetForTest()
+    {
+        m_output_model.resetAll();
+        m_submitted_commands.clear();
+        m_accept_commands = true;
+        setAvailableCommands({QStringLiteral("getbalance"),
+                              QStringLiteral("getblockcount"),
+                              QStringLiteral("getblockhash"),
+                              QStringLiteral("help")});
+        Q_EMIT submittedCommandsChanged();
+    }
+
+    Q_INVOKABLE void setAvailableCommands(const QStringList& commands)
+    {
+        if (m_available_commands == commands) return;
+        m_available_commands = commands;
+        Q_EMIT availableCommandsChanged();
+    }
+
+    // Make submitCommand() refuse, mirroring a console that is still busy.
+    Q_INVOKABLE void setAcceptCommands(bool accept) { m_accept_commands = accept; }
+
+Q_SIGNALS:
+    void executingChanged();
+    void availableCommandsChanged();
+    void submittedCommandsChanged();
+
+private:
+    MockRpcOutputListModel m_output_model;
+    QStringList m_available_commands;
+    QStringList m_submitted_commands;
+    bool m_executing{false};
+    bool m_accept_commands{true};
+    QColor m_request_color;
+    QColor m_reply_color;
+    QColor m_error_color;
+    QColor m_key_color;
+};
+
 class QmlTestsSetup : public QObject
 {
     Q_OBJECT
@@ -3408,6 +3563,8 @@ public Q_SLOTS:
         static MockBumpTransactionModel bump_model;
         static MockDesktopWindowBehaviorModel desktop_window_behavior_model;
         static MockDebugLogModel debug_log_model;
+        static MockRpcConsoleModel rpc_console_model;
+        rpc_console_model.resetForTest();
         recipients_model.setCurrent(&send_recipient);
         wallet_model.setActivityListModel(&activity_list_model);
         wallet_model.setBumpModel(&bump_model);
@@ -3475,6 +3632,8 @@ public Q_SLOTS:
         engine->rootContext()->setContextProperty(QStringLiteral("desktopWindowBehaviorModel"), &desktop_window_behavior_model);
         engine->rootContext()->setContextProperty(QStringLiteral("debugLogModel"), &debug_log_model);
         engine->rootContext()->setContextProperty(QStringLiteral("testDebugLogModel"), &debug_log_model);
+        engine->rootContext()->setContextProperty(QStringLiteral("rpcConsoleModel"), &rpc_console_model);
+        engine->rootContext()->setContextProperty(QStringLiteral("testRpcConsoleModel"), &rpc_console_model);
         engine->addImportPath(QStringLiteral(BITCOINQML_QML_SOURCE_DIR));
     }
 };