#16244 Move wallet creation out of the createwallet rpc into its own function (wallet)

https://github.com/bitcoin/bitcoin/16244

Host: jnewbery

Notes

  • 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 createwallet RPC method.
  • There should be no behaviour change from this PR.

Questions

  • 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 warning string used?
  • There are several suggestions for changing the function from ryanofsky, promag and empact. Are those good suggestions?

Meeting Log

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[0].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