From 11e62fa9972ffa2da6dcc7fb948a9242b39c3c19 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 10 Feb 2022 19:18:57 +0000 Subject: [PATCH 1/3] wallet: Remove `CWallet::GetKeyFromPool` Legacy transparent addresses for external use are now obtained directly via `GenerateNewKey(true)`. --- qa/rpc-tests/keypool.py | 8 ++++---- qa/rpc-tests/walletbackup.py | 38 +++++++++++++++++++++++++++++------- src/wallet/rpcwallet.cpp | 9 +++------ src/wallet/wallet.cpp | 23 +++------------------- src/wallet/wallet.h | 1 - 5 files changed, 41 insertions(+), 38 deletions(-) diff --git a/qa/rpc-tests/keypool.py b/qa/rpc-tests/keypool.py index ccba07ea9..d567be3a5 100755 --- a/qa/rpc-tests/keypool.py +++ b/qa/rpc-tests/keypool.py @@ -40,13 +40,13 @@ class KeyPoolTest(BitcoinTestFramework): bitcoind_processes[0].wait() # Restart node 0 nodes[0] = start_node(0, self.options.tmpdir) - # Keep creating keys - addr = nodes[0].getnewaddress() + # We can't create any external addresses, which don't use the keypool. + # We should get an error that we need to unlock the wallet. try: addr = nodes[0].getnewaddress() - raise AssertionError('Keypool should be exhausted after one address') + raise AssertionError('Wallet should be locked.') except JSONRPCException as e: - assert(e.error['code']==-12) + assert_equal(e.error['code'], -13) # put three new keys in the keypool nodes[0].walletpassphrase('test', 12000) diff --git a/qa/rpc-tests/walletbackup.py b/qa/rpc-tests/walletbackup.py index 463fe63b0..6260bdcd3 100755 --- a/qa/rpc-tests/walletbackup.py +++ b/qa/rpc-tests/walletbackup.py @@ -96,10 +96,10 @@ class WalletBackupTest(BitcoinTestFramework): self.sync_all() # As above, this mirrors the original bash test. - def start_three(self): - self.nodes[0] = start_node(0, self.options.tmpdir) - self.nodes[1] = start_node(1, self.options.tmpdir) - self.nodes[2] = start_node(2, self.options.tmpdir) + def start_three(self, extra_args=None): + self.nodes[0] = start_node(0, self.options.tmpdir, extra_args) + self.nodes[1] = start_node(1, self.options.tmpdir, extra_args) + self.nodes[2] = start_node(2, self.options.tmpdir, extra_args) connect_nodes(self.nodes[0], 3) connect_nodes(self.nodes[1], 3) connect_nodes(self.nodes[2], 3) @@ -191,6 +191,30 @@ class WalletBackupTest(BitcoinTestFramework): self.start_three() sync_blocks(self.nodes) + # We made extra transactions that involved addresses generated after the + # backups were taken, and external addresses do not use the keypool, so + # the balances shouldn't line up. + balance0backup = self.nodes[0].getbalance() + balance1backup = self.nodes[1].getbalance() + balance2backup = self.nodes[2].getbalance() + assert(balance0backup != balance0) + assert(balance1backup != balance1) + assert(balance2backup != balance2) + + # However, because addresses are derived deterministically, we can + # recover the balances by generating the extra addresses and then + # rescanning. + for i in range(5): + self.nodes[0].getnewaddress() + self.nodes[1].getnewaddress() + self.nodes[2].getnewaddress() + + logging.info("Re-starting nodes with -rescan") + self.stop_three() + self.start_three(['-rescan']) + sync_blocks(self.nodes) + + # TODO: Alter keypool to not use external addresses, so this passes. assert_equal(self.nodes[0].getbalance(), balance0) assert_equal(self.nodes[1].getbalance(), balance1) assert_equal(self.nodes[2].getbalance(), balance2) @@ -215,9 +239,9 @@ class WalletBackupTest(BitcoinTestFramework): sync_blocks(self.nodes) - assert_equal(self.nodes[0].getbalance(), balance0) - assert_equal(self.nodes[1].getbalance(), balance1) - assert_equal(self.nodes[2].getbalance(), balance2) + assert_equal(self.nodes[0].getbalance(), balance0backup) + assert_equal(self.nodes[1].getbalance(), balance1backup) + assert_equal(self.nodes[2].getbalance(), balance2backup) if __name__ == '__main__': diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index c4be019c7..a7a0c9056 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -182,14 +182,11 @@ UniValue getnewaddress(const UniValue& params, bool fHelp) const CChainParams& chainparams = Params(); EnsureWalletIsBackedUp(chainparams); - if (!pwalletMain->IsLocked()) - pwalletMain->TopUpKeyPool(); + EnsureWalletIsUnlocked(); // Generate a new key that is added to wallet - std::optional newKey = pwalletMain->GetKeyFromPool(); - if (!newKey.has_value()) - throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first"); - CKeyID keyID = newKey.value().GetID(); + CPubKey newKey = pwalletMain->GenerateNewKey(true); + CKeyID keyID = newKey.GetID(); std::string dummy_account; pwalletMain->SetAddressBook(keyID, dummy_account, "receive"); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3cd82ab9f..a60c61469 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5130,23 +5130,6 @@ void CWallet::ReturnKey(int64_t nIndex) LogPrintf("keypool return %d\n", nIndex); } -std::optional CWallet::GetKeyFromPool() -{ - int64_t nIndex = 0; - CKeyPool keypool; - { - LOCK(cs_wallet); - ReserveKeyFromKeyPool(nIndex, keypool); - if (nIndex == -1) - { - if (IsLocked()) return std::nullopt; - return GenerateNewKey(true); - } - KeepKey(nIndex); - return keypool.vchPubKey; - } -} - int64_t CWallet::GetOldestKeyPoolTime() { int64_t nIndex = 0; @@ -5715,9 +5698,9 @@ bool CWallet::InitLoadWallet(const CChainParams& params, bool clearWitnessCaches if (fFirstRun) { // Create new keyUser and set as default key - std::optional newDefaultKey = walletInstance->GetKeyFromPool(); - if (newDefaultKey.has_value()) { - walletInstance->SetDefaultKey(newDefaultKey.value()); + if (!walletInstance->IsCrypted()) { + CPubKey newDefaultKey = walletInstance->GenerateNewKey(true); + walletInstance->SetDefaultKey(newDefaultKey); if (!walletInstance->SetAddressBook(walletInstance->vchDefaultKey.GetID(), "", "receive")) return UIError(_("Cannot write default address") += "\n"); } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index cd8c69040..780acb996 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1536,7 +1536,6 @@ public: void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool); void KeepKey(int64_t nIndex); void ReturnKey(int64_t nIndex); - std::optional GetKeyFromPool(); int64_t GetOldestKeyPoolTime(); void GetAllReserveKeys(std::set& setAddress) const; From 2555aadf31275c983fb45b3629c88ccc1b234fc5 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 10 Feb 2022 19:36:18 +0000 Subject: [PATCH 2/3] wallet: Store internal transparent keys in the keypool Now that the keypool is not used by any consumers of external transparent keys, we switch it to only contain internal keys. To handle the impedance mismatch, we clear out the keypool when a mnemonic seed is generated for the wallet. --- src/wallet/wallet.cpp | 16 +++++++++++----- src/wallet/wallet.h | 2 ++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a60c61469..2abf57a7e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2279,7 +2279,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) Lock(); Unlock(strWalletPassphrase); - NewKeyPool(); + // TODO: migrate to a new mnemonic when encrypting an unencrypted wallet? Lock(); // Need to completely rewrite the wallet file; if we don't, bdb might keep @@ -3122,6 +3122,11 @@ void CWallet::GenerateNewSeed(Language language) // as a hdchain object CHDChain newHdChain(seed.Fingerprint(), nCreationTime); SetMnemonicHDChain(newHdChain, false); + + // Now that we can derive keys deterministically, clear out the legacy + // transparent keypool of all randomly-generated keys, and fill it with + // internal keys (for use as change addresses or miner outputs). + NewKeyPool(); } bool CWallet::SetMnemonicSeed(const MnemonicSeed& seed) @@ -5024,8 +5029,9 @@ bool CWallet::SetDefaultKey(const CPubKey &vchPubKey) } /** - * Mark old keypool keys as used, - * and generate all new keys + * Mark old keypool keys as used, and derive new internal keys. + * + * This is only used when first migrating to HD-derived transparent keys. */ bool CWallet::NewKeyPool() { @@ -5043,7 +5049,7 @@ bool CWallet::NewKeyPool() for (int i = 0; i < nKeys; i++) { int64_t nIndex = i+1; - walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey(true))); + walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey(false))); setKeyPool.insert(nIndex); } LogPrintf("CWallet::NewKeyPool wrote %d new keys\n", nKeys); @@ -5073,7 +5079,7 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) int64_t nEnd = 1; if (!setKeyPool.empty()) nEnd = *(--setKeyPool.end()) + 1; - if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(true)))) + if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(false)))) throw runtime_error("TopUpKeyPool(): writing generated key failed"); setKeyPool.insert(nEnd); LogPrintf("keypool added key %d, size=%u\n", nEnd, setKeyPool.size()); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 780acb996..eea65b761 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1531,7 +1531,9 @@ public: */ static CAmount GetRequiredFee(unsigned int nTxBytes); +private: bool NewKeyPool(); +public: bool TopUpKeyPool(unsigned int kpSize = 0); void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool); void KeepKey(int64_t nIndex); From 89b9bbaf33d8890a814e0f6958d394eaafc64010 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 10 Feb 2022 19:50:47 +0000 Subject: [PATCH 3/3] wallet: Separate counters for external and internal transparent keys This fixes a potential bug with importing the mnemonic into a third party transparent wallet. Previously, if a user called `getnewaddress`, made a bunch of transactions that generated at least 20 change addresses, and then called `getnewaddress` again, the two external addresses would be separated by a gap of more than 20. If this mnemonic were imported into a third party transparent wallet, the wallet would not detect any funds in the second (or subsequent) transparent addresses because it would detect 20 unused addresses in a row (via the BIP 44 default gap limit). Now, we track external and internal keys separately; repeated calls to `getnewaddress` will return addresses for sequential keys. This has the added benefit that the sequence of `getnewaddress` outputs will be the same after restoring from a backup. --- qa/rpc-tests/walletbackup.py | 1 - src/wallet/wallet.cpp | 4 ++-- src/wallet/walletdb.h | 23 +++++++++++++++-------- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/qa/rpc-tests/walletbackup.py b/qa/rpc-tests/walletbackup.py index 6260bdcd3..ee2f88460 100755 --- a/qa/rpc-tests/walletbackup.py +++ b/qa/rpc-tests/walletbackup.py @@ -214,7 +214,6 @@ class WalletBackupTest(BitcoinTestFramework): self.start_three(['-rescan']) sync_blocks(self.nodes) - # TODO: Alter keypool to not use external addresses, so this passes. assert_equal(self.nodes[0].getbalance(), balance0) assert_equal(self.nodes[1].getbalance(), balance1) assert_equal(self.nodes[2].getbalance(), balance2) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2abf57a7e..6fda85be3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -271,12 +271,12 @@ CPubKey CWallet::GenerateNewKey(bool external) transparent::AccountKey accountKey = this->GetLegacyAccountKey(); std::optional pubkey = std::nullopt; do { - auto index = hdChain.GetLegacyTKeyCounter(); + auto index = hdChain.GetLegacyTKeyCounter(external); auto key = external ? accountKey.DeriveExternalSpendingKey(index) : accountKey.DeriveInternalSpendingKey(index); - hdChain.IncrementLegacyTKeyCounter(); + hdChain.IncrementLegacyTKeyCounter(external); if (key.has_value()) { pubkey = AddTransparentSecretKey( hdChain.GetSeedFingerprint(), diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 36ed07fa5..9e9ad59ed 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -49,7 +49,8 @@ private: uint256 seedFp; int64_t nCreateTime; // 0 means unknown uint32_t accountCounter; - uint32_t legacyTKeyCounter; + uint32_t legacyTKeyExternalCounter; + uint32_t legacyTKeyInternalCounter; uint32_t legacySaplingKeyCounter; bool mnemonicSeedBackupConfirmed; @@ -61,7 +62,8 @@ private: seedFp.SetNull(); nCreateTime = 0; accountCounter = 0; - legacyTKeyCounter = 0; + legacyTKeyExternalCounter = 0; + legacyTKeyInternalCounter = 0; legacySaplingKeyCounter = 0; mnemonicSeedBackupConfirmed = false; } @@ -69,7 +71,7 @@ public: static const int VERSION_HD_BASE = 1; static const int CURRENT_VERSION = VERSION_HD_BASE; - CHDChain(uint256 seedFpIn, int64_t nCreateTimeIn): nVersion(CHDChain::CURRENT_VERSION), seedFp(seedFpIn), nCreateTime(nCreateTimeIn), accountCounter(0), legacyTKeyCounter(0), legacySaplingKeyCounter(0), mnemonicSeedBackupConfirmed(false) {} + CHDChain(uint256 seedFpIn, int64_t nCreateTimeIn): nVersion(CHDChain::CURRENT_VERSION), seedFp(seedFpIn), nCreateTime(nCreateTimeIn), accountCounter(0), legacyTKeyExternalCounter(0), legacyTKeyInternalCounter(0), legacySaplingKeyCounter(0), mnemonicSeedBackupConfirmed(false) {} ADD_SERIALIZE_METHODS; @@ -80,7 +82,8 @@ public: READWRITE(seedFp); READWRITE(nCreateTime); READWRITE(accountCounter); - READWRITE(legacyTKeyCounter); + READWRITE(legacyTKeyExternalCounter); + READWRITE(legacyTKeyInternalCounter); READWRITE(legacySaplingKeyCounter); READWRITE(mnemonicSeedBackupConfirmed); } @@ -105,12 +108,16 @@ public: accountCounter += 1; } - uint32_t GetLegacyTKeyCounter() { - return legacyTKeyCounter; + uint32_t GetLegacyTKeyCounter(bool external) { + return external ? legacyTKeyExternalCounter : legacyTKeyInternalCounter; } - void IncrementLegacyTKeyCounter() { - legacyTKeyCounter += 1; + void IncrementLegacyTKeyCounter(bool external) { + if (external) { + legacyTKeyExternalCounter += 1; + } else { + legacyTKeyInternalCounter += 1; + } } uint32_t GetLegacySaplingKeyCounter() const {