Merge #6093: feat: allow manipulation of CoinJoin salt using RPC

5943c93bdd feat: introduce `coinjoinsalt` RPC to allow manipulating per-wallet salt (Kittywhiskers Van Gogh)
4fa3d396f4 wallet: refactor `InitCoinJoinSalt()` to allow for setting a custom salt (Kittywhiskers Van Gogh)

Pull request description:

  ## Motivation

  CoinJoin utilizes a salt to randomize the number of rounds used in a mixing session (introduced in [dash#3661](https://github.com/dashpay/dash/pull/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).

  https://github.com/dashpay/dash/assets/63189531/dccf1b17-55af-423d-8c36-adea6163e060

  ## Demo

  **Based on [`c00a3665`](c00a366578)**

  https://github.com/dashpay/dash/assets/63189531/c60c10e3-e414-46af-a64e-60605a4e6d07

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

ACKs for top commit:
  UdjinM6:
    ACK 5943c93bdd

Tree-SHA512: 8e5bdd18ecce4c14683fbeb1812b0b683a5a5b38405653655c3a14b1ab32a22b244b198087876cd53ca12447c2027432afe4259ef3043be7fddf640911d998f0

doc/release-notes-6093.md

@@ -0,0 +1,4 @@
+New functionality
+-----------
+
+- 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.

src/rpc/coinjoin.cpp

@@ -181,6 +181,193 @@ static RPCHelpMan coinjoin_stop()
 },
     };
 }
+
+static RPCHelpMan coinjoinsalt()
+{
+    return RPCHelpMan{"coinjoinsalt",
+        "\nAvailable commands:\n"
+        "  generate  - Generate new CoinJoin salt\n"
+        "  get       - Fetch existing CoinJoin salt\n"
+        "  set       - Set new CoinJoin salt\n",
+        {
+            {"command", RPCArg::Type::STR, RPCArg::Optional::NO, "The command to execute"},
+        },
+        RPCResults{},
+        RPCExamples{""},
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
+    throw JSONRPCError(RPC_INVALID_PARAMETER, "Must be a valid command");
+},
+    };
+}
+
+static RPCHelpMan coinjoinsalt_generate()
+{
+    return RPCHelpMan{"coinjoinsalt generate",
+        "\nGenerate new CoinJoin salt and store it in the wallet database\n"
+        "Cannot generate new salt if CoinJoin mixing is in process or wallet has private keys disabled.\n",
+        {
+            {"overwrite", RPCArg::Type::BOOL, /* default */ "false", "Generate new salt even if there is an existing salt and/or there is CoinJoin balance"},
+        },
+        RPCResult{
+            RPCResult::Type::BOOL, "", "Status of CoinJoin salt generation and commitment"
+        },
+        RPCExamples{
+            HelpExampleCli("coinjoinsalt generate", "")
+          + HelpExampleRpc("coinjoinsalt generate", "")
+        },
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
+    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
+    if (!wallet) return NullUniValue;
+
+    const auto str_wallet = wallet->GetName();
+    if (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+        throw JSONRPCError(RPC_INVALID_REQUEST,
+                           strprintf("Wallet \"%s\" has private keys disabled, cannot perform CoinJoin!", str_wallet));
+    }
+
+    bool enable_overwrite{false}; // Default value
+    if (!request.params[0].isNull()) {
+        enable_overwrite = ParseBoolV(request.params[0], "overwrite");
+    }
+
+    if (!enable_overwrite && !wallet->GetCoinJoinSalt().IsNull()) {
+        throw JSONRPCError(RPC_INVALID_REQUEST,
+                           strprintf("Wallet \"%s\" already has set CoinJoin salt!", str_wallet));
+    }
+
+    const NodeContext& node = EnsureAnyNodeContext(request.context);
+    if (node.coinjoin_loader != nullptr) {
+        auto cj_clientman = node.coinjoin_loader->walletman().Get(wallet->GetName());
+        if (cj_clientman != nullptr && cj_clientman->IsMixing()) {
+            throw JSONRPCError(RPC_WALLET_ERROR,
+                               strprintf("Wallet \"%s\" is currently mixing, cannot change salt!", str_wallet));
+        }
+    }
+
+    const auto wallet_balance{wallet->GetBalance()};
+    const bool has_balance{(wallet_balance.m_anonymized
+                          + wallet_balance.m_denominated_trusted
+                          + wallet_balance.m_denominated_untrusted_pending) > 0};
+    if (!enable_overwrite && has_balance) {
+        throw JSONRPCError(RPC_WALLET_ERROR,
+                           strprintf("Wallet \"%s\" has CoinJoin balance, cannot continue!", str_wallet));
+    }
+
+    if (!wallet->SetCoinJoinSalt(GetRandHash())) {
+        throw JSONRPCError(RPC_INVALID_REQUEST,
+                           strprintf("Unable to set new CoinJoin salt for wallet \"%s\"!", str_wallet));
+    }
+
+    wallet->ClearCoinJoinRoundsCache();
+
+    return true;
+},
+    };
+}
+
+static RPCHelpMan coinjoinsalt_get()
+{
+    return RPCHelpMan{"coinjoinsalt get",
+        "\nFetch existing CoinJoin salt\n"
+        "Cannot fetch salt if wallet has private keys disabled.\n",
+        {},
+        RPCResult{
+            RPCResult::Type::STR_HEX, "", "CoinJoin salt"
+        },
+        RPCExamples{
+            HelpExampleCli("coinjoinsalt get", "")
+          + HelpExampleRpc("coinjoinsalt get", "")
+        },
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
+    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
+    if (!wallet) return NullUniValue;
+
+    const auto str_wallet = wallet->GetName();
+    if (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+        throw JSONRPCError(RPC_INVALID_REQUEST,
+                           strprintf("Wallet \"%s\" has private keys disabled, cannot perform CoinJoin!", str_wallet));
+    }
+
+    const auto salt{wallet->GetCoinJoinSalt()};
+    if (salt.IsNull()) {
+        throw JSONRPCError(RPC_WALLET_ERROR,
+                           strprintf("Wallet \"%s\" has no CoinJoin salt!", str_wallet));
+    }
+    return salt.GetHex();
+},
+    };
+}
+
+static RPCHelpMan coinjoinsalt_set()
+{
+    return RPCHelpMan{"coinjoinsalt set",
+        "\nSet new CoinJoin salt\n"
+        "Cannot set salt if CoinJoin mixing is in process or wallet has private keys disabled.\n"
+        "Will overwrite existing salt. The presence of a CoinJoin balance will cause the wallet to rescan.\n",
+        {
+            {"salt", RPCArg::Type::STR, RPCArg::Optional::NO, "Desired CoinJoin salt value for the wallet"},
+            {"overwrite", RPCArg::Type::BOOL, /* default */ "false", "Overwrite salt even if CoinJoin balance present"},
+        },
+        RPCResult{
+            RPCResult::Type::BOOL, "", "Status of CoinJoin salt change request"
+        },
+        RPCExamples{
+            HelpExampleCli("coinjoinsalt set", "f4184fc596403b9d638783cf57adfe4c75c605f6356fbc91338530e9831e9e16")
+          + HelpExampleRpc("coinjoinsalt set", "f4184fc596403b9d638783cf57adfe4c75c605f6356fbc91338530e9831e9e16")
+        },
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
+    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
+    if (!wallet) return NullUniValue;
+
+    const auto salt{ParseHashV(request.params[0], "salt")};
+    if (salt == uint256::ZERO) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid CoinJoin salt value");
+    }
+
+    bool enable_overwrite{false}; // Default value
+    if (!request.params[1].isNull()) {
+        enable_overwrite = ParseBoolV(request.params[1], "overwrite");
+    }
+
+    const auto str_wallet = wallet->GetName();
+    if (wallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
+        throw JSONRPCError(RPC_INVALID_REQUEST,
+                           strprintf("Wallet \"%s\" has private keys disabled, cannot perform CoinJoin!", str_wallet));
+    }
+
+    const NodeContext& node = EnsureAnyNodeContext(request.context);
+    if (node.coinjoin_loader != nullptr) {
+        auto cj_clientman = node.coinjoin_loader->walletman().Get(wallet->GetName());
+        if (cj_clientman != nullptr && cj_clientman->IsMixing()) {
+            throw JSONRPCError(RPC_WALLET_ERROR,
+                               strprintf("Wallet \"%s\" is currently mixing, cannot change salt!", str_wallet));
+        }
+    }
+
+    const auto wallet_balance{wallet->GetBalance()};
+    const bool has_balance{(wallet_balance.m_anonymized
+                          + wallet_balance.m_denominated_trusted
+                          + wallet_balance.m_denominated_untrusted_pending) > 0};
+    if (has_balance && !enable_overwrite) {
+        throw JSONRPCError(RPC_WALLET_ERROR,
+                           strprintf("Wallet \"%s\" has CoinJoin balance, cannot continue!", str_wallet));
+    }
+
+    if (!wallet->SetCoinJoinSalt(salt)) {
+        throw JSONRPCError(RPC_WALLET_ERROR,
+                           strprintf("Unable to set new CoinJoin salt for wallet \"%s\"!", str_wallet));
+    }
+
+    wallet->ClearCoinJoinRoundsCache();
+
+    return true;
+},
+    };
+}
 #endif // ENABLE_WALLET
 
 static RPCHelpMan getpoolinfo()
@@ -295,6 +482,10 @@ static const CRPCCommand commands[] =
     { "dash",                &coinjoin_reset,         },
     { "dash",                &coinjoin_start,         },
     { "dash",                &coinjoin_stop,          },
+    { "dash",                &coinjoinsalt,           },
+    { "dash",                &coinjoinsalt_generate,  },
+    { "dash",                &coinjoinsalt_get,       },
+    { "dash",                &coinjoinsalt_set,       },
 #endif // ENABLE_WALLET
 };
 // clang-format on

src/wallet/wallet.cpp

@@ -1448,6 +1448,15 @@ int CWallet::GetCappedOutpointCoinJoinRounds(const COutPoint& outpoint) const
     return realCoinJoinRounds > CCoinJoinClientOptions::GetRounds() ? CCoinJoinClientOptions::GetRounds() : realCoinJoinRounds;
 }
 
+void CWallet::ClearCoinJoinRoundsCache()
+{
+    LOCK(cs_wallet);
+    mapOutpointRoundsCache.clear();
+    MarkDirty();
+    // Notify UI
+    NotifyTransactionChanged(uint256::ONE, CT_UPDATED);
+}
+
 bool CWallet::IsDenominated(const COutPoint& outpoint) const
 {
     LOCK(cs_wallet);
@@ -2757,21 +2766,34 @@ const CTxOut& CWallet::FindNonChangeParentOutput(const CTransaction& tx, int out
     return ptx->vout[n];
 }
 
-void CWallet::InitCoinJoinSalt()
+const uint256& CWallet::GetCoinJoinSalt()
+{
+    if (nCoinJoinSalt.IsNull()) {
+        InitCJSaltFromDb();
+    }
+    return nCoinJoinSalt;
+}
+
+void CWallet::InitCJSaltFromDb()
 {
-    // Avoid fetching it multiple times
     assert(nCoinJoinSalt.IsNull());
 
     WalletBatch batch(GetDatabase());
     if (!batch.ReadCoinJoinSalt(nCoinJoinSalt) && batch.ReadCoinJoinSalt(nCoinJoinSalt, true)) {
+        // Migrate salt stored with legacy key
         batch.WriteCoinJoinSalt(nCoinJoinSalt);
     }
+}
 
-    while (nCoinJoinSalt.IsNull()) {
-        // We never generated/saved it
-        nCoinJoinSalt = GetRandHash();
-        batch.WriteCoinJoinSalt(nCoinJoinSalt);
+bool CWallet::SetCoinJoinSalt(const uint256& cj_salt)
+{
+    WalletBatch batch(GetDatabase());
+    // Only store new salt in CWallet if database write is successful
+    if (batch.WriteCoinJoinSalt(cj_salt)) {
+        nCoinJoinSalt = cj_salt;
+        return true;
     }
+    return false;
 }
 
 struct CompareByPriority
@@ -3942,11 +3964,14 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
         }
     }
 
-    InitCoinJoinSalt();
-
     if (nLoadWalletRet != DBErrors::LOAD_OK)
         return nLoadWalletRet;
 
+    /* If the CoinJoin salt is not set, try to set a new random hash as the salt */
+    if (GetCoinJoinSalt().IsNull() && !SetCoinJoinSalt(GetRandHash())) {
+        return DBErrors::LOAD_FAIL;
+    }
+
     return DBErrors::LOAD_OK;
 }
 

src/wallet/wallet.h

@@ -747,7 +747,7 @@ private:
     void AddToSpends(const uint256& wtxid) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     std::set<COutPoint> setWalletUTXO;
-    mutable std::map<COutPoint, int> mapOutpointRoundsCache;
+    mutable std::map<COutPoint, int> mapOutpointRoundsCache GUARDED_BY(cs_wallet);
 
     /**
      * Add a transaction to the wallet, or update it.  pIndex and posInBlock should
@@ -817,16 +817,16 @@ private:
      */
     uint256 m_last_block_processed GUARDED_BY(cs_wallet);
 
-    /** Pulled from wallet DB ("ps_salt") and used when mixing a random number of rounds.
+    /** Pulled from wallet DB ("cj_salt") and used when mixing a random number of rounds.
      *  This salt is needed to prevent an attacker from learning how many extra times
      *  the input was mixed based only on information in the blockchain.
      */
     uint256 nCoinJoinSalt;
 
     /**
-     * Fetches CoinJoin salt from database or generates and saves a new one if no salt was found in the db
+     * Populates nCoinJoinSalt with value from database (and migrates salt stored with legacy key).
      */
-    void InitCoinJoinSalt();
+    void InitCJSaltFromDb();
 
     /** Height of last block processed is used by wallet to know depth of transactions
      * without relying on Chain interface beyond asynchronous updates. For safety, we
@@ -872,6 +872,19 @@ public:
      */
     const std::string& GetName() const { return m_name; }
 
+    /**
+     * Get an existing CoinJoin salt. Will attempt to read database (and migrate legacy salts) if
+     * nCoinJoinSalt is empty but will skip database read if nCoinJoinSalt is populated.
+     **/
+    const uint256& GetCoinJoinSalt();
+
+    /**
+     * Write a new CoinJoin salt. This will directly write the new salt value into the wallet database.
+     * Ensuring that undesirable behaviour like overwriting the salt of a wallet that already uses CoinJoin
+     * is the responsibility of the caller.
+     **/
+    bool SetCoinJoinSalt(const uint256& cj_salt);
+
     // Map from governance object hash to governance object, they are added by gobject_prepare.
     std::map<uint256, Governance::Object> m_gobjects;
 
@@ -982,6 +995,8 @@ public:
     int GetRealOutpointCoinJoinRounds(const COutPoint& outpoint, int nRounds = 0) const;
     // respect current settings
     int GetCappedOutpointCoinJoinRounds(const COutPoint& outpoint) const;
+    // drop the internal cache to let Get...Rounds recalculate CJ balance from scratch and notify UI
+    void ClearCoinJoinRoundsCache();
 
     bool IsDenominated(const COutPoint& outpoint) const;
     bool IsFullyMixed(const COutPoint& outpoint) const;