--- Day changed Wed Sep 18 2019 00:22 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has quit [Ping timeout: 250 seconds] 01:10 -!- jonatack [~jon@54.76.13.109.rev.sfr.net] has joined #bitcoin-core-pr-reviews 01:31 -!- jonatack [~jon@54.76.13.109.rev.sfr.net] has quit [Ping timeout: 258 seconds] 01:33 -!- jonatack [~jon@37.166.86.165] has joined #bitcoin-core-pr-reviews 02:17 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-aljfsmnzbununyhz] has joined #bitcoin-core-pr-reviews 02:44 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 03:13 -!- jonatack [~jon@37.166.86.165] has quit [Quit: jonatack] 03:14 -!- jonatack [~jon@37.166.86.165] has joined #bitcoin-core-pr-reviews 03:52 -!- jonatack [~jon@37.166.86.165] has quit [Quit: jonatack] 03:53 -!- jonatack [~jon@37.166.86.165] has joined #bitcoin-core-pr-reviews 05:20 -!- jonatack [~jon@37.166.86.165] has quit [Quit: jonatack] 05:21 -!- jonatack [~jon@37.166.86.165] has joined #bitcoin-core-pr-reviews 05:40 -!- jonatack [~jon@37.166.86.165] has quit [Quit: jonatack] 05:41 -!- jonatack [~jon@37.166.86.165] has joined #bitcoin-core-pr-reviews 05:43 -!- jonatack_ [~jon@54.76.13.109.rev.sfr.net] has joined #bitcoin-core-pr-reviews 05:46 -!- jonatack [~jon@37.166.86.165] has quit [Ping timeout: 268 seconds] 06:11 -!- jonatack_ [~jon@54.76.13.109.rev.sfr.net] has quit [Ping timeout: 276 seconds] 07:29 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has joined #bitcoin-core-pr-reviews 07:36 -!- shesek [~shesek@unaffiliated/shesek] has quit [Ping timeout: 240 seconds] 07:41 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection] 07:42 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 07:46 -!- seven__ [~seven@BSN-77-101-62.static.siol.net] has joined #bitcoin-core-pr-reviews 07:47 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection] 07:47 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 07:53 -!- lightlike [~lightlike@2001:16b8:57b0:1900:e0bb:3cd3:91b1:3a70] has joined #bitcoin-core-pr-reviews 08:57 -!- ccdle12 [035f2f0c@ec2-3-95-47-12.compute-1.amazonaws.com] has joined #bitcoin-core-pr-reviews 09:06 -!- pinheadmz [~matthewzi@184.170.246.173] has quit [Read error: Connection reset by peer] 09:06 -!- pinheadmz_ [~matthewzi@pool-100-33-69-78.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews 09:14 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews 09:28 -!- sebastianvstaa [~sebastian@HSI-KBW-091-089-090-188.hsi2.kabelbw.de] has joined #bitcoin-core-pr-reviews 09:43 -!- michaelfolkson [~textual@2a00:23c5:be04:e501:5c08:17f7:f672:4239] has joined #bitcoin-core-pr-reviews 09:56 -!- clarkmoody [~clarkmood@167.71.122.99] has joined #bitcoin-core-pr-reviews 09:58 < jnewbery> we'll get started in a couple of minutes 09:58 < jnewbery> notes: https://bitcoin-core-review-club.github.io/15204.html 10:00 < jnewbery> hi! 10:00 < jkczyz> hi 10:01 < michaelfolkson> Hey 10:01 < sebastianvstaa> hey 10:01 < jnewbery> how many of you had a chance to build and test the PR this week? 10:01 < jonatack> hi! 10:01 < michaelfolkson> I have 10:02 < jnewbery> michaelfolkson: great. What did you find? 10:03 < jnewbery> anyone else? 10:03 < ccdle12> hi 10:03 < jonatack> Initial review this afternoon. 10:03 < michaelfolkson> Tests passed, functionality on GUI as expected... 10:04 < michaelfolkson> I didn't try things like passing it a directory with no wallet though. 10:04 < jonatack> I'd like to do more edge case testing but it looks good apart from a few minor things. 10:05 < jnewbery> yeah, I agree. There are probably quite a few edge cases 10:05 < jonatack> It would be good to have a way to display the full path that is discreet and user-friendly, but yes, can be in a follow-up. 10:06 < jnewbery> like if a non wallet file is named wallet.dat, or if the file is deleted while trying to load it, etc 10:06 < michaelfolkson> What edge cases need testing? No wallet in directory. Multiple wallets in directory. Anything else? 10:06 < jonatack> jnewbery: yes, good ones 10:06 < michaelfolkson> Yup 10:06 < jnewbery> michaelfolkson: good suggestions 10:07 < michaelfolkson> I think test cases to be tested should be included at the top of the PR 10:07 < jnewbery> first question: The first commit in this PR refactors the LoadWallet() function. Why? 10:08 < jonatack> "How to review this PR" in the PR description is an excellent lesson from this club early on. 10:09 < jonatack> To add abstraction for handling the external wallet case. 10:09 < jnewbery> jonatack: you mean it would have been helpful if the author had given hints on how to review? 10:09 < ccdle12> loads an external wallet (separate from the bitcoin core wallet) from a different directory path rather than the default location 10:09 < jkczyz> Keeps the existence check code in one place so it's not duplicated in both the wallet and RPC interface 10:10 < jonatack> Maybe not on this PR, but it's something I learned from you that I try to remember to do in my PRs. 10:10 < jnewbery> jkczyz: yeah that's the motivation 10:10 < michaelfolkson> : Yeah makes it much easier for reviewers. I like Jon's PRs :) 10:10 < jkczyz> The commit message could have stated why for posterity, too 10:10 -!- wahwhowah [8754845d@135.84.132.93] has joined #bitcoin-core-pr-reviews 10:10 < jnewbery> I think we've mentioned this before. Ideally the RPC would be a very thin layer and most of the logic is at a lower layer so it can be used by multiple interfaces 10:11 < jonatack> I don't want to be critical though, it's easy to not think about it until afterward. 10:12 < jnewbery> jkczyz: yeah, the commit log could have noted that. It's easy to forget to add a good commit log when changing a PR around 10:13 < jonatack> michaelfolkson: ty :) 10:13 < jnewbery> I thought this PR would be good to cover because it's really about testing the GUI. Building the GUI on different platforms isn't something I do very regularly, and isn't part of my regular review workflow 10:14 < jnewbery> To me it just seems quite time-consuming because it's not something I do very often 10:15 < jkczyz> Not being that familiar with the GUI code, are there many other non-manual way of testing? 10:15 < jnewbery> ok, so next question: How can this PR be tested (both manually and automatically)? 10:15 < jnewbery> jkczyz: snap! 10:16 < jkczyz> jnewbery: Should have read the questions more closely beforehand ;) 10:16 < jnewbery> there are qt tests in src/qt/test . Here's an example of a test case for the GUI: https://github.com/bitcoin/bitcoin/pull/10420 10:17 < jnewbery> I've never written any, but it looks like you can drive the GUI in the tests 10:18 < jonatack> I actually brought an old macbook pro back to life just for building and testing Bitcoin Core PRs on macOS, in parallel with my Linux laptop to save time. 10:18 < michaelfolkson> I'm unsure on automated testing... You need user to click around and do weird stuff which is hard to automate? 10:19 < michaelfolkson> I suppose not 10:19 < jnewbery> michaelfolkson: yeah, you can drive that in the automated tests. Take a look at the PR I linked to 10:20 < jonatack> jnewbery: thanks! I need to dig into the qt tests. 10:20 < jnewbery> It's doing things like "Press "Yes" or "Cancel" buttons in modal send confirmation dialog.", "Select row in table, invoke context menu...", etc 10:20 < jnewbery> jonatack: wow. That's dedication! 10:21 < jnewbery> ok, any other questions about this PR? The code should be quite straightforward to review 10:22 < michaelfolkson> I think there was one thing I misunderstood 10:22 < michaelfolkson> Wallets should be in separate directories by default right? You talked about that in the notes 10:23 < jkczyz> Given logic from rpcwallet.cpp was moved to wallet.cpp, do any tests need to be updated? Or added for that matter given the addtional logic around loading wallets? 10:23 < michaelfolkson> But there's also the directory where the list of the wallets is 10:24 < jnewbery> michaelfolkson: wallets used to be individual name.dat files. A PR a couple of years ago changed that to be name directories containing a wallet.dat file 10:24 < jnewbery> that was done as part of supporting 'external' wallets (wallets that aren't in bitcoind's default wallet directory) 10:25 < jnewbery> jkczyz: no tests broke, so I guess the answer is no! All of the wallet open/close/create tests are higher level functional tests, and since there's no change in external behaviour, they don't need to be changed for a refactor 10:26 < jnewbery> michaelfolkson: look at the help text for walletdir: 10:27 < jnewbery> -walletdir= 10:27 < jnewbery> Specify directory to hold wallets (default: /wallets if it 10:27 < jnewbery> exists, otherwise ) 10:27 < jnewbery> any wallet not in that directory is an 'external' wallet 10:27 < michaelfolkson> Ah cool thanks 10:27 < jonatack> michaelfolkson, jnewbery: Those changes were made in https://github.com/bitcoin/bitcoin/pull/11687 by ryanofsky, or an earlier PR? 10:27 < jonatack> "External wallet files" 10:28 < emilengler> Are there also ways to get the list of external wallets with an RPC command? 10:28 < jnewbery> yes, I think 11687 is where that change was made 10:28 < jnewbery> emilengler: you can list all the open wallets 10:29 < jkczyz> jnewbery: I guess my question is more around whether the logic should be tested directly via wallet.cpp tests or indirectly via rpcwallet.cpp tests. 10:29 < jnewbery> if there are external wallets that aren't open, bitcoind doesn't know about them (they could be anywhere in the filesystem) 10:29 < jkczyz> ah, via functional tests. I see no unit tests tjhen 10:30 < jnewbery> jkczyz: we don't have many unit tests for rpc* code. Now that the logic has been moved down into wallet.cpp, it might make sense to add unit tests 10:32 < jonatack> jnewbery: is there a particular PR that did that logic move? Looking in your meta-issue I didn't see any in particular yet. 10:33 < jonatack> Is that part of ryanofsky's modularity work these past couple years? 10:33 -!- wahwhowah [8754845d@135.84.132.93] has quit [Remote host closed the connection] 10:33 < jnewbery> jonatack: the logic move from rpc to wallet? That's the first commit in this PR (15204) 10:34 < jnewbery> not really that related to the ryanofsky modularity work 10:34 < jonatack> ty, gotcha 10:36 < jnewbery> any other questions? 10:36 < michaelfolkson> A broader question on the GUI. I'm assuming its existence and changes to it don't widen the attack surface area on Bitcoin Core much if at all? 10:36 < jnewbery> We can wrap up early if no-one has anything 10:37 < jnewbery> michaelfolkson: that's a very broad question! 10:37 < michaelfolkson> All feature changes on the GUI have already been tested for months within bitcoind 10:37 < michaelfolkson> Haha 10:37 < michaelfolkson> We do have 25 mins lol 10:37 < jnewbery> any code 'widens the attack surface area' 10:37 < michaelfolkson> Ok but it is limited 10:39 < michaelfolkson> Any questions ? 10:39 < jonatack> I've been under the impression that qt testing is generally punted here, for lack of a framework. 10:39 < sebastianvstaa> will try to be better prepared next time.. 10:39 < jonatack> Will look further into qt unit tests like the link you provided... 10:39 < sebastianvstaa> didnt manage to build the PR and run the tests 10:39 < jnewbery> at least since https://github.com/bitcoin/bitcoin/pull/10244, we have a well defined interface between the GUI and node 10:40 < jnewbery> so theoretically that somewhat limits how much any issue with the GUI could leak into the node 10:40 < jnewbery> multiprocess support would take that further so memory isn't shared between the GUI process and node process 10:41 < jnewbery> jonatack: yeah, I don't think I've seen anyone apart from ryanofsky write qt tests 10:41 < jonatack> Yes. Removing global locking of the GUI seems highly desired. 10:41 < jnewbery> jonatack: that'd be great. If you open a qt test PR, perhaps you can present it here 10:42 < jonatack> jnewbery: ty, I didn't realize about qt testing. Worth looking into! 10:43 < jnewbery> ok, unless there are any final questions, lets wrap it up there 10:43 < jonatack> sebastianvstaa: what blocked you? 10:43 < sebastianvstaa> lack of knowledge of the build environment 10:43 < jonatack> we can discuss after if you like 10:43 < sebastianvstaa> still need to understand git better 10:43 < sebastianvstaa> yes please. that would be cool :) 10:44 < jonatack> thanks, everyone! 10:44 < michaelfolkson> Thanks! 10:44 < sebastianvstaa> thanks! 10:45 < michaelfolkson> You going to chat here or privately ? 10:45 < michaelfolkson> I'd listen in if here 10:45 < jonatack> here unless anyone minds? 10:45 < sebastianvstaa> ok 10:45 < jonatack> sebastianvstaa: for building, what platform are you on? 10:46 < sebastianvstaa> ubuntu 18.04 10:46 < sebastianvstaa> managed to build core before 10:46 < sebastianvstaa> but not sure how to switch to the PR and build that 10:47 < sebastianvstaa> installed smartgit, but if you could show how to do it from command line, thats cool as well 10:47 < jonatack> sebastianvstaa: ok, you have detailed instructions in the repo at doc/build-unix.md or similar 10:47 < jnewbery> thanks everyone. I'll duck out now, but please feel free to continue the discussion here! 10:47 < sebastianvstaa> thanks jon! 10:47 < jonatack> cheers John! 10:47 < sebastianvstaa> *john 10:47 < clarkmoody> Adios 10:47 < michaelfolkson> Thanks John! 10:47 < jkczyz> bye! 10:48 < jonatack> sebastianvstaa: and in the beginning for me last March I wrote a simplified doc on building here: https://github.com/jonatack/bitcoin-development/blob/master/how-to-compile-bitcoin-core-from-source-on-linux-and-macOS.md 10:49 < sebastianvstaa> ok will check it 10:49 < jonatack> sebastianvstaa: once you've built, go throught the productivity notes and developer notes in /doc 10:49 -!- Zenton [~user@unaffiliated/vicenteh] has quit [Ping timeout: 276 seconds] 10:49 < sebastianvstaa> ok. have the build the whole thing before 10:49 < sebastianvstaa> but how to switch to the PR code and build that specifically? 10:50 < michaelfolkson> Basically clone the author of the PR's fork and checkout the PR branch 10:50 < michaelfolkson> That's what I've done 10:50 -!- ajonas [uid385278@gateway/web/irccloud.com/x-oflsnpqvlcnsttpw] has joined #bitcoin-core-pr-reviews 10:50 < jonatack> sebastianvstaa: notably, install ccache, and add git refspecs to your gitconfig 10:50 < sebastianvstaa> ok 10:51 < jonatack> sebastianvstaa: see/copy/steal lines 212-225 of my gitconfig: https://github.com/jonatack/dotfiles/blob/master/gitconfig 10:53 < sebastianvstaa> # Enable pulling in remote PRs and testing locally with: git checkout pr/999 10:53 < sebastianvstaa> # See this gist: https://gist.github.com/piscisaureus/3342247 10:53 < sebastianvstaa> [remote "origin"] 10:53 < sebastianvstaa> fetch = +refs/heads/*:refs/remotes/origin/* 10:53 < sebastianvstaa> fetch = +refs/pull/*/head:refs/remotes/origin/pr/* 10:53 < sebastianvstaa> # Reference Bitcoin PRs easily with refspecs. 10:53 < sebastianvstaa> # This adds an upstream-pull remote to your git repository, which can be fetched 10:53 < sebastianvstaa> # using git fetch --all or git fetch upstream-pull. Afterwards, you can use 10:53 < sebastianvstaa> # upstream-pull/NUMBER/head in arguments to git show, git checkout and anywhere 10:53 < sebastianvstaa> # a commit id would be acceptable to see the changes from pull request NUMBER. 10:53 < sebastianvstaa> # [remote "upstream-pull"] 10:53 < sebastianvstaa> # fetch = +refs/pull/*:refs/remotes/upstream-pull/* 10:53 < sebastianvstaa> # url = git@github.com:bitcoin/bitcoin.git 10:53 < sebastianvstaa> hmmm. ok. this? 10:53 < jonatack> sebastianvstaa: in closing, I'll leave you with a link to this doc of things I've been learning from this PR review club and from reviewing PRs these past months: 10:53 < jonatack> sebastianvstaa: https://github.com/jonatack/bitcoin-development/blob/master/how-to-review-bitcoin-core-prs.md 10:54 < jonatack> sebastianvstaa: warning, it's nearly a book now :D 10:54 < michaelfolkson> I'm here if you need basic questions! Jon is very friendly if you have non basic questions 10:54 < sebastianvstaa> thanks jonatack. will work through it 10:54 < sebastianvstaa> ok, thanks! 10:55 < jonatack> sebastianvstaa: yes. i only use the lines that are uncommented, but added the ones also recommended by the bitcoin core productivity doc which is a bit different from what i use 10:56 < jonatack> sebastianvstaa: jnewbery has a custom script which is much fancier and that pulls in the PR description too. you'll see a link to it in the How To Review doc I linked you to. 10:57 < jonatack> But the gitconfig I sent you is more than enough to get started. 10:58 < jonatack> michaelfolkson: thanks! 11:00 < jonatack> sebastianvstaa: last thing, to start running qt from a PR build, run from the repo root: src/qt/bitcoin-qt -testnet 11:01 < sebastianvstaa> ok 11:01 < jonatack> (to start running the GUI wallet, sorry) 11:02 < jonatack> sebastianvstaa: any problems, don't hesitate to PM us :) 11:05 < sebastianvstaa> jonatack: sure, thanks! 11:05 -!- csknk [~csknk@unaffiliated/csknk] has quit [Quit: leaving] 11:24 -!- michaelfolkson [~textual@2a00:23c5:be04:e501:5c08:17f7:f672:4239] has quit [Quit: Sleep mode] 11:44 < jonatack> query jnewbery The two questions you asked for today's review club were really good, deeper than they seemed. I learned a lot from today's session. Thanks! 11:44 < jonatack> heh forgot the / 11:44 -!- ccdle12 [035f2f0c@ec2-3-95-47-12.compute-1.amazonaws.com] has quit [Ping timeout: 260 seconds] 11:45 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-aljfsmnzbununyhz] has quit [Quit: Connection closed for inactivity] 11:47 -!- michaelfolkson [~textual@2a00:23c5:be04:e501:5c08:17f7:f672:4239] has joined #bitcoin-core-pr-reviews 12:02 -!- BlueMatt [~BlueMatt@unaffiliated/bluematt] has quit [Ping timeout: 240 seconds] 12:02 -!- BlueMatt [~BlueMatt@unaffiliated/bluematt] has joined #bitcoin-core-pr-reviews 12:05 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has quit [Ping timeout: 276 seconds] 12:07 -!- jonatack [~jon@37.168.169.168] has joined #bitcoin-core-pr-reviews 12:07 -!- jonatack [~jon@37.168.169.168] has quit [Client Quit] 12:08 -!- jonatack [~jon@37.168.169.168] has joined #bitcoin-core-pr-reviews 12:39 -!- michaelfolkson [~textual@2a00:23c5:be04:e501:5c08:17f7:f672:4239] has quit [Quit: Sleep mode] 12:39 -!- lightlike [~lightlike@2001:16b8:57b0:1900:e0bb:3cd3:91b1:3a70] has left #bitcoin-core-pr-reviews ["Leaving"] 12:39 -!- jonatack [~jon@37.168.169.168] has quit [Quit: jonatack] 12:40 -!- jonatack [~jon@37.168.169.168] has joined #bitcoin-core-pr-reviews 12:44 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 12:59 -!- wullon [~wullon@241.243.86.88.rdns.comcable.net] has joined #bitcoin-core-pr-reviews 13:06 -!- michaelfolkson [~textual@2a00:23c5:be04:e501:5c08:17f7:f672:4239] has joined #bitcoin-core-pr-reviews 13:15 -!- michaelfolkson [~textual@2a00:23c5:be04:e501:5c08:17f7:f672:4239] has quit [Quit: Sleep mode] 14:27 -!- ch4ot1c [45ccf4dc@cpe-69-204-244-220.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 14:33 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews 14:47 -!- clarkmoo_ [~clarkmood@47-218-248-206.bcstcmta04.res.dyn.suddenlink.net] has joined #bitcoin-core-pr-reviews 14:50 -!- clarkmoody [~clarkmood@167.71.122.99] has quit [Ping timeout: 240 seconds] 15:08 -!- clarkmoo_ [~clarkmood@47-218-248-206.bcstcmta04.res.dyn.suddenlink.net] has quit [Remote host closed the connection] 15:08 -!- clarkmoody [~clarkmood@167.71.122.99] has joined #bitcoin-core-pr-reviews 15:27 -!- jonatack [~jon@37.168.169.168] has quit [Read error: Connection reset by peer] 15:30 < fanquake> re that PR. There’s at least one way to make qt segfault while running that change. 15:32 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has joined #bitcoin-core-pr-reviews 15:50 -!- sebastianvstaa [~sebastian@HSI-KBW-091-089-090-188.hsi2.kabelbw.de] has quit [Quit: Leaving] 16:04 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Ping timeout: 258 seconds] 16:31 -!- provoostenator [~quassel@provoostenator.sprovoost.nl] has quit [Remote host closed the connection] 16:38 -!- provoostenator [~quassel@provoostenator.sprovoost.nl] has joined #bitcoin-core-pr-reviews 18:43 -!- clarkmoody [~clarkmood@167.71.122.99] has quit [] 19:05 -!- ch4ot1c [45ccf4dc@cpe-69-204-244-220.nyc.res.rr.com] has quit [Remote host closed the connection] 19:05 -!- emilengler_ [~emilengle@unaffiliated/emilengler] has joined #bitcoin-core-pr-reviews 19:09 -!- emilengler [~emilengle@unaffiliated/emilengler] has quit [Ping timeout: 245 seconds] 20:17 -!- ajonas [uid385278@gateway/web/irccloud.com/x-oflsnpqvlcnsttpw] has quit [Quit: Connection closed for inactivity] 22:49 -!- seven__ [~seven@BSN-77-101-62.static.siol.net] has quit [Ping timeout: 245 seconds]