The PR branch HEAD was fe6dc76b at the time of this review club meeting.
Notes
The 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 CRecipient has 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.
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?
What does the CreateSyncedWallet() function do? Are there any other places where it could be
reused?
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 spend_tests.cpp?
What does TestChain100Setup do for us? Why is it needed in spend_tests.cpp?
Why is there an extra :: in front of cs_mainhere?
(Hint: (::) is called a scope resolution operator). Why are these
lines
enclosed in their own scope?
The lambda check_txcaptures 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 [&var].
Can you think of any other test cases that should be added?
<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?
<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
<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
<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
<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.
<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
<stickies-v> S3RK: arguably it's not really the CreateSyncedWallet() function that does the mocking and syncing though, if my understanding is correct?
<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?
<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
<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).
<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
<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
<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)
<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
<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
<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)
<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.
<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)
<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
<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.
<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
<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
<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
<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?
<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
<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
<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
<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)
<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 đ€·
<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.
<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
<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)
<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?
<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