rpc: replace wallet raw pointers with references (#18592 rebased)

fanquake commented at 8:05 am on March 2, 2021: member

This is a rebased #18592.

This PR replaces raw pointers in rpcwallet.cpp and rpcdump.cpp with shared_ptr. The motivation for this PR is described here #18590

It seems that this PR is indirectly related to this issue: #13063 (review)

Notice: I have deliberately not changed the class WalletRescanReserver whose constructor expects a raw pointer, because it’s external and affects other areas, which I didn’t touch to avoid making this PR “viral”.

Fixes #18590

fanquake added the label Wallet on Mar 2, 2021

fanquake added the label RPC/REST/ZMQ on Mar 2, 2021

fanquake requested review from promag on Mar 2, 2021

fanquake requested review from MarcoFalke on Mar 2, 2021

fanquake requested review from ryanofsky on Mar 2, 2021

MarcoFalke renamed this:
~~rpc: replace raw pointers with shared_ptrs (#18592 rebased)~~
rpc: replace wallet raw pointers with references (#18592 rebased)
on Mar 2, 2021

MarcoFalke commented at 9:43 am on March 2, 2021: member

(edited title)

DrahtBot commented at 10:07 am on March 2, 2021: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21366 (refactor: replace util::Ref with std::any (C++17) by theStack)
#21359 (rpc: include_unsafe option for fundrawtransaction by t-bast)
#21329 (descriptor wallet: Cache last hardened xpub and use in normalized descriptors by achow101)
#21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)
#20640 (wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack)
#20583 (rpc: Add missing BlockUntilSyncedToCurrentChain to wallet RPCs by MarcoFalke)
#14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/wallet/rpcdump.cpp:1761 in 199c81f5fb outdated

1757@@ -1768,20 +1758,20 @@ RPCHelpMan listdescriptors()
1758         },
1759         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1760 {
1761-    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
1762-    if (!wallet) return NullUniValue;
1763+    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);

jnewbery commented at 10:19 am on March 2, 2021:

Our style is no longer to use pthing as the name for pointer variables, but I guess you did this to be consistent with the other functions in this file.

fanquake commented at 1:27 am on March 5, 2021:

Yea it’s done to keep the diff to a minimum, otherwise we’d also have to rename all the pwallet usage in each function.

jnewbery commented at 10:21 am on March 2, 2021: member

utACK 199c81f5fb54fa9d944968bf35c505e3941ddf67

I think this would be much easier to review if it was split into two commits:

Remove all the CWallet* const pwallet = wallet.get() lines and rename wallet to pwallet.
Change the function signatures to take references instead of pointers.

practicalswift commented at 11:58 am on March 2, 2021: contributor

Super Strong Concept ACK for safety reasons: pre-conditions should be stated explicitly and be enforced at compile-time where possible.

Thanks for tackling these!

I stumbled upon a similar case in the networking code the other day. More generally it seems like there are a few functions in our case base where we take T* instead of T& in places where we don’t safely handle the nullptr case. That’s quite confusing and potentially dangerous. I think it makes sense to convert these from T* to T& gradually over time. This PR is a good start :)

More specifically …

0void f_1(T& t) { t.foo(); }

… is preferable over …

0void f_2(T* t) { t->foo(); }

Live demo:

 0$ cat nullptr-deref.cpp
 1#include <iostream>
 2
 3struct T {
 4    void foo() { std::cout << this << "->foo()\n"; }
 5};
 6
 7void f_1(T& t) { t.foo(); }
 8
 9void f_2(T* t) { t->foo(); }
10
11int main(void)
12{
13    T t;
14    f_1(t);
15    f_2(&t);
16
17    // f_1(nullptr); // compile time error: no known conversion from nullptr_t to T&
18    f_2(nullptr);
19}
20$ clang++ -o nullptr-deref nullptr-deref.cpp && ./nullptr-deref
210x7fff9e191234->foo()
220x7fff9e191234->foo()
230->foo()
24$ g++ -o nullptr-deref nullptr-deref.cpp && ./nullptr-deref
250x7fff9e191234->foo()
260x7fff9e191234->foo()
270->foo()
28$ clang++ -fsanitize=undefined -o nullptr-deref nullptr-deref.cpp && ./nullptr-deref
290x7fff9e191234->foo()
300x7fff9e191234->foo()
31nullptr-deref.cpp:9:21: runtime error: member call on null pointer of type 'T'
32SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior nullptr-deref.cpp:9:21 in
33nullptr-deref.cpp:4:10: runtime error: member call on null pointer of type 'T *'
34SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior nullptr-deref.cpp:4:10 in
350->foo()

Note how f_1(nullptr) gives a nice compile-time error whereas f_2(nullptr) gives a run-time WTF (at best) :)

rpc: remove calls to CWallet.get()

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

4866934008

rpc: refactor rpc wallet functions to take references instead of pointers

Co-authored-by: MarcoFalke <falke.marco@gmail.com>
Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>

7c90c67b7e

fanquake force-pushed on Mar 5, 2021

fanquake commented at 1:28 am on March 5, 2021: member

I think this would be much easier to review if it was split into two commits:

This is now split into two commits.

ryanofsky approved

ryanofsky commented at 3:55 am on March 10, 2021: member

Code review ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044. Changes easy to review with --word-diff-regex=. -U0

MarcoFalke commented at 7:24 am on March 10, 2021: member

ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044 🐧

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044 🐧
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUhiuAwApvxFWa2GM0vLZKgdZ8tCocQ52qIb/VphCjBZlzqC9iDCdexw+68eQAgN
 80Q/EbzNXvZURPeOBOdoxH8tR25R83uvyLaCz/LAtDV65Md6a2aKdfohI5VmFUbbi
 9iOloTCEMLJL9GShzZwV8jf6ggcp/I8VDcCx6bwchJNf1q07V+ykiBnxeYFQlPM2Y
10fcDOc2G7jbhLuSH/TBI9tdoEzzYG6tqbOuaTJ/gwzz7W0MlN2OuqfWtdkLvq4HXG
11AOu4e2pBzL9QkUbi/rDkRRQCRIfZOM5g7ggWY7gSiRXv9QWFYhNqGYkqCGWkZRMH
12uZnk0yRLKXOEyfGBZlAScugPzTtLaI9Yf4cCbFigZE4xl7gyKRHgdNzx/Uldzh4F
13TjAJOwZBrLQepDyg0EqIAazdo7ELrdq/yXFirhimLZpnJxLwz8RNrxlFY3D0MFOF
14YHOR8rkn5eLI3Z79YFxxSq92S5utVpjhga7/C1e9wkVuiPzw4mfS5pUnzukRSY1j
15hla9xmWx
16=grAy
17-----END PGP SIGNATURE-----

Timestamp of file with hash a4b1e57819992cf30341a0c036b138d89f4d20fc296eae408b441244f39f67f2 -

MarcoFalke merged this on Mar 10, 2021

MarcoFalke closed this on Mar 10, 2021

fanquake deleted the branch on Mar 10, 2021

sidhujag referenced this in commit 5b248c46a6 on Mar 10, 2021

DrahtBot locked this on Aug 16, 2022

rpc: replace wallet raw pointers with references (#18592 rebased) #21331

Conflicts