Moves several more key management and metadata functions into LegacyScriptPubKeyMan from CWallet to further separate the two.
Note to reviewers: All of the if (auto spk_man = walletInstance->m_spk_man.get()) { blocks will be replaced with for loops in the next PR so you may see some things in those blocks that don’t necessarily make sense with an if but will with a for.
319@@ -320,8 +320,7 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
320 }
321 }
322 }
323- batch.reset(); //write before setting the flag
324- m_storage.SetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA);
325+ batch.reset();
In commit “Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata” (a5ca4664e74201a08cef1d147ed4446fd74784f8)
Could drop this reset line since batch is going out of scope anyway.
319@@ -320,8 +320,7 @@ void LegacyScriptPubKeyMan::UpgradeKeyMetadata()
320 }
321 }
322 }
323- batch.reset(); //write before setting the flag
324- m_storage.SetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA);
In commit “Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKeyMetadata” (a5ca4664e74201a08cef1d147ed4446fd74784f8):
Would it be possible to drop the WalletStorage SetWalletFlag method entirely after this? I don’t see another call even in #17261
873@@ -874,7 +874,8 @@ void LegacyScriptPubKeyMan::SetHDSeed(const CPubKey& seed)
874 newHdChain.seed_id = seed.GetID();
875 SetHDChain(newHdChain, false);
876 NotifyCanGetAddressesChanged();
877- m_wallet.UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET);
878+ WalletBatch batch(m_storage.GetDatabase());
879+ m_storage.UnsetWalletFlagWithDB(batch, WALLET_FLAG_BLANK_WALLET);
In commit “Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::SetHDSeed” (d35a98b6e04a3750c48bf21af64d0375fe2ae395)
All the UnsetWalletFlagWithDB calls are just unsetting WALLET_FLAG_BLANK_WALLET. Maybe WalletStorage should just have a specific UnsetBlankWalletFlag method and not expose more general flag set/unset methods. With multiple key managers per wallet, it might be safer if individual key managers don’t have ability to arbitrarily set flags shared with other key managers.
1400@@ -1401,9 +1401,17 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
1401 return false;
1402 }
1403 AssertLockHeld(spk_man->cs_wallet);
1404- if (!spk_man->ImportScriptPubKeys(label, script_pub_keys, have_solving_data, apply_label, timestamp)) {
1405+ if (!spk_man->ImportScriptPubKeys(script_pub_keys, have_solving_data, timestamp)) {
1406 return false;
1407 }
1408+ WalletBatch batch(*database);
In commit “Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan::ImportScriptPubKeys” (974d7749f1d9e55a01a2e30db2bdf6b89c18baff)
Could skip this whole block of code if apply_label is false
3774- if (!meta) {
3775- auto it = pwallet->m_script_metadata.find(CScriptID(scriptPubKey));
3776- if (it != pwallet->m_script_metadata.end()) {
3777- meta = &it->second;
3778+ if (!meta) {
3779+ meta = spk_man->GetMetadata(CScriptID(scriptPubKey));
In commit “Refactor: Move GetMetadata code out of getaddressinfo” (bdb538d2fa8b171f1a96580a0f0a7395d52b336e)
It doesn’t seem strictly true that this commit doesn’t change behavior. Before the code was looking up key_id only in mapKeyMetadata and CScriptID(scriptPubKey) only in m_script_metadata. Now it is looking up both values in both maps, which I guess is fine, but maybe more confusing and less efficient.
I think it’d probably be clearer and more type-safe to have separate GetKeyMetadata and GetScriptMetadata methods.
GetMetadata that takes a scriptPubKey, but it fit better to use uint160 for right now.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
286@@ -287,6 +287,23 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int size)
287 return TopUpKeyPool(size);
288 }
289
290+void LegacyScriptPubKeyMan::MarkUnusedAddresses(const CScript& script)
291+{
292+ AssertLockHeld(cs_wallet);
In commit “Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe” (8c47cb9202e27a86609d7b3af91dee1f09468125)
Note for other reviews: lock assert added here is needed to satisfy clang lock annotations and be able to call MarkReserveKeysAsUsed below. This line will be replaced by LOCK(cs_KeyStore); later in commit “Locking: Lock cs_KeyStore instead of cs_main in legacy keyman” 87cc2282b278d3891fc59823d883eef112606da9 from #17261
295+ std::map<CKeyID, int64_t>::const_iterator mi = m_pool_key_to_index.find(keyid);
296+ if (mi != m_pool_key_to_index.end()) {
297+ WalletLogPrintf("%s: Detected a used keypool key, mark all keypool key up to this key as used\n", __func__);
298+ MarkReserveKeysAsUsed(mi->second);
299+
300+ if (!TopUpKeyPool()) {
In commit “Refactor: Move MarkUnusedAddresses code out of CWallet::AddToWalletIfInvolvingMe” (8c47cb9202e27a86609d7b3af91dee1f09468125)
Note for other reviewers: The LegacyScriptPubKeyMan::TopUp call in the previous code is changed to a LegacyScriptPubKeyMan::TopUpKeyPool call here. The two methods will do the exact same thing even after all the other changes in #17261, so there is no difference in behavior.
It seems like the pattern is just to always call TopUp() externally and always call TopUpKeyPool() internally, but it’s not clear why the two methods are separate and I think it probably would clearer to combine them.
TopUpKeyPool() and Topup(). In the native descriptor wallet PR there’s an internal call to Topup() inside DescriptorScriptPubKeyMan::GetNewDestination and DescriptorScriptPubKeyMan::MarkUnusedAddresses: https://github.com/bitcoin/bitcoin/pull/16528/files#diff-5462ceb8a760a507152ab8b76bd48774R1459
LegacyScriptPubKeyMan, TopUp() and TopUpKeyPool() are the same. Since TopUpKeyPool was already being used throughout existing CWallet code that was moved into LegacyScriptPubKeyMan, I decided to let those calls just keep using TopUpKeyPool. But for the ScriptPubKeyMan interface itself, I wanted it to be named TopUp because that conceptually made more sense than calling it TopUpKeyPool.
281+}
282+
283+void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey)
284+{
285+ ReturnKey(index, internal, pubkey);
286+}
In commit “Refactor: Add new ScriptPubKeyMan virtual methods” (80a44800f35c3183edd8fc0914ed8edbd5c5887b)
Note for other reviewers: after future changes in #17261 there are some differences in functionality between the Reserve/Keep/Return destination methods and the Reserve/Keep/Return key methods, and they are called from different places, so there’s some justification for keeping them separate, though I think ideally we could follow up by combining them, or at least making clear what the differences in behavior are supposed to be between the two sets of functions.
361@@ -362,6 +362,41 @@ bool LegacyScriptPubKeyMan::CanGetAddresses(bool internal)
362 return keypool_has_keys;
363 }
364
365+bool LegacyScriptPubKeyMan::Upgrade(int prev_version, std::string& error)
366+{
367+ AssertLockHeld(cs_wallet);
In commit “Refactor: Move Upgrade code out of CWallet::CreateWalletFromFile” (3814d7550ca2d53a9aa1613d61f79dc72b95770b)
I think this added lock assert line is not actually necessary and could be dropped. Clang compiler seems fine without out it as of this commit.
re: #17304 (review)
Removed
The lock assert probably needs to be added back. I’m not sure why clang for me wasn’t showing the same error, but clang on travis legitimately errors that it needs the assert to call `MarkPreSplitKeys():
0wallet/scriptpubkeyman.cpp:399:9: error: calling function 'MarkPreSplitKeys' requires holding mutex 'cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
1 MarkPreSplitKeys();
3799+ int64_t time_first_key = std::numeric_limits<int64_t>::max();
3800+ if (auto spk_man = walletInstance->m_spk_man.get()) {
3801+ time_first_key = std::min(time_first_key, spk_man->GetTimeFirstKey());
3802+ }
3803+ if (time_first_key) {
3804+ if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(time_first_key - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
In commit “Refactor: Move nTimeFirstKey accesses out of CWallet” (e1b1702449a9172185d4bfb5e9043748328132ee)
I think the numeric_limits approach is not the clearest here, because if wallet doesn’t have any key managers, the code winds up calling findFirstBlock with an strange 0x7fffffffffffffff - TIMESTAMP_WINDOW value.
Better I think to use optional instead:
0Optional<int64_t> first_time;
1if (auto spk_man = walletInstance->m_spk_man.get()) {
2 int64_t time = spk_man->GetTimeFirstKey();
3 if (!first_time || time < *first_time) first_time = time;
4}
5if (time_first_key) {
6 if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(*first_time - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
7 rescan_height = *first_block;
8 }
9}
3629- CPubKey seed = walletInstance->m_spk_man->GenerateNewSeed();
3630- walletInstance->m_spk_man->SetHDSeed(seed);
3631- }
3632-
3633- // Top up the keypool
3634- if (walletInstance->m_spk_man->CanGenerateKeys() && !walletInstance->m_spk_man->TopUp()) {
In commit “Refactor: Move SetupGeneration code out of CWallet” (f95750d52a21a69ad55db8422f10c563fb9e08ac)
Note for other reviews: Previous code was calling TopUp, while new code is calling SetupGeneration which calls NewKeyPool which calls TopUpKeyPool. I think the effect is basically the same because fFirstRun is true here so the additional ErasePool calls in NewKeyPool should have no effect.
209@@ -210,9 +210,11 @@ WalletCreationStatus CreateWallet(interfaces::Chain& chain, const SecureString&
210 }
211
212 // Set a seed for the wallet
213- CPubKey master_pub_key = wallet->m_spk_man->GenerateNewSeed();
214- wallet->m_spk_man->SetHDSeed(master_pub_key);
215- wallet->m_spk_man->NewKeyPool();
216+ {
217+ if (auto spk_man = wallet->m_spk_man.get()) {
218+ spk_man->SetupGeneration();
In commit “Refactor: Move SetupGeneration code out of CWallet” (f95750d52a21a69ad55db8422f10c563fb9e08ac)
Should this be checking the return code from SetupGeneration and setting an error if it fails? Old code wasn’t checking the return value from NewKeyPool either, but that doesn’t seem right.
Not sure what your preference is @achow101, but it might be a good idea to replace #17261 with #17304 on the high priority review list https://github.com/bitcoin/bitcoin/projects/8, since #17261 builds on #17304.
This PR is also just making small and mostly obvious changes, so it should be more approachable for reviewers.
mac build errors
0Makefile:8720: recipe for target 'wallet/libbitcoin_wallet_a-scriptpubkeyman.o' failed
1wallet/scriptpubkeyman.cpp:399:9: error: calling function 'MarkPreSplitKeys' requires holding mutex 'cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
2 MarkPreSplitKeys();
3 ^
264@@ -265,6 +265,31 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn)
265 return true;
266 }
267
268+bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool)
269+{
270+ {
A lock is going to be added in the next step
The scope isn’t needed even with the lock and all other changes from #17261:
But I don’t think it’s a big deal. Could clean this up later if reservekey and reservedestination methods are merged later (https://github.com/bitcoin/bitcoin/pull/17304#discussion_r340311958)
3803@@ -3804,8 +3804,13 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
3804
3805 // No need to read and scan block if block was created before
3806 // our wallet birthday (as adjusted for block time variability)
3807- if (walletInstance->nTimeFirstKey) {
3808- if (Optional<int> first_block = locked_chain->findFirstBlockWithTimeAndHeight(walletInstance->nTimeFirstKey - TIMESTAMP_WINDOW, rescan_height, nullptr)) {
3809+ Optional<int64_t> time_first_key;
3810+ if (auto spk_man = walletInstance->m_spk_man.get()) {
3811+ int64_t time = spk_man->GetTimeFirstKey();
3812+ if (!time_first_key || time < *time_first_key) time_first_key = time;
ACK with non-blocking nits
“Refactor: Move GetMetadata code out of getaddressinfo” more accurately: “Refactor: Remove direct accesses to key metadata code from getaddressinfo”
same type of message nit for “Refactor: Move HavePrivateKeys code out of CWallet::CreateWalletFromFile” and “Refactor: Move SetupGeneration code out of CWallet”
565@@ -561,11 +566,11 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
566 Unlock(strWalletPassphrase);
567
568 // if we are using HD, replace the HD seed with a new one
569- if (m_spk_man->IsHDEnabled()) {
570- m_spk_man->SetHDSeed(m_spk_man->GenerateNewSeed());
571+ if (auto spk_man = m_spk_man.get()) {
572+ if (spk_man->IsHDEnabled()) {
573+ spk_man->SetupGeneration(true);
In commit “Refactor: Move SetupGeneration code out of CWallet” (ef97f0b5451e4959c0d0bf649a71eec9b226210e)
No need to change now, but it seems like it’d probably be a good idea to check for an error from SetupGeneration here as well.
295@@ -282,6 +296,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
296 //! Fetches a pubkey from mapWatchKeys if it exists there
297 bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const;
298
299+ /* SigningProvider overrides */
8bdac65e4c705f9f55c93b43792d036229cc9d5a
Sorry for nit picking here, but is this really necessary? What should be the rule of thumb?
3044 auto spk_man = m_spk_man.get();
3045 if (spk_man) {
3046- result = spk_man->GetNewDestination(type, label, dest, error);
3047+ result = spk_man->GetNewDestination(type, dest, error);
3048+ }
3049+ if (result) {
bf5f6c5214706ef61f80c46063061a624c3d505b
This should be in the above if?
No, just that it looks like a leftover where before result could be changed in between.
Actually I think this would look better without the result variable.
You can resolve this.
252@@ -253,6 +253,7 @@ void CWallet::UpgradeKeyMetadata()
253 if (m_spk_man) {
254 m_spk_man->UpgradeKeyMetadata();
255 }
256+ SetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA);
2590266c193d27c979ad6287d72555ca3df630bc
What if IsLocked()?
re: #17304 (review)
What if
IsLocked()?
Nice catch! This seems like a bug. I think either the m_storage.IsLocked() || m_storage.IsWalletFlagSet(WALLET_FLAG_KEY_ORIGIN_METADATA) check should move from LegacyScriptPubKeyMan::UpgradeKeyMetadata to
CWallet::UpgradeKeyMetadata, or preferably, theSetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA) call should move back in the other direction.
It’s also unclear why UpgradeKeyMetadata shoud be a virtual method instead of being specific to the LegacyScriptPubKeyMan class. It doesn’t seem like non-legacy keyman types should implement this upgrade function or be accessing this wallet flag.
CWallet::UpgradeKeyMetadata() and ScriptPubKeyMan::::UpgradeKeyMetadata() methods could both be dropped and walletdb.cpp could call pwallet->GetLegacyScriptPubKeyMan() and LegacyScriptPubKeyMan::UpgradeKeyMetadata directly for compatibility.
1405@@ -1406,9 +1406,19 @@ bool CWallet::ImportScriptPubKeys(const std::string& label, const std::set<CScri
1406 return false;
1407 }
1408 AssertLockHeld(spk_man->cs_wallet);
1409- if (!spk_man->ImportScriptPubKeys(label, script_pub_keys, have_solving_data, apply_label, timestamp)) {
1410+ if (!spk_man->ImportScriptPubKeys(script_pub_keys, have_solving_data, timestamp)) {
1411 return false;
1412 }
1413+ if (apply_label) {
1414+ WalletBatch batch(*database);
26fef3a525fd98f8205fd2f859f476fbf2cc6fcd
Just noting that there’s actually a behavior change here - now if LegacyScriptPubKeyMan::ImportScriptPubKeys fails no label is applied.
Also, another change, which I think is harmless, is that now 2 batches are always used - easily “reverted” by passing the batch to LegacyScriptPubKeyMan::ImportScriptPubKeys from this method.
ImportScriptPubKeys only fails when writing to the database fails, so you’ll have bigger problems if that happens.
Can verify move-only with:
git log -p -n1 --color-moved
This commit is move-only and doesn't change code or affect behavior.
This commit does not change behavior.
This commit does not change behavior.
This commit does not change behavior.
This commit does not change behavior.
SetWalletFlag is unused.
Does not change any behavior
This commit does not change behavior.
ScriptPubKeyMan is only using UnsetWalletFlagWithDB to unset the blank
wallet flag. Just make that it's own function and not expose the flag
writing directly.
This does not change behavior.
This commit does not change behavior.
This commit does not change behavior.
Easier to review ignoring whitespace:
git log -p -n1 -w
This commit does not change behavior.
This commit does not change behavior.
This commit does not change behavior.
This commit does not change behavior.
This commit does not change behavior.
This commit does not change behavior.
This commit does not change behavior.
This commit does not change behavior.
re-ACK 152b0a00d8e681dd098f6b548447b82ab54ebe3c
If you can append a commit to get rid of unused variable warnings in rpcdump.cpp that would be nice though (see #17338 by @jonatack).
0 wallet/rpcdump.cpp:137:28: error: unused variable 'spk_man' [-Werror,-Wunused-variable]
1 LegacyScriptPubKeyMan& spk_man = GetLegacyScriptPubKeyMan(*wallet);