backport: merge bitcoin#23334, #21206, #24196, #24530, #24635, #24502, #24494, #24677, #24602, #24644, #24820, #25507, #25825, #29042, partial bitcoin#23725 (wallet backports: part 5)

configure.ac

@@ -1333,7 +1333,6 @@ if test "$enable_fuzz" = "yes"; then
   bitcoin_enable_qt=no
   bitcoin_enable_qt_test=no
   bitcoin_enable_qt_dbus=no
-  enable_wallet=no
   use_bench=no
   use_upnp=no
   use_natpmp=no

doc/release-notes-6685.md

@@ -0,0 +1,5 @@
+Notable Changes
+---------------
+
+- To help prevent fingerprinting transactions created by the Dash Core wallet, change output
+  amounts are now randomized.

doc/tracing.md

@@ -168,6 +168,49 @@ Arguments passed:
 4. Value of the coin as `int64`
 5. If the coin is a coinbase as `bool`
 
+### Context `coin_selection`
+
+#### Tracepoint `coin_selection:selected_coins`
+
+Is called when `SelectCoins` completes.
+
+Arguments passed:
+1. Wallet name as `pointer to C-style string`
+2. Coin selection algorithm name as `pointer to C-style string`
+3. Selection target value as `int64`
+4. Calculated waste metric of the solution as `int64`
+5. Total value of the selected inputs as `int64`
+
+#### Tracepoint `coin_selection:normal_create_tx_internal`
+
+Is called when the first `CreateTransactionInternal` completes.
+
+Arguments passed:
+1. Wallet name as `pointer to C-style string`
+2. Whether `CreateTransactionInternal` succeeded as `bool`
+3. The expected transaction fee as an `int64`
+4. The position of the change output as an `int32`
+
+#### Tracepoint `coin_selection:attempting_aps_create_tx`
+
+Is called when `CreateTransactionInternal` is called the second time for the optimistic
+Avoid Partial Spends selection attempt. This is used to determine whether the next
+tracepoints called are for the Avoid Partial Spends solution, or a different transaction.
+
+Arguments passed:
+1. Wallet name as `pointer to C-style string`
+
+#### Tracepoint `coin_selection:aps_create_tx_internal`
+
+Is called when the second `CreateTransactionInternal` with Avoid Partial Spends enabled completes.
+
+Arguments passed:
+1. Wallet name as `pointer to C-style string`
+2. Whether the Avoid Partial Spends solution will be used as `bool`
+3. Whether `CreateTransactionInternal` succeeded as` bool`
+4. The expected transaction fee as an `int64`
+5. The position of the change output as an `int32`
+
 ## Adding tracepoints to Dash Core
 
 To add a new tracepoint, `#include <util/trace.h>` in the compilation unit where

src/Makefile.am

@@ -364,6 +364,7 @@ BITCOIN_CORE_H = \
   util/message.h \
   util/moneystr.h \
   util/overflow.h \
+  util/overloaded.h \
   util/ranges.h \
   util/readwritefile.h \
   util/underlying.h \

src/Makefile.test.include

@@ -201,6 +201,7 @@ BITCOIN_TESTS += \
   wallet/test/wallet_tests.cpp \
   wallet/test/walletdb_tests.cpp \
   wallet/test/wallet_crypto_tests.cpp \
+  wallet/test/wallet_transaction_tests.cpp \
   wallet/test/coinselector_tests.cpp \
   wallet/test/init_tests.cpp \
   wallet/test/ismine_tests.cpp \
@@ -215,6 +216,13 @@ if USE_BDB
 BITCOIN_TESTS += wallet/test/db_tests.cpp
 endif
 
+FUZZ_WALLET_SRC = \
+ wallet/test/fuzz/coinselection.cpp
+
+if USE_SQLITE
+FUZZ_WALLET_SRC += \
+ wallet/test/fuzz/notifications.cpp
+endif # USE_SQLITE
 
 BITCOIN_TEST_SUITE += \
   wallet/test/util.cpp \
@@ -223,7 +231,7 @@ BITCOIN_TEST_SUITE += \
   wallet/test/wallet_test_fixture.h \
   wallet/test/init_test_fixture.cpp \
   wallet/test/init_test_fixture.h
-endif
+endif # ENABLE_WALLET
 
 test_test_dash_SOURCES = $(BITCOIN_TEST_SUITE) $(BITCOIN_TESTS) $(JSON_TEST_FILES) $(RAW_TEST_FILES)
 test_test_dash_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) $(TESTDEFS) $(EVENT_CFLAGS)
@@ -243,14 +251,13 @@ test_test_dash_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
 FUZZ_SUITE_LD_COMMON += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
 endif
 
-FUZZ_SUITE_LDFLAGS_COMMON = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) $(LDFLAGS_WRAP_EXCEPTIONS) $(PTHREAD_FLAGS)
-
 if ENABLE_FUZZ_BINARY
 test_fuzz_fuzz_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
 test_fuzz_fuzz_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 test_fuzz_fuzz_LDADD = $(FUZZ_SUITE_LD_COMMON)
-test_fuzz_fuzz_LDFLAGS = $(FUZZ_SUITE_LDFLAGS_COMMON)
+test_fuzz_fuzz_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS) $(LDFLAGS_WRAP_EXCEPTIONS) $(PTHREAD_FLAGS)
 test_fuzz_fuzz_SOURCES = \
+ $(FUZZ_WALLET_SRC) \
  test/fuzz/addition_overflow.cpp \
  test/fuzz/addrman.cpp \
  test/fuzz/asmap.cpp \

src/bench/coin_selection.cpp

@@ -18,7 +18,7 @@ static void addCoin(const CAmount& nValue, const CWallet& wallet, std::vector<st
     tx.nLockTime = nextLockTime++; // so all transactions get different hashes
     tx.vout.resize(1);
     tx.vout[0].nValue = nValue;
-    wtxs.push_back(std::make_unique<CWalletTx>(MakeTransactionRef(std::move(tx))));
+    wtxs.push_back(std::make_unique<CWalletTx>(MakeTransactionRef(std::move(tx)), TxStateInactive{}));
 }
 
 // Simple benchmark for wallet coin selection. Note that it maybe be necessary
@@ -45,19 +45,20 @@ static void CoinSelection(benchmark::Bench& bench)
     // Create coins
     std::vector<COutput> coins;
     for (const auto& wtx : wtxs) {
-        coins.emplace_back(COutPoint(wtx->GetHash(), 0), wtx->tx->vout.at(0), /*depth=*/ 6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx->GetTxTime(), /*from_me=*/ true);
+        coins.emplace_back(COutPoint(wtx->GetHash(), 0), wtx->tx->vout.at(0), /*depth=*/6 * 24, GetTxSpendSize(wallet, *wtx, 0), /*spendable=*/true, /*solvable=*/true, /*safe=*/true, wtx->GetTxTime(), /*from_me=*/true);
     }
     const CoinEligibilityFilter filter_standard(1, 6, 0);
     FastRandomContext rand{};
     const CoinSelectionParams coin_selection_params{
         rand,
-        /* change_output_size= */ 34,
-        /* change_spend_size= */ 148,
-        /* effective_feerate= */ CFeeRate(0),
-        /* long_term_feerate= */ CFeeRate(0),
-        /* discard_feerate= */ CFeeRate(0),
-        /* tx_noinputs_size= */ 0,
-        /* avoid_partial= */ false,
+        /*change_output_size=*/ 34,
+        /*change_spend_size=*/ 148,
+        /*min_change_target=*/ CHANGE_LOWER,
+        /*effective_feerate=*/ CFeeRate(0),
+        /*long_term_feerate=*/ CFeeRate(0),
+        /*discard_feerate=*/ CFeeRate(0),
+        /*tx_noinputs_size=*/ 0,
+        /*avoid_partial=*/ false,
     };
     bench.run([&] {
         auto result = AttemptSelection(wallet, 1003 * COIN, filter_standard, coins, coin_selection_params);

src/qt/coincontroldialog.cpp

@@ -539,11 +539,10 @@ void CoinControlDialog::updateLabels(CCoinControl& m_coin_control, WalletModel *
                 nChange -= nPayFee;
             }
 
-            // Never create dust outputs; if we would, just add the dust to the fee.
-            if (nChange > 0 && nChange < MIN_CHANGE)
-            {
+            if (nChange > 0) {
                 // Assumes a p2pkh script size
                 CTxOut txout(nChange, CScript() << std::vector<unsigned char>(24, 0));
+                // Never create dust outputs; if we would, just add the dust to the fee.
                 if (IsDust(txout, model->node().getDustRelayFee()))
                 {
                     nPayFee += nChange;

src/test/fuzz/fuzz.cpp

@@ -69,7 +69,7 @@ auto& FuzzTargets()
 
 void FuzzFrameworkRegisterTarget(std::string_view name, TypeTestOneInput target, FuzzTargetOptions opts)
 {
-    const auto it_ins{FuzzTargets().try_emplace(name, FuzzTarget /* temporary can be dropped in C++20 */ {std::move(target), std::move(opts)})};
+    const auto it_ins{FuzzTargets().try_emplace(name, FuzzTarget /* temporary can be dropped after clang-16 */ {std::move(target), std::move(opts)})};
     Assert(it_ins.second);
 }
 

src/util/overloaded.h

@@ -0,0 +1,22 @@
+// Copyright (c) 2021-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_UTIL_OVERLOADED_H
+#define BITCOIN_UTIL_OVERLOADED_H
+
+namespace util {
+//! Overloaded helper for std::visit. This helper and std::visit in general are
+//! useful to write code that switches on a variant type. Unlike if/else-if and
+//! switch/case statements, std::visit will trigger compile errors if there are
+//! unhandled cases.
+//!
+//! Implementation comes from and example usage can be found at
+//! https://en.cppreference.com/w/cpp/utility/variant/visit#Example
+template<class... Ts> struct Overloaded : Ts... { using Ts::operator()...; };
+
+//! Explicit deduction guide (not needed after clang-17)
+template<class... Ts> Overloaded(Ts...) -> Overloaded<Ts...>;
+} // namespace util
+
+#endif // BITCOIN_UTIL_OVERLOADED_H

src/util/types.h

@@ -5,6 +5,7 @@
 #ifndef BITCOIN_UTIL_TYPES_H
 #define BITCOIN_UTIL_TYPES_H
 
+// Not needed after C++23 (DR, https://cplusplus.github.io/CWG/issues/2518.html)
 template <class>
 inline constexpr bool ALWAYS_FALSE{false};
 

src/validation.cpp

@@ -1943,8 +1943,9 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
                 error("DisconnectBlock(): transaction and undo data inconsistent");
                 return DISCONNECT_FAILED;
             }
-            for (unsigned int j = tx.vin.size(); j-- > 0;) {
-                const COutPoint &out = tx.vin[j].prevout;
+            for (unsigned int j = tx.vin.size(); j > 0;) {
+                --j;
+                const COutPoint& out = tx.vin[j].prevout;
                 int undoHeight = txundo.vprevout[j].nHeight;
                 int res = ApplyTxInUndo(std::move(txundo.vprevout[j]), view, out);
                 if (res == DISCONNECT_FAILED) return DISCONNECT_FAILED;

src/wallet/coinselection.cpp

@@ -66,7 +66,7 @@ std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_poo
 {
     if (utxo_pool.empty()) return std::nullopt;
 
-    SelectionResult result(selection_target);
+    SelectionResult result(selection_target, SelectionAlgorithm::BNB);
     CAmount curr_value = 0;
 
     std::vector<bool> curr_selection; // select the utxo at this index
@@ -165,6 +165,8 @@ std::optional<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_poo
             result.AddInput(utxo_pool.at(i));
         }
     }
+    result.ComputeAndSetWaste(CAmount{0});
+    assert(best_waste == result.GetWaste());
 
     return result;
 }
@@ -173,7 +175,7 @@ std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& ut
 {
     if (utxo_pool.empty()) return std::nullopt;
 
-    SelectionResult result(target_value);
+    SelectionResult result(target_value, SelectionAlgorithm::SRD);
 
     std::vector<size_t> indexes;
     indexes.resize(utxo_pool.size());
@@ -193,11 +195,24 @@ std::optional<SelectionResult> SelectCoinsSRD(const std::vector<OutputGroup>& ut
     return std::nullopt;
 }
 
-static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::vector<OutputGroup>& groups, const CAmount& nTotalLower, const CAmount& nTargetValue,
+/** Find a subset of the OutputGroups that is at least as large as, but as close as possible to, the
+ * target amount; solve subset sum.
+ * param@[in]   groups          OutputGroups to choose from, sorted by value in descending order.
+ * param@[in]   nTotalLower     Total (effective) value of the UTXOs in groups.
+ * param@[in]   nTargetValue    Subset sum target, not including change.
+ * param@[out]  vfBest          Boolean vector representing the subset chosen that is closest to
+ *                              nTargetValue, with indices corresponding to groups. If the ith
+ *                              entry is true, that means the ith group in groups was selected.
+ * param@[out]  nBest           Total amount of subset chosen that is closest to nTargetValue.
+ * param@[in]   iterations      Maximum number of tries.
+ */
+static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::vector<OutputGroup>& groups,
+                                  const CAmount& nTotalLower, const CAmount& nTargetValue,
                                   std::vector<char>& vfBest, CAmount& nBest, int iterations = 1000)
 {
     std::vector<char> vfIncluded;
 
+    // Worst case "best" approximation is just all of the groups.
     vfBest.assign(groups.size(), true);
     nBest = nTotalLower;
     int nBestInputCount = 0;
@@ -226,6 +241,8 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v
                     if (nTotal >= nTargetValue)
                     {
                         fReachedTarget = true;
+                        // If the total is between nTargetValue and nBest, it's our new best
+                        // approximation.
                         if (nTotal < nBest || (nTotal == nBest && nTotalInputCount < nBestInputCount))
                         {
                             nBest = nTotal;
@@ -264,29 +281,31 @@ bool less_then_denom (const OutputGroup& group1, const OutputGroup& group2)
     return (!found1 && found2);
 }
 
-std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue, FastRandomContext& rng, bool fFullyMixedOnly, CAmount maxTxFee)
+std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, const CAmount& nTargetValue,
+                                              CAmount change_target, FastRandomContext& rng, bool fFullyMixedOnly, CAmount maxTxFee)
 {
     if (groups.empty()) return std::nullopt;
 
-    SelectionResult result(nTargetValue);
+    SelectionResult result(nTargetValue, SelectionAlgorithm::KNAPSACK);
 
     // List of values less than target
     std::optional<OutputGroup> lowest_larger;
+    // Groups with selection amount smaller than the target and any change we might produce.
+    // Don't include groups larger than this, because they will only cause us to overshoot.
     std::vector<OutputGroup> applicable_groups;
     CAmount nTotalLower = 0;
 
     Shuffle(groups.begin(), groups.end(), rng);
 
     int tryDenomStart = 0;
-    CAmount nMinChange = MIN_CHANGE;
 
     if (fFullyMixedOnly) {
         // larger denoms first
         std::sort(groups.rbegin(), groups.rend(), CompareByPriority());
         // we actually want denoms only, so let's skip "non-denom only" step
         tryDenomStart = 1;
         // no change is allowed
-        nMinChange = 0;
+        change_target = 0;
     } else {
         // move denoms down on the list
         // try not to use denominated coins when not needed, save denoms for coinjoin
@@ -305,7 +324,7 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
             if (group.GetSelectionAmount() == nTargetValue) {
                 result.AddInput(group);
                 return result;
-            } else if (group.GetSelectionAmount() < nTargetValue + nMinChange) {
+            } else if (group.GetSelectionAmount() < nTargetValue + change_target) {
                 applicable_groups.push_back(group);
                 nTotalLower += group.GetSelectionAmount();
             } else if (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount()) {
@@ -348,14 +367,14 @@ std::optional<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups,
     CAmount nBest;
 
     ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue, vfBest, nBest);
-    if (nBest != nTargetValue && nMinChange != 0 && nTotalLower >= nTargetValue + nMinChange) {
-        ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue + nMinChange, vfBest, nBest);
+    if (nBest != nTargetValue && change_target != 0 && nTotalLower >= nTargetValue + change_target) {
+        ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue + change_target, vfBest, nBest);
     }
 
     // If we have a bigger coin and (either the stochastic approximation didn't find a good solution,
     //                                   or the next bigger coin is closer), return the bigger coin
     if (lowest_larger &&
-        ((nBest != nTargetValue && nBest < nTargetValue + nMinChange) || lowest_larger->GetSelectionAmount() <= nBest)) {
+        ((nBest != nTargetValue && nBest < nTargetValue + change_target) || lowest_larger->GetSelectionAmount() <= nBest)) {
         result.AddInput(*lowest_larger);
     } else {
         std::string log_message{"Coin selection best subset: "};
@@ -453,6 +472,17 @@ CAmount GetSelectionWaste(const std::set<COutput>& inputs, CAmount change_cost,
     return waste;
 }
 
+CAmount GenerateChangeTarget(CAmount payment_value, FastRandomContext& rng)
+{
+    if (payment_value <= CHANGE_LOWER / 2) {
+        return CHANGE_LOWER;
+    } else {
+        // random value between 50ksat and min (payment_value * 2, 1milsat)
+        const auto upper_bound = std::min(payment_value * 2, CHANGE_UPPER);
+        return rng.randrange(upper_bound - CHANGE_LOWER) + CHANGE_LOWER;
+    }
+}
+
 void SelectionResult::ComputeAndSetWaste(CAmount change_cost)
 {
     m_waste = GetSelectionWaste(m_selected_inputs, change_cost, m_target, m_use_effective);
@@ -504,3 +534,16 @@ std::string COutput::ToString() const
 {
     return strprintf("COutput(%s, %d, %d) [%s]", outpoint.hash.ToString(), outpoint.n, depth, FormatMoney(txout.nValue));
 }
+
+std::string GetAlgorithmName(const SelectionAlgorithm algo)
+{
+    switch (algo)
+    {
+    case SelectionAlgorithm::BNB: return "bnb";
+    case SelectionAlgorithm::KNAPSACK: return "knapsack";
+    case SelectionAlgorithm::SRD: return "srd";
+    case SelectionAlgorithm::MANUAL: return "manual";
+    // No default case to allow for compiler to warn
+    }
+    assert(false);
+}