-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: allow manipulation of CoinJoin salt using RPC #6093
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
This pull request has conflicts, please rebase. |
|
Let's split it in 3 separate PRs:
|
358f963 to
c89f777
Compare
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I have a solution how to avoid (very slow) rescans, pls check 78625f8
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should NotifyTransactionChanges be out of scope of LOCK of cs_wallet?
@UdjinM6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cs_wallet is held in all other use cases, should be fine here too imo.
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 019a3c070d50f97260cc1e2462c36274003e1b42
|
This pull request has conflicts, please rebase. |
Co-authored-by: UdjinM6 <[email protected]>
knst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK c52c9f86df0873f880a6dbab24300c3b7bbf1cc4
could you add a functional test for coinjoinsalt? You may create new PR, if you feel it is out-of-scope of this PR, but it have to be done for sack of enabling rpc-coverage linter which is not running due to https://github.com/dashpay/dash-issues/issues/63
doc/release-notes-6093.md
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe give little more details how possible to use it?
| - A new RPC command, `coinjoinsalt`, allows for manipulating a CoinJoin salt stored in a wallet. `coinjoinsalt get` will fetch an existing salt, `coinjoinsalt set` will allow setting a custom salt and `coinjoinsalt generate` will set a random hash as the new salt. | |
| - A new RPC command, `coinjoinsalt`, allows for manipulating a CoinJoin salt stored in a wallet. `coinjoinsalt get` will fetch an existing salt, `coinjoinsalt set` will allow setting a custom salt and `coinjoinsalt generate` will set a random hash as the new salt. It can be useful for correct recognition of transaction in case of recovery from backup or for debug purposes. |
knst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-utACK c52c9f8
could you add a functional test for coinjoinsalt? You may create new PR, if you feel it is out-of-scope of this PR, but it have to be done for sack of enabling rpc-coverage linter which is not running due to https://github.com/dashpay/dash-issues/issues/63
Will do in a follow-up |
PastaPastaPasta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 8e5e293d4ca36a9b88731648c7581d430173b05e
Co-authored-by: thephez <[email protected]> Co-authored-by: UdjinM6 <[email protected]>
knst
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-utACK c52c9f86df0873f880a6dbab24300c3b7bbf1cc4
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 5943c93
…etohd edge cases 69c37f4 rpc: make sure `upgradetohd` always has the passphrase for `UpgradeToHD` (Kittywhiskers Van Gogh) 619b640 wallet: unify HD chain generation in CWallet (Kittywhiskers Van Gogh) 163d318 wallet: unify HD chain generation in LegacyScriptPubKeyMan (Kittywhiskers Van Gogh) Pull request description: ## Motivation When filming demo footage for dashpay#6093, I realized that if I tried to create an encrypted blank legacy wallet and run `upgradetohd [mnemonic]`, the client would crash. ``` dash@b9c6631a824d:/src/dash$ ./src/qt/dash-qt QStandardPaths: XDG_RUNTIME_DIR not set, defaulting to '/tmp/runtime-dash' dash-qt: wallet/scriptpubkeyman.cpp:399: void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString &, const SecureString &, CKeyingMaterial): Assertion `res' failed. Posix Signal: Aborted No debug information available for stacktrace. You should add debug information and then run: dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5avkbxxg2lyebjwsz3omfwduicbmjxxe5dfmqaaa=== ``` The expected set of operations when performing privileged operations is to first use `walletpassphrase [passphrase] [time]` to unlock the wallet and then perform the privileged operation. This routine that applies for almost all privileged RPCs doesn't apply here, the unlock state of the wallet has no bearing on constructing an encrypted HD chain as it needs to be encrypted with the master key stored in the wallet, which in turn is encrypted with a key derived from the passphrase (i.e., `upgradetohd` imports **always** need the passphrase, if encrypted). You might have noticed that I used `upgradetohd [mnemonic]` instead of the correct syntax, `upgradetohd [mnemonic] "" [passphrase]` that is supposed to be used when supplying a mnemonic to an encrypted wallet, because when you run the former, you don't get told to enter the passphrase into the RPC command, you're told. ``` Error: Please enter the wallet passphrase with walletpassphrase first. ``` Which tells you to treat it like any other routine privileged operation and follow the routine as mentioned above. This is where insufficient validation starts rearing its head, we only validate the passphrase if we're supplied one even though we should be demanding one if the wallet is encrypted and it isn't supplied. We didn't supply a passphrase because we're following the normal routine, we unlocked the wallet so `EnsureWalletIsUnlocked()` is happy, so now the following happens. ``` upgradetohd() | Insufficient validation has allowed us to supply a blank passphrase | for an encrypted wallet |- CWallet::UpgradeToHD() |- CWallet::GenerateNewHDChainEncrypted() | We get our hands on vMasterKey by generating the key from our passphrase | and using it to unlock vCryptedMasterKey. | | There's one small problem, we don't know if the output of CCrypter::Decrypt | isn't just gibberish. Since we don't have a passphrase, whatever came from | CCrypter::SetKeyFromPassphrase isn't the decryption key, meaning, the | vMasterKey we just got is gibberish |- LegacyScriptPubKeyMan::GenerateNewCryptedHDChain() |- res = LegacyScriptPubKeyMan::EncryptHDChain() | |- EncryptSecret() | |- CCrypter::SetKey() | This is where everything unravels, the gibberish key's size doesn't | match WALLET_CRYPTO_KEY_SIZE, it's no good for encryption. We bail out. |- assert(res) We assume are inputs are safe so there's no real reason we should crash. Except our inputs aren't safe, so we crash. Welp! :c ``` This problem has existed for a while but didn't cause the client to crash, in v20.1.1 (1951298), trying to do the same thing would return you a vague error ``` Failed to generate encrypted HD wallet (code -4) ``` In the process of working on mitigating this crash, another edge case was discovered, where if the wallet was unlocked and an incorrect passphrase was provided to `upgradetohd`, the user would not receive any feedback that they entered the wrong passphrase and the client would similarly crash. ``` upgradetohd() | We've been supplied a passphrase, so we can try and validate it by | trying to unlock the wallet with it. If it fails, we know we got the | wrong passphrase. |- CWallet::Unlock() | | Before we bother unlocking the wallet, we should check if we're | | already unlocked, if we are, we can just say "unlock successful". | |- CWallet::IsLocked() | | Wallet is indeed unlocked. | |- return true; | The validation method we just tried to use has a bail-out mechanism | that we don't account for, the "unlock" succeded so I guess we have the | right passphrase. [...] (continue call chain as mentioned earlier) |- assert(res) Oh... ``` This pull request aims to resolve crashes caused by the above two edge cases. ## Additional Information As this PR was required me to add additional guardrails on `GenerateNewCryptedHDChain()` and `GenerateNewHDChainEncrypted()`, it was taken as an opportunity to resolve a TODO ([source](https://github.com/dashpay/dash/blob/9456d0761d8883cc293dffba11dacded517b5f8f/src/wallet/wallet.cpp#L5028-L5038)). The following mitigations have been implemented. * Validating `vMasterKey` size (any key not of `WALLET_CRYPTO_KEY_SIZE` size cannot be used for encryption and so, cannot be a valid key) * Validating `secureWalletPassphrase`'s presence to catch attempts at passing a blank value (an encrypted wallet cannot have a blank passphrase) * Using `Unlock()` to validate the correctness of `vMasterKey`. (the two other instances of iterating through `mapMasterKeys` use `Unlock()`, see [here](https://github.com/dashpay/dash/blob/1394c41c8d0afb8370726488a2888be30d238148/src/wallet/wallet.cpp#L5498-L5500) and [here](https://github.com/dashpay/dash/blob/1394c41c8d0afb8370726488a2888be30d238148/src/wallet/wallet.cpp#L429-L431)) * `Lock()`'ing the wallet before `Unlock()`'ing the wallet to avoid the `IsLocked()` bail-out condition and then restoring to the previous lock state afterwards. * Add an `IsCrypted()` check to see if `upgradetohd`'s `walletpassphrase` is allowed to be empty. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: knst: utACK 69c37f4 UdjinM6: utACK 69c37f4 PastaPastaPasta: utACK 69c37f4 Tree-SHA512: 4bda1f7155511447d6672bbaa22b909f5e2fc7efd1fd8ae1c61e0cdbbf3f6c28f6e8c1a8fe2a270fdedff7279322c93bf0f8e01890aff556fb17288ef6907b3e
|
@kwvg I just realized, there's no functional tests for rpc |
Motivation
CoinJoin utilizes a salt to randomize the number of rounds used in a mixing session (introduced in dash#3661). This was done to frustrate attempts at deobfuscating CoinJoin mixing transactions but also has the effect of deciding the mixing threshold at which a denomination is considered "sufficiently mixed", which in turn is tied this salt, that is in turn, tied to the wallet.
With wallets that utilized keypool generation, this was perfectly acceptable as each key was unrelated to the other and any meaningful attempts to backup/restore a wallet would entail backing up the entire wallet database, which includes the salt. Meaning, on restore, the wallet would show CoinJoin balances as reported earlier.
With the default activation of HD wallets in legacy wallets (and the introduction of descriptor wallets that construct HD wallets by default), addresses are deterministically generated and backups no longer require a backup of the wallet database wholesale.
Users who export their mnemonic and import them into a new wallet will find themselves with a different reported CoinJoin balance (see below).
cjsalt_preview.mp4
Demo
Based on
c00a3665cjsalt_demo.mp4
Checklist: