Merge cef87f7a48 into merged_master (Bitcoin PR #17290)

This one took a little bit of work. Basically everywhere that Andy's new code
didn't compile, I looked for a similarly shaped line in the pre-PR diff between
Bitcoin and Elements and adjusted it in the same way. Was pretty typical; all
CAmounts become CAmountMaps, etc.

I then spent about 7 hours chasing down a coin selection failure in the blocksign
functional tests, which ultimately turned out to be a Core issue. I added a
stopgap fix (commented, search for "stopgap" in wallet.cpp), and opened the issue
https://github.com/bitcoin/bitcoin/issues/20347

There was also a bug where we were not estimating change output sizes correctly
in case we had massive blinded outputs, which I had to fix to get the functional
tests to pass.

src/blind.h

@@ -17,6 +17,10 @@
 //! ELEMENTS:
 // 36-bit rangeproof size
 static const size_t DEFAULT_RANGEPROOF_SIZE = 2893;
+// 64-bit rangeproof size
+static const size_t MAX_RANGEPROOF_SIZE = 5126;
+// 3-input ASP size
+static const size_t DEFAULT_SURJECTIONPROOF_SIZE = 99;
 // 32 bytes of asset type, 32 bytes of asset blinding factor in sidechannel
 static const size_t SIDECHANNEL_MSG_SIZE = 64;
 

src/wallet/test/coinselector_tests.cpp

@@ -68,7 +68,7 @@ static void add_coin(const CAmount& nValue, int nInput, CoinSet& set)
     set.emplace(&wtx, nInput);
 }
 
-static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0)
+static void add_coin(CWallet& wallet, const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false)
 {
     balance += nValue;
     static int nextLockTime = 0;
@@ -76,12 +76,18 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa
     tx.nLockTime = nextLockTime++;        // so all transactions get different hashes
     tx.vout.resize(nInput + 1);
     tx.vout[nInput].nValue = nValue;
+    if (spendable) {
+        CTxDestination dest;
+        std::string error;
+        assert(wallet.GetNewDestination(OutputType::BECH32, "", dest, error));
+        tx.vout[nInput].scriptPubKey = GetScriptForDestination(dest);
+    }
     if (fIsFromMe) {
         // IsFromMe() returns (GetDebit() > 0), and GetDebit() is 0 if vin.empty(),
         // so stop vin being empty, and cache a non-zero Debit to fake out IsFromMe()
         tx.vin.resize(1);
     }
-    std::unique_ptr<CWalletTx> wtx = MakeUnique<CWalletTx>(&testWallet, MakeTransactionRef(std::move(tx)));
+    std::unique_ptr<CWalletTx> wtx = MakeUnique<CWalletTx>(&wallet, MakeTransactionRef(std::move(tx)));
     if (fIsFromMe)
     {
         CAmountMap map;
@@ -90,9 +96,13 @@ static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = fa
     }
     COutput output(wtx.get(), nInput, nAge, true /* spendable */, true /* solvable */, true /* safe */);
     vCoins.push_back(output);
-    testWallet.AddToWallet(*wtx.get());
+    wallet.AddToWallet(*wtx.get());
     wtxn.emplace_back(std::move(wtx));
 }
+static void add_coin(const CAmount& nValue, int nAge = 6*24, bool fIsFromMe = false, int nInput=0, bool spendable = false)
+{
+    add_coin(testWallet, nValue, nAge, fIsFromMe, nInput, spendable);
+}
 
 static void empty_wallet(void)
 {
@@ -267,20 +277,36 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
     vCoins.at(0).nInputBytes = 40; // Make sure that it has a negative effective value. The next check should assert if this somehow got through. Otherwise it will fail
     BOOST_CHECK(!SimpleSelectCoinsMinConf(testWallet,  1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used));
 
-    // Make sure that we aren't using BnB when there are preset inputs
+    // Test fees subtracted from output:
     empty_wallet();
-    add_coin(5 * CENT);
-    add_coin(3 * CENT);
-    add_coin(2 * CENT);
-    CCoinControl coin_control;
-    coin_control.fAllowOtherInputs = true;
-    coin_control.Select(COutPoint(vCoins.at(0).tx->GetHash(), vCoins.at(0).i));
-    CAmountMap mapTargetValue;
-    mapTargetValue[CAsset()] = 10 * CENT;
-    CAmountMap mapValueRet;
-    BOOST_CHECK(testWallet.SelectCoins(vCoins, mapTargetValue, setCoinsRet, mapValueRet, coin_control, coin_selection_params_bnb, bnb_used));
-    BOOST_CHECK(!bnb_used);
-    BOOST_CHECK(!coin_selection_params_bnb.use_bnb);
+    add_coin(1 * CENT);
+    vCoins.at(0).nInputBytes = 40;
+    BOOST_CHECK(!SimpleSelectCoinsMinConf(testWallet, 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used));
+    coin_selection_params_bnb.m_subtract_fee_outputs = true;
+    BOOST_CHECK(SimpleSelectCoinsMinConf(testWallet, 1 * CENT, filter_standard, GroupCoins(vCoins), setCoinsRet, nValueRet, coin_selection_params_bnb, bnb_used));
+    BOOST_CHECK_EQUAL(nValueRet, 1 * CENT);
+
+    // Make sure that can use BnB when there are preset inputs
+    empty_wallet();
+    {
+        std::unique_ptr<CWallet> wallet = MakeUnique<CWallet>(m_chain.get(), WalletLocation(), WalletDatabase::CreateMock());
+        bool firstRun;
+        wallet->LoadWallet(firstRun);
+        LOCK(wallet->cs_wallet);
+        add_coin(*wallet, 5 * CENT, 6 * 24, false, 0, true);
+        add_coin(*wallet, 3 * CENT, 6 * 24, false, 0, true);
+        add_coin(*wallet, 2 * CENT, 6 * 24, false, 0, true);
+        CCoinControl coin_control;
+        coin_control.fAllowOtherInputs = true;
+        coin_control.Select(COutPoint(vCoins.at(0).tx->GetHash(), vCoins.at(0).i));
+        coin_selection_params_bnb.effective_fee = CFeeRate(0);
+        CAmountMap mapTargetValue;
+        mapTargetValue[CAsset()] = 10 * CENT;
+        CAmountMap mapValueRet;
+        BOOST_CHECK(wallet->SelectCoins(vCoins, mapTargetValue, setCoinsRet, mapValueRet, coin_control, coin_selection_params_bnb, bnb_used));
+        BOOST_CHECK(bnb_used);
+        BOOST_CHECK(coin_selection_params_bnb.use_bnb);
+    }
 }
 
 BOOST_AUTO_TEST_CASE(knapsack_solver_test)

src/wallet/wallet.cpp

@@ -2330,7 +2330,11 @@ bool CWallet::SelectCoinsMinConf(const CAmountMap& mapTargetValue, const CoinEli
                 if (effective_value > 0) {
                     group.fee += coin.m_input_bytes < 0 ? 0 : coin_selection_params.effective_fee.GetFee(coin.m_input_bytes);
                     group.long_term_fee += coin.m_input_bytes < 0 ? 0 : long_term_feerate.GetFee(coin.m_input_bytes);
-                    group.effective_value += effective_value;
+                    if (coin_selection_params.m_subtract_fee_outputs) {
+                        group.effective_value += coin.value;
+                    } else {
+                        group.effective_value += effective_value;
+                    }
                     ++it;
                 } else {
                     it = group.Discard(coin);
@@ -2360,6 +2364,7 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
 {
     AssertLockHeld(cs_wallet); // mapWallet
     std::vector<COutput> vCoins(vAvailableCoins);
+    CAmountMap value_to_select = mapTargetValue;
 
     // coin control -> return all selected outputs (we want all selected to go into the transaction for sure)
     if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs)
@@ -2390,27 +2395,37 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
     coin_control.ListSelected(vPresetInputs);
     for (const COutPoint& outpoint : vPresetInputs)
     {
-        // For now, don't use BnB if preset inputs are selected. TODO: Enable this later
-        bnb_used = false;
-        coin_selection_params.use_bnb = false;
-
         std::map<uint256, CWalletTx>::const_iterator it = mapWallet.find(outpoint.hash);
         if (it != mapWallet.end())
         {
             const CWalletTx& wtx = it->second;
             // Clearly invalid input, fail
-            if (wtx.tx->vout.size() <= outpoint.n)
+            if (wtx.tx->vout.size() <= outpoint.n) {
+                bnb_used = false;
                 return false;
-
+            }
             // Just to calculate the marginal byte size
             CAmount amt = wtx.GetOutputValueOut(outpoint.n);
             if (amt < 0) {
                 continue;
             }
+            CInputCoin coin(&wtx, outpoint.n, wtx.GetSpendSize(outpoint.n, false));
             mapValueFromPresetInputs[wtx.GetOutputAsset(outpoint.n)] += amt;
-            setPresetCoins.insert(CInputCoin(&wtx, outpoint.n));
-        } else
+            if (coin.m_input_bytes <= 0) {
+                bnb_used = false;
+                return false; // Not solvable, can't estimate size for fee
+            }
+            coin.effective_value = coin.value - coin_selection_params.effective_fee.GetFee(coin.m_input_bytes);
+            if (coin_selection_params.use_bnb) {
+                value_to_select[coin.asset] -= coin.effective_value;
+            } else {
+                value_to_select[coin.asset] -= coin.value;
+            }
+            setPresetCoins.insert(coin);
+        } else {
+            bnb_used = false;
             return false; // TODO: Allow non-wallet inputs
+        }
     }
 
     // remove preset inputs from vCoins
@@ -2450,25 +2465,25 @@ bool CWallet::SelectCoins(const std::vector<COutput>& vAvailableCoins, const CAm
     bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS);
 
     // We will have to do coin selection on the difference between the target and the provided values.
-    // However, some inputs can be provided with assets that are not in the target, we need to make sure
-    // the map of the difference does not have negative values.
-    CAmountMap mapTargetMinusPreset = mapTargetValue - mapValueFromPresetInputs;
-    for (CAmountMap::const_iterator it = mapTargetMinusPreset.begin(); it != mapTargetMinusPreset.end();) {
+    // If value_to_select <= 0 for all asset types, we are done; but unlike in Bitcoin, this may be
+    // true for some assets whlie being false for others. So clear all the "completed" assets out
+    // of value_to_select before calling SelectCoinsMinConf.
+    for (CAmountMap::const_iterator it = value_to_select.begin(); it != value_to_select.end();) {
         if (it->second <= 0) {
-            it = mapTargetMinusPreset.erase(it);
+            it = value_to_select.erase(it);
         } else {
             ++it;
         }
     }
 
-    bool res = mapTargetValue <= mapValueFromPresetInputs ||
-        SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used) ||
-        SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(1, 1, 0), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used) ||
-        (m_spend_zero_conf_change && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) ||
-        (m_spend_zero_conf_change && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) ||
-        (m_spend_zero_conf_change && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) ||
-        (m_spend_zero_conf_change && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) ||
-        (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(mapTargetMinusPreset, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used));
+    bool res = value_to_select.empty() ||
+        SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 6, 0), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used) ||
+        SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(1, 1, 0), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used) ||
+        (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, 2), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) ||
+        (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::min((size_t)4, max_ancestors/3), std::min((size_t)4, max_descendants/3)), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) ||
+        (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors/2, max_descendants/2), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) ||
+        (m_spend_zero_conf_change && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, max_ancestors-1, max_descendants-1), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used)) ||
+        (m_spend_zero_conf_change && !fRejectLongChains && SelectCoinsMinConf(value_to_select, CoinEligibilityFilter(0, 1, std::numeric_limits<uint64_t>::max()), groups, setCoinsRet, mapValueRet, coin_selection_params, bnb_used));
 
     // because SelectCoinsMinConf clears the setCoinsRet, we now add the possible inputs to the coinset
     util::insert(setCoinsRet, setPresetCoins);
@@ -2905,8 +2920,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
             if (g_con_elementsmode) {
                 // Assume blinded output for coin selection purposes. Over-paying is ok!
                 change_prototype_txout.nAsset.vchCommitment.resize(33);
+                change_prototype_txout.nValue.vchCommitment.resize(33);
+                change_prototype_txout.nNonce.vchCommitment.resize(33);
                 coin_selection_params.change_output_size = GetSerializeSize(change_prototype_txout);
-                coin_selection_params.change_output_size += DEFAULT_RANGEPROOF_SIZE/WITNESS_SCALE_FACTOR;
+                coin_selection_params.change_output_size += (MAX_RANGEPROOF_SIZE + DEFAULT_SURJECTIONPROOF_SIZE + WITNESS_SCALE_FACTOR - 1)/WITNESS_SCALE_FACTOR;
             }
 
             CFeeRate discard_rate = GetDiscardRate(*this);
@@ -2922,7 +2939,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
 
             // BnB selector is the only selector used when this is true.
             // That should only happen on the first pass through the loop.
-            coin_selection_params.use_bnb = nSubtractFeeFromAmount == 0; // If we are doing subtract fee from recipient, then don't use BnB
+            coin_selection_params.use_bnb = true;
+            coin_selection_params.m_subtract_fee_outputs = nSubtractFeeFromAmount != 0; // If we are doing subtract fee from recipient, don't use effective values
+            //ELEMENTS: stopgap solution to https://github.com/bitcoin/bitcoin/issues/20347
+            bool one_more_try_20347 = false;
             // Start with no fee and loop until there is enough fee
             while (true)
             {
@@ -2949,8 +2969,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
                     mapValueToSelect[::policyAsset] += nFeeRet;
 
                 // vouts to the payees
-                coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size)
-                for (const CRecipient& recipient : vecSend)
+                if (!coin_selection_params.m_subtract_fee_outputs) {
+                    coin_selection_params.tx_noinputs_size = 11; // Static vsize overhead + outputs vsize. 4 nVersion, 4 nLocktime, 1 input count, 1 output count, 1 witness overhead (dummy, flag, stack size)
+                }
+                for (const auto& recipient : vecSend)
                 {
                     CTxOut txout(recipient.asset, recipient.nAmount, recipient.scriptPubKey);
                     txout.nNonce.vchCommitment = std::vector<unsigned char>(recipient.confidentiality_key.begin(), recipient.confidentiality_key.end());
@@ -2972,7 +2994,10 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
                         }
                     }
                     // Include the fee cost for outputs. Note this is only used for BnB right now
-                    coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
+                    if (!coin_selection_params.m_subtract_fee_outputs) {
+                        coin_selection_params.tx_noinputs_size += ::GetSerializeSize(txout, PROTOCOL_VERSION);
+                    }
+
                     // ELEMENTS: Core's logic isn't great here. We should be computing
                     // cost of making output + future spend. We're not as concerned
                     // about dust anyways, so let's focus upstream.
@@ -3299,7 +3324,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
                     }
                     break; // Done, enough fee included.
                 }
-                else if (!pick_new_inputs) {
+                else if (!pick_new_inputs && !one_more_try_20347) {
                     // This shouldn't happen, we should have had enough excess
                     // fee to pay for the new output and still meet nFeeNeeded
                     // Or we should have just subtracted fee from recipients and
@@ -3355,8 +3380,14 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
 
                 // If subtracting fee from recipients, we now know what fee we
                 // need to subtract, we have no reason to reselect inputs
+                // In case we used branch-and-bound, this could result in our transaction
+                // since increasing since we force-elided change on this iteration for bnb.
+                // In this case we turn on `one_more_try` so that `!pick_new_inputs` doesn't
+                // cause the loop to fail. This is a stopgap. See Core #20347.
+                one_more_try_20347 = false;
                 if (nSubtractFeeFromAmount > 0) {
                     pick_new_inputs = false;
+                    one_more_try_20347 = bnb_used;
                 }
 
                 // Include more fee and try again.

src/wallet/wallet.h

@@ -642,6 +642,8 @@ struct CoinSelectionParams
     size_t change_spend_size = 0;
     CFeeRate effective_fee = CFeeRate(0);
     size_t tx_noinputs_size = 0;
+    //! Indicate that we are subtracting the fee from outputs
+    bool m_subtract_fee_outputs = false;
 
     CoinSelectionParams(bool use_bnb, size_t change_output_size, size_t change_spend_size, CFeeRate effective_fee, size_t tx_noinputs_size) : use_bnb(use_bnb), change_output_size(change_output_size), change_spend_size(change_spend_size), effective_fee(effective_fee), tx_noinputs_size(tx_noinputs_size) {}
     CoinSelectionParams() {}