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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--85328a0da195eb286784d51f73fa0af9-->
š§ 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/20589487266</sub>
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"));
nit: replace magic number with MAX_STANDARD_TX_WEIGHT
done
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();
Would this be giving up too early? We could still find a different solution further down the function, no?
yeah I was mostly hot-patching here, making sure it never returns too big(because it did very often)
should be mitigated?
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:
- if (group.GetSelectionAmount() == nTargetValue) {
+ if (group.GetSelectionAmount() == nTargetValue && group.m_weight <= max_weight) {
done
Concept ACK, but instead of bailing immediately, it would be better to extend the conditions that immediately return a result to also require adhering to the 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:
- } else if (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount()) {
+ } else if (group.m_weight <= max_weight && (!lowest_larger || group.GetSelectionAmount() < lowest_larger->GetSelectionAmount())) {
taken
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) {
Does 0 even ever make sense?
as much a sense as "1". Could have minimal relay size here instead?
At second glance, lemme retract my comment. It doesnāt really hurt to allow values down to 0, even if that is less than a sensible use of that parameter. Might be helpful in tests to be able to restrict it fairly low.
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:
- if (nTotal < nBest)
+ 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
did the minimal suggested work to give knapsack a fighting chance
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)
Ah, yeah max_input_weight is a much better name. Thanks!
@murchandamus, I'm not entirely sure about this naming change. At this point, we are placed at the coin selection algos level, which are situated a level below the transaction creation process. There is no concept of transaction weight here; only selection weight.
max_selection_weight is also fine I think, both are better than status quo
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();
I donāt think we can ever hit this if block since we only set lowest_larger when itās weight is less than the max_input_weight.
makes sense, removed
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.
makes sense, removed
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.
fixed up the fuzz test, which was attempting larger than 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};
It would be better to make max_input_weight a fuzzed value.
If you do, please also add an 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
test_framework.authproxy.JSONRPCException: Only 4999872180 coins to cover target value of 5100000840 (-4)
I can turn the error on and off locally by adding/removing the string from the error.
The wallet code seems to be the only place where this pattern of 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
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:
"Transaction building will fail if this can not be satisfied."},
"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."},
As above
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."},
As above
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."},
As above
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?
if this is a bit of overcounting, that's probably ok?
Mh, itās a bit of a smell if we cannot explain this.
I considered input counter, empty witness stack, that we might estimate for a high-R signature in a PSBT, because we didnāt sign it, and that we sometimes get a lower 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?
would still like a good explanation for this
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?
<!--cf906140f33d8803c4a75a2196329ecb-->
š 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
if (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
if (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 | + }
This does not seem to be necessary. As 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};
To not add 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)
Same as above, the 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'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.
