Add Open External Wallet action (gui)

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

Host: jnewbery  -  PR author: promag

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

  113:00 <jnewbery> hi!
  213:00 <jkczyz> hi
  313:01 <michaelfolkson> Hey
  413:01 <sebastianvstaa> hey
  513:01 <jnewbery> how many of you had a chance to build and test the PR this week?
  613:01 <jonatack> hi!
  713:01 <michaelfolkson> I have
  813:02 <jnewbery> michaelfolkson: great. What did you find?
  913:03 <jnewbery> anyone else?
 1013:03 <ccdle12> hi
 1113:03 <jonatack> Initial review this afternoon.
 1213:03 <michaelfolkson> Tests passed, functionality on GUI as expected...
 1313:04 <michaelfolkson> I didn't try things like passing it a directory with no wallet though.
 1413:04 <jonatack> I'd like to do more edge case testing but it looks good apart from a few minor things.
 1513:05 <jnewbery> yeah, I agree. There are probably quite a few edge cases
 1613: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.
 1713:06 <jnewbery> like if a non wallet file is named wallet.dat, or if the file is deleted while trying to load it, etc
 1813:06 <michaelfolkson> What edge cases need testing? No wallet in directory. Multiple wallets in directory. Anything else?
 1913:06 <jonatack> jnewbery: yes, good ones
 2013:06 <michaelfolkson> Yup
 2113:06 <jnewbery> michaelfolkson: good suggestions
 2213:07 <michaelfolkson> I think test cases to be tested should be included at the top of the PR
 2313:07 <jnewbery> first question: The first commit in this PR refactors the LoadWallet() function. Why?
 2413:08 <jonatack> "How to review this PR" in the PR description is an excellent lesson from this club early on.
 2513:09 <jonatack> To add abstraction for handling the external wallet case.
 2613:09 <jnewbery> jonatack: you mean it would have been helpful if the author had given hints on how to review?
 2713:09 <ccdle12> loads an external wallet (separate from the bitcoin core wallet) from a different directory path rather than the default location
 2813:09 <jkczyz> Keeps the existence check code in one place so it's not duplicated in both the wallet and RPC interface
 2913: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.
 3013:10 <jnewbery> jkczyz: yeah that's the motivation
 3113:10 <michaelfolkson> <jnewbery>: Yeah makes it much easier for reviewers. I like Jon's PRs :)
 3213:10 <jkczyz> The commit message could have stated why for posterity, too
 3313: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
 3413:11 <jonatack> I don't want to be critical though, it's easy to not think about it until afterward.
 3513: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
 3613:13 <jonatack> michaelfolkson: ty :)
 3713: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
 3813:14 <jnewbery> To me it just seems quite time-consuming because it's not something I do very often
 3913:15 <jkczyz> Not being that familiar with the GUI code, are there many other non-manual way of testing?
 4013:15 <jnewbery> ok, so next question: How can this PR be tested (both manually and automatically)?
 4113:15 <jnewbery> jkczyz: snap!
 4213:16 <jkczyz> jnewbery: Should have read the questions more closely beforehand ;)
 4313: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
 4413:17 <jnewbery> I've never written any, but it looks like you can drive the GUI in the tests
 4513: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.
 4613:18 <michaelfolkson> I'm unsure on automated testing... You need user to click around and do weird stuff which is hard to automate?
 4713:19 <michaelfolkson> I suppose not
 4813:19 <jnewbery> michaelfolkson: yeah, you can drive that in the automated tests. Take a look at the PR I linked to
 4913:20 <jonatack> jnewbery: thanks! I need to dig into the qt tests.
 5013: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
 5113:20 <jnewbery> jonatack: wow. That's dedication!
 5213:21 <jnewbery> ok, any other questions about this PR? The code should be quite straightforward to review
 5313:22 <michaelfolkson> I think there was one thing I misunderstood
 5413:22 <michaelfolkson> Wallets should be in separate directories by default right? You talked about that in the notes
 5513: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?
 5613:23 <michaelfolkson> But there's also the directory where the list of the wallets is
 5713: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
 5813:24 <jnewbery> that was done as part of supporting 'external' wallets (wallets that aren't in bitcoind's default wallet directory)
 5913: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
 6013:26 <jnewbery> michaelfolkson: look at the help text for walletdir:
 6113:26 <jnewbery> -walletdir=<dir>
 6213:26 <jnewbery> Specify directory to hold wallets (default: <datadir>/wallets if it
 6313:26 <jnewbery> exists, otherwise <datadir>)
 6413:27 <jnewbery> any wallet not in that directory is an 'external' wallet
 6513:27 <michaelfolkson> Ah cool thanks
 6613:27 <jonatack> michaelfolkson, jnewbery: Those changes were made in https://github.com/bitcoin/bitcoin/pull/11687 by ryanofsky, or an earlier PR?
 6713:27 <jonatack> "External wallet files"
 6813:28 <emilengler> Are there also ways to get the list of external wallets with an RPC command?
 6913:28 <jnewbery> yes, I think 11687 is where that change was made
 7013:28 <jnewbery> emilengler: you can list all the open wallets
 7113: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.
 7213:29 <jnewbery> if there are external wallets that aren't open, bitcoind doesn't know about them (they could be anywhere in the filesystem)
 7313:29 <jkczyz> ah, via functional tests. I see no unit tests tjhen
 7413: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
 7513: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.
 7613:33 <jonatack> Is that part of ryanofsky's modularity work these past couple years?
 7713:33 <jnewbery> jonatack: the logic move from rpc to wallet? That's the first commit in this PR (15204)
 7813:34 <jnewbery> not really that related to the ryanofsky modularity work
 7913:34 <jonatack> ty, gotcha
 8013:36 <jnewbery> any other questions?
 8113: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?
 8213:36 <jnewbery> We can wrap up early if no-one has anything
 8313:37 <jnewbery> michaelfolkson: that's a very broad question!
 8413:37 <michaelfolkson> All feature changes on the GUI have already been tested for months within bitcoind
 8513:37 <michaelfolkson> Haha
 8613:37 <michaelfolkson> We do have 25 mins lol
 8713:37 <jnewbery> any code 'widens the attack surface area'
 8813:37 <michaelfolkson> Ok but it is limited
 8913:39 <michaelfolkson> Any questions <sebastianvstaa>?
 9013:39 <jonatack> I've been under the impression that qt testing is generally punted here, for lack of a framework.
 9113:39 <sebastianvstaa> will try to be better prepared next time..
 9213:39 <jonatack> Will look further into qt unit tests like the link you provided...
 9313:39 <sebastianvstaa> didnt manage to build the PR and run the tests
 9413:39 <jnewbery> at least since https://github.com/bitcoin/bitcoin/pull/10244, we have a well defined interface between the GUI and node
 9513:40 <jnewbery> so theoretically that somewhat limits how much any issue with the GUI could leak into the node
 9613:40 <jnewbery> multiprocess support would take that further so memory isn't shared between the GUI process and node process
 9713:41 <jnewbery> jonatack: yeah, I don't think I've seen anyone apart from ryanofsky write qt tests
 9813:41 <jonatack> Yes. Removing global locking of the GUI seems highly desired.
 9913:41 <jnewbery> jonatack: that'd be great. If you open a qt test PR, perhaps you can present it here
10013:42 <jonatack> jnewbery: ty, I didn't realize about qt testing. Worth looking into!
10113:43 <jnewbery> ok, unless there are any final questions, lets wrap it up there
10213:43 <jonatack> sebastianvstaa: what blocked you?
10313:43 <sebastianvstaa> lack of knowledge of the build environment
10413:43 <jonatack> we can discuss after if you like
10513:43 <sebastianvstaa> still need to understand git better
10613:43 <sebastianvstaa> yes please. that would be cool :)
10713:44 <jonatack> thanks, everyone!
10813:44 <michaelfolkson> Thanks!
10913:44 <sebastianvstaa> thanks!
11013:45 <michaelfolkson> You going to chat here or privately <jonatack>?
11113:45 <michaelfolkson> I'd listen in if here
11213:45 <jonatack> here unless anyone minds?
11313:45 <sebastianvstaa> ok
11413:45 <jonatack> sebastianvstaa: for building, what platform are you on?
11513:46 <sebastianvstaa> ubuntu 18.04
11613:46 <sebastianvstaa> managed to build core before
11713:46 <sebastianvstaa> but not sure how to switch to the PR and build that
11813:47 <sebastianvstaa> installed smartgit, but if you could show how to do it from command line, thats cool as well
11913:47 <jonatack> sebastianvstaa: ok, you have detailed instructions in the repo at doc/build-unix.md or similar
12013:47 <jnewbery> thanks everyone. I'll duck out now, but please feel free to continue the discussion here!
12113:47 <sebastianvstaa> thanks jon!
12213:47 <jonatack> cheers John!
12313:47 <sebastianvstaa> *john
12413:47 <clarkmoody> Adios
12513:47 <michaelfolkson> Thanks John!
12613:47 <jkczyz> bye!
12713: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
12813:49 <sebastianvstaa> ok will check it
12913:49 <jonatack> sebastianvstaa: once you've built, go throught the productivity notes and developer notes in /doc
13013:49 <sebastianvstaa> ok. have the build the whole thing before
13113:49 <sebastianvstaa> but how to switch to the PR code and build that specifically?
13213:50 <michaelfolkson> Basically clone the author of the PR's fork and checkout the PR branch
13313:50 <michaelfolkson> That's what I've done
13413:50 <jonatack> sebastianvstaa: notably, install ccache, and add git refspecs to your gitconfig
13513:50 <sebastianvstaa> ok
13613:51 <jonatack> sebastianvstaa: see/copy/steal lines 212-225 of my gitconfig: https://github.com/jonatack/dotfiles/blob/master/gitconfig
13713:53 <sebastianvstaa> # Enable pulling in remote PRs and testing locally with: git checkout pr/999
13813:53 <sebastianvstaa> # See this gist: https://gist.github.com/piscisaureus/3342247
13913:53 <sebastianvstaa> [remote "origin"]
14013:53 <sebastianvstaa> fetch = +refs/heads/*:refs/remotes/origin/*
14113:53 <sebastianvstaa> fetch = +refs/pull/*/head:refs/remotes/origin/pr/*
14213:53 <sebastianvstaa> # Reference Bitcoin PRs easily with refspecs.
14313:53 <sebastianvstaa> # This adds an upstream-pull remote to your git repository, which can be fetched
14413:53 <sebastianvstaa> # using git fetch --all or git fetch upstream-pull. Afterwards, you can use
14513:53 <sebastianvstaa> # upstream-pull/NUMBER/head in arguments to git show, git checkout and anywhere
14613:53 <sebastianvstaa> # a commit id would be acceptable to see the changes from pull request NUMBER.
14713:53 <sebastianvstaa> # [remote "upstream-pull"]
14813:53 <sebastianvstaa> # fetch = +refs/pull/*:refs/remotes/upstream-pull/*
14913:53 <sebastianvstaa> # url = git@github.com:bitcoin/bitcoin.git
15013:53 <sebastianvstaa> hmmm. ok. this?
15113: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:
15213:53 <jonatack> sebastianvstaa: https://github.com/jonatack/bitcoin-development/blob/master/how-to-review-bitcoin-core-prs.md
15313:54 <jonatack> sebastianvstaa: warning, it's nearly a book now :D
15413:54 <michaelfolkson> I'm here if you need basic questions! Jon is very friendly if you have non basic questions
15513:54 <sebastianvstaa> thanks jonatack. will work through it
15613:54 <sebastianvstaa> ok, thanks!
15713: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
15813: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.
15913:57 <jonatack> But the gitconfig I sent you is more than enough to get started.
16013:58 <jonatack> michaelfolkson: thanks!
16114:00 <jonatack> sebastianvstaa: last thing, to start running qt from a PR build, run from the repo root: src/qt/bitcoin-qt -testnet
16214:01 <sebastianvstaa> ok
16314:01 <jonatack> (to start running the GUI wallet, sorry)
16414:02 <jonatack> sebastianvstaa: any problems, don't hesitate to PM us :)
16514:05 <sebastianvstaa> jonatack: sure, thanks!