wallet: Allow negative effective value inputs when subtracting fee from outputs #23534

achow101 · 2021-11-17T05:12:43Z

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

test/functional/rpc_fundrawtransaction.py

DrahtBot · 2021-11-18T03:01:14Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

meshcollider

Code review ACK c351e53

Checked that the new test fails on current master but passes with this PR.

S3RK · 2021-11-22T07:19:40Z

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!).

S3RK

some nits below

test/functional/rpc_fundrawtransaction.py

gruve-p · 2021-11-23T08:11:31Z

ACK c351e53

fanquake · 2021-11-25T07:45:05Z

cc @yurayakimenko

src/wallet/spend.cpp

test/functional/rpc_fundrawtransaction.py

instagibbs · 2021-11-25T08:28:34Z

concept ACK, if someone wants to do "send max value possible" we could support this as another option and choose positive only groups again.

rajarshimaitra

tACK c351e53

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?

test/functional/rpc_fundrawtransaction.py

luke-jr

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?

achow101 · 2021-12-05T18:54:15Z

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?

instagibbs · 2021-12-10T01:25:59Z

ACK f44c3ce

Sjors · 2021-12-10T05:59:31Z

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.

achow101 · 2021-12-10T14:28:23Z

I've changed this to try negative EVs if there are no solutions with the normal groupings.

meshcollider

Code review ACK a26a64c

Haven't re-verified test

instagibbs

good change to simplify the behavior difference

reACK a26a64c

instagibbs · 2021-12-14T04:03:33Z

src/wallet/spend.cpp

@@ -403,6 +403,20 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
        results.push_back(*srd_result);
    }

+    // If we didn't find any solutions, and are subtracting the fee from the outputs, we may need to include negative effective value coins.
+    // So try BnB and SRD again with all_groups.
+    // It is safe to do this because a solution including negative EVs will have a greater waste.


What about just throwing these two selections into the mix unconditionally? Would that be correct? Efficiency aside.

glozow · 2022-01-03T13:44:20Z

src/wallet/spend.cpp

+            bnb_result->ComputeAndSetWaste(CAmount(0));
+            results.push_back(*bnb_result);
+        }
+        if (auto srd_result{SelectCoinsSRD(all_groups, srd_target)}) {


Might this trigger the Assume(group.GetSelectionAmount() > 0) statement in SelectCoinsSRD()?

(also note the comment on the declaration for SelectCoinsSRD() in coinselection.h states that it expects positive ev groups, might need to change that)

murchandamus · 2022-01-05T16:45:16Z

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.

ryanofsky · 2022-01-05T16:56:42Z

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.

achow101 · 2022-01-05T17:52:19Z

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.

achow101 · 2022-03-10T14:21:57Z

Closing this in favor of introducing sendall to solve the problem.

fanquake added the Wallet label Nov 17, 2021

maflcko added the Needs backport (22.x) label Nov 17, 2021

maflcko added this to the 22.1 milestone Nov 17, 2021

fanquake reviewed Nov 17, 2021

View reviewed changes

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

achow101 force-pushed the no-change-fee-w-sffo branch from 05ef67a to c351e53 Compare November 17, 2021 18:12

This was referenced Nov 18, 2021

Optimize coin selection by dropping BnB upper limit #23367

Closed

wallet: Introduce SelectionResult for encapsulating a coin selection solution #22019

Merged

meshcollider reviewed Nov 22, 2021

View reviewed changes

meshcollider mentioned this pull request Nov 22, 2021

subtractFeeFromOutputs option in fundrawtransaction RPC call is not working as expected on v22.0 #23026

Closed

S3RK reviewed Nov 22, 2021

View reviewed changes

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

fanquake requested a review from instagibbs November 25, 2021 07:44

instagibbs reviewed Nov 25, 2021

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

test/functional/rpc_fundrawtransaction.py Show resolved Hide resolved

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

rajarshimaitra reviewed Nov 27, 2021

View reviewed changes

test/functional/rpc_fundrawtransaction.py Outdated Show resolved Hide resolved

luke-jr suggested changes Nov 30, 2021

View reviewed changes

achow101 force-pushed the no-change-fee-w-sffo branch from c351e53 to b9af533 Compare December 5, 2021 19:28

DrahtBot mentioned this pull request Dec 6, 2021

wallet: add config to prioritize a solution that doesn't create change in coin selection #23475

Closed

DrahtBot added the Needs rebase label Dec 9, 2021

wallet: Set change fee only if not subtracting fee from outputs

227803c

achow101 force-pushed the no-change-fee-w-sffo branch from b9af533 to f44c3ce Compare December 9, 2021 17:35

DrahtBot removed the Needs rebase label Dec 9, 2021

fanquake requested review from meshcollider and Sjors December 10, 2021 01:32

fanquake changed the title ~~wallet: Allow negtive effective value inputs when subtracting fee from outputs~~ wallet: Allow negative effective value inputs when subtracting fee from outputs Dec 10, 2021

achow101 added 2 commits December 10, 2021 09:10

wallet: Try selecting with negative ev coins when subtractFeeFromOutputs

89d1d6f

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.

tests: Test for spending neg ev UTXOs when subtracting fee from outputs

a26a64c

When subtracting fee from outputs, it should be okay to spend negative ev UTXOs because the recipient is paying for it.

achow101 force-pushed the no-change-fee-w-sffo branch from f44c3ce to a26a64c Compare December 10, 2021 14:27

meshcollider reviewed Dec 13, 2021

View reviewed changes

instagibbs approved these changes Dec 14, 2021

View reviewed changes

glozow reviewed Jan 4, 2022

View reviewed changes

glozow mentioned this pull request Jan 21, 2022

Add 'sendall' RPC née sweep #24118

Merged

fanquake added Needs backport (22.x) and removed Needs backport (22.x) labels Mar 5, 2022

achow101 closed this Mar 10, 2022

maflcko removed the Needs backport (22.x) label Mar 31, 2022

achow101 mentioned this pull request May 16, 2022

subtractfeefromamount=true fails with insufficient funds #25129

Closed

bitcoin locked and limited conversation to collaborators Mar 31, 2023

wallet: Allow negative effective value inputs when subtracting fee from outputs #23534

wallet: Allow negative effective value inputs when subtracting fee from outputs #23534

Uh oh!

Conversation

achow101 commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Nov 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

S3RK commented Nov 22, 2021

Uh oh!

S3RK left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gruve-p commented Nov 23, 2021

Uh oh!

fanquake commented Nov 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Nov 25, 2021

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Dec 5, 2021

Uh oh!

instagibbs commented Dec 10, 2021

Uh oh!

Sjors commented Dec 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Dec 10, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

glozow Jan 3, 2022

Choose a reason for hiding this comment

Uh oh!

glozow Jan 3, 2022

Choose a reason for hiding this comment

Uh oh!

murchandamus commented Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky commented Jan 5, 2022

Uh oh!

achow101 commented Jan 5, 2022

Uh oh!

achow101 commented Mar 10, 2022

Uh oh!

Uh oh!

achow101 commented Nov 17, 2021 •

edited

Loading

DrahtBot commented Nov 18, 2021 •

edited

Loading

S3RK left a comment •

edited

Loading

Sjors commented Dec 10, 2021 •

edited

Loading

instagibbs left a comment •

edited

Loading

murchandamus commented Jan 5, 2022 •

edited

Loading