Create wallet menu option (gui)

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

  113:02 <jnewbery> hi!
  213:02 <dmkathayat_> Hi!
  313:02 <davterra> Hi!
  413:02 <ariard> hi
  513:03 <peevsie> yo
  613:03 <jnewbery> Today's PR is https://github.com/bitcoin/bitcoin/pull/15450
  713:03 <pinheadmz> buenos dias
  813:03 <emzy> Hi
  913:03 <michaelfolkson> What up. Head spinning trying to work out how all these PRs overlap lol
 1013: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
 1113:03 <jonatack> hi
 1213:04 <jnewbery> So rather than me talking for 5 minutes now, let's just get straight into questions
 1313:04 <jonatack> is no testing for qt a thing
 1413:05 <jnewbery> Most qt PRs don't include tests
 1513:05 <jnewbery> But there are some in src/qt/test
 1613:06 <MarcoFalke> Yeah, and those are more like gui unit tests, not actual gui tests
 1713:07 <MarcoFalke> Currently the only way to test the GUI is by running it by hand
 1813: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?
 1913: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
 2013:10 <jnewbery> You can see that it's mostly just parsing arguments and then calling CWallet::CreateWalletFromFile() which does the heavy lifting
 2113:11 <jnewbery> This PR adds a new method to the node interface called createWallet() which basically does the same stuff
 2213:11 <jnewbery> https://github.com/bitcoin/bitcoin/pull/15450/files#diff-7e7292143c1cc2b4125d85f1ae5cf046R261
 2313: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.
 2413:12 <jonatack> unit tests
 2513:12 <jnewbery> jonatack: Yes, I think that would be valuable contribution
 2613:12 <jonatack> end to end functional ones?
 2713:12 <jnewbery> yes
 2813:12 <jonatack> ty
 2913:13 <jnewbery> I don't know anything about testing QT
 3013:13 <jnewbery> But I imagine there's documentation on the QT website
 3113:13 <MarcoFalke> Indeed, I'd welcome any progress in that direction
 3213: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.
 3313:13 <MarcoFalke> I don't know if there are existing framworks
 3413: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
 3513: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
 3613: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
 3713: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
 3813:17 <jnewbery> Yep, it's all C++
 3913:19 <jnewbery> Did everyone manage to build the PR and do some manual testing?
 4013:19 <jnewbery> Any questions about that?
 4113:20 <jnewbery> I guess that's it for today. Short meeting!
 4213:20 <jnewbery> Before we go, does anyone have requests for future PRs to cover?
 4313:21 <jonatack> i compiled but didn't run it yet because i didn't see it in root or source
 4413:21 <jonatack> any pointers ?
 4513:21 <jnewbery> jonatack: it should be at src/qt/bitcoin-qt
 4613:21 <jnewbery> if not, perhaps you're not configured to build the qt binary. Check the output from when you ran `configure`
 4713:22 <jnewbery> You should see: 'with gui / qt = yes'
 4813:22 <jonatack> all good
 4913:22 <emzy> I had no time to do it. Maybe I will check it later.
 5013:22 <michaelfolkson> For the creating blank empty wallets PR, what's the motivation for doing this? Importing a seed from elsewhere?
 5113:22 <jonatack> i never run qt, thanks for the tpi
 5213:22 <jonatack> tip
 5313:22 <michaelfolkson> #15226
 5413:23 <jnewbery> michaelfolkson: by default, a new wallet will generate an HD seed
 5513: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
 5613:23 <jnewbery> if you want to import your own seed, you can create a blank wallet and then import
 5713: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
 5813:25 <jnewbery> #15226 also made #15006 (Add option to create an encrypted wallet) much simpler
 5913:25 <jonatack> thanks marco
 6013: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?
 6113:26 <MarcoFalke> Created an issue for that https://github.com/bitcoin/bitcoin/issues/16075
 6213:26 <MarcoFalke> jonatack: et al ^
 6313:26 <ariard> Do this also mean things have been modified which make tests longer to run ?
 6413:26 <jonatack> MarcoFalke: nice
 6513:26 <jnewbery> ariard: Yes, I believe force-pushing the branch will trigger a rebuild on travis
 6613:27 <jnewbery> just run `git commit --amend` to update the timestamp and then `git force push`
 6713:27 <jonatack> today's pr has seen no activity since 3 months.
 6813:28 <ariard> jnewbery: thanks for tip!
 6913:28 <jonatack> does achow have too much on his plate, does he need more help
 7013:28 <jonatack> or is it not a priority
 7113: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
 7213:29 <jonatack> right
 7313: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
 7413:32 <jnewbery> any other questions?
 7513:32 <jonatack> MarcoFalke:
 7613:32 <jonatack> do you plan to resubmit a pr
 7713:32 <jonatack> for the f tests speedup
 7813:32 <jonatack> you had 2
 7913:32 <michaelfolkson> achow is overseeing the Bitcoin Core hardware wallet interface too isn't he? That is ongoing
 8013:33 <MarcoFalke> jonatack: Good question. Not sure if we are allowed to discuss other prs here
 8113:33 <MarcoFalke> jnewbery: ? ^
 8213:33 <jonatack> related to ariard's q
 8313: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)
 8413:34 <jonatack> ok
 8513:35 <jonatack> qt is now running for me ty for the help jnewbery
 8613:36 <jnewbery> michaelfolkson: yes, achow maintains HWI
 8713:36 <jnewbery> jonatack: great!
 8813:36 <jnewbery> ok, unless there are any final questions, let's wrap it up.
 8913:37 <jnewbery> Thanks all! See you next week
 9013:37 <emzy> Tnx!
 9113:37 <jonatack> cheers! thanks everyone
 9213:38 <michaelfolkson> A quick final question? :)
 9313:38 <jnewbery> go for it!
 9413:39 <jnewbery> michaelfolkson: was 'A quick final question?' the final question or is there a question after that question?
 9513: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?
 9613:40 <michaelfolkson> Sorry yes
 9713:40 <jnewbery> Which different PRs? This one doesn't currently conflict with anything
 9813:40 <michaelfolkson> And there needs to be more feedback on the design. I saw Sjors gave some suggestions for what the UI should be
 9913:41 <michaelfolkson> I was seeing a bunch of conflicts on one of the PRs you linked to
10013:42 <jnewbery> Yeah, i think it's still appropriate to give design feedback at this stage
10113:42 <jonatack> michael: perhaps adding a recent review can kick the pr back into action
10213:42 <jnewbery> I agree. achow will probably be more motivated to work on it if he knows there are people keen to review
10313:44 <michaelfolkson> Ok cool. It is just design considerations now right?
10413:44 <jnewbery> No, the code needs review too
10513:44 <jnewbery> all aspects of the PR
10613:45 <michaelfolkson> Ok understood, thanks
10713:45 <michaelfolkson> Ok I'll let you go, many thanks
10813:46 <jnewbery> ok, thanks again. See you all next time!