fix: mixing for partially unlocked descriptor wallets #6092
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
knst
changed the title
kwvg
requested changes
Jul 3, 2024
Collaborator
There was a problem hiding this comment.
w.r.t c944908
UPDATE 3: References to a potential transaction history bug have been removed as they were caused by a filter setting and was not a bug (see correcting comment), balance reporting was working as expected. Approved.
| void LegacyScriptPubKeyMan::UpgradeKeyMetadata() | ||
| { | ||
| LOCK(cs_KeyStore); // mapKeyMetadata | ||
| if (m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA) || !IsHDEnabled()) { |
Collaborator
There was a problem hiding this comment.
nit: m_storage.IsLocked(/* fForMixing = */ false)
Collaborator
Author
|
@kwvg you don't see transaction history for CJ because you have a filter "most common" activated: |
Collaborator
@knst Odd, it was a clean instance, hadn't activated any filter in any instances, current or prior. Thanks for pointing it out, have corrected post! |
UdjinM6
approved these changes
Jul 7, 2024
PastaPastaPasta
added a commit
to PastaPastaPasta/dash
that referenced
this pull request
Jul 15, 2024
…lets 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](dashpay#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
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 15, 2024
db82817 Merge #6106: feat: create new composite quorum-command platformsign (pasta) a45e6df Merge #6104: fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta) 7330982 Merge #6100: feat: make whitelist works with composite commands for platform needs (pasta) 9998ffd Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code (pasta) cdf7a25 Merge #6095: fix: createwallet to require 'load_on_startup' for descriptor wallets (pasta) c1c2c55 Merge #6092: fix: mixing for partially unlocked descriptor wallets (pasta) 1175486 Merge #6073: feat: add logging for RPC HTTP requests: command, user, http-code, time of running (pasta) Pull request description: ## Issue being fixed or feature implemented Backports a set of 6 PRs needed in rc.2 ## What was done? Backported PRs with labels ## How Has This Been Tested? ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] 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 _(for repository code-owners and collaborators only)_ ACKs for top commit: kwvg: LGTM, utACK db82817 UdjinM6: utACK db82817 Tree-SHA512: 1b242c5db04bd5873ef622543bc2a25e29567f15962c677ea51ff05cb784291968d18f419bf611c206b912e8f15d687208ae75af33aab89038b6f0167d99c4bf
Member
|
back ported in #6115 |
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 29, 2024
98a3393 chore: set release to true (pasta) cd0a3a6 Merge #6154: chore: remove trailing whitespaces in release notes (pasta) 6bc60a7 Merge #6151: chore: update seeds for v21 release (pasta) 88e949a Merge #6146: chore: bump assumevalid, minchainwork, checkpoints, chaintxdata (pasta) cc14427 Merge #6144: docs: release notes for v21.0.0 (pasta) 0a8ece1 Merge #6122: chore: translations 2024-07 (pasta) 146d244 Merge #6140: feat: harden all sporks on mainnet to current values (pasta) 024d272 Merge #6126: feat: enable EHF activation of MN_RR on mainnet (pasta) e780b3d Merge #6125: docs: update manpages for 21.0 (pasta) 5ede23c Merge #6118: docs: add release notes notifying change of default branch to `develop` (pasta) 1b6fe9c Merge #6117: docs: update supported versions in SECURITY.md (pasta) 27d20be Merge #6116: fix: mitigate crashes associated with some upgradetohd edge cases (pasta) db82817 Merge #6106: feat: create new composite quorum-command platformsign (pasta) a45e6df Merge #6104: fix: adjust incorrect parameter description that says there is a default that doesn't exist (pasta) 7330982 Merge #6100: feat: make whitelist works with composite commands for platform needs (pasta) 9998ffd Merge #6096: feat: split type of error in submitchainlock - return enum in CL verifying code (pasta) cdf7a25 Merge #6095: fix: createwallet to require 'load_on_startup' for descriptor wallets (pasta) c1c2c55 Merge #6092: fix: mixing for partially unlocked descriptor wallets (pasta) 1175486 Merge #6073: feat: add logging for RPC HTTP requests: command, user, http-code, time of running (pasta) Pull request description: ## Issue being fixed or feature implemented Suppressed changes from be83865 so the diff is empty. ## What was done? ## How Has This Been Tested? ## Breaking Changes ## 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 _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: ACK 158cf86; no diff knst: ACK 158cf86 Tree-SHA512: 3310a39fbcb45bdf09f885fe77ba769c0a715869a3bb287eaf0f2cf54b35a7e1f832c88df3bd31097eabf2d375515c1b87ff05e0c3282cef642833a154c42bbe
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
As noticed by kwvg, 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: