refactor: actual immutable pointing #22787

pull kallewoof wants to merge 2 commits into bitcoin:master from kallewoof:202108-const-shared-ptrs changing 7 files +40 −40
  1. kallewoof commented at 4:24 AM on August 24, 2021: member 
    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.

  2. fanquake added the label Refactoring on Aug 24, 2021
  3. fanquake added the label Wallet on Aug 24, 2021
  4. kallewoof cross-referenced this on Aug 24, 2021 from issue rpc/wallet: add simulaterawtransaction RPC by kallewoof
  5. bitcoin deleted a comment on Aug 24, 2021
  6. bitcoin deleted a comment on Aug 24, 2021
  7. kallewoof force-pushed on Aug 24, 2021
  8. DrahtBot commented at 8:58 AM on August 24, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  9. jnewbery commented at 11:05 AM on August 24, 2021: member

    ~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
  10. kallewoof commented at 11:11 AM on August 24, 2021: member

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

  11. in src/wallet/interfaces.cpp:533 in 30d9674851 outdated 
     529 | @@ -530,7 +530,7 @@ class WalletClientImpl : public WalletClient
 530 |      //! WalletClient methods
 531 |      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
 532 |      {
 533 | -        std::shared_ptr<CWallet> wallet;
 534 | +        std::shared_ptr<CWallet> const wallet;
    jnewbery commented at 12:22 PM on August 24, 2021:

    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.

    kallewoof commented at 12:32 PM on August 24, 2021:

    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.

  12. jnewbery commented at 12:27 PM on August 24, 2021: member

    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.

  13. kallewoof commented at 12:35 PM on August 24, 2021: member

    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.

  14. DrahtBot cross-referenced this on Aug 24, 2021 from issue Make bech32m the default for RPC, opt-in for GUI by Sjors
  15. DrahtBot cross-referenced this on Aug 24, 2021 from issue Remove `gArgs` from `wallet.h` and `wallet.cpp` by kiminuo
  16. DrahtBot cross-referenced this on Aug 24, 2021 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky
  17. DrahtBot cross-referenced this on Aug 24, 2021 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
  18. kallewoof force-pushed on Aug 25, 2021
  19. kallewoof force-pushed on Aug 25, 2021
  20. kallewoof commented at 7:40 AM on August 25, 2021: member

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

  21. kallewoof force-pushed on Aug 25, 2021
  22. meshcollider commented at 9:18 AM on August 25, 2021: contributor

    Concept ACK

  23. theStack commented at 12:06 AM on August 26, 2021: contributor

    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")

  24. lsilva01 commented at 4:52 AM on August 26, 2021: contributor

    Concept ACK

  26. kallewoof commented at 7:03 AM on August 26, 2021: member

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

  27. DrahtBot added the label Needs rebase on Aug 26, 2021
  28. kallewoof force-pushed on Aug 26, 2021
  29. DrahtBot removed the label Needs rebase on Aug 26, 2021
  30. theStack cross-referenced this on Aug 26, 2021 from issue refactor: use CWallet const shared pointers in dump{privkey,wallet} by theStack
  31. theStack approved
  32. theStack commented at 1:36 PM on August 26, 2021: contributor

    Code-review ACK 6b98ce32bfcd2b7b93875bbdbdd16b5df47eb94f 🍶

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

  33. promag commented at 12:14 PM on August 29, 2021: member

    Concept ACK.

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

  34. kallewoof commented at 3:22 AM on August 30, 2021: member

    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?

  35. bitcoin deleted a comment on Aug 30, 2021
  36. bitcoin deleted a comment on Aug 30, 2021
  37. promag referenced this in commit 6b98ce32bf on Sep 2, 2021
  38. kallewoof commented at 2:40 AM on September 3, 2021: member

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

  39. practicalswift commented at 1:57 PM on September 4, 2021: contributor

    Concept ACK

  40. bitcoin deleted a comment on Sep 8, 2021
  41. DrahtBot cross-referenced this on Sep 9, 2021 from issue refactor: Remove `gArgs` from `wallet.h` and `wallet.cpp` (2) by kiminuo
  42. kiminuo commented at 8:29 AM on September 19, 2021: contributor

    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
  43. kallewoof commented at 8:47 AM on September 19, 2021: member

    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.

  44. DrahtBot cross-referenced this on Sep 29, 2021 from issue Remove `-rescan` startup parameter by meshcollider
  45. DrahtBot added the label Needs rebase on Sep 30, 2021
  46. kallewoof force-pushed on Oct 1, 2021
  47. DrahtBot removed the label Needs rebase on Oct 1, 2021
  48. theStack approved
  49. theStack commented at 9:10 PM on October 2, 2021: contributor

    re-ACK 52a26e08113005240fd915ce842fe480a2c25fee

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

  50. DrahtBot cross-referenced this on Oct 15, 2021 from issue tests: remove usage of LegacyScriptPubKeyMan from some wallet tests by achow101
  51. DrahtBot added the label Needs rebase on Oct 22, 2021
  52. refactor: const shared_ptrs
    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.
    96461989a2
  53. refactor: use CWallet const shared pointers when possible
    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.
    54011e7aa2
  54. kallewoof force-pushed on Oct 25, 2021
  55. DrahtBot removed the label Needs rebase on Oct 25, 2021
  56. theStack approved
  57. theStack commented at 11:24 PM on October 28, 2021: contributor

    re-ACK 54011e7aa274bdc1b921440cc8b4623aa1e0d89e

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

  58. MarcoFalke merged this on Oct 29, 2021
  59. MarcoFalke closed this on Oct 29, 2021
  60. kallewoof deleted the branch on Oct 29, 2021
  61. sidhujag referenced this in commit 6f6f500b8c on Oct 29, 2021
  62. laanwj referenced this in commit e7feb73f07 on Nov 10, 2021
  63. sidhujag referenced this in commit a5de714ef0 on Nov 10, 2021
  64. bitcoin locked this on Oct 30, 2022
Contributors
kallewoofDrahtBotjnewberymeshcollidertheStacklsilva01kiminuopromagpracticalswift
Labels
RefactoringWallet
Linked (view graph)
#22751 rpc/wallet: add simulaterawtransaction RPC#22805 refactor: use CWallet const shared pointers in dump{privkey,wallet}
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC