[Do not merge] Stop advancing best block and shutdown node if keypool drops below critical threshold #10882

jnewbery · 2017-07-19T18:20:35Z

Add keypool_critical (configurable). If the number of keys in the keypool drops below this limit while the wallet is rescanning, shutdown the node. Do not shutdown the node if the wallet is 'current', ie it is receiving BlockConnected calls from the node.
Add keypool_min (configurable). If the number of keys in the keypool drops below this limit, stop advancing the wallet's best block. This forces the wallet to rescan from the point that it dropped below the limit the next time that it starts up. This is a toggle controlled by m_update_best_block, which doesn't get unset until the wallet has rescanned with the keypool above keypool_min.
Add bypasskeypoolcritical (command line argument). This disables the keypool_critical behavior, so the user has a chance to top up their keypool.
don't allow user actions like getnewaddress to cause the keypool to drop below the critical limit (return an error telling the user to unlock and topup their keypool).

This is a simpler version of #10830 , which caused the node to stop sync'ing if the keypool dropped below a certain limit. It is built on top of #11022 which does the following:

if a key in the keypool is used, mark all keys in the keypool up to that key as used
try to top up the keypool when keys from the keypool are used.

This PR couldn't be merged for v0.15 because there are some edge cases that make this dangerous and could result in users not being able to start up the node without onerous recovery steps.

ryanofsky · 2017-07-19T18:43:31Z

test/functional/keypool-restore.py

+        super().__init__()
+        self.setup_clean_chain = True
+        self.num_nodes = 2
+        self.extra_args = [['-usehd=0'], ['-usehd=1', '-keypool=100', '-keypoolemin=20']]


keypoolemin seems misspelled

instagibbs · 2017-07-19T20:03:42Z

There really needs to be some sort of recourse presented to the user upon shutdown.

ryanofsky · 2017-07-19T20:52:12Z

test/functional/keypool-restore.py

+        # self.nodes[0].generate(1)
+        self.sync_all()
+
+        assert_equal(self.nodes[1].getbalance(), 15)


On test failure happening this line, I think the reason is that encryptwallet generates an entirely new HD master key, so addr_enc_oldpool and addr_enc_extpool come from a new HD master key which is not part of "hd.bak"

Restoring "hd.enc.bak" instead of "hd.bak" makes this check pass: ryanofsky@93349ad

Thank you for this insight! Yes - that's what was causing the test to fail.

I've made some progress with the test. I've pushed what I've got so far, and hope to finish it off tomorrow morning.

ryanofsky · 2017-07-19T21:10:17Z

src/wallet/wallet.cpp

+                if (id > foundIndex) break; // set*KeyPool is ordered
+
+                CKeyPool keypool;
+                if (!walletdb.ReadPool(id, keypool)) {


Call seems unnecessary since keypool variable isn't used.

ryanofsky · 2017-07-19T21:24:22Z

src/wallet/wallet.cpp

+                LogPrintf("Parameter Interaction: keypool size (%d) must be larger than keypool minimum size for encrypted wallets (%d)\n", keypool_size, keypool_min);
+                SoftSetArg("-keypool", std::to_string(keypool_min));
+            }
+            InitWarning(_("You are using an encrypted HD wallet. You may miss incoming or outgoing transactions."));


FWIW, suggested more detailed wording for the warning message here: #10240 (comment)

Updated. Let me know what you think of the new text.

ryanofsky · 2017-07-19T21:27:38Z

src/wallet/wallet.cpp

+            InitWarning(_("You are using an encrypted HD wallet. You may miss incoming or outgoing transactions."));
+        } else {
+            if (keypool_size < keypool_min && keypool_size < DEFAULT_KEYPOOL_MIN) {
+                InitWarning(_("Your keypool size is below the recommended limit for HD rescans. You may miss incoming or outgoing transactions."));


Suggested more detailed wording for of this warning, too: #10240 (comment)

Updated. Let me know what you think of the new text.

ryanofsky · 2017-07-19T21:37:20Z

src/wallet/wallet.cpp

+            }
+            InitWarning(_("You are using an encrypted HD wallet. You may miss incoming or outgoing transactions."));
+        } else {
+            if (keypool_size < keypool_min && keypool_size < DEFAULT_KEYPOOL_MIN) {


I don't see any drawbacks to dropping DEFAULT_KEYPOOL_MIN condition here and just treating keypool_min like a normal value the user can control. Keeping this condition might make sense if the && were || (to make warning more paranoid), but currently it seems senseless.

yes - fixed

ryanofsky · 2017-07-19T21:41:42Z

src/wallet/wallet.cpp

+        // if the remaining keypool size is below the gap limit, shutdown
+        LogPrintf("%s: Keypool is too small. Shutting down. internal keypool: %d, external keypool: %d, keypool minimum: %d\n",
+                  __func__, setInternalKeyPool.size(), setExternalKeyPool.size(), keypool_min);
+        const static std::string error_msg = "Keypool is too small. Shutting down";


Static is a little unusual here, maybe drop it to avoid adding symbols to the binary.

ryanofsky · 2017-07-19T21:52:13Z

src/wallet/wallet.cpp

@@ -3523,18 +3576,78 @@ void CReserveKey::ReturnKey()
    vchPubKey = CPubKey();
 }

+void CWallet::CheckKeypoolMinSize() {
+    unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
+    if (IsHDEnabled() && (setInternalKeyPool.size() < keypool_min || (setExternalKeyPool.size() < keypool_min))) {


I'm not 100% clear on this, but if this is an hd wallet, but not an hd-split wallet should we only be checking setExternalKeyPool here because setInternalKeyPool will be empty? See

bitcoin/src/wallet/wallet.cpp

Line 3202 in 9e8d6a3

if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT))

I think you're right. Fixed

ryanofsky · 2017-07-19T22:05:31Z

src/wallet/wallet.cpp

+             * the mostly recently created transactions from newer versions of the wallet.
+             */
+            std::set<CKeyID> keyPool;
+            GetAllReserveKeys(keyPool);


It seems like this code would more efficient and maybe simpler if instead of making a set of CKeyID's here, we made a map from CKeyID -> (keypool_index, is_internal) and passed it into MarkReserveKeysAsUsed, so the first two loops in that function could be removed.

Yes, you're right that this would be much more efficient. However, GetAllReserveKeys() is also called elsewhere, which would also need to be modified to accept a map, so I'd prefer not to change it as part of this PR.

This can be fixed in a follow-up PR unless you think the performance in MarkReserveKeysAsUsed() is unacceptable.

ryanofsky · 2017-07-19T22:12:46Z

src/wallet/wallet.cpp

+        }
+    }
+
+    if (IsHDEnabled() && !TopUpKeyPool()) {


@sipa's comment about only topping up hd key pools would seem to apply here and maybe to CheckKeypoolMinSize: #10240 (review)

Yes. Applies here. I don't think I need to change CheckKeypoolMinSize() though. The node should only be shutdown for HD wallets.

ryanofsky · 2017-07-19T22:18:51Z

src/wallet/wallet.cpp

+    CWalletDB walletdb(*dbw);
+    for (std::set<int64_t> *setKeyPool : {&setInternalKeyPool, &setExternalKeyPool}) {
+        int64_t foundIndex = -1;
+        for (const int64_t& id : *setKeyPool) {


Maybe use index instead of id here and other places to distinguish from pool indices from key ids.

jnewbery · 2017-07-20T15:18:01Z

Thanks for the review @ryanofsky . I've addressed all of your concerns except the GetAllReserveKeys() refactor which can be done later.

All the changes are in separate fixup commits which can be squashed later.

jnewbery · 2017-07-20T15:18:53Z

Test is fixed. It required #10703 since the test involves node1 emitting warnings to stderr.

instagibbs · 2017-07-20T16:18:27Z

src/wallet/wallet.cpp

+            }
+            walletInstance->TopUpKeyPool();
+        }
+        walletInstance->CheckKeypoolMinSize();


We need to hold the wallet lock here or we fail the assertion at wallet/wallet.cpp:830 in CanSupportFeature.

is it ok to lock in CheckKeypoolMinSize() instead?

I think so. You'd be calling for the lock twice in the other path, but that's ok right?

yes, I don't think there's a problem doing that.

ryanofsky

utACK 965594a, left minor suggestions but this seems reasonable.

I do think using a map would probably simplify the code (even without changing the GetAllReserveKeys method), not just make it more efficient, so it might be something to reconsider if you end up making more changes for another reason.

Also maybe squash the commit history. I don't think it it's helpful at all. Would just keep the MOVEONLY commit, the #10703 commit, and then combine everything else in another commit.

ryanofsky · 2017-07-20T15:45:27Z

src/wallet/wallet.cpp

+            }
+        }
+
+        auto it = std::begin(*setKeyPool);


Maybe declare closer to while loop, also maybe write it = setKeyPool->begin()

ryanofsky · 2017-07-20T15:49:33Z

src/wallet/wallet.cpp

+        }
+    }
+
+    if (!TopUpKeyPool()) {


Maybe move TopUpKeyPool & CheckKeypoolMinSize out of MarkReserveKeysAsUsed (to the one caller) so MarkReserveKeysAsUsed only does what the name suggests.

ryanofsky · 2017-07-20T16:02:24Z

src/wallet/wallet.cpp

+        unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
+        if (walletInstance->IsCrypted()) {
+            if (keypool_size < keypool_min) {
+                LogPrintf("Parameter Interaction: keypool size (%d) must be larger than keypool minimum size for encrypted wallets (%d)\n", keypool_size, keypool_min);


If keypool size was provided nothing actually happens here so the log message seems misleading. Maybe change SoftSetArg to ForceSetArg or only log this if SoftSetArg returns true.

ryanofsky · 2017-07-20T16:03:09Z

src/wallet/wallet.cpp

+                LogPrintf("Parameter Interaction: keypool size (%d) must be larger than keypool minimum size for encrypted wallets (%d)\n", keypool_size, keypool_min);
+                SoftSetArg("-keypool", std::to_string(keypool_min));
+            }
+            InitWarning(strprintf(_("You are using an encrypted HD wallet. If you are restoring an old HD wallet that has not been topped up with the most recently "


Maybe explicitly warn about shutdowns, too

ryanofsky · 2017-07-20T16:48:44Z

test/functional/keypool-restore.py

+        self.num_nodes = 2
+        self.extra_args = [['-usehd=0'], ['-usehd=1', '-keypool=100', '-keypoolmin=20']]
+
+    def run_test(self):


Maybe split this up into test_restore(encrypted=False/True) or test_encrypted_restore/test_unencrypted_restore calls to make the test less repetitive and simpler to understand.

jnewbery · 2017-07-20T17:21:10Z

Squashed down to 4 commits. Will address @instagibbs and @ryanofsky feedback next.

gmaxwell · 2017-07-20T18:21:50Z

Can we make this also suppress the relock while scanning ... so that it's viable to just unlock and have it stay unlocked until its done-ish?

What will this do if, without any rescan, I exhaust all the keys in the wallet... will it falsely trigger?

instagibbs · 2017-07-20T19:35:23Z

src/wallet/wallet.cpp

@@ -3523,18 +3582,68 @@ void CReserveKey::ReturnKey()
    vchPubKey = CPubKey();
 }

+void CWallet::CheckKeypoolMinSize() {


Method name should reflect the fact it will shut down upon failure.

suggested name?

jnewbery · 2017-07-20T20:03:38Z

I've implemented @ryanofsky's suggestion for simplifying MarkReserveKeysAsUsed() and improved the error message if the node shuts down.

instagibbs · 2017-07-20T20:05:03Z

light tACK

Can confirm the new directions upon failure to topup using Crypted lead to recovery of proper index position.

instagibbs · 2017-07-20T20:07:24Z

src/wallet/wallet.cpp

+            }
+            walletInstance->TopUpKeyPool();
+        }
+        walletInstance->CheckKeypoolMinSize();


The new error message doesn't quite work here, but it's also quite unlikely to happen, so maybe moot.

sorry - I don't understand this comment. Can you explain what you mean by the new error message not quite working?

mistaken, ignore

instagibbs · 2017-07-20T20:12:06Z

src/wallet/wallet.cpp

+                                      "You can manually top-up your wallet keypool as follows:\n"
+                                      " - restart bitcoin with -keypoolmin set to 0\n"
+                                      " - unlock the wallet using walletpassphrase\n"
+                                      " - restart with -rescan. This will redownload the blockchain if you are running a pruned node.";


Just noting in case I'm underestimating the likelihood/cost: unlikely but this could be painful if they're using more than 1500 keys, as this might trigger multiple times causing them to re-download the chain multiple times.

This will go away with proper long-term fix.

What's special about 1500?

Ok my math is off, I think 1000, I just meant the 2nd time this could occur.

ryanofsky · 2017-07-20T20:24:43Z

src/wallet/wallet.cpp

+                                      "You can manually top-up your wallet keypool as follows:\n"
+                                      " - restart bitcoin with -keypoolmin set to 0\n"
+                                      " - unlock the wallet using walletpassphrase\n"
+                                      " - restart with -rescan. This will redownload the blockchain if you are running a pruned node.";


Relatedly, maybe this PR should explicitly avoid calling `WriteBestBlock` when the keypool size is too low, in case shutdown takes a long time or -keypoolmin is set to 0 to recover.

Relatedly, maybe this PR should explicitly avoid calling WriteBestBlock when the keypool size is too low,

ACK. Very much so. If not for having no mechanism for telling you that your wallet has fallen behind, this would more or less eliminate the need for shutdown. though it's weird that you'd lower the minimum to bypass the shutdown and inadvertently update the height.

Maybe there should be two flags:

-keypoolmin as the threshold for calling WriteBestBlock
-keypoolcriticalmin or -keypoolshutdownmin as the threshold for shutting down

Both flags could be set to 500. This still wouldn't provide a safe recovery path that avoids a complete rescan (we would need make walletpassphrase top up and rescan from bestblock after unlocking), but something like that could be added in the future, and in the meantime this would provide a safer way to avoid shutdowns if a rescan can't be done right away.

jnewbery · 2017-07-21T14:09:34Z

Added two commits:

rename current option keypoolcritical - if the keypool falls below this, then shutdown the node
add option keypoolmin - if the keypool falls below this, don't update the wallet's best block.

If people think that's conceptually the right approach, then I'll squash down into sensible commits.

ryanofsky · 2017-07-21T14:52:12Z

src/wallet/wallet.cpp

@@ -382,6 +415,11 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,

 void CWallet::SetBestChain(const CBlockLocator& loc)
 {
+    unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
+    if (IsHDEnabled() && ((CanSupportFeature(FEATURE_HD_SPLIT) && setInternalKeyPool.size() < keypool_min) || (setExternalKeyPool.size() < keypool_min))) {


Do you think there would be drawbacks to dropping the IsHDEnabled check here? Seems like that would be the safer thing to do.

Might also be good to have a helper method to avoid duplicating logic here and in the shutdown code. e.g.:

bool CWallet::HasUnusedKeys(int min_keys) const { return setExternalKeyPool.size() >= min_keys && (setInternalKeyPool.size() >= min_keys || !CanSupportFeature(FEATURE_HD_SPLIT)); }

As discussed, if the user is trying to restore from an old non-HD wallet and they drop below keypoolmin/keypoolcritical, there's very little they can do (since topup will generate new random keys). I'll leave this in for now, but if you think we definitely should shutdown or stop advancing best block for non-HD wallets, let me know.

Unless you strongly feel that I should add the helper method, I'll leave this as explicitly coded in both places.

ryanofsky · 2017-07-21T14:57:56Z

src/wallet/wallet.h

-    void GetAllReserveKeys(std::set<CKeyID>& setAddress) const;
+    void CheckKeypoolMinSize();
+    void MarkReserveKeysAsUsed(const int64_t keypool_index, const bool internal);
+    void GetAllReserveKeys(std::map<CKeyID, std::pair<int64_t, bool>>& mapKeyPool) const;


I would maybe define a little struct here to make code calling this more readable.

struct ReserveKey { int64_t index; bool internal; }; std::map<CKeyID, ReserveKey> GetAllReserveKeys() const;

Returning the map this way would also let callers loop over it easily or use auto.

ryanofsky · 2017-07-21T15:12:43Z

src/wallet/wallet.cpp

@@ -3523,30 +3589,65 @@ void CReserveKey::ReturnKey()
    vchPubKey = CPubKey();
 }

-static void LoadReserveKeysToSet(std::set<CKeyID>& setAddress, const std::set<int64_t>& setKeyPool, CWalletDB& walletdb) {
-    for (const int64_t& id : setKeyPool)
+void CWallet::CheckKeypoolMinSize() {


Could s/CheckKeypoolMinSize/CheckKeypoolCriticalSize

There was also an old comment about renaming this. #10882 (comment). Could go with something like MaybeShutdownForLaggingWallet

Good point. Changed name to ShutdownIfKeypoolCritical()

ryanofsky · 2017-07-21T15:16:32Z

src/wallet/wallet.cpp

@@ -3834,6 +3903,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)

        strUsage += HelpMessageOpt("-dblogsize=<n>", strprintf("Flush wallet database activity from memory to disk log every <n> megabytes (default: %u)", DEFAULT_WALLET_DBLOGSIZE));
        strUsage += HelpMessageOpt("-flushwallet", strprintf("Run a thread to flush wallet periodically (default: %u)", DEFAULT_FLUSHWALLET));
+        strUsage += HelpMessageOpt("-keypoolcritical", strprintf(_("If the keypool drops below this number of keys and we are unable to generate new keys, shutdown (default: %u)"), DEFAULT_KEYPOOL_CRITICAL));


Need to add -keypoolmin I think too.

jnewbery · 2017-07-21T17:13:58Z

@ryanofsky I've addressed all of your comments. Let me know if you're happy with the last three commits and I'll squash them down for the benefit of other reviewers,

ryanofsky

utACK 89af21ddcd7837c45cf8e261711d227e4f8448db

ryanofsky · 2017-07-21T17:17:48Z

src/wallet/wallet.cpp

+std::map<CKeyID, ReserveKey> CWallet::GetAllReserveKeys() const
+{
+    std::map<CKeyID, ReserveKey> mapKeyPool;
+    mapKeyPool.clear();


Could drop clear, map will already be empty

jnewbery · 2017-07-21T18:25:06Z

All comments should now be addressed and changes squashed down into 6 commits:

Allow tests to pass when stderr is non-empty is [tests] Allow tests to pass when stderr is non-empty #10703
MOVEONLY move CAffectedKeysVisitor is a code move
Add ReserveKey struct and return it from GetAllReserveKeys() is a slight refactor of existing code
Add keypoolcritical adds the node shutdown functionality
Add keypoolmin adds the don't-advance-wallet-best-block functionality
add keypool restore functional test adds tests

Ready for wider review.

jnewbery · 2017-08-10T14:27:49Z

As requested by @gmaxwell I've pulled out the basic keypool mark-used/topup functionality into #11022. This PR is a superset of that and also contains the keypool_min/keypool_critical functionality to stop the node/prevent best block from advancing if the keypool drops below a threshold.

ryanofsky · 2017-08-10T16:30:55Z

Note to reviewers, this is now based on #11022, so changes in commits before "Addkeypoolmin" and "Add keypoolcritical" can be discussed there.

ryanofsky · 2017-08-10T18:39:54Z

src/wallet/wallet.cpp

@@ -1644,6 +1644,10 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f

        fScanningWallet = false;
    }
+
+    // Check that we haven't dropped below the keypool_critical threshold.
+    ShutdownIfKeypoolCritical();


In commit "[wallet] Add keypoolcritical"

I'm confused by this. It seems like this is the only call to ShutdownIfKeypoolCritical() in the whole PR, and it's in the rescan function, so the node will no longer shut down if the wallet is locked and the keypool gradually drains, unless the user manually triggers a rescan or restarts the node? If this is behavior that's intended, it should be documented in the -keypoolcritical help. It might also be good to update the PR description (which is currently out of date) to list the changes here and say what problems are fixed and not fixed by them.

Yes, that's the current implementation:

07:58 < gmaxwell> jnewbery: Can we distinguish the case where we are current vs rescanning? If so, then we just shouldn't perform the shutdown when we're current.

(https://botbot.me/freenode/bitcoin-core-dev/2017-08-08/?msg=89552785&page=2)

We'll need to discuss this in the meeting today. There doesn't seem to be agreement about what this PR should even be trying to do.

Thread #10882 (comment)

That's fine, but it seems that current -keypoolcritical help description is pretty misleading. It would be good to update it, as well as the PR description.

maflcko · 2017-08-10T19:12:51Z

Cleared the 0.15.0 milestone for now. Let's focus on #11022 first.

ryanofsky · 2017-08-10T19:09:14Z

src/wallet/rpcwallet.cpp

@@ -161,8 +161,8 @@ UniValue getnewaddress(const JSONRPCRequest& request)

    // Generate a new key that is added to wallet
    CPubKey newKey;
-    if (!pwallet->GetKeyFromPool(newKey)) {
-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");
+    if (!pwallet->GetKeyFromPool(newKey, false, true)) {


In commit "[wallet] Don't allow getnewaddress to drop keypool to critical."

Do you want to make the same change to getrawchangeaddress? (as mentioned #10882 (comment))

ryanofsky · 2017-08-10T19:13:34Z

src/wallet/wallet.cpp

@@ -1644,6 +1644,10 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f

        fScanningWallet = false;
    }
+
+    // Check that we haven't dropped below the keypool_critical threshold.
+    ShutdownIfKeypoolCritical();


Thread #10882 (comment)

That's fine, but it seems that current -keypoolcritical help description is pretty misleading. It would be good to update it, as well as the PR description.

ryanofsky · 2017-08-10T19:20:20Z

src/wallet/wallet.cpp

+    if (walletInstance->IsHDEnabled()) {
+        unsigned int keypool_size = GetArg("-keypool", DEFAULT_KEYPOOL_SIZE);
+        unsigned int keypool_critical = GetArg("-keypoolcritical", DEFAULT_KEYPOOL_CRITICAL);
+        if (keypool_size < keypool_critical) {


Thread #10882 (comment)

I don't see a change, should log print just be changed to "smaller or equal to"?

ryanofsky · 2017-08-10T19:33:38Z

src/wallet/wallet.cpp

+    if (IsHDEnabled() && !HasUnusedKeys(keypool_min) && m_update_best_block) {
+        LogPrintf("Keypool has fallen below keypool_min (%s). Wallet will no longer watch for new transactions and best block height will not be advanced.\n", keypool_min);
+        LogPrintf("Unlock wallet, top up keypool and rescan to resume watching for new transactions.\n");
+        m_update_best_block = false;


In commit "[wallet] Add m_update_best_block"

As mentioned #10882 (comment), I think the transition from true -> false m_update_best_block should just happen after the topup in AddToWalletIfInvolvingMe. So this logic, and the logic in SetBestChain could be removed. SetBestChain would only read m_update_best_block, not write to it or interact with the keypool.

d34957e [wallet] [tests] Add keypool topup functional test (Jonas Schnelli) 095142d [wallet] keypool mark-used and topup (John Newbery) c25d90f [wallet] Add HasUnusedKeys() helper (John Newbery) f2123e3 [wallet] Cache keyid -> keypool id mappings (John Newbery) 83f1ec3 [wallet] Don't hold cs_LastBlockFile while calling setBestChain (John Newbery) 2376bfc [wallet] [moveonly] Move LoadKeyPool to cpp (Matt Corallo) cab8557 [wallet] [moveonly] Move CAffectedKeysVisitor (Jonas Schnelli) Pull request description: This PR contains the first part of #10882 : - if a key from the keypool is used, mark all keys up to that key as used, and then try to top up the keypool - top up the keypool on startup Notably, it does not stop the node or prevent the best block from advancing if the keypool drops below a threshold (which means that transactions may be missed and funds lost if restoring from an old HD wallet backup). Tree-SHA512: ac681fefeaf7ec2aab2fa1da93d12273ea80bd05eb48d7b3b551ea6e5d975dd97ba7de52b7fba52993823280ac4079cc36cf78a27dac708107ebf8fb6326142b

Add a -keypoolcritical option. If an HD wallet's keypool drops below this size and can't be topped up (since the wallet is encrypted), stop the node.

If an HD wallet's keypool falls below keypoolmin, don't update its best block until the keypool has been topped up.

m_update_best_block is set to false the first time we try to SetBestChain and the keypool has falled below keypool_min. After that we won't try to update the wallet's best block until we've rescanned from that point with the keypool above keypool_min.

bypasskeypoolcritical is a hidden command line option that allows the user to restart bitcoin with an encrypted wallet below the keypool_critical threshold. Bitcoind won't shut down, but it also won't advance the wallet best block until the keypool is topped up.

Adds a bool parameter to GetKeyFromPool(), which will cause the function to return failure if taking a key from the keypool would result in hitting the keypool_critical threshold.

If getting a new address would cause the keypool to drop below the keypool_critical threshold, fail the RPC and prompt the user to unlock the wallet so the keypool can be topped up.

In almost all cases, a call to GetKeyFromPool() should fail if getting a new key would result in the keypool dropping below critical.

This updates the keypool-topup.py test script to test the keypool critical functionality (ie the node shuts down if the keypool drops below the keypool_critical threshold and the wallet can't topup its keypool)

jnewbery · 2017-08-14T15:50:35Z

rebased on master and updated PR notes. Shouldn't be merged without further work to make sure this is safe.

@gmaxwell - could you comment here on the edge cases here (for future reference).

jnewbery · 2017-08-29T18:26:29Z

Closing for now. There were several edge-cases discovered that meant that this approach needs a bit more careful consideration. @gmaxwell is going to add some notes to this PR so if we do pick this later, we don't need to rediscover the edge-cases.

droark · 2017-12-09T10:04:59Z

@jnewbery - Just happened to come across this PR while going through some commits. Did you ever get the edge case notes?

jnewbery · 2017-12-09T15:35:42Z

I'm not sure if Greg collected them together. You can probably find them if you scrape through the IRC logs.

d34957e [wallet] [tests] Add keypool topup functional test (Jonas Schnelli) 095142d [wallet] keypool mark-used and topup (John Newbery) c25d90f [wallet] Add HasUnusedKeys() helper (John Newbery) f2123e3 [wallet] Cache keyid -> keypool id mappings (John Newbery) 83f1ec3 [wallet] Don't hold cs_LastBlockFile while calling setBestChain (John Newbery) 2376bfc [wallet] [moveonly] Move LoadKeyPool to cpp (Matt Corallo) cab8557 [wallet] [moveonly] Move CAffectedKeysVisitor (Jonas Schnelli) Pull request description: This PR contains the first part of bitcoin#10882 : - if a key from the keypool is used, mark all keys up to that key as used, and then try to top up the keypool - top up the keypool on startup Notably, it does not stop the node or prevent the best block from advancing if the keypool drops below a threshold (which means that transactions may be missed and funds lost if restoring from an old HD wallet backup). Tree-SHA512: ac681fefeaf7ec2aab2fa1da93d12273ea80bd05eb48d7b3b551ea6e5d975dd97ba7de52b7fba52993823280ac4079cc36cf78a27dac708107ebf8fb6326142b

jnewbery mentioned this pull request Jul 19, 2017

[WIP] [wallet] keypool restore #10830

Closed

3 tasks

ryanofsky reviewed Jul 19, 2017

View reviewed changes

jonasschnelli added the Wallet label Jul 20, 2017

jnewbery force-pushed the keypool_topup branch from 7f518ef to 151be4b Compare July 20, 2017 15:08

jnewbery force-pushed the keypool_topup branch from 151be4b to 965594a Compare July 20, 2017 15:19

jnewbery changed the title ~~[WIP] Keypool topup~~ Keypool topup Jul 20, 2017

instagibbs reviewed Jul 20, 2017

View reviewed changes

ryanofsky reviewed Jul 20, 2017

View reviewed changes

jnewbery force-pushed the keypool_topup branch from 965594a to 61d604d Compare July 20, 2017 17:18

laanwj added this to the 0.15.0 milestone Jul 20, 2017

instagibbs reviewed Jul 20, 2017

View reviewed changes

ryanofsky reviewed Jul 20, 2017

View reviewed changes

ryanofsky reviewed Jul 21, 2017

View reviewed changes

jnewbery force-pushed the keypool_topup branch from 64ef214 to 89af21d Compare July 21, 2017 17:13

ryanofsky reviewed Jul 21, 2017

View reviewed changes

jnewbery force-pushed the keypool_topup branch from 89af21d to a4f0200 Compare July 21, 2017 18:22

jnewbery changed the title ~~Keypool topup~~ Stop advancing best block and shutdown node if keypool drops below critical threshold Aug 10, 2017

jnewbery force-pushed the keypool_topup branch from e098a2c to 64ccb77 Compare August 10, 2017 17:59

ryanofsky reviewed Aug 10, 2017

View reviewed changes

maflcko removed this from the 0.15.0 milestone Aug 10, 2017

ryanofsky reviewed Aug 10, 2017

View reviewed changes

jnewbery added 8 commits August 14, 2017 11:37

[wallet] Add keypoolcritical

ebf3133

Add a -keypoolcritical option. If an HD wallet's keypool drops below this size and can't be topped up (since the wallet is encrypted), stop the node.

[wallet] Add keypoolmin

02d69ae

If an HD wallet's keypool falls below keypoolmin, don't update its best block until the keypool has been topped up.

[wallet] Add m_update_best_block

60c74c3

m_update_best_block is set to false the first time we try to SetBestChain and the keypool has falled below keypool_min. After that we won't try to update the wallet's best block until we've rescanned from that point with the keypool above keypool_min.

[wallet] Add fail_on_critical to GetKeyFromPool()

40ce25c

Adds a bool parameter to GetKeyFromPool(), which will cause the function to return failure if taking a key from the keypool would result in hitting the keypool_critical threshold.

[wallet] Don't allow getnewaddress to drop keypool to critical.

1a916ed

If getting a new address would cause the keypool to drop below the keypool_critical threshold, fail the RPC and prompt the user to unlock the wallet so the keypool can be topped up.

[wallet] Don't let keypool drop below critical

e29ea7e

In almost all cases, a call to GetKeyFromPool() should fail if getting a new key would result in the keypool dropping below critical.

[wallet] [tests] Add keypool critical functional test

3140a97

This updates the keypool-topup.py test script to test the keypool critical functionality (ie the node shuts down if the keypool drops below the keypool_critical threshold and the wallet can't topup its keypool)

jnewbery force-pushed the keypool_topup branch from 64ccb77 to 3140a97 Compare August 14, 2017 15:37

jnewbery changed the title ~~Stop advancing best block and shutdown node if keypool drops below critical threshold~~ [Do not merge] Stop advancing best block and shutdown node if keypool drops below critical threshold Aug 14, 2017

jnewbery closed this Aug 29, 2017

jnewbery mentioned this pull request Aug 30, 2017

[wallet] loadwallet RPC - load wallet at runtime #10740

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[Do not merge] Stop advancing best block and shutdown node if keypool drops below critical threshold #10882

[Do not merge] Stop advancing best block and shutdown node if keypool drops below critical threshold #10882

Uh oh!

Conversation

jnewbery commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 20, 2017

Uh oh!

jnewbery commented Jul 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 19, 2017 •

edited

Loading

instagibbs Jul 20, 2017 •

edited

Loading