From 63895fde23781fbeadaf37fdff8f15e4e975226d Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 21 May 2020 23:15:41 -0400 Subject: [PATCH] Merge #19046: Replace CWallet::Set* functions that use memonly with Add/Load variants 3a9aba21a49a6d80bd187940d5e26893937b6832 Split SetWalletFlags into Add/LoadWalletFlags (Andrew Chow) d9cd095b5965fc20c09f401370e7ba99446663e3 Split SetActiveScriptPubKeyMan into Add/LoadActiveScriptPubKeyMan (Andrew Chow) 0122fbab4c340b23ae56173de6c5ab866ba25ab8 Split SetHDChain into AddHDChain and LoadHDChain (Andrew Chow) Pull request description: `SetHDChaiin`, `SetActiveScriptPubKeyMan`, and `SetWalletFlags` have a `memonly` argument which is kind of confusing, as noted in https://github.com/bitcoin/bitcoin/pull/17681#discussion_r427633081. This PR replaces those functions with `Add*` and `Load*` variants so that they follow the pattern used elsewhere in the wallet. `AddHDChain`, `AddActiveScriptPubKeyMan`, and `AddWalletFlags` both set their respective variables in `CWallet` and writes them to disk. These functions are used by the actions which modify the wallet such as `sethdseed`, `importdescriptors`, and creating a new wallet. `LoadHDChain`, `LoadActiveScriptPubKeyMan`, and `LoadWalletFlags` just set the `CWallet` variables. These functions are used by `LoadWallet` when loading the wallet from disk. ACKs for top commit: jnewbery: Code review ACK 3a9aba21a49a6d80bd187940d5e26893937b6832 ryanofsky: Code review ACK 3a9aba21a49a6d80bd187940d5e26893937b6832. Only changes since last review tweaks making m_wallet_flags updates more safe meshcollider: utACK 3a9aba21a49a6d80bd187940d5e26893937b6832 Tree-SHA512: 365aeaafc5ba42879c0eb797ec3beb29ab70e27f917dc880763f743420b3be6ddf797240996beed8a9ad70fb212c2590253c6b44c9dc244529c3939d9538983f --- src/wallet/rpcdump.cpp | 2 +- src/wallet/rpcwallet.cpp | 4 +-- src/wallet/scriptpubkeyman.cpp | 48 +++++++++++++++++----------------- src/wallet/scriptpubkeyman.h | 10 +++---- src/wallet/wallet.cpp | 45 +++++++++++++++++++------------ src/wallet/wallet.h | 15 +++++++---- src/wallet/walletdb.cpp | 8 +++--- 7 files changed, 75 insertions(+), 57 deletions(-) diff --git a/src/wallet/rpcdump.cpp b/src/wallet/rpcdump.cpp index 230a6fb2c7..7fa0c83b47 100644 --- a/src/wallet/rpcdump.cpp +++ b/src/wallet/rpcdump.cpp @@ -1766,7 +1766,7 @@ static UniValue ProcessDescriptorImport(CWallet * const pwallet, const UniValue& if (!w_desc.descriptor->GetOutputType()) { warnings.push_back("Unknown output type, cannot set descriptor to active."); } else { - pwallet->SetActiveScriptPubKeyMan(spk_manager->GetID(), internal); + pwallet->AddActiveScriptPubKeyMan(spk_manager->GetID(), internal); } } diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index caf69ad1b8..8495b7efbf 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -4335,8 +4335,8 @@ static RPCHelpMan sethdseed() if (!newHdChain.SetSeed(SecureVector(key.begin(), key.end()), true)) { throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key: SetSeed failed"); } - if (!spk_man.SetHDChainSingle(newHdChain, false)) { - throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key: SetHDChainSingle failed"); + if (!spk_man.AddHDChainSingle(newHdChain)) { + throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid private key: AddHDChainSingle failed"); } // add default account newHdChain.AddAccount(); diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index bb1cc9d140..d7ec27bf3e 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -268,7 +268,7 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat if (!hdChainCurrent.IsNull()) { assert(EncryptHDChain(master_key, m_hd_chain)); - assert(SetHDChain(m_hd_chain)); + assert(LoadHDChain(m_hd_chain)); CHDChain hdChainCrypted; assert(GetHDChain(hdChainCrypted)); @@ -277,7 +277,7 @@ bool LegacyScriptPubKeyMan::Encrypt(const CKeyingMaterial& master_key, WalletBat assert(hdChainCurrent.GetID() == hdChainCrypted.GetID()); assert(hdChainCurrent.GetSeedHash() != hdChainCrypted.GetSeedHash()); - assert(SetHDChain(*encrypted_batch, hdChainCrypted, false)); + assert(AddHDChain(*encrypted_batch, hdChainCrypted)); } encrypted_batch = nullptr; @@ -396,7 +396,7 @@ void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString& secure CHDChain hdChainPrev = hdChainTmp; bool res = EncryptHDChain(vMasterKey, hdChainTmp); assert(res); - res = SetHDChain(hdChainTmp); + res = LoadHDChain(hdChainTmp); assert(res); CHDChain hdChainCrypted; @@ -407,8 +407,8 @@ void LegacyScriptPubKeyMan::GenerateNewCryptedHDChain(const SecureString& secure assert(hdChainPrev.GetID() == hdChainCrypted.GetID()); assert(hdChainPrev.GetSeedHash() != hdChainCrypted.GetSeedHash()); - if (!SetHDChainSingle(hdChainCrypted, false)) { - throw std::runtime_error(std::string(__func__) + ": SetHDChainSingle failed"); + if (!AddHDChainSingle(hdChainCrypted)) { + throw std::runtime_error(std::string(__func__) + ": AddHDChainSingle failed"); } } @@ -426,8 +426,8 @@ void LegacyScriptPubKeyMan::GenerateNewHDChain(const SecureString& secureMnemoni // add default account newHdChain.AddAccount(); - if (!SetHDChainSingle(newHdChain, false)) { - throw std::runtime_error(std::string(__func__) + ": SetHDChainSingle failed"); + if (!AddHDChainSingle(newHdChain)) { + throw std::runtime_error(std::string(__func__) + ": AddHDChainSingle failed"); } if (!NewKeyPool()) { @@ -435,14 +435,24 @@ void LegacyScriptPubKeyMan::GenerateNewHDChain(const SecureString& secureMnemoni } } -bool LegacyScriptPubKeyMan::SetHDChain(WalletBatch &batch, const CHDChain& chain, bool memonly) +bool LegacyScriptPubKeyMan::LoadHDChain(const CHDChain& chain) { LOCK(cs_KeyStore); - if (!SetHDChain(chain)) + if (m_storage.HasEncryptionKeys() != chain.IsCrypted()) return false; + + m_hd_chain = chain; + return true; +} + +bool LegacyScriptPubKeyMan::AddHDChain(WalletBatch &batch, const CHDChain& chain) +{ + LOCK(cs_KeyStore); + + if (!LoadHDChain(chain)) return false; - if (!memonly) { + { if (chain.IsCrypted() && encrypted_batch) { if (!encrypted_batch->WriteHDChain(chain)) throw std::runtime_error(std::string(__func__) + ": WriteHDChain failed for encrypted batch"); @@ -458,10 +468,10 @@ bool LegacyScriptPubKeyMan::SetHDChain(WalletBatch &batch, const CHDChain& chain return true; } -bool LegacyScriptPubKeyMan::SetHDChainSingle(const CHDChain& chain, bool memonly) +bool LegacyScriptPubKeyMan::AddHDChainSingle(const CHDChain& chain) { WalletBatch batch(m_storage.GetDatabase()); - return SetHDChain(batch, chain, memonly); + return AddHDChain(batch, chain); } bool LegacyScriptPubKeyMan::GetDecryptedHDChain(CHDChain& hdChainRet) @@ -1090,16 +1100,6 @@ bool LegacyScriptPubKeyMan::AddWatchOnly(const CScript& dest, int64_t nCreateTim return AddWatchOnly(dest); } -bool LegacyScriptPubKeyMan::SetHDChain(const CHDChain& chain) -{ - LOCK(cs_KeyStore); - - if (m_storage.HasEncryptionKeys() != chain.IsCrypted()) return false; - - m_hd_chain = chain; - return true; -} - bool LegacyScriptPubKeyMan::HaveHDKey(const CKeyID &address, CHDChain& hdChainCurrent) const { LOCK(cs_KeyStore); @@ -1322,8 +1322,8 @@ void LegacyScriptPubKeyMan::DeriveNewChildKey(WalletBatch &batch, CKeyMetadata& if (!hdChainCurrent.SetAccount(nAccountIndex, acc)) throw std::runtime_error(std::string(__func__) + ": SetAccount failed"); - if (!SetHDChain(batch, hdChainCurrent, false)) { - throw std::runtime_error(std::string(__func__) + ": SetHDChain failed"); + if (!AddHDChain(batch, hdChainCurrent)) { + throw std::runtime_error(std::string(__func__) + ": AddHDChain failed"); } if (!AddHDPubKey(batch, childKey.Neuter(), fInternal)) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index ecbc56dd8b..3351cb3a9f 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -278,12 +278,8 @@ private: /** Add a KeyOriginInfo to the wallet */ bool AddKeyOriginWithDB(WalletBatch& batch, const CPubKey& pubkey, const KeyOriginInfo& info); - /* Set the HD chain model (chain child index counters) */ - bool SetHDChain(WalletBatch &batch, const CHDChain& chain, bool memonly); - bool EncryptHDChain(const CKeyingMaterial& vMasterKeyIn, CHDChain& chain); bool DecryptHDChain(const CKeyingMaterial& vMasterKeyIn, CHDChain& hdChainRet) const; - bool SetHDChain(const CHDChain& chain); /* the HD chain data model (external chain counters) */ CHDChain m_hd_chain GUARDED_BY(cs_KeyStore); @@ -398,11 +394,15 @@ public: //! Generate a new key CPubKey GenerateNewKey(WalletBatch& batch, uint32_t nAccountIndex, bool fInternal /*= false*/) EXCLUSIVE_LOCKS_REQUIRED(cs_KeyStore); + /* Set the HD chain model (chain child index counters) and writes it to the database */ + bool AddHDChain(WalletBatch &batch, const CHDChain& chain); + //! Load a HD chain model (used by LoadWallet) + bool LoadHDChain(const CHDChain& chain); /** * Set the HD chain model (chain child index counters) using temporary wallet db object * which causes db flush every time these methods are used */ - bool SetHDChainSingle(const CHDChain& chain, bool memonly); + bool AddHDChainSingle(const CHDChain& chain); //! Adds a watch-only address to the store, without saving it to disk (used by LoadWallet) bool LoadWatchOnly(const CScript &dest); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e1f2537561..b5575a46a7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1684,19 +1684,28 @@ bool CWallet::IsWalletFlagSet(uint64_t flag) const return (m_wallet_flags & flag); } -bool CWallet::SetWalletFlags(uint64_t overwriteFlags, bool memonly) +bool CWallet::LoadWalletFlags(uint64_t flags) { LOCK(cs_wallet); - m_wallet_flags = overwriteFlags; - if (((overwriteFlags & KNOWN_WALLET_FLAGS) >> 32) ^ (overwriteFlags >> 32)) { + if (((flags & KNOWN_WALLET_FLAGS) >> 32) ^ (flags >> 32)) { // contains unknown non-tolerable wallet flags return false; } - if (!memonly && !WalletBatch(GetDatabase()).WriteWalletFlags(m_wallet_flags)) { + m_wallet_flags = flags; + + return true; +} + +bool CWallet::AddWalletFlags(uint64_t flags) +{ + LOCK(cs_wallet); + // We should never be writing unknown non-tolerable wallet flags + assert(!(((flags & KNOWN_WALLET_FLAGS) >> 32) ^ (flags >> 32))); + if (!WalletBatch(GetDatabase()).WriteWalletFlags(flags)) { throw std::runtime_error(std::string(__func__) + ": writing wallet flags failed"); } - return true; + return LoadWalletFlags(flags); } int64_t CWalletTx::GetTxTime() const @@ -4575,7 +4584,8 @@ std::shared_ptr CWallet::Create(interfaces::Chain& chain, interfaces::C if (fFirstRun) { walletInstance->SetMaxVersion(FEATURE_LATEST); - walletInstance->SetWalletFlags(wallet_creation_flags, false); + + walletInstance->AddWalletFlags(wallet_creation_flags); // Only create LegacyScriptPubKeyMan when not descriptor wallet if (!walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { @@ -4600,8 +4610,8 @@ std::shared_ptr CWallet::Create(interfaces::Chain& chain, interfaces::C } LOCK(walletInstance->cs_wallet); if (auto spk_man = walletInstance->GetLegacyScriptPubKeyMan()) { - if (!spk_man->SetHDChainSingle(newHdChain, false)) { - error = strprintf(_("%s failed"), "SetHDChainSingle"); + if (!spk_man->AddHDChainSingle(newHdChain)) { + error = strprintf(_("%s failed"), "AddHDChainSingle"); return nullptr; } } @@ -5649,12 +5659,21 @@ void CWallet::SetupDescriptorScriptPubKeyMans() spk_manager->SetupDescriptorGeneration(master_key); uint256 id = spk_manager->GetID(); m_spk_managers[id] = std::move(spk_manager); - SetActiveScriptPubKeyMan(id, internal); + AddActiveScriptPubKeyMan(id, internal); } } } -void CWallet::SetActiveScriptPubKeyMan(uint256 id, bool internal, bool memonly) +void CWallet::AddActiveScriptPubKeyMan(uint256 id, bool internal) +{ + WalletBatch batch(GetDatabase()); + if (!batch.WriteActiveScriptPubKeyMan(id, internal)) { + throw std::runtime_error(std::string(__func__) + ": writing active ScriptPubKeyMan id failed"); + } + LoadActiveScriptPubKeyMan(id, internal); +} + +void CWallet::LoadActiveScriptPubKeyMan(uint256 id, bool internal) { WalletLogPrintf("Setting spkMan to active: id = %s, type = %d, internal = %d\n", id.ToString(), static_cast(OutputType::LEGACY), static_cast(internal)); auto& spk_mans = internal ? m_internal_spk_managers : m_external_spk_managers; @@ -5662,12 +5681,6 @@ void CWallet::SetActiveScriptPubKeyMan(uint256 id, bool internal, bool memonly) spk_man->SetInternal(internal); spk_mans = spk_man; - if (!memonly) { - WalletBatch batch(GetDatabase()); - if (!batch.WriteActiveScriptPubKeyMan(id, internal)) { - throw std::runtime_error(std::string(__func__) + ": writing active ScriptPubKeyMan id failed"); - } - } NotifyCanGetAddressesChanged(); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index a0c5618ac5..8889cf364a 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1334,7 +1334,9 @@ public: /** overwrite all flags by the given uint64_t returns false if unknown, non-tolerable flags are present */ - bool SetWalletFlags(uint64_t overwriteFlags, bool memOnly); + bool AddWalletFlags(uint64_t flags); + /** Loads the flags into the wallet. (used by LoadWallet) */ + bool LoadWalletFlags(uint64_t flags); /** Determine if we are a legacy wallet */ bool IsLegacy() const; @@ -1415,12 +1417,15 @@ public: //! Instantiate a descriptor ScriptPubKeyMan from the WalletDescriptor and load it void LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc); - //! Sets the active ScriptPubKeyMan for the specified type and internal + //! Adds the active ScriptPubKeyMan for the specified type and internal. Writes it to the wallet file //! @param[in] id The unique id for the ScriptPubKeyMan - //! @param[in] type The OutputType this ScriptPubKeyMan provides addresses for //! @param[in] internal Whether this ScriptPubKeyMan provides change addresses - //! @param[in] memonly Whether to record this update to the database. Set to true for wallet loading, normally false when actually updating the wallet. - void SetActiveScriptPubKeyMan(uint256 id, bool internal, bool memonly = false); + void AddActiveScriptPubKeyMan(uint256 id, bool internal); + + //! Loads an active ScriptPubKeyMan for the specified type and internal. (used by LoadWallet) + //! @param[in] id The unique id for the ScriptPubKeyMan + //! @param[in] internal Whether this ScriptPubKeyMan provides change addresses + void LoadActiveScriptPubKeyMan(uint256 id, bool internal); //! Create new DescriptorScriptPubKeyMans and add them to the wallet void SetupDescriptorScriptPubKeyMans(); diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 3c9d2394bd..83f24d0828 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -516,7 +516,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, CHDChain chain; ssValue >> chain; assert ((strType == DBKeys::CRYPTED_HDCHAIN) == chain.IsCrypted()); - if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->SetHDChainSingle(chain, true)) + if (!pwallet->GetOrCreateLegacyScriptPubKeyMan()->LoadHDChain(chain)) { strErr = "Error reading wallet database: SetHDChain failed"; return false; @@ -557,7 +557,7 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue, } else if (strType == DBKeys::FLAGS) { uint64_t flags; ssValue >> flags; - if (!pwallet->SetWalletFlags(flags, true)) { + if (!pwallet->LoadWalletFlags(flags)) { strErr = "Error reading wallet database: Unknown non-tolerable wallet flags found"; return false; } @@ -768,10 +768,10 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet) // Set the active ScriptPubKeyMans for (auto spk_man : wss.m_active_external_spks) { - pwallet->SetActiveScriptPubKeyMan(spk_man.second, /* internal */ false, /* memonly */ true); + pwallet->LoadActiveScriptPubKeyMan(spk_man.second, /* internal */ false); } for (auto spk_man : wss.m_active_internal_spks) { - pwallet->SetActiveScriptPubKeyMan(spk_man.second, /* internal */ true, /* memonly */ true); + pwallet->LoadActiveScriptPubKeyMan(spk_man.second, /* internal */ true); } // Set the descriptor caches