#15204 Add Open External Wallet action (gui)

https://github.com/bitcoin/bitcoin/15204

Host: jnewbery

Notes

  • 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 .dat files (eg wallet1.dat, wallet2.dat, etc). PR 11687 changed that to make wallet files directory-based (eg wallet1/wallet.dat, 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.

Questions

  • The first commit in this PR refactors the LoadWallet() function. Why?
  • How can this PR be tested (both manually and automatically)?

Meeting Log

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 = git@github.com: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!