wallet/refactor: refer to CWallet immutably when possible

kallewoof commented at 10:43 am on March 2, 2020: member

A lot of places refer to CWallet*’s as CWallet * const, which translates to “an immutable pointer to a mutable CWallet instance”; this is

often not what the author meant, especially as a lot of these places do not at all modify the wallet object, and
confusing, as it tends to suggest that this is a proper way to refer to a constant CWallet instance.

This PR changes references to wallets to const CWallet* const whenever immutability is expected. This should result in no behavioral changes at all, and improved compile-time error checking.

Note from irc:

<sipa> sounds good to me; this is the sort of change that as long as it compiles, the behavior shouldn’t change <sipa> though in general it may lead to introducing automatic copying of objects sometimes (e.g. trying to std::move a const object will work, but generally result in a copy rather than an efficient move) <sipa> CWallet objects aren’t copied or moved though

make BlockUntilSyncedToCurrentChain() const

The method checks the chain tip for the best block, and calls SyncWithValidationInterfaceQueue() (a standalone function) if necessary.

dc2d0650fd

wallet: make CanGenerateKeys() const

This method simply checks if HD is or can be enabled and does not require mutability.

ddc93557ad

wallet: make KeypoolCountExternalKeys() const

This method returns the sum of the key pool sizes. It does no modification.

037fa770eb

wallet: make CanGetAddresses() const

CWallet::CanGetAddresses() is used to check whether the wallet has available or is able to produce keys for addresses. It uses the ScriptPubKeyMan::CanGetAddresses(), which in turn uses the const KeypoolCountExternalKeys() method, all which do counting and no modifications.

8cd0b86340

wallet/db: make Backup() const

This method is the to-disk equivalent of serialize methods which are also const.

d366795d18

wallet/db: make IsDummy() const

This method does a simple check and no modifications.

7b3587b29d

wallet: use constant CWallets in rpcdump.cpp

* GetWalletAddressesForKey is, as the name implies, immutable; the one change besides the parameter constness is a [] -> .at() change, to a verified-existing key.
* dumpprivkey and dumpwallet are both similarly immutable, for obvious reasons.

22d329ad0e

wallet/spkm: make GetOldestKeyPoolTime() const

The method checks the oldest key time for key pools and returns the oldest. It does no modifications.

227b9dd2d6

wallet: make getters const df3a818d2a

wallet: make BackupWallet() const 57c569e4d9

wallet: make ReserveDestination pwallet ivar const d9b0ebc1da

wallet: use constant CWallets in rpcwallet.cpp

* GetAvoidReuseFlag: simply gets the flag, without modifying the wallet
* ListReceived: helper function to produce lists
* ListTransactions: produces a list of transactions, without modifications; two cases of map [] -> .at() for verified-existing keys
* DescribeWalletAddress: generates a description of a given wallet address without changing the wallet
* The following functions produce a list without making any modifications to the wallet:
  * listaddressgroupings
  * listreceivedbyaddress
  * listreceivedbylabel
  * listtransactions
  * listsinceblock
  * listlockunspent
  * listunspent
  * listlabels
  * getreceivedbyaddress
  * getreceivedbylabel
  * getaddressesbylabel
* signmessage: uses the wallet to procure a private key for signing, but does no modifications
* getbalance, getunconfirmedbalance: calculates the wallet balance, without any modifications
* gettransaction: procures transaction without any modifications
* backupwallet: makes a backup of the wallet to disk, without changing said wallet
* getwalletinfo: produces info about wallet without any modifications
* signrawtransactionwithwallet: modifies incoming transaction on the fly by signing with private key procured from within wallet; no modifications to wallet
* getaddressinfo: gets information about the given address, with no modifications done to the wallet; one case of [] -> .at() and one ::iterator -> ::const_iterator
* walletprocesspsbt: processes the given PSBT on the fly, without modifying the wallet

79facb11e9

fanquake added the label Wallet on Mar 2, 2020

promag commented at 11:01 am on March 2, 2020: member

Concept ACK.

DrahtBot commented at 11:04 am on March 2, 2020: 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:

#18278 (interfaces: Describe and follow some code conventions by ryanofsky)
#18192 (Bugfix: Wallet: Safely deal with change in the address book by luke-jr)
#16378 (The ultimate send RPC by Sjors)
#14942 (wallet: Make scan / abort status private to CWallet by Empact)
#11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

MarcoFalke commented at 2:44 pm on March 2, 2020: member

As you are changing this, my preference would be to just use const CWallet& wallet, which has all the same benefits as const CWallet* const, but also can’t be nullptr.

promag commented at 3:09 pm on March 2, 2020: member

@MarcoFalke that would be a larger change. I don’t mind this PR as it is and do that in other PR.

kallewoof commented at 3:11 pm on March 2, 2020: member

I love the idea, and started working on it, but as @promag points out, the diff becomes significantly larger. I can make a competing PR if people would like to compare the two, otherwise I think a follow-up PR to go from const CWallet* to const CWallet& might be worthwhile.

promag commented at 3:13 pm on March 2, 2020: member

Unless some script-fu master sorts some scripted diff.

practicalswift commented at 8:56 pm on March 2, 2020: contributor

Concept ACK

const CWallet& would be even better :)

kallewoof force-pushed on Mar 3, 2020

kallewoof commented at 3:49 am on March 3, 2020: member

I made a new branch and switched to const CWallet&. It builds on top of this one. I can switch to it if people find it worthwhile, but it pushes the diff from 68 to ~330 lines: https://github.com/kallewoof/bitcoin/compare/2002-const-fixes...kallewoof:2002-const-fixes-depointered (diff vs this PR)