From c1c2c5569007f12100ddb1c79044ef7e5193924c Mon Sep 17 00:00:00 2001 From: pasta Date: Tue, 9 Jul 2024 09:36:46 -0500 Subject: [PATCH] Merge #6092: fix: mixing for partially unlocked descriptor wallets c94490839912e21965a01655fa24f837f037b4f6 refactor: simplify implementation of function CWallet::IsLocked (Konstantin Akimov) 685cf34cb9d4505c0ff2980e515bfa7dbc8d3e81 fix: unlock descriptor wallet for mixing-only (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented As [noticed by kwvg](https://github.com/dashpay/dash/pull/6090#pullrequestreview-2153639183), mixing doesn't work with descriptor wallets if do "unlock only for mixing". This PR is aiming to fix this issue. https://github.com/dashpay/dash-issues/issues/59 ## What was done? Removed default argument "bool mixing = false" from WalletStorage interface, Refactored a logic of CWallet::IsLocked to make it shorter, clearer and unified with bitcoin. ## How Has This Been Tested? A. Run Dash-Qt with descriptor wallet, run mixing, enter passphrase. The wallet is partially unlocked (for mixing only) - possible to see yellow lock in status. Mixing happens. B. Open "send transaction dialog", try to send a transaction: the app asks password to unlock wallet as expected. Though, I am not sure how exactly to test that **all** rpc are indeed locked when descriptor wallet is unlocked for mixing only. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: LGTM, ~utACK~ light ACK c94490839912e21965a01655fa24f837f037b4f6 kwvg: ACK c94490839912e21965a01655fa24f837f037b4f6 PastaPastaPasta: utACK c94490839912e21965a01655fa24f837f037b4f6 Tree-SHA512: 236c895dd75042449282b051e90781ace1c941a3b2c34bb29ddadb6e62ba9c8d57c2a677ed98847630ff7fb6df4e14d2b59f3473d8f299ec104afeeb8103930c --- src/wallet/scriptpubkeyman.cpp | 8 ++++---- src/wallet/scriptpubkeyman.h | 2 +- src/wallet/wallet.cpp | 14 ++------------ 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index b3840905ea..678022e1f2 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -331,7 +331,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(WalletBatch &batch, const CScrip void LegacyScriptPubKeyMan::UpgradeKeyMetadata() { LOCK(cs_KeyStore); // mapKeyMetadata - if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA) || !IsHDEnabled()) { + if (m_storage.IsLocked(false) || m_storage.IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA) || !IsHDEnabled()) { return; } @@ -1901,7 +1901,7 @@ void DescriptorScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, std::map DescriptorScriptPubKeyMan::GetKeys() const { AssertLockHeld(cs_desc_man); - if (m_storage.HasEncryptionKeys() && !m_storage.IsLocked()) { + if (m_storage.HasEncryptionKeys() && !m_storage.IsLocked(true)) { KeyMap keys; for (auto key_pair : m_map_crypted_keys) { const CPubKey& pubkey = key_pair.second.first; @@ -2014,7 +2014,7 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const } if (m_storage.HasEncryptionKeys()) { - if (m_storage.IsLocked()) { + if (m_storage.IsLocked(true)) { return false; } @@ -2423,7 +2423,7 @@ bool DescriptorScriptPubKeyMan::GetDescriptorString(std::string& out) const void DescriptorScriptPubKeyMan::UpgradeDescriptorCache() { LOCK(cs_desc_man); - if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) { + if (m_storage.IsLocked(false) || m_storage.IsWalletFlagSet(WALLET_FLAG_LAST_HARDENED_XPUB_CACHED)) { return; } diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 7188ce9d1d..9c639b73eb 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -40,7 +40,7 @@ public: virtual void SetMinVersion(enum WalletFeature, WalletBatch* = nullptr) = 0; virtual const CKeyingMaterial& GetEncryptionKey() const = 0; virtual bool HasEncryptionKeys() const = 0; - virtual bool IsLocked(bool fForMixing = false) const = 0; + virtual bool IsLocked(bool fForMixing) const = 0; // for LegacyScriptPubKeyMan::TopUpInner needs: virtual void UpdateProgress(const std::string&, int) = 0; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4bd880888c..29d6104270 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5459,21 +5459,11 @@ bool CWallet::IsLocked(bool fForMixing) const { if (!IsCrypted()) return false; - bool result; - { - LOCK(cs_wallet); - result = vMasterKey.empty(); - } - // fForMixing fOnlyMixingAllowed return - // --------------------------------------- - // true true result - // true false result - // false true true - // false false result if(!fForMixing && fOnlyMixingAllowed) return true; - return result; + LOCK(cs_wallet); + return vMasterKey.empty(); } bool CWallet::Lock(bool fAllowMixing)