Skip to content

Conversation

@kwvg
Copy link
Collaborator

@kwvg kwvg commented Jul 3, 2024

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 c00a3665

cjsalt_demo.mp4

Checklist:

  • 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
  • I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

@kwvg kwvg changed the title feat: allow manipulation of CoinJoin salt using RPC, make coinjoin a composite command and remove inapplicable keypool exhaustion warnings feat: allow manipulation of CoinJoin salt using RPC, make coinjoin a composite command and remove inapplicable keypool exhaustion warning Jul 3, 2024
@kwvg kwvg added this to the 21 milestone Jul 3, 2024
@kwvg kwvg requested a review from UdjinM6 July 9, 2024 14:39
@github-actions
Copy link

github-actions bot commented Jul 9, 2024

This pull request has conflicts, please rebase.

@UdjinM6
Copy link

UdjinM6 commented Jul 10, 2024

Let's split it in 3 separate PRs:

  • cj salt manipulation
  • make coinjoin a composite command
  • fixing getcoinjoininfo for descriptor wallets

@kwvg kwvg modified the milestones: 21, 21.1 Jul 11, 2024
@kwvg kwvg force-pushed the cjsalt branch 2 times, most recently from 358f963 to c89f777 Compare July 11, 2024 10:38
@kwvg kwvg changed the title feat: allow manipulation of CoinJoin salt using RPC, make coinjoin a composite command and remove inapplicable keypool exhaustion warning feat: allow manipulation of CoinJoin salt using RPC Jul 11, 2024
Copy link

@UdjinM6 UdjinM6 left a 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

@kwvg kwvg marked this pull request as ready for review July 11, 2024 20:56
@kwvg kwvg requested review from PastaPastaPasta, UdjinM6 and knst July 11, 2024 20:58
Copy link
Collaborator

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

Copy link

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
UdjinM6 previously approved these changes Jul 11, 2024
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 019a3c070d50f97260cc1e2462c36274003e1b42

@kwvg kwvg requested a review from knst July 15, 2024 02:45
@github-actions
Copy link

This pull request has conflicts, please rebase.

@kwvg kwvg marked this pull request as ready for review July 19, 2024 17:29
@kwvg kwvg requested review from UdjinM6 and knst July 19, 2024 17:30
knst
knst previously approved these changes Jul 19, 2024
Copy link
Collaborator

@knst knst left a 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

Copy link
Collaborator

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?

Suggested change
- 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
knst previously approved these changes Jul 19, 2024
Copy link
Collaborator

@knst knst left a 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

@kwvg
Copy link
Collaborator Author

kwvg commented Jul 19, 2024

could you add a functional test for coinjoinsalt?

Will do in a follow-up

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 8e5e293d4ca36a9b88731648c7581d430173b05e

@kwvg kwvg dismissed stale reviews from PastaPastaPasta and knst via 5943c93 July 20, 2024 04:11
@kwvg kwvg requested review from PastaPastaPasta, UdjinM6 and knst July 20, 2024 04:12
Copy link
Collaborator

@knst knst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-utACK c52c9f86df0873f880a6dbab24300c3b7bbf1cc4

Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5943c93

@PastaPastaPasta PastaPastaPasta merged commit a1875db into dashpay:develop Jul 20, 2024
PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Jul 23, 2024
…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
@UdjinM6 UdjinM6 modified the milestones: 21.1, 21.2 Aug 8, 2024
@thephez thephez added the RPC Some notable changes to RPC params/behaviour/descriptions label Sep 17, 2024
@knst
Copy link
Collaborator

knst commented Sep 26, 2024

@kwvg I just realized, there's no functional tests for rpc coinjoinsalt. Our linter complains about it:

$ test/functional/test_runner.py -j20  --previous-releases --coverage --extended
.....
Uncovered RPC commands:
  - cleardiscouraged
  - coinjoinsalt <-------
  - debug
  - getblockheaders
  - getmerkleblocks
  - getpoolinfo
  - voteraw

See also https://github.com/dashpay/dash-issues/issues/63

@UdjinM6 UdjinM6 modified the milestones: 21.2, 22 Oct 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants