#16244 Move wallet creation out of the createwallet rpc into its own function (wallet)
- This is a small refactor PR which makes PR 15450 (which we reviewed a few weeks ago) simpler and cleaner.
- From that review club: “we try to make the RPC layer as thin as possible so functionality can be shared between RPC and QT”. This PR applies that rule to the
- There should be no behaviour change from this PR.
- We ensure that refactors don’t change behaviour by having the functionality covered by tests. Where is this functionality tested?
- Are all the failure modes tested? If not, why not?
- Where is the
- There are several suggestions for changing the function from ryanofsky, promag and empact. Are those good suggestions?
13:00 < jnewbery> hi 13:00 < fjahr> hi 13:00 < behradkhodayar> Hi 13:00 < digi_james> hello 13:00 < elichai2> Hi :) 13:00 < pinheadmz> oy! 13:00 < hugohn> aloha 13:00 < nijak> hello hello 13:00 < lightlike> hi 13:01 < jnewbery> Notes and questionds here: https://bitcoin-core-review-club.github.io/16244.html 13:02 < jnewbery> My internet connection is a bit flakey today, so apologies if I drop. You'll just have to continue without me! 13:02 < jnewbery> who had a chance to review the PR and notes? 13:02 < digi_james> Yup 13:02 < lightlike> me too 13:02 < fjahr> 🙋♂️ 13:02 < pinheadmz> read through the code 13:03 < elichai2> yea 13:03 < jnewbery> Great. First question should be pretty simple. Where is this code tested? 13:03 < fjahr> functional/rpc_createwallet 13:04 < pinheadmz> well... funcitonal/wallet_createwallet.py ? 13:04 < jnewbery> Good guess, but it's not quite right. 13:04 < lightlike> functional/wallet_multiwallet.py 13:04 < jnewbery> All the wallet functional tests are prefixed with wallet_ 13:04 < jnewbery> Yeah, wallet_createwallet.py 13:05 < jonas_> çççç 13:05 < jonas_> typo! 13:06 < jnewbery> So when reviewing this PR, I think it's worth reading through that test to satisfy yourselves that the functinoality is tested 13:07 < elichai2> Is there a "integration test" that verifies that the wallet file was created correctly? 13:07 < hugohn> in the PR, variable decelerations are moved closer to where they are used (instead of being at the top of the function), is that a Core style thing? 13:07 < elichai2> (in case of modifications like this to the `CreateWallet` function) 13:08 < jnewbery> Next up: did people check taht the different failure modes are tested? 13:08 < digi_james> elichai2: In regards to the wallet files, I suspect in the db tests .... 13:08 < jnewbery> elichai: I'd call wallet_createwallet.py an integration test. It's creating the wallet and then running RPCs like `getwalletinfo` 13:09 < jnewbery> but there's no test on the actual wallet file 13:09 < fjahr> I did not get a complete overview but I think I did not see the 'wallet already exists' case being tested 13:09 < hugohn> *declaration 13:09 < jnewbery> I'd expect that to be covered by the wallet_multiwallet.py test, which tests wallets being unloaded and reloaded 13:09 < elichai2> but the fact that `getwalletinfo` workd correctly on `CreateWallet` doesn't mean the wallet format didn't change, right? 13:10 < jnewbery> hugohn: usually declarations are just above where the variable is used, but I think that's just convention rather than anything in the style guide 13:10 < lightlike> fjahr: that one is tested in wallet_multiwallet.py 13:11 < fjahr> jnewbery: true, it's there 13:11 < jnewbery> (and obviously don't open PRs to 'fix' code to use that convention) 13:12 < jnewbery> I think there are some error conditions that it's quite difficult to simulate in the test framework. For example if there are file system errors 13:12 < digi_james> I think thats what the warning string is for 13:12 < digi_james> (file system error) 13:12 < jnewbery> Right, I was about to ask. Where is the warning string used? 13:13 < digi_james> wait no, warning if it successfully recovers corrupt file 13:13 < digi_james> the berkeley db wrapper I believe 13:14 < jnewbery> Right, i found it quite interesting that this warning string that's passed up and down the stack is only actually used in one place 13:14 < fjahr> in VerifyWallets? 13:14 < digi_james> :) 13:15 < pinheadmz> Can I ask how the tests are calling the right function? 13:15 < jnewbery> here: https://github.com/bitcoin/bitcoin/blob/6c1e45c4c41676f80ac6fb8d48cfbcf839593f19/src/wallet/db.cpp#L426 13:15 < pinheadmz> the tests call createwallet() 13:15 < pinheadmz> and I see CreateWallet in wallettool.cpp 13:16 < pinheadmz> this PR adds CreateWallet to wallet.cpp 13:16 < pinheadmz> but I dont undertand which function the tests used to call - and why they call the new one now 13:16 < jnewbery> pinheadmz: yes, the createwallet call in the functional test is to the RPC method 13:16 < pinheadmz> oh ok its just to the RPC 13:16 < pinheadmz> and the PR redirects the actual wallet creation to wallet.cpp 13:16 < pinheadmz> so whats the method in wallettool ? 13:17 < jnewbery> right, in a call like self.nodes.createwallet(), that createwallet() gets converted into an RPC call 13:17 < jnewbery> take a look at the TestNode() class in the test framework 13:18 < jnewbery> particularly the __getattr__() method 13:18 < pinheadmz> "Dispatches any unrecognised messages to the RPC connection or a CLI instance" -- clever 13:19 < jnewbery> pinheadmz: yes the PR creates a new CreateWallet() function in libbitcoin_wallet 13:19 < jnewbery> ok, last question from me. There are several suggestions for changing the function from ryanofsky, promag and empact. Are those good suggestions? 13:20 < behradkhodayar> So what about covering "Are all the failure modes tested? If not, why not?" 13:21 < jnewbery> behradkhodayar: did you have anything to add on that? Any failure modes you saw that aren't covered? 13:22 < behradkhodayar> jnewbery: Sorry, Just missed it. TBT, I was expecting a talk about candidate occasions may makes a kind of failure. 13:22 < behradkhodayar> Not yet! 13:23 < hugohn> RE: names. do you think the RPC* methods should be maybe prefixed with rpc_ or something, to avoid having the same function names everywhere? slightly confusing 13:23 < jnewbery> ok, did anyone have any other questions about the PR? 13:23 < jnewbery> hugohn: all RPC methods are lowercasewithoutspaces . Most functions in the codebase are CamelCase 13:24 < hugohn> ic 13:24 < jnewbery> once you know that convention, it's not confusing 13:24 < digi_james> I am unsure of the flags, some is handled before CreateWallet, some of the flag manipulation happen sin CreateWallet. The discussion in regards to consistency with QT i couldnt fully understand. 13:25 < jnewbery> digi_james: which comment? 13:26 < digi_james> "new create wallet function [could take] separate option arguments instead of wallet flags 13:27 < lightlike> jnewbery: I found it interesting that the author decided not to address nits in order not to invalidate existing ACKs. Would prior ACKs really be ignored if he changed something small like enum to enum class? 13:28 < jnewbery> digi_james: the flags are persistant and stored in the wallet bdb file. I think Russ's comment was that the RPC method shouldn't really use these flags. It should just take options and pass them through as booleans to the CreateWallet() function 13:29 < digi_james> jnewbery: I see ... 13:29 < achow101> digi_james: I felt that a lot of bools would have the same effect as using the wallet flags because it basically is. it's also less typing to use the flags 13:29 < jnewbery> lightlike: Yes. When reviewers ACK, they ACK the commit hash. _Any_ change to the branch changes the commit hash, so invalidates the existing ACKs 13:29 < achow101> lightlike: the github-merge script used to merge PRs will not detect ACKs on hold commit hashes 13:29 < achow101> s/hold/old 13:30 < jnewbery> I thought this might be interesting for people here: https://github.com/bitcoin/bitcoin/pull/16244#issuecomment-509974339 13:30 < jnewbery> One thing that you'll need to get used to if you want to be a Bitcoin Core reviewer is that you'll need to rereview PRs quite a lot 13:31 < jnewbery> whenever a PR needs rebase, then your ACKs will be invalidated and you'll need to rereview 13:31 < pinheadmz> are ACK, utACK messages and their commit IDs actually parsed by anything besides... people? :-) 13:32 < achow101> pinheadmz: github-merge.py script used for merging 13:32 < pinheadmz> oh interesting, so they really are invalidated by new commits 13:32 < achow101> take a look at recent PR merges, they all contain the ACKs of the PR for the commit that was merged 13:32 < hugohn> I do agree in principle that external-facing APIs should be more explicit regarding param names, options, etc. The optimization (turned into a bitmask) can be done at a lower level. Helps readability & debugging. 13:32 < jnewbery> what you want to check is basically an interdiff - that the diff of the pre-rebase branch is equivalent to the diff of the post-rebase branch. If it's a simple rebase, then often I'll just replicate the rebase myself and verify that I get the same end result as the author 13:32 < jnewbery> if so, I'm satisfied that I can ACK the new commit hash 13:33 < jnewbery> Yes, just run git log on your master branch and look at the PR merge commits - you'll see that they contain the ACKs that the PR received 13:34 < pinheadmz> very cool 13:35 < jnewbery> hugohn: yeah, I tend to agree, but also agree that it can be done as a follow-up PR 13:35 < jnewbery> pinheadmz: if we were super paranoid we could sign our ACKs 13:36 < pinheadmz> ive seen that before in bitcoin core reviews 13:36 < jnewbery> I think Marco is the only one who signs his github review comments 13:36 < pinheadmz> haha 13:36 < pinheadmz> and even with the merge script incorporating ACKs, etc - the commit is still signed by the maintainer's signign key 13:37 < pinheadmz> (thought I saw a luke-jr signed ACK before too :-) ) 13:38 < jnewbery> any other questions on the code changes? 13:39 < behradkhodayar> This was my first session here & I found it very useful. Thank you everyone 13:39 < behradkhodayar> Just one general question: Are we going to cover ALL upcomming PRs here in this series of meetings? 13:39 < digi_james> I am unsure of how the rpc methods of the wallet can be easily decoupled compared to node rpc methods. 13:40 < digi_james> I've been digging around and noticed the wallet rpc methods are registered in the RpcHandlerImpl constructor, the global rpcTable 13:40 < achow101> digi_james: decoupled in what way? 13:40 < digi_james> For example, if the wallet were to directly receive rpc calls 13:40 < jnewbery> behradkhodayar: we cover one per week. If you have any requests, please comment on https://github.com/bitcoin-core-review-club/bitcoin-core-review-club.github.io/issues/14 13:40 < digi_james> I believe its all in rpcTable (all rpc methods) 13:41 < jnewbery> digi_james: is this in the context of process separation? 13:41 < digi_james> but wallet rpc methods sneak into this global via RpcHandlerImpl 13:41 < digi_james> jnewbery: exactly ... 13:41 < digi_james> I presume this PR is part of that effort? 13:41 < behradkhodayar> jnewbery: Sure, Thanks. 13:41 < achow101> digi_james: it isn't really part of that effort 13:42 < digi_james> achow101: got it. 13:42 < achow101> it was part of #15450 which someone asked to be split out into a separate PR. this is part of the multiwallet project 13:42 < jnewbery> digi_james: not really. It's to make sure that https://github.com/bitcoin/bitcoin/pull/15450 doesn't introduce a lot of code duplication 13:42 < jnewbery> it's really one of the final pieces of multiwallet: https://github.com/bitcoin/bitcoin/projects/2 13:43 < digi_james> Ah ok, thx 13:43 < achow101> this is part of one of my long term projects of getting rid of the default wallet 13:44 < jnewbery> \o/ 13:44 < hugohn> achow101: what is wrong with the default wallet? 13:45 < jnewbery> digi_james: multiprocess bitcoin is here: https://github.com/bitcoin/bitcoin/pull/10102 . Probably worth chatting to Russ if you have questions about how the wallet RPC would be implemented 13:45 < achow101> hugohn: it's kind of unintuitive and uses a bunch of default settings which waste space and time. e.g. you can't make a born-encrypted default wallet, so if you want to encrypt your wallet, you first generate 2000 keys that will never be used, encrypt it, and generate 2000 more keys 13:46 < hugohn> achow101: gotcha, thanks! is moving to descriptor-based wallet part of this effort? 13:46 < achow101> or perhaps you don't want to use keys at all and just want a watch only wallet, well too bad, your gonna have to first have the default wallet and if you don't set your config files correctly, it will be loaded which can result in funds being sent there instead of the intented recipient of your watcho nly 13:46 < digi_james> jnewbery: Cheers 13:47 < jnewbery> hugohn: I think they're independent 13:47 < achow101> native descriptor wallets are a separate project and isn't really related to the default wallet. that's more for hardware wallets 13:48 < jnewbery> but obviously there are interactions between the two 13:48 < hugohn> achow101 jnewbery cool, thanks guys 13:48 < jnewbery> there was discussion in IRC some months ago about why removing default wallets is good 13:48 < elichai2> achow101: adding some sort of "setup" to the QT would be awesome :) 13:48 < jnewbery> Probably 3-6 months ago 13:49 < jnewbery> ok, about ten minutes left. Has anyone been holding back? 13:49 < achow101> probably coincided with https://github.com/bitcoin/bitcoin/pull/15454 being opened 13:50 < jnewbery> last chance! 13:51 < pinheadmz> thanks jnewbery ! 13:51 < jnewbery> ok, before we go, I have something new for you all. 13:51 < jnewbery> The point of review club is to _review_, not just to talk about reviewing! 13:52 < jnewbery> so as homework, I encourage you all to go to https://github.com/bitcoin/bitcoin/pull/16244 and review 13:53 < jnewbery> if you don't have any comments on the code, you can still test and leave an ACK saying what you tested 13:53 < behradkhodayar> jnewbery: haha! Sure, Thanks 13:53 < jnewbery> there were at least 8 "hi"s at the start, so we should be able to have quite an impact if we all test and review the PR 13:54 < jnewbery> that's all! 13:54 < fjahr> jnewbery: Thanks! 13:55 < jnewbery> Next week, we'll cover #15169 Parallelize CheckInputs() in AcceptToMemoryPool(). digi_james is going to host (thanks digi_james!) 13:55 < digi_james> Thanks jnewbery achow101 and everybody else! 13:55 < digi_james> jnewbery: looking forward 13:55 < jnewbery> if anyone else wants to host in future, DM me and we can set it up. 13:56 < jnewbery> goodbye all! 13:56 < lightlike> thanks jnewbery achow101 13:56 < hugohn> thanks jnewbery achow101 & everyone! 13:57 < behradkhodayar> Thanks everyone! 13:57 < jonas_> Merged in real time! 13:58 < MarcoFalke> Ah, missed that this was in this meeting 13:58 < lightlike> heh, so much for the reviewing recommendation :-) 13:58 < achow101> post merge reviews are good too 13:58 < MarcoFalke> Yeah, and addressing the feedback 13:58 < MarcoFalke> https://github.com/bitcoin/bitcoin/pull/16244#issuecomment-509651561 13:58 < jnewbery> haha. review club so good that PRs just merged 13:59 < jnewbery> Yes, addressing that feedback could be a fun task for anyone here