The PR branch HEAD was 93f37c00dd at the time of this review club meeting.
Notes
The RPCs fundrawtransaction, send, sendmany, and sendtoaddress have
subtract_fee_from_outputs, subtractfeefrom, and subtractfeefromamount
arguments/options respectively. As these all implement the same behavior,
we’ll refer to them collectively as subtractfeefromamount (SFFA). For outputs
marked with SFFA, the amount assigned to the recipient is reduced
corresponding to the transaction’s fee. If multiple outputs of a transaction
are marked, the fee is split equally among them.
The effective value of a UTXO is the result of its value minus the fees
necessary to spend it. The effective value therefore is dependent on the
current fee rate and UTXOs with a low value will have a negative effective
value at sufficiently high fee rates.
PR #17331 updated the wallet to use effective value in its coin
selection. This allowed to use a fixed selection target calculated from the
remaining parts of the transaction while inputs already had accounted for
their own cost, whereas previously the selection was trying to hit a dynamic
selection target as the fees needed to be amended to reflect the increasing
count of inputs.
Around the same time, the wallet’s default behavior was updated to skip
inputs with a negative effective value, under the assumption that users would
prefer to defer usage of an input rather than paying more than its value for
it to be spent.
In concert, these changes
broke an established
pattern for emptying a wallet by calling getbalance and using
fundrawtransaction with SFFA by specifying the full balance.
After some attempts had been made to reconcile effective values with SFFA,
PR #24118 was proposed as
an alternative solution. It allows users to send all UTXOs in the wallet.
<kouloumos> haven't tested what's stated in the notes but is says that the wallet’s default behavior was updated to skip inputs with a negative effective value which broke an established pattern for emptying a wallet by calling `getbalance` and using `fundrawtransaction` with SFFA by specifying the full balance
<Murch> ls55: previously, you would probably do a call to `getbalance` and then do a `send` with that amount as the recipient amount but flag the output as "subtract_fee_from_output"
<Murch> Yes, there was a conflict between getting the full balance of your wallet and the wallet by default filtering out the uneconomical UTXOs when building a transaction
<larryruane> "When would you use one and when the other?" -- is it a matter of whether we want to pay someone a specific amount, versus we're really paying ourselves (so the exact amount of value transferred isn't that important)?
<ls55> `Sendall` is to be used when the user wants to use given UTXOs to pay a destination and SFFA when the user wants to use a given budget to pay an address.
<Murch> So, `sendall` will either use all UTXOs in the default mode, or a specific subset when it's defined, and `sffo` will use a specific budget to create an output.
<Murch> Which means that with `sffo` the input selection is left to the wallet, and the poor recipient may pay for a single input or fifteen, depending on your wallet's UTXO pool composition ^^
<Murch> "This sweep API might be less safe than send APIs because it doesn't force you to specify amount you are trying to send. It's easier to fat-finger by accidentally sweeping the wrong wallet[…]."
<ls55> Yes, there is a valid concern. This RPC enables unsuspecting users can send an amount larger than they intended. The other send RPCs force the user to specify amount.
<lightlike> I think, also thieves could clean out our wallet non-interactively now if they manage to get us send a single command. before, they would have hat to guess the amount or interactively query the balance first.
<otech> I think it is valid, but not extremely concerning... not the theft concern but the larger than expected amount concern maybe should be addressed
<josibake> idk if id characterize it as "unsafe". unsafe, in my mind, implies there is something that can be exploited/bad even if the user is using it correctly
<Murch> lightlike: I guess that's true, but either way they would have to have access to your cli at that point, which means they could run both commands, or have gained your trust enough that you just run a command that says "sendall" in your terminal to one of their addresses ^^
<Murch> larryruane: Yeah, I guess that would be possible, but it would also severely compromise the UX improvement this new RPC provides in the first place
<ls55> `send_max` changes the default behavior of spending all UTXOs to maximizing the output amount of the transaction by skipping uneconomic UTXOs. So this doesn't allow users to choose UTXOs as they can choose uneconomical ones.
<Murch> ls55: Yeah, they both specify or restrict which UTXOs will be used by the transaction, but if you want an explicit set, it wouldn't be completely clear whether you want it forced so, or post-filtered.
<Murch> Another reason is that if the parent transactions had low feerates, our transaction might have a lower feerate than we aimed for, because Bitcoin Core currently does not bump low feerate parents automagically yet
<Murch> Generally, spending unconfirmed UTXOs is just more complicated, and it doesn't really align well with the use-case of emptying a wallet completely
<Murch> ls55: We also never spend foreign unconfirmed outputs in regular transactions. We just allow to spend unconfirmed change if we're unable to send a transaction otherwise
<Murch> larryruane: No, package relay ensures that a parent with a feerate below the dynamic minRelayTxFeeRate will still propagate if the child is affluent enough
<Murch> I think available coins should generally filter unconfirmed foreign UTXOs but there is at least some eligibility filters that will permit unconfirmed change outputs that were sent by your own wallet to itself
<lightlike> why is it possible to have multiple recipients with an unspecified amount, that each get the same share? Is there a particular use case for that?
<larryruane> hope this isn't too high-level of a question: I assume most transactions don't come from the bitcoin core wallet, so when we make an improvement like this, is part of the reason to provide a (working) model for other wallet devs to follow?
<Murch> Beyond emptying wallets, and especially with a glance at the cleanup decorator in the tests, can you guess where in the codebase sendall may find use?
<josibake> lightlike: in the case of sweeping a wallet, tedium. i might know that i want 5 utxos in the new wallet , but dont really want to do the match and right out all the amounts
<Murch> also, larryruane, yeah, I think that while Bitcoin Core wallet won't be the wallet with the most and coolest features, I think we should aim to provide an example of what a wallet should be able to do and how that could work
<lightlike> so the other use case would be to clean out a wallet between two tests, so that it doesn't have any unwanted utxos that might meddle with later tests.
<josibake> there are also tests that use the premined chain for performance, but you end up with a bunch of coinbase utxos. this might be a nice way to use the premined regtest chain but then clean out the wallet before starting
<achow101> larryruane: decorators are things in python that wrap a function. decorators take a function, and can do stuff before and after the function is run. @cleanup in particular runs the function, then cleans up the wallet after the test
<Murch> So, these RPCs allow different ways of specifying the fee rate. You can either give a specific fee rate in sat/vB, or you can provide a block target in number of blocks you'd like this transaction to be confirmed in
<josibake> still reading through all of the feedback on the PR, but is there a TLDR as to why a new RPC was the agreed on approach vs adding a parameter to existing RPCs?
<larryruane> Murch: one nit (i could leave as a PR comment), at the beginning of the description, i think `fSubtractFeeAmount` should be `fSubtractFeeFromAmount`?