Allow negative effective value inputs when subtracting fee from outputs (wallet)

Host: glozow  -  PR author: achow101

The PR branch HEAD was a26a64c63d at the time of this review club meeting.


  • The fundrawtransaction RPC allows users to pass in a transaction with outputs filled in, and the wallet “funds” the payment and transaction fees by selecting UTXOs from the wallet’s UTXO pool to spend in the transaction’s inputs. The process of selecting which UTXOs to use is known as Coin Selection.

    • While transaction outputs have an amount, nValue, each added input increases the fee required to keep the transaction at a given feerate. As such, the effective value (nValue reduced by the UTXO’s input size multiplied by the given feerate) is used during coin selection instead.

    • We have discussed Coin Selection and effective values in a previous review club.

    • The subtractFeeFromOutputs argument in fundrawtransaction causes the payment amounts in the transaction to be reduced by the fee amount. We have discussed the behavior around subtracting fee from recipients in a previous review club.

  • PR #23534 changes coin selection behavior to allow UTXOs with negative effective values to be selected when subtractFeeFromOutputs is on.


Conceptual Questions

  1. Did you review the PR? What was your review approach?

  2. What does the subtractFeeFromOutputs argument do in fundrawtransaction; how do you use it? Why might a user opt to subtract fees from outputs?

  3. What does it mean for a coin to have negative effective value? Why does it usually not make sense to select negative effective value UTXOs?

  4. What does this PR do? What are the advantages of disadvantages of its changes?

  5. Since this is a wallet PR: could this change cause users to pay extreme fees, end up with dust outputs, or leak privacy? Would this be confusing to you as a user if you are using the default settings?

Implementation Questions

  1. What does the first commit, set change fee only if not subtracting fee from outputs, do? Does it change behavior?

  2. How does AttemptSelection() know that the user wants to subtract fees from recipient outputs? (Trace the fundrawtransaction call; how does it eventually end up calling AttemptSelection())?

  3. What is the type of bnb_result declared here (and srd_result a few lines below)? How does it resolve as a boolean expression in the if statement?

    7a. (Bonus) Why might we use auto instead of declaring the type explicitly here?

    7b. (Bonus 2) Can you find other examples of this calling pattern in the codebase?

  4. Why does the test need to call generate here?

  5. Can you think of any logic that isn’t tested by the functional test added in this PR? Do you have any suggestions to better test the changes added in this PR?

Meeting Log

  117:00 <glozow> #startmeeting
  217:00 <glozow> Happy New Year! Welcome to PR review club!
  317:00 <b10c__> hi (lurking today)
  417:00 <glozow> We're looking at PR #23534 today, notes and questions in the usual place:
  517:00 <larryruane> hi
  617:00 <lightlike> hi
  717:00 <kouloumos> hi
  817:00 <tarun> hi
  917:00 <kalpa> hi
 1017:00 <erik> hi
 1117:00 <kodyl> yo
 1217:00 <glozow> Feel free to say hi, and let us know if it's your first time
 1317:01 <svav> Hi
 1417:01 <kalpa> first time for me
 1517:01 <rahim> hi (lurking), first time!
 1617:01 <turkycat> hi, first time. will observe
 1717:01 <janoside> Hi, first time
 1817:01 <erik> It's my first time, here
 1917:01 <stickies-v> hiya
 2017:01 <kodyl> first time (lurking)
 2117:01 <Julianmrodri> hi, first time
 2217:01 <Loris> hi, first time :)
 2317:01 <glozow> kalpa: rahim: turkycat: janoside: erik: kodyl: Julianmrodri: Loris: Welcome!
 2417:01 <tim14> first time (lurking)
 2517:01 <glozow> wow so many first-timers :D
 2617:01 <fqxjkdtbwnm5> Hi, first time :-)
 2717:01 <turkycat> just discovered via twitter
 2817:01 <glozow> Quick poll: have you reviewed the PR or read the notes? y/n?
 2917:02 <kodyl> y
 3017:02 <janoside> y
 3117:02 <rahim> n
 3217:02 <Loris> n
 3317:02 <turkycat> y and y
 3417:02 <kalpa> nn
 3517:02 <michaelfolkson> hi
 3617:02 <stickies-v> 0.5y, glossed over it
 3717:02 <erik> y
 3817:02 <kouloumos> n
 3917:02 <glozow> For those of you who reviewed, what was your review approach?
 4017:02 <tarun> y
 4117:02 <svav> y read the notes
 4217:03 <glozow> tim14: fqxjkdtbwnm5: Welcome as well
 4317:03 <tarun> read the notes reviewed the questions
 4417:03 <sipa> Looks correct to me.
 4517:03 <glozow> awesome, let's dive into the questions
 4617:03 <glozow> What does the `subtractFeeFromOutputs` argument do in fundrawtransaction; how do you use it? Why might a user opt to subtract fees from outputs?
 4717:04 <kouloumos> It subtracts the fee from the outputs. You specify from which outputs you want that fee to be subtracted and it will be equally deducted from the amount of each one.
 4817:04 <glozow> kouloumos: yes!
 4917:05 <erik> It subtracts the fee from the sending amount. It's useful for self transfers, like during wallet migration
 5017:05 <glozow> Can anyone tell us a use case for this?
 5117:05 <willcl_ark> hi
 5217:05 <stickies-v> It is especially useful when sweeping a wallet, because you don't know how much fees you'll have to pay before iterating over all the inputs
 5317:05 <glozow> erik: right! since the payment amount will be reduced, we'd imagine that you wouldn't use this to pay a merchant for example
 5417:06 <glozow> stickies-v: yes! what does "sweeping a wallet" mean?
 5517:06 <brunoerg> hi
 5617:07 <stickies-v> Sending all your UTXOs to one or multiple new addresses. Breaking the piggy bank and putting everything in a shiny new one :)
 5717:07 <glozow> stickies-v: perfect
 5817:08 <glozow> next question: What does it mean for a coin to have negative effective value? Why does it usually not make sense to select negative effective value UTXOs?
 5917:08 <larryruane> the fee increase that would be required to include that input is greater than the value of the input
 6017:09 <kalpa> Do negative effective value UTXOs even exist?
 6117:09 <glozow> larryruane: correct
 6217:09 <brunoerg> the cost to move this coin is greater than its value?
 6317:09 <glozow> kalpa: yes they do! it also depends on the feerate at which you're trying to construct this transaction
 6417:09 <glozow> perhaps it would be helpful for someone to tell us what effective value means :)
 6517:09 <glozow> brunoerg: correct
 6617:09 <turkycat> utxo value - fee per utxo
 6717:09 <kalpa> oh ok understood
 6817:10 <stickies-v> effective value is the nValue of the UTXO minus (tx size * current feerate)
 6917:11 <callebtc> what's a typical threshold value for a too-small utxo (i.e. negative effective value)?
 7017:11 <glozow> stickies-v: yep exactly
 7117:11 <glozow> callebtc: good question, i think it'd be a few hundred - a few thousand sats
 7217:13 <glozow> Alright next question: What does this PR do? What are the advantages of disadvantages of its changes?
 7317:13 <stickies-v> a 2-input 2-output p2wpkh tx is 208.5 vbytes, at a feerate of 5 sats/vbyte you'd get roughly 1000sats?
 7417:13 <callebtc> btw, if I understand correctly, the problem would also propagate to the next utxo: if your effective value is just marginally positive, the output (the input of the next tx) could then have a negative effective value, correct?
 7517:14 <larryruane> stickies-v: "tx size * current feerate" - is it tx size or input size?
 7617:14 <glozow> callebtc: hm, i don't think anything would propagate to the next utxo. the problem here is, when we spend this output in our transaction (i.e. we create an input that refers to this UTXO), the size of the input * feerate is more than the UTXO's nValue
 7717:14 yeah i just realized i've been confusing the two. that should be input size, so my previous example is off too
 7817:15 <glozow> larryruane: stickies-v: oh you're right, i didn't look closely enough. yes it's the size of the input.
 7917:16 <glozow> yeah so an input to spend a p2wpkh is ~68vB, and at 5sat/vB that's about 340 sats
 8017:18 <George[m]1> What’s very strange to me is that I joined IRC as the ‘zenlo’ user which shows up here.
 8117:18 <glozow> are we okay to move on to the next question?
 8217:18 <erik> Yes
 8317:19 <brunoerg> y
 8417:19 <glozow> okay: what does this PR do?
 8517:19 <callebtc> glozow: > What does this PR do? I think it allows one to spend negative EV utxo's which might be desirable if the user wants to, for example, clear out the entire wallet.
 8617:20 <kalpa> it allows us to consolidate small UTXO values?
 8717:20 <glozow> callebtc: yes, it allows us to include the negative effective value UTXOs in a transaction when `subtractFeeFromOutputs` is on
 8817:20 <glozow> kalpa: yes that's what the effect is!
 8917:20 <callebtc> Understood! Can I ask, why it wasn't possible in the first place?
 9017:21 <glozow> stickies-v mentioned sweeping wallets, so let's consider an alternative approach. Why don't we just have a `sweepwallet` RPC?
 9117:22 <glozow> callebtc: because selecting negative effective value inputs would mean you're not getting closer to funding your payment amount, just throwing away extra money to fees
 9217:22 <stickies-v> I don't think it's a very common use case, so maybe no need to clutter the RPC with an extra method when the fundrawtransaction method can already get the job done?
 9317:23 <lightlike> why is it desirable to sweep the wallet when we are actually donating fees to the miners? why not just forget about these utxos instead and move only those coins with a positive effective value?
 9417:24 <kalpa> well fundrawtransaction is the method to create a transaction so it wouldn't make sense to create another one
 9517:24 <brunoerg> lightlike: privacy?
 9617:24 <stickies-v> lightlike yeah that's my number one mystery with this entire PR, not sure why you wouldn't just leave the negative EVs behind
 9717:24 <m011> I also think fundrawtransaction method gets the job done. This only needs a parameter that use the wallet balance instead of specifying a value.
 9817:24 <glozow> lightlike: good question. i guess that could make sense to the user, but then we're cluttering the UTXO set.
 9917:24 <achow101> stickies-v: alternatively, why should we clutter all of the coin selection code with subtractFeeFromOutputs handling when a sweep function would be fairly simple to create.
10017:24 <willcl_ark> Sometimes you just wanna use up all UTXOs
10117:25 <rahim> lightlike: couldn't a negative effective value input become a "positive" effective value one eventually, assuming price increases?
10217:25 <larryruane> basic question: with this sweeping use-case, the user *wants* to spend negative-effective-value UTXOs, even though doing that is uneconomical? If so, I don't see why that's desired
10317:25 <lightlike> brunoerg: wouldn't the new solution be actually worse to privacy than just forgetting about the utxos, because it allows third partieds to connects more utxos?
10417:25 <willcl_ark> You're also altruistically reducing the UTXO set size
10517:25 <glozow> achow101: +1, an extra RPC method is better than complicating the coin selection logic imo
10617:25 <stickies-v> achow101 excellent point, I don't have any counterpoints haha
10717:26 <larryruane> (lightlike +1 you beat me to it on that question)
10817:26 <kalpa> if btc goes to a million dollars it could make sense to sweep all dust outputs into one now
10917:26 <glozow> rahim: good point, if you're able to create very low feerate transactions in the future, they might be positive effective value
11017:26 <glozow> but this has nothing to do with the exchange rate with USD
11117:27 <fqxjkdtbwnm5> @rahim, I do not think so, because everything written above is in the "unit" sats and correct independent of USD price
11217:27 <brunoerg> lightlike: hmmmm, good point.
11317:27 <achow101> there is some philosophical questions around whether sweeping should ignore negative ev utxos. imo users would expect that sweeping results in their wallet balance becoming 0, but not spending negative ev utxos means that there will be a non-zero remaining balance
11417:27 <larryruane> kalpa: I'm not sure the btc price matters, because wouldnt everything (fees, etc.) scale linearly?
11517:28 <rahim> gotcha; fees are calculated from the tx size and independent of price, right?
11617:28 <erik> rahim: feerate is calculated in sats
11717:28 <callebtc> Is there a technical reason why there isn't a `sweepwallet` RPC (excluding complexity, bloat, ugliness, etc)?
11817:28 <glozow> larryruane: if i'm sweeping a wallet, i'll probably throw away my keys/backups, so i'd prefer not to leave anything behind. i'd also feel like i'm dumping plastic bags into the ocean, just not very eco-friendly to keep those in the UTXO set
11917:29 <larryruane> glozow: +1
12017:29 <constantin21> achow101: I think it comes down to donating dust to miners or "deflating" bitcoin supply?
12117:29 <achow101> callebtc: not in particular
12217:30 <glozow> okay we're halfway through the hour so i'll throw out the final conceptual question: Since this is a wallet PR, could this change cause users to pay extreme fees, end up with dust outputs, or leak privacy? Would this be confusing to you as a user if you are using the default settings?
12317:30 <svav> Can someone give a simple explanation as to why this PR was felt necessary? If fees make it uneconomical to spend a particular UTXO, why would you ever bother spending it? What is the reason for this existence of this PR? X)
12417:30 <glozow> (I included these in the notes as examples of what you might want to ask yourself when reviewing the PR)
12517:31 <callebtc> I think it would be helpful to try to find an answer to the "why does this PR even exist?" questions
12617:31 <achow101> svav: it is a bug fix for a reported bug where someone was sweeping their entire wallet but got an insufficient funds error. the root cause was found to be that their wallet contained negative ev utxos.
12717:32 <glozow>
12817:32 <achow101> however they were able to sweep with an older version of the software, so this is technically a regression
12917:32 <kalpa> consolidating your UTXOs could be a privacy leak but that also happens with other coin selection algorithms
13017:32 <constantin21> svav : It enables donating unobtainable dust to miners
13117:33 <janoside> glozow: Pay extreme fees: no (changes seem independent of fee rate). End up with dust outputs: no (this will destroy dust outputs). Leak privacy: yes, possibly (history of the dust outputs can now be linked to the spend).
13217:33 <glozow> kalpa: i don't think that's caused by this PR, but a side effect of consolidation.
13317:33 <kalpa> glozow, of course
13417:34 <glozow> janoside: same thing, this does not worsen the privacy of our coin selection implementation, it's always been the case that consolidating UTXOs links them to the same wallet
13517:36 <glozow> any further questions about the concept? this discussion has been very lively \o/
13617:36 <svav> If a user was sweeping their wallet, would they currently be adding each input manually?
13717:36 <Murch> achow101: that sounds more like an issue of the wallet not distinguishing the balance and spendable balance
13817:37 <kalpa> glozow, so this PR does not affect the gui wallet functionality correct?
13917:37 <achow101> Murch: the spendable balance depends on the feerate though, and having a balance that constantly changes doesn't sound like good UX
14017:37 <lightlike> but I think it does worsen the privacy of that particular transaction - using less utxos (which are not necessary/economical) should improve privacy.
14117:37 <m011> Correct. This PR does not affect the GUI
14217:38 <svav> Apart from sweeping a wallet, are there any other production circumstances where a coin being able to have a negative effective value would be useful/needed?
14317:38 <glozow> kalpa: depends on what you mean. the gui wallet is the same as the wallet, so no. but nothing should change in the gui itself, so yes.
14417:38 <glozow> lightlike: ah that is a good point
14517:39 <kalpa> glozow, but the gui wallet does not use the fundrawtransaction RPC or does it? or is it supposed to do so in the future?
14617:39 <Murch> achow101: Sure, but in the context of building a transaction with a specific feerate, the spendable balance would be stable.
14717:40 <achow101> kalpa: the gui, fundrwatransaction, and the send* functions, etc. all end up using the same CreateTransaction function which contains all of the coin selection logic
14817:42 <callebtc> does anyone know how many utxo's out there have negative EV?
14917:42 <sipa> that depends on the feerate
15017:42 <callebtc> And how many sats they effectively lock away?
15117:42 <larryruane> just so everyone's aware, a sophisticated user can also choose the inputs (createrawtransaction) to have complete control (not use the built-in coin selection algorithms)
15217:43 <glozow> svav: i can't think of any other use cases, so I'm a proponent of adding a `sweepwallet` RPC that sweeps all UTXOs. i think if this is the expected behavior, users should be able to say they specifically want to sweep
15317:43 <callebtc> sipa: isn't a lower bound 1sat/vbyte anyway?
15417:43 <sipa> in practice yes
15517:44 <sipa> But the answer to the question "how many utxos have negative EV" isn't well defined - it depends on the feerate the spender wants to use.
15617:44 <glozow> maybe one day we can lower the min relay feerate
15717:44 — glozow ducks
15817:44 <callebtc> I see. I'll rephrase: how many UTXO's have negative EV at 1sat/vB fee rate?
15917:44 <sipa> Probably very little, because the dust rule prevents such UTXOs from being created.
16017:45 <m011> I think adding something like "max"parameter to fundrwatransaction or send RPC can be a better approach.
16117:45 <callebtc> I guess I'd have to ask the on chain metrics people...
16217:45 <callebtc> sipa: I see
16317:45 <stickies-v> sipa but the dust rule is policy right, so non-core clients may not adhere to that? and I don't think the dust policy has always been in place?
16417:46 <sipa> Sure, but the minrelayfee of 1 sat/vbyte is also policy.
16517:47 <stickies-v> ah, right
16617:47 <glozow> stickies-v: non-core clients would be silly not to adhere to policy, since it would make their mempool very unreliable
16717:47 <erik> Maybe a few very old utxos from minrelayfee times
16817:48 <erik> But may be minor
16917:48 <stickies-v> yeah good point - and to be clear i'm not advocating for any of this, just checking boundaries
17017:48 <rahim> could the wallet construct the transaction, but not broadcast it until the EV is positive? Simply "waiting" doesn't seem very elegant...
17117:48 <sipa> rahim: That's equivalent to just using a lower feerate.
17217:48 <glozow> rahim: the feerate of the transaction wouldn't change after you've created it
17317:48 <sipa> The feerate is choice by the user.
17417:49 <Murch> glozow: Wouldn't they'd just have a few extra transactions below minRelayTxFeeRate? I.e. expect a few more txes to be in a block when it would otherwise not be full?
17517:49 <rahim> gotcha, makes sense
17617:49 <glozow> but yeah you could just wait until the feerates are low to sweep your wallet
17717:49 <glozow> Murch: sorry who's they?
17817:50 <rahim> glozow: Andreas Antonop has talked about that before, right
17917:50 <Murch> People with a lower minRelayTxFeerate: how would their Mempool be "unreliable"?
18017:50 <glozow> oh. the 1sat/vB floor doesn't change even if your mempool is empty, so if your mempool accepts something under 1sat/vB, that doesn't mean it'll propagate
18117:50 <glozow> ideally your mempool is a good indicator of what miners have in their mempools
18217:51 <Murch> Ah yes, in that regard they would be unreliable.
18317:52 <glozow> alright i'll just throw out a few of the implementation questions now, since we're running out of time
18417:52 <glozow> What is the type of `bnb_result` declared here (and srd_result a few lines below)? How does it resolve as a boolean expression in the if statement?
18517:53 <erik> It's std::optional
18617:53 <larryruane> it's an optional, which means it can have a value or not, sort of replaces using a null pointer to mean no value
18717:53 <glozow> erik: almost there, it's an optional what?
18817:53 <stickies-v> SelectionResult
18917:53 <glozow> bingo
19017:54 <glozow> larryruane: yep, true if there's a `SelectionResult`, false if it's a nullopt
19117:54 <glozow> but on that line, why use `auto`?
19217:54 <erik> Optional can be used in expressions, if some data is returned it return true, false otherwise
19317:54 <larryruane> you can test an optional value as if it's a boolean, but a more explicit way is to call `has_value()` (i think)
19417:54 <erik> Like Some() in Rust
19517:55 <sipa> Or Just in Haskell.
19617:55 <glozow> erik: yeah exactly
19717:55 <erik> glozow: Auto instructs the compiler to infer the type
19817:55 <erik> With is std::optional
19917:55 <erik> *infer based on the assigment
20017:55 <glozow> erik: yup
20117:56 <glozow> Can you find other examples of this calling pattern in the codebase? We use it quite often
20217:56 <kalpa> what does bnb and srd stand for?
20317:56 <glozow> kalpa: Branch and Bound, Single Random Draw. coin selection algos
20417:57 <larryruane> you can use the indirect (*) to fetch the value or you can be more explicit and call the `value()` method
20517:57 <sipa> Super pedantic nit: auto isn't "the compiler inferring the type"; it is specified by the language what the type is; just implied by the expression rather than being explicit (I say this because it isn't like compilers that are smarter can be better at this or so).
20617:57 <brunoerg> interesting
20717:58 <glozow> sipa: right
20817:58 <glozow> and you won't accidentally cast something
20917:58 <kouloumos> kalpa: good discussion on those at
21017:59 <glozow> alrighty the rest of the questions are left as an exercise to the PR reviewer :) thank you all for coming!
21117:59 <glozow> #endmeeting