Skip to content

LegacyScriptPubKeyMan code cleanups #17300

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Oct 31, 2019
Merged

LegacyScriptPubKeyMan code cleanups #17300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
52cf68f
Refactor: Add GetLegacyScriptPubKeyMan helper
ryanofsky Oct 29, 2019
628d11b
Add back mistakenly removed AssertLockHeld
ryanofsky Oct 29, 2019
2632b1f
doc: Clarify WalletStorage / Wallet relation
ryanofsky Oct 29, 2019
4b28a05
Fix misplaced AssertLockHeld
ryanofsky Oct 29, 2019
53fe0b7
Fix missing strFailReason in CreateTransaction
ryanofsky Oct 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 30 additions & 42 deletions src/wallet/rpcdump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,15 @@ 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 @@ -125,10 +134,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 = pwallet->GetLegacyScriptPubKeyMan();
if (!spk_man) {
throw JSONRPCError(RPC_WALLET_ERROR, "This type of wallet does not support this command");
}
LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);

WalletRescanReserver reserver(pwallet);
bool fRescan = true;
Expand Down Expand Up @@ -256,10 +262,7 @@ UniValue importaddress(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 = GetLegacyScriptPubKeyMan(*pwallet);

std::string strLabel;
if (!request.params[1].isNull())
Expand Down Expand Up @@ -462,10 +465,7 @@ UniValue importpubkey(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 = GetLegacyScriptPubKeyMan(*wallet);

std::string strLabel;
if (!request.params[1].isNull())
Expand Down Expand Up @@ -549,10 +549,7 @@ UniValue importwallet(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 = GetLegacyScriptPubKeyMan(*wallet);

if (pwallet->chain().havePruned()) {
// Exit early and print an error.
Expand Down Expand Up @@ -711,10 +708,7 @@ UniValue dumpprivkey(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 = GetLegacyScriptPubKeyMan(*wallet);

auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);
Expand All @@ -726,12 +720,12 @@ UniValue dumpprivkey(const JSONRPCRequest& request)
if (!IsValidDestination(dest)) {
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid Bitcoin address");
}
auto keyid = GetKeyForDestination(*spk_man, dest);
auto keyid = GetKeyForDestination(spk_man, dest);
if (keyid.IsNull()) {
throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to a key");
}
CKey vchSecret;
if (!spk_man->GetKey(keyid, vchSecret)) {
if (!spk_man.GetKey(keyid, vchSecret)) {
throw JSONRPCError(RPC_WALLET_ERROR, "Private key for address " + strAddress + " is not known");
}
return EncodeSecret(vchSecret);
Expand Down Expand Up @@ -765,14 +759,11 @@ UniValue dumpwallet(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 = GetLegacyScriptPubKeyMan(*wallet);

auto locked_chain = pwallet->chain().lock();
LOCK(pwallet->cs_wallet);
AssertLockHeld(spk_man->cs_wallet);
AssertLockHeld(spk_man.cs_wallet);

EnsureWalletIsUnlocked(pwallet);

Expand All @@ -794,10 +785,10 @@ UniValue dumpwallet(const JSONRPCRequest& request)
throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot open wallet dump file");

std::map<CKeyID, int64_t> mapKeyBirth;
const std::map<CKeyID, int64_t>& mapKeyPool = spk_man->GetAllReserveKeys();
const std::map<CKeyID, int64_t>& mapKeyPool = spk_man.GetAllReserveKeys();
pwallet->GetKeyBirthTimes(*locked_chain, mapKeyBirth);

std::set<CScriptID> scripts = spk_man->GetCScripts();
std::set<CScriptID> scripts = spk_man.GetCScripts();

// sort time/key pairs
std::vector<std::pair<int64_t, CKeyID> > vKeyBirth;
Expand All @@ -816,11 +807,11 @@ UniValue dumpwallet(const JSONRPCRequest& request)
file << "\n";

// add the base58check encoded extended master if the wallet uses HD
CKeyID seed_id = spk_man->GetHDChain().seed_id;
CKeyID seed_id = spk_man.GetHDChain().seed_id;
if (!seed_id.IsNull())
{
CKey seed;
if (spk_man->GetKey(seed_id, seed)) {
if (spk_man.GetKey(seed_id, seed)) {
CExtKey masterKey;
masterKey.SetSeed(seed.begin(), seed.size());

Expand All @@ -833,20 +824,20 @@ UniValue dumpwallet(const JSONRPCRequest& request)
std::string strAddr;
std::string strLabel;
CKey key;
if (spk_man->GetKey(keyid, key)) {
if (spk_man.GetKey(keyid, key)) {
file << strprintf("%s %s ", EncodeSecret(key), strTime);
if (GetWalletAddressesForKey(spk_man, pwallet, keyid, strAddr, strLabel)) {
if (GetWalletAddressesForKey(&spk_man, pwallet, keyid, strAddr, strLabel)) {
file << strprintf("label=%s", strLabel);
} else if (keyid == seed_id) {
file << "hdseed=1";
} else if (mapKeyPool.count(keyid)) {
file << "reserve=1";
} else if (spk_man->mapKeyMetadata[keyid].hdKeypath == "s") {
} else if (spk_man.mapKeyMetadata[keyid].hdKeypath == "s") {
file << "inactivehdseed=1";
} else {
file << "change=1";
}
file << strprintf(" # addr=%s%s\n", strAddr, (spk_man->mapKeyMetadata[keyid].has_key_origin ? " hdkeypath="+WriteHDKeypath(spk_man->mapKeyMetadata[keyid].key_origin.path) : ""));
file << strprintf(" # addr=%s%s\n", strAddr, (spk_man.mapKeyMetadata[keyid].has_key_origin ? " hdkeypath="+WriteHDKeypath(spk_man.mapKeyMetadata[keyid].key_origin.path) : ""));
}
}
file << "\n";
Expand All @@ -855,11 +846,11 @@ UniValue dumpwallet(const JSONRPCRequest& request)
std::string create_time = "0";
std::string address = EncodeDestination(ScriptHash(scriptid));
// get birth times for scripts with metadata
auto it = spk_man->m_script_metadata.find(scriptid);
if (it != spk_man->m_script_metadata.end()) {
auto it = spk_man.m_script_metadata.find(scriptid);
if (it != spk_man.m_script_metadata.end()) {
create_time = FormatISO8601DateTime(it->second.nCreateTime);
}
if(spk_man->GetCScript(scriptid, script)) {
if(spk_man.GetCScript(scriptid, script)) {
file << strprintf("%s %s script=1", HexStr(script.begin(), script.end()), create_time);
file << strprintf(" # addr=%s\n", address);
}
Expand Down Expand Up @@ -1355,10 +1346,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)

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

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 = GetLegacyScriptPubKeyMan(*wallet);

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

Expand Down
2 changes: 2 additions & 0 deletions src/wallet/scriptpubkeyman.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,8 @@ bool LegacyScriptPubKeyMan::AddKeyPubKey(const CKey& secret, const CPubKey &pubk

bool LegacyScriptPubKeyMan::AddKeyPubKeyWithDB(WalletBatch& batch, const CKey& secret, const CPubKey& pubkey)
{
AssertLockHeld(cs_wallet);

// Make sure we aren't adding private keys to private key disabled wallets
assert(!m_storage.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS));

Expand Down
2 changes: 1 addition & 1 deletion src/wallet/scriptpubkeyman.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ enum class OutputType;
// It provides access to things that are part of the entire wallet and not specific to a ScriptPubKeyMan such as
// wallet flags, wallet version, encryption keys, encryption status, and the database itself. This allows a
// ScriptPubKeyMan to have callbacks into CWallet without causing a circular dependency.
// WalletStorage should be the same for all ScriptPubKeyMans.
// WalletStorage should be the same for all ScriptPubKeyMans of a wallet.
class WalletStorage
{
public:
Expand Down
8 changes: 2 additions & 6 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,8 +249,8 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const

void CWallet::UpgradeKeyMetadata()
{
AssertLockHeld(m_spk_man->cs_wallet);
if (m_spk_man) {
AssertLockHeld(m_spk_man->cs_wallet);
m_spk_man->UpgradeKeyMetadata();
}
}
Expand Down Expand Up @@ -2783,11 +2783,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
SignatureData sigdata;

const SigningProvider* provider = GetSigningProvider();
if (!provider) {
return false;
}

if (!ProduceSignature(*provider, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata))
if (!provider || !ProduceSignature(*provider, MutableTransactionSignatureCreator(&txNew, nIn, coin.txout.nValue, SIGHASH_ALL), scriptPubKey, sigdata))
{
strFailReason = _("Signing transaction failed").translated;
return false;
Expand Down