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

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

Host: jnewbery  -  PR author: achow101

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

  113:00 <jnewbery> hi
  213:00 <fjahr> hi
  313:00 <behradkhodayar> Hi
  413:00 <digi_james> hello
  513:00 <elichai2> Hi :)
  613:00 <pinheadmz> oy!
  713:00 <hugohn> aloha
  813:00 <nijak> hello hello
  913:00 <lightlike> hi
 1013:01 <jnewbery> Notes and questionds here: https://bitcoin-core-review-club.github.io/16244.html
 1113:02 <jnewbery> My internet connection is a bit flakey today, so apologies if I drop. You'll just have to continue without me!
 1213:02 <jnewbery> who had a chance to review the PR and notes?
 1313:02 <digi_james> Yup
 1413:02 <lightlike> me too
 1513:02 <fjahr> 🙋‍♂️
 1613:02 <pinheadmz> read through the code
 1713:03 <elichai2> yea
 1813:03 <jnewbery> Great. First question should be pretty simple. Where is this code tested?
 1913:03 <fjahr> functional/rpc_createwallet
 2013:04 <pinheadmz> well... funcitonal/wallet_createwallet.py ?
 2113:04 <jnewbery> Good guess, but it's not quite right.
 2213:04 <lightlike> functional/wallet_multiwallet.py
 2313:04 <jnewbery> All the wallet functional tests are prefixed with wallet_
 2413:04 <jnewbery> Yeah, wallet_createwallet.py
 2513:05 <jonas_> çççç
 2613:05 <jonas_> typo!
 2713:06 <jnewbery> So when reviewing this PR, I think it's worth reading through that test to satisfy yourselves that the functinoality is tested
 2813:07 <elichai2> Is there a "integration test" that verifies that the wallet file was created correctly?
 2913: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?
 3013:07 <elichai2> (in case of modifications like this to the `CreateWallet` function)
 3113:08 <jnewbery> Next up: did people check taht the different failure modes are tested?
 3213:08 <digi_james> elichai2: In regards to the wallet files, I suspect in the db tests ....
 3313:08 <jnewbery> elichai: I'd call wallet_createwallet.py an integration test. It's creating the wallet and then running RPCs like `getwalletinfo`
 3413:09 <jnewbery> but there's no test on the actual wallet file
 3513:09 <fjahr> I did not get a complete overview but I think I did not see the 'wallet already exists' case being tested
 3613:09 <hugohn> *declaration
 3713:09 <jnewbery> I'd expect that to be covered by the wallet_multiwallet.py test, which tests wallets being unloaded and reloaded
 3813:09 <elichai2> but the fact that `getwalletinfo` workd correctly on `CreateWallet` doesn't mean the wallet format didn't change, right?
 3913: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
 4013:10 <lightlike> fjahr: that one is tested in wallet_multiwallet.py
 4113:11 <fjahr> jnewbery: true, it's there
 4213:11 <jnewbery> (and obviously don't open PRs to 'fix' code to use that convention)
 4313: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
 4413:12 <digi_james> I think thats what the warning string is for
 4513:12 <digi_james> (file system error)
 4613:12 <jnewbery> Right, I was about to ask. Where is the warning string used?
 4713:13 <digi_james> wait no, warning if it successfully recovers corrupt file
 4813:13 <digi_james> the berkeley db wrapper I believe
 4913: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
 5013:14 <fjahr> in VerifyWallets?
 5113:14 <digi_james> :)
 5213:15 <pinheadmz> Can I ask how the tests are calling the right function?
 5313:15 <jnewbery> here: https://github.com/bitcoin/bitcoin/blob/6c1e45c4c41676f80ac6fb8d48cfbcf839593f19/src/wallet/db.cpp#L426
 5413:15 <pinheadmz> the tests call createwallet()
 5513:15 <pinheadmz> and I see CreateWallet in wallettool.cpp
 5613:16 <pinheadmz> this PR adds CreateWallet to wallet.cpp
 5713:16 <pinheadmz> but I dont undertand which function the tests used to call - and why they call the new one now
 5813:16 <jnewbery> pinheadmz: yes, the createwallet call in the functional test is to the RPC method
 5913:16 <pinheadmz> oh ok its just to the RPC
 6013:16 <pinheadmz> and the PR redirects the actual wallet creation to wallet.cpp
 6113:16 <pinheadmz> so whats the method in wallettool ?
 6213:17 <jnewbery> right, in a call like self.nodes[0].createwallet(), that createwallet() gets converted into an RPC call
 6313:17 <jnewbery> take a look at the TestNode() class in the test framework
 6413:18 <jnewbery> particularly the __getattr__() method
 6513:18 <pinheadmz> "Dispatches any unrecognised messages to the RPC connection or a CLI instance" -- clever
 6613:19 <jnewbery> pinheadmz: yes the PR creates a new CreateWallet() function in libbitcoin_wallet
 6713:19 <jnewbery> ok, last question from me. There are several suggestions for changing the function from ryanofsky, promag and empact. Are those good suggestions?
 6813:20 <behradkhodayar> So what about covering "Are all the failure modes tested? If not, why not?"
 6913:21 <jnewbery> behradkhodayar: did you have anything to add on that? Any failure modes you saw that aren't covered?
 7013:22 <behradkhodayar> jnewbery: Sorry, Just missed it. TBT, I was expecting a talk about candidate occasions may makes a kind of failure.
 7113:22 <behradkhodayar> Not yet!
 7213: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
 7313:23 <jnewbery> ok, did anyone have any other questions about the PR?
 7413:23 <jnewbery> hugohn: all RPC methods are lowercasewithoutspaces . Most functions in the codebase are CamelCase
 7513:24 <hugohn> ic
 7613:24 <jnewbery> once you know that convention, it's not confusing
 7713: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.
 7813:25 <jnewbery> digi_james: which comment?
 7913:26 <digi_james> "new create wallet function [could take] separate option arguments instead of wallet flags
 8013: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?
 8113: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
 8213:29 <digi_james> jnewbery: I see ...
 8313: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
 8413: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
 8513:29 <achow101> lightlike: the github-merge script used to merge PRs will not detect ACKs on hold commit hashes
 8613:29 <achow101> s/hold/old
 8713:30 <jnewbery> I thought this might be interesting for people here: https://github.com/bitcoin/bitcoin/pull/16244#issuecomment-509974339
 8813: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
 8913:31 <jnewbery> whenever a PR needs rebase, then your ACKs will be invalidated and you'll need to rereview
 9013:31 <pinheadmz> are ACK, utACK messages and their commit IDs actually parsed by anything besides... people? :-)
 9113:32 <achow101> pinheadmz: github-merge.py script used for merging
 9213:32 <pinheadmz> oh interesting, so they really are invalidated by new commits
 9313:32 <achow101> take a look at recent PR merges, they all contain the ACKs of the PR for the commit that was merged
 9413: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.
 9513: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
 9613:32 <jnewbery> if so, I'm satisfied that I can ACK the new commit hash
 9713: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
 9813:34 <pinheadmz> very cool
 9913:35 <jnewbery> hugohn: yeah, I tend to agree, but also agree that it can be done as a follow-up PR
10013:35 <jnewbery> pinheadmz: if we were super paranoid we could sign our ACKs
10113:36 <pinheadmz> ive seen that before in bitcoin core reviews
10213:36 <jnewbery> I think Marco is the only one who signs his github review comments
10313:36 <pinheadmz> haha
10413:36 <pinheadmz> and even with the merge script incorporating ACKs, etc - the commit is still signed by the maintainer's signign key
10513:37 <pinheadmz> (thought I saw a luke-jr signed ACK before too :-) )
10613:38 <jnewbery> any other questions on the code changes?
10713:39 <behradkhodayar> This was my first session here & I found it very useful. Thank you everyone
10813:39 <behradkhodayar> Just one general question: Are we going to cover ALL upcomming PRs here in this series of meetings?
10913:39 <digi_james> I am unsure of how the rpc methods of the wallet can be easily decoupled compared to node rpc methods.
11013:40 <digi_james> I've been digging around and noticed the wallet rpc methods are registered in the RpcHandlerImpl constructor, the global rpcTable
11113:40 <achow101> digi_james: decoupled in what way?
11213:40 <digi_james> For example, if the wallet were to directly receive rpc calls
11313: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
11413:40 <digi_james> I believe its all in rpcTable (all rpc methods)
11513:41 <jnewbery> digi_james: is this in the context of process separation?
11613:41 <digi_james> but wallet rpc methods sneak into this global via RpcHandlerImpl
11713:41 <digi_james> jnewbery: exactly ...
11813:41 <digi_james> I presume this PR is part of that effort?
11913:41 <behradkhodayar> jnewbery: Sure, Thanks.
12013:41 <achow101> digi_james: it isn't really part of that effort
12113:42 <digi_james> achow101: got it.
12213: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
12313: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
12413:42 <jnewbery> it's really one of the final pieces of multiwallet: https://github.com/bitcoin/bitcoin/projects/2
12513:43 <digi_james> Ah ok, thx
12613:43 <achow101> this is part of one of my long term projects of getting rid of the default wallet
12713:44 <jnewbery> \o/
12813:44 <hugohn> achow101: what is wrong with the default wallet?
12913: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
13013: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
13113:46 <hugohn> achow101: gotcha, thanks! is moving to descriptor-based wallet part of this effort?
13213: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
13313:46 <digi_james> jnewbery: Cheers
13413:47 <jnewbery> hugohn: I think they're independent
13513:47 <achow101> native descriptor wallets are a separate project and isn't really related to the default wallet. that's more for hardware wallets
13613:48 <jnewbery> but obviously there are interactions between the two
13713:48 <hugohn> achow101 jnewbery cool, thanks guys
13813:48 <jnewbery> there was discussion in IRC some months ago about why removing default wallets is good
13913:48 <elichai2> achow101: adding some sort of "setup" to the QT would be awesome :)
14013:48 <jnewbery> Probably 3-6 months ago
14113:49 <jnewbery> ok, about ten minutes left. Has anyone been holding back?
14213:49 <achow101> probably coincided with https://github.com/bitcoin/bitcoin/pull/15454 being opened
14313:50 <jnewbery> last chance!
14413:51 <pinheadmz> thanks jnewbery !
14513:51 <jnewbery> ok, before we go, I have something new for you all.
14613:51 <jnewbery> The point of review club is to _review_, not just to talk about reviewing!
14713:52 <jnewbery> so as homework, I encourage you all to go to https://github.com/bitcoin/bitcoin/pull/16244 and review
14813:53 <jnewbery> if you don't have any comments on the code, you can still test and leave an ACK saying what you tested
14913:53 <behradkhodayar> jnewbery: haha! Sure, Thanks
15013: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
15113:54 <jnewbery> that's all!
15213:54 <fjahr> jnewbery: Thanks!
15313:55 <jnewbery> Next week, we'll cover #15169 Parallelize CheckInputs() in AcceptToMemoryPool(). digi_james is going to host (thanks digi_james!)
15413:55 <digi_james> Thanks jnewbery achow101 and everybody else!
15513:55 <digi_james> jnewbery: looking forward
15613:55 <jnewbery> if anyone else wants to host in future, DM me and we can set it up.
15713:56 <jnewbery> goodbye all!
15813:56 <lightlike> thanks jnewbery achow101
15913:56 <hugohn> thanks jnewbery achow101 & everyone!
16013:57 <behradkhodayar> Thanks everyone!
16113:57 <jonas_> Merged in real time!
16213:58 <MarcoFalke> Ah, missed that this was in this meeting
16313:58 <lightlike> heh, so much for the reviewing recommendation :-)
16413:58 <achow101> post merge reviews are good too
16513:58 <MarcoFalke> Yeah, and addressing the feedback
16613:58 <MarcoFalke> https://github.com/bitcoin/bitcoin/pull/16244#issuecomment-509651561
16713:58 <jnewbery> haha. review club so good that PRs just merged
16813:59 <jnewbery> Yes, addressing that feedback could be a fun task for anyone here