From e292ea499bb3e30339a3aaa30e90fe232f202c79 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 29 Feb 2024 12:43:52 +0100 Subject: [PATCH] [wallet, rpc]: add `max_tx_weight` to tx funding options This allows a transaction's weight to be bound under a certain weight if possible and desired. This can be beneficial for future RBF attempts, or whenever a more restricted spend topology is desired. Co-authored-by: Greg Sanders --- src/rpc/client.cpp | 3 ++ src/wallet/coincontrol.h | 2 ++ src/wallet/coinselection.cpp | 31 +++++++++++++++----- src/wallet/coinselection.h | 8 +++-- src/wallet/rpc/spend.cpp | 18 ++++++++++++ src/wallet/spend.cpp | 8 +++-- src/wallet/test/fuzz/coinselection.cpp | 11 +++++-- test/functional/rpc_psbt.py | 13 +++++++- test/functional/test_framework/blocktools.py | 1 + 9 files changed, 79 insertions(+), 16 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index b8dc148eaea2d0..17b5ff24cb8987 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -146,6 +146,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "fundrawtransaction", 1, "conf_target"}, { "fundrawtransaction", 1, "replaceable"}, { "fundrawtransaction", 1, "solving_data"}, + { "fundrawtransaction", 1, "max_tx_weight"}, { "fundrawtransaction", 2, "iswitness" }, { "walletcreatefundedpsbt", 0, "inputs" }, { "walletcreatefundedpsbt", 1, "outputs" }, @@ -164,6 +165,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "walletcreatefundedpsbt", 3, "conf_target"}, { "walletcreatefundedpsbt", 3, "replaceable"}, { "walletcreatefundedpsbt", 3, "solving_data"}, + { "walletcreatefundedpsbt", 3, "max_tx_weight"}, { "walletcreatefundedpsbt", 4, "bip32derivs" }, { "walletprocesspsbt", 1, "sign" }, { "walletprocesspsbt", 3, "bip32derivs" }, @@ -208,6 +210,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "send", 4, "conf_target"}, { "send", 4, "replaceable"}, { "send", 4, "solving_data"}, + { "send", 4, "max_tx_weight"}, { "sendall", 0, "recipients" }, { "sendall", 1, "conf_target" }, { "sendall", 3, "fee_rate"}, diff --git a/src/wallet/coincontrol.h b/src/wallet/coincontrol.h index b2f813383dc5a9..d36314312ad1f4 100644 --- a/src/wallet/coincontrol.h +++ b/src/wallet/coincontrol.h @@ -115,6 +115,8 @@ class CCoinControl std::optional m_locktime; //! Version std::optional m_version; + //! Caps weight of resulting tx + std::optional m_max_tx_weight{std::nullopt}; CCoinControl(); diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index e32a12e80f7907..2f3bb2f1949cca 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -597,11 +597,12 @@ util::Result SelectCoinsSRD(const std::vector& utx * 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. + * paramp[in] max_selection_weight The maximum allowed weight for a selection result to be valid. * param@[in] iterations Maximum number of tries. */ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::vector& groups, const CAmount& nTotalLower, const CAmount& nTargetValue, - std::vector& vfBest, CAmount& nBest, int iterations = 1000) + std::vector& vfBest, CAmount& nBest, int max_selection_weight, int iterations = 1000) { std::vector vfIncluded; @@ -613,6 +614,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v { vfIncluded.assign(groups.size(), false); CAmount nTotal = 0; + int selected_coins_weight{0}; bool fReachedTarget = false; for (int nPass = 0; nPass < 2 && !fReachedTarget; nPass++) { @@ -627,9 +629,9 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v if (nPass == 0 ? insecure_rand.randbool() : !vfIncluded[i]) { nTotal += groups[i].GetSelectionAmount(); + selected_coins_weight += groups[i].m_weight; vfIncluded[i] = true; - if (nTotal >= nTargetValue) - { + if (nTotal >= nTargetValue && selected_coins_weight <= max_selection_weight) { fReachedTarget = true; // If the total is between nTargetValue and nBest, it's our new best // approximation. @@ -639,6 +641,7 @@ static void ApproximateBestSubset(FastRandomContext& insecure_rand, const std::v vfBest = vfIncluded; } nTotal -= groups[i].GetSelectionAmount(); + selected_coins_weight -= groups[i].m_weight; vfIncluded[i] = false; } } @@ -652,6 +655,7 @@ util::Result KnapsackSolver(std::vector& groups, c { SelectionResult result(nTargetValue, SelectionAlgorithm::KNAPSACK); + bool max_weight_exceeded{false}; // List of values less than target std::optional lowest_larger; // Groups with selection amount smaller than the target and any change we might produce. @@ -662,6 +666,10 @@ util::Result KnapsackSolver(std::vector& groups, c Shuffle(groups.begin(), groups.end(), rng); for (const OutputGroup& group : groups) { + if (group.m_weight > max_selection_weight) { + max_weight_exceeded = true; + continue; + } if (group.GetSelectionAmount() == nTargetValue) { result.AddInput(group); return result; @@ -677,11 +685,18 @@ util::Result KnapsackSolver(std::vector& groups, c for (const auto& group : applicable_groups) { result.AddInput(group); } - return result; + if (result.GetWeight() <= max_selection_weight) return result; + else max_weight_exceeded = true; + + // Try something else + result.Clear(); } if (nTotalLower < nTargetValue) { - if (!lowest_larger) return util::Error(); + if (!lowest_larger) { + if (max_weight_exceeded) return ErrorMaxWeightExceeded(); + return util::Error(); + } result.AddInput(*lowest_larger); return result; } @@ -691,9 +706,9 @@ util::Result KnapsackSolver(std::vector& groups, c std::vector vfBest; CAmount nBest; - ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue, vfBest, nBest); + ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue, vfBest, nBest, max_selection_weight); if (nBest != nTargetValue && nTotalLower >= nTargetValue + change_target) { - ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue + change_target, vfBest, nBest); + ApproximateBestSubset(rng, applicable_groups, nTotalLower, nTargetValue + change_target, vfBest, nBest, max_selection_weight); } // If we have a bigger coin and (either the stochastic approximation didn't find a good solution, @@ -728,7 +743,7 @@ util::Result KnapsackSolver(std::vector& groups, c LogPrint(BCLog::SELECTCOINS, "%stotal %s\n", log_message, FormatMoney(nBest)); } } - + Assume(result.GetWeight() <= max_selection_weight); return result; } diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index d77333fd0855ff..3cd9833a01cb99 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -174,10 +174,13 @@ struct CoinSelectionParams { * 1) Received from other wallets, 2) replacing other txs, 3) that have been replaced. */ bool m_include_unsafe_inputs = false; + /** The maximum weight for this transaction. */ + std::optional m_max_tx_weight{std::nullopt}; CoinSelectionParams(FastRandomContext& rng_fast, int change_output_size, int change_spend_size, CAmount min_change_target, CFeeRate effective_feerate, - CFeeRate long_term_feerate, CFeeRate discard_feerate, int tx_noinputs_size, bool avoid_partial) + CFeeRate long_term_feerate, CFeeRate discard_feerate, int tx_noinputs_size, bool avoid_partial, + std::optional max_tx_weight = std::nullopt) : rng_fast{rng_fast}, change_output_size(change_output_size), change_spend_size(change_spend_size), @@ -186,7 +189,8 @@ struct CoinSelectionParams { m_long_term_feerate(long_term_feerate), m_discard_feerate(discard_feerate), tx_noinputs_size(tx_noinputs_size), - m_avoid_partial_spends(avoid_partial) + m_avoid_partial_spends(avoid_partial), + m_max_tx_weight(max_tx_weight) { } CoinSelectionParams(FastRandomContext& rng_fast) diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index 6060f017ce9944..b3cf6a893eb160 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -535,6 +535,7 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact {"minconf", UniValueType(UniValue::VNUM)}, {"maxconf", UniValueType(UniValue::VNUM)}, {"input_weights", UniValueType(UniValue::VARR)}, + {"max_tx_weight", UniValueType(UniValue::VNUM)}, }, true, true); @@ -702,6 +703,14 @@ CreatedTransactionResult FundTransaction(CWallet& wallet, const CMutableTransact } } + if (options.exists("max_tx_weight")) { + coinControl.m_max_tx_weight = options["max_tx_weight"].getInt(); + const int minimum_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR; + if (coinControl.m_max_tx_weight < minimum_tx_weight || coinControl.m_max_tx_weight > MAX_STANDARD_TX_WEIGHT) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid parameter, max tx weight must be between %d and %d", minimum_tx_weight, MAX_STANDARD_TX_WEIGHT)); + } + } + if (recipients.empty()) throw JSONRPCError(RPC_INVALID_PARAMETER, "TX must have at least one output"); @@ -787,6 +796,8 @@ RPCHelpMan fundrawtransaction() }, }, }, + {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n" + "Transaction building will fail if this can not be satisfied."}, }, FundTxDoc()), RPCArgOptions{ @@ -1241,6 +1252,8 @@ RPCHelpMan send() {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, }, }, + {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n" + "Transaction building will fail if this can not be satisfied."}, }, FundTxDoc()), RPCArgOptions{.oneline_description="options"}}, @@ -1288,6 +1301,9 @@ RPCHelpMan send() // Automatically select coins, unless at least one is manually selected. Can // be overridden by options.add_inputs. coin_control.m_allow_other_inputs = rawTx.vin.size() == 0; + if (options.exists("max_tx_weight")) { + coin_control.m_max_tx_weight = options["max_tx_weight"].getInt(); + } SetOptionsInputWeights(options["inputs"], options); // Clear tx.vout since it is not meant to be used now that we are passing outputs directly. // This sets us up for a future PR to completely remove tx from the function signature in favor of passing inputs directly @@ -1690,6 +1706,8 @@ RPCHelpMan walletcreatefundedpsbt() {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."}, }, }, + {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n" + "Transaction building will fail if this can not be satisfied."}, }, FundTxDoc()), RPCArgOptions{.oneline_description="options"}}, diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 7a1a1209cb80b1..dfc17f6a7726cc 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -692,7 +692,9 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co }; // Maximum allowed weight for selected coins. - int max_selection_weight = MAX_STANDARD_TX_WEIGHT - (coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR); + int max_transaction_weight = coin_selection_params.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT); + int tx_weight_no_input = coin_selection_params.tx_noinputs_size * WITNESS_SCALE_FACTOR; + int max_selection_weight = std::max((max_transaction_weight - tx_weight_no_input), 0); // SFFO frequently causes issues in the context of changeless input sets: skip BnB when SFFO is active if (!coin_selection_params.m_subtract_fee_outputs) { @@ -702,7 +704,8 @@ util::Result ChooseSelectionResult(interfaces::Chain& chain, co } // Deduct change weight because remaining Coin Selection algorithms can create change output - max_selection_weight -= (coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR); + int change_outputs_weight = coin_selection_params.change_output_size * WITNESS_SCALE_FACTOR; + max_selection_weight = std::max((max_selection_weight - change_outputs_weight), 0); // The knapsack solver has some legacy behavior where it will spend dust outputs. We retain this behavior, so don't filter for positive only here. if (auto knapsack_result{KnapsackSolver(groups.mixed_group, nTargetValue, coin_selection_params.m_min_change_target, coin_selection_params.rng_fast, max_selection_weight)}) { @@ -993,6 +996,7 @@ static util::Result CreateTransactionInternal( CoinSelectionParams coin_selection_params{rng_fast}; // Parameters for coin selection, init with dummy coin_selection_params.m_avoid_partial_spends = coin_control.m_avoid_partial_spends; coin_selection_params.m_include_unsafe_inputs = coin_control.m_include_unsafe_inputs; + coin_selection_params.m_max_tx_weight = coin_control.m_max_tx_weight.value_or(MAX_STANDARD_TX_WEIGHT); // Set the long term feerate estimate to the wallet's consolidate feerate coin_selection_params.m_long_term_feerate = wallet.m_consolidate_feerate; diff --git a/src/wallet/test/fuzz/coinselection.cpp b/src/wallet/test/fuzz/coinselection.cpp index 0aa5dedfe31343..c6a3be1bdd3de0 100644 --- a/src/wallet/test/fuzz/coinselection.cpp +++ b/src/wallet/test/fuzz/coinselection.cpp @@ -256,29 +256,34 @@ FUZZ_TARGET(coinselection) (void)group.EligibleForSpending(filter); } + int max_selection_weight = fuzzed_data_provider.ConsumeIntegralInRange(0, std::numeric_limits::max()); + // Run coinselection algorithms auto result_bnb = coin_params.m_subtract_fee_outputs ? util::Error{Untranslated("BnB disabled when SFFO is enabled")} : - SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, MAX_STANDARD_TX_WEIGHT); + SelectCoinsBnB(group_pos, target, coin_params.m_cost_of_change, max_selection_weight); if (result_bnb) { assert(result_bnb->GetChange(coin_params.min_viable_change, coin_params.m_change_fee) == 0); assert(result_bnb->GetSelectedValue() >= target); + assert(result_bnb->GetWeight() <= max_selection_weight); (void)result_bnb->GetShuffledInputVector(); (void)result_bnb->GetInputSet(); } - auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, MAX_STANDARD_TX_WEIGHT); + auto result_srd = SelectCoinsSRD(group_pos, target, coin_params.m_change_fee, fast_random_context, max_selection_weight); if (result_srd) { assert(result_srd->GetSelectedValue() >= target); assert(result_srd->GetChange(CHANGE_LOWER, coin_params.m_change_fee) > 0); // Demonstrate that SRD creates change of at least CHANGE_LOWER + assert(result_srd->GetWeight() <= max_selection_weight); result_srd->ComputeAndSetWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_srd->GetShuffledInputVector(); (void)result_srd->GetInputSet(); } CAmount change_target{GenerateChangeTarget(target, coin_params.m_change_fee, fast_random_context)}; - auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, MAX_STANDARD_TX_WEIGHT); + auto result_knapsack = KnapsackSolver(group_all, target, change_target, fast_random_context, max_selection_weight); if (result_knapsack) { assert(result_knapsack->GetSelectedValue() >= target); + assert(result_knapsack->GetWeight() <= max_selection_weight); result_knapsack->ComputeAndSetWaste(coin_params.min_viable_change, coin_params.m_cost_of_change, coin_params.m_change_fee); (void)result_knapsack->GetShuffledInputVector(); (void)result_knapsack->GetInputSet(); diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 016aa3ba119bcd..a076b2dc76e0ae 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -8,6 +8,7 @@ from itertools import product from random import randbytes +from test_framework.blocktools import MAX_STANDARD_TX_WEIGHT from test_framework.descriptors import descsum_create from test_framework.key import H_POINT from test_framework.messages import ( @@ -32,6 +33,7 @@ PSBT_OUT_TAP_TREE, ) from test_framework.script import CScript, OP_TRUE +from test_framework.script_util import MIN_STANDARD_TX_NONWITNESS_SIZE from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_approx, @@ -187,6 +189,16 @@ def run_test(self): # Create and fund a raw tx for sending 10 BTC psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt'] + self.log.info("Test that max_tx_weight option is sanity checked and fails when expected") + dest_arg = [{self.nodes[0].getnewaddress(): 1}] + min_tx_weight = MIN_STANDARD_TX_NONWITNESS_SIZE * WITNESS_SCALE_FACTOR + assert_raises_rpc_error(-8, f"Invalid parameter, max tx weight must be between {min_tx_weight} and {MAX_STANDARD_TX_WEIGHT}", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": -1}) + assert_raises_rpc_error(-8, f"Invalid parameter, max tx weight must be between {min_tx_weight} and {MAX_STANDARD_TX_WEIGHT}", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": 400001}) + assert_raises_rpc_error(-8, f"Invalid parameter, max tx weight must be between {min_tx_weight} and {MAX_STANDARD_TX_WEIGHT}", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": 0}) + + self.log.info("Test that a funded PSBT is always faithful to max_tx_weight option") + assert_raises_rpc_error(-4, "The inputs size exceeds the maximum weight", self.nodes[0].walletcreatefundedpsbt, [], dest_arg, 0, {"max_tx_weight": min_tx_weight}) + self.nodes[0].walletcreatefundedpsbt(outputs=dest_arg, locktime=0) # If inputs are specified, do not automatically add more: utxo1 = self.nodes[0].listunspent()[0] assert_raises_rpc_error(-4, "The preselected coins total amount does not cover the transaction target. " @@ -994,6 +1006,5 @@ def test_psbt_input_keys(psbt_input, keys): self.log.info("Test descriptorprocesspsbt raises if an invalid sighashtype is passed") assert_raises_rpc_error(-8, "all is not a valid sighash parameter.", self.nodes[2].descriptorprocesspsbt, psbt, [descriptor], sighashtype="all") - if __name__ == '__main__': PSBTTest().main() diff --git a/test/functional/test_framework/blocktools.py b/test/functional/test_framework/blocktools.py index cfd923bab37913..fa9293b508bd2c 100644 --- a/test/functional/test_framework/blocktools.py +++ b/test/functional/test_framework/blocktools.py @@ -48,6 +48,7 @@ WITNESS_SCALE_FACTOR = 4 MAX_BLOCK_SIGOPS = 20000 MAX_BLOCK_SIGOPS_WEIGHT = MAX_BLOCK_SIGOPS * WITNESS_SCALE_FACTOR +MAX_STANDARD_TX_WEIGHT = 400000 # Genesis block time (regtest) TIME_GENESIS_BLOCK = 1296688602