Add 'sendall' RPC née sweep (
wallet, rpc) Mar 16, 2022
The PR branch HEAD was 93f37c00dd at the time of this review club meeting.
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.
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
Did you review the PR?
Concept ACK, approach ACK, tested ACK, or
When would you use
sendall and when SFFA?
comment suggested that
this RPC is potentially unsafe to have. Do you agree with this, and why?
inputs exclusive options?
sendall restricted to spend only confirmed UTXOs?
Why are there two ways of specifying recipients?
Looking at the cleanup decorator in the tests, can you guess where in the
sendall may find use?
How are the fee estimation instructions up for interpretation?
1 17:00 <Murch> #startmeeting
3 17:00 <Murch> Welcome to PR review club. Today we're looking at #24118: Add 'sendall' RPC née sweep
7 17:00 <hernanmarino_> Hi
9 17:01 <Murch> Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?
13 17:02 <svav> No but I read the notes
14 17: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! :)
15 17:02 <michaelfolkson> hi
17 17:02 <brunoerg> Concept ACK
18 17:02 <larryruane> compiled, ran tests, concept ACK, haven't review code much
19 17:02 <lightlike> didnt review the code much, but I tried it out on regtest.
20 17:03 <Murch> Alright, would someone like to give a one sentence summary of what the PR is trying to achieve?
21 17:03 <larryruane> I like how the functional tests log what they're doing!
22 17:03 <larryruane> clean out a wallet? :)
24 17:03 <Murch> Thanks, larryruane
25 17:04 <hernanmarino_> Concept ACK here, no time to review this week
26 17:04 <Murch> Yes, it adds a new PR that can be used to send all funds from the wallet to a destination.
27 17:04 <josibake> Concept ACK (read the notes, ran tests, light code review)
28 17:04 <ls55> Compiled, tested and tACK
29 17:04 <Murch> Sweet, thanks for all the review :)
30 17:04 <svav> Provide a way to clear a wallet after the previous way was broken
31 17:04 <Murch> svav: Good point, how was the previous way broken?
32 17:05 <ls55> Which is the previous way ?
33 17: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
34 17: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"
35 17:06 <hernanmarino_> afaik previous changes attempting SFFA made it imposible to include some inputs, if i understood correctly
36 17:07 <brunoerg> hernanmarino_: because of negative values?
37 17: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
38 17:07 <Murch> It would lead to cases where you wouldn't be able to select an amount as large as the full balance
39 17:08 <hernanmarino_> brunoerg: yes, i believe that's the case
40 17:09 <Murch> Okay, so now that we're introducing `sendall`, why do we still have `SFFO`?
41 17:09 <Murch> When would you use one and when the other?
42 17:09 <josibake> might be a silly question but what is the difference between SFFA and SFFO?
43 17:09 <hernanmarino_> sendall, when you want to empty your wallet
44 17:10 <achow101> josibake: there is none. we just hate naming things consistently
45 17:10 <Murch> josibake: Not a silly question, but it's actually the same, but had like three or four names on different RPCs
46 17:10 <josibake> Murch, achow101: lol nice
48 17:11 <Murch> hernanmarino: Yes, although we also permit specifying a set of UTXOs to be used as inputs specifically
49 17:11 <josibake> id still want SFF[A,O], if i want to fully spend a UTXO
50 17:11 <josibake> e.g sending a UTXO from an old address type to a new address type
51 17:11 <hernanmarino_> Murch: great, didn't notice that
52 17:11 <Murch> I'd actually suggest to use `sendall` then ^^
53 17: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)?
54 17:11 <Murch> Although you can do it by crafting a raw transaction directly of course
55 17:12 <Murch> larryruane: Yes, paying yourself can play a role.
56 17:12 <Murch> I wouldn't say that's the best way to delineate it, though
57 17:12 <ls55> Murch: Thanks
58 17: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.
59 17:13 <Murch> ls55: Right!
60 17: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
61 17:13 <larryruane> with SFFO, the wallet is doing the coin selection? whereas with `sendall`, it's being told which UTXOs to use?
62 17:13 <Murch> Also, `sendall` will never create a change output, while SFFO will totally do that
63 17:13 <Murch> lightlike: Yes!
65 17:14 <Murch> larryruane: Correct
66 17:14 <hernanmarino_> Murch: your last point is interesting
67 17: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.
68 17:15 <larryruane> I guess we wouldn't want to create a change output if we're trying to empty out the wallet! (?)
69 17: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 ^^
70 17:15 <Murch> larryruane: Right!
71 17:15 <josibake> interesting, i didnt realize SFFO can be used with a change output, but that makes sense
72 17:15 <Murch> Okay, now that we know what was the previous toolset and what we have now:
73 17:15 <Murch> A comment suggested that this RPC is potentially unsafe to have. Do you agree with this, and why?
74 17: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[…]."
75 17: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.
76 17:17 <brunoerg> Yes, I agree
77 17: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.
78 17:17 <brunoerg> ls55: +1
79 17:17 <hernanmarino_> I personally do not thinks this is a big concern ..
80 17:17 <josibake> lightlike: great point
81 17: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
82 17: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
83 17:19 <larryruane> maybe the user could specify a maximum amount, and the RPC fails if this is exceeded?
84 17: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 ^^
85 17:19 <larryruane> (i.e. a sanity check)
86 17:19 <josibake> whereas the examples here are more pointing out it can be a potential foot gun
87 17:19 <achow101> It is a valid concern, but I don't think that it's something we need to address
88 17:20 <brunoerg> maybe it's a dangerous command, not unsafe
89 17: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
90 17:20 <achow101> I think the RPC name is also pretty obvious as what it does
91 17:20 <achow101> moreso than the previous name of sweep
92 17:21 <josibake> achow101: +1, the name already implies use with caution
93 17:21 <B_1o1> brunoerg: +1
94 17:21 <larryruane> achow101: yes, or we could even rename it `sendallandwereallydomeanall`
95 17:21 <larryruane> (JK)
97 17:22 <Murch> Randomly need to respond to a call back with "yes", "yes, I'm sure" "go ahead already" :p
98 17:22 <Murch> okay, moving on
99 17:22 <hernanmarino_> :P
100 17:22 <Murch> Why are send_max and inputs exclusive options?
101 17:22 <ls55> achow101: +1
102 17: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.
103 17: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.
104 17:24 <Murch> So we choose to interpret specifying both as an "unclear" and don't proceed
105 17:25 <otech> Makes sense
106 17:25 <Murch> Why is sendall restricted to spend only confirmed UTXOs?
107 17:27 <ls55> Because it is safer, I think. `wallet::CachedTxIsTrusted()` validates this (if UTXOs are confirmed).
108 17:27 <josibake> you don't have to worry about complexity with replacable tx's if you only spend confirmed
109 17:28 <Murch> Yep, that's part of it
110 17:29 <ls55> in the case of Bitcoin Core, confirmed means >= 1 and not >= 6, correct?
111 17:29 <josibake> also (forgot to check), confirmed means 1 conf in this context? iirc `AvailableCoins` first prefers 6 conf and then relaxes to 1
112 17: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
113 17:30 <achow101> josibake: we can set our own confirmation requirements. iirc it's 1 for sendall
114 17:30 <Murch> Yes, since we're trying to clear out the wallet completely, we relax it to anything confirmed
115 17: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
116 17:30 <achow101> AvailableCoins itself does not filter for number of confirmations
117 17:31 <Murch> If you want to empty it, why are you still receiving new funds to it?
118 17:31 <ls55> Murch: But why would this (low feerates) be a problem in `sendall` but not in the other `send..` commands?
119 17:31 <larryruane> "... does not bump low feerate parents automagically yet" -- is that what package relay will solve?
120 17: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
121 17:32 <josibake> achow101: you're right, my bad, its later `AttemptSelection`
122 17: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
123 17:32 <ls55> achow101: I think `AvailableCoins` calls `wallet::CachedTxIsTrusted()` which filters for number of confirmations, doesn't it ?
124 17:33 <B_1o1> Murch: is this RPC change also aimed to help reduce the UXTO set?
125 17:33 <achow101> It will check for at least 1 confirmation. But it can also be told to allow untrusted utxos
127 17:33 <Murch> Eventually
128 17:34 <josibake> ls55: i think it just checks for negative nDepth, which filters out conflicts
129 17:34 <Murch> B_1o1: How do you mean?
130 17:34 <Murch> It could be used to create a consolidation transaction to yourself, if that's what you mean, yes.
131 17:35 <Murch> Why are there two ways of specifying recipients on `sendall`?
132 17:36 <B_1o1> Murch: I meant by being able to sweep uneconomic UTXOs and consolidate
134 17: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
135 17:37 <lightlike> to allow for the use case of doing a payment with some part, and cleaning out the rest?
136 17:37 <larryruane> "Why are there two ways of specifying recipients on `sendall`?" -- because you may or may not want to specify an amount?
137 17:37 <Murch> lightlike: Correct
138 17:37 <ls55> josibake:
140 17:37 <ls55> bool CachedTxIsTrusted(const CWallet& wallet, const CWalletTx& wtx, std::set<uint256>& trusted_parents)
142 17:37 <ls55> AssertLockHeld(wallet.cs_wallet);
143 17:37 <ls55> int nDepth = wallet.GetTxDepthInMainChain(wtx);
144 17:37 <ls55> if (nDepth >= 1) return true;
145 17:37 <ls55> if (nDepth < 0) return false;
147 17:37 <ls55> I think it accepts nDepth >= 1
148 17:38 <ls55> I think `sendall` does not call `AttemptSelection` at any point
149 17: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
150 17:38 <Murch> ls55: That is correct
151 17: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?
152 17: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.
153 17: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?
154 17:39 <Murch> lightlike: E.g. if you always sweep your wallet at the end of the week between two business partners, or similar
155 17:40 <Murch> larryruane: Aha, may I turn that into the next question?
156 17: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?
157 17:41 <larryruane> sorry can you explain what the cleanup decorator is? I don't know that term
158 17: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
159 17: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
160 17:42 <larryruane> oh i see! some more python for me to learn :)
161 17:42 <ls55> I am not sure I understand this question. Aside from `sendall` itself, the only RPC that exists in def cleanup` is `getbalances`.
162 17:42 <larryruane> ls55: it's in the python test
164 17:43 <josibake> seems like this rpc could be really useful in functional testing
165 17:43 <Murch> ls55: Sorry, what I mean is, where else might be an RPC to empty wallets useful?
166 17:43 <Murch> josibake: Bingo
167 17: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.
169 17:44 <josibake> lightlike: ++1
170 17:44 <larryruane> may be too much to explain here, murch, but what are those `@cleanup` doing? (it's okay if too much to say)
171 17:44 <ls55> lightlike: ++1
172 17:44 <Murch> It's a decorator that gets called after the corresponding function that it's on
173 17:44 <achow101> we happen to have a few tests that use sffo to empty a wallet. sendall would be a suitable replacement for them
174 17:45 <larryruane> achow101: good idea, would that be better as a separate PR or included in this one?
175 17: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
176 17:45 <otech> (y) (y) (y)
177 17:45 <Murch> achow101 has been very supportive of this PR because he wants to use it to fix a bunch of the tests ^^
178 17: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
179 17:46 <Murch> larryruane: it'll be a separate PR, though
180 17: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
181 17:46 <otech> Thanks for explanation of the cleanup decorator... adds learning value to this review club !
182 17:47 <Murch> How are the fee estimation instructions up for interpretation?
183 17:47 <ls55> `conf_target`, `estimate_mode` and `fee_rate` ?
184 17:47 <Murch> (This refers to one of the extracted functions in an earlier commit)
185 17:47 <Murch> ls55: yes, go on
186 17:47 <B_1o1> otech: +1
187 17:49 <Murch> I'm referring to `static void InterpretFeeEstimationInstructions(const UniValue& conf_target, const UniValue& estimate_mode, const UniValue& fee_rate, UniValue& options)`
188 17:49 <larryruane> Murch: where are those instructions? in the `sendall` help?
189 17:50 <Murch> Okay, sorry my question isn't greatly phrased
190 17:50 <Murch> What I mean is, why do we have an extra function to figure out what feerate the user wants us to use?
192 17:51 <larryruane> factored out because of use by both `send` and `sendall`! brilliant!
193 17:51 <Murch> thanks :)
194 17:52 <larryruane> BTW I love how the commits are organized, refactorings in separate commits, excellent lesson for us all
195 17: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
196 17:53 <Murch> if you go with the latter, you also may specify the estimation mode, which can be either `CONSERVATIVE` or `ECONOMICAL`
197 17:53 <Murch> Now, even worse, you can provide these either as positional arguments or as options
198 17:53 <ls55> "the commits are organized, refactorings in separate commits" that is great and make the reviews easier
199 17: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.
200 17:54 <Murch> thanks larryruane and ls55
201 17:54 <Murch> So, this was the questions I had prepared. Any more comments or questions?
204 17: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?
205 17:57 <larryruane> Murch: one nit (i could leave as a PR comment), at the beginning of the description, i think `fSubtractFeeAmount` should be `fSubtractFeeFromAmount`?
206 17:58 <larryruane> (i looked up that symbol in my trusty vscode and couldn't find it!)
207 17:58 <achow101> otech: conf_target and estimate_mode are in FundTxDoc
208 17:58 <Murch> otech: good catch
209 17:58 <Murch> larryruane: You are correct, thanks
210 17:58 <Murch> will apply
211 17:59 <achow101> josibake: we had discussed how that would look in e.g. send, but the api always came out kind of ugly.
212 17:59 <Murch> josibake: it's not clear to me how you would achieve the same with a new parameter
214 17:59 <achow101> it would be something like either checking that the amount == balance, or use some magic value for the amount
215 18:00 <Murch> like being allowed to pass "EVERYTHING" for the amount of an output?
216 18:00 <Murch> Okay, that's time! Thanks for coming
217 18:00 <Murch> #endmeeting