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

jnewbery commented at 6:20 pm on July 19, 2017: member

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.

in test/functional/keypool-restore.py:16 in d2d67fdf7d outdated

11+class KeypoolRestoreTest(BitcoinTestFramework):
12+    def __init__(self):
13+        super().__init__()
14+        self.setup_clean_chain = True
15+        self.num_nodes = 2
16+        self.extra_args = [['-usehd=0'], ['-usehd=1', '-keypool=100', '-keypoolemin=20']]

ryanofsky commented at 6:43 pm on July 19, 2017:

keypoolemin seems misspelled

instagibbs commented at 8:03 pm on July 19, 2017: member

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

in test/functional/keypool-restore.py:78 in d2d67fdf7d outdated

73+
74+        # self.sync_all()
75+        # self.nodes[0].generate(1)
76+        self.sync_all()
77+
78+        assert_equal(self.nodes[1].getbalance(), 15)

ryanofsky commented at 8:52 pm on July 19, 2017:

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: https://github.com/ryanofsky/bitcoin/commit/93349ad5d915ee152a1a84920f18cbe0cab8b036

jnewbery commented at 9:41 pm on July 19, 2017:

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.

in src/wallet/wallet.cpp:3634 in d2d67fdf7d outdated

3630+            while (it != std::end(*setKeyPool)) {
3631+                const int64_t& id = *(it);
3632+                if (id > foundIndex) break; // set*KeyPool is ordered
3633+
3634+                CKeyPool keypool;
3635+                if (!walletdb.ReadPool(id, keypool)) {

ryanofsky commented at 9:10 pm on July 19, 2017:

Call seems unnecessary since keypool variable isn’t used.

jnewbery commented at 3:15 pm on July 20, 2017:

removed

in src/wallet/wallet.cpp:4048 in d2d67fdf7d outdated

4043+        if (walletInstance->IsCrypted()) {
4044+            if (keypool_size < keypool_min) {
4045+                LogPrintf("Parameter Interaction: keypool size (%d) must be larger than keypool minimum size for encrypted wallets (%d)\n", keypool_size, keypool_min);
4046+                SoftSetArg("-keypool", std::to_string(keypool_min));
4047+            }
4048+            InitWarning(_("You are using an encrypted HD wallet. You may miss incoming or outgoing transactions."));

ryanofsky commented at 9:24 pm on July 19, 2017:

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

jnewbery commented at 3:16 pm on July 20, 2017:

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

in src/wallet/wallet.cpp:4051 in d2d67fdf7d outdated

4046+                SoftSetArg("-keypool", std::to_string(keypool_min));
4047+            }
4048+            InitWarning(_("You are using an encrypted HD wallet. You may miss incoming or outgoing transactions."));
4049+        } else {
4050+            if (keypool_size < keypool_min && keypool_size < DEFAULT_KEYPOOL_MIN) {
4051+                InitWarning(_("Your keypool size is below the recommended limit for HD rescans. You may miss incoming or outgoing transactions."));

ryanofsky commented at 9:27 pm on July 19, 2017:

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

jnewbery commented at 3:16 pm on July 20, 2017:

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

in src/wallet/wallet.cpp:4050 in d2d67fdf7d outdated

4045+                LogPrintf("Parameter Interaction: keypool size (%d) must be larger than keypool minimum size for encrypted wallets (%d)\n", keypool_size, keypool_min);
4046+                SoftSetArg("-keypool", std::to_string(keypool_min));
4047+            }
4048+            InitWarning(_("You are using an encrypted HD wallet. You may miss incoming or outgoing transactions."));
4049+        } else {
4050+            if (keypool_size < keypool_min && keypool_size < DEFAULT_KEYPOOL_MIN) {

ryanofsky commented at 9:37 pm on July 19, 2017:

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.

jnewbery commented at 3:16 pm on July 20, 2017:

yes - fixed

in src/wallet/wallet.cpp:3585 in d2d67fdf7d outdated

3580+    unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
3581+    if (IsHDEnabled() && (setInternalKeyPool.size() < keypool_min || (setExternalKeyPool.size() < keypool_min))) {
3582+        // if the remaining keypool size is below the gap limit, shutdown
3583+        LogPrintf("%s: Keypool is too small. Shutting down. internal keypool: %d, external keypool: %d, keypool minimum: %d\n",
3584+                  __func__, setInternalKeyPool.size(), setExternalKeyPool.size(), keypool_min);
3585+        const static std::string error_msg = "Keypool is too small. Shutting down";

ryanofsky commented at 9:41 pm on July 19, 2017:

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

jnewbery commented at 3:15 pm on July 20, 2017:

yup

in src/wallet/wallet.cpp:3581 in d2d67fdf7d outdated

3575@@ -3523,18 +3576,78 @@ void CReserveKey::ReturnKey()
3576     vchPubKey = CPubKey();
3577 }
3578 
3579+void CWallet::CheckKeypoolMinSize() {
3580+    unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
3581+    if (IsHDEnabled() && (setInternalKeyPool.size() < keypool_min || (setExternalKeyPool.size() < keypool_min))) {

ryanofsky commented at 9:52 pm on July 19, 2017:

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 https://github.com/bitcoin/bitcoin/blob/9e8d6a3fb43a2433ef46aaf95511650e3888f730/src/wallet/wallet.cpp#L3202

jnewbery commented at 3:15 pm on July 20, 2017:

I think you’re right. Fixed

in src/wallet/wallet.cpp:1046 in d2d67fdf7d outdated

1041+             * have appeared in a new transaction. If so, remove those keys from the keypool.
1042+             * This can happen when restoring an old wallet backup that does not contain
1043+             * the mostly recently created transactions from newer versions of the wallet.
1044+             */
1045+            std::set<CKeyID> keyPool;
1046+            GetAllReserveKeys(keyPool);

ryanofsky commented at 10:05 pm on July 19, 2017:

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.

jnewbery commented at 3:15 pm on July 20, 2017:

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.

in src/wallet/wallet.cpp:3644 in d2d67fdf7d outdated

3640+                it = setKeyPool->erase(it);
3641+            }
3642+        }
3643+    }
3644+
3645+    if (IsHDEnabled() && !TopUpKeyPool()) {

ryanofsky commented at 10:12 pm on July 19, 2017:

@sipa’s comment about only topping up hd key pools would seem to apply here and maybe to CheckKeypoolMinSize: #10240#pullrequestreview-49644483

jnewbery commented at 3:16 pm on July 20, 2017:

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

in src/wallet/wallet.cpp:3610 in 7f518efcf3 outdated

3606+{
3607+    AssertLockHeld(cs_wallet);
3608+    CWalletDB walletdb(*dbw);
3609+    for (std::set<int64_t> *setKeyPool : {&setInternalKeyPool, &setExternalKeyPool}) {
3610+        int64_t foundIndex = -1;
3611+        for (const int64_t& id : *setKeyPool) {

ryanofsky commented at 10:18 pm on July 19, 2017:

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

jnewbery commented at 3:15 pm on July 20, 2017:

done

jonasschnelli added the label Wallet on Jul 20, 2017

jnewbery force-pushed on Jul 20, 2017

jnewbery commented at 3:18 pm on July 20, 2017: member

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 commented at 3:18 pm on July 20, 2017: member

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

jnewbery force-pushed on Jul 20, 2017

jnewbery renamed this:
~~[WIP] Keypool topup~~
Keypool topup
on Jul 20, 2017

in src/wallet/wallet.cpp:3626 in 965594a552 outdated

3622+                }
3623+                break;
3624+            }
3625+        }
3626+
3627+        auto it = std::begin(*setKeyPool);

ryanofsky commented at 3:45 pm on July 20, 2017:

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

jnewbery commented at 5:56 pm on July 20, 2017:

done

in src/wallet/wallet.cpp:3639 in 965594a552 outdated

3635+                it = setKeyPool->erase(it);
3636+            }
3637+        }
3638+    }
3639+
3640+    if (!TopUpKeyPool()) {

ryanofsky commented at 3:49 pm on July 20, 2017:

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

jnewbery commented at 5:56 pm on July 20, 2017:

done

in src/wallet/wallet.cpp:4040 in 965594a552 outdated

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

ryanofsky commented at 4:02 pm on July 20, 2017:

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.

jnewbery commented at 5:56 pm on July 20, 2017:

done

in src/wallet/wallet.cpp:4043 in 965594a552 outdated

4038+        if (walletInstance->IsCrypted()) {
4039+            if (keypool_size < keypool_min) {
4040+                LogPrintf("Parameter Interaction: keypool size (%d) must be larger than keypool minimum size for encrypted wallets (%d)\n", keypool_size, keypool_min);
4041+                SoftSetArg("-keypool", std::to_string(keypool_min));
4042+            }
4043+            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 "

ryanofsky commented at 4:03 pm on July 20, 2017:

Maybe explicitly warn about shutdowns, too

jnewbery commented at 5:56 pm on July 20, 2017:

done

in src/wallet/wallet.cpp:4052 in 965594a552 outdated

4047+                InitWarning(strprintf(_("Your keypool is configured to store %d keys, which is below the keypool minimum size of %d. Using a larger keypool will make it less "
4048+                                        "likely that your wallet will be missing transactions and funds if it is restored from an old backup."), keypool_size, keypool_min));
4049+            }
4050+            walletInstance->TopUpKeyPool();
4051+        }
4052+        walletInstance->CheckKeypoolMinSize();

instagibbs commented at 4:18 pm on July 20, 2017:

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

jnewbery commented at 5:25 pm on July 20, 2017:

is it ok to lock in CheckKeypoolMinSize() instead?

instagibbs commented at 5:28 pm on July 20, 2017:

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

jnewbery commented at 5:50 pm on July 20, 2017:

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

in test/functional/keypool-restore.py:38 in 965594a552 outdated

30+        super().__init__()
31+        self.setup_clean_chain = True
32+        self.num_nodes = 2
33+        self.extra_args = [['-usehd=0'], ['-usehd=1', '-keypool=100', '-keypoolmin=20']]
34+
35+    def run_test(self):

ryanofsky commented at 4:48 pm on July 20, 2017:

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 commented at 5:56 pm on July 20, 2017:

done

ryanofsky commented at 5:03 pm on July 20, 2017: member

utACK 965594a5528ce74e6a1d51232da729c166d176d3, 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.

jnewbery force-pushed on Jul 20, 2017

jnewbery commented at 5:21 pm on July 20, 2017: member

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

gmaxwell commented at 6:21 pm on July 20, 2017: contributor

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?

laanwj added this to the milestone 0.15.0 on Jul 20, 2017

in src/wallet/wallet.cpp:3585 in 691991b6aa outdated

3581@@ -3523,18 +3582,68 @@ void CReserveKey::ReturnKey()
3582     vchPubKey = CPubKey();
3583 }
3584 
3585+void CWallet::CheckKeypoolMinSize() {

instagibbs commented at 7:35 pm on July 20, 2017:

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

jnewbery commented at 8:02 pm on July 20, 2017:

suggested name?

jnewbery commented at 8:03 pm on July 20, 2017: member

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

instagibbs commented at 8:05 pm on July 20, 2017: member

light tACK

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

in src/wallet/wallet.cpp:4038 in b12a6a051a outdated

4033+                InitWarning(strprintf(_("Your keypool is configured to store %d keys, which is below the keypool minimum size of %d. Using a larger keypool will make it less "
4034+                                        "likely that your wallet will be missing transactions and funds if it is restored from an old backup."), keypool_size, keypool_min));
4035+            }
4036+            walletInstance->TopUpKeyPool();
4037+        }
4038+        walletInstance->CheckKeypoolMinSize();

instagibbs commented at 8:07 pm on July 20, 2017:

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

jnewbery commented at 1:27 pm on July 21, 2017:

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

instagibbs commented at 1:30 pm on July 21, 2017:

mistaken, ignore

in src/wallet/wallet.cpp:3645 in b12a6a051a outdated

3597+                                      "This is probably because you are restoring an old backup wallet file which has not been topped up with the most recently "
3598+                                      "derived keys, and so you would not detect transactions involving those keys.\n"
3599+                                      "You can manually top-up your wallet keypool as follows:\n"
3600+                                      " - restart bitcoin with -keypoolmin set to 0\n"
3601+                                      " - unlock the wallet using walletpassphrase\n"
3602+                                      " - restart with -rescan. This will redownload the blockchain if you are running a pruned node.";

instagibbs commented at 8:12 pm on July 20, 2017:

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.

jnewbery commented at 1:28 pm on July 21, 2017:

What’s special about 1500?

instagibbs commented at 1:29 pm on July 21, 2017:

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

in src/wallet/wallet.cpp:3645 in b12a6a051a outdated

3597+                                      "This is probably because you are restoring an old backup wallet file which has not been topped up with the most recently "
3598+                                      "derived keys, and so you would not detect transactions involving those keys.\n"
3599+                                      "You can manually top-up your wallet keypool as follows:\n"
3600+                                      " - restart bitcoin with -keypoolmin set to 0\n"
3601+                                      " - unlock the wallet using walletpassphrase\n"
3602+                                      " - restart with -rescan. This will redownload the blockchain if you are running a pruned node.";

ryanofsky commented at 8:24 pm on July 20, 2017:

0
1Relatedly, 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.

gmaxwell commented at 7:17 am on July 21, 2017:

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.

ryanofsky commented at 9:02 am on July 21, 2017:

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 commented at 2:09 pm on July 21, 2017: member

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.

in src/wallet/wallet.cpp:422 in 64ef2140f3 outdated

414@@ -382,6 +415,11 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
415 
416 void CWallet::SetBestChain(const CBlockLocator& loc)
417 {
418+    unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
419+    if (IsHDEnabled() && ((CanSupportFeature(FEATURE_HD_SPLIT) && setInternalKeyPool.size() < keypool_min) || (setExternalKeyPool.size() < keypool_min))) {

ryanofsky commented at 2:52 pm on July 21, 2017:

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.:

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

jnewbery commented at 4:36 pm on July 21, 2017:

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.

in src/wallet/wallet.h:1001 in 64ef2140f3 outdated

 995@@ -988,7 +996,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 996     void ReturnKey(int64_t nIndex, bool fInternal);
 997     bool GetKeyFromPool(CPubKey &key, bool internal = false);
 998     int64_t GetOldestKeyPoolTime();
 999-    void GetAllReserveKeys(std::set<CKeyID>& setAddress) const;
1000+    void CheckKeypoolMinSize();
1001+    void MarkReserveKeysAsUsed(const int64_t keypool_index, const bool internal);
1002+    void GetAllReserveKeys(std::map<CKeyID, std::pair<int64_t, bool>>& mapKeyPool) const;

ryanofsky commented at 2:57 pm on July 21, 2017:

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

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

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

jnewbery commented at 5:13 pm on July 21, 2017:

done

in src/wallet/wallet.cpp:3592 in 64ef2140f3 outdated

3588@@ -3523,30 +3589,65 @@ void CReserveKey::ReturnKey()
3589     vchPubKey = CPubKey();
3590 }
3591 
3592-static void LoadReserveKeysToSet(std::set<CKeyID>& setAddress, const std::set<int64_t>& setKeyPool, CWalletDB& walletdb) {
3593-    for (const int64_t& id : setKeyPool)
3594+void CWallet::CheckKeypoolMinSize() {

ryanofsky commented at 3:12 pm on July 21, 2017:

Could s/CheckKeypoolMinSize/CheckKeypoolCriticalSize

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

jnewbery commented at 4:36 pm on July 21, 2017:

Good point. Changed name to ShutdownIfKeypoolCritical()

in src/wallet/wallet.cpp:3993 in 64ef2140f3 outdated

3902@@ -3834,6 +3903,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
3903 
3904         strUsage += HelpMessageOpt("-dblogsize=<n>", strprintf("Flush wallet database activity from memory to disk log every <n> megabytes (default: %u)", DEFAULT_WALLET_DBLOGSIZE));
3905         strUsage += HelpMessageOpt("-flushwallet", strprintf("Run a thread to flush wallet periodically (default: %u)", DEFAULT_FLUSHWALLET));
3906+        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));

ryanofsky commented at 3:16 pm on July 21, 2017:

Need to add -keypoolmin I think too.

jnewbery commented at 4:36 pm on July 21, 2017:

done

jnewbery force-pushed on Jul 21, 2017

jnewbery commented at 5:13 pm on July 21, 2017: member

@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,

in src/wallet/wallet.cpp:3641 in 89af21ddcd outdated

3643+}
3644+
3645+std::map<CKeyID, ReserveKey> CWallet::GetAllReserveKeys() const
3646+{
3647+    std::map<CKeyID, ReserveKey> mapKeyPool;
3648+    mapKeyPool.clear();

ryanofsky commented at 5:17 pm on July 21, 2017:

Could drop clear, map will already be empty

jnewbery commented at 5:38 pm on July 21, 2017:

dropped

ryanofsky commented at 5:20 pm on July 21, 2017: member

utACK 89af21ddcd7837c45cf8e261711d227e4f8448db

jnewbery force-pushed on Jul 21, 2017

jnewbery commented at 6:25 pm on July 21, 2017: member

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

Allow tests to pass when stderr is non-empty is #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 force-pushed on Jul 24, 2017

jnewbery commented at 4:44 pm on July 24, 2017: member

Locking issue has been resolved. This should now pass Travis.

I’ve added a new commit don’t hold cs_LastBlockFile while calling setBestChain

in src/wallet/wallet.cpp:3637 in 53f32125ee outdated

3629-        mapKeyPool[keyID] = ReserveKey {id, keypool.fInternal};
3630+        mapKeyPool[keyID] = ReserveKey {index, keypool.fInternal};
3631+    }
3632+}
3633+
3634+void CWallet::MarkReserveKeysAsUsed(const ReserveKey reserve_key)

ryanofsky commented at 7:09 pm on July 25, 2017:

In commit “[wallet] Add keypoolcritical”

Maybe add & if this isn’t an intentional optimization, it’s a little more conventional in c++ to pass structs by reference instead of value to avoid potentially slow copies.

in src/wallet/wallet.cpp:3633 in 53f32125ee outdated

3625+        if (!walletdb.ReadPool(index, keypool))
3626             throw std::runtime_error(std::string(__func__) + ": read failed");
3627         assert(keypool.vchPubKey.IsValid());
3628         CKeyID keyID = keypool.vchPubKey.GetID();
3629-        mapKeyPool[keyID] = ReserveKey {id, keypool.fInternal};
3630+        mapKeyPool[keyID] = ReserveKey {index, keypool.fInternal};

ryanofsky commented at 7:10 pm on July 25, 2017:

In commit “[wallet] Add keypoolcritical”

Could make PR a little smaller by moving these changes to previous commit.

ryanofsky commented at 7:19 pm on July 25, 2017: member

utACK 91f3fb95d9e197a3f7758e1a4f37df003affd28f. Changes since last review: new lock in SetBestChain and unlock in FlushStateToDisk, changed MarkReserveKeysAsUsed arguments, changed LoadReserveKeysToMap error string

I also posted a comment in #10238 (comment) about potential future optimizations.

ryanofsky commented at 7:42 pm on July 25, 2017: member

@jnewbery, can you explain the reason for dependency on #10703? It does seem useful to have travis continue checking for unexpected error messages (see https://botbot.me/freenode/bitcoin-core-dev/msg/89036779/)

MarcoFalke commented at 9:04 pm on July 25, 2017: member

After discussion on irc:

Would it be possible to pipe the stderr into a SpooledTempFile instead? This way one can check that the error string matches. The issue with the current approach is that it is active for all tests, even though none of the other tests needs it. This might hide serious bugs by accident.

One nit: Please remove the execute bit from util.py

in src/wallet/wallet.cpp:1056 in 53f32125ee outdated

1045+             * the mostly recently created transactions from newer versions of the wallet.
1046+             */
1047+            std::map<CKeyID, ReserveKey> keyPool = GetAllReserveKeys();
1048+
1049+            // loop though all outputs
1050+            for(const CTxOut& txout: tx.vout) {

sipa commented at 6:38 am on July 26, 2017:

Nit: space after for

jnewbery commented at 9:15 am on July 26, 2017:

fixed

jnewbery force-pushed on Jul 26, 2017

jnewbery commented at 7:09 am on July 26, 2017: member

Reworked with stderr capture/checking done within the test.

Also addressed @MarcoFalke’s nit and rebased.

in test/functional/keypool-restore.py:101 in ec59b2eaac outdated

 96+                # node1 should shutdown because it can't topup its keypool
 97+                self.bitcoind_processes[1].wait(2)
 98+
 99+                log_stderr.seek(0)
100+                stderr = log_stderr.read().decode('utf-8')
101+                assert "Number of keys in keypool is below critical minimum and the wallet is encrytped. Bitcoin will now shutdown to avoid loss of funds." in stderr

laanwj commented at 8:45 am on July 26, 2017:

“encrytped”

jnewbery commented at 9:14 am on July 26, 2017:

fixed

sipa commented at 8:53 am on July 26, 2017: member

utACK ec59b2eaaccd4d42d98345e9719265f102494e46

jnewbery force-pushed on Jul 26, 2017

TheBlueMatt commented at 7:15 pm on July 26, 2017: member

As it is this looks to me to be a major performance regression due to calling GetAllReserveKeys in every matching AddToWalletIfInvolvingMe. #10238 is a relatively simple patch to avoid the massive disk churn, but would need rebase. Alternatively, it would be an equivalently simple change to just keep the reserved keys map created in GetAllReserveKeys in memory, and probably even remove more lines than it adds.

ryanofsky commented at 8:10 pm on July 26, 2017: member

As it is this looks to me to be a major performance regression due to calling GetAllReserveKeys in every matching AddToWalletIfInvolvingMe. #10238 is a relatively simple patch to avoid the massive disk churn, but would need rebase. Alternatively, it would be an equivalently simple change to just keep the reserved keys map created in GetAllReserveKeys in memory, and probably even remove more lines than it adds.

https://github.com/ryanofsky/bitcoin/commit/79e2411e6fcc544326a02de2b0b5756c17c3531f is a straightforward, though not very good-looking way of avoiding GetAllReserveKeys calls on each matching transaction. Probably a nicer approach would be to keep an updated map in memory like you suggest, but I wanted to post this just in case the other approach leads to bugs or delays.

in src/wallet/wallet.h:85 in c3c9224c04 outdated

81@@ -82,6 +82,7 @@ class CTxMemPool;
82 class CBlockPolicyEstimator;
83 class CWalletTx;
84 struct FeeCalculation;
85+struct ReserveKey { int64_t index; bool internal; };

morcos commented at 8:24 pm on July 26, 2017:

could you rename this to be something that isn’t so easily confused with CReserveKey

jnewbery commented at 4:46 am on July 27, 2017:

any suggestions? Is ReserveKeyIndex better? ReserveKeyIndexChain?

morcos commented at 8:46 pm on July 26, 2017: member

This seems a bit like a hack, but what about instead of the sequence of operations currently recommended:

0" - restart bitcoin with -keypoolcritical set to 0\n"
1" - unlock the wallet using walletpassphrase\n"
2" - restart with -rescan. This will redownload the blockchain if you are running "

We instead changed the check for updating SetBestBlock to recognize the condition of the first restart and continue to not update the best block in the wallet. This could detect a separate command line argument such as -bypasskeypoolcritical which is used in that check and the shutdown check.

Then the second restart would not require a rescan.

So instructions are:

0" - restart bitcoin with -bypasskeypoolcritical\n"
1" - unlock the wallet using walletpassphrase\n"
2" - restart without -bypasskeypoolcritical. This might redownload the blockchain if you are running"

Suppose there is still a pruning edge case?

TheBlueMatt commented at 8:58 pm on July 26, 2017: member

I did https://github.com/TheBlueMatt/bitcoin/commits/2017-07-keypool-topup-no-perf-regression to fix the performance regression, it looks fine to me (note minor changes in “[wallet] Add keypoolcritical” which is a bit slower - now must read keypool before erasing, but it shouldn’t be nearly as bad and #10238 should fix it) and is really pretty simple.

ryanofsky commented at 9:06 pm on July 26, 2017: member

We instead changed the check for updating SetBestBlock to recognize the condition of the first restart and continue to not update the best block in the wallet. This could detect a separate command line argument such as -bypasskeypoolcritical which is used in that check and the shutdown check.

Maybe we could avoid the need for a -bypasskeypoolcritical command line option with a CWallet “bool m_update_best_block” member that starts off true, gets set to false whenever the number of keys falls below GetArg("-keypoolmin"), and only gets set to back to true when there is a rescan from at or before the best block while there are more than -keypoolmin keys.

sipa commented at 0:41 am on July 27, 2017: member

As it is this looks to me to be a major performance regression due to calling GetAllReserveKeys in every matching AddToWalletIfInvolvingMe. #10238 is a relatively simple patch to avoid the massive disk churn, but would need rebase.

Ouch, I wasn’t aware that GetAllReserveKeys read all the keys from disk. #10283 looks like the right solution, and shouldn’t be hard to rebase?

TheBlueMatt commented at 0:46 am on July 27, 2017: member

I don’t believe #10238 fixes the issue directly (doesn’t provide the bidirectional mapping, I was incorrect), but is also a nice cleanup.

On July 26, 2017 8:42:03 PM EDT, Pieter Wuille notifications@github.com wrote:

As it is this looks to me to be a major performance regression due to calling GetAllReserveKeys in every matching AddToWalletIfInvolvingMe. #10238 is a relatively simple patch to avoid the massive disk churn, but would need rebase.

Ouch, I wasn’t aware that GetAllReserveKeys read all the keys from disk. #10283 looks like the right solution, and shouldn’t be hard to rebase?

– You are receiving this because you commented. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/10882#issuecomment-318223556

jnewbery force-pushed on Jul 27, 2017

jnewbery commented at 6:42 am on July 27, 2017: member

Thanks @TheBlueMatt . I’ve force pushed your branch here. @morcos - I’ve added your suggested bypasskeypoolcritical option. Can you take a look at the [wallet] add bypasskeypoolcritical commit and see if it captures what you were suggesting accurately? @ryanofsky - I think your solution would involve storing a new key in the wallet DB, since at the moment the only way to rescan is to restart the node. Am I understanding correctly?

ryanofsky commented at 10:20 am on July 27, 2017: member

@ryanofsky - I think your solution would involve storing a new key in the wallet DB, since at the moment the only way to rescan is to restart the node. Am I understanding correctly?

No, you don’t need to add anything new to the database, best block is all you need (because bitcoin automatically rescans from best block on startup, and you can initialize m_update_best_block based on keypool size). I included the part about setting m_update_best_block back to true again when there’s a rescan with sufficient keys just because in the future we will have good rescan RPC. For now, the only way to get m_update_best_block set back to true would be to restart like the instructions say (or import a key in some clever way to trigger a rescan).

in src/wallet/wallet.cpp:1054 in 00b3b74fd8 outdated

1045@@ -1006,6 +1046,33 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransactionRef& ptx, const CBlockI
1046         if (fExisted && !fUpdate) return false;
1047         if (fExisted || IsMine(tx) || IsFromMe(tx))
1048         {
1049+            /* Check if any keys in the wallet keypool that were supposed to be unused
1050+             * have appeared in a new transaction. If so, remove those keys from the keypool.
1051+             * This can happen when restoring an old wallet backup that does not contain
1052+             * the mostly recently created transactions from newer versions of the wallet.
1053+             */
1054+            std::map<CKeyID, int64_t> keyPool = GetAllReserveKeys();

morcos commented at 4:13 pm on July 27, 2017:

Access the member directly to avoid a copy?

Also this will avoid running out of the keypool inside of one scriptPubKey :)

jnewbery commented at 7:44 am on July 28, 2017:

I believe Matt’s commit 64dc2dddf52a48c9cef158951f46395683a26595 resolves your concern about copying. GetAllReserveKeys() returns a const reference

I don’t understand your comment about running out of the keypool. Is it still an issue?

morcos commented at 9:54 am on July 28, 2017:

No my concern still exists. It returns a const reference but then you immediately copy it to your local variable. Also in that same code path you call TopUpKeyPool so now there will be more reserved keys but the next time you lookup you are still looking up in your stale copy

in src/wallet/wallet.cpp:3648 in 00b3b74fd8 outdated

3657+                                      "This is probably because you are restoring an old backup wallet file which has not been topped up with the most recently "
3658+                                      "derived keys, and so you would not detect transactions involving those keys.\n"
3659+                                      "You can manually top-up your wallet keypool as follows:\n"
3660+                                      " - restart bitcoin with -bypasskeypoolcritical\n"
3661+                                      " - unlock the wallet using walletpassphrase\n"
3662+                                      " - restart without -bypasskeypoolcritical. This may redownload the blockchain if you are running a pruned node.";

morcos commented at 4:31 pm on July 27, 2017:

Maybe: “- restart without -bypasskeypoolcritical.” followed by: “NOTE: if you have a pruned node that prunes blocks your wallet hasn’t scanned yet when restarting with -bypasskeypoolcritical then you may need to do a complete reindex. Consider raising the prune limit temporarily for both restarts to avoid this.”

jnewbery commented at 7:52 am on July 28, 2017:

Yes, makes sense. Changed

morcos approved

morcos commented at 4:33 pm on July 27, 2017: member

This is looking pretty good to me.

jnewbery force-pushed on Jul 28, 2017

jnewbery commented at 7:56 am on July 28, 2017: member

@TheBlueMatt - I don’t fully understand the implications of m_pool_key_to_id and returning a const references to the internal cache in GetAllReserveKeys(). For example, Cory has suggested that this might not be thread-safe, but perhaps that’s ok in wallet code?

Reviewers who have previously ACKed this PR - please be aware of the additional commits.

morcos commented at 9:53 am on July 28, 2017: member

@jnewbery I just kind of assumed it was protected by cs_wallet, but we should verify.

No my concern still exists. It returns a const reference but then you immediately copy it to your local variable. Also in that same code path you call TopUpKeyPool so now there will be more reserved keys but the next time you lookup you are still looking up in your stale copy

TheBlueMatt commented at 1:57 pm on July 30, 2017: member

@jnewbery heh, yea, I just generally always assume everything in wallet takes LOCK2(cs_main, cs_wallet). The GetAllReserveKeys call in rpcdump as well as AddToWalletIfInvolvingMe both take/require cs_wallet, so it looks fine to me?

jnewbery commented at 0:08 am on July 31, 2017: member

@morcos - please look at commit fixup: don’t make local copy of m_pool_key_to_id. I think that addresses your point above. @TheBlueMatt - yes, all access to m_pool_key_to_id is protected by cs_wallet. The only place that isn’t explicit is in LoadKeyPool(). I’ve added an AssertLockHeld() in fixup: explicitly check lock is held in LoadKeyPool.

I’ll squash both commits if you’re happy with them.

morcos commented at 3:10 pm on July 31, 2017: member

@jnewbery looks good i think!

in src/wallet/wallet.cpp:3642 in c83673b9ec outdated

3637+                                      " - restart bitcoin with -keypoolcritical set to 0\n"
3638+                                      " - unlock the wallet using walletpassphrase\n"
3639+                                      " - restart with -rescan. This will redownload the blockchain if you are running a pruned node.";
3640+        uiInterface.ThreadSafeMessageBox(error_msg, "", CClientUIInterface::MSG_ERROR);
3641+        StartShutdown();
3642+        throw std::runtime_error(error_msg);

TheBlueMatt commented at 6:02 pm on August 1, 2017:

Wait, why is this safe? I’m both highly skeptical that throwing here doesn’t blow up all kinds of potential callers and that this is neccessary. Better to just call StartShutdown() and then avoid writing wallet best block.

in src/wallet/wallet.cpp:4053 in c83673b9ec outdated

4048+                LogPrintf("Parameter Interaction: keypool size (%d) must be larger than keypool critical size for encrypted wallets (%d)\n", keypool_size, keypool_critical);
4049+                ForceSetArg("-keypool", std::to_string(keypool_critical));
4050+            }
4051+        } else {
4052+            if (keypool_size < keypool_critical) {
4053+                InitWarning(strprintf(_("Your keypool is configured to store %d keys, which is below the keypool critical size of %d. Using a larger keypool will make it less "

TheBlueMatt commented at 6:08 pm on August 1, 2017:

I think this needs to be more than a warning, no? There is no check in ShutdownIfKeypoolCritical to only exit if we failed to top up (ie if we hit this warning, the first time we see ShutdownIfKeypoolCritical we will exit, no?

morcos commented at 7:08 pm on August 1, 2017:

agreed, or we need the parameter interaction to increase keypool to keypool_critical? or something?

TheBlueMatt commented at 7:11 pm on August 1, 2017:

Yea, think I’d vote for a paramter interaction.

jnewbery commented at 3:59 pm on August 2, 2017:

If we hit this warning, then the wallet is not encrypted, and TopUpKeyPool() should succeed.

If the wallet is encrypted we’ll hit the branch above and we force set keypool to keypool_critical.

jnewbery commented at 4:20 pm on August 2, 2017:

oh, I see. Yes, I’ll change the parameter interaction to set keypool to keypool_critical for all wallets.

in src/wallet/wallet.cpp:3990 in 667f98454e outdated

3926@@ -3922,6 +3927,7 @@ std::string CWallet::GetWalletHelpString(bool showDebug)
3927     {
3928         strUsage += HelpMessageGroup(_("Wallet debugging/testing options:"));
3929 
3930+        strUsage += HelpMessageOpt("-bypasskeypoolcritical", _("Bypass keypool critical limit. Don't shutdown the node if keypool drops below keypoolcritical limit (but don't advance BestBlock)"));

TheBlueMatt commented at 6:12 pm on August 1, 2017:

The doc is wrong (we do advance the best block), but I prefer the doc version, so maybe just remove the check in SetBestChain.

morcos commented at 6:52 pm on August 1, 2017:

huh? @TheBlueMatt

TheBlueMatt commented at 7:10 pm on August 1, 2017:

Oh, guess the doc is right, but just super confusing (double negative-ish, but not really).Maybe “but still don’t advance”?

jnewbery commented at 5:02 pm on August 2, 2017: member

@TheBlueMatt I’ve pushed a fixup commit that addresses your two comments. If you’re happy I’ll squash.

jnewbery force-pushed on Aug 2, 2017

jnewbery commented at 7:07 pm on August 2, 2017: member

@TheBlueMatt commits squashed. Ready for final review.

jnewbery force-pushed on Aug 2, 2017

morcos commented at 7:29 pm on August 2, 2017: member

~utACK 7cca786~ ~utACK 9858566~ ~utACK 7cca786 (don’t ask)~ utACK 85079e6

jnewbery commented at 7:29 pm on August 2, 2017: member

Unrebased at @morcos’s request. Presquahed branch is here: https://github.com/jnewbery/bitcoin/releases/tag/pr10882.4

jnewbery force-pushed on Aug 2, 2017

jnewbery commented at 9:34 pm on August 2, 2017: member

rebased on master. Don’t ask

TheBlueMatt commented at 11:19 pm on August 2, 2017: member

utACK 85079e6ab3e7d0596c010df22256ccb3c2a9be03 (minus the test bits)

in src/wallet/wallet.cpp:4119 in 60e440076d outdated

4052@@ -3984,6 +4053,19 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
4053 
4054     RegisterValidationInterface(walletInstance);
4055 
4056+    // Make sure we have enough keys in our keypool if HD is enabled
4057+    if (walletInstance->IsHDEnabled()) {
4058+        unsigned int keypool_size = GetArg("-keypool", DEFAULT_KEYPOOL_SIZE);
4059+        unsigned int keypool_critical = GetArg("-keypoolcritical", DEFAULT_KEYPOOL_CRITICAL);
4060+        if (keypool_size < keypool_critical) {

instagibbs commented at 1:01 am on August 3, 2017:

nit: current logic is “larger or equal to”

ryanofsky commented at 7:36 pm on August 3, 2017:

Thread #10882 (review)

This particular line seems correct, but I guess the printf below should be updated.

jnewbery commented at 3:12 pm on August 4, 2017:

yes - fixed

ryanofsky commented at 7:20 pm on August 10, 2017:

Thread #10882 (review)

I don’t see a change, should log print just be changed to “smaller or equal to”?

instagibbs commented at 1:09 am on August 3, 2017: member

utACK https://github.com/bitcoin/bitcoin/commit/85079e6ab3e7d0596c010df22256ccb3c2a9be03

gmaxwell commented at 7:35 am on August 3, 2017: contributor

I’m still not clear on what prevent a forced shutdown under this pattern: start a new node, sync, encrypt wallet, run getnewaddress 501 times, get a payment. Where is it distinguishing the keypool going under critical while behind vs while current?

jnewbery commented at 10:02 am on August 3, 2017: member

@gmaxwell - currently getnewaddress will fail if the keypool has run out. How about we change it so that it fails if it hits the keypool_critical limit?

in test/functional/keypool-restore.py:9 in 85079e6ab3 outdated

0@@ -0,0 +1,121 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2017 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test HD Wallet keypool restore function.
6+
7+Two nodes. Node1 is under test. Node0 is providing transactions and generating blocks.
8+
9+Repeat test twice, once with encrypted wallet and once with unencrypted wallet:

ryanofsky commented at 6:54 pm on August 3, 2017:

In commit “[wallet] [tests] Add keypool restore functional test”

It would be nice to have tests for non-hd wallet behavior too, since it’s easy to forget about non-hd case when changing this code and accidentally break something. I think this could be as simple as adding a boolean hd argument to _test_restore, and calling it with true and false. If it’s more complicated I would maybe add a comment about this being a possible way to extend the test in the future.

HD split could be another test variant

jnewbery commented at 9:35 pm on August 4, 2017:

I think this can be added later.

gmaxwell commented at 7:03 pm on August 3, 2017: contributor

@jnewbery That seems reasonable to me!

in src/wallet/wallet.h:980 in b90ba239ac outdated

973@@ -973,10 +974,10 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
974     bool TopUpKeyPool(unsigned int kpSize = 0);
975     void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool fRequestedInternal);
976     void KeepKey(int64_t nIndex);
977-    void ReturnKey(int64_t nIndex, bool fInternal);
978+    void ReturnKey(int64_t nIndex, bool fInternal, const CPubKey& pubkey);
979     bool GetKeyFromPool(CPubKey &key, bool internal = false);
980     int64_t GetOldestKeyPoolTime();
981-    void GetAllReserveKeys(std::set<CKeyID>& setAddress) const;
982+    const std::map<CKeyID, int64_t>& GetAllReserveKeys() const { return m_pool_key_to_id; }

ryanofsky commented at 7:18 pm on August 3, 2017:

In commit “Cache keyid -> keypool id mappings”

Would be better to call this m_pool_key_to_index instead of m_pool_key_to_id because we already have key ids (which are hashes, not ordinals), and we are already using index/nIndex to refer to the ordinals in other places.

jnewbery commented at 3:10 pm on August 4, 2017:

agree - changed

in src/wallet/wallet.cpp:3652 in 60e440076d outdated

3647+                  __func__, setInternalKeyPool.size(), setExternalKeyPool.size(), keypool_critical);
3648+        const std::string error_msg = "Number of keys in keypool is below critical minimum and the wallet is encrypted. Bitcoin will now shutdown to avoid loss of funds.\n"
3649+                                      "This is probably because you are restoring an old backup wallet file which has not been topped up with the most recently "
3650+                                      "derived keys, and so you would not detect transactions involving those keys.\n"
3651+                                      "You can manually top-up your wallet keypool as follows:\n"
3652+                                      " - restart bitcoin with -keypoolcritical set to 0\n"

ryanofsky commented at 7:26 pm on August 3, 2017:

In commit “[wallet] Add keypoolcritical”

Maybe add (to prevent bitcoin from shutting down again)

jnewbery commented at 3:15 pm on August 4, 2017:

added

in src/wallet/wallet.cpp:3653 in 60e440076d outdated

3648+        const std::string error_msg = "Number of keys in keypool is below critical minimum and the wallet is encrypted. Bitcoin will now shutdown to avoid loss of funds.\n"
3649+                                      "This is probably because you are restoring an old backup wallet file which has not been topped up with the most recently "
3650+                                      "derived keys, and so you would not detect transactions involving those keys.\n"
3651+                                      "You can manually top-up your wallet keypool as follows:\n"
3652+                                      " - restart bitcoin with -keypoolcritical set to 0\n"
3653+                                      " - unlock the wallet using walletpassphrase\n"

ryanofsky commented at 7:28 pm on August 3, 2017:

In commit “[wallet] Add keypoolcritical”

Maybe add (to refill the keypool)

jnewbery commented at 3:15 pm on August 4, 2017:

added

jnewbery commented at 7:59 pm on August 3, 2017: member

I’ve added three new commits:

add HasUnusedKeys() helper is just a refactor
return error from getnewaddress if keypool drops to critical prevents getnewaddress from returning a new address if the keypool drops to critical
add checks for keypool critical does the same for the other places that GetKeyFromPool() is called

I’m not sure if we want (3). It can be omitted if not required.

in src/wallet/wallet.cpp:422 in 2503ec6974 outdated

416@@ -417,6 +417,12 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
417 
418 void CWallet::SetBestChain(const CBlockLocator& loc)
419 {
420+    LOCK(cs_wallet); //nWalletMaxVersion
421+    unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
422+    if (IsHDEnabled() && ((CanSupportFeature(FEATURE_HD_SPLIT) && setInternalKeyPool.size() < keypool_min) || (setExternalKeyPool.size() < keypool_min))) {

ryanofsky commented at 8:02 pm on August 3, 2017:

In commit “[wallet] Add keypoolmin”

As alluded to in #10882 (comment) and #10882 (comment), this check is not as safe as it appears, because if keypool gets drained below the minimum, then is refilled later after unlocking, there might be missing transactions which will not get rescanned in the interval of time where the keypool was below the minimum. I would at least add a comment here mentioning this, if you don’t want to add an m_update_best_block bool to fix it.

By the way, another advantage of adding a m_update_best_block bool is that it would let us print a log message when the keypool falls below the minimum (right now there is no indication, and we probably don’t want to add a log statement here since it would spam the log).

jnewbery commented at 5:31 pm on August 4, 2017:

Ah, your allusion was a bit subtle for me. I thought your original comment was “this is an alternative way to do it” rather than “the current way is unsafe. You should do this instead”

Can you take a look at https://github.com/jnewbery/bitcoin/commit/145a06521bd458e214cd9c8d163c9a1ebbd63189 . Is that what you had in mind?

With that change, I think we can remove bypasskeypoolcritical and update the recovery instructions to say set -keypool_critical=0. Do you agree?

ryanofsky commented at 7:03 pm on August 4, 2017:

Thread #10882 (review)

Posted a question in https://github.com/jnewbery/bitcoin/commit/145a06521bd458e214cd9c8d163c9a1ebbd63189, but I think this seems basically right. Few things

I don’t think this change necessarily needs to be part of this PR if it will delay the merge. Could be done as a followup.
It seems like it would be a little safer (though maybe equivalent) and more logical to change m_update_best_block from true to false in AddToWalletIfInvolvingMe between TopUpKeyPool and ShutdownIfKeypoolCritical, since similar checks are being done there, and it would make it obvious that if there’s any dip in keypool size while transactions are being processed, the flag will flip.
When I talked about this with Alex he wanted to keep -bypasskeypoolcritical even if -keypool_critical=0 can be made safe & equivalent, because it makes purpose of the flag more explicit.
Would be good to use the HasUnusedKeys helper to simplify the logic.

jnewbery commented at 9:35 pm on August 4, 2017:

Done (in fact I’ve added the change to m_update_best_block false in AddToWalletIfInvolvingMe() and SetBestChain())

ryanofsky commented at 8:07 pm on August 3, 2017: member

utACK 85079e6ab3e7d0596c010df22256ccb3c2a9be03. This seems fine to merge. I made some suggestions but they could be addressed separately.

in src/qt/addresstablemodel.cpp:373 in 98a1e38a65 outdated

369@@ -369,17 +370,14 @@ QString AddressTableModel::addRow(const QString &type, const QString &label, con
370     {
371         // Generate a new address to associate with given label
372         CPubKey newKey;
373-        if(!wallet->GetKeyFromPool(newKey))
374-        {
375+        if (!wallet->GetKeyFromPool(newKey) || (wallet->IsHDEnabled() && !wallet->HasUnusedKeys(GetArg("-keypoolcritical", DEFAULT_KEYPOOL_CRITICAL)))) {

ryanofsky commented at 8:19 pm on August 3, 2017:

In commit “[wallet] add checks for keypool critical”

It seems like a less repetitive (and safer?) approach would be to make GetKeyFromPool return false when number of keys is below critical, and maybe have an override if needed.

jnewbery commented at 9:08 pm on August 4, 2017:

ok, done

in src/wallet/rpcwallet.cpp:164 in 58c26e85dd outdated

160@@ -161,8 +161,8 @@ UniValue getnewaddress(const JSONRPCRequest& request)
161 
162     // Generate a new key that is added to wallet
163     CPubKey newKey;
164-    if (!pwallet->GetKeyFromPool(newKey)) {
165-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");
166+    if (!pwallet->GetKeyFromPool(newKey) || (pwallet->IsHDEnabled() && !pwallet->HasUnusedKeys(GetArg("-keypoolcritical", DEFAULT_KEYPOOL_CRITICAL)))) {

ryanofsky commented at 8:23 pm on August 3, 2017:

In commit “[wallet] return error from getnewaddress if keypool drops to critical”

Should keypool size be checked before calling GetKeyFromPool instead of after, to avoid getting a key and throwing it away?

ryanofsky commented at 8:30 pm on August 3, 2017:

In commit “[wallet] return error from getnewaddress if keypool drops to critical”

Maybe compare against -keypoolmin instead of -keypoolcritical to be more conservative, and because otherwise this change will have no effect (because the node will be shut down) unless -bypasskeypoolcritical happens to be in use. Also I think it’s good just for sake of simplicity if -bypasskeypoolcritical and -keypoolcritical=0 are synonyms for the same thing. Here you are introducing a subtle difference.

Either way, -keypoolmin or -keypoolcritical documentation could be updated to mention this new behavior, too.

jnewbery commented at 9:10 pm on August 4, 2017:

I originally wasn’t doing this because GetKeyFromPool() calls into ReserveKeyFromKeyPool() which tops up the keypool. I didn’t want to fail GetKeyFromPool() when the wallet is unlocked and able to top up the keypool.

I’ve now added a TopUpKeyPool() call in GetKeyFromPool(), so we try to top up before checking that the keypool hasn’t fallen to the critical level.

jnewbery commented at 9:11 pm on August 4, 2017:

Changed the check to HasUnusedKeys(GetArg("-keypoolcritical", DEFAULT_KEYPOOL_CRITICAL) + 1), which will prevent us from dropping to the keypool_critical threshold.

ryanofsky commented at 8:35 pm on August 3, 2017: member

utACK 98a1e38a658e6c500b0cb9f979118f8f21733aa0. Only change since last review is GetKeyFromPool being disallowed on keypool critical condition.

ryanofsky commented at 8:39 pm on August 3, 2017: member

I’m not sure if we want (3). It can be omitted if not required.

Seems good to me to disallow GetKeyFromPool() generally like this is doing. getrawchangeaddress RPC is another thing to consider disabling.

jnewbery force-pushed on Aug 4, 2017

jnewbery commented at 9:07 pm on August 4, 2017: member

Pushed a new branch, which I think should address all of @ryanofsky’s review comments.

Old branch here for comparison: https://github.com/jnewbery/bitcoin/releases/tag/pr10882.5

jnewbery force-pushed on Aug 4, 2017

laanwj commented at 11:52 am on August 5, 2017: member

Various issues with Travis have appeared after the last push. (I’ve tried restarting the build and it didn’t change anything)

ryanofsky commented at 12:05 pm on August 5, 2017: member

I think this should just be rolled back and merged as 85079e6, which got a bunch of ACKs, and was straightforward, and did the job. The improvements that have been made since then are nice, but fairly minor, and backwards compatible, so they could be done in a separate PR.

laanwj commented at 6:53 am on August 7, 2017: member

@ryanofsky Agree. In any case, we’ll need to do something here today if this is still to make 0.15.0rc1.

jonasschnelli commented at 6:54 am on August 7, 2017: contributor

Agree with @ryanofsky. utACK the older https://github.com/bitcoin/bitcoin/commit/85079e6ab3e7d0596c010df22256ccb3c2a9be03

jnewbery force-pushed on Aug 7, 2017

jnewbery commented at 1:17 pm on August 7, 2017: member

ok, reverted to https://github.com/bitcoin/bitcoin/commit/85079e6ab3e7d0596c010df22256ccb3c2a9be03

Other branch is here: https://github.com/jnewbery/bitcoin/releases/tag/pr10882.6 . In that branch I fixed one problem with travis (I accidentally removed the bypasscritical help text in the rebase). Other problem was import-rescan.py failing, which I wasn’t able to fix.

ryanofsky commented at 3:12 pm on August 7, 2017: member

Seems like this can be merged now.

jnewbery force-pushed on Aug 8, 2017

ryanofsky commented at 3:28 pm on August 8, 2017: member

From discussion in IRC, this isn’t going to be merged as is because it will caused bad upgrade experience (immediately shutting down) for older locked HD wallets with less than 500 keys. John is implementing a solution “remove [ShutdownIfKeypoolCritical call] from CreateWalletFromFile(), and only call it from AddToWalletIfInvolvingMe() if that function was called by SyncTransaction() (and not by ScanForWalletTransactions())” https://botbot.me/freenode/bitcoin-core-dev/msg/89554129/.

This change seems fine to me, though I wonder if it won’t just delay the shutdown instead of causing an immediate shutdown?

Another way to solve this problem to would be delay -keypoolcritical enforcement on an older database until it is unlocked. E.g. add a new ENFORCE_KEYPOOL_CRITICAL flag to the database, and only enforce the critical minimum when it is present. If not present, add the flag to the database when the wallet is unlocked (or on startup if the wallet is unencrypted).

ryanofsky commented at 1:42 pm on August 9, 2017: member

This appears to still be in flux

From https://botbot.me/freenode/bitcoin-core-dev/msg/89571365/

<jnewbery> I’ve pushed a branch which I think covers gmaxwell’s requested logic: - don’t shutdown the node on startup if keypool is < keypool_critical - don’t shutdown the node if wallet is current - DO shutdown the node if rescanning and keypool drops below keypool_critical

From https://botbot.me/freenode/bitcoin-core-dev/msg/89581944/

<gmaxwell> jnewbery: I really think we should split the PR and merge the mark-used and topup if possible stuff, and make the shutdown and don’t update parts a seperate PR. The first part is done, solves the issue for unlocked wallets. And even for locked wallets results in rescan instructions that we can give to succesfully rescan “(unlock the wallet with a long time, run rescan)”… and doesn’t have an risk of making things worse for anyone.

jnewbery force-pushed on Aug 10, 2017

jnewbery commented at 2:19 pm on August 10, 2017: member

re-ordered commits. The only difference with the new branch is renaming keypool-restore.py to keypool-topup.py, and calling TopUpKeyPool() on wallet load for all wallets (not just HD wallets). Old branch is here: https://github.com/jnewbery/bitcoin/releases/tag/pr10882.7

jnewbery commented at 2:27 pm on August 10, 2017: member

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.

jnewbery renamed this:
~~Keypool topup~~
Stop advancing best block and shutdown node if keypool drops below critical threshold
on Aug 10, 2017

ryanofsky commented at 4:30 pm on August 10, 2017: member

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

jnewbery force-pushed on Aug 10, 2017

in src/wallet/wallet.cpp:1680 in 4444d84f4b outdated

1643@@ -1644,6 +1644,10 @@ CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, bool f
1644 
1645         fScanningWallet = false;
1646     }
1647+
1648+    // Check that we haven't dropped below the keypool_critical threshold.
1649+    ShutdownIfKeypoolCritical();

ryanofsky commented at 6:39 pm on August 10, 2017:

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.

jnewbery commented at 6:56 pm on August 10, 2017:

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.

ryanofsky commented at 7:13 pm on August 10, 2017:

Thread #10882 (review)

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.

in src/wallet/rpcwallet.cpp:164 in b622d038fc outdated

160@@ -161,8 +161,8 @@ UniValue getnewaddress(const JSONRPCRequest& request)
161 
162     // Generate a new key that is added to wallet
163     CPubKey newKey;
164-    if (!pwallet->GetKeyFromPool(newKey)) {
165-        throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first");
166+    if (!pwallet->GetKeyFromPool(newKey, false, true)) {

ryanofsky commented at 7:09 pm on August 10, 2017:

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))

MarcoFalke removed this from the milestone 0.15.0 on Aug 10, 2017

MarcoFalke commented at 7:12 pm on August 10, 2017: member

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

in src/wallet/wallet.cpp:1676 in 5a5b858cc1 outdated

1669+    // If the keypool has dropped below -keypoolmin, then stop updating the bestblock height. We can rescan later once the wallet is unlocked.
1670+    unsigned int keypool_min = GetArg("-keypoolmin", DEFAULT_KEYPOOL_MIN);
1671+    if (IsHDEnabled() && !HasUnusedKeys(keypool_min) && m_update_best_block) {
1672+        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);
1673+        LogPrintf("Unlock wallet, top up keypool and rescan to resume watching for new transactions.\n");
1674+        m_update_best_block = false;

ryanofsky commented at 7:33 pm on August 10, 2017:

In commit “[wallet] Add m_update_best_block”

As mentioned #10882 (review), 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.

laanwj referenced this in commit 653a46dd91 on Aug 14, 2017

[wallet] Add keypoolcritical

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.

ebf3133930

[wallet] Add keypoolmin

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

02d69ae43d

[wallet] Add m_update_best_block

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.

60c74c3a4a

[wallet] Add bypasskeypoolcritical

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.

72f0bff4fb

[wallet] Add fail_on_critical to GetKeyFromPool()

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.

40ce25c812

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

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.

1a916edb8e

[wallet] Don't let keypool drop below critical

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

e29ea7eac8

[wallet] [tests] Add keypool critical functional test

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)

3140a973c3

jnewbery force-pushed on Aug 14, 2017

jnewbery commented at 3:50 pm on August 14, 2017: member

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 renamed this:
~~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
on Aug 14, 2017

jnewbery commented at 6:26 pm on August 29, 2017: member

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.

jnewbery closed this on Aug 29, 2017

droark commented at 10:04 am on December 9, 2017: contributor

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

jnewbery commented at 3:35 pm on December 9, 2017: member

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

PastaPastaPasta referenced this in commit 546865aca1 on Sep 18, 2019

PastaPastaPasta referenced this in commit f1312e1539 on Sep 20, 2019

PastaPastaPasta referenced this in commit c438c9322f on Sep 20, 2019

barrystyle referenced this in commit 2cf9a16f02 on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

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