Skip to content

Commit eea6196

Browse files
author
MarcoFalke
committed
Merge #21331: rpc: replace wallet raw pointers with references (#18592 rebased)
7c90c67 rpc: refactor rpc wallet functions to take references instead of pointers (fanquake) 4866934 rpc: remove calls to CWallet.get() (fanquake) Pull request description: This is a rebased #18592. > This PR replaces raw pointers in `rpcwallet.cpp` and `rpcdump.cpp` with **shared_ptr**. The motivation for this PR is described here bitcoin/bitcoin#18590 > It seems that this PR is indirectly related to this issue: bitcoin/bitcoin#13063 (comment) > 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 bitcoin/bitcoin#18590 ACKs for top commit: MarcoFalke: ACK 7c90c67 🐧 ryanofsky: Code review ACK 7c90c67. Changes easy to review with `--word-diff-regex=. -U0` Tree-SHA512: 32d69c813026b02260e8a89de9d6a5ab9e87826ba230687246583ac7a80c8c3fd00318da4658f1450e04c23d2c77ae765862de0d2a110b1312b3b69a1161e7ba
2 parents ceb6df3 + 7c90c67 commit eea6196

File tree

3 files changed

+190
-241
lines changed

3 files changed

+190
-241
lines changed

‎src/wallet/rpcdump.cpp‎

Lines changed: 53 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,13 @@ static std::string DecodeDumpString(const std::string &str) {
5656
return ret.str();
5757
}
5858

59-
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)
59+
static bool GetWalletAddressesForKey(LegacyScriptPubKeyMan* spk_man, const CWallet& wallet, const CKeyID& keyid, std::string& strAddr, std::string& strLabel) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
6060
{
6161
bool fLabelFound = false;
6262
CKey key;
6363
spk_man->GetKey(keyid, key);
6464
for (const auto& dest : GetAllDestinationsForKey(key.GetPubKey())) {
65-
const auto* address_book_entry = pwallet->FindAddressBookEntry(dest);
65+
const auto* address_book_entry = wallet.FindAddressBookEntry(dest);
6666
if (address_book_entry) {
6767
if (!strAddr.empty()) {
6868
strAddr += ",";
@@ -73,7 +73,7 @@ static bool GetWalletAddressesForKey(LegacyScriptPubKeyMan* spk_man, const CWall
7373
}
7474
}
7575
if (!fLabelFound) {
76-
strAddr = EncodeDestination(GetDestinationForKey(key.GetPubKey(), pwallet->m_default_address_type));
76+
strAddr = EncodeDestination(GetDestinationForKey(key.GetPubKey(), wallet.m_default_address_type));
7777
}
7878
return fLabelFound;
7979
}
@@ -118,22 +118,21 @@ RPCHelpMan importprivkey()
118118
},
119119
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
120120
{
121-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
122-
if (!wallet) return NullUniValue;
123-
CWallet* const pwallet = wallet.get();
121+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
122+
if (!pwallet) return NullUniValue;
124123

125124
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
126125
throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
127126
}
128127

129-
EnsureLegacyScriptPubKeyMan(*wallet, true);
128+
EnsureLegacyScriptPubKeyMan(*pwallet, true);
130129

131130
WalletRescanReserver reserver(*pwallet);
132131
bool fRescan = true;
133132
{
134133
LOCK(pwallet->cs_wallet);
135134

136-
EnsureWalletIsUnlocked(pwallet);
135+
EnsureWalletIsUnlocked(*pwallet);
137136

138137
std::string strSecret = request.params[0].get_str();
139138
std::string strLabel = "";
@@ -210,9 +209,8 @@ RPCHelpMan abortrescan()
210209
},
211210
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
212211
{
213-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
214-
if (!wallet) return NullUniValue;
215-
CWallet* const pwallet = wallet.get();
212+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
213+
if (!pwallet) return NullUniValue;
216214

217215
if (!pwallet->IsScanning() || pwallet->IsAbortingRescan()) return false;
218216
pwallet->AbortRescan();
@@ -249,9 +247,8 @@ RPCHelpMan importaddress()
249247
},
250248
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
251249
{
252-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
253-
if (!wallet) return NullUniValue;
254-
CWallet* const pwallet = wallet.get();
250+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
251+
if (!pwallet) return NullUniValue;
255252

256253
EnsureLegacyScriptPubKeyMan(*pwallet, true);
257254

@@ -335,9 +332,8 @@ RPCHelpMan importprunedfunds()
335332
RPCExamples{""},
336333
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
337334
{
338-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
339-
if (!wallet) return NullUniValue;
340-
CWallet* const pwallet = wallet.get();
335+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
336+
if (!pwallet) return NullUniValue;
341337

342338
CMutableTransaction tx;
343339
if (!DecodeHexTx(tx, request.params[0].get_str())) {
@@ -397,9 +393,8 @@ RPCHelpMan removeprunedfunds()
397393
},
398394
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
399395
{
400-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
401-
if (!wallet) return NullUniValue;
402-
CWallet* const pwallet = wallet.get();
396+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
397+
if (!pwallet) return NullUniValue;
403398

404399
LOCK(pwallet->cs_wallet);
405400

@@ -445,11 +440,10 @@ RPCHelpMan importpubkey()
445440
},
446441
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
447442
{
448-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
449-
if (!wallet) return NullUniValue;
450-
CWallet* const pwallet = wallet.get();
443+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
444+
if (!pwallet) return NullUniValue;
451445

452-
EnsureLegacyScriptPubKeyMan(*wallet, true);
446+
EnsureLegacyScriptPubKeyMan(*pwallet, true);
453447

454448
std::string strLabel;
455449
if (!request.params[1].isNull())
@@ -527,11 +521,10 @@ RPCHelpMan importwallet()
527521
},
528522
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
529523
{
530-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
531-
if (!wallet) return NullUniValue;
532-
CWallet* const pwallet = wallet.get();
524+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
525+
if (!pwallet) return NullUniValue;
533526

534-
EnsureLegacyScriptPubKeyMan(*wallet, true);
527+
EnsureLegacyScriptPubKeyMan(*pwallet, true);
535528

536529
if (pwallet->chain().havePruned()) {
537530
// Exit early and print an error.
@@ -550,7 +543,7 @@ RPCHelpMan importwallet()
550543
{
551544
LOCK(pwallet->cs_wallet);
552545

553-
EnsureWalletIsUnlocked(pwallet);
546+
EnsureWalletIsUnlocked(*pwallet);
554547

555548
fsbridge::ifstream file;
556549
file.open(request.params[0].get_str(), std::ios::in | std::ios::ate);
@@ -684,15 +677,14 @@ RPCHelpMan dumpprivkey()
684677
},
685678
[&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
686679
{
687-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
688-
if (!wallet) return NullUniValue;
689-
const CWallet* const pwallet = wallet.get();
680+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
681+
if (!pwallet) return NullUniValue;
690682

691-
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet);
683+
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet);
692684

693685
LOCK2(pwallet->cs_wallet, spk_man.cs_KeyStore);
694686

695-
EnsureWalletIsUnlocked(pwallet);
687+
EnsureWalletIsUnlocked(*pwallet);
696688

697689
std::string strAddress = request.params[0].get_str();
698690
CTxDestination dest = DecodeDestination(strAddress);
@@ -747,7 +739,7 @@ RPCHelpMan dumpwallet()
747739

748740
LOCK2(wallet.cs_wallet, spk_man.cs_KeyStore);
749741

750-
EnsureWalletIsUnlocked(&wallet);
742+
EnsureWalletIsUnlocked(wallet);
751743

752744
fs::path filepath = request.params[0].get_str();
753745
filepath = fs::absolute(filepath);
@@ -809,7 +801,7 @@ RPCHelpMan dumpwallet()
809801
CKey key;
810802
if (spk_man.GetKey(keyid, key)) {
811803
file << strprintf("%s %s ", EncodeSecret(key), strTime);
812-
if (GetWalletAddressesForKey(&spk_man, &wallet, keyid, strAddr, strLabel)) {
804+
if (GetWalletAddressesForKey(&spk_man, wallet, keyid, strAddr, strLabel)) {
813805
file << strprintf("label=%s", strLabel);
814806
} else if (keyid == seed_id) {
815807
file << "hdseed=1";
@@ -1169,7 +1161,7 @@ static UniValue ProcessImportDescriptor(ImportData& import_data, std::map<CKeyID
11691161
return warnings;
11701162
}
11711163

1172-
static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
1164+
static UniValue ProcessImport(CWallet& wallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
11731165
{
11741166
UniValue warnings(UniValue::VARR);
11751167
UniValue result(UniValue::VOBJ);
@@ -1184,7 +1176,7 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
11841176
const bool add_keypool = data.exists("keypool") ? data["keypool"].get_bool() : false;
11851177

11861178
// Add to keypool only works with privkeys disabled
1187-
if (add_keypool && !pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
1179+
if (add_keypool && !wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
11881180
throw JSONRPCError(RPC_INVALID_PARAMETER, "Keys can only be imported to the keypool when private keys are disabled");
11891181
}
11901182

@@ -1206,29 +1198,29 @@ static UniValue ProcessImport(CWallet * const pwallet, const UniValue& data, con
12061198
}
12071199

12081200
// If private keys are disabled, abort if private keys are being imported
1209-
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !privkey_map.empty()) {
1201+
if (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !privkey_map.empty()) {
12101202
throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
12111203
}
12121204

12131205
// Check whether we have any work to do
12141206
for (const CScript& script : script_pub_keys) {
1215-
if (pwallet->IsMine(script) & ISMINE_SPENDABLE) {
1207+
if (wallet.IsMine(script) & ISMINE_SPENDABLE) {
12161208
throw JSONRPCError(RPC_WALLET_ERROR, "The wallet already contains the private key for this address or script (\"" + HexStr(script) + "\")");
12171209
}
12181210
}
12191211

12201212
// All good, time to import
1221-
pwallet->MarkDirty();
1222-
if (!pwallet->ImportScripts(import_data.import_scripts, timestamp)) {
1213+
wallet.MarkDirty();
1214+
if (!wallet.ImportScripts(import_data.import_scripts, timestamp)) {
12231215
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding script to wallet");
12241216
}
1225-
if (!pwallet->ImportPrivKeys(privkey_map, timestamp)) {
1217+
if (!wallet.ImportPrivKeys(privkey_map, timestamp)) {
12261218
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding key to wallet");
12271219
}
1228-
if (!pwallet->ImportPubKeys(ordered_pubkeys, pubkey_map, import_data.key_origins, add_keypool, internal, timestamp)) {
1220+
if (!wallet.ImportPubKeys(ordered_pubkeys, pubkey_map, import_data.key_origins, add_keypool, internal, timestamp)) {
12291221
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
12301222
}
1231-
if (!pwallet->ImportScriptPubKeys(label, script_pub_keys, have_solving_data, !internal, timestamp)) {
1223+
if (!wallet.ImportScriptPubKeys(label, script_pub_keys, have_solving_data, !internal, timestamp)) {
12321224
throw JSONRPCError(RPC_WALLET_ERROR, "Error adding address to wallet");
12331225
}
12341226

@@ -1336,13 +1328,12 @@ RPCHelpMan importmulti()
13361328
},
13371329
[&](const RPCHelpMan& self, const JSONRPCRequest& mainRequest) -> UniValue
13381330
{
1339-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(mainRequest);
1340-
if (!wallet) return NullUniValue;
1341-
CWallet* const pwallet = wallet.get();
1331+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(mainRequest);
1332+
if (!pwallet) return NullUniValue;
13421333

13431334
RPCTypeCheck(mainRequest.params, {UniValue::VARR, UniValue::VOBJ});
13441335

1345-
EnsureLegacyScriptPubKeyMan(*wallet, true);
1336+
EnsureLegacyScriptPubKeyMan(*pwallet, true);
13461337

13471338
const UniValue& requests = mainRequest.params[0];
13481339

@@ -1368,7 +1359,7 @@ RPCHelpMan importmulti()
13681359
UniValue response(UniValue::VARR);
13691360
{
13701361
LOCK(pwallet->cs_wallet);
1371-
EnsureWalletIsUnlocked(pwallet);
1362+
EnsureWalletIsUnlocked(*pwallet);
13721363

13731364
// Verify all timestamps are present before importing any keys.
13741365
CHECK_NONFATAL(pwallet->chain().findBlock(pwallet->GetLastBlockHash(), FoundBlock().time(nLowestTimestamp).mtpTime(now)));
@@ -1380,7 +1371,7 @@ RPCHelpMan importmulti()
13801371

13811372
for (const UniValue& data : requests.getValues()) {
13821373
const int64_t timestamp = std::max(GetImportTimestamp(data, now), minimumTimestamp);
1383-
const UniValue result = ProcessImport(pwallet, data, timestamp);
1374+
const UniValue result = ProcessImport(*pwallet, data, timestamp);
13841375
response.push_back(result);
13851376

13861377
if (!fRescan) {
@@ -1447,7 +1438,7 @@ RPCHelpMan importmulti()
14471438
};
14481439
}
14491440

1450-
static UniValue ProcessDescriptorImport(CWallet * const pwallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
1441+
static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, const int64_t timestamp) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
14511442
{
14521443
UniValue warnings(UniValue::VARR);
14531444
UniValue result(UniValue::VOBJ);
@@ -1516,7 +1507,7 @@ static UniValue ProcessDescriptorImport(CWallet * const pwallet, const UniValue&
15161507
}
15171508

15181509
// If the wallet disabled private keys, abort if private keys exist
1519-
if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !keys.keys.empty()) {
1510+
if (wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) && !keys.keys.empty()) {
15201511
throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
15211512
}
15221513

@@ -1540,7 +1531,7 @@ static UniValue ProcessDescriptorImport(CWallet * const pwallet, const UniValue&
15401531
}
15411532

15421533
// If private keys are enabled, check some things.
1543-
if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
1534+
if (!wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
15441535
if (keys.keys.empty()) {
15451536
throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import descriptor without private keys to a wallet with private keys enabled");
15461537
}
@@ -1552,7 +1543,7 @@ static UniValue ProcessDescriptorImport(CWallet * const pwallet, const UniValue&
15521543
WalletDescriptor w_desc(std::move(parsed_desc), timestamp, range_start, range_end, next_index);
15531544

15541545
// Check if the wallet already contains the descriptor
1555-
auto existing_spk_manager = pwallet->GetDescriptorScriptPubKeyMan(w_desc);
1546+
auto existing_spk_manager = wallet.GetDescriptorScriptPubKeyMan(w_desc);
15561547
if (existing_spk_manager) {
15571548
LOCK(existing_spk_manager->cs_desc_man);
15581549
if (range_start > existing_spk_manager->GetWalletDescriptor().range_start) {
@@ -1561,7 +1552,7 @@ static UniValue ProcessDescriptorImport(CWallet * const pwallet, const UniValue&
15611552
}
15621553

15631554
// Add descriptor to the wallet
1564-
auto spk_manager = pwallet->AddWalletDescriptor(w_desc, keys, label, internal);
1555+
auto spk_manager = wallet.AddWalletDescriptor(w_desc, keys, label, internal);
15651556
if (spk_manager == nullptr) {
15661557
throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Could not add descriptor '%s'", descriptor));
15671558
}
@@ -1571,7 +1562,7 @@ static UniValue ProcessDescriptorImport(CWallet * const pwallet, const UniValue&
15711562
if (!w_desc.descriptor->GetOutputType()) {
15721563
warnings.push_back("Unknown output type, cannot set descriptor to active.");
15731564
} else {
1574-
pwallet->AddActiveScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), internal);
1565+
wallet.AddActiveScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), internal);
15751566
}
15761567
}
15771568

@@ -1641,9 +1632,8 @@ RPCHelpMan importdescriptors()
16411632
},
16421633
[&](const RPCHelpMan& self, const JSONRPCRequest& main_request) -> UniValue
16431634
{
1644-
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(main_request);
1645-
if (!wallet) return NullUniValue;
1646-
CWallet* const pwallet = wallet.get();
1635+
std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(main_request);
1636+
if (!pwallet) return NullUniValue;
16471637

16481638
// Make sure wallet is a descriptor wallet
16491639
if (!pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
@@ -1665,15 +1655,15 @@ RPCHelpMan importdescriptors()
16651655
UniValue response(UniValue::VARR);
16661656
{
16671657
LOCK(pwallet->cs_wallet);
1668-
EnsureWalletIsUnlocked(pwallet);
1658+
EnsureWalletIsUnlocked(*pwallet);
16691659

16701660
CHECK_NONFATAL(pwallet->chain().findBlock(pwallet->GetLastBlockHash(), FoundBlock().time(lowest_timestamp).mtpTime(now)));
16711661

16721662
// Get all timestamps and extract the lowest timestamp
16731663
for (const UniValue& request : requests.getValues()) {
16741664
// This throws an error if "timestamp" doesn't exist
16751665
const int64_t timestamp = std::max(GetImportTimestamp(request, now), minimum_timestamp);
1676-
const UniValue result = ProcessDescriptorImport(pwallet, request, timestamp);
1666+
const UniValue result = ProcessDescriptorImport(*pwallet, request, timestamp);
16771667
response.push_back(result);
16781668

16791669
if (lowest_timestamp > timestamp ) {
@@ -1775,7 +1765,7 @@ RPCHelpMan listdescriptors()
17751765
throw JSONRPCError(RPC_WALLET_ERROR, "listdescriptors is not available for non-descriptor wallets");
17761766
}
17771767

1778-
EnsureWalletIsUnlocked(wallet.get());
1768+
EnsureWalletIsUnlocked(*wallet);
17791769

17801770
LOCK(wallet->cs_wallet);
17811771

0 commit comments

Comments
 (0)