wallet: use silent_payments as change fallback OutputType

This will allow wallets with only silent payment descriptors to use silent payment outputs as a change

Author: Eunovo

src/wallet/spend.cpp

@@ -10,6 +10,7 @@
 #include <consensus/amount.h>
 #include <consensus/validation.h>
 #include <interfaces/chain.h>
+#include <key_io.h>
 #include <node/types.h>
 #include <numeric>
 #include <policy/policy.h>
@@ -1139,6 +1140,7 @@ std::optional<std::map<size_t, WitnessV1Taproot>> CreateSilentPaymentOutputs(
 static util::Result<CreatedTransactionResult> CreateTransactionInternal(
         CWallet& wallet,
         const std::vector<CRecipient>& vecSend,
+        const OutputType change_type,
         std::optional<unsigned int> change_pos,
         const CCoinControl& coin_control,
         bool sign) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
@@ -1166,7 +1168,6 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     coin_selection_params.tx_noinputs_size = 10 + GetSizeOfCompactSize(vecSend.size()); // bytes for output count
 
     CAmount recipients_sum = 0;
-    const OutputType change_type = wallet.TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : wallet.m_default_change_type, vecSend);
     ReserveDestination reservedest(&wallet, change_type);
     unsigned int outputs_to_subtract_fee_from = 0; // The number of outputs which we are subtracting the fee from
     for (const auto& recipient : vecSend) {
@@ -1302,26 +1303,29 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
            result.GetSelectedValue());
 
     std::vector<CRecipient> mutableVecSend = vecSend;
-    if (coin_control.m_silent_payment) {
+    bool fSilentPayment = coin_control.m_silent_payment || std::holds_alternative<V0SilentPaymentDestination>(change_dest);
+    if (fSilentPayment) {
         // Get the silent payment destinations, generate the scriptPubKeys,
         // and update vecSend with the generated scriptPubKeys
         std::map<size_t, V0SilentPaymentDestination> sp_dests;
+        size_t sp_change_index = 0;
         for (size_t i = 0; i < mutableVecSend.size(); ++i) {
             if (const auto* sp = std::get_if<V0SilentPaymentDestination>(&mutableVecSend.at(i).dest)) {
                 // Keep track of the index in vecSend
                 sp_dests[i] = *sp;
+                sp_change_index += 1;
             }
         }
-        if (const auto* change_sp = std::get_if<V0SilentPaymentDestination>(&change_dest)) {
+        if (const auto* sp_change = std::get_if<V0SilentPaymentDestination>(&change_dest)) {
             // Generate output for change too
-            sp_dests[mutableVecSend.size()] = *change_sp;
+            sp_dests[sp_change_index] = *sp_change;
         }
         const auto& silent_payment_tr_spks = CreateSilentPaymentOutputs(wallet, sp_dests, result.GetInputSet(), error);
         if (!silent_payment_tr_spks.has_value()) {
             return util::Error{error};
         }
         for (const auto& [out_idx, tr_dest] : *silent_payment_tr_spks) {
-            if (out_idx == mutableVecSend.size()) {
+            if (out_idx == sp_change_index) {
                 change_dest = tr_dest;
                 continue;
             }
@@ -1535,7 +1539,7 @@ static util::Result<CreatedTransactionResult> CreateTransactionInternal(
     // TODO: this feels a bit hacky, but given the current way we handle change and receiving addresses,
     // I think this is good enough. Ultimately, improving our change detection and how we handle the address
     // seems like a better time to revisit this.
-    if (coin_control.m_silent_payment && wallet.IsWalletFlagSet(WALLET_FLAG_SILENT_PAYMENTS)) {
+    if (fSilentPayment && wallet.IsWalletFlagSet(WALLET_FLAG_SILENT_PAYMENTS)) {
         // If our wallet supports receiving silent payments, check if this transaction is a self transfer
         std::map<COutPoint, Coin> spent_coins;
         for (const auto& utxo : result.GetInputSet()) {
@@ -1565,14 +1569,33 @@ util::Result<CreatedTransactionResult> CreateTransaction(
 
     LOCK(wallet.cs_wallet);
 
-    auto res = CreateTransactionInternal(wallet, vecSend, change_pos, coin_control, sign);
-    TRACEPOINT(coin_selection, normal_create_tx_internal,
+    wallet::CreatedTransactionResult txr_ungrouped;
+    OutputType change_type = wallet.TransactionChangeType(coin_control.m_change_type ? *coin_control.m_change_type : wallet.m_default_change_type, vecSend);
+    auto res1 = CreateTransactionInternal(wallet, vecSend, change_type, change_pos, coin_control, sign);
+    if (!res1) {
+        change_type = wallet.TransactionChangeType(
+            coin_control.m_change_type ? *coin_control.m_change_type : wallet.m_default_change_type,
+            vecSend, /* exclude_sp= */true);
+        auto res2 = CreateTransactionInternal(wallet, vecSend, change_type, change_pos, coin_control, false);
+
+        TRACEPOINT(coin_selection, normal_create_tx_internal,
+           wallet.GetName().c_str(),
+           bool(res2),
+           res2 ? res2->fee : 0,
+           res2 && res2->change_pos.has_value() ? int32_t(*res2->change_pos) : -1);
+        if (!res2) return res2;
+
+        txr_ungrouped = *res2;
+    } else {
+        TRACEPOINT(coin_selection, normal_create_tx_internal,
            wallet.GetName().c_str(),
-           bool(res),
-           res ? res->fee : 0,
-           res && res->change_pos.has_value() ? int32_t(*res->change_pos) : -1);
-    if (!res) return res;
-    const auto& txr_ungrouped = *res;
+           bool(res1),
+           res1->fee,
+           res1->change_pos.has_value() ? int32_t(*res1->change_pos) : -1);
+
+        txr_ungrouped = *res1;
+    }
+
     // try with avoidpartialspends unless it's enabled already
     if (txr_ungrouped.fee > 0 /* 0 means non-functional fee rate estimation */ && wallet.m_max_aps_fee > -1 && !coin_control.m_avoid_partial_spends) {
         TRACEPOINT(coin_selection, attempting_aps_create_tx, wallet.GetName().c_str());
@@ -1586,7 +1609,7 @@ util::Result<CreatedTransactionResult> CreateTransaction(
             ExtractDestination(txr_ungrouped.tx->vout[*txr_ungrouped.change_pos].scriptPubKey, tmp_cc.destChange);
         }
 
-        auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_pos, tmp_cc, sign);
+        auto txr_grouped = CreateTransactionInternal(wallet, vecSend, change_type, change_pos, tmp_cc, sign);
         // if fee of this alternative one is within the range of the max fee, we use this one
         const bool use_aps{txr_grouped.has_value() ? (txr_grouped->fee <= txr_ungrouped.fee + wallet.m_max_aps_fee) : false};
         TRACEPOINT(coin_selection, aps_create_tx_internal,
@@ -1601,7 +1624,7 @@ util::Result<CreatedTransactionResult> CreateTransaction(
             if (use_aps) return txr_grouped;
         }
     }
-    return res;
+    return txr_ungrouped;
 }
 
 util::Result<CreatedTransactionResult> FundTransaction(CWallet& wallet, const CMutableTransaction& tx, const std::vector<CRecipient>& vecSend, std::optional<unsigned int> change_pos, bool lockUnspents, CCoinControl coinControl)

src/wallet/spend.h

@@ -219,6 +219,8 @@ struct CreatedTransactionResult
 
     CreatedTransactionResult(CTransactionRef _tx, CAmount _fee, std::optional<unsigned int> _change_pos, const OutputType change_type, const FeeCalculation& _fee_calc)
         : tx(_tx), fee(_fee), fee_calc(_fee_calc), change_pos(_change_pos), change_type(change_type) {}
+
+    CreatedTransactionResult() = default;
 };
 
 /**

src/wallet/wallet.cpp

@@ -2237,7 +2237,7 @@ SigningResult CWallet::SignMessage(const std::string& message, const PKHash& pkh
     return SigningResult::PRIVATE_KEY_NOT_AVAILABLE;
 }
 
-OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend) const
+OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend, bool exclude_sp) const
 {
     // If -changetype is specified, always use that change type.
     if (change_type) {
@@ -2270,7 +2270,7 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang
     }
 
     const bool has_sp_spkman(GetScriptPubKeyMan(OutputType::SILENT_PAYMENTS, /*internal=*/true));
-    if (has_sp_spkman && (any_sp || any_tr)) {
+    if (has_sp_spkman && (any_sp || (any_tr && !exclude_sp))) {
         return OutputType::SILENT_PAYMENTS;
     }
     const bool has_bech32m_spkman(GetScriptPubKeyMan(OutputType::BECH32M, /*internal=*/true));
@@ -2295,6 +2295,11 @@ OutputType CWallet::TransactionChangeType(const std::optional<OutputType>& chang
         return OutputType::LEGACY;
     }
 
+    bool is_silent_payments_enabled{!IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)};
+    if (is_silent_payments_enabled && has_sp_spkman && !exclude_sp) {
+        // If silent payments are enabled, use the silent payments spkman for change
+        return OutputType::SILENT_PAYMENTS;
+    }
     if (has_bech32m_spkman) {
         return OutputType::BECH32M;
     }

src/wallet/wallet.h

@@ -661,7 +661,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     bool ShouldResend() const;
     void ResubmitWalletTransactions(bool relay, bool force);
 
-    OutputType TransactionChangeType(const std::optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend) const;
+    OutputType TransactionChangeType(const std::optional<OutputType>& change_type, const std::vector<CRecipient>& vecSend, bool exclude_sp = false) const;
 
     /** Fetch the inputs and sign with SIGHASH_ALL. */
     bool SignTransaction(CMutableTransaction& tx) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

test/functional/wallet_silentpayments_receiving.py

@@ -147,7 +147,29 @@ def do_nothing():
 
         self.log.info("Wallet persistence verified successfully")
 
-    def test_import_rescan(self):
+    def test_import_descriptors(self):
+        self.log.info("Check import silent payments descriptor into blank wallet")
+
+        self.nodes[0].createwallet(wallet_name="import_blank", blank=True, silent_payments=True)
+        wallet = self.nodes[0].get_wallet_rpc("import_blank")
+
+        descriptor = "sp([eea23daf/352h/0h/0h/1h/0]cRCGWnoELVHTr9oZWz1TUp7jmdBJPS3Kx8UCaHvfpxL6KiVSHH1A,[eea23daf/352h/0h/0h/0h/0]cUxxbQ67tepsEn3AUKSCWAKrvLwP6MPk55DAptcqxqDiXN6yKkNW)#q42ue5dg"
+        res = wallet.importdescriptors([{
+            "desc": descriptor,
+            "active": True,
+            "next_index": 0,
+            "timestamp": "now"
+        }])
+        assert_equal(res[0]["success"], True)
+
+        self.def_wallet.sendtoaddress(wallet.getnewaddress(address_type="silent-payments"), 10)
+        self.generate(self.nodes[0], 1)
+        assert_equal(wallet.getbalance(), 10)
+
+        wallet.send({self.def_wallet.getnewaddress(): 5})
+        self.generate(self.nodes[0], 1)
+        assert_approx(wallet.getbalance(), 5, 0.0001)
+
         self.log.info("Check import rescan works for silent payments")
 
         self.nodes[0].createwallet(wallet_name="alice", silent_payments=True)
@@ -298,7 +320,7 @@ def run_test(self):
         self.test_encrypting_unencrypted()
         self.test_basic()
         self.test_wallet_persistence()
-        self.test_import_rescan()
+        self.test_import_descriptors()
         self.test_createwallet_descriptor()
         self.test_backup_and_restore()
         self.test_getaddressinfo()