This PR contains the first part of #10882 :
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).
cs_LastBlockFile shouldn't be held while calling wallet functions.
This commit adds basic keypool mark-used and topup:
- try to topup the keypool on initial load
- if a key in the keypool is used, mark all keys before that as used and
try to top up
208 | @@ -209,7 +209,7 @@ def start_node(self, i, dirname, extra_args=None, rpchost=None, timewait=None, b 209 | datadir = os.path.join(dirname, "node" + str(i)) 210 | if binary is None: 211 | binary = os.getenv("BITCOIND", "bitcoind") 212 | - args = [binary, "-datadir=" + datadir, "-server", "-keypool=1", "-discover=0", "-rest", "-logtimemicros", "-debug", "-debugexclude=libevent", "-debugexclude=leveldb", "-mocktime=" + str(self.mocktime), "-uacomment=testnode%d" % i] 213 | + args = [binary, "-datadir=" + datadir, "-server", "-keypool=1", "-keypoolcritical=0", "-discover=0", "-rest", "-logtimemicros", "-debug", "-debugexclude=libevent", "-debugexclude=leveldb", "-mocktime=" + str(self.mocktime), "-uacomment=testnode%d" % i]
In commit "[wallet] keypool mark-used and topup"
It looks like these test changes got added to the wrong commit.
Thanks Russ. You're right, this was in the wrong commit and shouldn't be in this PR. Fixed
3610 | @@ -3611,6 +3611,11 @@ void CReserveKey::ReturnKey() 3611 | vchPubKey = CPubKey(); 3612 | } 3613 | 3614 | +bool CWallet::HasUnusedKeys(int min_keys) const
In commit "[wallet] Add HasUnusedKeys() helper"
Should use size_t instead of int to silence some warnings
wallet/wallet.cpp: In member function ‘bool CWallet::HasUnusedKeys(int) const’:
wallet/wallet.cpp:3663:38: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
return setExternalKeyPool.size() >= min_keys && (setInternalKeyPool.size() >= min_keys || !CanSupportFeature(FEATURE_HD_SPLIT));
^
wallet/wallet.cpp:3663:80: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
return setExternalKeyPool.size() >= min_keys && (setInternalKeyPool.size() >= min_keys || !CanSupportFeature(FEATURE_HD_SPLIT));
3651 | + 3652 | + CKeyPool keypool; 3653 | + if (walletdb.ReadPool(index, keypool)) { //TODO: This should be unnecessary 3654 | + m_pool_key_to_index.erase(keypool.vchPubKey.GetID()); 3655 | + } 3656 | + walletdb.ErasePool(index);
In commit "[wallet] keypool mark-used and topup"
An older version of this PR was calling KeepKey instead of doing this manually. It'd be nice to either call KeepKey here, or add the "keypool keep" log print here from that method. Logging about the key being kept would help debugging and make this code clearer.
Slightly tested ACK d34957e17e8c9740104533aaf4a896e93548c87e. Confirmed disabling topup breaks the new test. Would be nice to get this merged, because this makes updating unlocked wallets a lot safer, and because merging it would considerably simplify #10882.
utACK
utACK d34957e17e8c9740104533aaf4a896e93548c87e
utACK
Thanks for the code review @ryanofsky . This branch now has some ACKs so I'm going to hold off making any additional changes (unless others think them necessary)
I'll happily review and ACK a cleanup PR after v0.15
utACK d34957e17e8c9740104533aaf4a896e93548c87e
:tada:
This brings two new warnings:
wallet/wallet.cpp:3668:38: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
return setExternalKeyPool.size() >= min_keys && (setInternalKeyPool.size() >= min_keys || !CanSupportFeature(FEATURE_HD_SPLIT));
~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~
wallet/wallet.cpp:3668:80: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
return setExternalKeyPool.size() >= min_keys && (setInternalKeyPool.size() >= min_keys || !CanSupportFeature(FEATURE_HD_SPLIT));
~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~
2 warnings generated.
Thanks @paveljanik . This was pointed out by @ryanofsky before but I didn't fix it because this PR had already collected a few ACKs
Cleanup commits here: #11044 . Not required for v0.15.