Skip to content

Commit

Permalink
Merge #6092: fix: mixing for partially unlocked descriptor wallets
Browse files Browse the repository at this point in the history
c944908 refactor: simplify implementation of function CWallet::IsLocked (Konstantin Akimov)
685cf34 fix: unlock descriptor wallet for mixing-only (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  As [noticed by kwvg](#6090 (review)), mixing doesn't work with descriptor wallets if do "unlock only for mixing". This PR is aiming to fix this issue.
  dashpay/dash-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 c944908
  kwvg:
    ACK c944908
  PastaPastaPasta:
    utACK c944908

Tree-SHA512: 236c895dd75042449282b051e90781ace1c941a3b2c34bb29ddadb6e62ba9c8d57c2a677ed98847630ff7fb6df4e14d2b59f3473d8f299ec104afeeb8103930c
  • Loading branch information
PastaPastaPasta committed Jul 9, 2024
2 parents 38249a5 + c944908 commit 4f5991f
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 17 deletions.
8 changes: 4 additions & 4 deletions src/wallet/scriptpubkeyman.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down Expand Up @@ -1901,7 +1901,7 @@ void DescriptorScriptPubKeyMan::ReturnDestination(int64_t index, bool internal,
std::map<CKeyID, CKey> 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;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/scriptpubkeyman.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ class WalletStorage
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;
Expand Down
14 changes: 2 additions & 12 deletions src/wallet/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 4f5991f

Please sign in to comment.