Skip to content

Commit

Permalink
Merge bitcoin#17381: LegacyScriptPubKeyMan code cleanups
Browse files Browse the repository at this point in the history 
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 bitcoin#17300 and bitcoin#17304 review comments

ACKs for top commit:
  Sjors:
    re-ACK 05b224a
  laanwj:
    Code review ACK 05b224a

Tree-SHA512: 12fd86637088515b744c028e0501c5d21a9cf9ee9c9cfd70e9cb65d44611ea5643abd5f6f101105caa5aff015d74de606f074f08af7dae8429f929d21288ab45
  • Loading branch information
@laanwj
laanwj authored and sidhujag committed Nov 7, 2019
1 parent 471e5a2 commit 682a477
Show file tree
Hide file tree
Showing 6 changed files with 56 additions and 67 deletions.
23 changes: 7 additions & 16 deletions src/wallet/rpcdump.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,6 @@ static void RescanWallet(CWallet& wallet, const WalletRescanReserver& reserver,
}
}

static LegacyScriptPubKeyMan& GetLegacyScriptPubKeyMan(CWallet& wallet)
{
LegacyScriptPubKeyMan* spk_man = wallet.GetLegacyScriptPubKeyMan();
if (!spk_man) {
throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
}
return *spk_man;
}

UniValue importprivkey(const JSONRPCRequest& request)
{
std::shared_ptr<CWallet> const wallet = GetWalletForJSONRPCRequest(request);
Expand Down Expand Up @@ -134,7 +125,7 @@ UniValue importprivkey(const JSONRPCRequest& request)
throw JSONRPCError(RPC_WALLET_ERROR, "Cannot import private keys to a wallet with private keys disabled");
}

LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
EnsureLegacyScriptPubKeyMan(*wallet);

WalletRescanReserver reserver(pwallet);
bool fRescan = true;
Expand Down Expand Up @@ -262,7 +253,7 @@ UniValue importaddress(const JSONRPCRequest& request)
},
}.Check(request);

LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*pwallet);
EnsureLegacyScriptPubKeyMan(*pwallet);

std::string strLabel;
if (!request.params[1].isNull())
Expand Down Expand Up @@ -465,7 +456,7 @@ UniValue importpubkey(const JSONRPCRequest& request)
},
}.Check(request);

LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
EnsureLegacyScriptPubKeyMan(*wallet);

std::string strLabel;
if (!request.params[1].isNull())
Expand Down Expand Up @@ -549,7 +540,7 @@ UniValue importwallet(const JSONRPCRequest& request)
},
}.Check(request);

LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
EnsureLegacyScriptPubKeyMan(*wallet);

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

LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet);

auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);
Expand Down Expand Up @@ -759,7 +750,7 @@ UniValue dumpwallet(const JSONRPCRequest& request)
},
}.Check(request);

LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
LegacyScriptPubKeyMan& spk_man = EnsureLegacyScriptPubKeyMan(*wallet);

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

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

LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);
EnsureLegacyScriptPubKeyMan(*wallet);

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

Expand Down
43 changes: 19 additions & 24 deletions src/wallet/rpcwallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,15 @@ void EnsureWalletIsUnlocked(const CWallet* pwallet)
}
}

LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet)
{
LegacyScriptPubKeyMan* spk_man = wallet.GetLegacyScriptPubKeyMan();
if (!spk_man) {
throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
}
return *spk_man;
}

static void WalletTxToJSON(interfaces::Chain& chain, interfaces::Chain::Lock& locked_chain, const CWalletTx& wtx, UniValue& entry)
{
int confirms = wtx.GetDepthInMainChain(locked_chain);
Expand Down Expand Up @@ -972,10 +981,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request)
},
}.Check(request);

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

auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);
Expand All @@ -993,7 +999,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request)
if (IsHex(keys_or_addrs[i].get_str()) && (keys_or_addrs[i].get_str().length() == 66 || keys_or_addrs[i].get_str().length() == 130)) {
pubkeys.push_back(HexToPubKey(keys_or_addrs[i].get_str()));
} else {
pubkeys.push_back(AddrToPubKey(spk_man, keys_or_addrs[i].get_str()));
pubkeys.push_back(AddrToPubKey(&spk_man, keys_or_addrs[i].get_str()));
}
}

Expand All @@ -1006,7 +1012,7 @@ static UniValue addmultisigaddress(const JSONRPCRequest& request)

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

UniValue result(UniValue::VOBJ);
Expand Down Expand Up @@ -3879,15 +3885,7 @@ UniValue getaddressinfo(const JSONRPCRequest& request)

ScriptPubKeyMan* spk_man = pwallet->GetScriptPubKeyMan();
if (spk_man) {
CKeyID key_id = GetKeyForDestination(*provider, dest);
const CKeyMetadata* meta = nullptr;
if (!key_id.IsNull()) {
meta = spk_man->GetMetadata(key_id);
}
if (!meta) {
meta = spk_man->GetMetadata(CScriptID(scriptPubKey));
}
if (meta) {
if (const CKeyMetadata* meta = spk_man->GetMetadata(dest)) {
ret.pushKV("timestamp", meta->nCreateTime);
if (meta->has_key_origin) {
ret.pushKV("hdkeypath", WriteHDKeypath(meta->key_origin.path));
Expand Down Expand Up @@ -4053,10 +4051,7 @@ UniValue sethdseed(const JSONRPCRequest& request)
},
}.Check(request);

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

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

CPubKey master_pub_key;
if (request.params[1].isNull()) {
master_pub_key = spk_man->GenerateNewSeed();
master_pub_key = spk_man.GenerateNewSeed();
} else {
CKey key = DecodeSecret(request.params[1].get_str());
if (!key.IsValid()) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key");
}

if (HaveKey(*spk_man, key)) {
if (HaveKey(spk_man, key)) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Already have this key (either as an HD seed or as a loose private key)");
}

master_pub_key = spk_man->DeriveNewSeed(key);
master_pub_key = spk_man.DeriveNewSeed(key);
}

spk_man->SetHDSeed(master_pub_key);
if (flush_key_pool) spk_man->NewKeyPool();
spk_man.SetHDSeed(master_pub_key);
if (flush_key_pool) spk_man.NewKeyPool();

return NullUniValue;
}
Expand Down
2 changes: 2 additions & 0 deletions src/wallet/rpcwallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
class CRPCTable;
class CWallet;
class JSONRPCRequest;
class LegacyScriptPubKeyMan;
class UniValue;
struct PartiallySignedTransaction;
class CTransaction;
Expand Down Expand Up @@ -42,6 +43,7 @@ std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& reques
std::string HelpRequiringPassphrase(const CWallet*);
void EnsureWalletIsUnlocked(const CWallet*);
bool EnsureWalletIsAvailable(const CWallet*, bool avoidException);
LegacyScriptPubKeyMan& EnsureLegacyScriptPubKeyMan(CWallet& wallet);

UniValue getaddressinfo(const JSONRPCRequest& request);
UniValue signrawtransactionwithwallet(const JSONRPCRequest& request);
Expand Down
45 changes: 22 additions & 23 deletions src/wallet/scriptpubkeyman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
bool LegacyScriptPubKeyMan::GetNewDestination(const OutputType type, CTxDestination& dest, std::string& error)
{
error.clear();
TopUpKeyPool();
TopUp();

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

bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool)
{
{
if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
return false;
}
if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
return false;
}
return true;
}
Expand All @@ -282,11 +280,6 @@ void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, cons
ReturnKey(index, internal, pubkey);
}

bool LegacyScriptPubKeyMan::TopUp(unsigned int size)
{
return TopUpKeyPool(size);
}

void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
{
AssertLockHeld(cs_wallet);
Expand All @@ -297,7 +290,7 @@ void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
WalletLogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__);
MarkReserveKeysAsUsed(mi->second);

if (!TopUpKeyPool()) {
if (!TopUp()) {
WalletLogPrintf("%s: Topping up keypool failed (locked wallet)\n", __func__);
}
}
Expand Down Expand Up @@ -401,7 +394,7 @@ bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error)
}
// Regenerate the keypool if upgraded to HD
if (hd_upgrade) {
if (!TopUpKeyPool()) {
if (!TopUp()) {
error = _("Unable to generate keys").translated;
return false;
}
Expand Down Expand Up @@ -476,18 +469,24 @@ int64_t LegacyScriptPubKeyMan::GetTimeFirstKey() const
return nTimeFirstKey;
}

const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(uint160 id) const
const CKeyMetadata* LegacyScriptPubKeyMan::GetMetadata(const CTxDestination& dest) const
{
AssertLockHeld(cs_wallet);
auto it = mapKeyMetadata.find(CKeyID(id));
if (it != mapKeyMetadata.end()) {
return &it->second;
} else {
auto it2 = m_script_metadata.find(CScriptID(id));
if (it2 != m_script_metadata.end()) {
return &it2->second;

CKeyID key_id = GetKeyForDestination(*this, dest);
if (!key_id.IsNull()) {
auto it = mapKeyMetadata.find(key_id);
if (it != mapKeyMetadata.end()) {
return &it->second;
}
}

CScript scriptPubKey = GetScriptForDestination(dest);
auto it = m_script_metadata.find(CScriptID(scriptPubKey));
if (it != m_script_metadata.end()) {
return &it->second;
}

return nullptr;
}

Expand Down Expand Up @@ -1023,15 +1022,15 @@ bool LegacyScriptPubKeyMan::NewKeyPool()

m_pool_key_to_index.clear();

if (!TopUpKeyPool()) {
if (!TopUp()) {
return false;
}
WalletLogPrintf("LegacyScriptPubKeyMan::NewKeyPool rewrote keypool\n");
}
return true;
}

bool LegacyScriptPubKeyMan::TopUpKeyPool(unsigned int kpSize)
bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize)
{
if (!CanGenerateKeys()) {
return false;
Expand Down Expand Up @@ -1148,7 +1147,7 @@ bool LegacyScriptPubKeyMan::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& key
{
LOCK(cs_wallet);

TopUpKeyPool();
TopUp();

bool fReturningInternal = fRequestedInternal;
fReturningInternal &= (IsHDEnabled() && m_storage.CanSupportFeature(FEATURE_HD_SPLIT)) || m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
Expand Down
6 changes: 3 additions & 3 deletions src/wallet/scriptpubkeyman.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@ class ScriptPubKeyMan

virtual int64_t GetTimeFirstKey() const { return 0; }

virtual const CKeyMetadata* GetMetadata(uint160 id) const { return nullptr; }
//! Return address metadata
virtual const CKeyMetadata* GetMetadata(const CTxDestination& dest) const { return nullptr; }
};

class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProvider
Expand Down Expand Up @@ -302,7 +303,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv

int64_t GetTimeFirstKey() const override;

const CKeyMetadata* GetMetadata(uint160 id) const override;
const CKeyMetadata* GetMetadata(const CTxDestination& dest) const override;

bool CanGetAddresses(bool internal = false) override;

Expand Down Expand Up @@ -355,7 +356,6 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv

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

Expand Down
4 changes: 3 additions & 1 deletion src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
// if we are using HD, replace the HD seed with a new one
if (auto spk_man = m_spk_man.get()) {
if (spk_man->IsHDEnabled()) {
spk_man->SetupGeneration(true);
if (!spk_man->SetupGeneration(true)) {
return false;
}
}
}
Lock();
Expand Down

0 comments on commit 682a477

Please sign in to comment.