refactor: Move many functions into LegacyScriptPubKeyMan and further separate it from CWallet #17304

achow101 · 2019-10-29T19:03:33Z

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.

DrahtBot · 2019-10-29T20:07:47Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17283 (rpc: improve getaddressinfo test coverage, help, code docs by jonatack)
#17237 (wallet: LearnRelatedScripts only if KeepDestination by promag)
#16946 (wallet: include a checksum of encrypted private keys by achow101)
#16910 (wallet: reduce loading time by using unordered maps by achow101)
#16224 (gui: Bilingual GUI error messages by hebasto)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)

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.

ryanofsky

Code review ACK e1b1702. Left comments and suggestions which could be followed up later if we want to avoid losing momentum on #17261

src/wallet/scriptpubkeyman.cpp

src/wallet/wallet.cpp

ryanofsky · 2019-10-29T20:02:42Z

src/wallet/rpcwallet.cpp

-        if (it != pwallet->m_script_metadata.end()) {
-            meta = &it->second;
+        if (!meta) {
+            meta = spk_man->GetMetadata(CScriptID(scriptPubKey));


In commit "Refactor: Move GetMetadata code out of getaddressinfo" (bdb538d)

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.

From the ScriptPubKeyMan design view, it doesn't make sense to have different metadata for keys and scripts, at least exposed publicly to the wallet. It should really be GetMetadata that takes a scriptPubKey, but it fit better to use uint160 for right now.

ryanofsky · 2019-10-29T20:37:41Z

src/wallet/scriptpubkeyman.cpp

+bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool)
+{
+    {
+        if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
+            return false;
+        }
+    }
+    return true;
+}
+
+void LegacyScriptPubKeyMan::KeepDestination(int64_t index)
+{
+    KeepKey(index);
+}
+
+void LegacyScriptPubKeyMan::ReturnDestination(int64_t index, bool internal, const CPubKey& pubkey)
+{
+    ReturnKey(index, internal, pubkey);
+}


In commit "Refactor: Add new ScriptPubKeyMan virtual methods" (80a4480)

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.

src/wallet/scriptpubkeyman.cpp

src/wallet/wallet.cpp

ryanofsky · 2019-10-29T21:07:57Z

src/wallet/wallet.cpp

-        }
-
-        // Top up the keypool
-        if (walletInstance->m_spk_man->CanGenerateKeys() && !walletInstance->m_spk_man->TopUp()) {


In commit "Refactor: Move SetupGeneration code out of CWallet" (f95750d)

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.

src/wallet/wallet.cpp

ryanofsky

Code review ACK 5631c01. Just a bunch of suggested changes made since the last review.

ryanofsky · 2019-10-29T22:37:44Z

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.

achow101 · 2019-10-29T22:58:10Z

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.

Yeah, it should replace that. #17261 was added to the list before I made this pr.

instagibbs · 2019-10-30T15:13:49Z

mac build errors

Makefile:8720: recipe for target 'wallet/libbitcoin_wallet_a-scriptpubkeyman.o' failed
wallet/scriptpubkeyman.cpp:399:9: error: calling function 'MarkPreSplitKeys' requires holding mutex 'cs_wallet' exclusively [-Werror,-Wthread-safety-analysis]
        MarkPreSplitKeys();
        ^

ryanofsky

Code review ACK b04a526. Only change is adding back missing lock assert.

ryanofsky · 2019-10-31T13:28:48Z

This PR has 18 commits, so it could be useful to get some Approach ACKS here on whether it's a reasonable size, or should be made smaller by dropping some number of commits, or dropping particular commits that seem more difficult to review. All of the commits should be small and straightforward, but it is also easily possible to scale back this PR if necessary.

instagibbs

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"

instagibbs · 2019-10-31T15:09:22Z

src/wallet/scriptpubkeyman.cpp

@@ -265,6 +265,31 @@ bool LegacyScriptPubKeyMan::EncryptKeys(CKeyingMaterial& vMasterKeyIn)
    return true;
 }

+bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, int64_t& index, CKeyPool& keypool)
+{
+    {


why the scope here?

A lock is going to be added in the next step

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:

bitcoin/src/wallet/scriptpubkeyman.cpp

Lines 275 to 290 in 35b0f7b

bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool)

{

if (!CanGetAddresses(internal)) {

return false;

}

{

LOCK(cs_KeyStore);

if (!ReserveKeyFromKeyPool(index, keypool, internal)) {

return false;

}

LearnRelatedScripts(keypool.vchPubKey, type);

address = GetDestinationForKey(keypool.vchPubKey, type);

}

return true;

}

But I don't think it's a big deal. Could clean this up later if reservekey and reservedestination methods are merged later (#17304 (comment))

I'll just leave this as is so it can get merged sooner :)

instagibbs · 2019-10-31T15:35:38Z

src/wallet/wallet.cpp

+        Optional<int64_t> time_first_key;
+        if (auto spk_man = walletInstance->m_spk_man.get()) {
+            int64_t time = spk_man->GetTimeFirstKey();
+            if (!time_first_key || time < *time_first_key) time_first_key = time;


I realize this is stopping mid-way, but this was slightly confusing. Add a comment saying this is temporarily going to always be true until a loop is introduced?

That generally applies throughout this PR. So I guess a note to reviewers in the thread should be good enough?

ok! Consider this your note, future reviewers

Sjors · 2019-10-31T17:32:52Z

I'm going through these commits now. I've seen this code a few times before, but if others want to eject one or more commits to get it merged faster, that could make sense.

ryanofsky · 2019-10-31T17:56:29Z

src/wallet/wallet.cpp

-            m_spk_man->SetHDSeed(m_spk_man->GenerateNewSeed());
+        if (auto spk_man = m_spk_man.get()) {
+            if (spk_man->IsHDEnabled()) {
+                spk_man->SetupGeneration(true);


In commit "Refactor: Move SetupGeneration code out of CWallet" (ef97f0b)

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.

Sjors · 2019-10-31T20:11:09Z

Code review ACK up to b04a526.

promag

Light code review ACK b04a526 with some comments.

promag · 2019-11-02T00:42:52Z

src/wallet/scriptpubkeyman.h

@@ -282,6 +296,7 @@ class LegacyScriptPubKeyMan : public ScriptPubKeyMan, public FillableSigningProv
    //! Fetches a pubkey from mapWatchKeys if it exists there
    bool GetWatchPubKey(const CKeyID &address, CPubKey &pubkey_out) const;

+    /* SigningProvider overrides */


8bdac65

Sorry for nit picking here, but is this really necessary? What should be the rule of thumb?

Is what necessary?

The category comment.

promag · 2019-11-02T00:50:03Z

src/wallet/wallet.cpp

-        result = spk_man->GetNewDestination(type, label, dest, error);
+        result = spk_man->GetNewDestination(type, dest, error);
+    }
+    if (result) {


bf5f6c5

This should be in the above if?

Does it really matter?

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.

promag · 2019-11-02T00:55:26Z

src/wallet/wallet.cpp

@@ -253,6 +253,7 @@ void CWallet::UpgradeKeyMetadata()
    if (m_spk_man) {
        m_spk_man->UpgradeKeyMetadata();
    }
+    SetWalletFlag(WALLET_FLAG_KEY_ORIGIN_METADATA);


2590266

What if IsLocked()?

re: #17304 (comment)

2590266

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.

It seems like the CWallet::UpgradeKeyMetadata() and ScriptPubKeyMan::::UpgradeKeyMetadata() methods could both be dropped and walletdb.cpp could call pwallet->GetLegacyScriptPubKeyMan() and LegacyScriptPubKeyMan::UpgradeKeyMetadata directly for compatibility.

Changed to be non-virtual and moved that check out to CWallet.

promag · 2019-11-02T01:06:35Z

src/wallet/wallet.cpp

        return false;
    }
+    if (apply_label) {
+        WalletBatch batch(*database);


26fef3a

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.

Those are harmless. 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.

…ewDestination This commit does not change behavior.

…Metadata This commit does not change behavior.

SetWalletFlag is unused. Does not change any behavior

…HDSeed This commit does not change behavior.

…ScriptPubKeyMan 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.

…::ImportScriptPubKeys This commit does not change behavior.

This commit does not change behavior.

Pass CTxDestination instead of more ambiguous uint160 hash value. This is more type safe and more efficient since it avoids doing map lookups that will always fail and were not done previously before a18edd7 from bitcoin#17304 Change suggested by Andrew Chow <achow101-github@achow101.com> in bitcoin#17304 (comment) and bitcoin#17381 (comment)

Previous discussion bitcoin#17304 (comment)

Suggested bitcoin#17304 (comment) by Gregory Sanders <gsanders87@gmail.com> Reason for keeping the `return true` `return false` verbosity is that more code will be added after the ReserveKeyFromKeyPool() call before returning.

Suggested bitcoin#17304 (comment) by me

Summary: Can verify move-only with: git log -p -n1 --color-moved This commit is move-only and doesn't change code or affect behavior. bitcoin/bitcoin@b4cb18b --- Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7147

… as virtual Summary: This commit does not change behavior. bitcoin/bitcoin@533d8b3 --- Depends on D7147 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7148

Summary: This commit does not change behavior. bitcoin/bitcoin@acedc5b --- Depends on D7148 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7149

…yScriptPubKeyMan::GetNewDestination Summary: This commit does not change behavior. bitcoin/bitcoin@769acef --- Depends on D7149 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7151

…tPubKeyMan::UpgradeKeyMetadata Summary: This commit does not change behavior. bitcoin/bitcoin@4c5491f --- Depends on D7151 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7152

Summary: SetWalletFlag is unused. Does not change any behavior bitcoin/bitcoin@0391aba --- Depends on D7152 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7153

…cyScriptPubKeyMan::SetHDSeed Summary: This commit does not change behavior. bitcoin/bitcoin@78e7cbc --- Depends on D7153 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7154

…setBlankWalletFlag in ScriptPubKeyMan Summary: 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. bitcoin/bitcoin@fc2867f --- Depends on D7154 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7155

… LegacyScriptPubKeyMan::ImportScriptPubKeys Summary: This commit does not change behavior. bitcoin/bitcoin@67be6b9 --- Depends on D7155 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7156

…ethod definition Summary: This commit does not change behavior. bitcoin/bitcoin@9716bbe --- Depends on D7156 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7157

…essinfo Summary: Easier to review ignoring whitespace: git log -p -n1 -w This commit does not change behavior. bitcoin/bitcoin@a18edd7 --- Depends on D7157 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7158

… CWallet::AddToWalletIfInvolvingMe Summary: This commit does not change behavior. bitcoin/bitcoin@46865ec --- Depends on D7158 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7159

…eateWalletFromFile Summary: This commit does not change behavior. bitcoin/bitcoin@8b0d82b --- Depends on D7159 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7160

…llet::CreateWalletFromFile Summary: This commit does not change behavior. bitcoin/bitcoin@f45d12b --- Depends on D7160 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7161

…llet Summary: This commit does not change behavior. bitcoin/bitcoin@0eac708 --- Depends on D7161 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Subscribers: jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7162

Summary: This commit does not change behavior. bitcoin/bitcoin@089e17d --- Depends on D7162 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7163

Summary: This commit does not change behavior. bitcoin/bitcoin@7ef47b8 --- Depends on D7163 Partial backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7164

…Wallet Summary: This commit does not change behavior. bitcoin/bitcoin@152b0a0 --- Depends on D7164 Concludes backport of Core [[bitcoin/bitcoin#17304 | PR17304]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, jasonbcox Reviewed By: #bitcoin_abc, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D7165

Summary: Pass CTxDestination instead of more ambiguous uint160 hash value. This is more type safe and more efficient since it avoids doing map lookups that will always fail and were not done previously before bitcoin/bitcoin@a18edd7 Change suggested by Andrew Chow <achow101-github@achow101.com> in bitcoin/bitcoin#17304 (comment) and bitcoin/bitcoin#17381 (comment) --- bitcoin/bitcoin@4a0abf6 Depends on D7416 Partial backport of Core [[bitcoin/bitcoin#17381 | PR17381]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7417

…pKeyPool method Summary: Previous discussion bitcoin/bitcoin#17304 (comment) --- bitcoin/bitcoin@491a599 Depends on D7417 Partial backport of Core [[bitcoin/bitcoin#17381 | PR17381]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7418

Summary: Suggested bitcoin/bitcoin#17304 (comment) by Gregory Sanders <gsanders87@gmail.com> Reason for keeping the `return true` `return false` verbosity is that more code will be added after the ReserveKeyFromKeyPool() call before returning. --- bitcoin/bitcoin@bfd826a Depends on D7418 Partial backport of Core [[bitcoin/bitcoin#17381 | PR17381]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7419

…cryptWallet Summary: Suggested bitcoin/bitcoin#17304 (comment) by me --- bitcoin/bitcoin@05b224a Depends on D7419 Concludes backport of Core [[bitcoin/bitcoin#17381 | PR17381]] Test Plan: ninja check check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D7420

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

achow101 mentioned this pull request Oct 29, 2019

Make ScriptPubKeyMan an actual interface and the wallet to have multiple #17261

Merged

fanquake added Refactoring Wallet labels Oct 29, 2019

ryanofsky approved these changes Oct 29, 2019

View reviewed changes

achow101 force-pushed the wallet-box-pr-3 branch from e1b1702 to 5631c01 Compare October 29, 2019 22:09

ryanofsky approved these changes Oct 29, 2019

View reviewed changes

achow101 force-pushed the wallet-box-pr-3 branch from 5631c01 to b04a526 Compare October 30, 2019 15:29

ryanofsky approved these changes Oct 30, 2019

View reviewed changes

instagibbs approved these changes Oct 31, 2019

View reviewed changes

ryanofsky reviewed Oct 31, 2019

View reviewed changes

ryanofsky mentioned this pull request Nov 1, 2019

LegacyScriptPubKeyMan code cleanups #17300

Merged

promag reviewed Nov 2, 2019

View reviewed changes

achow101 force-pushed the wallet-box-pr-3 branch from b04a526 to 707a817 Compare November 2, 2019 02:55

achow101 added 10 commits November 1, 2019 22:56

MOVEONLY: Reorder LegacyScriptPubKeyMan methods

b4cb18b

Can verify move-only with: git log -p -n1 --color-moved This commit is move-only and doesn't change code or affect behavior.

Refactor: Declare LegacyScriptPubKeyMan methods as virtual

533d8b3

This commit does not change behavior.

Refactor: Add new ScriptPubKeyMan virtual methods

acedc5b

This commit does not change behavior.

Refactor: Move SetAddressBook call out of LegacyScriptPubKeyMan::GetN…

769acef

…ewDestination This commit does not change behavior.

Refactor: Move SetWalletFlag out of LegacyScriptPubKeyMan::UpgradeKey…

4c5491f

…Metadata This commit does not change behavior.

Remove SetWalletFlag from WalletStorage

0391aba

SetWalletFlag is unused. Does not change any behavior

Refactor: Remove UnsetWalletFlag call from LegacyScriptPubKeyMan::Set…

78e7cbc

…HDSeed This commit does not change behavior.

refactor: Replace UnsetWalletFlagWithDB with UnsetBlankWalletFlag in …

fc2867f

…ScriptPubKeyMan 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.

Refactor: Move SetAddressBookWithDB call out of LegacyScriptPubKeyMan…

67be6b9

…::ImportScriptPubKeys This commit does not change behavior.

Refactor: Move LoadKey LegacyScriptPubKeyMan method definition

9716bbe

This commit does not change behavior.

ryanofsky mentioned this pull request Nov 19, 2019

wallet: Various fixes and cleanup to keypool handling in LegacyScriptPubKeyMan and CWallet #17373

Merged

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 17, 2020

Get rid of confusing LegacyScriptPubKeyMan::TopUpKeyPool method

70c6c86

Previous discussion bitcoin#17304 (comment)

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 17, 2020

Add missing SetupGeneration error handling in EncryptWallet

bec485a

Suggested bitcoin#17304 (comment) by me

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

	bool LegacyScriptPubKeyMan::GetReservedDestination(const OutputType type, bool internal, CTxDestination& address, int64_t& index, CKeyPool& keypool)
	{
	if (!CanGetAddresses(internal)) {
	return false;
	}

	{
	LOCK(cs_KeyStore);
	if (!ReserveKeyFromKeyPool(index, keypool, internal)) {
	return false;
	}
	LearnRelatedScripts(keypool.vchPubKey, type);
	address = GetDestinationForKey(keypool.vchPubKey, type);
	}
	return true;
	}

refactor: Move many functions into LegacyScriptPubKeyMan and further separate it from CWallet #17304

refactor: Move many functions into LegacyScriptPubKeyMan and further separate it from CWallet #17304

Conversation

achow101 commented Oct 29, 2019 • edited Loading

DrahtBot commented Oct 29, 2019 • edited Loading

Conflicts

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky commented Oct 29, 2019

achow101 commented Oct 29, 2019

instagibbs commented Oct 30, 2019

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky commented Oct 31, 2019

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Oct 31, 2019

Choose a reason for hiding this comment

Sjors commented Oct 31, 2019 • edited Loading

promag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky Nov 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Oct 29, 2019 •

edited

Loading

DrahtBot commented Oct 29, 2019 •

edited

Loading

Sjors commented Oct 31, 2019 •

edited

Loading

ryanofsky Nov 2, 2019 •

edited

Loading