#15450 Create wallet menu option (gui)

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

Host: jnewbery

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

13:02 < jnewbery> hi!
13:02 < dmkathayat_> Hi!
13:02 < davterra> Hi!
13:02 < ariard> hi
13:03 < peevsie> yo
13:03 < jnewbery> Today's PR is https://github.com/bitcoin/bitcoin/pull/15450
13:03 < pinheadmz> buenos dias
13:03 < emzy> Hi
13:03 < michaelfolkson> What up. Head spinning trying to work out how all these PRs overlap lol
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
13:03 < jonatack> hi
13:04 < jnewbery> So rather than me talking for 5 minutes now, let's just get straight into questions
13:04 < jonatack> is no testing for qt a thing
13:05 < jnewbery> Most qt PRs don't include tests
13:05 < jnewbery> But there are some in src/qt/test
13:06 < MarcoFalke> Yeah, and those are more like gui unit tests, not actual gui tests
13:07 < MarcoFalke> Currently the only way to test the GUI is by running it by hand
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?
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
13:10 < jnewbery> You can see that it's mostly just parsing arguments and then calling CWallet::CreateWalletFromFile() which does the heavy lifting
13:11 < jnewbery> This PR adds a new method to the node interface called createWallet() which basically does the same stuff
13:11 < jnewbery> https://github.com/bitcoin/bitcoin/pull/15450/files#diff-7e7292143c1cc2b4125d85f1ae5cf046R261
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.
13:12 < jonatack> unit tests
13:12 < jnewbery> jonatack: Yes, I think that would be valuable contribution
13:12 < jonatack> end to end functional ones?
13:12 < jnewbery> yes
13:12 < jonatack> ty
13:13 < jnewbery> I don't know anything about testing QT
13:13 < jnewbery> But I imagine there's documentation on the QT website
13:13 < MarcoFalke> Indeed, I'd welcome any progress in that direction
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.
13:13 < MarcoFalke> I don't know if there are existing framworks
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
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
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
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
13:17 < jnewbery> Yep, it's all C++
13:19 < jnewbery> Did everyone manage to build the PR and do some manual testing?
13:19 < jnewbery> Any questions about that?
13:20 < jnewbery> I guess that's it for today. Short meeting!
13:20 < jnewbery> Before we go, does anyone have requests for future PRs to cover?
13:21 < jonatack> i compiled but didn't run it yet because i didn't see it in root or source
13:21 < jonatack> any pointers ?
13:21 < jnewbery> jonatack: it should be at src/qt/bitcoin-qt
13:21 < jnewbery> if not, perhaps you're not configured to build the qt binary. Check the output from when you ran `configure`
13:22 < jnewbery> You should see: 'with gui / qt = yes'
13:22 < jonatack> all good
13:22 < emzy> I had no time to do it. Maybe I will check it later.
13:22 < michaelfolkson> For the creating blank empty wallets PR, what's the motivation for doing this? Importing a seed from elsewhere?
13:22 < jonatack> i never run qt, thanks for the tpi
13:22 < jonatack> tip
13:22 < michaelfolkson> #15226
13:23 < jnewbery> michaelfolkson: by default, a new wallet will generate an HD seed
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
13:23 < jnewbery> if you want to import your own seed, you can create a blank wallet and then import
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
13:25 < jnewbery> #15226 also made #15006 (Add option to create an encrypted wallet) much simpler
13:25 < jonatack> thanks marco
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?
13:26 < MarcoFalke> Created an issue for that https://github.com/bitcoin/bitcoin/issues/16075
13:26 < MarcoFalke> jonatack: et al ^
13:26 < ariard> Do this also mean things have been modified which make tests longer to run ?
13:26 < jonatack> MarcoFalke: nice
13:26 < jnewbery> ariard: Yes, I believe force-pushing the branch will trigger a rebuild on travis
13:27 < jnewbery> just run `git commit --amend` to update the timestamp and then `git force push`
13:27 < jonatack> today's pr has seen no activity since 3 months.
13:28 < ariard> jnewbery: thanks for tip!
13:28 < jonatack> does achow have too much on his plate, does he need more help
13:28 < jonatack> or is it not a priority
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
13:29 < jonatack> right
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
13:32 < jnewbery> any other questions?
13:32 < jonatack> MarcoFalke:
13:32 < jonatack> do you plan to resubmit a pr
13:32 < jonatack> for the f tests speedup
13:32 < jonatack> you had 2
13:32 < michaelfolkson> achow is overseeing the Bitcoin Core hardware wallet interface too isn't he? That is ongoing
13:33 < MarcoFalke> jonatack: Good question. Not sure if we are allowed to discuss other prs here
13:33 < MarcoFalke> jnewbery: ? ^
13:33 < jonatack> related to ariard's q
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)
13:34 < jonatack> ok
13:35 < jonatack> qt is now running for me ty for the help jnewbery
13:36 < jnewbery> michaelfolkson: yes, achow maintains HWI
13:36 < jnewbery> jonatack: great!
13:36 < jnewbery> ok, unless there are any final questions, let's wrap it up.
13:37 < jnewbery> Thanks all! See you next week
13:37 < emzy> Tnx!
13:37 < jonatack> cheers! thanks everyone
13:38 < michaelfolkson> A quick final question? :)
13:38 < jnewbery> go for it!
13:39 < jnewbery> michaelfolkson: was 'A quick final question?' the final question or is there a question after that question?
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?
13:40 < michaelfolkson> Sorry yes
13:40 < jnewbery> Which different PRs? This one doesn't currently conflict with anything
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
13:41 < michaelfolkson> I was seeing a bunch of conflicts on one of the PRs you linked to
13:42 < jnewbery> Yeah, i think it's still appropriate to give design feedback at this stage
13:42 < jonatack> michael: perhaps adding a recent review can kick the pr back into action
13:42 < jnewbery> I agree. achow will probably be more motivated to work on it if he knows there are people keen to review
13:44 < michaelfolkson> Ok cool. It is just design considerations now right?
13:44 < jnewbery> No, the code needs review too
13:44 < jnewbery> all aspects of the PR
13:45 < michaelfolkson> Ok understood, thanks
13:45 < michaelfolkson> Ok I'll let you go, many thanks
13:46 < jnewbery> ok, thanks again. See you all next time!