Add Open External Wallet action (
gui) Sep 18, 2019
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
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
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)?
3 13:01 <michaelfolkson> Hey
4 13:01 <sebastianvstaa> hey
5 13:01 <jnewbery> how many of you had a chance to build and test the PR this week?
7 13:01 <michaelfolkson> I have
8 13:02 <jnewbery> michaelfolkson: great. What did you find?
9 13:03 <jnewbery> anyone else?
11 13:03 <jonatack> Initial review this afternoon.
12 13:03 <michaelfolkson> Tests passed, functionality on GUI as expected...
13 13:04 <michaelfolkson> I didn't try things like passing it a directory with no wallet though.
14 13:04 <jonatack> I'd like to do more edge case testing but it looks good apart from a few minor things.
15 13:05 <jnewbery> yeah, I agree. There are probably quite a few edge cases
16 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.
17 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
18 13:06 <michaelfolkson> What edge cases need testing? No wallet in directory. Multiple wallets in directory. Anything else?
19 13:06 <jonatack> jnewbery: yes, good ones
20 13:06 <michaelfolkson> Yup
21 13:06 <jnewbery> michaelfolkson: good suggestions
22 13:07 <michaelfolkson> I think test cases to be tested should be included at the top of the PR
23 13:07 <jnewbery> first question: The first commit in this PR refactors the LoadWallet() function. Why?
24 13:08 <jonatack> "How to review this PR" in the PR description is an excellent lesson from this club early on.
25 13:09 <jonatack> To add abstraction for handling the external wallet case.
26 13:09 <jnewbery> jonatack: you mean it would have been helpful if the author had given hints on how to review?
27 13:09 <ccdle12> loads an external wallet (separate from the bitcoin core wallet) from a different directory path rather than the default location
28 13:09 <jkczyz> Keeps the existence check code in one place so it's not duplicated in both the wallet and RPC interface
29 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.
30 13:10 <jnewbery> jkczyz: yeah that's the motivation
31 13:10 <michaelfolkson> <jnewbery>: Yeah makes it much easier for reviewers. I like Jon's PRs :)
32 13:10 <jkczyz> The commit message could have stated why for posterity, too
33 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
34 13:11 <jonatack> I don't want to be critical though, it's easy to not think about it until afterward.
35 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
36 13:13 <jonatack> michaelfolkson: ty :)
37 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
38 13:14 <jnewbery> To me it just seems quite time-consuming because it's not something I do very often
39 13:15 <jkczyz> Not being that familiar with the GUI code, are there many other non-manual way of testing?
40 13:15 <jnewbery> ok, so next question: How can this PR be tested (both manually and automatically)?
41 13:15 <jnewbery> jkczyz: snap!
42 13:16 <jkczyz> jnewbery: Should have read the questions more closely beforehand ;)
44 13:17 <jnewbery> I've never written any, but it looks like you can drive the GUI in the tests
45 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.
46 13:18 <michaelfolkson> I'm unsure on automated testing... You need user to click around and do weird stuff which is hard to automate?
47 13:19 <michaelfolkson> I suppose not
48 13:19 <jnewbery> michaelfolkson: yeah, you can drive that in the automated tests. Take a look at the PR I linked to
49 13:20 <jonatack> jnewbery: thanks! I need to dig into the qt tests.
50 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
51 13:20 <jnewbery> jonatack: wow. That's dedication!
52 13:21 <jnewbery> ok, any other questions about this PR? The code should be quite straightforward to review
53 13:22 <michaelfolkson> I think there was one thing I misunderstood
54 13:22 <michaelfolkson> Wallets should be in separate directories by default right? You talked about that in the notes
55 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?
56 13:23 <michaelfolkson> But there's also the directory where the list of the wallets is
57 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
58 13:24 <jnewbery> that was done as part of supporting 'external' wallets (wallets that aren't in bitcoind's default wallet directory)
59 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
60 13:26 <jnewbery> michaelfolkson: look at the help text for walletdir:
61 13:26 <jnewbery> -walletdir=<dir>
62 13:26 <jnewbery> Specify directory to hold wallets (default: <datadir>/wallets if it
63 13:26 <jnewbery> exists, otherwise <datadir>)
64 13:27 <jnewbery> any wallet not in that directory is an 'external' wallet
65 13:27 <michaelfolkson> Ah cool thanks
67 13:27 <jonatack> "External wallet files"
68 13:28 <emilengler> Are there also ways to get the list of external wallets with an RPC command?
69 13:28 <jnewbery> yes, I think 11687 is where that change was made
70 13:28 <jnewbery> emilengler: you can list all the open wallets
71 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.
72 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)
73 13:29 <jkczyz> ah, via functional tests. I see no unit tests tjhen
74 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
75 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.
76 13:33 <jonatack> Is that part of ryanofsky's modularity work these past couple years?
77 13:33 <jnewbery> jonatack: the logic move from rpc to wallet? That's the first commit in this PR (15204)
78 13:34 <jnewbery> not really that related to the ryanofsky modularity work
79 13:34 <jonatack> ty, gotcha
80 13:36 <jnewbery> any other questions?
81 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?
82 13:36 <jnewbery> We can wrap up early if no-one has anything
83 13:37 <jnewbery> michaelfolkson: that's a very broad question!
84 13:37 <michaelfolkson> All feature changes on the GUI have already been tested for months within bitcoind
85 13:37 <michaelfolkson> Haha
86 13:37 <michaelfolkson> We do have 25 mins lol
87 13:37 <jnewbery> any code 'widens the attack surface area'
88 13:37 <michaelfolkson> Ok but it is limited
89 13:39 <michaelfolkson> Any questions <sebastianvstaa>?
90 13:39 <jonatack> I've been under the impression that qt testing is generally punted here, for lack of a framework.
91 13:39 <sebastianvstaa> will try to be better prepared next time..
92 13:39 <jonatack> Will look further into qt unit tests like the link you provided...
93 13:39 <sebastianvstaa> didnt manage to build the PR and run the tests
95 13:40 <jnewbery> so theoretically that somewhat limits how much any issue with the GUI could leak into the node
96 13:40 <jnewbery> multiprocess support would take that further so memory isn't shared between the GUI process and node process
97 13:41 <jnewbery> jonatack: yeah, I don't think I've seen anyone apart from ryanofsky write qt tests
98 13:41 <jonatack> Yes. Removing global locking of the GUI seems highly desired.
99 13:41 <jnewbery> jonatack: that'd be great. If you open a qt test PR, perhaps you can present it here
100 13:42 <jonatack> jnewbery: ty, I didn't realize about qt testing. Worth looking into!
101 13:43 <jnewbery> ok, unless there are any final questions, lets wrap it up there
102 13:43 <jonatack> sebastianvstaa: what blocked you?
103 13:43 <sebastianvstaa> lack of knowledge of the build environment
104 13:43 <jonatack> we can discuss after if you like
105 13:43 <sebastianvstaa> still need to understand git better
106 13:43 <sebastianvstaa> yes please. that would be cool :)
107 13:44 <jonatack> thanks, everyone!
108 13:44 <michaelfolkson> Thanks!
109 13:44 <sebastianvstaa> thanks!
110 13:45 <michaelfolkson> You going to chat here or privately <jonatack>?
111 13:45 <michaelfolkson> I'd listen in if here
112 13:45 <jonatack> here unless anyone minds?
113 13:45 <sebastianvstaa> ok
114 13:45 <jonatack> sebastianvstaa: for building, what platform are you on?
115 13:46 <sebastianvstaa> ubuntu 18.04
116 13:46 <sebastianvstaa> managed to build core before
117 13:46 <sebastianvstaa> but not sure how to switch to the PR and build that
118 13:47 <sebastianvstaa> installed smartgit, but if you could show how to do it from command line, thats cool as well
119 13:47 <jonatack> sebastianvstaa: ok, you have detailed instructions in the repo at doc/build-unix.md or similar
120 13:47 <jnewbery> thanks everyone. I'll duck out now, but please feel free to continue the discussion here!
121 13:47 <sebastianvstaa> thanks jon!
122 13:47 <jonatack> cheers John!
123 13:47 <sebastianvstaa> *john
124 13:47 <clarkmoody> Adios
125 13:47 <michaelfolkson> Thanks John!
128 13:49 <sebastianvstaa> ok will check it
129 13:49 <jonatack> sebastianvstaa: once you've built, go throught the productivity notes and developer notes in /doc
130 13:49 <sebastianvstaa> ok. have the build the whole thing before
131 13:49 <sebastianvstaa> but how to switch to the PR code and build that specifically?
132 13:50 <michaelfolkson> Basically clone the author of the PR's fork and checkout the PR branch
133 13:50 <michaelfolkson> That's what I've done
134 13:50 <jonatack> sebastianvstaa: notably, install ccache, and add git refspecs to your gitconfig
135 13:50 <sebastianvstaa> ok
137 13:53 <sebastianvstaa> # Enable pulling in remote PRs and testing locally with: git checkout pr/999
139 13:53 <sebastianvstaa> [remote "origin"]
140 13:53 <sebastianvstaa> fetch = +refs/heads/*:refs/remotes/origin/*
141 13:53 <sebastianvstaa> fetch = +refs/pull/*/head:refs/remotes/origin/pr/*
142 13:53 <sebastianvstaa> # Reference Bitcoin PRs easily with refspecs.
143 13:53 <sebastianvstaa> # This adds an upstream-pull remote to your git repository, which can be fetched
144 13:53 <sebastianvstaa> # using git fetch --all or git fetch upstream-pull. Afterwards, you can use
145 13:53 <sebastianvstaa> # upstream-pull/NUMBER/head in arguments to git show, git checkout and anywhere
146 13:53 <sebastianvstaa> # a commit id would be acceptable to see the changes from pull request NUMBER.
147 13:53 <sebastianvstaa> # [remote "upstream-pull"]
148 13:53 <sebastianvstaa> # fetch = +refs/pull/*:refs/remotes/upstream-pull/*
149 13:53 <sebastianvstaa> # url = firstname.lastname@example.org:bitcoin/bitcoin.git
150 13:53 <sebastianvstaa> hmmm. ok. this?
151 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:
153 13:54 <jonatack> sebastianvstaa: warning, it's nearly a book now :D
154 13:54 <michaelfolkson> I'm here if you need basic questions! Jon is very friendly if you have non basic questions
155 13:54 <sebastianvstaa> thanks jonatack. will work through it
156 13:54 <sebastianvstaa> ok, thanks!
157 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
158 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.
159 13:57 <jonatack> But the gitconfig I sent you is more than enough to get started.
160 13:58 <jonatack> michaelfolkson: thanks!
161 14:00 <jonatack> sebastianvstaa: last thing, to start running qt from a PR build, run from the repo root: src/qt/bitcoin-qt -testnet
162 14:01 <sebastianvstaa> ok
163 14:01 <jonatack> (to start running the GUI wallet, sorry)
164 14:02 <jonatack> sebastianvstaa: any problems, don't hesitate to PM us :)
165 14:05 <sebastianvstaa> jonatack: sure, thanks!