wallet/refactor: refer to CWallet immutably when possible #18241

kallewoof · 2020-03-02T10:43:48Z

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

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

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

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

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.

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

This method does a simple check and no modifications.

* 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.

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

* 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

promag · 2020-03-02T11:01:09Z

Concept ACK.

DrahtBot · 2020-03-02T11:04:04Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

maflcko · 2020-03-02T14:44:18Z

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 · 2020-03-02T15:09:55Z

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

kallewoof · 2020-03-02T15:11:19Z

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 · 2020-03-02T15:13:49Z

Unless some script-fu master sorts some scripted diff.

practicalswift · 2020-03-02T20:56:37Z

Concept ACK

const CWallet& would be even better :)

kallewoof · 2020-03-03T03:49:02Z

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: kallewoof/bitcoin@2002-const-fixes...kallewoof:2002-const-fixes-depointered (diff vs this PR)

Empact · 2020-03-03T13:21:51Z

ACK 79facb1

Could squash into one or a few commits. I also like moving to references but I don't mind them arriving over time.

kallewoof · 2020-03-03T14:17:44Z

@Empact I tried to justify/analyze each change in each commit, so would prefer not to squash but if people find it hard to review, I can do so.

promag · 2020-03-03T14:24:43Z

d9b0ebc has a typo in commit message.

kallewoof · 2020-03-04T01:39:02Z

@promag What typo? "ivar" is a common way to refer to instance variables where I'm from.

promag · 2020-03-04T07:47:30Z

@kallewoof TIL!

ACK 79facb1.

fjahr · 2020-03-04T22:23:10Z

ACK 79facb1

laanwj · 2020-03-06T22:39:41Z

ACK 79facb1

… possible 79facb1 wallet: use constant CWallets in rpcwallet.cpp (Karl-Johan Alm) d9b0ebc wallet: make ReserveDestination pwallet ivar const (Karl-Johan Alm) 57c569e wallet: make BackupWallet() const (Karl-Johan Alm) df3a818 wallet: make getters const (Karl-Johan Alm) 227b9dd wallet/spkm: make GetOldestKeyPoolTime() const (Karl-Johan Alm) 22d329a wallet: use constant CWallets in rpcdump.cpp (Karl-Johan Alm) 7b3587b wallet/db: make IsDummy() const (Karl-Johan Alm) d366795 wallet/db: make Backup() const (Karl-Johan Alm) 8cd0b86 wallet: make CanGetAddresses() const (Karl-Johan Alm) 037fa77 wallet: make KeypoolCountExternalKeys() const (Karl-Johan Alm) ddc9355 wallet: make CanGenerateKeys() const (Karl-Johan Alm) dc2d065 make BlockUntilSyncedToCurrentChain() const (Karl-Johan Alm) Pull request description: A lot of places refer to `CWallet*`'s as `CWallet * const`, which translates to *"an immutable pointer to a mutable `CWallet` instance"*; this is 1. often not what the author meant, especially as a lot of these places do not at all modify the wallet object, and 2. 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 ACKs for top commit: laanwj: ACK 79facb1 Empact: ACK bitcoin@79facb1 promag: ACK 79facb1. fjahr: ACK 79facb1 Tree-SHA512: 80a80c1a52f0f788d0ccb268b53bc0f46c796643a3c5a22b55bbbde4ffa6c7e347784e5e53b1e488a3b4e14399e31d5be9417ad5b6319c74a462609e9b1a98e8

Summary: * make BlockUntilSyncedToCurrentChain() const The method checks the chain tip for the best block, and calls SyncWithValidationInterfaceQueue() (a standalone function) if necessary. * wallet: make CanGenerateKeys() const This method simply checks if HD is or can be enabled and does not require mutability. * wallet: make KeypoolCountExternalKeys() const This method returns the sum of the key pool sizes. It does no modification. * 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. * wallet/db: make IsDummy() const This method does a simple check and no modifications. * wallet/db: make Backup() const This method is the to-disk equivalent of serialize methods which are also const. * 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. * wallet/spkm: make GetOldestKeyPoolTime() const The method checks the oldest key time for key pools and returns the oldest. It does no modifications. * wallet: make getters const * wallet: make BackupWallet() const * wallet: make ReserveDestination pwallet ivar const * 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 This is a backport of Core [[bitcoin/bitcoin#18241 | PR18241]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, PiRK Reviewed By: PiRK Differential Revision: https://reviews.bitcoinabc.org/D8115

… possible 79facb1 wallet: use constant CWallets in rpcwallet.cpp (Karl-Johan Alm) d9b0ebc wallet: make ReserveDestination pwallet ivar const (Karl-Johan Alm) 57c569e wallet: make BackupWallet() const (Karl-Johan Alm) df3a818 wallet: make getters const (Karl-Johan Alm) 227b9dd wallet/spkm: make GetOldestKeyPoolTime() const (Karl-Johan Alm) 22d329a wallet: use constant CWallets in rpcdump.cpp (Karl-Johan Alm) 7b3587b wallet/db: make IsDummy() const (Karl-Johan Alm) d366795 wallet/db: make Backup() const (Karl-Johan Alm) 8cd0b86 wallet: make CanGetAddresses() const (Karl-Johan Alm) 037fa77 wallet: make KeypoolCountExternalKeys() const (Karl-Johan Alm) ddc9355 wallet: make CanGenerateKeys() const (Karl-Johan Alm) dc2d065 make BlockUntilSyncedToCurrentChain() const (Karl-Johan Alm) Pull request description: A lot of places refer to `CWallet*`'s as `CWallet * const`, which translates to *"an immutable pointer to a mutable `CWallet` instance"*; this is 1. often not what the author meant, especially as a lot of these places do not at all modify the wallet object, and 2. 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 ACKs for top commit: laanwj: ACK 79facb1 Empact: ACK bitcoin@79facb1 promag: ACK 79facb1. fjahr: ACK 79facb1 Tree-SHA512: 80a80c1a52f0f788d0ccb268b53bc0f46c796643a3c5a22b55bbbde4ffa6c7e347784e5e53b1e488a3b4e14399e31d5be9417ad5b6319c74a462609e9b1a98e8

kallewoof added 12 commits March 2, 2020 17:26

make BlockUntilSyncedToCurrentChain() const

dc2d065

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

wallet: make CanGenerateKeys() const

ddc9355

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

wallet: make KeypoolCountExternalKeys() const

037fa77

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

wallet/db: make Backup() const

d366795

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

wallet/db: make IsDummy() const

7b3587b

This method does a simple check and no modifications.

wallet/spkm: make GetOldestKeyPoolTime() const

227b9dd

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

wallet: make getters const

df3a818

wallet: make BackupWallet() const

57c569e

wallet: make ReserveDestination pwallet ivar const

d9b0ebc

fanquake added the Wallet label Mar 2, 2020

kallewoof force-pushed the 2002-const-fixes branch from f4b60d9 to 79facb1 Compare March 3, 2020 03:45

DrahtBot mentioned this pull request Mar 6, 2020

interfaces: Describe and follow some code conventions #18278

Merged

fanquake merged commit 4d80274 into bitcoin:master Mar 6, 2020

kallewoof deleted the 2002-const-fixes branch March 7, 2020 02:09

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet/refactor: refer to CWallet immutably when possible #18241

wallet/refactor: refer to CWallet immutably when possible #18241

Uh oh!

kallewoof commented Mar 2, 2020

Uh oh!

promag commented Mar 2, 2020

Uh oh!

DrahtBot commented Mar 2, 2020 •

edited

Loading

Uh oh!

maflcko commented Mar 2, 2020

Uh oh!

promag commented Mar 2, 2020

Uh oh!

kallewoof commented Mar 2, 2020

Uh oh!

promag commented Mar 2, 2020

Uh oh!

practicalswift commented Mar 2, 2020

Uh oh!

kallewoof commented Mar 3, 2020

Uh oh!

Empact commented Mar 3, 2020

Uh oh!

kallewoof commented Mar 3, 2020

Uh oh!

promag commented Mar 3, 2020

Uh oh!

kallewoof commented Mar 4, 2020

Uh oh!

promag commented Mar 4, 2020

Uh oh!

fjahr commented Mar 4, 2020

Uh oh!

laanwj commented Mar 6, 2020

Uh oh!

Uh oh!

wallet/refactor: refer to CWallet immutably when possible #18241

wallet/refactor: refer to CWallet immutably when possible #18241

Uh oh!

Conversation

kallewoof commented Mar 2, 2020

Uh oh!

promag commented Mar 2, 2020

Uh oh!

DrahtBot commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Mar 2, 2020

Uh oh!

promag commented Mar 2, 2020

Uh oh!

kallewoof commented Mar 2, 2020

Uh oh!

promag commented Mar 2, 2020

Uh oh!

practicalswift commented Mar 2, 2020

Uh oh!

kallewoof commented Mar 3, 2020

Uh oh!

Empact commented Mar 3, 2020

Uh oh!

kallewoof commented Mar 3, 2020

Uh oh!

promag commented Mar 3, 2020

Uh oh!

kallewoof commented Mar 4, 2020

Uh oh!

promag commented Mar 4, 2020

Uh oh!

fjahr commented Mar 4, 2020

Uh oh!

laanwj commented Mar 6, 2020

Uh oh!

Uh oh!

DrahtBot commented Mar 2, 2020 •

edited

Loading