Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
@jnewbery
Copy link
Member

jnewbery commented Jul 19, 2017

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

@jnewbery jnewbery referenced this pull request Jul 19, 2017

Closed

[WIP] [wallet] keypool restore #10830

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

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

keypoolemin seems misspelled

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Jul 19, 2017

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

# self.nodes[0].generate(1)
self.sync_all()

assert_equal(self.nodes[1].getbalance(), 15)

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

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

This comment has been minimized.

@jnewbery

jnewbery Jul 19, 2017

Author Member

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.

if (id > foundIndex) break; // set*KeyPool is ordered

CKeyPool keypool;
if (!walletdb.ReadPool(id, keypool)) {

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

Call seems unnecessary since keypool variable isn't used.

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

removed

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."));

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

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

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."));

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

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

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

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

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.

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

yes - fixed

// 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";

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

yup

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

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

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

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

I think you're right. Fixed

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

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

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.

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

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.

}
}

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

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

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

src/wallet/wallet.cpp Outdated
CWalletDB walletdb(*dbw);
for (std::set<int64_t> *setKeyPool : {&setInternalKeyPool, &setExternalKeyPool}) {
int64_t foundIndex = -1;
for (const int64_t& id : *setKeyPool) {

This comment has been minimized.

@ryanofsky

ryanofsky Jul 19, 2017

Contributor

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

done

@jnewbery jnewbery force-pushed the jnewbery:keypool_topup branch Jul 20, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jul 20, 2017

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

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jul 20, 2017

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

@jnewbery jnewbery force-pushed the jnewbery:keypool_topup branch to 965594a Jul 20, 2017

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

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

This comment has been minimized.

@instagibbs

instagibbs Jul 20, 2017

Member

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

is it ok to lock in CheckKeypoolMinSize() instead?

This comment has been minimized.

@instagibbs

instagibbs Jul 20, 2017

Member

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

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

@ryanofsky
Copy link
Contributor

ryanofsky left a comment

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.

}
}

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

This comment has been minimized.

@ryanofsky

ryanofsky Jul 20, 2017

Contributor

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

done

}
}

if (!TopUpKeyPool()) {

This comment has been minimized.

@ryanofsky

ryanofsky Jul 20, 2017

Contributor

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

done

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

This comment has been minimized.

@ryanofsky

ryanofsky Jul 20, 2017

Contributor

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.

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

done

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 "

This comment has been minimized.

@ryanofsky

ryanofsky Jul 20, 2017

Contributor

Maybe explicitly warn about shutdowns, too

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

done

self.num_nodes = 2
self.extra_args = [['-usehd=0'], ['-usehd=1', '-keypool=100', '-keypoolmin=20']]

def run_test(self):

This comment has been minimized.

@ryanofsky

ryanofsky Jul 20, 2017

Contributor

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.

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

done

@jnewbery jnewbery force-pushed the jnewbery:keypool_topup branch from 965594a to 61d604d Jul 20, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jul 20, 2017

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

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Jul 20, 2017

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 laanwj added this to the 0.15.0 milestone Jul 20, 2017

src/wallet/wallet.cpp Outdated
@@ -3523,18 +3582,68 @@ void CReserveKey::ReturnKey()
vchPubKey = CPubKey();
}

void CWallet::CheckKeypoolMinSize() {

This comment has been minimized.

@instagibbs

instagibbs Jul 20, 2017

Member

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

This comment has been minimized.

@jnewbery

jnewbery Jul 20, 2017

Author Member

suggested name?

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jul 20, 2017

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

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Jul 20, 2017

light tACK

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

src/wallet/wallet.cpp Outdated
}
walletInstance->TopUpKeyPool();
}
walletInstance->CheckKeypoolMinSize();

This comment has been minimized.

@instagibbs

instagibbs Jul 20, 2017

Member

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

This comment has been minimized.

@jnewbery

jnewbery Jul 21, 2017

Author Member

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

This comment has been minimized.

@instagibbs

instagibbs Jul 21, 2017

Member

mistaken, ignore

src/wallet/wallet.cpp Outdated
"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.";

This comment has been minimized.

@instagibbs

instagibbs Jul 20, 2017

Member

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.

This comment has been minimized.

@jnewbery

jnewbery Jul 21, 2017

Author Member

What's special about 1500?

This comment has been minimized.

@instagibbs

instagibbs Jul 21, 2017

Member

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

src/wallet/wallet.cpp Outdated
"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.";

This comment has been minimized.

@ryanofsky

ryanofsky Jul 20, 2017

Contributor

~~~This seems safe, but practically speaking shouldn't a manual rescan be unnecessary in most cases, because if the node shuts down when the keypool size is low, restarting should automatically rescan from ReadBestBlock.~~~ EDIT: Never mind, too many race conditions to think about here. Not sure what a good solution is.

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.

This comment has been minimized.

@gmaxwell

gmaxwell Jul 21, 2017

Member

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.

This comment has been minimized.

@ryanofsky

ryanofsky Jul 21, 2017

Contributor

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

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jul 21, 2017

Added two commits:

  1. rename current option keypoolcritical - if the keypool falls below this, then shutdown the node
  2. 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.

src/wallet/wallet.cpp Outdated
@@ -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))) {

This comment has been minimized.

@ryanofsky

ryanofsky Jul 21, 2017

Contributor

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

This comment has been minimized.

@jnewbery

jnewbery Jul 21, 2017

Author Member

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.

src/wallet/wallet.h Outdated
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;

This comment has been minimized.

@ryanofsky

ryanofsky Jul 21, 2017

Contributor

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.

This comment has been minimized.

@jnewbery

jnewbery Jul 21, 2017

Author Member

done

src/wallet/wallet.cpp Outdated
@@ -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() {

This comment has been minimized.

@ryanofsky

ryanofsky Jul 21, 2017

Contributor

Could s/CheckKeypoolMinSize/CheckKeypoolCriticalSize

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

This comment has been minimized.

@jnewbery

jnewbery Jul 21, 2017

Author Member

Good point. Changed name to ShutdownIfKeypoolCritical()

src/wallet/wallet.cpp Outdated
@@ -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));

This comment has been minimized.

@ryanofsky

ryanofsky Jul 21, 2017

Contributor

Need to add -keypoolmin I think too.

This comment has been minimized.

@jnewbery

jnewbery Jul 21, 2017

Author Member

done

@jnewbery jnewbery force-pushed the jnewbery:keypool_topup branch Jul 21, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jul 21, 2017

@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
Copy link
Contributor

ryanofsky left a comment

utACK 89af21ddcd7837c45cf8e261711d227e4f8448db

src/wallet/wallet.cpp Outdated
std::map<CKeyID, ReserveKey> CWallet::GetAllReserveKeys() const
{
std::map<CKeyID, ReserveKey> mapKeyPool;
mapKeyPool.clear();

This comment has been minimized.

@ryanofsky

ryanofsky Jul 21, 2017

Contributor

Could drop clear, map will already be empty

This comment has been minimized.

@jnewbery

jnewbery Jul 21, 2017

Author Member

dropped

@jnewbery jnewbery force-pushed the jnewbery:keypool_topup branch Jul 21, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Jul 21, 2017

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Aug 8, 2017

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

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Aug 9, 2017

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 jnewbery force-pushed the jnewbery:keypool_topup branch from d2d80ae Aug 10, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 10, 2017

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 jnewbery referenced this pull request Aug 10, 2017

Merged

Basic keypool topup #11022

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 10, 2017

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 jnewbery changed the title Keypool topup Stop advancing best block and shutdown node if keypool drops below critical threshold Aug 10, 2017

@ryanofsky

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Aug 10, 2017

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

@jnewbery jnewbery force-pushed the jnewbery:keypool_topup branch Aug 10, 2017

src/wallet/wallet.cpp Outdated
@@ -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();

This comment has been minimized.

@ryanofsky

ryanofsky Aug 10, 2017

Contributor

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.

This comment has been minimized.

@jnewbery

jnewbery Aug 10, 2017

Author Member

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.

This comment has been minimized.

@ryanofsky

ryanofsky Aug 10, 2017

Contributor

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.

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

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Aug 10, 2017

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

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

This comment has been minimized.

@ryanofsky

ryanofsky Aug 10, 2017

Contributor

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

src/wallet/wallet.cpp Outdated
@@ -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();

This comment has been minimized.

@ryanofsky

ryanofsky Aug 10, 2017

Contributor

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.

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

This comment has been minimized.

@ryanofsky

ryanofsky Aug 10, 2017

Contributor

Thread #10882 (comment)

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

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;

This comment has been minimized.

@ryanofsky

ryanofsky Aug 10, 2017

Contributor

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.

laanwj added a commit that referenced this pull request Aug 14, 2017

Merge #11022: Basic keypool topup
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

jnewbery added some commits Jul 18, 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.
[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.
[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.
[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.
[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.
[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.
[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.
[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)

@jnewbery jnewbery force-pushed the jnewbery:keypool_topup branch to 3140a97 Aug 14, 2017

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 14, 2017

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

This comment has been minimized.

Copy link
Member Author

jnewbery commented Aug 29, 2017

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

This comment has been minimized.

Copy link
Contributor

droark commented Dec 9, 2017

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

@jnewbery

This comment has been minimized.

Copy link
Member Author

jnewbery commented Dec 9, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.