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

[fanquake]

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 (comment)

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

[maflcko]

(edited title)

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

[jnewbery]

utACK 199c81f

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

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

[jnewbery]

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]

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.

[practicalswift]

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 …

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

… is preferable over …

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

Live demo:

$ cat nullptr-deref.cpp
#include <iostream>

struct T {
    void foo() { std::cout << this << "->foo()\n"; }
};

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

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

int main(void)
{
    T t;
    f_1(t);
    f_2(&t);

    // f_1(nullptr); // compile time error: no known conversion from nullptr_t to T&
    f_2(nullptr);
}
$ clang++ -o nullptr-deref nullptr-deref.cpp && ./nullptr-deref
0x7fff9e191234->foo()
0x7fff9e191234->foo()
0->foo()
$ g++ -o nullptr-deref nullptr-deref.cpp && ./nullptr-deref
0x7fff9e191234->foo()
0x7fff9e191234->foo()
0->foo()
$ clang++ -fsanitize=undefined -o nullptr-deref nullptr-deref.cpp && ./nullptr-deref
0x7fff9e191234->foo()
0x7fff9e191234->foo()
nullptr-deref.cpp:9:21: runtime error: member call on null pointer of type 'T'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior nullptr-deref.cpp:9:21 in
nullptr-deref.cpp:4:10: runtime error: member call on null pointer of type 'T *'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior nullptr-deref.cpp:4:10 in
0->foo()

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

[fanquake]

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

This is now split into two commits.

[ryanofsky]

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

[maflcko]

ACK 7c90c67 🐧

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 7c90c67b7e6f598f9ffdc136ded2b533b78ed044 🐧
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhiuAwApvxFWa2GM0vLZKgdZ8tCocQ52qIb/VphCjBZlzqC9iDCdexw+68eQAgN
0Q/EbzNXvZURPeOBOdoxH8tR25R83uvyLaCz/LAtDV65Md6a2aKdfohI5VmFUbbi
iOloTCEMLJL9GShzZwV8jf6ggcp/I8VDcCx6bwchJNf1q07V+ykiBnxeYFQlPM2Y
fcDOc2G7jbhLuSH/TBI9tdoEzzYG6tqbOuaTJ/gwzz7W0MlN2OuqfWtdkLvq4HXG
AOu4e2pBzL9QkUbi/rDkRRQCRIfZOM5g7ggWY7gSiRXv9QWFYhNqGYkqCGWkZRMH
uZnk0yRLKXOEyfGBZlAScugPzTtLaI9Yf4cCbFigZE4xl7gyKRHgdNzx/Uldzh4F
TjAJOwZBrLQepDyg0EqIAazdo7ELrdq/yXFirhimLZpnJxLwz8RNrxlFY3D0MFOF
YHOR8rkn5eLI3Z79YFxxSq92S5utVpjhga7/C1e9wkVuiPzw4mfS5pUnzukRSY1j
hla9xmWx
=grAy
-----END PGP SIGNATURE-----

Timestamp of file with hash a4b1e57819992cf30341a0c036b138d89f4d20fc296eae408b441244f39f67f2 -