Create wallet menu option (gui)
- 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
createwalletRPC 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.
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!