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.
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
0wallet/wallet.cpp: In member function ‘bool CWallet::HasUnusedKeys(int) const’:
1wallet/wallet.cpp:3663:38: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
2 return setExternalKeyPool.size() >= min_keys && (setInternalKeyPool.size() >= min_keys || !CanSupportFeature(FEATURE_HD_SPLIT));
3 ^
4wallet/wallet.cpp:3663:80: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
5 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.
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
This brings two new warnings:
0wallet/wallet.cpp:3668:38: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
1 return setExternalKeyPool.size() >= min_keys && (setInternalKeyPool.size() >= min_keys || !CanSupportFeature(FEATURE_HD_SPLIT));
2 ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~
3wallet/wallet.cpp:3668:80: warning: comparison of integers of different signs: 'size_type' (aka 'unsigned long') and 'int' [-Wsign-compare]
4 return setExternalKeyPool.size() >= min_keys && (setInternalKeyPool.size() >= min_keys || !CanSupportFeature(FEATURE_HD_SPLIT));
5 ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~
62 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.