Wallet test: Add test for subtract fee from recipient behavior (
tests, wallet) Jul 28, 2021
The PR branch HEAD was fe6dc76b at the time of this review club meeting.
CreateTransaction() function constructs a transaction based on a list of
CRecipient objects. We can
think of the resulting transaction as a redistribution of the input coins to new owners in three
different categories (not all are mandatory):
Recipients: Outputs are created for each of the recipients specified when creating the transaction.
Miner: The miner can claim the difference between the transaction’s inputs and outputs as part of their
mining reward. While it’s possible to create a transaction with no fee, miners are less likely to
mine it and Bitcoin Core nodes won’t accept it into their mempools.
Self: The wallet might create a change output back to itself if the inputs exceed the amount needed for
the payment(s) and fees. This output isn’t necessarily present in every transaction.
Before selecting inputs, the wallet calculates a
target amount based on the total payment
amount(s) and fees. If a
fSubtractFeeAmount=true, the fee is deducted from the
payment, and thus included in the target amount instead of added to it.
If a change output would be dust (i.e. it’s not economical to create and spend the output because the
fee is higher than the amount), it is “dropped” and absorbed into one of the other payments.
The expected behavior is to put it back into the recipient output(s) rather than giving it to the miner.
Did you review the PR?
Concept ACK, approach ACK, tested ACK, or NACK?
The commit message for
notes “no change in behavior.” How might your review strategy differ based on whether a commit is
supposed to change behavior?
What does the
CreateSyncedWallet() function do? Are there any other places where it could be
What does it mean to “subtract fee from recipient” when creating a transaction?
What behavior that “might have recently changed in #17331” is being tested in
TestChain100Setup do for us? Why is it needed in
Why is there an extra
:: in front of
::) is called a scope resolution operator). Why are
enclosed in their own scope?
What is the value of
fee set in
What exactly does
the local variable,
std::unique_ptr<CWallet> wallet, by reference, so that it can be used in the lambda function. Why
is this capture by reference instead of by value? Hint: to capture the variable
var by value, the
capture clause (also known as lambda introducer) must be
[var] instead of
Can you think of any other test cases that should be added?
1 17:00 <glozow> #startmeeting
3 17:00 <glozow> welcome to PR Review Club! :D
4 17:00 <dopedsilicon> Hiiiiii
6 17:00 <jomsox> hello everybody!
10 17:00 <stickies-v> hi everyone!
11 17:00 <glozow> we're* looking at PR #22155 Wallet test: Add test for subtract fee from recipient behavior today
17 17:01 <glozow> Did anybody get a chance to review the PR? y/n
20 17:01 <glozow> o, is it anybody's first time?
21 17:02 <stickies-v> n (only partially - will mostly be listening/learning)
22 17:02 <merkle_noob[m]> Hello everyone.
24 17:02 <Azorcode> Hello Guys
26 17:02 <absently> hello shadowy super coders
28 17:03 <glozow> jarolrod: thank you
29 17:03 <merkle_noob[m]> It's my first time joining in early😅
30 17:03 <glozow> i think the review club website is displaying it with s, that's not the first time i've pasted a bad link :O
31 17:03 <glozow> merkle_noob[m]: welcome!
32 17:04 <glozow> let's start reviewing this PR together :) The commit message for the first commit notes "no change in behavior." How might your review strategy differ based on whether a commit is supposed to change behavior?
33 17:04 <larryruane> review: n (only very little)
34 17:04 <absently> I don't know why my pr comparing script hasn't worked for this PR ?:| git diff HEAD $(git merge-base HEAD master)
35 17:04 <merkle_noob[m]> glozow: Thanks! I hope to learn a lot today🙏
36 17:05 <jnewbery> absently: it got merged this morning, so all the commits are also in master
37 17:05 <glozow> absently: maybe your local master is behind?
38 17:05 <absently> jnewbery ah that would do it!
39 17:05 <glozow> oh right, there wouldn't be a diff if it's in master
40 17:05 <josibake> hi, (sorry a lil late)
41 17:05 <jnewbery> (but that's no reason not to review the PR!)
42 17:06 <absently> it's a handy little script (when it works ;] )
43 17:06 <raj_> absently, you should get the diffs if you compare by commit hashes.
44 17:06 <stickies-v> If a commit claims to not change behaviour, I would focus more on ensuring it actually doesn't. For behaviour changing commits, I think it's important to focus more on potential new vulnerabilities because of the change
45 17:06 <murch> glozow: I would focus more looking on how it improves the existing behavior instead of considering for each line how it might break something in the first pass
46 17:06 <glozow> stickies-v: great answer!
47 17:06 <theStack> for refactoring or "no change in behavior" commits, it's often helpful to pass extra arguments to view the diff, to verify it's move-only... like e.g. --move-colored or --ignore-space-change
50 17:07 <larryruane> theStack: +1
51 17:07 <glozow> murch: theStack: yeah definitely
52 17:07 <biteskola> hi! nice to be here!
53 17:07 <glozow> i like --color-moved=dimmed_zebra
54 17:07 <glozow> (if it's a moveonly)
55 17:08 <larryruane> is there a reason not to always use those diff options?
56 17:08 <raj_> also can we expect no-behaviour change shouldn't fail any functional test?
57 17:08 <josibake> are move only and "no change in behavior" the same thing?
58 17:08 <sipa> josibake: no
59 17:08 <glozow> larryruane: i guess sometimes whitespace affects the code, e.g. in python
60 17:08 <sipa> there are refactors possible that don't change behavior but possibly substantially change the code
61 17:08 <theStack> larryruane: hm i could image e.g. within strings spacing could be important
62 17:09 <larryruane> josibake: you could replace a linear search with a tree search, and that wouldn't be move-only
63 17:09 <murch> josibake: No!
64 17:09 <sipa> josibake: move-only commits are just a subset of no-behavior-change ones (and a subset that's particularly easy to review)
65 17:09 <sipa> even just comments/documentation changes are not move-only
66 17:09 <larryruane> well I guess, is performance change a no-behavior change??
67 17:09 <jnewbery> raj_: functional tests should *always* pass on all commits
68 17:10 <sipa> larryruane: debatable; i'd call it no observable behavior change :)
69 17:10 <murch> raj_: I think that the expectation is that every commit should pass all tests
70 17:10 <raj_> jnewbery, murch ah silly me..
71 17:10 <glozow> i agree^
72 17:10 <sipa> easiest approach: first delete all the tests *hides*
73 17:11 <jnewbery> step two: delete all the code
74 17:11 <jnewbery> no bugs
76 17:11 <b10c> no review club either :(
78 17:11 <glozow> we can review remove-only PRs
79 17:11 <dopedsilicon> :(
80 17:11 <jnewbery> b10c: ah, good point. Let's not do that then
81 17:12 <glozow> ok next question: What does the `CreateSyncedWallet()` function do? Are there any other places where it could be reused?
82 17:12 <merkle_noob[m]> So if I understand correctly, an analogy could be like breaking a large class into a set of small classes/interfaces, etc while ensuring that the code behaves the same way functionally... Please correct me ifI'm wrong.
83 17:13 <glozow> merkle_noob[m]: yeah, that's probably an example of a no-behavior-change change
84 17:14 <S3RK> It creates a new CWallet with mock db and syncs it to the test chain tip
85 17:16 <glozow> merkle_noob[m]: maybe somewhat relevant. in bitcoin core, i've seen a lot of PRs that first do a bunch of refactors, then 1-2 commits changing behavior and it makes stuff much easier to review
86 17:16 <glozow> S3RK: yep!
87 17:16 <stickies-v> S3RK: arguably it's not really the CreateSyncedWallet() function that does the mocking and syncing though, if my understanding is correct?
88 17:17 <S3RK> yes, it calls other funcs to achieve that :)
89 17:18 <glozow> next question: What does it mean to "subtract fee from recipient" when creating a transaction?
90 17:18 <S3RK> not sure about the second part of the question though. Maybe it could be reused in other test modules?
91 17:18 <merkle_noob[m]> glozow: I see... Thanks for the info...
92 17:19 <larryruane> `CreateSyncedWallet()` returns a `std::unique_ptr<CWallet>` -- is my understanding correct that this is similar to a `new` (allocates memory) but is somehow preferrable?
93 17:19 <svav> It means the recipient pays the fee, so it's deducted from the transaction amount that they were going to receive.
94 17:19 <raj_> it means the recipient pays for the fee.. noob question: is this always true?
95 17:19 <larryruane> (oh sorry, we had moved on)
96 17:19 <glozow> larryruane: no worries, everyone should feel free to ask any question at any time
97 17:20 <S3RK> raj_ it's not always the case
98 17:20 <josibake> S3RK that was my understanding, that it's a utility to use any time you want a .. synced wallet, to avoid repeating the calls to the other functions
99 17:20 <murch> glozow: The recipient amount that's specified in the transaction amount is reduced by the amount of fees the transaction pays. If there are multiple outputs with this instruction the fee is distributed equally among them (iirc).
100 17:20 <raj_> Oh ya.. CRecipient it has a bool flag to decide that..
101 17:20 <theStack> when sending amount n and a txfee fee, the recipient receives (n - fee). normally the recipient would receive n and the fee is deducted from the sender
102 17:21 <murch> raj_: It should be false by default
103 17:21 <sipa> larryruane: you know what a unique_ptr does?
104 17:21 <stickies-v> S3RK: sorry had a closer look, you're absolutely right about the mocking and syncing
105 17:21 <glozow> make_unique will allocate it in dynamic memory (like `new` but not exactly the same thing) and return a `std::unique_ptr` which "owns" that piece of memory and will handle releasing it when it goes out of scope
106 17:22 <glozow> murch: theStack: raj_: good answers
107 17:22 <glozow> followup question: what happens if there are multiple recipients in the tx?
108 17:22 <larryruane> glozow: I see, that's definitely better (in general) prevents memory leaks
110 17:22 <raj_> thanks murch , yes we are turning it on in the test..
111 17:23 <sipa> glozow: make_unique pretty just calls new under the hood and feeds it to the unique_ptr constructor
112 17:23 <raj_> glozow, it should distribute the extra equally?
113 17:23 <larryruane> sipa: I think the `unique_ptr` class prevents copying the pointer, so there's no need to keep a reference count (IIUC)
114 17:23 <sipa> larryruane: that's the difference with shared_ptr
115 17:24 <jnewbery> larryruane: if you like learning from books, I'd stronly recommend Effective Modern C++ by Meyers. There are a few chapters in there about smart pointers (unique_ptr and shared_ptr)
116 17:24 <sipa> raw pointers don't have any management; you're responsible for cleaning them up yourself
117 17:24 <larryruane> jnewbery: sipa: thanks, will do
118 17:24 <glozow> raj_: yeah. wonder where that code is
119 17:24 <jnewbery> (although std::make_unique<T>() wasn't introduced until C++14, so it's not covered in that book)
121 17:25 <sipa> larryruane: a unique_ptr is really just a wrapper around a raw pointer in practice, but it (a) prevents copying as you say and (b) automatically destroys the object when the unique_ptr goes out of scope, so you don't need to worry about calling free yourself - it's said that the unique_ptr "owns" the pointer
122 17:25 <glozow> jnewbery: no, i'm pretty sure it's covered
123 17:25 <glozow> that's the book with the peacock on the cover right? there's a chapter on `new` vs `make_shared` i think
124 17:26 <sipa> glozow: make_shared is in c++11; make_unique is not
125 17:26 <absently> sipa what did you mean by "calling free yourself"?
126 17:26 <sipa> absently: i'm wrong; i meant calling "delete" yourself
127 17:26 <jnewbery> glozow: ah ok, I'm sure there are some subsequent changes to smart pointers that weren't available when that book was published. Can't remember exactly what
128 17:26 <larryruane> glozow: you're right, beginning on page 118
129 17:26 <glozow> larryruane: hohoho
130 17:27 <sipa> (make_shared is also a lot more interesting than make_unique; you can't implement make_shared with the same efficiency yourself; make_unique is literally just new + unique_ptr constructor)
131 17:27 <jnewbery> I've been shown up in my knowledge of the Effective C++ books 😳
132 17:28 <murch> jnewbery: Next someone will beat you at Carcasonne
133 17:28 <merkle_noob[m]> glozow: So based on the code, it does fee subtraction equally for all recipients.
134 17:28 <glozow> merkle_noob[m]: yep
135 17:28 <glozow> next question: What behavior that "might have recently changed in #17331" is being tested in spend_tests.cpp?
136 17:29 <josibake> glozow: if im reading the code correctly, cant the first recipient end up paying slightly more?
137 17:29 <glozow> or, what exactly is spent_tests testing?
138 17:29 <merkle_noob[m]> glozow: I was instead thinking that it calculated the fee based on the amount sent to each recipient, and then carried out fee subtraction.
139 17:30 <jnewbery> sipa: I'm not sure I'd say that the unique_ptr "owns" the pointer, rather that the unique_ptr "owns" the object that the pointer points to (ie is responsible for its lifetime and releasing resources when it's no longer needed)
140 17:30 <glozow> josibake: right, any remainder is paid by the first recipient
141 17:30 <raj_> glozow, the test is ensuring that dust changes are added to the recipient, not in fee.. Although I am not sure if thats something that was changed in #17331
143 17:30 <glozow> was hosted by murch
144 17:30 <sipa> jnewbery: fair point
145 17:32 <glozow> raj_: right, what is "dust change" ? :)
146 17:33 <raj_> glozow, a change that is uneconomical to spend.
147 17:33 <murch> When the excess of the input selection beyond the sum of recipient outputs and fees is smaller than the cost of creating a change output
148 17:33 <absently> glozow change that is below a threshold
149 17:33 <murch> WEll, actually smaller than creating and spending the change
150 17:33 <glozow> raj_: right, so we wanted to make a change output, but then we realized it was such a tiny amount that it would cost more to spend it
151 17:34 <glozow> so we decide we're not going to make the change output afterall
152 17:34 <glozow> what happens if we just drop the output? who gets that money?
153 17:35 <raj_> follow up question, the dust amount in test is 123, is it just random?
154 17:35 <stickies-v> glozow: the miner does
155 17:35 <glozow> stickies-v: correct
156 17:35 <glozow> is there a better way to allocate those funds?
157 17:36 <larryruane> could conceivably burn it, then it would go back to everyone
158 17:36 <glozow> (in a tx where we're subtracting fees from recipients)
159 17:36 <larryruane> (in effect... smaller total supply)
160 17:36 <S3RK> depends on how we define "better" but there are other ways
161 17:36 <theStack> larryruane: interesting idea :)
162 17:37 <glozow> raj_: i believe the 123 is arbitrary
163 17:37 <glozow> well, it's definitely small enough to be dust
164 17:37 <stickies-v> probably we'd prefer the recipient to pay slightly less fees given that we're transacting with them?
165 17:37 <glozow> but i imagine 120 would have been fine too
166 17:37 <glozow> stickies-v: exactly. subtract less from the recipients
167 17:37 <murch> raj_: Usually the dust limit is calculated from `(input vsize + output vsize)*3`, so it seems to be arbitrary
168 17:37 <glozow> that's the behavior being tested here
169 17:38 <glozow> any questions about this?
170 17:38 <murch> larryruane: burning it would require creating an ouptut, tho
171 17:38 <raj_> glozow, in the last test then, we are testing with to_reduce = fee + 123, If 123 is random, I wonder how far we can increase it before the test fails, ie. it creates a change output.
172 17:39 <raj_> it failed at 1000, what should be the bound here?
173 17:39 <raj_> fee is 1340..
174 17:39 <absently> larryruane destroying money/wealth reduces our capacity to express our needs, using the money to induce block production is long-term incentive compatible with bitcoin operation
175 17:40 <glozow> raj_: nice testing!
176 17:40 <glozow> and yeah, 1340 is the answer to q8
177 17:41 <murch> raj_: The dust limit for p2wpkh should be 298 sat/vB, iirc.
178 17:41 <raj_> glozow, Ah sorry for spoiler.. :D
179 17:41 <josibake> similar to how the first recipient gets the most subtracted, couldn't you just give back to the first recipient if there is a dust change?
180 17:41 <larryruane> absently: yes, I'm not saying burning is a good idea, just theoretically possible (but as murch says, that would require an output anyway) ... but many people mistakenly think that destroying money is actual waste, but it is not, like even with fiat, if you burn a $100 bill, you're making everyone else slightly better off
181 17:41 <josibake> seems like it would balance out for the first recipient
182 17:41 <glozow> raj_: not a problem at all :P good testing
183 17:42 <raj_> glozow, murch, does it make sense to test this bound in the test also?
184 17:42 <absently> larryruane seems we have different opinions - that's fine :)
185 17:42 <murch> josibake: I thought the first recipient only pays the remainder additionally if it doesn't cleanly divides by the `n` recipients
186 17:42 <glozow> josibake: i think the first recipient paying remainder is inevitable and a pretty insignificant amount, but when we're refunding we also would want that to be somewhat equal
187 17:43 <murch> glozow: Yeah, tthat's what I was trying to say
188 17:43 <murch> But you put that much more clearly
189 17:44 <glozow> murch: tanks tanks
190 17:44 <josibake> glozow: that makes sense, i wasn't thinking of the relative size of the two. dust could actually be worth quite a bit more which is why redistributing is better?
191 17:44 <murch> raj_: It should be tested where the dust limit is enforced. I don't think it would be good practice to test behavior explicitly here that isn't in the purview of the tested function
192 17:45 <theStack> absently: i don't think a decrease in money supply is a problem at all (i think austrian economists pretty much agree that the total money supply doesn't matter); if it is, we would have a serious problem, lots of private keys will get lost forever
193 17:45 <theStack> (sorry for off-topic :x)
194 17:46 <murch> josibake: If you have three recipients that you divide the fee among, the first will pay up to 2 sats more. But dust will be up to 297 sats even for the most blockweight efficient output type currently used on the network
195 17:46 <larryruane> even satoshi (although i agree not infallable) wrote something about unspendable outputs being a gift to everyone (i don't have a reference handy)
196 17:46 <murch> (Yes, ...)
197 17:46 <glozow> josibake: yeah, the dust here is 123 satoshis. whereas i'm pretty sure if you have 3 recipients, at most the first recipient is paying 2 satoshis extra 🤷
198 17:46 <glozow> murch: oops i said what you said better this time
200 17:47 <glozow> okay i wanna make sure we get to the c++ questions. Why is there an extra :: in front of cs_main?
202 17:47 <josibake> glozow, murch: thanks, real numbers help haha
203 17:47 <raj_> murch, yes that makes sense, but in the last test we are checking that even its ok to over pay the recipient, so it might make sense to check we are overpaying upto the max bound, instead of a random extra.
204 17:48 <larryruane> glozow: does that emphasize it's a global variable, and also keeps it from being confused with an object member?
205 17:48 <larryruane> I don't think there are any object members called `cs_main` so only the first reason applies? I've always wondered this
206 17:48 <raj_> glozow, I always wondered but never dared to ask..
207 17:49 <larryruane> raj_: I DOUBLE DOG dare you! (haha0
208 17:49 <absently> glozow in order to define a function outside a class
209 17:49 <murch> raj_: By using an arbitrary limit rather than the actual number the test remains as good as it is even when the actual number changes
210 17:49 <raj_> random guess, is it because they are defined in the current namespace?
211 17:49 <S3RK> raj_ I think this can make the test more fragile as it makes it dependent on unrelated implementation details
212 17:49 <glozow> `::` is the scope resolution operator
213 17:50 <glozow> we're not defining a function here
214 17:50 <larryruane> murch: Yes, I think we don't want to make tests to fragile, right?
215 17:50 <absently> oh >_<
216 17:50 <murch> raj_: It's also nice to test things with various numbers so you don't end up having some hidden behavior where it only ever works for a specific value
217 17:50 <raj_> murch, larryruane yes that makes sense.. thanks..
219 17:51 <glozow> here, we're inside a local scope and want to clarify that we're using a variable defined outside the scope
220 17:52 <S3RK> is it required tho? are there multiple options to resolve it?
221 17:52 <absently> ah that helps me ty
223 17:52 <glozow> S3RK: I thiiiink it would still work if you removed it
224 17:53 <larryruane> But why do we see `cs_main` in so many places without the `::`?
225 17:53 <glozow> this particular test i mean
226 17:53 <raj_> glozow, that outside scope is which one? The one immediately out or any parent scope of the current scope?
227 17:53 <glozow> tbh i am not sure
228 17:54 <larryruane> is it the case that all _new_ instances of `cs_main` should be `::cs_main`?
229 17:55 <glozow> larryruane: no, i think it depends on the scope of the code
230 17:56 <larryruane> if I may ask one other question as we're close on time (feel free to ignore), why is `check_tx` a lambda, instead of a normal function declared just before the function that calls it? just to reduce its scope to where it's needed, to keep it close to where it's used? I like the idea, just curious about the reason(s)
231 17:57 <glozow> ah good point, lemme ask my favorite question before we run out of time: The lambda check_tx captures the local variable, std::unique_ptr<CWallet> wallet, by reference, so that it can be used in the lambda function. Why is this capture by reference instead of by value?
232 17:57 <raj_> glozow, because it would drop the wallet at return if we passed by value?
233 17:58 <larryruane> glozow: is it because `check_tx` modifies the wallet object?
234 17:58 <glozow> raj_: _can_ we pass the wallet by value?
235 17:58 <larryruane> (also it's more efficient, but that's not so important in test code)
236 17:58 <sipa> because you don't want to copy the entire gargantuan wallet object?
237 17:59 <sipa> also it'd lose the transactiont that was created
238 17:59 <S3RK> larryruane my guess is that it's lambda to confine it to the scope of this particular test and not the whole file which could contain more different tests
239 17:59 <larryruane> (it may not modify the wallet, now that I look at it again)
240 17:59 <larryruane> S3RK: +1
241 18:00 <raj_> glozow, we cant? yes there is an error, but i don't understand what it says..
242 18:00 <glozow> hint: `wallet` is a `std::unique_ptr<CWallet>`
244 18:00 <raj_> ohhh.. right..
245 18:00 <sipa> then it's obviously not possible; i should have checked the code first
247 18:01 <jnewbery> passing by value makes a copy of the thing being passed
248 18:01 <glozow> yep, you can't pass a copy of the unique pointer
249 18:01 <raj_> CreateSyncdWallet returns unique pointer.
250 18:01 <glozow> i imagine that's also why it's a lambda instead of a helper function
251 18:01 <glozow> oh oops we're out of time!
252 18:01 <glozow> #endmeeting
253 18:01 <jnewbery> Right, unique_ptr doesn't have a copy ctor (because if it did it wouldn't be unique!)
254 18:02 <glozow> exactly
256 18:02 <jnewbery> thanks glozow!!
257 18:02 <larryruane> jnewbery: glozow: great answers, thanks
258 18:02 <raj_> thanks glozow for hosting, really great one to dig into, learned a ton..
259 18:02 <glozow> larryruane: i think that's chapter 1 of effective modern c++!
260 18:02 <absently> thanks glozow et al
261 18:02 <theStack> thanks for hosting glozow
262 18:03 <larryruane> ah ok.. thanks glozow this was great! thanks to everyone!
263 18:03 <glozow> also, that lambda can be a `const auto`
264 18:03 <josibake> thanks everyone, still a c++ n00b so this was super helpful
265 18:03 <stickies-v> a lot of new stuff for me today, thanks for hosting this very useful session glozow and everyone else for contributing!
266 18:03 <svav> Thanks glozow and all
267 18:03 <josibake> jnewbery: gonnat grab a copy of effective c++ :)
268 18:03 <S3RK> thank you for hosting!
269 18:03 <jnewbery> josibake: it's a great read :)
270 18:03 <sipa> josibake: make sure it's not a unique_ptr<effective c++>
271 18:03 <glozow> thanks everyone :D glad that people were willing to dig into some c++
272 18:03 <biteskola> thanks! :)
273 18:03 <merkle_noob[m]> Thanks glozow, and to every other person who participated. Learnt a ton...
274 18:04 <larryruane> sipa: 🤣
275 18:04 <josibake> sipa: lol
276 18:04 <murch> Thanks for hosting!
277 18:04 <glozow> sipa: 😂