This allows a transaction’s weight to be bound under a certain weight if possible an desired. This can be beneficial for future RBF attempts, or whenever a more restricted spend topology is desired.
See https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/11?u=instagibbs for more discussion.
Seeking concept ACK and maybe some help on approach for testing.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | murchandamus |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
FundTransaction refactor by josibake)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
1489@@ -1472,6 +1490,18 @@ RPCHelpMan sendall()
1490 const CAmount fee_from_size{fee_rate.GetFee(tx_size.vsize)};
1491 const CAmount effective_value{total_input_value - fee_from_size};
1492
1493+ // Cap tx weight based on estimated size if requested
1494+ if (options.exists("max_tx_weight")) {
1495+ const auto max_tx_weight = options["max_tx_weight"].getInt<int>();
1496+ if (max_tx_weight < 0 || max_tx_weight > MAX_STANDARD_TX_WEIGHT) {
1497+ throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Invalid parameter, max tx weight must be between 0 and 400000"));
MAX_STANDARD_TX_WEIGHT
336@@ -335,12 +337,18 @@ util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, c
337 for (const auto& group : applicable_groups) {
338 result.AddInput(group);
339 }
340+ // We may still fail if max is low
341+ if (result.GetWeight() > max_weight) return ErrorMaxWeightExceeded();
321@@ -322,6 +322,8 @@ util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, c
322 for (const OutputGroup& group : groups) {
323 if (group.GetSelectionAmount() == nTargetValue) {
324 result.AddInput(group);
325+ // We may still fail if max is low
326+ if (result.GetWeight() > max_weight) return ErrorMaxWeightExceeded();
Instead, how about:
0- if (group.GetSelectionAmount() == nTargetValue) {
1+ if (group.GetSelectionAmount() == nTargetValue && group.m_weight <= max_weight) {
336@@ -335,12 +337,18 @@ util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, c
337 for (const auto& group : applicable_groups) {
338 result.AddInput(group);
339 }
340+ // We may still fail if max is low
In line 331:
0- } else if (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount()) {
1+ } else if (group.m_weight <= max_weight && (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount())) {
1489@@ -1472,6 +1490,18 @@ RPCHelpMan sendall()
1490 const CAmount fee_from_size{fee_rate.GetFee(tx_size.vsize)};
1491 const CAmount effective_value{total_input_value - fee_from_size};
1492
1493+ // Cap tx weight based on estimated size if requested
1494+ if (options.exists("max_tx_weight")) {
1495+ const auto max_tx_weight = options["max_tx_weight"].getInt<int>();
1496+ if (max_tx_weight < 0 || max_tx_weight > MAX_STANDARD_TX_WEIGHT) {
366@@ -359,6 +367,9 @@ util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, c
367 if (lowest_larger &&
368 ((nBest != nTargetValue && nBest < nTargetValue + change_target) || lowest_larger->GetSelectionAmount() <= nBest)) {
369 result.AddInput(*lowest_larger);
370+
I would suggest that you pass the max_weight as an additional parameter to ApproximateBestSubset(…) and keep track of the current weight (e.g. nTotalWeight) in parallel to nTotal and only designate a new nBest and vfBest if it’s not just better but also a permitted weight:
0- if (nTotal < nBest)
1+ if (nTotal < nBest && nTotalWeight <= max_weight)
for extra credit you could also deselect not just after exceeding the target but also when you exceed max_weight
Continuing…
Got some more suggestions how you can give Knapsack a bit of a fighting chance, although we should get rid of Knapsack instead of improving it.
Also, Knapsack initializes its result with returning the entire UTXO pool, so you might want need to change that in case that’s more than max_weight to either be empty or fail before returning.
73@@ -74,7 +74,7 @@ struct {
74 static const size_t TOTAL_TRIES = 100000;
75
76 util::Result<SelectionResult> SelectCoinsBnB(std::vector<OutputGroup>& utxo_pool, const CAmount& selection_target, const CAmount& cost_of_change,
77- int max_weight)
78+ int max_input_weight)
max_input_weight is a much better name. Thanks!
372 if (lowest_larger &&
373 ((nBest != nTargetValue && nBest < nTargetValue + change_target) || lowest_larger->GetSelectionAmount() <= nBest)) {
374 result.AddInput(*lowest_larger);
375+
376+ // We may still fail if max is low
377+ if (result.GetWeight() > max_input_weight) return ErrorMaxWeightExceeded();
if block since we only set lowest_larger when it’s weight is less than the max_input_weight.
387 // Return closest UTXO above target
388 result.Clear();
389 result.AddInput(*lowest_larger);
390+
391+ // We may still fail if max is low
392+ if (result.GetWeight() > max_input_weight) return ErrorMaxWeightExceeded();
I don’t think this condition can be hit.
AFAICT, there is only be a single scenario in which vfBest can contain an input selection that is in excess of the max_input_weight: ApproximateBestSubset initializes vfBest with all of applicable_groups without checking the weight. If it then never finds a better input selection throughout the exploration, it would be possible that it returns an overweight vfBest. Since lowest_larger is only set when it adheres to the weight limit, we either have replaced vfBest with an adequate lowest_larger at this point, or have already returned ErrorMaxWeightExceeded.
ACK 069c1652ed22b5e22537f63946cedd6c68487b22
Thanks for cleanly distinguishing max_tx_weight and max_input_weight!
This allows a transaction's weight to be bound under
a certain weight if possible an desired. This
can be beneficial for future RBF attempts, or
whenever a more restricted spend topology is
desired.
MAX_STANDARD_TX_WEIGHT coins and not actually rejecting them as they should have previously, causing fuzz failure. Invariants in test should work now.
111@@ -112,22 +112,25 @@ FUZZ_TARGET(coinselection)
112 std::vector<OutputGroup> group_all;
113 GroupCoins(fuzzed_data_provider, utxo_pool, coin_params, /*positive_only=*/false, group_all);
114
115+ // Calculate upperbound of given input weight, allow all to be selected
116+ int total_input_weight{0};
max_input_weight a fuzzed value.
assert that checks whether the fuzzed value is greater than or equal to the result’s weight.
345+ result.Clear();
346 }
347
348 if (nTotalLower < nTargetValue) {
349- if (!lowest_larger) return util::Error();
350+ if (!lowest_larger) return util::Error{Untranslated(strprintf("Only %d coins to cover target value of %d", nTotalLower, nTargetValue))};
hmmm, this change to give a useful error message seems to break tests; I start getting failures in rpc_psbt.py
Can I get some assistance on how this stuff is supposed to bubble up @murchandamus
Is the breakage not an actual issue: https://cirrus-ci.com/task/6124189116006400?logs=ci#L3274
0test_framework.authproxy.JSONRPCException: Only 4999872180 coins to cover target value of 5100000840 (-4)
util::Error() is used. Everywhere actual error strings are always provided.
The empty util::Error() is used to retrieve a “no solution found for the provided inputs set”. If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.
The goal of this distinction is to differentiate between errors that should be bubbled up and coin selection algorithms’ partial solutions. Remember that we are running each coin selection algorithm up to 7 times, extending the input set on every round. The “no solution found” return isn’t definitive; it varies depending on the algorithm and the filtered coin set. That’s why other errors, like the max weight exceeded, are stored and take precedence if no solution is found after running all selection rounds.
I haven’t gone deeper over this PR yet but the message you are adding here doesn’t seems like an error. It is just describing the “no solution found for the provided input set” transient situation.
The empty util::Error() is used to retrieve a “no solution found for the provided inputs set”. If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.
Ok. So then this is just a (confusing) misuse of the Util:: API, where the wallet code is shoehorning non-error handling into an error handler.
The empty util::Error() is used to retrieve a “no solution found for the provided inputs set”. If you specify an error message, then the wallet coin selection procedure treats it as a real error and bubble it up when no other selection algorithm succeeded.
Ok. So then this is just a (confusing) misuse of the
Util::API, where the wallet code is shoehorning non-error handling into an error handler.
Ideally, we should retrieve more info within the “no solution found” return path and not just the string message. I was waiting until we expand the result class functionality to accept a generic error return type. E.g. having util::Result<M, E> would let us do something like util::Result<SelectionResult, SelectionError>, where SelectionError would contain the string msg and a severity level int value. Which would help us differentiate the different outcomes and bubble up the highest severity error (if any). I actually left a comment for this in the code.
I haven’t gone deeper over this PR yet but the message you are adding here doesn’t seems like an error. It is just describing the “no solution found for the provided input set” transient situation.
Are you saying it’s not confusing to give insufficient funds message on failure to stay within the weight limits given? I guess that’s fine for now?
I haven’t gone deeper over this PR yet but the message you are adding here doesn’t seems like an error. It is just describing the “no solution found for the provided input set” transient situation.
Are you saying it’s not confusing to give insufficient funds message on failure to stay within the weight limits given? I guess that’s fine for now?
Not really. I was only referring to this line in question. Which, in a first glance, isn’t retrieving an exceeded weight limit error. Still, give me a day to review the PR in-detail and will join the efforts here.
still waiting on some feedback on this point before moving forward with the PR @instagibbs, see https://github.com/furszy/bitcoin-core/commit/b2d3ac32cb51a1f350b5a4005e6da39d712b8fa9. It makes the process consistent, returning the same “weight exceeded” error message when such scenario occurs.
Still, I would like to revisit this PR further after you tackle #29264 (review) and most other comments.
807@@ -799,6 +808,8 @@ RPCHelpMan fundrawtransaction()
808 },
809 },
810 },
811+ {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n"
812+ "Transaction selection will fail if this can not be satisfied."},
I think you meant either “Transaction building” or “Coin selection” here:
0 "Transaction building will fail if this can not be satisfied."},
0 "Coin selection will fail if this can not be satisfied."},
1249@@ -1239,6 +1250,8 @@ RPCHelpMan send()
1250 {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."},
1251 },
1252 },
1253+ {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n"
1254+ "Transaction selection will fail if this can not be satisfied."},
1352@@ -1337,6 +1353,8 @@ RPCHelpMan sendall()
1353 {"send_max", RPCArg::Type::BOOL, RPCArg::Default{false}, "When true, only use UTXOs that can pay for their own fees to maximize the output amount. When 'false' (default), no UTXO is left behind. send_max is incompatible with providing specific inputs."},
1354 {"minconf", RPCArg::Type::NUM, RPCArg::Default{0}, "Require inputs with at least this many confirmations."},
1355 {"maxconf", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "Require inputs with at most this many confirmations."},
1356+ {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n"
1357+ "Transaction selection will fail if this can not be satisfied."},
1707@@ -1679,6 +1708,8 @@ RPCHelpMan walletcreatefundedpsbt()
1708 {"vout_index", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The zero-based output index, before a change output is added."},
1709 },
1710 },
1711+ {"max_tx_weight", RPCArg::Type::NUM, RPCArg::Default{MAX_STANDARD_TX_WEIGHT}, "The maximum acceptable transaction weight.\n"
1712+ "Transaction selection will fail if this can not be satisfied."},
199+
200+ sendall_dest = [self.nodes[0].getnewaddress()]
201+ max_send_psbt_result = self.nodes[0].sendall(sendall_dest, options={"psbt": True})
202+ max_send_weight = self.nodes[0].decoderawtransaction(max_send_psbt_result["hex"])['weight']
203+ assert_raises_rpc_error(-4, "Requested max tx weight exceeded:", self.nodes[0].sendall, sendall_dest, options={"max_tx_weight": max_send_weight - 1, "psbt": True})
204+ self.nodes[0].sendall(sendall_dest, options={"max_tx_weight": max_send_weight + 4, "psbt": True}) # off by 4 sometimes?
My first thought would be that we are perhaps not counting the empty witness stack of a non-witness input? :thinking:
Is this a blocker that needs to be addressed first?
r value in the signature and therefore one of the two values is a byte shorter. But our default address type would be at least wrapped segwit, so that would only explain a weight difference of 1. I don’t have a good idea at the moment. @achow101: Do you perhaps have an idea why this is happening?
I verified that this behavior happened, intermittently I get the weight of the transaction off by 4 bytes. I spent sometime debugging but did not figure why yet.
Going by this suggestion #29264 (review) the test is now gone in the new PR #29523 , I believe the issue is unrelated.
Should I open an issue for this?
🐙 This pull request conflicts with the target branch and needs rebase.
322@@ -320,13 +323,13 @@ util::Result<SelectionResult> KnapsackSolver(std::vector<OutputGroup>& groups, c
323 Shuffle(groups.begin(), groups.end(), rng);
324
325 for (const OutputGroup& group : groups) {
326- if (group.GetSelectionAmount() == nTargetValue) {
suggestion, add
0if (group.m_weight <= max_input_weight) continue;
on top of the for loop instead of checking within each condition.
Currently, you add groups that are above weight to applicable_groups
I think you mean
0if (group.m_weight > max_input_weight) continue;
This way we jusk skip group whose weight is larger than max_input_weight and avoid checking the weight limit for all the conditional branches below.
348 if (nTotalLower < nTargetValue) {
349- if (!lowest_larger) return util::Error();
350+ if (!lowest_larger) return util::Error{Untranslated(strprintf("Only %d coins to cover target value of %d", nTotalLower, nTargetValue))};
351 result.AddInput(*lowest_larger);
352- return result;
353+ if (result.GetWeight() <= max_input_weight) return result;
nit: lowest_larger can’t be above max_input_weight. You already checked it above. And in other similar cases below you don’t check it second time.
I prefer if we filter above weight inputs once at the top of the function.
1497+ throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Invalid parameter, max tx weight must be between 0 %d", MAX_STANDARD_TX_WEIGHT));
1498+ }
1499+ if (max_tx_weight < tx_size.weight) {
1500+ throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Requested max tx weight exceeded: %d vs %s", options["max_tx_weight"].getInt<int>(), tx_size.weight));
1501+ }
1502+ }
sendall() doesn’t make use of the internal coin selection procedures, wouldn’t make more sense to return the weight in the command result and let the user decide whether to broadcast the tx based on it?
115@@ -115,6 +116,8 @@ class CCoinControl
116 std::optional<uint32_t> m_locktime;
117 //! Version
118 std::optional<uint32_t> m_version;
119+ //! Caps weight of resulting tx
120+ int m_max_tx_weight{MAX_STANDARD_TX_WEIGHT};
policy/policy.h dependency, which comes with other dependencies (script/interpreter.h, script/solver.h, etc..), would suggest to make this field optional. Treating std::nullopt as MAX_STANDARD_TX_WEIGHT internally. This is what we already do for other default values.
182
183 CoinSelectionParams(FastRandomContext& rng_fast, size_t change_output_size, size_t change_spend_size,
184 CAmount min_change_target, CFeeRate effective_feerate,
185- CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial)
186+ CFeeRate long_term_feerate, CFeeRate discard_feerate, size_t tx_noinputs_size, bool avoid_partial,
187+ int32_t max_tx_weight=MAX_STANDARD_TX_WEIGHT)
policy.h dependency generates some noise on me. Would suggest to not default initialize m_max_tx_weight. Callers are already forced to provide the weight in the constructor.
I’m not looking to push this forward for now, if someone wants to take it over be my guest. Draft for now.
I opened #29523, which took over from this, and addressed the review comments here.
