--- Log opened Wed Jul 28 00:00:22 2021
00:54 -!- babasancheti [~babasanch@43.249.232.15] has joined #bitcoin-core-pr-reviews
01:32 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews
01:36 -!- _andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 244 seconds]
03:14 -!- babasancheti [~babasanch@43.249.232.15] has quit [Quit: Client closed]
03:20 -!- b10c [uid500648@id-500648.charlton.irccloud.com] has joined #bitcoin-core-pr-reviews
03:49 -!- promag_ [~promag@188.250.84.129] has joined #bitcoin-core-pr-reviews
03:49 -!- promag [~promag@188.250.84.129] has quit [Read error: Connection reset by peer]
03:49 -!- promag_ [~promag@188.250.84.129] has quit [Read error: Connection reset by peer]
03:50 -!- promag [~promag@188.250.84.129] has joined #bitcoin-core-pr-reviews
03:51 -!- promag_ [~promag@188.250.84.129] has joined #bitcoin-core-pr-reviews
03:51 -!- promag [~promag@188.250.84.129] has quit [Read error: Connection reset by peer]
03:53 -!- promag [~promag@188.250.84.129] has joined #bitcoin-core-pr-reviews
03:53 -!- promag_ [~promag@188.250.84.129] has quit [Read error: Connection reset by peer]
03:54 -!- promag_ [~promag@188.250.84.129] has joined #bitcoin-core-pr-reviews
03:54 -!- promag [~promag@188.250.84.129] has quit [Read error: Connection reset by peer]
03:55 -!- promag [~promag@188.250.84.129] has joined #bitcoin-core-pr-reviews
03:55 -!- promag_ [~promag@188.250.84.129] has quit [Read error: Connection reset by peer]
03:56 -!- promag_ [~promag@188.250.84.129] has joined #bitcoin-core-pr-reviews
03:56 -!- promag [~promag@188.250.84.129] has quit [Read error: Connection reset by peer]
03:58 -!- promag [~promag@188.250.84.129] has joined #bitcoin-core-pr-reviews
03:58 -!- promag_ [~promag@188.250.84.129] has quit [Read error: Connection reset by peer]
04:25 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection]
04:25 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews
05:25 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection]
05:26 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews
07:53 -!- belcher [~belcher@user/belcher] has quit [Ping timeout: 240 seconds]
08:08 -!- prusnak is now known as stick
08:09 -!- prusnak[m] is now known as stick[m]
08:49 -!- nelsongaldeman [~nelsongal@90.214.246.92] has joined #bitcoin-core-pr-reviews
08:51 -!- ngaldeman94 [~ngaldeman@90.214.246.92] has joined #bitcoin-core-pr-reviews
08:51 -!- ngaldeman94 [~ngaldeman@90.214.246.92] has left #bitcoin-core-pr-reviews []
08:55 -!- nelsongaldeman is now known as ngaldeman
09:04 -!- ngaldeman [~nelsongal@90.214.246.92] has quit [Ping timeout: 258 seconds]
09:48 -!- theStack [~honeybadg@vps1648322.vs.webtropia-customer.com] has joined #bitcoin-core-pr-reviews
09:49 -!- dopedsilicon [~dopedsili@103.159.243.27] has joined #bitcoin-core-pr-reviews
09:51 -!- kiran [~kiran@218.185.248.66] has joined #bitcoin-core-pr-reviews
09:55 -!- raj_ [~raj_@103.77.139.179] has joined #bitcoin-core-pr-reviews
09:58 -!- bitcoin_guy [~bitcoin_g@cpe-66-65-99-218.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews
09:59 -!- murch [~murchin@user/murch] has joined #bitcoin-core-pr-reviews
09:59 < murch> hullo
09:59 -!- jomsox [~jomsox@177.249.204.156] has joined #bitcoin-core-pr-reviews
09:59 < glozow> herro
09:59 < raj_> Hello.. 
10:00 < glozow> #startmeeting
10:00 < jnewbery> hiiii
10:00 < glozow> welcome to PR Review Club! :D
10:00 < dopedsilicon> Hiiiiii
10:00 < glozow> We'
10:00 -!- absently [~absently@185.212.170.140] has joined #bitcoin-core-pr-reviews
10:00 < jomsox> hello everybody!
10:00 < absently> hi
10:00 < theStack> hi
10:00 -!- stickies-v [~stickies-@93-142-139-108.adsl.net.t-com.hr] has joined #bitcoin-core-pr-reviews
10:00 < sipa> ohai
10:00 < stickies-v> hi everyone!
10:00 < glozow> we're* looking at PR #22155 Wallet test: Add test for subtract fee from recipient behavior today
10:00 < larryruane> hi
10:00 < murch> Hello
10:01 < glozow> Notes are here: https://bitcoincore.reviews/22155
10:01 < S3RK> hi
10:01 < glozow> PR is here:  https://github.com/bitcoin/bitcoin/pulls/22155 
10:01 < glozow> Did anybody get a chance to review the PR? y/n
10:01 -!- Azorcode [~Azorcode@201.242.208.94] has joined #bitcoin-core-pr-reviews
10:01 < S3RK> y
10:01 < raj_> 0.3y
10:01 < glozow> o, is it anybody's first time?
10:02 < stickies-v> n (only partially - will mostly be listening/learning)
10:02 < merkle_noob[m]> Hello everyone.
10:02 < schmidty> hi
10:02 < Azorcode> Hello Guys 
10:02 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
10:02 < jarolrod> fixed the link: https://github.com/bitcoin/bitcoin/pull/22155
10:02 < absently>  hello shadowy super coders
10:02 < jarolrod> 🥃
10:03 < glozow> jarolrod: thank you
10:03 < merkle_noob[m]> It's my first time joining in early😅
10: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
10:03 -!- Mruga [~Mruga@host-198-90-94-129.public.eastlink.ca] has joined #bitcoin-core-pr-reviews
10:03 < glozow> merkle_noob[m]: welcome!
10: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?
10:04 < larryruane> review: n (only very little)
10:04 -!- biteskola [~biteskola@184.76.76.188.dynamic.jazztel.es] has joined #bitcoin-core-pr-reviews
10:04 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
10: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)
10:04 < merkle_noob[m]> glozow: Thanks! I hope to learn a lot today🙏
10:05 < jnewbery> absently: it got merged this morning, so all the commits are also in master
10:05 < glozow> absently: maybe your local master is behind?
10:05 < absently>  jnewbery ah that would do it!
10:05 < glozow> oh right, there wouldn't be a diff if it's in master
10:05 < josibake> hi, (sorry a lil late)
10:05 < jnewbery> (but that's no reason not to review the PR!)
10:06 < absently>  it's a handy little script (when it works ;] )
10:06 < raj_> absently, you should get the diffs if you compare by commit hashes. 
10: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
10: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
10:06 < glozow> stickies-v: great answer!
10: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
10:06 < svav> Hi
10:07 < b10c> hi
10:07 < larryruane> theStack: +1
10:07 < glozow> murch: theStack: yeah definitely
10:07 < biteskola> hi! nice to be here!
10:07 -!- ngaldeman [~ngaldeman@90.214.246.92] has joined #bitcoin-core-pr-reviews
10:07 < glozow> i like --color-moved=dimmed_zebra
10:07 < glozow> (if it's a moveonly)
10:08 < larryruane> is there a reason not to always use those diff options?
10:08 < raj_> also can we expect no-behaviour change shouldn't fail any functional test? 
10:08 < josibake> are move only and "no change in behavior" the same thing?
10:08 < sipa> josibake: no
10:08 < glozow> larryruane: i guess sometimes whitespace affects the code, e.g. in python
10:08 < sipa> there are refactors possible that don't change behavior but possibly substantially change the code
10:08 < theStack> larryruane: hm i could image e.g. within strings spacing could be important
10:09 < larryruane> josibake: you could replace a linear search with a tree search, and that wouldn't be move-only
10:09 < murch> josibake: No!
10:09 < sipa> josibake: move-only commits are just a subset of no-behavior-change ones (and a subset that's particularly easy to review)
10:09 < sipa> even just comments/documentation changes are not move-only
10:09 -!- dariusp [~dariusp@c-73-252-226-175.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews
10:09 < larryruane> well I guess, is performance change a no-behavior change?? 
10:09 < jnewbery> raj_: functional tests should *always* pass on all commits
10:10 < sipa> larryruane: debatable; i'd call it no observable behavior change :)
10:10 < murch> raj_: I think that the expectation is that every commit should pass all tests
10:10 < raj_> jnewbery, murch ah silly me.. 
10:10 < glozow> i agree^
10:10 < sipa> easiest approach: first delete all the tests *hides*
10:11 < jnewbery> step two: delete all the code
10:11 < jnewbery> no bugs
10:11 < sipa> :D
10:11 < b10c> no review club either :(
10:11 < absently> :C
10:11 < glozow> we can review remove-only PRs
10:11 < dopedsilicon> :(
10:11 < jnewbery> b10c: ah, good point. Let's not do that then
10:12 < glozow> ok next question: What does the `CreateSyncedWallet()` function do? Are there any other places where it could be reused?
10: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.
10:13 < glozow> merkle_noob[m]: yeah, that's probably an example of a no-behavior-change change
10:14 < S3RK> It creates a new CWallet with mock db and syncs it to the test chain tip
10: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
10:16 < glozow> S3RK: yep!
10:16 < stickies-v> S3RK: arguably it's not really the CreateSyncedWallet() function that does the mocking and syncing though, if my understanding is correct?
10:17 < S3RK> yes, it calls other funcs to achieve that :)
10:18 < glozow> next question: What does it mean to "subtract fee from recipient" when creating a transaction?
10:18 < S3RK> not sure about the second part of the question though. Maybe it could be reused in other test modules?
10:18 < merkle_noob[m]> glozow: I see... Thanks for the info...
10: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?
10:19 < svav> It means the recipient pays the fee, so it's deducted from the transaction amount that they were going to receive.
10:19 < raj_> it means the recipient pays for the fee.. noob question: is this always true? 
10:19 < larryruane> (oh sorry, we had moved on)
10:19 < glozow> larryruane: no worries, everyone should feel free to ask any question at any time
10:20 < S3RK> raj_ it's not always the case
10: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
10: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).
10:20 < raj_> Oh ya.. CRecipient it has a bool flag to decide that.. 
10: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
10:21 < murch> raj_: It should be false by default
10:21 < sipa> larryruane: you know what a unique_ptr does?
10:21 < stickies-v> S3RK: sorry had a closer look, you're absolutely right about the mocking and syncing
10: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
10:22 < glozow> murch: theStack: raj_: good answers
10:22 < glozow> followup question: what happens if there are multiple recipients in the tx?
10:22 < larryruane> glozow: I see, that's definitely better (in general) prevents memory leaks
10:22 < glozow> larryruane: https://en.cppreference.com/w/cpp/memory/unique_ptr and we'll discuss it more in a later question :D
10:22 < raj_> thanks murch , yes we are turning it on in the test.. 
10:23 < sipa> glozow: make_unique pretty just calls new under the hood and feeds it to the unique_ptr constructor
10:23 < raj_> glozow, it should distribute the extra equally?  
10:23 < larryruane> sipa: I think the `unique_ptr` class prevents copying the pointer, so there's no need to keep a reference count (IIUC)
10:23 < sipa> larryruane: that's the difference with shared_ptr
10: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)
10:24 < sipa> raw pointers don't have any management; you're responsible for cleaning them up yourself
10:24 < larryruane> jnewbery: sipa: thanks, will do
10:24 < glozow> raj_: yeah. wonder where that code is
10:24 < jnewbery> (although std::make_unique<T>() wasn't introduced until C++14, so it's not covered in that book)
10:25 < glozow> aha: https://github.com/bitcoin/bitcoin/blob/4b1fb50def0dea0cd320bc43c12d9a12edde0390/src/wallet/spend.cpp#L800
10: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
10:25 < glozow> jnewbery: no, i'm pretty sure it's covered
10:25 < glozow> that's the book with the peacock on the cover right? there's a chapter on `new` vs `make_shared` i think
10:25 -!- jacob54 [~jacob@23.105.140.89] has joined #bitcoin-core-pr-reviews
10:26 < sipa> glozow: make_shared is in c++11; make_unique is not
10:26 < absently> sipa what did you mean by "calling free yourself"?
10:26 < sipa> absently: i'm wrong; i meant calling "delete" yourself
10: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
10:26 < larryruane> glozow: you're right, beginning on page 118
10:26 < glozow> larryruane: hohoho
10: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)
10:27 < jnewbery> I've been shown up in my knowledge of the Effective C++ books 😳
10:28 < murch> jnewbery: Next someone will beat you at Carcasonne
10:28 < merkle_noob[m]> glozow: So based on the code, it does fee subtraction equally for all recipients.
10:28 < glozow> merkle_noob[m]: yep
10:28 < glozow> next question: What behavior that "might have recently changed in #17331" is being tested in spend_tests.cpp?
10:29 < josibake> glozow: if im reading the code correctly, cant the first recipient end up paying slightly more?
10:29 < glozow> or, what exactly is spent_tests testing?
10: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.
10: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)
10:30 < glozow> josibake: right, any remainder is paid by the first recipient
10: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
10:30 < glozow> btw, we also did a review club on #17331 if y'all are interested: https://bitcoincore.reviews/17331
10:30 < glozow> was hosted by murch
10:30 < sipa> jnewbery: fair point
10:32 < glozow> raj_: right, what is "dust change" ? :)
10:33 < raj_> glozow, a change that is uneconomical to spend. 
10: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
10:33 < absently>  glozow change that is below a threshold
10:33 < murch> WEll, actually smaller than creating and spending the change
10: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
10:34 < glozow> so we decide we're not going to make the change output afterall
10:34 < glozow> what happens if we just drop the output? who gets that money?
10:35 < raj_> follow up question, the dust amount in test is 123, is it just random?   
10:35 < stickies-v> glozow: the miner does
10:35 < glozow> stickies-v: correct
10:35 < glozow> is there a better way to allocate those funds?
10:36 < larryruane> could conceivably burn it, then it would go back to everyone
10:36 < glozow> (in a tx where we're subtracting fees from recipients)
10:36 < larryruane> (in effect... smaller total supply)
10:36 < S3RK> depends on how we define "better" but there are other ways
10:36 < theStack> larryruane: interesting idea :)
10:37 < glozow> raj_: i believe the 123 is arbitrary
10:37 < glozow> well, it's definitely small enough to be dust
10:37 < stickies-v> probably we'd prefer the recipient to pay slightly less fees given that we're transacting with them?
10:37 < glozow> but i imagine 120 would have been fine too
10:37 < glozow> stickies-v: exactly. subtract less from the recipients
10:37 < murch> raj_: Usually the dust limit is calculated from `(input vsize + output vsize)*3`, so it seems to be arbitrary
10:37 < glozow> that's the behavior being tested here
10:38 < glozow> any questions about this?
10:38 < murch> larryruane: burning it would require creating an ouptut, tho
10: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. 
10:39 < raj_> it failed at 1000, what should be the bound here?  
10:39 < raj_> fee is 1340.. 
10: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
10:40 -!- Pa3 [~Pa@88.184.14.37.dynamic.jazztel.es] has joined #bitcoin-core-pr-reviews
10:40 < glozow> raj_: nice testing!
10:40 < glozow> and yeah, 1340 is the answer to q8
10:41 < murch> raj_: The dust limit for p2wpkh should be 298 sat/vB, iirc.
10:41 < raj_> glozow, Ah sorry for spoiler.. :D
10: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?
10: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
10:41 < josibake> seems like it would balance out for the first recipient
10:41 < glozow> raj_: not a problem at all :P good testing
10:42 < raj_> glozow,  murch, does it make sense to test this bound in the test also?
10:42 < absently> larryruane seems we have different opinions - that's fine :)
10:42 < murch> josibake: I thought the first recipient only pays the remainder additionally if it doesn't cleanly divides by the `n` recipients
10: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
10:43 < murch> glozow: Yeah, tthat's what I was trying to say
10:43 < murch> But you put that much more clearly
10:44 -!- Mruga [~Mruga@host-198-90-94-129.public.eastlink.ca] has quit [Quit: Connection closed]
10:44 < glozow> murch: tanks tanks
10: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?
10: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
10: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
10:45 < theStack> (sorry for off-topic :x)
10: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
10: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)
10:46 < murch> (Yes, ...)
10: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 🤷
10:46 < glozow> murch: oops i said what you said better this time
10:46 < murch> :D
10:47 < glozow> okay i wanna make sure we get to the c++ questions. Why is there an extra :: in front of cs_main?
10:47 < glozow> here: https://github.com/bitcoin/bitcoin/blob/fe6dc76b7c9c5405f37464a3b19fcf82aaf22861/src/wallet/test/util.cpp#L21
10:47 < josibake> glozow, murch: thanks, real numbers help haha
10: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.  
10:48 < larryruane> glozow: does that emphasize it's a global variable, and also keeps it from being confused with an object member?
10: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
10:48 < raj_> glozow, I always wondered but never dared to ask.. 
10:49 < larryruane> raj_: I DOUBLE DOG dare you! (haha0
10:49 < absently> glozow in order to define a function outside a class
10: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
10:49 < raj_> random guess, is it because they are defined in the current namespace?
10:49 < S3RK> raj_ I think this can make the test more fragile as it makes it dependent on unrelated implementation details
10:49 < glozow> `::` is the scope resolution operator
10:50 < glozow> we're not defining a function here
10:50 < larryruane> murch: Yes, I think we don't want to make tests to fragile, right?
10:50 < absently>  oh >_<
10: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
10:50 < raj_> murch, larryruane yes that makes sense.. thanks.. 
10:50 < glozow> defining funciton outside class would be something like this: https://github.com/bitcoin/bitcoin/blob/4b1fb50def0dea0cd320bc43c12d9a12edde0390/src/validation.cpp#L537
10:51 < glozow> here, we're inside a local scope and want to clarify that we're using a variable defined outside the scope
10:52 < S3RK> is it required tho? are there multiple options to resolve it?
10:52 < absently> ah that helps me ty
10:52 < glozow> murch: i agree. i assume that's also why it sets `fOverrideFeeRate=true` https://github.com/bitcoin/bitcoin/pull/22155/files#diff-5a646a2670e34037c595ea495997a0cb4900775bcb677b58d567bb083b579b9bR33
10:52 < glozow> S3RK: I thiiiink it would still work if you removed it
10:53 < larryruane> But why do we see `cs_main` in so many places without the `::`? 
10:53 < glozow> this particular test i mean
10:53 < raj_> glozow, that outside scope is which one? The one immediately out or any parent scope of the current scope? 
10:53 < glozow> tbh i am not sure
10:54 < larryruane> is it the case that all _new_ instances of `cs_main` should be `::cs_main`?
10:55 < glozow> larryruane: no, i think it depends on the scope of the code
10: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)
10: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? 
10:57 < raj_> glozow, because it would drop the wallet at return if we passed by value?
10:58 < larryruane> glozow: is it because `check_tx` modifies the wallet object?
10:58 < glozow> raj_: _can_ we pass the wallet by value?
10:58 < larryruane> (also it's more efficient, but that's not so important in test code)
10:58 < sipa> because you don't want to copy the entire gargantuan wallet object?
10:59 < sipa> also it'd lose the transactiont that was created
10: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
10:59 < larryruane> (it may not modify the wallet, now that I look at it again)
10:59 -!- ngaldeman [~ngaldeman@90.214.246.92] has quit [Ping timeout: 240 seconds]
10:59 -!- bitcoin_guy [~bitcoin_g@cpe-66-65-99-218.nyc.res.rr.com] has quit [Quit: Connection closed]
10:59 < larryruane> S3RK: +1
11:00 < raj_> glozow, we cant? yes there is an error, but i don't understand what it says.. 
11:00 < glozow> hint: `wallet` is a `std::unique_ptr<CWallet>` 
11:00 -!- Polaris1205 [~Polaris12@cpe-24-102-91-203.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews
11:00 < sipa> oh.
11:00 < raj_> ohhh.. right.. 
11:00 < sipa> then it's obviously not possible; i should have checked the code first
11:00 < glozow> teehee
11:01 < jnewbery> passing by value makes a copy of the thing being passed
11:01 < glozow> yep, you can't pass a copy of the unique pointer
11:01 < raj_> CreateSyncdWallet returns unique pointer. 
11:01 < glozow> i imagine that's also why it's a lambda instead of a helper function
11:01 < glozow> oh oops we're out of time!
11:01 < glozow> #endmeeting
11:01 < jnewbery> Right, unique_ptr doesn't have a copy ctor (because if it did it wouldn't be unique!)
11:02 < glozow> exactly
11:02 < larryruane> on line 19 https://github.com/bitcoin/bitcoin/blob/fe6dc76b7c9c5405f37464a3b19fcf82aaf22861/src/wallet/test/util.cpp#L19 the lambda is declared as an `auto`, maybe we can discuss next time, I'm never sure if it's better to use `auto` or write out the type
11:02 < jnewbery> thanks glozow!!
11:02 < larryruane> jnewbery: glozow: great answers, thanks
11:02 < raj_> thanks glozow for hosting, really great one to dig into, learned a ton.. 
11:02 < glozow> larryruane: i think that's chapter 1 of effective modern c++!
11:02 < absently> thanks glozow et al
11:02 < theStack> thanks for hosting glozow
11:03 < larryruane> ah ok.. thanks glozow this was great! thanks to everyone!
11:03 < glozow> also, that lambda can be a `const auto`
11:03 < josibake> thanks everyone, still a c++ n00b so this was super helpful
11:03 < stickies-v> a lot of new stuff for me today, thanks for hosting this very useful session glozow and everyone else for contributing!
11:03 < svav> Thanks glozow and all
11:03 < josibake> jnewbery: gonnat grab a copy of effective c++ :)
11:03 < S3RK> thank you for hosting!
11:03 -!- raj_ [~raj_@103.77.139.179] has quit [Quit: Leaving]
11:03 < jnewbery> josibake: it's a great read :)
11:03 < sipa> josibake: make sure it's not a unique_ptr<effective c++>
11:03 < glozow> thanks everyone :D glad that people were willing to dig into some c++ 
11:03 < biteskola> thanks! :)
11:03 < merkle_noob[m]> Thanks glozow, and to every other person who participated. Learnt a ton...
11:04 < larryruane> sipa: 🤣
11:04 < josibake> sipa: lol
11:04 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
11:04 < murch> Thanks for hosting!
11:04 < glozow> sipa: 😂 
11:05 -!- stickies-v [~stickies-@93-142-139-108.adsl.net.t-com.hr] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
11:05 -!- Azorcode [~Azorcode@201.242.208.94] has quit [Quit: Connection closed]
11:06 < merkle_noob[m]> I had another question, but the discussion around  the dust value made things clear for me...
11:07 -!- jomsox [~jomsox@177.249.204.156] has quit [Quit: Connection closed]
11:08 < murch> merkle_noob[m]: Great :)
11:09 < murch> This channel tends to also be very helpful for other questions around getting to know the codebase or the implemented behavior
11:10 -!- Polaris1205 [~Polaris12@cpe-24-102-91-203.nyc.res.rr.com] has quit [Quit: Connection closed]
11:11 < merkle_noob[m]> murch: My question was on why the the fee subtraction was not based on the amount sent to each recipient, but instead was equal to all recipients...
11:11 -!- tomken [~tomken@adsl-89-217-158-164.adslplus.ch] has joined #bitcoin-core-pr-reviews
11:11 -!- dopedsilicon [~dopedsili@103.159.243.27] has quit [Quit: Connection closed]
11:12 -!- kiran [~kiran@218.185.248.66] has quit [Quit: Connection closed]
11:12 -!- jomsox [~jomsox@177.249.204.156] has joined #bitcoin-core-pr-reviews
11:12 -!- biteskola [~biteskola@184.76.76.188.dynamic.jazztel.es] has quit [Ping timeout: 268 seconds]
11:13 -!- jomsox [~jomsox@177.249.204.156] has quit [Client Quit]
11:15 < merkle_noob[m]> murch: Thanks for the info :)
11:17 -!- rich- [rich@2600:3c00::f03c:92ff:fe8e:dce6] has joined #bitcoin-core-pr-reviews
11:18 -!- belcher [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
11:19 < murch> merkle_noob[m]: I guess it fits better with how fees behave for on-chain transactions in general: fees are relative to the blockspace used, not relative to the amount transferred
11:23 -!- dariusp [~dariusp@c-73-252-226-175.hsd1.ca.comcast.net] has quit [Quit: Ping timeout (120 seconds)]
11:24 < merkle_noob[m]> murch: Oh, I see. I seriously misunderstood then...
11:24 < merkle_noob[m]> I also misunderstood the role of dust, then...
11:26 < murch> I mean, it could totally be done differently, but I'm guessing why the implementer chose to do it this way.
11:26 < murch> It's also easy. :D
11:27 < murch> merkle_noob[m]: What did you mean with your comment on dust? Is there a question lurking? :)
11:27 < merkle_noob[m]> murch: OK, I see😅
11:28 < merkle_noob[m]> murch: I thought that the dust value was part of the reason for the current fee subtraction from several recipients...
11:29 < merkle_noob[m]>  * murch: I thought that the dust value was part of the reason for the current behavior of fee subtraction from several recipients...
11:29 < sipa> fee subtraction is a specific use case; e.g. when you want to move your entire wallet balance, you don't want to go guess how much will remain after the fee, you just want to say "send my entire balance, minus what it costs"
11:30 < sipa> it's not always desired; most commonly, for normal payments, you want the sender to pay the fee
11:30 < sipa> (so they have an incentive to minimize it)
11:31 < merkle_noob[m]> sipa: I see...
11:36 -!- notmandatory [notmandato@2600:3c00::f03c:92ff:fe8e:dce6] has quit [Quit: ZNC 1.7.2+deb3 - https://znc.in]
11:36 -!- notmandatory [~notmandat@shindig.notmandatory.org] has joined #bitcoin-core-pr-reviews
11:36 < merkle_noob[m]> sipa: I was trying to understand the fee subtraction behavior in the case of several recipients...
11:39 -!- rich- is now known as sandipndev
11:40 -!- absently [~absently@185.212.170.140] has left #bitcoin-core-pr-reviews []
11:41 < murch> merkle_noob[m]: One application would be for example to consolidate a ton of small funds, but split them to multiple chunks into the same wallet. E.g. create 3 outputs in a single wallet from 1,000*0.001₿ worth 0.5₿, 0.3₿, and 0.2₿
11:41 < murch> Just because there are multiple recipient outputs doesn't mean they're going to more than one recipient. :)
11:42 < murch> I think some services were also interested in generally making recipients cover fees, but splitting the fees of a transaction batching multiple payments between the multiple recipients is still cheaper than making separate transactions
11:43 -!- sandipndev [rich@2600:3c00::f03c:92ff:fe8e:dce6] has quit [Quit: ZNC 1.7.2+deb3 - https://znc.in]
11:43 -!- babasancheti [~babasanch@43.249.232.15] has joined #bitcoin-core-pr-reviews
11:47 < merkle_noob[m]> murch: OK, now I get it :)
11:59 -!- notmandatory [~notmandat@shindig.notmandatory.org] has quit [Quit: ZNC 1.7.2+deb3 - https://znc.in]
11:59 -!- notmandatory [notmandato@2600:3c00::f03c:92ff:fe8e:dce6] has joined #bitcoin-core-pr-reviews
12:03 -!- jacob54 [~jacob@23.105.140.89] has quit [Quit: Connection closed]
12:03 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection]
12:04 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews
12:08 -!- notmandatory [notmandato@2600:3c00::f03c:92ff:fe8e:dce6] has quit [Quit: ZNC 1.7.2+deb3 - https://znc.in]
12:08 -!- yakshaver [rich@2600:3c00::f03c:92ff:fe8e:dce6] has quit [Quit: ZNC 1.7.2+deb3 - https://znc.in]
12:08 -!- yakshaver [yakshaver@2600:3c00::f03c:92ff:fe8e:dce6] has joined #bitcoin-core-pr-reviews
12:09 -!- notmandatory [~notmandat@shindig.notmandatory.org] has joined #bitcoin-core-pr-reviews
12:14 -!- Pa3 [~Pa@88.184.14.37.dynamic.jazztel.es] has quit [Quit: Client closed]
12:14 -!- babasancheti [~babasanch@43.249.232.15] has quit [Quit: Client closed]
12:15 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
12:17 -!- babasancheti [~babasanch@43.249.232.15] has joined #bitcoin-core-pr-reviews
12:36 -!- murch [~murchin@user/murch] has quit [Quit: WeeChat 2.8]
12:39 -!- babasancheti [~babasanch@43.249.232.15] has quit [Quit: Client closed]
12:45 -!- tomken [~tomken@adsl-89-217-158-164.adslplus.ch] has quit [Quit: Connection closed]
13:40 -!- rage-proof [~rage-proo@ip5f5bf037.dynamic.kabel-deutschland.de] has joined #bitcoin-core-pr-reviews
14:40 -!- rage-proof [~rage-proo@ip5f5bf037.dynamic.kabel-deutschland.de] has quit [Quit: Connection closed]
15:29 -!- promag [~promag@188.250.84.129] has quit [Remote host closed the connection]
15:30 -!- promag [~promag@188.250.84.129] has joined #bitcoin-core-pr-reviews
17:13 -!- belcher_ [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
17:16 -!- belcher [~belcher@user/belcher] has quit [Ping timeout: 245 seconds]
17:43 -!- b10c [uid500648@id-500648.charlton.irccloud.com] has quit [Quit: Connection closed for inactivity]
20:09 -!- thebatzuk [~thebatzuk@189.237.6.142] has joined #bitcoin-core-pr-reviews
20:11 -!- thebatzuk [~thebatzuk@189.237.6.142] has quit [Client Quit]
21:38 -!- babasancheti [~babasanch@43.249.232.15] has joined #bitcoin-core-pr-reviews
22:16 -!- babasancheti [~babasanch@43.249.232.15] has quit [Quit: Client closed]
23:23 -!- belcher_ [~belcher@user/belcher] has quit [Ping timeout: 245 seconds]
23:28 -!- belcher_ [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
--- Log closed Thu Jul 29 00:00:22 2021