Merge pull request 'Simplify store wallet logic' (#297) from tobtoht/feather:store_on_close into master

Reviewed-on: https://git.wownero.com/feather/feather/pulls/297

src/appcontext.cpp

@@ -108,13 +108,10 @@ AppContext::AppContext(QCommandLineParser *cmdargs) {
     this->ws = new WSClient(this, m_wsUrl);
     connect(this->ws, &WSClient::WSMessage, this, &AppContext::onWSMessage);
 
-    // timers
+    // Store the wallet every 2 minutes
-    m_storeTimer.setSingleShot(true);
+    m_storeTimer.start(2 * 60 * 1000);
     connect(&m_storeTimer, &QTimer::timeout, [this](){
-        if (!this->currentWallet)
+        this->storeWallet();
-            return;
-        qDebug() << "Storing wallet";
-        this->currentWallet->store();
     });
 
     // restore height lookup
@@ -248,14 +245,20 @@ void AppContext::onCreateTransactionError(const QString &msg) {
     emit endTransaction();
 }
 
-void AppContext::walletClose(bool emitClosedSignal) {
+void AppContext::closeWallet(bool emitClosedSignal, bool storeWallet) {
-    if(this->currentWallet == nullptr) return;
+    if (this->currentWallet == nullptr)
-    emit walletClosing();
+        return;
-    //ctx->currentWallet->store();  @TODO: uncomment to store on wallet close
+
+    emit walletAboutToClose();
+
+    if (storeWallet) {
+        this->storeWallet();
+    }
+
     this->currentWallet->disconnect();
     this->walletManager->closeWallet();
-    if(this->currentWallet != nullptr)
     this->currentWallet = nullptr;
+
     if (emitClosedSignal)
         emit walletClosed();
 }
@@ -298,16 +301,16 @@ void AppContext::onWalletOpened(Wallet *wallet) {
             qCritical() << errMsg;
             this->walletManager->clearWalletCache(this->walletPath);
             errMsg = QString("%1\n\nAttempted to clean wallet cache. Please restart Feather.").arg(errMsg);
-            this->walletClose(false);
+            this->closeWallet(false);
             emit walletOpenedError(errMsg);
         } else if(errMsg.contains("wallet cannot be opened as")) {
-            this->walletClose(false);
+            this->closeWallet(false);
             emit walletOpenedError(errMsg);
         } else if(errMsg.contains("is opened by another wallet program")) {
-            this->walletClose(false);
+            this->closeWallet(false);
             emit walletOpenedError(errMsg);
         } else {
-            this->walletClose(false);
+            this->closeWallet(false);
             emit walletOpenPasswordNeeded(!this->walletPassword.isEmpty(), wallet->path());
         }
         return;
@@ -665,9 +668,7 @@ void AppContext::donateBeg() {
     config()->set(Config::donateBeg, donationCounter);
 }
 
-AppContext::~AppContext() {
+AppContext::~AppContext() {}
-    this->walletClose(false);
-}
 
 // ############################################## LIBWALLET QT #########################################################
 
@@ -696,10 +697,10 @@ void AppContext::onUnconfirmedMoneyReceived(const QString &txId, quint64 amount)
 void AppContext::onWalletUpdate() {
     if (this->currentWallet->synchronized()) {
         this->refreshModels();
+        this->storeWallet();
     }
 
     this->updateBalance();
-    this->storeWallet();
 }
 
 void AppContext::onWalletRefreshed(bool success) {
@@ -769,10 +770,12 @@ void AppContext::onTransactionCommitted(bool status, PendingTransaction *tx, con
 }
 
 void AppContext::storeWallet() {
-    if (m_storeTimer.isActive())
+    // Do not store a synchronizing wallet: store() is NOT thread safe and may crash the wallet
+    if (this->currentWallet == nullptr || !this->currentWallet->synchronized())
         return;
 
-    m_storeTimer.start(60000);
+    qDebug() << "Storing wallet";
+    this->currentWallet->store();
 }
 
 void AppContext::updateBalance() {

src/appcontext.h

@@ -102,11 +102,13 @@ public:
     void initRestoreHeights();
     void initWS();
     void donateBeg();
-    void walletClose(bool emitClosedSignal = true);
-    void storeWallet();
     void refreshModels();
     void setWindowTitle(bool mining = false);
 
+    // Closes the currently opened wallet
+    void closeWallet(bool emitClosedSignal = true, bool storeWallet = false);
+    void storeWallet();
+
 public slots:
     void onOpenWallet(const QString& path, const QString &password);
     void onCreateTransaction(const QString &address, quint64 amount, const QString &description, bool all);
@@ -136,6 +138,12 @@ private slots:
     void onTransactionCommitted(bool status, PendingTransaction *t, const QStringList& txid);
 
 signals:
+    // Emitted just before the wallet is closed
+    void walletAboutToClose();
+
+    // Emitted after a wallet has been closed
+    void walletClosed();
+
     void balanceUpdated(quint64 balance, quint64 spendable);
     void blockchainSync(int height, int target);
     void refreshSync(int height, int target);
@@ -143,7 +151,6 @@ signals:
     void blockHeightWSUpdated(QMap<QString, int> heights);
     void walletSynchronized();
     void walletOpened();
-    void walletClosed();
     void walletCreatedError(const QString &msg);
     void walletCreated(Wallet *wallet);
     void walletOpenedError(QString msg);
@@ -166,12 +173,10 @@ signals:
     void donationNag();
     void initiateTransaction();
     void endTransaction();
-    void walletClosing();
     void setTitle(const QString &title); // set window title
 
 private:
     const int m_donationBoundary = 15;
-    UtilsNetworking *m_utilsNetworkingNodes{};
     QTimer m_storeTimer;
     QUrl m_wsUrl = QUrl(QStringLiteral("ws://7e6egbawekbkxzkv4244pqeqgoo4axko2imgjbedwnn6s5yb6b7oliqd.onion/ws"));
 };

src/contactswidget.cpp

@@ -133,7 +133,6 @@ void ContactsWidget::newContact(QString address, QString name)
     }
 
     m_ctx->currentWallet->addressBook()->addRow(address, "", name);
-    m_ctx->storeWallet();
 }
 
 void ContactsWidget::deleteContact()

src/mainwindow.cpp

@@ -103,6 +103,7 @@ MainWindow::MainWindow(AppContext *ctx, QWidget *parent) :
     connect(m_ctx->XMRTo, &XmrTo::connectionSuccess, ui->xmrToWidget, &XMRToWidget::onConnectionSuccess);
     connect(m_ctx, &AppContext::balanceUpdated, ui->xmrToWidget, &XMRToWidget::onBalanceUpdated);
     connect(m_ctx->XMRTo, &XmrTo::openURL, this, [=](const QString &url){ Utils::externalLinkWarning(this, url); });
+    connect(m_ctx, &AppContext::walletClosed, ui->xmrToWidget, &XMRToWidget::onWalletClosed);
     ui->xmrToWidget->setHistoryModel(m_ctx->XMRTo->tableModel);
 #else
     ui->tabExchanges->setTabVisible(0, false);
@@ -198,11 +199,12 @@ MainWindow::MainWindow(AppContext *ctx, QWidget *parent) :
     });
 
     // libwalletqt
-    connect(this, &MainWindow::walletClosed, ui->xmrToWidget, &XMRToWidget::onWalletClosed);
+    connect(m_ctx, &AppContext::walletClosed, [this]{
-    connect(this, &MainWindow::walletClosed, ui->sendWidget, &SendWidget::onWalletClosed);
+        this->onWalletClosed();
+    });
+    connect(m_ctx, &AppContext::walletClosed, ui->sendWidget, &SendWidget::onWalletClosed);
     connect(m_ctx, &AppContext::balanceUpdated, this, &MainWindow::onBalanceUpdated);
     connect(m_ctx, &AppContext::walletOpened, this, &MainWindow::onWalletOpened);
-    connect(m_ctx, &AppContext::walletClosed, this, QOverload<>::of(&MainWindow::onWalletClosed));
     connect(m_ctx, &AppContext::walletOpenedError, this, &MainWindow::onWalletOpenedError);
     connect(m_ctx, &AppContext::walletCreatedError, this, &MainWindow::onWalletCreatedError);
     connect(m_ctx, &AppContext::walletCreated, this, &MainWindow::onWalletCreated);
@@ -281,7 +283,6 @@ MainWindow::MainWindow(AppContext *ctx, QWidget *parent) :
     // Receive
     connect(ui->receiveWidget, &ReceiveWidget::generateSubaddress, [=]() {
         m_ctx->currentWallet->subaddress()->addRow( m_ctx->currentWallet->currentSubaddressAccount(), "");
-        m_ctx->storeWallet();
     });
     connect(ui->receiveWidget, &ReceiveWidget::showTransactions, [this](const QString &text) {
         ui->historyWidget->setSearchText(text);
@@ -328,7 +329,7 @@ MainWindow::MainWindow(AppContext *ctx, QWidget *parent) :
     });
     connect(ui->coinsWidget, &CoinsWidget::sweepOutput, m_ctx, &AppContext::onSweepOutput);
 
-    connect(m_ctx, &AppContext::walletClosing, [=]{
+    connect(m_ctx, &AppContext::walletAboutToClose, [=]{
         if (!config()->get(Config::showTabHome).toBool())
             ui->tabWidget->setCurrentIndex(Tabs::HISTORY);
         else
@@ -540,15 +541,9 @@ void MainWindow::showWizard(WalletWizard::Page startPage) {
     m_wizard->show();
 }
 
-void MainWindow::onWalletClosed() {
-    this->onWalletClosed(WalletWizard::Page_Menu);
-}
-
 void MainWindow::onWalletClosed(WalletWizard::Page page) {
-    emit walletClosed();
     m_statusLabelBalance->clear();
     m_statusLabelStatus->clear();
-    this->setWindowTitle("Feather");
     this->showWizard(page);
 }
 
@@ -628,32 +623,17 @@ void MainWindow::onWalletOpened() {
             m_ctx->currentWallet->subaddress()->addRow(m_ctx->currentWallet->currentSubaddressAccount(), "");
         }
     }
-    connect(m_ctx->currentWallet->subaddress(), &Subaddress::labelChanged, [this]{
-        m_ctx->storeWallet();
-    });
 
     // history page
     m_ctx->currentWallet->history()->refresh(m_ctx->currentWallet->currentSubaddressAccount());
     ui->historyWidget->setModel(m_ctx->currentWallet->historyModel(), m_ctx->currentWallet);
-    connect(m_ctx->currentWallet->history(), &TransactionHistory::txNoteChanged, [this]{
-        m_ctx->storeWallet();
-    });
 
     // contacts widget
     ui->contactWidget->setModel(m_ctx->currentWallet->addressBookModel());
-    connect(m_ctx->currentWallet->addressBook(), &AddressBook::descriptionChanged, [this]{
-        m_ctx->storeWallet();
-    });
 
     // coins page
     m_ctx->currentWallet->coins()->refresh(m_ctx->currentWallet->currentSubaddressAccount());
     ui->coinsWidget->setModel(m_ctx->currentWallet->coinsModel(), m_ctx->currentWallet->coins());
-    connect(m_ctx->currentWallet->coins(), &Coins::coinFrozen, [this]{
-        m_ctx->storeWallet();
-    });
-    connect(m_ctx->currentWallet->coins(), &Coins::coinThawed, [this]{
-        m_ctx->storeWallet();
-    });
 
     this->touchbarShowWallet();
     this->updatePasswordIcon();
@@ -983,8 +963,10 @@ void MainWindow::menuQuitClicked() {
 }
 
 void MainWindow::menuWalletCloseClicked() {
-    if(m_ctx->currentWallet == nullptr) return;
+    if (m_ctx->currentWallet == nullptr)
-    m_ctx->walletClose(true);
+        return;
+
+    m_ctx->closeWallet(true, true);
 }
 
 void MainWindow::menuWalletOpenClicked() {
@@ -1005,8 +987,7 @@ void MainWindow::menuWalletOpenClicked() {
         return;
     }
 
-    m_ctx->walletClose(false);
+    m_ctx->closeWallet(false);
-    emit walletClosed();
     m_ctx->onOpenWallet(path, "");
 }
 
@@ -1112,10 +1093,6 @@ void MainWindow::importContacts() {
         }
     }
 
-    if(inserts > 0) {
-        m_ctx->storeWallet();
-    }
-
     QMessageBox::information(this, "Contacts imported", QString("Total contacts imported: %1").arg(inserts));
 }
 
@@ -1243,7 +1220,7 @@ void MainWindow::importOutputs() {
 }
 
 void MainWindow::cleanupBeforeClose() {
-    m_ctx->walletManager->closeWallet();
+    m_ctx->closeWallet(false, true);
     m_ctx->tor->stop();
 
     this->saveGeo();
@@ -1321,7 +1298,7 @@ void MainWindow::importTransaction() {
 }
 
 void MainWindow::updateNetStats() {
-    if (!m_ctx->currentWallet) {
+    if (m_ctx->currentWallet == nullptr) {
         m_statusLabelNetStats->setText("");
         return;
     }

src/mainwindow.h

@@ -137,15 +137,13 @@ public slots:
     void onBalanceUpdated(quint64 balance, quint64 spendable);
     void onSynchronized();
     void onWalletOpened();
-    void onWalletClosed();
+    void onWalletClosed(WalletWizard::Page page = WalletWizard::Page_Menu);
-    void onWalletClosed(WalletWizard::Page page);
     void onConnectionStatusChanged(int status);
     void onCreateTransactionError(const QString &message);
     void onCreateTransactionSuccess(PendingTransaction *tx, const QString &address, const quint32 &mixin);
     void onTransactionCommitted(bool status, PendingTransaction *tx, const QStringList& txid);
 
 signals:
-    void walletClosed();
     void closed();
 
 private:
@@ -202,9 +200,6 @@ private:
     StatusBarButton *m_statusBtnSeed;
     StatusBarButton *m_statusBtnTor;
 
-    SubaddressProxyModel *subaddressProxyModel;
-    TransactionHistoryModel *txHistModel;
-    CoinsModel *coinsModel;
 #ifdef Q_OS_MAC
     QAction *m_touchbarActionWelcome;
     KDMacTouchBar *m_touchbar;

src/widgets/xmrigwidget.cpp

@@ -84,11 +84,11 @@ XMRigWidget::XMRigWidget(AppContext *ctx, QWidget *parent) :
     // username/password
     connect(ui->lineEdit_password, &QLineEdit::editingFinished, [=]() {
         m_ctx->currentWallet->setCacheAttribute("feather.xmrig_password", ui->lineEdit_password->text());
-        m_ctx->currentWallet->store();
+        m_ctx->storeWallet();
     });
     connect(ui->lineEdit_address, &QLineEdit::editingFinished, [=]() {
         m_ctx->currentWallet->setCacheAttribute("feather.xmrig_username", ui->lineEdit_address->text());
-        m_ctx->currentWallet->store();
+        m_ctx->storeWallet();
     });
 
     // checkbox connects