Skip to content

Commit 976cc76

Browse files
laanwjlaanwj
committed
Merge #17381: LegacyScriptPubKeyMan code cleanups
05b224a Add missing SetupGeneration error handling in EncryptWallet (Russell Yanofsky) bfd826a Clean up nested scope in GetReservedDestination (Russell Yanofsky) 491a599 Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method (Russell Yanofsky) 4a0abf6 Pass CTxDestination to ScriptPubKeyMan::GetMetadata (Russell Yanofsky) b07b07c Add EnsureLegacyScriptPubKeyMan and use in rpcwallet.cpp (Russell Yanofsky) Pull request description: This PR implements suggested code cleanups from #17300 and #17304 review comments ACKs for top commit: Sjors: re-ACK 05b224a laanwj: Code review ACK 05b224a Tree-SHA512: 12fd86637088515b744c028e0501c5d21a9cf9ee9c9cfd70e9cb65d44611ea5643abd5f6f101105caa5aff015d74de606f074f08af7dae8429f929d21288ab45
2 parents 6f4e247 + 05b224a commit 976cc76

File tree

6 files changed

+56
-67
lines changed

6 files changed

+56
-67
lines changed

‎src/wallet/rpcdump.cpp

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -87,15 +87,6 @@ static void RescanWallet(CWallet& wallet, const WalletRescanReserver& reserver,
8787
}
8888
}
8989

90-
static LegacyScriptPubKeyMan& GetLegacyScriptPubKeyMan(CWallet& wallet)
91-
{
92-
LegacyScriptPubKeyMan* spk_man = wallet.GetLegacyScriptPubKeyMan();
93-
if (!spk_man) {
94-
throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
95-
}
96-
return *spk_man;
97-
}
98-
9990
UniValue importprivkey(const JSONRPCRequest& request)
10091
{
10192
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
@@ -134,7 +125,7 @@ UniValue importprivkey(const JSONRPCRequest& request)
134125
throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
135126
}
136127

137-
LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
128+
EnsureLegacyScriptPubKeyMan(*wallet);
138129

139130
WalletRescanReserver reserver(pwallet);
140131
bool fRescan = true;
@@ -262,7 +253,7 @@ UniValue importaddress(const JSONRPCRequest& request)
262253
},
263254
}.Check(request);
264255

265-
LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*pwallet);
256+
EnsureLegacyScriptPubKeyMan(*pwallet);
266257

267258
std::string strLabel;
268259
if (!request.params[1].isNull())
@@ -465,7 +456,7 @@ UniValue importpubkey(const JSONRPCRequest& request)
465456
},
466457
}.Check(request);
467458

468-
LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
459+
EnsureLegacyScriptPubKeyMan(*wallet);
469460

470461
std::string strLabel;
471462
if (!request.params[1].isNull())
@@ -549,7 +540,7 @@ UniValue importwallet(const JSONRPCRequest& request)
549540
},
550541
}.Check(request);
551542

552-
LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
543+
EnsureLegacyScriptPubKeyMan(*wallet);
553544

554545
if (pwallet->chain().havePruned()) {
555546
// Exit early and print an error.
@@ -708,7 +699,7 @@ UniValue dumpprivkey(const JSONRPCRequest& request)
708699
},
709700
}.Check(request);
710701

711-
LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
702+
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet);
712703

713704
auto locked_chain = pwallet->chain().lock();
714705
LOCK(pwallet->cs_wallet);
@@ -759,7 +750,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
759750
},
760751
}.Check(request);
761752

762-
LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
753+
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet);
763754

764755
auto locked_chain = pwallet->chain().lock();
765756
LOCK(pwallet->cs_wallet);
@@ -1346,7 +1337,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
13461337

13471338
RPCTypeCheck(mainRequest.params, {UniValue::VARR, UniValue::VOBJ});
13481339

1349-
LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
1340+
EnsureLegacyScriptPubKeyMan(*wallet);
13501341

13511342
const UniValue& requests = mainRequest.params[0];
13521343

‎src/wallet/rpcwallet.cpp

Lines changed: 19 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,15 @@ void EnsureWalletIsUnlocked(const CWallet* pwallet)
124124
}
125125
}
126126

127+
LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet)
128+
{
129+
LegacyScriptPubKeyMan* spk_man = wallet.GetLegacyScriptPubKeyMan();
130+
if (!spk_man) {
131+
throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
132+
}
133+
return *spk_man;
134+
}
135+
127136
static void WalletTxToJSON(interfaces::Chain& chain, interfaces::Chain::Lock& locked_chain, const CWalletTx& wtx, UniValue& entry)
128137
{
129138
int confirms = wtx.GetDepthInMainChain(locked_chain);
@@ -966,10 +975,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request)
966975
},
967976
}.Check(request);
968977

969-
LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan();
970-
if (!spk_man) {
971-
throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
972-
}
978+
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet);
973979

974980
auto locked_chain = pwallet->chain().lock();
975981
LOCK(pwallet->cs_wallet);
@@ -987,7 +993,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request)
987993
if (IsHex(keys_or_addrs[i].get_str()) && (keys_or_addrs[i].get_str().length() == 66 || keys_or_addrs[i].get_str().length() == 130)) {
988994
pubkeys.push_back(HexToPubKey(keys_or_addrs[i].get_str()));
989995
} else {
990-
pubkeys.push_back(AddrToPubKey(spk_man, keys_or_addrs[i].get_str()));
996+
pubkeys.push_back(AddrToPubKey(&spk_man, keys_or_addrs[i].get_str()));
991997
}
992998
}
993999

@@ -1000,7 +1006,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request)
10001006

10011007
// Construct using pay-to-script-hash:
10021008
CScript inner;
1003-
CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, *spk_man, inner);
1009+
CTxDestination dest = AddAndGetMultisigDestination(required, pubkeys, output_type, spk_man, inner);
10041010
pwallet->SetAddressBook(dest, label, "send");
10051011

10061012
UniValue result(UniValue::VOBJ);
@@ -3759,15 +3765,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)
37593765

37603766
ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan();
37613767
if (spk_man) {
3762-
CKeyID key_id = GetKeyForDestination(*provider, dest);
3763-
const CKeyMetadata* meta = nullptr;
3764-
if (!key_id.IsNull()) {
3765-
meta = spk_man->GetMetadata(key_id);
3766-
}
3767-
if (!meta) {
3768-
meta = spk_man->GetMetadata(CScriptID(scriptPubKey));
3769-
}
3770-
if (meta) {
3768+
if (const CKeyMetadata* meta = spk_man->GetMetadata(dest)) {
37713769
ret.pushKV("timestamp", meta->nCreateTime);
37723770
if (meta->has_key_origin) {
37733771
ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path));
@@ -3933,10 +3931,7 @@ UniValue sethdseed(const JSONRPCRequest& request)
39333931
},
39343932
}.Check(request);
39353933

3936-
LegacyScriptPubKeyMan* spk_man = pwallet->GetLegacyScriptPubKeyMan();
3937-
if (!spk_man) {
3938-
throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
3939-
}
3934+
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*pwallet);
39403935

39413936
if (pwallet->chain().isInitialBlockDownload()) {
39423937
throw JSONRPCError(RPC_CLIENT_IN_INITIAL_DOWNLOAD, "Cannot set a new HD seed while still in Initial Block Download");
@@ -3963,22 +3958,22 @@ UniValue sethdseed(const JSONRPCRequest& request)
39633958

39643959
CPubKey master_pub_key;
39653960
if (request.params[1].isNull()) {
3966-
master_pub_key = spk_man->GenerateNewSeed();
3961+
master_pub_key = spk_man.GenerateNewSeed();
39673962
} else {
39683963
CKey key = DecodeSecret(request.params[1].get_str());
39693964
if (!key.IsValid()) {
39703965
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key");
39713966
}
39723967

3973-
if (HaveKey(*spk_man, key)) {
3968+
if (HaveKey(spk_man, key)) {
39743969
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Already have this key (either as an HD seed or as a loose private key)");
39753970
}
39763971

3977-
master_pub_key = spk_man->DeriveNewSeed(key);
3972+
master_pub_key = spk_man.DeriveNewSeed(key);
39783973
}
39793974

3980-
spk_man->SetHDSeed(master_pub_key);
3981-
if (flush_key_pool) spk_man->NewKeyPool();
3975+
spk_man.SetHDSeed(master_pub_key);
3976+
if (flush_key_pool) spk_man.NewKeyPool();
39823977

39833978
return NullUniValue;
39843979
}

‎src/wallet/rpcwallet.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
class CRPCTable;
1313
class CWallet;
1414
class JSONRPCRequest;
15+
class LegacyScriptPubKeyMan;
1516
class UniValue;
1617
struct PartiallySignedTransaction;
1718
class CTransaction;
@@ -40,6 +41,7 @@ std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& reques
4041
std::string HelpRequiringPassphrase(const CWallet*);
4142
void EnsureWalletIsUnlocked(const CWallet*);
4243
bool EnsureWalletIsAvailable(const CWallet*, bool avoidException);
44+
LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet);
4345

4446
UniValue getaddressinfo(const JSONRPCRequest& request);
4547
UniValue signrawtransactionwithwallet(const JSONRPCRequest& request);

‎src/wallet/scriptpubkeyman.cpp

Lines changed: 22 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
1515
{
1616
error.clear();
17-
TopUpKeyPool();
17+
TopUp();
1818

1919
// Generate a new key that is added to wallet
2020
CPubKey new_key;
@@ -264,10 +264,8 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn)
264264

265265
bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool)
266266
{
267-
{
268-
if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
269-
return false;
270-
}
267+
if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
268+
return false;
271269
}
272270
return true;
273271
}
@@ -282,11 +280,6 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, cons
282280
ReturnKey(index, internal, pubkey);
283281
}
284282

285-
bool LegacyScriptPubKeyMan::TopUp(unsigned int size)
286-
{
287-
return TopUpKeyPool(size);
288-
}
289-
290283
void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
291284
{
292285
AssertLockHeld(cs_wallet);
@@ -297,7 +290,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
297290
WalletLogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__);
298291
MarkReserveKeysAsUsed(mi->second);
299292

300-
if (!TopUpKeyPool()) {
293+
if (!TopUp()) {
301294
WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__);
302295
}
303296
}
@@ -401,7 +394,7 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error)
401394
}
402395
// Regenerate the keypool if upgraded to HD
403396
if (hd_upgrade) {
404-
if (!TopUpKeyPool()) {
397+
if (!TopUp()) {
405398
error = _("Unable to generate keys").translated;
406399
return false;
407400
}
@@ -476,18 +469,24 @@ int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const
476469
return nTimeFirstKey;
477470
}
478471

479-
const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(uint160 id) const
472+
const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(const CTxDestination& dest) const
480473
{
481474
AssertLockHeld(cs_wallet);
482-
auto it = mapKeyMetadata.find(CKeyID(id));
483-
if (it != mapKeyMetadata.end()) {
484-
return &it->second;
485-
} else {
486-
auto it2 = m_script_metadata.find(CScriptID(id));
487-
if (it2 != m_script_metadata.end()) {
488-
return &it2->second;
475+
476+
CKeyID key_id = GetKeyForDestination(*this, dest);
477+
if (!key_id.IsNull()) {
478+
auto it = mapKeyMetadata.find(key_id);
479+
if (it != mapKeyMetadata.end()) {
480+
return &it->second;
489481
}
490482
}
483+
484+
CScript scriptPubKey = GetScriptForDestination(dest);
485+
auto it = m_script_metadata.find(CScriptID(scriptPubKey));
486+
if (it != m_script_metadata.end()) {
487+
return &it->second;
488+
}
489+
491490
return nullptr;
492491
}
493492

@@ -1023,15 +1022,15 @@ bool LegacyScriptPubKeyMan::NewKeyPool()
10231022

10241023
m_pool_key_to_index.clear();
10251024

1026-
if (!TopUpKeyPool()) {
1025+
if (!TopUp()) {
10271026
return false;
10281027
}
10291028
WalletLogPrintf("LegacyScriptPubKeyMan::NewKeyPool rewrote keypool\n");
10301029
}
10311030
return true;
10321031
}
10331032

1034-
bool LegacyScriptPubKeyMan::TopUpKeyPool(unsigned int kpSize)
1033+
bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize)
10351034
{
10361035
if (!CanGenerateKeys()) {
10371036
return false;
@@ -1148,7 +1147,7 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key
11481147
{
11491148
LOCK(cs_wallet);
11501149

1151-
TopUpKeyPool();
1150+
TopUp();
11521151

11531152
bool fReturningInternal = fRequestedInternal;
11541153
fReturningInternal &= (IsHDEnabled() && m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) || m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);

‎src/wallet/scriptpubkeyman.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,8 @@ class ScriptPubKeyMan
186186

187187
virtual int64_t GetTimeFirstKey() const { return 0; }
188188

189-
virtual const CKeyMetadata* GetMetadata(uint160 id) const { return nullptr; }
189+
//! Return address metadata
190+
virtual const CKeyMetadata* GetMetadata(const CTxDestination& dest) const { return nullptr; }
190191
};
191192

192193
class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider
@@ -302,7 +303,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
302303

303304
int64_t GetTimeFirstKey() const override;
304305

305-
const CKeyMetadata* GetMetadata(uint160 id) const override;
306+
const CKeyMetadata* GetMetadata(const CTxDestination& dest) const override;
306307

307308
bool CanGetAddresses(bool internal = false) override;
308309

@@ -355,7 +356,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
355356

356357
//! Load a keypool entry
357358
void LoadKeyPool(int64_t nIndex, const CKeyPool &keypool) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
358-
bool TopUpKeyPool(unsigned int kpSize = 0);
359359
bool NewKeyPool();
360360
void MarkPreSplitKeys() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
361361

‎src/wallet/wallet.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -572,7 +572,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
572572
// if we are using HD, replace the HD seed with a new one
573573
if (auto spk_man = m_spk_man.get()) {
574574
if (spk_man->IsHDEnabled()) {
575-
spk_man->SetupGeneration(true);
575+
if (!spk_man->SetupGeneration(true)) {
576+
return false;
577+
}
576578
}
577579
}
578580
Lock();

0 commit comments

Comments
 (0)