rpc: replace raw pointers with shared_ptrs #18592

pull brakmic wants to merge 1 commits into bitcoin:master from brakmic:rpcwallet-replace-pointers changing 3 files +146 −199
  1. brakmic commented at 8:58 AM on April 11, 2020: contributor

    This PR replaces raw pointers in rpcwallet.cpp and rpcdump.cpp with shared_ptr. The motivation for this PR is described here #18590

    The currently available unit and functional tests complete without errors.

    It seems that this PR is indirectly related to this issue: #13063 (review)

    Notice: I have deliberately not changed the class WalletRescanReserver whose constructor expects a raw pointer, because it's external and affects other areas, which I didn't touch to avoid making this PR "viral".

    Fixes https://github.com/bitcoin/bitcoin/issues/18590

  2. fanquake added the label RPC/REST/ZMQ on Apr 11, 2020
  3. fanquake added the label Wallet on Apr 11, 2020
  4. in src/wallet/rpcwallet.cpp:43 in 0b3ee61549 outdated 
      39 | @@ -40,7 +40,7 @@
  40 |  static const std::string WALLET_ENDPOINT_BASE = "/wallet/";
  41 |  static const std::string HELP_REQUIRING_PASSPHRASE{"\nRequires wallet passphrase to be set with walletpassphrase call if wallet is encrypted.\n"};
  42 |  
  43 | -static inline bool GetAvoidReuseFlag(const CWallet* const pwallet, const UniValue& param) {
  44 | +static inline bool GetAvoidReuseFlag(const std::shared_ptr<CWallet> pwallet, const UniValue& param) {
    MarcoFalke commented at 10:23 AM on April 11, 2020:

    read-only access to a wallet that is not a nullptr should probably be

    static inline bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param) {

    All RPCs check wallet for nullptr at the beginning of the function, so there shouldn't be an occasion where this is assumption fails.

    brakmic commented at 10:38 AM on April 11, 2020:

    Yes, have changed it. Have also changed the CWallet reference variable name in ParseIncludeWatchonly.

  5. in src/wallet/rpcwallet.cpp:114 in dc1feb63fa outdated 
     110 | @@ -111,13 +111,32 @@ bool EnsureWalletIsAvailable(const CWallet* pwallet, bool avoidException)
 111 |          "Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).");
 112 |  }
 113 |  
 114 | +bool EnsureWalletRefIsAvailable(const std::shared_ptr<CWallet> pwallet, bool avoidException)
    promag commented at 10:54 AM on April 11, 2020:

    Instead of new method, you could change EnsureWalletIsAvailable and update rpcdump.cpp? Also, const std::shared_ptr<CWallet>& avoids incr/decr usage count.

    brakmic commented at 10:59 AM on April 11, 2020:

    Yes, I could, but I didn't want to affect anything outside rpcwallet.cpp. At least not without consultation with others.

    promag commented at 11:02 AM on April 11, 2020:

    You can consider rpcdump.cpp and rpcwallet.cpp as one, these belong to libbitcoin_wallet. Furthermore, the diff is more of the same.

    brakmic commented at 11:03 AM on April 11, 2020:

    Ok, then I will adapt rpcdump.cpp as well.

  6. in src/wallet/rpcwallet.cpp:133 in dc1feb63fa outdated 
     128 |      if (pwallet->IsLocked()) {
 129 |          throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: Please enter the wallet passphrase with walletpassphrase first.");
 130 |      }
 131 |  }
 132 |  
 133 | +void EnsureWalletRefIsUnlocked(const std::shared_ptr<CWallet> pwallet)
    promag commented at 10:54 AM on April 11, 2020:

    Same as above.

  7. promag commented at 10:56 AM on April 11, 2020: member

    Concept ACK.

  8. DrahtBot commented at 12:36 PM on April 11, 2020: 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:

    • #18814 (rpc: Relock wallet only if most recent callback by promag)
    • #18734 (Allow simple multiwallet rpc calls by jonasschnelli)
    • #18654 (rpc: separate bumpfee's psbt creation function into psbtbumpfee by achow101)
    • #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
    • #18354 (Protect wallet by using shared pointers by bvbfan)
    • #18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)
    • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
    • #16377 ([rpc] don't automatically append inputs in walletcreatefundedpsbt by Sjors)
    • #14707 ([RPC] Include coinbase transactions in receivedby RPCs by andrewtoth)
    • #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.

  9. in src/wallet/rpcdump.cpp:57 in d7ea444df7 outdated 
      53 | @@ -54,7 +54,7 @@ static std::string DecodeDumpString(const std::string &str) {
  54 |      return ret.str();
  55 |  }
  56 |  
  57 | -static bool GetWalletAddressesForKey(LegacyScriptPubKeyMan* spk_man, const CWallet* const pwallet, const CKeyID& keyid, std::string& strAddr, std::string& strLabel) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
  58 | +static bool GetWalletAddressesForKey(LegacyScriptPubKeyMan* spk_man, const std::shared_ptr<CWallet> pwallet, const CKeyID& keyid, std::string& strAddr, std::string& strLabel) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
    MarcoFalke commented at 12:48 PM on April 11, 2020:

    Methods that only access members of the wallet struct don't need a shared pointer, which can be null and has minimal overhead for reference counting. A plain reference is sufficient.

    static bool GetWalletAddressesForKey(LegacyScriptPubKeyMan* spk_man, const CWallet& wallet, const CKeyID& keyid, std::string& strAddr, std::string& strLabel) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
  10. in src/wallet/rpcwallet.cpp:321 in d7ea444df7 outdated 
     317 | @@ -321,7 +318,7 @@ static UniValue setlabel(const JSONRPCRequest& request)
 318 |  }
 319 |  
 320 |  
 321 | -static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, CWallet * const pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue)
 322 | +static CTransactionRef SendMoney(interfaces::Chain::Lock& locked_chain, const std::shared_ptr<CWallet> pwallet, const CTxDestination &address, CAmount nValue, bool fSubtractFeeFromAmount, const CCoinControl& coin_control, mapValue_t mapValue)
    MarcoFalke commented at 12:49 PM on April 11, 2020:

    same here

  11. in src/wallet/rpcwallet.cpp:1016 in d7ea444df7 outdated 
    1012 | @@ -1025,7 +1013,7 @@ struct tallyitem
1013 |      }
1014 |  };
1015 |  
1016 | -static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const CWallet* const pwallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
1017 | +static UniValue ListReceived(interfaces::Chain::Lock& locked_chain, const std::shared_ptr<CWallet> pwallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
    MarcoFalke commented at 12:50 PM on April 11, 2020:

    Same here (and everywhere else)

    brakmic commented at 1:20 PM on April 11, 2020:

    Thanks. Have adapted them all.

  12. in src/wallet/rpcdump.cpp:1167 in 5fb8dc8a1c outdated 
    1163 | @@ -1173,7 +1164,7 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
1164 |      return warnings;
1165 |  }
1166 |  
1167 | -static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
1168 | +static UniValue ProcessImport(std::shared_ptr<CWallet> pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
    MarcoFalke commented at 1:36 PM on April 11, 2020:

    same here

    brakmic commented at 2:17 PM on April 11, 2020:

    Replaced them all. Executed unit tests passed.

    MarcoFalke commented at 6:42 PM on April 11, 2020:

    What about this one? ;)

    brakmic commented at 6:44 PM on April 11, 2020:

    Ah, ok, understand. I was thinking about raw pointers.

  13. in src/wallet/rpcwallet.cpp:1276 in 5fb8dc8a1c outdated 
    1272 | @@ -1287,7 +1273,7 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
1273 |   * @param  filter_ismine  The "is mine" filter flags.
1274 |   * @param  filter_label   Optional label string to filter incoming transactions.
1275 |   */
1276 | -static void ListTransactions(interfaces::Chain::Lock& locked_chain, const CWallet* const pwallet, const CWalletTx& wtx, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter_ismine, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
1277 | +static void ListTransactions(interfaces::Chain::Lock& locked_chain, const std::shared_ptr<CWallet> pwallet, const CWalletTx& wtx, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter_ismine, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
    MarcoFalke commented at 1:38 PM on April 11, 2020:

    same

  14. in src/wallet/rpcwallet.cpp:2993 in 5fb8dc8a1c outdated 
    2989 | @@ -3020,7 +2990,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
2990 |      return results;
2991 |  }
2992 |  
2993 | -void FundTransaction(CWallet* const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options)
2994 | +void FundTransaction(std::shared_ptr<CWallet> const pwallet, CMutableTransaction& tx, CAmount& fee_out, int& change_position, UniValue options)
    MarcoFalke commented at 1:38 PM on April 11, 2020:

    same

  15. in src/wallet/rpcwallet.cpp:3659 in 5fb8dc8a1c outdated 
    3655 | @@ -3691,7 +3656,7 @@ class DescribeWalletAddressVisitor : public boost::static_visitor<UniValue>
3656 |      UniValue operator()(const WitnessUnknown& id) const { return UniValue(UniValue::VOBJ); }
3657 |  };
3658 |  
3659 | -static UniValue DescribeWalletAddress(const CWallet* const pwallet, const CTxDestination& dest)
3660 | +static UniValue DescribeWalletAddress(const std::shared_ptr<CWallet> pwallet, const CTxDestination& dest)
    MarcoFalke commented at 1:38 PM on April 11, 2020:

    same

  16. MarcoFalke commented at 1:39 PM on April 11, 2020: member

    Looks like you forgot some. Maybe this could be split up into two commits? No strong opinion though.

  17. brakmic commented at 1:43 PM on April 11, 2020: contributor

    Looks like you forgot some. Maybe this could be split up into two commits? No strong opinion though.

    Oops, you're right. Thanks. Still some unneeded shared pointers. Will replace them, no problem.

  18. DrahtBot cross-referenced this on Apr 11, 2020 from issue refactor: consolidate sendmany and sendtoaddress code by Sjors
  19. DrahtBot cross-referenced this on Apr 11, 2020 from issue Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101
  20. DrahtBot cross-referenced this on Apr 11, 2020 from issue [rpc] don't automatically append inputs in walletcreatefundedpsbt by Sjors
  21. DrahtBot cross-referenced this on Apr 11, 2020 from issue [wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof
  22. DrahtBot cross-referenced this on Apr 11, 2020 from issue Remove CWalletTx merging logic from AddToWallet by ryanofsky
  23. in src/wallet/rpcwallet.cpp:114 in bf42f44706 outdated 
     110 | @@ -111,7 +111,7 @@ bool EnsureWalletIsAvailable(const CWallet* pwallet, bool avoidException)
 111 |          "Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).");
 112 |  }
 113 |  
 114 | -void EnsureWalletIsUnlocked(const CWallet* pwallet)
 115 | +void EnsureWalletIsUnlocked(const std::shared_ptr<CWallet>& pwallet)
    MarcoFalke commented at 5:15 PM on April 11, 2020:

    This is also called when the pointer is never nullptr, so a reference is more appropriate.

    void EnsureWalletIsUnlocked(const CWallet& wallet)
    brakmic commented at 5:24 PM on April 11, 2020:

    Adapted. Unit tests passing.

    promag commented at 6:24 PM on April 11, 2020:

    Yeah, my bad.

  24. MarcoFalke approved
  25. MarcoFalke commented at 5:18 PM on April 11, 2020: member

    I am still seeing two pointers, otherwise ACK

  26. brakmic commented at 5:27 PM on April 11, 2020: contributor

    I am still seeing two pointers, otherwise ACK

    Two pointers?

    But I already changed: ListTransactions , DescribeWalletAddress and FundTransaction. Or did you mean the latest change with EnsureWalletIsUnlocked ?

    --EDIT: Nevermind, yes, it was about EnsureWalletIsUnlocked.

  27. promag commented at 11:53 PM on April 11, 2020: member

    Code review ACK e6817785da4531648969af6f1202a4fa3b92707a.

  28. DrahtBot cross-referenced this on Apr 12, 2020 from issue rpc: Raise error in getbalance if minconf is not zero by promag
  29. MarcoFalke commented at 12:54 AM on April 12, 2020: member

    ACK e6817785da4531648969af6f1202a4fa3b92707a

  30. DrahtBot cross-referenced this on Apr 12, 2020 from issue wallet: Refactor WalletRescanReserver to use wallet reference by promag
  31. DrahtBot cross-referenced this on Apr 12, 2020 from issue rpc: remove deprecated CRPCCommand constructor by MarcoFalke
  32. practicalswift commented at 7:45 PM on April 13, 2020: contributor

    Concept ACK

  33. DrahtBot cross-referenced this on Apr 13, 2020 from issue rpc: gui: Don't change behavior based on private keys disabled, instead add new buttons/rpcs/menu items by achow101
  34. Sjors commented at 3:13 PM on April 15, 2020: member

    @achow101 does this significantly get in the way of #15764?

  35. achow101 commented at 4:27 PM on April 15, 2020: member

    @achow101 does this significantly get in the way of #15764?

    No

  36. DrahtBot cross-referenced this on Apr 15, 2020 from issue rpc: separate bumpfee's psbt creation function into psbtbumpfee by achow101
  37. DrahtBot cross-referenced this on Apr 16, 2020 from issue wallet: Add BlockUntilSyncedToCurrentChain to dumpwallet by MarcoFalke
  38. DrahtBot cross-referenced this on Apr 17, 2020 from issue Reverse cs_main, cs_wallet lock order and reduce cs_main locking by ariard
  39. DrahtBot cross-referenced this on Apr 18, 2020 from issue wallet: Avoid translating RPC errors by MarcoFalke
  40. DrahtBot added the label Needs rebase on Apr 19, 2020
  41. DrahtBot removed the label Needs rebase on Apr 19, 2020
  42. DrahtBot cross-referenced this on Apr 22, 2020 from issue Allow simple multiwallet rpc calls by jonasschnelli
  43. DrahtBot added the label Needs rebase on Apr 23, 2020
  44. DrahtBot removed the label Needs rebase on Apr 23, 2020
  45. DrahtBot cross-referenced this on Apr 28, 2020 from issue Use shared pointers only in validation interface by bvbfan
  46. DrahtBot cross-referenced this on Apr 29, 2020 from issue rpc: Relock wallet only if most recent callback by promag
  47. DrahtBot added the label Needs rebase on May 1, 2020
  48. DrahtBot removed the label Needs rebase on May 1, 2020
  49. DrahtBot cross-referenced this on May 3, 2020 from issue [RPC] Include coinbase transactions in receivedby RPCs by andrewtoth
  50. DrahtBot added the label Needs rebase on May 4, 2020
  51. DrahtBot removed the label Needs rebase on May 4, 2020
  52. DrahtBot added the label Needs rebase on May 6, 2020
  53. DrahtBot removed the label Needs rebase on May 6, 2020
  54. rpc: replace raw pointers with shared_ptrs
        Co-authored-by: MarcoFalke <falke.marco@gmail.com>
    Co-authored-by: João Barbosa <joao.paulo.barbosa@gmail.com>
    c13d3c31b3
  55. ryanofsky approved
  56. ryanofsky commented at 1:58 AM on May 7, 2020: contributor

    Code review ACK c13d3c31b31a684e8ca560014a08f4cc33e1390d. Looks perfect!

  57. ryanofsky commented at 2:00 AM on May 7, 2020: contributor

    Travis error sync_mempools timeout in feature_fee_estimation.py in https://travis-ci.org/github/bitcoin/bitcoin/jobs/683889542#L3121 is presumably unrelated

  58. brakmic closed this on May 10, 2020
  59. ryanofsky commented at 9:08 PM on May 13, 2020: contributor

    closed this 3 days ago

    Believe this was just a rageclose (exciting!) and someone else could pick this up

  60. adamjonas added the label Up for grabs on Jan 15, 2021
  61. fanquake cross-referenced this on Mar 2, 2021 from issue rpc: replace wallet raw pointers with references (#18592 rebased) by fanquake
  62. fanquake removed the label Up for grabs on Mar 2, 2021
  63. DrahtBot cross-referenced this on Mar 2, 2021 from issue descriptor wallet: Cache last hardened xpub and use in normalized descriptors by achow101
  64. DrahtBot cross-referenced this on Mar 2, 2021 from issue wallet, rpc: update listdescriptors response format by S3RK
  65. DrahtBot cross-referenced this on Mar 2, 2021 from issue wallet, refactor: return out-params of CreateTransaction() as optional struct by theStack
  66. DrahtBot cross-referenced this on Mar 2, 2021 from issue rpc: Add missing BlockUntilSyncedToCurrentChain to wallet RPCs by MarcoFalke
  67. DrahtBot cross-referenced this on Mar 4, 2021 from issue refactor: Make CWalletTx sync state type-safe by ryanofsky
  68. DrahtBot cross-referenced this on Mar 4, 2021 from issue rpc: include_unsafe option for fundrawtransaction by t-bast
  69. DrahtBot cross-referenced this on Mar 5, 2021 from issue refactor: replace util::Ref with std::any (C++17) by theStack
  70. MarcoFalke referenced this in commit eea6196c3d on Mar 10, 2021
  71. sidhujag referenced this in commit 5b248c46a6 on Mar 10, 2021
  72. bitcoin locked this on Aug 16, 2022
Contributors
brakmicMarcoFalkepromagDrahtBotpracticalswiftSjorsachow101ryanofsky
Labels
WalletRPC/REST/ZMQ
Linked (view graph)
#13063 Use shared pointer to retain wallet instance#15764 Native descriptor wallets#18590 rpc: raw pointers should be replaced with shared_ptr#21331 rpc: replace wallet raw pointers with references (#18592 rebased)
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-19 12:54 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me