fix: auto backup issue with descriptor wallets for CJ #6090
Conversation
This file contains 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
force-pushed
the
descriptors-autobackup-cj
branch
from
a391ce8
to
7198f97
Compare
knst
force-pushed
the
descriptors-autobackup-cj
branch
from
7198f97
to
bebea4b
Compare
|
The first version of PR fixed GUI, but CoinJoin didn't actually started. Added missing changes to |
kwvg
requested changes
Jul 2, 2024
There was a problem hiding this comment.
w.r.t. bebea4b
CoinJoin seems to work as expected when the wallet is completely unlocked (i.e. with the red unlock icon on the bottom right)! 🎉
coinjoin_full_unlock.mp4
There seems to be issues with CoinJoin if the wallet is partially unlocked (i.e. for CoinJoin only, represented with the yellow unlock icon on the bottom right), specifically.
- CoinJoin doesn't seem to be occurring (no new transactions created, no progress)
- The client hangs and freezes, responding to mouse input with multi-second delay, no error or debug log entry to accompany it.
coinjoin_partial_unlock.mp4
There also seems to be a UI bug that prevents the CoinJoin UI from being seen (requiring me to toggle "Enable advanced interface" to view it) though it's very unlikely this bug is related to descriptor wallets.
5 tasks
PastaPastaPasta
added a commit
that referenced
this pull request
Jul 9, 2024
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
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Qt CoinJoin session has problems (#5579 (review)):
https://github.com/dashpay/dash-issues/issues/59
What was done?
Disables check for "remaining keys left" and "auto-backups" for non-legacy wallet.
How Has This Been Tested?
Run unit/functional test
Try to run CJ mixing for descriptor wallet.
Breaking Changes
N/A
Checklist: