From 286acdf6951194536ead88de80a7940b320bd056 Mon Sep 17 00:00:00 2001 From: Czarek Nakamoto Date: Sat, 11 May 2024 16:25:10 +0200 Subject: [PATCH] store crash fix Monero wallet crashes (sometimes) when it is syncing, while the proper solution (that can be seen in feather) is to not store wallet while it is being synced, this is not acceptable for mobile wallets where OS can just come and kill the wallet because it felt like it. This patch depends on the background-sync patch, but to use it as a standalone fix grabbing the definition for the LOCK_REFRESH macro should be enough. tobtoht suggested: _say you want to store every 15 minutes during background sync. you stop the refresh every 15 minutes. then do something like this in the callback:_ ``` // Make sure this doesn't run in the refresh thread onRefreshed() { if (hasItBeen15MinutesSinceWeStored()) { store(); } if (shouldWeContinueRefreshing()) { startRefresh(); } } ``` which works for crashes after the wallet is initially synced but doesn't solve the issue for wallet that are syncing (it would just wait for it to finish before actually storing). Also imo store() functin should store the wallet, no matter the current state. --- src/wallet/api/wallet.cpp | 25 ++++++++++++------------- src/wallet/api/wallet.h | 1 - src/wallet/wallet2.cpp | 12 +++++++++++- src/wallet/wallet2.h | 3 +++ 4 files changed, 26 insertions(+), 15 deletions(-) diff --git a/src/wallet/api/wallet.cpp b/src/wallet/api/wallet.cpp index 04f278e15..29339207f 100644 --- a/src/wallet/api/wallet.cpp +++ b/src/wallet/api/wallet.cpp @@ -57,8 +57,8 @@ using namespace cryptonote; #define MONERO_DEFAULT_LOG_CATEGORY "WalletAPI" #define LOCK_REFRESH() \ - bool refresh_enabled = m_refreshEnabled; \ - m_refreshEnabled = false; \ + bool refresh_enabled = m_wallet->get_refresh_enabled(); \ + m_wallet->set_refresh_enabled(false); \ m_wallet->stop(); \ m_refreshCV.notify_one(); \ boost::mutex::scoped_lock lock(m_refreshMutex); \ @@ -468,7 +468,7 @@ WalletImpl::WalletImpl(NetworkType nettype, uint64_t kdf_rounds) m_wallet2Callback.reset(new Wallet2CallbackImpl(this)); m_wallet->callback(m_wallet2Callback.get()); m_refreshThreadDone = false; - m_refreshEnabled = false; + m_wallet->set_refresh_enabled(false); m_addressBook.reset(new AddressBookImpl(this)); m_subaddress.reset(new SubaddressImpl(this)); m_coins.reset(new CoinsImpl(this)); @@ -1092,6 +1092,7 @@ void WalletImpl::stop() bool WalletImpl::store(const std::string &path) { clearStatus(); + LOCK_REFRESH(); try { if (path.empty()) { m_wallet->store(); @@ -2877,10 +2878,10 @@ void WalletImpl::refreshThreadFunc() } LOG_PRINT_L3(__FUNCTION__ << ": refresh lock acquired..."); - LOG_PRINT_L3(__FUNCTION__ << ": m_refreshEnabled: " << m_refreshEnabled); + LOG_PRINT_L3(__FUNCTION__ << ": m_refreshEnabled: " << m_wallet->get_refresh_enabled()); LOG_PRINT_L3(__FUNCTION__ << ": m_status: " << status()); LOG_PRINT_L3(__FUNCTION__ << ": m_refreshShouldRescan: " << m_refreshShouldRescan); - if (m_refreshEnabled) { + if (m_wallet->get_refresh_enabled()) { LOG_PRINT_L3(__FUNCTION__ << ": refreshing..."); doRefresh(); } @@ -2912,7 +2913,7 @@ void WalletImpl::doRefresh() } m_wallet->find_and_save_rings(false); } else { - LOG_PRINT_L3(__FUNCTION__ << ": skipping refresh - daemon is not synced"); + LOG_PRINT_L3(__FUNCTION__ << ": skipping refresh - daemon is not synced"); } // assuming if we have empty history, it wasn't initialized yet // for further history changes client need to update history in @@ -2925,7 +2926,7 @@ void WalletImpl::doRefresh() success = false; setStatusError(e.what()); break; - }while(!rescan && (rescan=m_refreshShouldRescan.exchange(false))); // repeat if not rescanned and rescan was requested + }while(m_wallet->get_refresh_enabled() && !rescan && (rescan=m_refreshShouldRescan.exchange(false))); // repeat if not rescanned and rescan was requested m_is_connected = success; if (m_wallet2Callback->getListener()) { @@ -2936,9 +2937,9 @@ void WalletImpl::doRefresh() void WalletImpl::startRefresh() { - if (!m_refreshEnabled) { + if (!m_wallet->get_refresh_enabled()) { LOG_PRINT_L2(__FUNCTION__ << ": refresh started/resumed..."); - m_refreshEnabled = true; + m_wallet->set_refresh_enabled(true); m_refreshCV.notify_one(); } } @@ -2948,7 +2949,7 @@ void WalletImpl::startRefresh() void WalletImpl::stopRefresh() { if (!m_refreshThreadDone) { - m_refreshEnabled = false; + m_wallet->set_refresh_enabled(false); m_refreshThreadDone = true; m_refreshCV.notify_one(); m_refreshThread.join(); @@ -2959,9 +2960,7 @@ void WalletImpl::pauseRefresh() { LOG_PRINT_L2(__FUNCTION__ << ": refresh paused..."); // TODO synchronize access - if (!m_refreshThreadDone) { - m_refreshEnabled = false; - } + m_wallet->set_refresh_enabled(false); } diff --git a/src/wallet/api/wallet.h b/src/wallet/api/wallet.h index 7b885e866..23d266de2 100644 --- a/src/wallet/api/wallet.h +++ b/src/wallet/api/wallet.h @@ -325,7 +325,6 @@ private: std::unique_ptr m_subaddressAccount; // multi-threaded refresh stuff - std::atomic m_refreshEnabled; std::atomic m_refreshThreadDone; std::atomic m_refreshIntervalMillis; std::atomic m_refreshShouldRescan; diff --git a/src/wallet/wallet2.cpp b/src/wallet/wallet2.cpp index 4073974d9..0a0541cc8 100644 --- a/src/wallet/wallet2.cpp +++ b/src/wallet/wallet2.cpp @@ -1204,6 +1204,7 @@ wallet2::wallet2(network_type nettype, uint64_t kdf_rounds, bool unattended, std m_upper_transaction_weight_limit(0), m_run(true), m_callback(0), + m_refreshEnabled(false), m_trusted_daemon(false), m_nettype(nettype), m_multisig_rounds_passed(0), @@ -1418,6 +1419,14 @@ bool wallet2::set_daemon(std::string daemon_address, boost::optionalset_proxy(address); @@ -4217,8 +4226,9 @@ void wallet2::refresh(bool trusted_daemon, uint64_t start_height, uint64_t & blo // infer when we get an incoming output bool first = true, last = false; - while(m_run.load(std::memory_order_relaxed) && blocks_fetched < max_blocks) + while(m_run.load(std::memory_order_relaxed) && blocks_fetched < max_blocks && m_refreshEnabled) { + LOG_ERROR("SYNCING"); uint64_t next_blocks_start_height; std::vector next_blocks; std::vector next_parsed_blocks; diff --git a/src/wallet/wallet2.h b/src/wallet/wallet2.h index d1e68baac..556306f20 100644 --- a/src/wallet/wallet2.h +++ b/src/wallet/wallet2.h @@ -1086,6 +1086,8 @@ private: boost::optional daemon_login = boost::none, bool trusted_daemon = true, epee::net_utils::ssl_options_t ssl_options = epee::net_utils::ssl_support_t::e_ssl_support_autodetect); bool set_proxy(const std::string &address); + bool get_refresh_enabled(); + void set_refresh_enabled(bool val); void stop() { m_run.store(false, std::memory_order_relaxed); m_message_store.stop(); } @@ -2037,6 +2039,7 @@ private: boost::recursive_mutex m_daemon_rpc_mutex; + bool m_refreshEnabled; bool m_trusted_daemon; i_wallet2_callback* m_callback; hw::device::device_type m_key_device_type; -- 2.39.2