Add 'sendall' RPC née sweep (wallet, rpc)

https://github.com/bitcoin/bitcoin/pull/24118

Host: Xekyo  -  PR author: Xekyo

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.

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. When would you use sendall and when SFFA?

  3. A comment suggested that this RPC is potentially unsafe to have. Do you agree with this, and why?

  4. Why are send_max and inputs exclusive options?

  5. Why is sendall restricted to spend only confirmed UTXOs?

  6. Why are there two ways of specifying recipients?

  7. Looking at the cleanup decorator in the tests, can you guess where in the codebase sendall may find use?

  8. How are the fee estimation instructions up for interpretation?

Meeting Log

  117:00 <Murch> #startmeeting
  217:00 <svav> Hi
  317:00 <Murch> Welcome to PR review club. Today we're looking at #24118: Add 'sendall' RPC née sweep
  417:00 <brunoerg> Hi
  517:00 <ls55> Hi
  617:00 <lightlike> hi
  717:00 <hernanmarino_> Hi
  817:00 <kouloumos> hi
  917:01 <Murch> Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?
 1017:01 <achow101> hi
 1117:01 <larryruane> hi
 1217:02 <B_1o1> hi all
 1317:02 <svav> No but I read the notes
 1417:02 <Murch> Also, a reminder: if you have any questions of comments, you don't need to ask to say anything, just go right ahead! :)
 1517:02 <michaelfolkson> hi
 1617:02 <otech> hello
 1717:02 <brunoerg> Concept ACK
 1817:02 <larryruane> compiled, ran tests, concept ACK, haven't review code much
 1917:02 <lightlike> didnt review the code much, but I tried it out on regtest.
 2017:03 <Murch> Alright, would someone like to give a one sentence summary of what the PR is trying to achieve?
 2117:03 <larryruane> I like how the functional tests log what they're doing!
 2217:03 <larryruane> clean out a wallet? :)
 2317:03 <josibake> hi
 2417:03 <Murch> Thanks, larryruane
 2517:04 <hernanmarino_> Concept ACK here, no time to review this week
 2617:04 <Murch> Yes, it adds a new PR that can be used to send all funds from the wallet to a destination.
 2717:04 <josibake> Concept ACK (read the notes, ran tests, light code review)
 2817:04 <ls55> Compiled, tested and tACK
 2917:04 <Murch> Sweet, thanks for all the review :)
 3017:04 <svav> Provide a way to clear a wallet after the previous way was broken
 3117:04 <Murch> svav: Good point, how was the previous way broken?
 3217:05 <ls55> Which is the previous way ?
 3317:06 <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
 3417:06 <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"
 3517:06 <hernanmarino_> afaik previous changes attempting SFFA made it imposible to include some inputs, if i understood correctly
 3617:07 <brunoerg> hernanmarino_: because of negative values?
 3717:07 <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
 3817:07 <Murch> It would lead to cases where you wouldn't be able to select an amount as large as the full balance
 3917:08 <hernanmarino_> brunoerg: yes, i believe that's the case
 4017:09 <Murch> Okay, so now that we're introducing `sendall`, why do we still have `SFFO`?
 4117:09 <Murch> When would you use one and when the other?
 4217:09 <josibake> might be a silly question but what is the difference between SFFA and SFFO?
 4317:09 <hernanmarino_> sendall, when you want to empty your wallet
 4417:10 <achow101> josibake: there is none. we just hate naming things consistently
 4517:10 <Murch> josibake: Not a silly question, but it's actually the same, but had like three or four names on different RPCs
 4617:10 <josibake> Murch, achow101: lol nice
 4717:10 <brunoerg> lol
 4817:11 <Murch> hernanmarino: Yes, although we also permit specifying a set of UTXOs to be used as inputs specifically
 4917:11 <josibake> id still want SFF[A,O], if i want to fully spend a UTXO
 5017:11 <josibake> e.g sending a UTXO from an old address type to a new address type
 5117:11 <hernanmarino_> Murch: great, didn't notice that
 5217:11 <Murch> I'd actually suggest to use `sendall` then ^^
 5317:11 <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)?
 5417:11 <Murch> Although you can do it by crafting a raw transaction directly of course
 5517:12 <Murch> larryruane: Yes, paying yourself can play a role.
 5617:12 <Murch> I wouldn't say that's the best way to delineate it, though
 5717:12 <ls55> Murch: Thanks
 5817:13 <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.
 5917:13 <Murch> ls55: Right!
 6017:13 <lightlike> SFFO if I want to pay up to a given max amount or with the recipient paying for the fees, but I don't want to micromanage UTXOs
 6117:13 <larryruane> with SFFO, the wallet is doing the coin selection? whereas with `sendall`, it's being told which UTXOs to use?
 6217:13 <Murch> Also, `sendall` will never create a change output, while SFFO will totally do that
 6317:13 <Murch> lightlike: Yes!
 6417:13 <otech> (y)
 6517:14 <Murch> larryruane: Correct
 6617:14 <hernanmarino_> Murch: your last point is interesting
 6717:14 <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.
 6817:15 <larryruane> I guess we wouldn't want to create a change output if we're trying to empty out the wallet! (?)
 6917:15 <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 ^^
 7017:15 <Murch> larryruane: Right!
 7117:15 <josibake> interesting, i didnt realize SFFO can be used with a change output, but that makes sense
 7217:15 <Murch> Okay, now that we know what was the previous toolset and what we have now:
 7317:15 <Murch> A comment suggested that this RPC is potentially unsafe to have. Do you agree with this, and why?
 7417:16 <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[…]."
 7517:16 <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.
 7617:17 <brunoerg> Yes, I agree
 7717:17 <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.
 7817:17 <brunoerg> ls55: +1
 7917:17 <hernanmarino_> I personally do not thinks this is a big concern ..
 8017:17 <josibake> lightlike: great point
 8117:19 <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
 8217:19 <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
 8317:19 <larryruane> maybe the user could specify a maximum amount, and the RPC fails if this is exceeded?
 8417:19 <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 ^^
 8517:19 <larryruane> (i.e. a sanity check)
 8617:19 <josibake> whereas the examples here are more pointing out it can be a potential foot gun
 8717:19 <achow101> It is a valid concern, but I don't think that it's something we need to address
 8817:20 <brunoerg> maybe it's a dangerous command, not unsafe
 8917:20 <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
 9017:20 <achow101> I think the RPC name is also pretty obvious as what it does
 9117:20 <achow101> moreso than the previous name of sweep
 9217:21 <josibake> achow101: +1, the name already implies use with caution
 9317:21 <B_1o1> brunoerg: +1
 9417:21 <larryruane> achow101: yes, or we could even rename it `sendallandwereallydomeanall`
 9517:21 <larryruane> (JK)
 9617:21 <Murch> haha
 9717:22 <Murch> Randomly need to respond to a call back with "yes", "yes, I'm sure" "go ahead already" :p
 9817:22 <Murch> okay, moving on
 9917:22 <hernanmarino_> :P
10017:22 <Murch> Why are send_max and inputs exclusive options?
10117:22 <ls55> achow101: +1
10217:23 <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.
10317:24 <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.
10417:24 <Murch> So we choose to interpret specifying both as an "unclear" and don't proceed
10517:25 <otech> Makes sense
10617:25 <Murch> Why is sendall restricted to spend only confirmed UTXOs?
10717:27 <ls55> Because it is safer, I think. `wallet::CachedTxIsTrusted()` validates this (if UTXOs are confirmed).
10817:27 <josibake> you don't have to worry about complexity with replacable tx's if you only spend confirmed
10917:28 <Murch> Yep, that's part of it
11017:29 <ls55> in the case of Bitcoin Core, confirmed means >= 1 and not >= 6, correct?
11117:29 <josibake> also (forgot to check), confirmed means 1 conf in this context? iirc `AvailableCoins` first prefers 6 conf and then relaxes to 1
11217:29 <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
11317:30 <achow101> josibake: we can set our own confirmation requirements. iirc it's 1 for sendall
11417:30 <Murch> Yes, since we're trying to clear out the wallet completely, we relax it to anything confirmed
11517:30 <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
11617:30 <achow101> AvailableCoins itself does not filter for number of confirmations
11717:31 <Murch> If you want to empty it, why are you still receiving new funds to it?
11817:31 <ls55> Murch: But why would this (low feerates) be a problem in `sendall` but not in the other `send..` commands?
11917:31 <larryruane> "... does not bump low feerate parents automagically yet" -- is that what package relay will solve?
12017:31 <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
12117:32 <josibake> achow101: you're right, my bad, its later `AttemptSelection`
12217:32 <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
12317:32 <ls55> achow101: I think `AvailableCoins` calls `wallet::CachedTxIsTrusted()` which filters for number of confirmations, doesn't it ?
12417:33 <B_1o1> Murch: is this RPC change also aimed to help reduce the UXTO set?
12517:33 <achow101> It will check for at least 1 confirmation. But it can also be told to allow untrusted utxos
12617:33 <Murch> larryruane: https://github.com/Xekyo/bitcoin/commit/c50030817637356cbef79e41bc702bdb7c3c0363 <- this may help with that, though :)
12717:33 <Murch> Eventually
12817:34 <josibake> ls55: i think it just checks for negative nDepth, which filters out conflicts
12917:34 <Murch> B_1o1: How do you mean?
13017:34 <Murch> It could be used to create a consolidation transaction to yourself, if that's what you mean, yes.
13117:35 <Murch> Why are there two ways of specifying recipients on `sendall`?
13217:36 <B_1o1> Murch: I meant by being able to sweep uneconomic UTXOs and consolidate
13317:36 <Murch> I see
13417:36 <Murch> Bitcoin Core is already somewhat aggressively spending uneconomic UTXOs by default, so I don't think this PR will have a big impact on that
13517:37 <lightlike> to allow for the use case of doing a payment with some part, and cleaning out the rest?
13617:37 <larryruane> "Why are there two ways of specifying recipients on `sendall`?" -- because you may or may not want to specify an amount?
13717:37 <Murch> lightlike: Correct
13817:37 <ls55> josibake:
13917:37 <ls55> ``
14017:37 <ls55> bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents)
14117:37 <ls55> {
14217:37 <ls55> AssertLockHeld(wallet.cs_wallet);
14317:37 <ls55> int nDepth = wallet.GetTxDepthInMainChain(wtx);
14417:37 <ls55> if (nDepth >= 1) return true;
14517:37 <ls55> if (nDepth < 0) return false;
14617:37 <ls55> ``
14717:37 <ls55> I think it accepts nDepth >= 1
14817:38 <ls55> I think `sendall` does not call `AttemptSelection` at any point
14917:38 <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
15017:38 <Murch> ls55: That is correct
15117:38 <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?
15217:39 <Murch> Since `sendall` already knows the UTXOs it will spend (either all, all economic, or a given set), we don't need to run coin selection.
15317:39 <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?
15417:39 <Murch> lightlike: E.g. if you always sweep your wallet at the end of the week between two business partners, or similar
15517:40 <Murch> larryruane: Aha, may I turn that into the next question?
15617:40 <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?
15717:41 <larryruane> sorry can you explain what the cleanup decorator is? I don't know that term
15817:41 <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
15917:42 <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
16017:42 <larryruane> oh i see! some more python for me to learn :)
16117:42 <ls55> I am not sure I understand this question. Aside from `sendall` itself, the only RPC that exists in def cleanup` is `getbalances`.
16217:42 <larryruane> ls55: it's in the python test
16317:42 <Murch> https://github.com/bitcoin/bitcoin/pull/24118/files#diff-904d2e2d19041ffe0de3d038df31dc4cbb7a548f461c96333cd3a5486eaf50d2R17
16417:43 <josibake> seems like this rpc could be really useful in functional testing
16517:43 <Murch> ls55: Sorry, what I mean is, where else might be an RPC to empty wallets useful?
16617:43 <Murch> josibake: Bingo
16717:43 <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.
16817:43 <Murch> :D
16917:44 <josibake> lightlike: ++1
17017:44 <larryruane> may be too much to explain here, murch, but what are those `@cleanup` doing? (it's okay if too much to say)
17117:44 <ls55> lightlike: ++1
17217:44 <Murch> It's a decorator that gets called after the corresponding function that it's on
17317:44 <achow101> we happen to have a few tests that use sffo to empty a wallet. sendall would be a suitable replacement for them
17417:45 <larryruane> achow101: good idea, would that be better as a separate PR or included in this one?
17517:45 <Murch> It is a convenient way of defining an "after()" for all the tests, because we don't have that in the Bitcoin Core python testing framework
17617:45 <otech> (y) (y) (y)
17717:45 <Murch> achow101 has been very supportive of this PR because he wants to use it to fix a bunch of the tests ^^
17817:46 <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
17917:46 <Murch> larryruane: it'll be a separate PR, though
18017:46 <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
18117:46 <otech> Thanks for explanation of the cleanup decorator... adds learning value to this review club !
18217:47 <Murch> How are the fee estimation instructions up for interpretation?
18317:47 <ls55> `conf_target`, `estimate_mode` and `fee_rate` ?
18417:47 <Murch> (This refers to one of the extracted functions in an earlier commit)
18517:47 <Murch> ls55: yes, go on
18617:47 <B_1o1> otech: +1
18717:49 <Murch> I'm referring to `static void InterpretFeeEstimationInstructions(const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, UniValue& options)`
18817:49 <larryruane> Murch: where are those instructions? in the `sendall` help?
18917:50 <Murch> Okay, sorry my question isn't greatly phrased
19017:50 <Murch> What I mean is, why do we have an extra function to figure out what feerate the user wants us to use?
19117:50 <Murch> https://github.com/bitcoin/bitcoin/pull/24118/files#diff-26141d9c7da21eeb4b9e3ffedfaad83212d4710a9e62888f7abea076ca1d0538R57
19217:51 <larryruane> factored out because of use by both `send` and `sendall`! brilliant!
19317:51 <Murch> thanks :)
19417:52 <larryruane> BTW I love how the commits are organized, refactorings in separate commits, excellent lesson for us all
19517:52 <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
19617:53 <Murch> if you go with the latter, you also may specify the estimation mode, which can be either `CONSERVATIVE` or `ECONOMICAL`
19717:53 <Murch> Now, even worse, you can provide these either as positional arguments or as options
19817:53 <ls55> "the commits are organized, refactorings in separate commits" that is great and make the reviews easier
19917:53 <Murch> So, we need to figure out what the user passed, that no conflicting information was provided and what feerate to take from it.
20017:54 <Murch> thanks larryruane and ls55
20117:54 <Murch> So, this was the questions I had prepared. Any more comments or questions?
20217:55 <otech> Why are these arguments located here: https://github.com/bitcoin/bitcoin/pull/24118/files#diff-26141d9c7da21eeb4b9e3ffedfaad83212d4710a9e62888f7abea076ca1d0538R1270-R1273
20317:55 <otech> But then only `fee_rate` is located here: https://github.com/bitcoin/bitcoin/pull/24118/files#diff-26141d9c7da21eeb4b9e3ffedfaad83212d4710a9e62888f7abea076ca1d0538R1279
20417:56 <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?
20517:57 <larryruane> Murch: one nit (i could leave as a PR comment), at the beginning of the description, i think `fSubtractFeeAmount` should be `fSubtractFeeFromAmount`?
20617:58 <larryruane> (i looked up that symbol in my trusty vscode and couldn't find it!)
20717:58 <achow101> otech: conf_target and estimate_mode are in FundTxDoc
20817:58 <Murch> otech: good catch
20917:58 <Murch> larryruane: You are correct, thanks
21017:58 <Murch> will apply
21117:59 <achow101> josibake: we had discussed how that would look in e.g. send, but the api always came out kind of ugly.
21217:59 <Murch> josibake: it's not clear to me how you would achieve the same with a new parameter
21317:59 <otech> achow101 ah yes I see it thanks https://github.com/bitcoin/bitcoin/pull/24118/files#diff-26141d9c7da21eeb4b9e3ffedfaad83212d4710a9e62888f7abea076ca1d0538R456
21417:59 <achow101> it would be something like either checking that the amount == balance, or use some magic value for the amount
21518:00 <Murch> like being allowed to pass "EVERYTHING" for the amount of an output?
21618:00 <Murch> Okay, that's time! Thanks for coming
21718:00 <Murch> #endmeeting