Add Open External Wallet action (gui)

Sep 18, 2019

https://github.com/bitcoin/bitcoin/pull/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

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

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!

126

13:47

<jkczyz> bye!

127

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

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

136

13:51

<jonatack> sebastianvstaa: see/copy/steal lines 212-225 of my gitconfig: https://github.com/jonatack/dotfiles/blob/master/gitconfig

137

13:53

<sebastianvstaa> # Enable pulling in remote PRs and testing locally with: git checkout pr/999

138

13:53

<sebastianvstaa> # See this gist: https://gist.github.com/piscisaureus/3342247

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

152

13:53

<jonatack> sebastianvstaa: https://github.com/jonatack/bitcoin-development/blob/master/how-to-review-bitcoin-core-prs.md

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