Move wallet creation out of the createwallet rpc into its own function (
wallet) Jul 10, 2019
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.
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?
3 13:00 <behradkhodayar> Hi
4 13:00 <digi_james> hello
8 13:00 <nijak> hello hello
11 13:02 <jnewbery> My internet connection is a bit flakey today, so apologies if I drop. You'll just have to continue without me!
12 13:02 <jnewbery> who had a chance to review the PR and notes?
14 13:02 <lightlike> me too
16 13:02 <pinheadmz> read through the code
18 13:03 <jnewbery> Great. First question should be pretty simple. Where is this code tested?
19 13:03 <fjahr> functional/rpc_createwallet
20 13:04 <pinheadmz> well... funcitonal/wallet_createwallet.py ?
21 13:04 <jnewbery> Good guess, but it's not quite right.
22 13:04 <lightlike> functional/wallet_multiwallet.py
23 13:04 <jnewbery> All the wallet functional tests are prefixed with wallet_
24 13:04 <jnewbery> Yeah, wallet_createwallet.py
27 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
28 13:07 <elichai2> Is there a "integration test" that verifies that the wallet file was created correctly?
29 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?
30 13:07 <elichai2> (in case of modifications like this to the `CreateWallet` function)
31 13:08 <jnewbery> Next up: did people check taht the different failure modes are tested?
32 13:08 <digi_james> elichai2: In regards to the wallet files, I suspect in the db tests ....
33 13:08 <jnewbery> elichai: I'd call wallet_createwallet.py an integration test. It's creating the wallet and then running RPCs like `getwalletinfo`
34 13:09 <jnewbery> but there's no test on the actual wallet file
35 13:09 <fjahr> I did not get a complete overview but I think I did not see the 'wallet already exists' case being tested
36 13:09 <hugohn> *declaration
37 13:09 <jnewbery> I'd expect that to be covered by the wallet_multiwallet.py test, which tests wallets being unloaded and reloaded
38 13:09 <elichai2> but the fact that `getwalletinfo` workd correctly on `CreateWallet` doesn't mean the wallet format didn't change, right?
39 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
40 13:10 <lightlike> fjahr: that one is tested in wallet_multiwallet.py
41 13:11 <fjahr> jnewbery: true, it's there
42 13:11 <jnewbery> (and obviously don't open PRs to 'fix' code to use that convention)
43 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
44 13:12 <digi_james> I think thats what the warning string is for
45 13:12 <digi_james> (file system error)
46 13:12 <jnewbery> Right, I was about to ask. Where is the warning string used?
47 13:13 <digi_james> wait no, warning if it successfully recovers corrupt file
48 13:13 <digi_james> the berkeley db wrapper I believe
49 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
50 13:14 <fjahr> in VerifyWallets?
52 13:15 <pinheadmz> Can I ask how the tests are calling the right function?
54 13:15 <pinheadmz> the tests call createwallet()
55 13:15 <pinheadmz> and I see CreateWallet in wallettool.cpp
56 13:16 <pinheadmz> this PR adds CreateWallet to wallet.cpp
57 13:16 <pinheadmz> but I dont undertand which function the tests used to call - and why they call the new one now
58 13:16 <jnewbery> pinheadmz: yes, the createwallet call in the functional test is to the RPC method
59 13:16 <pinheadmz> oh ok its just to the RPC
60 13:16 <pinheadmz> and the PR redirects the actual wallet creation to wallet.cpp
61 13:16 <pinheadmz> so whats the method in wallettool ?
62 13:17 <jnewbery> right, in a call like self.nodes.createwallet(), that createwallet() gets converted into an RPC call
63 13:17 <jnewbery> take a look at the TestNode() class in the test framework
64 13:18 <jnewbery> particularly the __getattr__() method
65 13:18 <pinheadmz> "Dispatches any unrecognised messages to the RPC connection or a CLI instance" -- clever
66 13:19 <jnewbery> pinheadmz: yes the PR creates a new CreateWallet() function in libbitcoin_wallet
67 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?
68 13:20 <behradkhodayar> So what about covering "Are all the failure modes tested? If not, why not?"
69 13:21 <jnewbery> behradkhodayar: did you have anything to add on that? Any failure modes you saw that aren't covered?
70 13:22 <behradkhodayar> jnewbery: Sorry, Just missed it. TBT, I was expecting a talk about candidate occasions may makes a kind of failure.
71 13:22 <behradkhodayar> Not yet!
72 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
73 13:23 <jnewbery> ok, did anyone have any other questions about the PR?
74 13:23 <jnewbery> hugohn: all RPC methods are lowercasewithoutspaces . Most functions in the codebase are CamelCase
76 13:24 <jnewbery> once you know that convention, it's not confusing
77 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.
78 13:25 <jnewbery> digi_james: which comment?
79 13:26 <digi_james> "new create wallet function [could take] separate option arguments instead of wallet flags
80 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?
81 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
82 13:29 <digi_james> jnewbery: I see ...
83 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
84 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
85 13:29 <achow101> lightlike: the github-merge script used to merge PRs will not detect ACKs on hold commit hashes
86 13:29 <achow101> s/hold/old
88 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
89 13:31 <jnewbery> whenever a PR needs rebase, then your ACKs will be invalidated and you'll need to rereview
90 13:31 <pinheadmz> are ACK, utACK messages and their commit IDs actually parsed by anything besides... people? :-)
91 13:32 <achow101> pinheadmz: github-merge.py script used for merging
92 13:32 <pinheadmz> oh interesting, so they really are invalidated by new commits
93 13:32 <achow101> take a look at recent PR merges, they all contain the ACKs of the PR for the commit that was merged
94 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.
95 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
96 13:32 <jnewbery> if so, I'm satisfied that I can ACK the new commit hash
97 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
98 13:34 <pinheadmz> very cool
99 13:35 <jnewbery> hugohn: yeah, I tend to agree, but also agree that it can be done as a follow-up PR
100 13:35 <jnewbery> pinheadmz: if we were super paranoid we could sign our ACKs
101 13:36 <pinheadmz> ive seen that before in bitcoin core reviews
102 13:36 <jnewbery> I think Marco is the only one who signs his github review comments
103 13:36 <pinheadmz> haha
104 13:36 <pinheadmz> and even with the merge script incorporating ACKs, etc - the commit is still signed by the maintainer's signign key
105 13:37 <pinheadmz> (thought I saw a luke-jr signed ACK before too :-) )
106 13:38 <jnewbery> any other questions on the code changes?
107 13:39 <behradkhodayar> This was my first session here & I found it very useful. Thank you everyone
108 13:39 <behradkhodayar> Just one general question: Are we going to cover ALL upcomming PRs here in this series of meetings?
109 13:39 <digi_james> I am unsure of how the rpc methods of the wallet can be easily decoupled compared to node rpc methods.
110 13:40 <digi_james> I've been digging around and noticed the wallet rpc methods are registered in the RpcHandlerImpl constructor, the global rpcTable
111 13:40 <achow101> digi_james: decoupled in what way?
112 13:40 <digi_james> For example, if the wallet were to directly receive rpc calls
114 13:40 <digi_james> I believe its all in rpcTable (all rpc methods)
115 13:41 <jnewbery> digi_james: is this in the context of process separation?
116 13:41 <digi_james> but wallet rpc methods sneak into this global via RpcHandlerImpl
117 13:41 <digi_james> jnewbery: exactly ...
118 13:41 <digi_james> I presume this PR is part of that effort?
119 13:41 <behradkhodayar> jnewbery: Sure, Thanks.
120 13:41 <achow101> digi_james: it isn't really part of that effort
121 13:42 <digi_james> achow101: got it.
122 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
125 13:43 <digi_james> Ah ok, thx
126 13:43 <achow101> this is part of one of my long term projects of getting rid of the default wallet
128 13:44 <hugohn> achow101: what is wrong with the default wallet?
130 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
131 13:46 <hugohn> achow101: gotcha, thanks! is moving to descriptor-based wallet part of this effort?
132 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
133 13:46 <digi_james> jnewbery: Cheers
134 13:47 <jnewbery> hugohn: I think they're independent
135 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
136 13:48 <jnewbery> but obviously there are interactions between the two
137 13:48 <hugohn> achow101 jnewbery cool, thanks guys
138 13:48 <jnewbery> there was discussion in IRC some months ago about why removing default wallets is good
139 13:48 <elichai2> achow101: adding some sort of "setup" to the QT would be awesome :)
140 13:48 <jnewbery> Probably 3-6 months ago
141 13:49 <jnewbery> ok, about ten minutes left. Has anyone been holding back?
143 13:50 <jnewbery> last chance!
144 13:51 <pinheadmz> thanks jnewbery !
145 13:51 <jnewbery> ok, before we go, I have something new for you all.
146 13:51 <jnewbery> The point of review club is to _review_, not just to talk about reviewing!
148 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
149 13:53 <behradkhodayar> jnewbery: haha! Sure, Thanks
150 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
151 13:54 <jnewbery> that's all!
152 13:54 <fjahr> jnewbery: Thanks!
153 13:55 <jnewbery> Next week, we'll cover #15169 Parallelize CheckInputs() in AcceptToMemoryPool(). digi_james is going to host (thanks digi_james!)
154 13:55 <digi_james> Thanks jnewbery achow101 and everybody else!
155 13:55 <digi_james> jnewbery: looking forward
156 13:55 <jnewbery> if anyone else wants to host in future, DM me and we can set it up.
157 13:56 <jnewbery> goodbye all!
158 13:56 <lightlike> thanks jnewbery achow101
159 13:56 <hugohn> thanks jnewbery achow101 & everyone!
160 13:57 <behradkhodayar> Thanks everyone!
161 13:57 <jonas_> Merged in real time!
162 13:58 <MarcoFalke> Ah, missed that this was in this meeting
163 13:58 <lightlike> heh, so much for the reviewing recommendation :-)
164 13:58 <achow101> post merge reviews are good too
165 13:58 <MarcoFalke> Yeah, and addressing the feedback
167 13:58 <jnewbery> haha. review club so good that PRs just merged
168 13:59 <jnewbery> Yes, addressing that feedback could be a fun task for anyone here