Often the goal with subtracting fee from outputs is to sweep an entire wallet’s balance. As such, it is reasonable to allow negative effective value UTXOs as inputs as otherwise the target value (the balance) cannot be reached. However this is not always desirable as there may be solutions with only positive EV UTXOs, so we only try with negative EV UTXOs if we are unable to find any solutions with the normal groupings.
Fixes #23026
1156+ self.generate(self.nodes[0], 1)
1157+
1158+ created = w.createrawtransaction([], [{self.nodes[0].getnewaddress(address_type="bech32"): 1.00001000}])
1159+
1160+ # With 15 sat/vb, the low value input is negative ev. This should still fund.
1161+ funded = w.fundrawtransaction(created, {"subtractFeeFromOutputs":[0], "fee_rate": 15})
test/functional/rpc_fundrawtransaction.py:1161:9: F841 local variable 'funded' is assigned to but never used
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
No conflicts as of last run.
Code review ACK c351e53f42c85c7ed95bde842f07554c9798dbcb
Checked that the new test fails on current master but passes with this PR.
1143@@ -1143,6 +1144,21 @@ def test_feerate_rounding(self):
1144 rawtx = w.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(address_type="bech32"): 1 - 0.00000202}])
1145 assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, {"fee_rate": 1.85})
1146
1147+ def test_sffo_with_negative_ev(self):
1148+ self.log.info("Testing that subtract fee from outputs with negative effective value outputs still works")
0 def test_wallet_sweaping(self):
1 self.log.info("Testing that wallet sweeping works even with negative effective value.")
Test that enable subtractFeeFromOutputs will include negative effective value UTXOs.
1156+ self.generate(self.nodes[0], 1)
1157+
1158+ created = w.createrawtransaction([], [{self.nodes[0].getnewaddress(address_type="bech32"): 1.00001000}])
1159+
1160+ # With 15 sat/vb, the low value input is negative ev. This should still fund.
1161+ w.fundrawtransaction(created, {"subtractFeeFromOutputs":[0], "fee_rate": 15})
subtractFeeFromOutputs
Added an assert_raises_rpc_error.
Also made the fee rate calculation programmatic
Concept ACK.
Did a light code review and I think it’s better to drop the first commit as it’s not required to fix the linked bug. If you’d like to keep it, I’d suggest to make inclusion/exclusion of the change fee explicit when stetting targets for coinselection algorithms, rather than implicit m_change_fee = 0 (it’s not zero, it’s just payed by recipient!).
383@@ -384,7 +384,7 @@ bool AttemptSelection(const CWallet& wallet, const CAmount& nTargetValue, const
384 std::vector<std::tuple<CAmount, std::set<CInputCoin>, CAmount>> results;
385
386 // Note that unlike KnapsackSolver, we do not include the fee for creating a change output as BnB will not create a change output.
387- std::vector<OutputGroup> positive_groups = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, true /* positive_only */);
388+ std::vector<OutputGroup> positive_groups = GroupOutputs(wallet, coins, coin_selection_params, eligibility_filter, !coin_selection_params.m_subtract_fee_outputs/* positive_only */);
non_knapsack_groups?
groups and the groups that KnapsackSolver uses to be knapsack_groups. It’s better for the special case to be named after the special case rather than the normal case to be named as the negation of the special case.
1143@@ -1143,6 +1144,21 @@ def test_feerate_rounding(self):
1144 rawtx = w.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(address_type="bech32"): 1 - 0.00000202}])
1145 assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, {"fee_rate": 1.85})
1146
1147+ def test_sffo_with_negative_ev(self):
1148+ self.log.info("Testing that subtract fee from outputs with negative effective value outputs still works")
1149+
1150+ self.nodes[1].createwallet("negevsffotest")
1151+ w = self.nodes[1].get_wallet_rpc("negevsffotest")
w has no balance/utxos, may seem duh but it helps readability of the two wallet dance
tACK https://github.com/bitcoin/bitcoin/pull/23534/commits/c351e53f42c85c7ed95bde842f07554c9798dbcb
Verified that the test fails in master.
Though I am thinking, the problem of negative ev utxos are only relevant for sweeping transactions with fundrawtransaction rpc (there are other ways to create sweeps). Does this warrant removing negative ev for all cases with subtract_fee_outputs? Maybe we can have a more sweeping focused approach to trigger BnB?
But this solution means you will end up with less bitcoins on the receiving end, doesn’t it? Users probably would prefer the higher amount received.
I suppose the best UX would be to either ignore the net-negative UTXOs and simply send less than the requested amount. But that is apt to unprovably “burn” the dust unless we can somehow publish the dust private keys for others to sweep (but that seems dangerous and potentially ties to some centralised publisher).
Second-best might be to effectively reduce the fee rate by simply refusing to pay more for these UTXOs than they’re worth. But care is needed to ensure the user is aware of this and transactions don’t get stuck.
Also, any of this should only occur when trying to send the entire balance I think?
But this solution means you will end up with less bitcoins on the receiving end, doesn’t it?
Yes.
Users probably would prefer the higher amount received.
I’m not sure. Subtract fee from outputs has always been “X amount leaves the wallet”, so I think having less than that leave would be surprising?
I suppose the best UX would be to either ignore the net-negative UTXOs and simply send less than the requested amount. But that is apt to unprovably “burn” the dust unless we can somehow publish the dust private keys for others to sweep (but that seems dangerous and potentially ties to some centralised publisher).
Negative EV UTXOs can become positive if the feerate were to drop (assuming it was high).
Second-best might be to effectively reduce the fee rate by simply refusing to pay more for these UTXOs than they’re worth. But care is needed to ensure the user is aware of this and transactions don’t get stuck.
I think that might be dangerous, and surprising to users.
Also, any of this should only occur when trying to send the entire balance I think?
I think it is reasonable for this to occur for all subtract fee from outputs sends. Maybe it should try positive only first, and if that fails, allow negative?
Nit: typo in the PR title “negtive” (may make it harder to find later)
I typically use the “subtract fee” feature in combination with manual coin selection (for which this PR has no effect). That’s different from the use case of emptying out a wallet. At minimum the RPC docs should warn that this will spend negative net value coins. The GUI should also warn, preferably only when this is actually the case. It can get quite expensive if a wallet has lots of dust.
The first commit is worth its own PR if this doesn’t make it.
Sometimes subtractFeeFromOutputs requires spending negative ev UTXOs
(e.g. when sweeping an entire wallet). If we are unable to find any
solutions normally, then try again but this time allowing negative ev
UTXOs.
When subtracting fee from outputs, it should be okay to spend negative
ev UTXOs because the recipient is paying for it.
Code review ACK a26a64c63dd8d55d0ac972f5420f5dd27e7f53d8
Haven’t re-verified test
402@@ -403,6 +403,20 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
403 results.push_back(*srd_result);
404 }
405
406+ // If we didn't find any solutions, and are subtracting the fee from the outputs, we may need to include negative effective value coins.
407+ // So try BnB and SRD again with all_groups.
408+ // It is safe to do this because a solution including negative EVs will have a greater waste.
good change to simplify the behavior difference
reACK https://github.com/bitcoin/bitcoin/pull/23534/commits/a26a64c63dd8d55d0ac972f5420f5dd27e7f53d8
409+ if (results.size() == 0 && coin_selection_params.m_subtract_fee_outputs) {
410+ if (auto bnb_result{SelectCoinsBnB(all_groups, nTargetValue, coin_selection_params.m_cost_of_change)}) {
411+ bnb_result->ComputeAndSetWaste(CAmount(0));
412+ results.push_back(*bnb_result);
413+ }
414+ if (auto srd_result{SelectCoinsSRD(all_groups, srd_target)}) {
Assume(group.GetSelectionAmount() > 0) statement in SelectCoinsSRD()?
SelectCoinsSRD() in coinselection.h states that it expects positive ev groups, might need to change that)
I already mentioned this in person, but I find that “subtract fee” leads to unintuitive behavior, especially in the context of sweeping (and makes all tests a lot more complicated).
It would make more sense to me that the wallet never spends UTXOs with negative effective value unless explicitly tasked to do so, since it just costs users (either sender or receiver) more fees and more blockspace for the debatable reason of not leaving any UTXOs behind. The main use-case where I see “subtract fee from recipient” making sense, is in the context of Coin Control, when the sender picks the inputs explicitly and doesn’t want to calculate the appropriate fee manually.
Especially in the case of sweeping, I agree with Luke, that the user should prefer getting more money over using all UTXOs. If they sweep at minFeeRate, they’re getting the optimal outcome anyway. We might want to introduce a dedicated sweep function that sends the maximum by default, and optionally can be toggled to spend all UTXOs.
I already mentioned this in person, but I find that “subtract fee” leads to unintuitive behavior, especially in the context of sweeping (and makes all tests a lot more complicated).
Could you maybe illustrate what the unintuitive behavior is with an example? The rest of your comment about “user should get more money by default” obviously sounds good if it doesn’t have other tradeoffs.
The main use-case where I see “subtract fee from recipient” making sense, is in the context of Coin Control, when the sender picks the inputs explicitly and doesn’t want to calculate the appropriate fee manually.
And that scenario could be considered sweeping as well. Sweeping can be generalized to “spend UTXOs XYZ fully with all value going to one output” where XYZ could be all UTXOs in the wallet, or a preset input list.
Especially in the case of sweeping, I agree with Luke, that the user should prefer getting more money over using all UTXOs. If they sweep at
minFeeRate, they’re getting the optimal outcome anyway. We might want to introduce a dedicated sweep function that sends the maximum by default, and optionally can be toggled to spend all UTXOs.
I think it is unintuitive and not very good UX to not spend uneconomical UTXOs in a sweep. I think that users would expect a sweep function to spend all UTXOs regardless, and it would be surprising if sweeping a wallet left behind any balance, even uneconomical.
Could you maybe illustrate what the unintuitive behavior is with an example?
I’m not sure about unintuitive, but subtractFeeFromOutputs definitely causes a lot of headaches in coin selection code, so getting rid of it would greatly simplify a lot of it. It would certainly make the logic much easier to follow.
For some historical context, #5831 added subtractFeeFromAmount (which was a rebase of #4331) and that points to 2 issues where the proposed use case is sweeping.
Labels
Wallet
Milestone
22.1