--- 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` -- 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() 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 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` 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 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