Add Open External Wallet action (gui)
- This is the last PR in a series of PRs to add all multiwallet functionality to the GUI (open wallet, close wallet and create wallet).
- A previous PR added the functionality to open wallets in the GUI. That was limited to opening wallets that were located in bitcoind’s default wallet directory.
- This PR allows opening external wallets, ie wallets that are located in locations other than the default wallet directory.
- Prior to PR 11687, wallets
were created as individual
wallet2.dat, etc). PR 11687 changed that to make wallet files directory-based (eg
wallet2/wallet.dat, etc. That was done because the wallet creates several additional log and database files that need to be kept separately for separate wallets.
- This PR was marked for the v0.19 milestone, but has missed the feature freeze deadline.
- The first commit in this PR refactors the
- How can this PR be tested (both manually and automatically)?
13:00 < jnewbery> hi! 13:00 < jkczyz> hi 13:01 < michaelfolkson> Hey 13:01 < sebastianvstaa> hey 13:01 < jnewbery> how many of you had a chance to build and test the PR this week? 13:01 < jonatack> hi! 13:01 < michaelfolkson> I have 13:02 < jnewbery> michaelfolkson: great. What did you find? 13:03 < jnewbery> anyone else? 13:03 < ccdle12> hi 13:03 < jonatack> Initial review this afternoon. 13:03 < michaelfolkson> Tests passed, functionality on GUI as expected... 13:04 < michaelfolkson> I didn't try things like passing it a directory with no wallet though. 13:04 < jonatack> I'd like to do more edge case testing but it looks good apart from a few minor things. 13:05 < jnewbery> yeah, I agree. There are probably quite a few edge cases 13: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. 13:06 < jnewbery> like if a non wallet file is named wallet.dat, or if the file is deleted while trying to load it, etc 13:06 < michaelfolkson> What edge cases need testing? No wallet in directory. Multiple wallets in directory. Anything else? 13:06 < jonatack> jnewbery: yes, good ones 13:06 < michaelfolkson> Yup 13:06 < jnewbery> michaelfolkson: good suggestions 13:07 < michaelfolkson> I think test cases to be tested should be included at the top of the PR 13:07 < jnewbery> first question: The first commit in this PR refactors the LoadWallet() function. Why? 13:08 < jonatack> "How to review this PR" in the PR description is an excellent lesson from this club early on. 13:09 < jonatack> To add abstraction for handling the external wallet case. 13:09 < jnewbery> jonatack: you mean it would have been helpful if the author had given hints on how to review? 13:09 < ccdle12> loads an external wallet (separate from the bitcoin core wallet) from a different directory path rather than the default location 13:09 < jkczyz> Keeps the existence check code in one place so it's not duplicated in both the wallet and RPC interface 13: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. 13:10 < jnewbery> jkczyz: yeah that's the motivation 13:10 < michaelfolkson> <jnewbery>: Yeah makes it much easier for reviewers. I like Jon's PRs :) 13:10 < jkczyz> The commit message could have stated why for posterity, too 13: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 13:11 < jonatack> I don't want to be critical though, it's easy to not think about it until afterward. 13: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 13:13 < jonatack> michaelfolkson: ty :) 13: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 13:14 < jnewbery> To me it just seems quite time-consuming because it's not something I do very often 13:15 < jkczyz> Not being that familiar with the GUI code, are there many other non-manual way of testing? 13:15 < jnewbery> ok, so next question: How can this PR be tested (both manually and automatically)? 13:15 < jnewbery> jkczyz: snap! 13:16 < jkczyz> jnewbery: Should have read the questions more closely beforehand ;) 13: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 13:17 < jnewbery> I've never written any, but it looks like you can drive the GUI in the tests 13: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. 13:18 < michaelfolkson> I'm unsure on automated testing... You need user to click around and do weird stuff which is hard to automate? 13:19 < michaelfolkson> I suppose not 13:19 < jnewbery> michaelfolkson: yeah, you can drive that in the automated tests. Take a look at the PR I linked to 13:20 < jonatack> jnewbery: thanks! I need to dig into the qt tests. 13: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 13:20 < jnewbery> jonatack: wow. That's dedication! 13:21 < jnewbery> ok, any other questions about this PR? The code should be quite straightforward to review 13:22 < michaelfolkson> I think there was one thing I misunderstood 13:22 < michaelfolkson> Wallets should be in separate directories by default right? You talked about that in the notes 13: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? 13:23 < michaelfolkson> But there's also the directory where the list of the wallets is 13: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 13:24 < jnewbery> that was done as part of supporting 'external' wallets (wallets that aren't in bitcoind's default wallet directory) 13: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 13:26 < jnewbery> michaelfolkson: look at the help text for walletdir: 13:26 < jnewbery> -walletdir=<dir> 13:26 < jnewbery> Specify directory to hold wallets (default: <datadir>/wallets if it 13:26 < jnewbery> exists, otherwise <datadir>) 13:27 < jnewbery> any wallet not in that directory is an 'external' wallet 13:27 < michaelfolkson> Ah cool thanks 13:27 < jonatack> michaelfolkson, jnewbery: Those changes were made in https://github.com/bitcoin/bitcoin/pull/11687 by ryanofsky, or an earlier PR? 13:27 < jonatack> "External wallet files" 13:28 < emilengler> Are there also ways to get the list of external wallets with an RPC command? 13:28 < jnewbery> yes, I think 11687 is where that change was made 13:28 < jnewbery> emilengler: you can list all the open wallets 13: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. 13:29 < jnewbery> if there are external wallets that aren't open, bitcoind doesn't know about them (they could be anywhere in the filesystem) 13:29 < jkczyz> ah, via functional tests. I see no unit tests tjhen 13: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 13: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. 13:33 < jonatack> Is that part of ryanofsky's modularity work these past couple years? 13:33 < jnewbery> jonatack: the logic move from rpc to wallet? That's the first commit in this PR (15204) 13:34 < jnewbery> not really that related to the ryanofsky modularity work 13:34 < jonatack> ty, gotcha 13:36 < jnewbery> any other questions? 13: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? 13:36 < jnewbery> We can wrap up early if no-one has anything 13:37 < jnewbery> michaelfolkson: that's a very broad question! 13:37 < michaelfolkson> All feature changes on the GUI have already been tested for months within bitcoind 13:37 < michaelfolkson> Haha 13:37 < michaelfolkson> We do have 25 mins lol 13:37 < jnewbery> any code 'widens the attack surface area' 13:37 < michaelfolkson> Ok but it is limited 13:39 < michaelfolkson> Any questions <sebastianvstaa>? 13:39 < jonatack> I've been under the impression that qt testing is generally punted here, for lack of a framework. 13:39 < sebastianvstaa> will try to be better prepared next time.. 13:39 < jonatack> Will look further into qt unit tests like the link you provided... 13:39 < sebastianvstaa> didnt manage to build the PR and run the tests 13:39 < jnewbery> at least since https://github.com/bitcoin/bitcoin/pull/10244, we have a well defined interface between the GUI and node 13:40 < jnewbery> so theoretically that somewhat limits how much any issue with the GUI could leak into the node 13:40 < jnewbery> multiprocess support would take that further so memory isn't shared between the GUI process and node process 13:41 < jnewbery> jonatack: yeah, I don't think I've seen anyone apart from ryanofsky write qt tests 13:41 < jonatack> Yes. Removing global locking of the GUI seems highly desired. 13:41 < jnewbery> jonatack: that'd be great. If you open a qt test PR, perhaps you can present it here 13:42 < jonatack> jnewbery: ty, I didn't realize about qt testing. Worth looking into! 13:43 < jnewbery> ok, unless there are any final questions, lets wrap it up there 13:43 < jonatack> sebastianvstaa: what blocked you? 13:43 < sebastianvstaa> lack of knowledge of the build environment 13:43 < jonatack> we can discuss after if you like 13:43 < sebastianvstaa> still need to understand git better 13:43 < sebastianvstaa> yes please. that would be cool :) 13:44 < jonatack> thanks, everyone! 13:44 < michaelfolkson> Thanks! 13:44 < sebastianvstaa> thanks! 13:45 < michaelfolkson> You going to chat here or privately <jonatack>? 13:45 < michaelfolkson> I'd listen in if here 13:45 < jonatack> here unless anyone minds? 13:45 < sebastianvstaa> ok 13:45 < jonatack> sebastianvstaa: for building, what platform are you on? 13:46 < sebastianvstaa> ubuntu 18.04 13:46 < sebastianvstaa> managed to build core before 13:46 < sebastianvstaa> but not sure how to switch to the PR and build that 13:47 < sebastianvstaa> installed smartgit, but if you could show how to do it from command line, thats cool as well 13:47 < jonatack> sebastianvstaa: ok, you have detailed instructions in the repo at doc/build-unix.md or similar 13:47 < jnewbery> thanks everyone. I'll duck out now, but please feel free to continue the discussion here! 13:47 < sebastianvstaa> thanks jon! 13:47 < jonatack> cheers John! 13:47 < sebastianvstaa> *john 13:47 < clarkmoody> Adios 13:47 < michaelfolkson> Thanks John! 13:47 < jkczyz> bye! 13: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 13:49 < sebastianvstaa> ok will check it 13:49 < jonatack> sebastianvstaa: once you've built, go throught the productivity notes and developer notes in /doc 13:49 < sebastianvstaa> ok. have the build the whole thing before 13:49 < sebastianvstaa> but how to switch to the PR code and build that specifically? 13:50 < michaelfolkson> Basically clone the author of the PR's fork and checkout the PR branch 13:50 < michaelfolkson> That's what I've done 13:50 < jonatack> sebastianvstaa: notably, install ccache, and add git refspecs to your gitconfig 13:50 < sebastianvstaa> ok 13:51 < jonatack> sebastianvstaa: see/copy/steal lines 212-225 of my gitconfig: https://github.com/jonatack/dotfiles/blob/master/gitconfig 13:53 < sebastianvstaa> # Enable pulling in remote PRs and testing locally with: git checkout pr/999 13:53 < sebastianvstaa> # See this gist: https://gist.github.com/piscisaureus/3342247 13:53 < sebastianvstaa> [remote "origin"] 13:53 < sebastianvstaa> fetch = +refs/heads/*:refs/remotes/origin/* 13:53 < sebastianvstaa> fetch = +refs/pull/*/head:refs/remotes/origin/pr/* 13:53 < sebastianvstaa> # Reference Bitcoin PRs easily with refspecs. 13:53 < sebastianvstaa> # This adds an upstream-pull remote to your git repository, which can be fetched 13:53 < sebastianvstaa> # using git fetch --all or git fetch upstream-pull. Afterwards, you can use 13:53 < sebastianvstaa> # upstream-pull/NUMBER/head in arguments to git show, git checkout and anywhere 13:53 < sebastianvstaa> # a commit id would be acceptable to see the changes from pull request NUMBER. 13:53 < sebastianvstaa> # [remote "upstream-pull"] 13:53 < sebastianvstaa> # fetch = +refs/pull/*:refs/remotes/upstream-pull/* 13:53 < sebastianvstaa> # url = firstname.lastname@example.org:bitcoin/bitcoin.git 13:53 < sebastianvstaa> hmmm. ok. this? 13: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: 13:53 < jonatack> sebastianvstaa: https://github.com/jonatack/bitcoin-development/blob/master/how-to-review-bitcoin-core-prs.md 13:54 < jonatack> sebastianvstaa: warning, it's nearly a book now :D 13:54 < michaelfolkson> I'm here if you need basic questions! Jon is very friendly if you have non basic questions 13:54 < sebastianvstaa> thanks jonatack. will work through it 13:54 < sebastianvstaa> ok, thanks! 13: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 13: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. 13:57 < jonatack> But the gitconfig I sent you is more than enough to get started. 13:58 < jonatack> michaelfolkson: thanks! 14:00 < jonatack> sebastianvstaa: last thing, to start running qt from a PR build, run from the repo root: src/qt/bitcoin-qt -testnet 14:01 < sebastianvstaa> ok 14:01 < jonatack> (to start running the GUI wallet, sorry) 14:02 < jonatack> sebastianvstaa: any problems, don't hesitate to PM us :) 14:05 < sebastianvstaa> jonatack: sure, thanks!