Create wallet menu option (gui)

May 22, 2019

https://github.com/bitcoin/bitcoin/pull/15450

Host: jnewbery  -  PR author: achow101

Notes

• Bitcoin uses QT for its GUI. The minimum version is 5.5. Online documentation.
• This PR is one of the last in a series of multiwallet PRs. The first of those was #8694 which added multiwallet support through the RPC interface and was merged in June 2017.
• Adding the various multiwallet functions (create/open/close wallet) at runtime is tracked by Issue 13059.
• We already have UI support for opening and closing wallets. This PR adds UI support for creating wallets.
• Creating wallets dynamically is relatively new functionality (merged in #13058 in June 2018). There have been a couple of additional options added to the createwallet RPC since it was added: Disable private keys, Blank wallets and born-encrypted wallets.
• This PR was opened before the born-encrypted wallets PR was merged, so potentially could be simplified now that the functionality exists within the wallet component.
• For GUI PRs, I always like to build and manually test the PR to get a sense of what the new UI functionality feels like. If you’re testing on the same machine that you run your mainnet node on, REMEMBER TO MOVE OR BACKUP YOUR WALLET/DATADIR BEFORE TESTING!
• Feel free to leave feedback on UI PRs about usability as well as comments on the code.

Meeting Log

1

13:02

<jnewbery> hi!

2

13:02

<dmkathayat_> Hi!

3

13:02

<davterra> Hi!

4

13:02

<ariard> hi

5

13:03

<peevsie> yo

6

13:03

<jnewbery> Today's PR is https://github.com/bitcoin/bitcoin/pull/15450

7

13:03

<pinheadmz> buenos dias

8

13:03

<emzy> Hi

9

13:03

<michaelfolkson> What up. Head spinning trying to work out how all these PRs overlap lol

10

13:03

<jnewbery> I wrote my review notes here: https://bitcoin-core-review-club.github.io/15450.html to save us some time at the start of the meeting

11

13:03

<jonatack> hi

12

13:04

<jnewbery> So rather than me talking for 5 minutes now, let's just get straight into questions

13

13:04

<jonatack> is no testing for qt a thing

14

13:05

<jnewbery> Most qt PRs don't include tests

15

13:05

<jnewbery> But there are some in src/qt/test

16

13:06

<MarcoFalke> Yeah, and those are more like gui unit tests, not actual gui tests

17

13:07

<MarcoFalke> Currently the only way to test the GUI is by running it by hand

18

13:08

<michaelfolkson> So to confirm I understand what is going on. You can currently create a new independent HD wallet using bitcoind but until now you couldn't using Qt. Qt can't leverage bitcoind RPC so the C++ code needs to be written afresh for Qt in addition to editing the GUI?

19

13:10

<jnewbery> michaelfolkson: we try to make the RPC layer as thin as possible so functionality can be shared between RPC and QT. Here's the createwallet RPC: https://github.com/bitcoin/bitcoin/blob/1c177c3a004f91eca743bb3a0dd9534a544026d5/src/wallet/rpcwallet.cpp#L2642

20

13:10

<jnewbery> You can see that it's mostly just parsing arguments and then calling CWallet::CreateWalletFromFile() which does the heavy lifting

21

13:11

<jnewbery> This PR adds a new method to the node interface called createWallet() which basically does the same stuff

22

13:11

<jnewbery> https://github.com/bitcoin/bitcoin/pull/15450/files#diff-7e7292143c1cc2b4125d85f1ae5cf046R261

23

13:12

<jonatack> thanks! i suppose then that units are added for any new non-qt code. seem might still be good to add e2e tests for the user flows.

24

13:12

<jonatack> unit tests

25

13:12

<jnewbery> jonatack: Yes, I think that would be valuable contribution

26

13:12

<jonatack> end to end functional ones?

27

13:12

<jnewbery> yes

28

13:12

<jonatack> ty

29

13:13

<jnewbery> I don't know anything about testing QT

30

13:13

<jnewbery> But I imagine there's documentation on the QT website

31

13:13

<MarcoFalke> Indeed, I'd welcome any progress in that direction

32

13:13

<jnewbery> michaelfolkson: to follow up on my comment in the review notes "This PR was opened before the born-encrypted wallets PR was merged, so potentially could be simplified now that the functionality exists within the wallet component.

33

13:13

<MarcoFalke> I don't know if there are existing framworks

34

13:14

<jnewbery> You can see here: https://github.com/bitcoin/bitcoin/pull/15450/files#diff-fef859e81d3321e85221891fd768efadR75 that there's some duplication of logic for creating an encrypted wallet

35

13:16

<jnewbery> that logic now exists in the wallet RPC: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L2710 . Perhaps this could be factored out of there into wallet.cpp and then shared by the RPC and QT

36

13:16

<michaelfolkson> <jnewbery>: Thanks, that's useful. I wasn't expecting to see any C++ code for this PR, I was expecting to just be looking at GUI design. But I forgot that Qt wasn't just a front end

37

13:16

<jnewbery> I'm not entirely sure of that though. I haven't checked whether there are subtle differences in what logic is required for QT and RPC

38

13:17

<jnewbery> Yep, it's all C++

39

13:19

<jnewbery> Did everyone manage to build the PR and do some manual testing?

40

13:19

<jnewbery> Any questions about that?

41

13:20

<jnewbery> I guess that's it for today. Short meeting!

42

13:20

<jnewbery> Before we go, does anyone have requests for future PRs to cover?

43

13:21

<jonatack> i compiled but didn't run it yet because i didn't see it in root or source

44

13:21

<jonatack> any pointers ?

45

13:21

<jnewbery> jonatack: it should be at src/qt/bitcoin-qt

46

13:21

<jnewbery> if not, perhaps you're not configured to build the qt binary. Check the output from when you ran `configure`

47

13:22

<jnewbery> You should see: 'with gui / qt = yes'

48

13:22

<jonatack> all good

49

13:22

<emzy> I had no time to do it. Maybe I will check it later.

50

13:22

<michaelfolkson> For the creating blank empty wallets PR, what's the motivation for doing this? Importing a seed from elsewhere?

51

13:22

<jonatack> i never run qt, thanks for the tpi

52

13:22

<jonatack> tip

53

13:22

<michaelfolkson> #15226

54

13:23

<jnewbery> michaelfolkson: by default, a new wallet will generate an HD seed

55

13:23

<MarcoFalke> To follow up on the "How to test the GUI" question earlier: The only framework I am aware of is used by operating systems: OpenQA. I think it is a Open Suse in-house product. See https://openqa.opensuse.org/tests/940238#step/bootloader/3

56

13:23

<jnewbery> if you want to import your own seed, you can create a blank wallet and then import

57

13:24

<jnewbery> another motivation: when you encrypt the wallet, it'll create a new seed, so if you created a wallet that wasn't blank, you'd have a seed from before encryption and then a seed from after encryption, which perhaps you don't want

58

13:25

<jnewbery> #15226 also made #15006 (Add option to create an encrypted wallet) much simpler

59

13:25

<jonatack> thanks marco

60

13:25

<ariard> I had a a more general question which is on travis build process, I often get a timeout error "Error! Initial build successful, but not enough time..." What's the process to do after that ? Force-push new branch tip?

61

13:26

<MarcoFalke> Created an issue for that https://github.com/bitcoin/bitcoin/issues/16075

62

13:26

<MarcoFalke> jonatack: et al ^

63

13:26

<ariard> Do this also mean things have been modified which make tests longer to run ?

64

13:26

<jonatack> MarcoFalke: nice

65

13:26

<jnewbery> ariard: Yes, I believe force-pushing the branch will trigger a rebuild on travis

66

13:27

<jnewbery> just run `git commit --amend` to update the timestamp and then `git force push`

67

13:27

<jonatack> today's pr has seen no activity since 3 months.

68

13:28

<ariard> jnewbery: thanks for tip!

69

13:28

<jonatack> does achow have too much on his plate, does he need more help

70

13:28

<jonatack> or is it not a priority

71

13:29

<jnewbery> It might just not be a priority for him. I know he has a PR for a descriptor-based wallet, which is large and more important

72

13:29

<jonatack> right

73

13:30

<jnewbery> In general, if you see PRs that haven't had attention from their author for a while, there's nothing wrong in offering to take over maintaining the PR

74

13:32

<jnewbery> any other questions?

75

13:32

<jonatack> MarcoFalke:

76

13:32

<jonatack> do you plan to resubmit a pr

77

13:32

<jonatack> for the f tests speedup

78

13:32

<jonatack> you had 2

79

13:32

<michaelfolkson> achow is overseeing the Bitcoin Core hardware wallet interface too isn't he? That is ongoing

80

13:33

<MarcoFalke> jonatack: Good question. Not sure if we are allowed to discuss other prs here

81

13:33

<MarcoFalke> jnewbery: ? ^

82

13:33

<jonatack> related to ariard's q

83

13:33

<jnewbery> Is it directly related to this week's PR? If not, I think #bitcoin-core-dev is the better place to discuss it (so all interested parties can see it)

84

13:34

<jonatack> ok

85

13:35

<jonatack> qt is now running for me ty for the help jnewbery

86

13:36

<jnewbery> michaelfolkson: yes, achow maintains HWI

87

13:36

<jnewbery> jonatack: great!

88

13:36

<jnewbery> ok, unless there are any final questions, let's wrap it up.

89

13:37

<jnewbery> Thanks all! See you next week

90

13:37

<emzy> Tnx!

91

13:37

<jonatack> cheers! thanks everyone

92

13:38

<michaelfolkson> A quick final question? :)

93

13:38

<jnewbery> go for it!

94

13:39

<jnewbery> michaelfolkson: was 'A quick final question?' the final question or is there a question after that question?

95

13:40

<michaelfolkson> So what are next steps for advancing this? Someone who understands the code for the GUI needs to oversee all the merge conflicts between these different PRs?

96

13:40

<michaelfolkson> Sorry yes

97

13:40

<jnewbery> Which different PRs? This one doesn't currently conflict with anything

98

13:40

<michaelfolkson> And there needs to be more feedback on the design. I saw Sjors gave some suggestions for what the UI should be

99

13:41

<michaelfolkson> I was seeing a bunch of conflicts on one of the PRs you linked to

100

13:42

<jnewbery> Yeah, i think it's still appropriate to give design feedback at this stage

101

13:42

<jonatack> michael: perhaps adding a recent review can kick the pr back into action

102

13:42

<jnewbery> I agree. achow will probably be more motivated to work on it if he knows there are people keen to review

103

13:44

<michaelfolkson> Ok cool. It is just design considerations now right?

104

13:44

<jnewbery> No, the code needs review too

105

13:44

<jnewbery> all aspects of the PR

106

13:45

<michaelfolkson> Ok understood, thanks

107

13:45

<michaelfolkson> Ok I'll let you go, many thanks

108

13:46

<jnewbery> ok, thanks again. See you all next time!