wallet: replace raw pointer with const reference in AddrToPubKey #17584

This PR replaces a redundant reference-to-pointer conversion in addmultisigaddress from wallet/rpcwallet.cpp. It also makes the API from rpc/util.h look more straightforward as AddrToPubKey now uses const references like other functions from there.

I am not sure why there is a ref-to-ptr conversion in addmultisignatures, so I can only speculate that this is because of "historical reasons".

The ref-to-ptr conversion happens here: https://github.com/bitcoin/bitcoin/blob/master/src/wallet/rpcwallet.cpp#L1001

There, the address of LegacyScriptPubKeyMan& is given to AddrToPubKey.

Later, in AddrToPubKey, it gets converted back to a reference, because GetKeyForDestination in rpc/util.cpp expects a const ref: https://github.com/bitcoin/bitcoin/blob/master/src/rpc/util.cpp#L140

Regards,

Concept ACK

Any idea why it was a pointer in the first place? Always happy to get rid of pointers.

concept ACK

The pointer argument was introduced in 1df206f854. Maybe @achow101 remembers?

At that time the keystore argument was used in two expressions inside AddrToPubKey(): GetKeyForDestination(*keystore, dest) and keystore->GetPubKey(). The first takes a const reference arg and in the second GetPubKey() method is marked as const (even back then). So it cannot have been that keystore was a pointer (to non-const!) only to be able to use it inside AddrToPubKey().

However at that time all callers of GetKeyForDestination() used a dereferenced pointer:

1df206f854:src/rpc/misc.cpp:229:            CKeyID key_id = GetKeyForDestination(*pwallet, dest);
1df206f854:src/rpc/util.cpp:33:    CKeyID key = GetKeyForDestination(*keystore, dest);
1df206f854:src/wallet/rpcdump.cpp:606:    auto keyid = GetKeyForDestination(*pwallet, dest);

So maybe it was due to some code mimicing?

Other callers of GetKeyForDestination() have switched to using a const reference, so this change should be welcome.

Concept ACK

So maybe it was due to some code mimicing?

More like code copying.

Concept ACK

Code review ACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a.

ACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged.

ACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a

utACK 1a3a256d5e0443d19757c1f1fceb9c9ede17758a