The PR branch HEAD was a26a64c63d at the time of this review club meeting.
Notes
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.
Questions
Conceptual Questions
Did you review the PR? What was your review approach?
What does the subtractFeeFromOutputs argument do in fundrawtransaction; how do you use it?
Why might a user opt to subtract fees from outputs?
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?
What does this PR do? What are the advantages of disadvantages of its changes?
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
What does the first
commit,
set change fee only if not subtracting fee from outputs, do? Does it change behavior?
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())?
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?
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?
<glozow> What does the `subtractFeeFromOutputs` argument do in fundrawtransaction; how do you use it? Why might a user opt to subtract fees from outputs?
<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.
<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
<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?
<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?
<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
<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.
<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
<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?
<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?
<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.
<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.
<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
<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?
<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
<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
<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?
<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)
<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.
<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).
<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
<lightlike> but I think it does worsen the privacy of that particular transaction - using less utxos (which are not necessary/economical) should improve privacy.
<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?
<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
<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)
<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
<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?
<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?
<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
<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).