From 96461989a2de737151bc4fb216221bf49cb53ce6 Mon Sep 17 00:00:00 2001 From: Karl-Johan Alm Date: Wed, 25 Aug 2021 16:37:14 +0900 Subject: 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. --- src/qt/test/addressbooktests.cpp | 2 +- src/qt/test/wallettests.cpp | 2 +- src/wallet/rpcwallet.cpp | 4 ++-- src/wallet/test/wallet_tests.cpp | 12 ++++++------ src/wallet/wallet.cpp | 6 +++--- src/wallet/wallettool.cpp | 8 ++++---- 6 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/qt/test/addressbooktests.cpp b/src/qt/test/addressbooktests.cpp index 729957699a..ede0e4cf9e 100644 --- a/src/qt/test/addressbooktests.cpp +++ b/src/qt/test/addressbooktests.cpp @@ -63,7 +63,7 @@ void TestAddAddressesToSendBook(interfaces::Node& node) auto wallet_client = interfaces::MakeWalletClient(*test.m_node.chain, *Assert(test.m_node.args)); test.m_node.wallet_client = wallet_client.get(); node.setContext(&test.m_node); - std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockWalletDatabase()); wallet->LoadWallet(); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); { diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index c74c8f25b3..badaf019b0 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -141,7 +141,7 @@ void TestGUI(interfaces::Node& node) auto wallet_client = interfaces::MakeWalletClient(*test.m_node.chain, *Assert(test.m_node.args)); test.m_node.wallet_client = wallet_client.get(); node.setContext(&test.m_node); - std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(node.context()->chain.get(), "", CreateMockWalletDatabase()); wallet->LoadWallet(); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index f2f28c83ff..8ba81e8746 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -103,7 +103,7 @@ std::shared_ptr GetWalletForJSONRPCRequest(const JSONRPCRequest& reques std::string wallet_name; if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) { - std::shared_ptr pwallet = GetWallet(context, wallet_name); + const std::shared_ptr pwallet = GetWallet(context, wallet_name); if (!pwallet) throw JSONRPCError(RPC_WALLET_NOT_FOUND, "Requested wallet does not exist or is not loaded"); return pwallet; } @@ -2840,7 +2840,7 @@ static RPCHelpMan createwallet() options.create_passphrase = passphrase; bilingual_str error; std::optional load_on_start = request.params[6].isNull() ? std::nullopt : std::optional(request.params[6].get_bool()); - std::shared_ptr wallet = CreateWallet(context, request.params[0].get_str(), load_on_start, options, status, error, warnings); + const std::shared_ptr wallet = CreateWallet(context, request.params[0].get_str(), load_on_start, options, status, error, warnings); if (!wallet) { RPCErrorCode code = status == DatabaseStatus::FAILED_ENCRYPT ? RPC_WALLET_ENCRYPTION_FAILED : RPC_WALLET_ERROR; throw JSONRPCError(code, error.original); diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 0965128ade..7a658c10a2 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -41,7 +41,7 @@ static_assert(WALLET_INCREMENTAL_RELAY_FEE >= DEFAULT_INCREMENTAL_RELAY_FEE, "wa BOOST_FIXTURE_TEST_SUITE(wallet_tests, WalletTestingSetup) -static std::shared_ptr TestLoadWallet(WalletContext& context) +static const std::shared_ptr TestLoadWallet(WalletContext& context) { DatabaseOptions options; options.create_flags = WALLET_FLAG_DESCRIPTORS; @@ -208,7 +208,7 @@ BOOST_FIXTURE_TEST_CASE(importmulti_rescan, TestChain100Setup) // before the missing block, and success for a key whose creation time is // after. { - std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); WITH_LOCK(wallet->cs_wallet, wallet->SetLastBlockProcessed(newTip->nHeight, newTip->GetBlockHash())); WalletContext context; @@ -274,7 +274,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) { WalletContext context; context.args = &gArgs; - std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); { auto spk_man = wallet->GetOrCreateLegacyScriptPubKeyMan(); LOCK2(wallet->cs_wallet, spk_man->cs_KeyStore); @@ -296,7 +296,7 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // Call importwallet RPC and verify all blocks with timestamps >= BLOCK_TIME // were scanned, and no prior blocks were scanned. { - std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); LOCK(wallet->cs_wallet); wallet->SetupLegacyScriptPubKeyMan(); @@ -606,7 +606,7 @@ BOOST_FIXTURE_TEST_CASE(ListCoinsTest, ListCoinsTestingSetup) BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) { { - std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); wallet->SetupLegacyScriptPubKeyMan(); wallet->SetMinVersion(FEATURE_LATEST); wallet->SetWalletFlag(WALLET_FLAG_DISABLE_PRIVATE_KEYS); @@ -616,7 +616,7 @@ BOOST_FIXTURE_TEST_CASE(wallet_disableprivkeys, TestChain100Setup) BOOST_CHECK(!wallet->GetNewDestination(OutputType::BECH32, "", dest, error)); } { - std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); + const std::shared_ptr wallet = std::make_shared(m_node.chain.get(), "", CreateDummyWalletDatabase()); LOCK(wallet->cs_wallet); wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS); wallet->SetMinVersion(FEATURE_LATEST); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 803e88cda2..342e9ead89 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -221,7 +221,7 @@ std::shared_ptr LoadWalletInternal(WalletContext& context, const std::s } context.chain->initMessage(_("Loading wallet…").translated); - std::shared_ptr wallet = CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings); + const std::shared_ptr wallet = CWallet::Create(context, name, std::move(database), options.create_flags, error, warnings); if (!wallet) { error = Untranslated("Wallet loading failed.") + Untranslated(" ") + error; status = DatabaseStatus::FAILED_LOAD; @@ -301,7 +301,7 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& // Make the wallet context.chain->initMessage(_("Loading wallet…").translated); - std::shared_ptr wallet = CWallet::Create(context, name, std::move(database), wallet_creation_flags, error, warnings); + const std::shared_ptr wallet = CWallet::Create(context, name, std::move(database), wallet_creation_flags, error, warnings); if (!wallet) { error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error; status = DatabaseStatus::FAILED_CREATE; @@ -2540,7 +2540,7 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri int64_t nStart = GetTimeMillis(); // TODO: Can't use std::make_shared because we need a custom deleter but // should be possible to use std::allocate_shared. - std::shared_ptr walletInstance(new CWallet(chain, name, std::move(database)), ReleaseWallet); + const std::shared_ptr walletInstance(new CWallet(chain, name, std::move(database)), ReleaseWallet); bool rescan_required = false; DBErrors nLoadWalletRet = walletInstance->LoadWallet(); if (nLoadWalletRet != DBErrors::LOAD_OK) { diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index 88f0a2ce20..086415f152 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -40,7 +40,7 @@ static void WalletCreate(CWallet* wallet_instance, uint64_t wallet_creation_flag wallet_instance->TopUpKeyPool(); } -static std::shared_ptr MakeWallet(const std::string& name, const fs::path& path, DatabaseOptions options) +static const std::shared_ptr MakeWallet(const std::string& name, const fs::path& path, DatabaseOptions options) { DatabaseStatus status; bilingual_str error; @@ -151,7 +151,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) options.require_format = DatabaseFormat::SQLITE; } - std::shared_ptr wallet_instance = MakeWallet(name, path, options); + const std::shared_ptr wallet_instance = MakeWallet(name, path, options); if (wallet_instance) { WalletShowInfo(wallet_instance.get()); wallet_instance->Close(); @@ -159,7 +159,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) } else if (command == "info") { DatabaseOptions options; options.require_existing = true; - std::shared_ptr wallet_instance = MakeWallet(name, path, options); + const std::shared_ptr wallet_instance = MakeWallet(name, path, options); if (!wallet_instance) return false; WalletShowInfo(wallet_instance.get()); wallet_instance->Close(); @@ -184,7 +184,7 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) } else if (command == "dump") { DatabaseOptions options; options.require_existing = true; - std::shared_ptr wallet_instance = MakeWallet(name, path, options); + const std::shared_ptr wallet_instance = MakeWallet(name, path, options); if (!wallet_instance) return false; bilingual_str error; bool ret = DumpWallet(*wallet_instance, error); -- cgit v1.2.3 From 54011e7aa274bdc1b921440cc8b4623aa1e0d89e Mon Sep 17 00:00:00 2001 From: Karl-Johan Alm Date: Wed, 25 Aug 2021 16:39:04 +0900 Subject: refactor: use CWallet const shared pointers when possible While const shared_ptr gives us an immutable shared pointer to a mutable X (we can't set it to some other X later), shared_ptr gives us a shared pointer to an immutable X. Importantly, we can recast shared_ptr into shared_ptr, 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 gives immutability to X. --- src/wallet/rpcdump.cpp | 2 +- src/wallet/rpcwallet.cpp | 44 ++++++++++++++++++++++---------------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 9b09bc23d6..1f13b80f3e 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1788,7 +1788,7 @@ RPCHelpMan listdescriptors() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const wallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr wallet = GetWalletForJSONRPCRequest(request); if (!wallet) return NullUniValue; if (!wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 8ba81e8746..86bfa10d88 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -570,7 +570,7 @@ static RPCHelpMan listaddressgroupings() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -627,7 +627,7 @@ static RPCHelpMan signmessage() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; LOCK(pwallet->cs_wallet); @@ -729,7 +729,7 @@ static RPCHelpMan getreceivedbyaddress() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -767,7 +767,7 @@ static RPCHelpMan getreceivedbylabel() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -807,7 +807,7 @@ static RPCHelpMan getbalance() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -846,7 +846,7 @@ static RPCHelpMan getunconfirmedbalance() RPCExamples{""}, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -1234,7 +1234,7 @@ static RPCHelpMan listreceivedbyaddress() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -1276,7 +1276,7 @@ static RPCHelpMan listreceivedbylabel() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -1461,7 +1461,7 @@ static RPCHelpMan listtransactions() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -1577,7 +1577,7 @@ static RPCHelpMan listsinceblock() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; const CWallet& wallet = *pwallet; @@ -1718,7 +1718,7 @@ static RPCHelpMan gettransaction() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -1829,7 +1829,7 @@ static RPCHelpMan backupwallet() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -2331,7 +2331,7 @@ static RPCHelpMan listlockunspent() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; LOCK(pwallet->cs_wallet); @@ -2424,9 +2424,9 @@ static RPCHelpMan getbalances() HelpExampleRpc("getbalances", "")}, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const rpc_wallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr rpc_wallet = GetWalletForJSONRPCRequest(request); if (!rpc_wallet) return NullUniValue; - CWallet& wallet = *rpc_wallet; + const CWallet& wallet = *rpc_wallet; // Make sure the results are valid at least up to the most recent block // the user could have gotten from another RPC command prior to now @@ -2500,7 +2500,7 @@ static RPCHelpMan getwalletinfo() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; // Make sure the results are valid at least up to the most recent block @@ -3030,7 +3030,7 @@ static RPCHelpMan listunspent() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; int nMinDepth = 1; @@ -3593,7 +3593,7 @@ RPCHelpMan signrawtransactionwithwallet() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; RPCTypeCheck(request.params, {UniValue::VSTR, UniValue::VARR, UniValue::VSTR}, true); @@ -4058,7 +4058,7 @@ RPCHelpMan getaddressinfo() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; LOCK(pwallet->cs_wallet); @@ -4165,7 +4165,7 @@ static RPCHelpMan getaddressesbylabel() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; LOCK(pwallet->cs_wallet); @@ -4226,7 +4226,7 @@ static RPCHelpMan listlabels() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; LOCK(pwallet->cs_wallet); @@ -4555,7 +4555,7 @@ static RPCHelpMan walletprocesspsbt() }, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue { - std::shared_ptr const pwallet = GetWalletForJSONRPCRequest(request); + const std::shared_ptr pwallet = GetWalletForJSONRPCRequest(request); if (!pwallet) return NullUniValue; const CWallet& wallet{*pwallet}; -- cgit v1.2.3