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

    0static 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: 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:

    • #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.

    0static 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. 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.

    0void 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.
  19. MarcoFalke approved
  20. MarcoFalke commented at 5:18 pm on April 11, 2020: member
    I am still seeing two pointers, otherwise ACK
  21. 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.

  22. promag commented at 11:53 pm on April 11, 2020: member
    Code review ACK e6817785da4531648969af6f1202a4fa3b92707a.
  23. MarcoFalke commented at 0:54 am on April 12, 2020: member
    ACK e6817785da4531648969af6f1202a4fa3b92707a
  24. practicalswift commented at 7:45 pm on April 13, 2020: contributor
    Concept ACK
  25. Sjors commented at 3:13 pm on April 15, 2020: member
    @achow101 does this significantly get in the way of #15764?
  26. achow101 commented at 4:27 pm on April 15, 2020: member

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

    No

  27. DrahtBot added the label Needs rebase on Apr 19, 2020
  28. DrahtBot removed the label Needs rebase on Apr 19, 2020
  29. DrahtBot added the label Needs rebase on Apr 23, 2020
  30. DrahtBot removed the label Needs rebase on Apr 23, 2020
  31. DrahtBot added the label Needs rebase on May 1, 2020
  32. DrahtBot removed the label Needs rebase on May 1, 2020
  33. DrahtBot added the label Needs rebase on May 4, 2020
  34. DrahtBot removed the label Needs rebase on May 4, 2020
  35. DrahtBot added the label Needs rebase on May 6, 2020
  36. DrahtBot removed the label Needs rebase on May 6, 2020
  37. 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
  38. ryanofsky approved
  39. ryanofsky commented at 1:58 am on May 7, 2020: member
    Code review ACK c13d3c31b31a684e8ca560014a08f4cc33e1390d. Looks perfect!
  40. ryanofsky commented at 2:00 am on May 7, 2020: member
    Travis error sync_mempools timeout in feature_fee_estimation.py in https://travis-ci.org/github/bitcoin/bitcoin/jobs/683889542#L3121 is presumably unrelated
  41. brakmic closed this on May 10, 2020
  42. ryanofsky commented at 9:08 pm on May 13, 2020: member

    closed this 3 days ago

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

  43. adamjonas added the label Up for grabs on Jan 15, 2021
  44. fanquake removed the label Up for grabs on Mar 2, 2021
  45. MarcoFalke referenced this in commit eea6196c3d on Mar 10, 2021
  46. sidhujag referenced this in commit 5b248c46a6 on Mar 10, 2021
  47. DrahtBot locked this on Aug 16, 2022


brakmic MarcoFalke promag DrahtBot practicalswift Sjors achow101 ryanofsky

Labels
Wallet RPC/REST/ZMQ

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: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me