refactor: actual immutable pointing #22787

kallewoof · 2021-08-24T04:24:55Z

const std::shared_ptr<CWallet> wallet = x;

means we can not do wallet = y, but we can totally do wallet->DestructiveOperation(), contrary to what that line looks like.

This PR

introduces a new convention: always use const shared pointers to CWallets (even when we mutate the pointed-to thing)
uses const shared_ptr<const CWallet> everywhere where wallets are not modified

In the future, this should preferably apply to all shared pointers, not limited to just CWallets.

Both of these serve the same purpose: to dispell the misconception that const shared_ptr<X> immutates X. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons.

DrahtBot · 2021-08-24T08:58:16Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22928 (refactor: Remove gArgs from wallet.h and wallet.cpp (2) by kiminuo)
#22260 (Make bech32m the default for RPC, opt-in for GUI by Sjors)

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 · 2021-08-24T11:05:53Z

~~I'm a NACK on this.~~ We use the (common) convention of putting const to the left of the thing that's const, except in the case of a const pointer to type (T* const) where the const needs to go to the right of the * to indicate that it's a const pointer rather than a pointer to const. Changing the style just in this case would be inconsistent and confusing.

Hopefully now that this has bitten you once, you'll not get tripped up by it again. FWIW, I think the notation for smart pointers:

const std::shared_ptr<T> "constant shared pointer to mutable T"
std::shared_ptr<const T> "mutable shared pointer to constant T"

is a bit easier to read than for raw pointers:

const T* "mutable pointer to constant T"
T* const "constant pointer to mutable T"

This is also similar for STL containers:

const std::vector<T> - constant vector of Ts
std::vector<const T> - mutable vector of constant Ts

kallewoof · 2021-08-24T11:11:56Z

I'm a NACK on this. We use the (common) convention of putting const to the left of the thing that's const, except in the case of a const pointer to type (T* const) where the const needs to go to the right of the * to indicate that it's a const pointer rather than a pointer to const. Changing the style just in this case would be inconsistent and confusing.

I can easily move the const to the left-hand side. We are definitely not consistent with putting it on the left, as you can see all the cases of wallet const shared pointers right now (search for std::shared_ptr<CWallet> const in the code base for an example).

That's not what my PR is about though: it's about putting const inside the shared pointer, which is a feature that is supported, and that makes the thing inside it immutable, which is what you expect when you see the const sign. You do not expect to be able to call destructive methods or delete things inside a wallet that is marked const, which you can do with the above shared pointer.

Hopefully now that this has bitten you once, you'll not get tripped up by it again.

It is definitely not the first time it's bitten me, personally, but maybe I'm just slow. :)

Edit: it bites me often because I keep thinking of a const std::shared_ptr<X> as having a const X inside of it, due to it being const, but in reality it's simply a shared pointer that cannot be e.g. .reset().

jnewbery

That's not what my PR is about though: it's about putting const inside the shared pointer, which is a feature that is supported, and that makes the thing inside it immutable, which is what you expect when you see the const sign.

Ah, sorry - I misunderstood. I have no objection to making const things const, although I have a strong preference for following the convention of placing const on the left side of the type.

jnewbery · 2021-08-24T12:22:49Z

src/wallet/interfaces.cpp

    std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) override
    {
-        std::shared_ptr<CWallet> wallet;
+        std::shared_ptr<CWallet> const wallet;


Declaring a local const std::shared_ptr<T> const_ptr; is pointless. It can't be set to anything after this declaration since it's const.

In fact, this wallet variable isn't used, so I think it can just be removed.

Yes, this one just sorta tagged along. I have no idea why it is there, so I didn't touch it beyond letting it tag along with the convention change.

kallewoof · 2021-08-24T12:35:50Z

I have a strong preference for following the convention of placing const on the left side of the type.

I'm totally fine with putting const to the left. However, that's definitely not what is happening in the code, to the point where I ended up putting it on the right here to stay with the code style, despite having no preference about it myself.

kallewoof · 2021-08-25T07:40:15Z

Switched to left-hand-const in all touched places.

meshcollider · 2021-08-25T09:18:20Z

Concept ACK

theStack · 2021-08-26T00:06:31Z

Concept ACK

Const-correctness is important; to quote Herb Sutter on this subject:

Safety-incorrect riflemen are not long for this world. Nor are const-incorrect programmers, carpenters who don't have time for
hard-hats, and electricians who don't have time to identify the live wire. There is no excuse for ignoring the safety mechanisms
provided with a product, and there is particularly no excuse for programmers too lazy to write const-correct code.

(source: "Exceptional C++: 47 Engineering Puzzles, Programming Problems, and Solutions")

lsilva01 · 2021-08-26T04:52:31Z

Concept ACK

kiminuo · 2021-08-26T06:46:04Z

Concept ACK

(For historians, the discussion started here: https://github.com/bitcoin/bitcoin/pull/22751/files#r693517224)

Edit: Maybe some of these can be converted too:

bitcoin/src/wallet/wallet.h

Line 164 in 84be9a8

OutputType const type;
bitcoin/src/wallet/transaction.h

Lines 354 to 355 in 84be9a8

CWalletTx(CWalletTx const &) = delete;

void operator=(CWalletTx const &x) = delete;

bitcoin/src/bench/nanobench.h

Lines 1756 to 1759 in 84be9a8

    
           std::streamsize const mPrecision; 
        
           std::streamsize const mWidth; 
        
           std::ostream::char_type const mFill; 
        
           std::ostream::fmtflags const mFmtFlags;

bitcoin/src/bench/nanobench.h

Line 2441 in 84be9a8

uint64_t const numIters = 100000U + (std::random_device{}() & 3);

kallewoof · 2021-08-26T07:03:13Z

@kiminuo I'm limiting the scope on this PR to only shared pointers to CWallets to avoid a too big diff.

theStack

Code-review ACK 6b98ce3 🍶

Based on this PR, I opened a follow-up #22805 which also uses CWallet const shared pointers on the dumpprivkey and dumpwallet RPC methods.

promag

Concept ACK.

6b98ce3 could just change to std::shared_ptr<const T> though.

kallewoof · 2021-08-30T03:22:54Z

could just change to std::shared_ptr<const T> though.

I'm not sure I follow. Are you suggesting replacing const std::shared_ptr<const CWallet> with const std::shared_ptr<const T>? Where does T come from?

While const shared_ptr<X> gives us an immutable shared pointer to a mutable X (we can't set it to some other X later), shared_ptr<const X> gives us a shared pointer to an immutable X. Importantly, we can recast shared_ptr<X> into shared_ptr<const X>, but not the other way around. We do this for two reasons: because it makes the code safer to guarantee the wallet is not modified, and because it further dispells the misconception that const shared_ptr<X> gives immutability to X.

kallewoof · 2021-09-03T02:40:18Z

@promag That makes sense to me, but it isn't clear to the reviewer why some lines are left as ... const ... and some are changed to const ... ....

practicalswift · 2021-09-04T13:57:43Z

Concept ACK

kiminuo · 2021-09-19T08:29:49Z

I'm limiting the scope on this PR to only shared pointers to CWallets to avoid a too big diff.

So should the lines like the following ones be fixed too? I mean to move const from the right side to the left side.

src\wallet\rpcdump.cpp:122:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcdump.cpp:213:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcdump.cpp:251:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcdump.cpp:339:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcdump.cpp:400:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcdump.cpp:447:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcdump.cpp:528:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcdump.cpp:684:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcdump.cpp:734:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcdump.cpp:1346:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(mainRequest);
src\wallet\rpcdump.cpp:1661:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(main_request);
src\wallet\rpcwallet.cpp:255:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:308:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:354:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:491:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:915:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:988:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:1786:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:1859:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:1912:    std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:2000:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:2053:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:2097:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:2170:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:2324:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:2586:    std::shared_ptr<CWallet> const wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
src\wallet\rpcwallet.cpp:2673:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:3356:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:3539:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:3678:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:4203:            std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:4330:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:4527:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:4607:    std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
src\wallet\rpcwallet.cpp:4664:            std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);

Produced by:

Get-ChildItem -Recurse *.* | Select-String "std::shared_ptr<CWallet>"  | Select-String "const std::shared_ptr<CWallet>" -notMatch

kallewoof · 2021-09-19T08:47:03Z

So should the lines like the following ones be fixed too? I mean to move const from the right side to the left side.

It's tempting. Right now I'm only changing the lines that I'm touching for other reasons. If others agree, I can ramp it up to include all cases.

theStack

re-ACK 52a26e0

Verified via git range-diff 6b98ce32...52a26e08 that changes since my previous ACK were only rebase-related.

Introduce convention to use const shared pointers everywhere, unless the shared pointer is modified at some point, which it very rarely is. We want this convention, as it helps alleviate the misconception that a const shared pointer somehow results in a pointer to an immutable object, which is false.

While const shared_ptr<X> gives us an immutable shared pointer to a mutable X (we can't set it to some other X later), shared_ptr<const X> gives us a shared pointer to an immutable X. Importantly, we can recast shared_ptr<X> into shared_ptr<const X>, but not the other way around. We do this for two reasons: because it makes the code safer to guarantee the wallet is not modified, and because it further dispells the misconception that const shared_ptr<X> gives immutability to X.

theStack

re-ACK 54011e7

Verified via git range-diff 52a26e08...54011e7a that changes since my previous ACK were only rebase-related.

54011e7 refactor: use CWallet const shared pointers when possible (Karl-Johan Alm) 9646198 refactor: const shared_ptrs (Karl-Johan Alm) Pull request description: ```C++ const std::shared_ptr<CWallet> wallet = x; ``` means we can not do `wallet = y`, but we can totally do `wallet->DestructiveOperation()`, contrary to what that line looks like. This PR * introduces a new convention: always use const shared pointers to `CWallet`s (even when we mutate the pointed-to thing) * uses `const shared_ptr<const CWallet>` everywhere where wallets are not modified In the future, this should preferably apply to all shared pointers, not limited to just `CWallet`s. Both of these serve the same purpose: to dispell the misconception that `const shared_ptr<X>` immutates `X`. It doesn't, and it's dangerous to leave this misconception as is, for obvious reasons. ACKs for top commit: theStack: re-ACK 54011e7 Tree-SHA512: 3bf4062fc821751be30770c6b4ead10a016847970f155a0a5156f304347d221b9830840030c2fbfba8cd1e282f4eda45f5b4107fe6df8138afdcb6c2e95a2836

…vkey,wallet} d150fe3 refactor: use `CWallet` const shared pointers in dump{privkey,wallet} RPCs (Sebastian Falbesoner) ec2792d refactor: use const `LegacyScriptPubKeyMan` references in dump{privkey,wallet} RPCs (Sebastian Falbesoner) 29905c0 refactor: avoid multiple key->metadata lookups in dumpwallet RPC (Sebastian Falbesoner) Pull request description: ~~This PR is based on #22787 ("refactor: actual immutable pointing"), which should be reviewed first.~~ (merged by now) It aims to make the CWallet shared pointers actually immutable also for the `dumpprivkey` and `dumpwallet` RPC methods. For doing that, some more preparations are needed; we need a const-counterpart to the helper `EnsureLegacyScriptPubKeyMan` that accepts a const CWallet pointer and accordingly also returns a const `LegacyScriptPubKeyMan` instance. The metadata lookup in `dumpwallet` is changed to not need a mutable `ScriptPubKeyMan` instance by avoiding using the `operator[]` in its mapKeyMetadata map, which also avoids repeated lookups. ACKs for top commit: laanwj: Code review ACK d150fe3 Tree-SHA512: 90ac05e21f40c6d0ebb479a71c545da2fd89940b9ca3409d9f932abc983bf8830d34c45086e68880d0d1e994846fbefee7534eec142ff4268e0fa28a1a411d36

…ump{privkey,wallet} d150fe3 refactor: use `CWallet` const shared pointers in dump{privkey,wallet} RPCs (Sebastian Falbesoner) ec2792d refactor: use const `LegacyScriptPubKeyMan` references in dump{privkey,wallet} RPCs (Sebastian Falbesoner) 29905c0 refactor: avoid multiple key->metadata lookups in dumpwallet RPC (Sebastian Falbesoner) Pull request description: ~~This PR is based on bitcoin#22787 ("refactor: actual immutable pointing"), which should be reviewed first.~~ (merged by now) It aims to make the CWallet shared pointers actually immutable also for the `dumpprivkey` and `dumpwallet` RPC methods. For doing that, some more preparations are needed; we need a const-counterpart to the helper `EnsureLegacyScriptPubKeyMan` that accepts a const CWallet pointer and accordingly also returns a const `LegacyScriptPubKeyMan` instance. The metadata lookup in `dumpwallet` is changed to not need a mutable `ScriptPubKeyMan` instance by avoiding using the `operator[]` in its mapKeyMetadata map, which also avoids repeated lookups. ACKs for top commit: laanwj: Code review ACK d150fe3 Tree-SHA512: 90ac05e21f40c6d0ebb479a71c545da2fd89940b9ca3409d9f932abc983bf8830d34c45086e68880d0d1e994846fbefee7534eec142ff4268e0fa28a1a411d36

fanquake added Refactoring Wallet labels Aug 24, 2021

kallewoof mentioned this pull request Aug 24, 2021

rpc/wallet: add simulaterawtransaction RPC #22751

Merged

bitcoin deleted a comment Aug 24, 2021

kallewoof force-pushed the 202108-const-shared-ptrs branch from b7ac1de to 30d9674 Compare August 24, 2021 04:51

jnewbery reviewed Aug 24, 2021

View reviewed changes

This was referenced Aug 24, 2021

Make bech32m the default for RPC, opt-in for GUI #22260

Closed

Remove gArgs from wallet.h and wallet.cpp #22183

Merged

multiprocess: Add bitcoin-gui -ipcconnect option #19461

Draft

multiprocess: Add bitcoin-wallet -ipcconnect option #19460

Draft

kallewoof force-pushed the 202108-const-shared-ptrs branch 2 times, most recently from 636ffa9 to 247e7a3 Compare August 25, 2021 07:39

kallewoof force-pushed the 202108-const-shared-ptrs branch from 247e7a3 to cf89cbe Compare August 25, 2021 07:43

DrahtBot added the Needs rebase label Aug 26, 2021

kallewoof force-pushed the 202108-const-shared-ptrs branch from cf89cbe to 6b98ce3 Compare August 26, 2021 10:14

DrahtBot removed the Needs rebase label Aug 26, 2021

theStack mentioned this pull request Aug 26, 2021

refactor: use CWallet const shared pointers in dump{privkey,wallet} #22805

Merged

theStack approved these changes Aug 26, 2021

View reviewed changes

promag reviewed Aug 29, 2021

View reviewed changes

bitcoin deleted a comment from jaysonmald35 Aug 30, 2021

bitcoin deleted a comment from cryptod321 Sep 8, 2021

DrahtBot mentioned this pull request Sep 9, 2021

refactor: Remove gArgs from wallet.h and wallet.cpp (2) #22928

Merged

DrahtBot mentioned this pull request Sep 29, 2021

Remove -rescan startup parameter #23123

Closed

DrahtBot added the Needs rebase label Sep 30, 2021

kallewoof force-pushed the 202108-const-shared-ptrs branch from 6b98ce3 to 52a26e0 Compare October 1, 2021 02:23

DrahtBot removed the Needs rebase label Oct 1, 2021

theStack approved these changes Oct 2, 2021

View reviewed changes

DrahtBot mentioned this pull request Oct 15, 2021

tests: remove usage of LegacyScriptPubKeyMan from some wallet tests #23288

Merged

DrahtBot added the Needs rebase label Oct 22, 2021

kallewoof added 2 commits October 25, 2021 16:12

kallewoof force-pushed the 202108-const-shared-ptrs branch from 52a26e0 to 54011e7 Compare October 25, 2021 07:13

DrahtBot removed the Needs rebase label Oct 25, 2021

theStack approved these changes Oct 28, 2021

View reviewed changes

maflcko merged commit baa9fc9 into bitcoin:master Oct 29, 2021

kallewoof deleted the 202108-const-shared-ptrs branch October 29, 2021 12:25

bitcoin locked and limited conversation to collaborators Oct 30, 2022

refactor: actual immutable pointing #22787

refactor: actual immutable pointing #22787

Uh oh!

Conversation

kallewoof commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jnewbery commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

kallewoof Aug 24, 2021

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Aug 24, 2021

Uh oh!

kallewoof commented Aug 25, 2021

Uh oh!

meshcollider commented Aug 25, 2021

Uh oh!

theStack commented Aug 26, 2021

Uh oh!

lsilva01 commented Aug 26, 2021

Uh oh!

kiminuo commented Aug 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Aug 26, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Sep 3, 2021

Uh oh!

practicalswift commented Sep 4, 2021

Uh oh!

kiminuo commented Sep 19, 2021

Uh oh!

kallewoof commented Sep 19, 2021

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

kallewoof commented Aug 24, 2021 •

edited

Loading

DrahtBot commented Aug 24, 2021 •

edited

Loading

jnewbery commented Aug 24, 2021 •

edited

Loading

kallewoof commented Aug 24, 2021 •

edited

Loading

kiminuo commented Aug 26, 2021 •

edited

Loading

kallewoof commented Aug 30, 2021 •

edited

Loading