Browse Source

Merge #10952: [wallet] Remove vchDefaultKey and have better first run detection

e53615b Remove vchDefaultKey and have better first run detection (Andrew Chow)

Pull request description:

  Removes vchDefaultKey which was only used for first run detection. Improves wallet first run detection by checking to see if any keys were read from the database.

  This also fixes a (rather contrived) case where an encrypted non-HD wallet has corruption such that the default key is no longer valid and is loaded into a Core version that supports HD wallets. This causes a runtime exception since a new hd master key is generated as the software believes the wallet file is newly created but cannot add the generated key to the wallet since it is encrypted. I was only able to replicate this error by creating a non-hd wallet, encrypting it, then editing the wallet using `db_dump` and `db_load` before loading the wallet with hd enabled. This problem has been reported by [two](https://bitcointalk.org/index.php?topic=1993244.0) [users](https://bitcointalk.org/index.php?topic=1746976.msg17511261#msg17511261) so it is something that can happen, although that raises the question of "what corrupted the default key".

  ~P.S. I don't know what's up with the whitespace changes. I think my text editor is doing something stupid but I don't think those are important enough to attempt undoing them.~ Undid those

Tree-SHA512: 63b485f356566e8ffa033ad9b7101f7f6b56372b29ec2a43b947b0eeb1ada4c2cfe24740515d013aedd5f51aa1890dfbe499d2c5c062fc1b5d272324728a7d55
0.16
Wladimir J. van der Laan 7 years ago
parent
commit
262167393d
No known key found for this signature in database
GPG Key ID: 1E4AED62986CD25D
  1. 2
      src/wallet/crypter.h
  2. 26
      src/wallet/wallet.cpp
  3. 4
      src/wallet/wallet.h
  4. 17
      src/wallet/walletdb.cpp
  5. 2
      src/wallet/walletdb.h
  6. 2
      test/functional/keypool-topup.py
  7. 4
      test/functional/wallet-hd.py

2
src/wallet/crypter.h

@ -113,7 +113,6 @@ public:
class CCryptoKeyStore : public CBasicKeyStore class CCryptoKeyStore : public CBasicKeyStore
{ {
private: private:
CryptedKeyMap mapCryptedKeys;
CKeyingMaterial vMasterKey; CKeyingMaterial vMasterKey;
@ -131,6 +130,7 @@ protected:
bool EncryptKeys(CKeyingMaterial& vMasterKeyIn); bool EncryptKeys(CKeyingMaterial& vMasterKeyIn);
bool Unlock(const CKeyingMaterial& vMasterKeyIn); bool Unlock(const CKeyingMaterial& vMasterKeyIn);
CryptedKeyMap mapCryptedKeys;
public: public:
CCryptoKeyStore() : fUseCrypto(false), fDecryptionThoroughlyChecked(false) CCryptoKeyStore() : fUseCrypto(false), fDecryptionThoroughlyChecked(false)

26
src/wallet/wallet.cpp

@ -3123,9 +3123,11 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
} }
} }
// This wallet is in its first run if all of these are empty
fFirstRunRet = mapKeys.empty() && mapCryptedKeys.empty() && mapWatchKeys.empty() && setWatchOnly.empty() && mapScripts.empty();
if (nLoadWalletRet != DB_LOAD_OK) if (nLoadWalletRet != DB_LOAD_OK)
return nLoadWalletRet; return nLoadWalletRet;
fFirstRunRet = !vchDefaultKey.IsValid();
uiInterface.LoadWallet(this); uiInterface.LoadWallet(this);
@ -3135,7 +3137,6 @@ DBErrors CWallet::LoadWallet(bool& fFirstRunRet)
DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut) DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256>& vHashOut)
{ {
AssertLockHeld(cs_wallet); // mapWallet AssertLockHeld(cs_wallet); // mapWallet
vchDefaultKey = CPubKey();
DBErrors nZapSelectTxRet = CWalletDB(*dbw,"cr+").ZapSelectTx(vHashIn, vHashOut); DBErrors nZapSelectTxRet = CWalletDB(*dbw,"cr+").ZapSelectTx(vHashIn, vHashOut);
for (uint256 hash : vHashOut) for (uint256 hash : vHashOut)
mapWallet.erase(hash); mapWallet.erase(hash);
@ -3164,7 +3165,6 @@ DBErrors CWallet::ZapSelectTx(std::vector<uint256>& vHashIn, std::vector<uint256
DBErrors CWallet::ZapWalletTx(std::vector<CWalletTx>& vWtx) DBErrors CWallet::ZapWalletTx(std::vector<CWalletTx>& vWtx)
{ {
vchDefaultKey = CPubKey();
DBErrors nZapWalletTxRet = CWalletDB(*dbw,"cr+").ZapWalletTx(vWtx); DBErrors nZapWalletTxRet = CWalletDB(*dbw,"cr+").ZapWalletTx(vWtx);
if (nZapWalletTxRet == DB_NEED_REWRITE) if (nZapWalletTxRet == DB_NEED_REWRITE)
{ {
@ -3240,14 +3240,6 @@ const std::string& CWallet::GetAccountName(const CScript& scriptPubKey) const
return DEFAULT_ACCOUNT_NAME; return DEFAULT_ACCOUNT_NAME;
} }
bool CWallet::SetDefaultKey(const CPubKey &vchPubKey)
{
if (!CWalletDB(*dbw).WriteDefaultKey(vchPubKey))
return false;
vchDefaultKey = vchPubKey;
return true;
}
/** /**
* Mark old keypool keys as used, * Mark old keypool keys as used,
* and generate all new keys * and generate all new keys
@ -4019,13 +4011,11 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
if (!walletInstance->SetHDMasterKey(masterPubKey)) if (!walletInstance->SetHDMasterKey(masterPubKey))
throw std::runtime_error(std::string(__func__) + ": Storing master key failed"); throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
} }
CPubKey newDefaultKey;
if (walletInstance->GetKeyFromPool(newDefaultKey, false)) { // Top up the keypool
walletInstance->SetDefaultKey(newDefaultKey); if (!walletInstance->TopUpKeyPool()) {
if (!walletInstance->SetAddressBook(walletInstance->vchDefaultKey.GetID(), "", "receive")) { InitError(_("Unable to generate initial keys") += "\n");
InitError(_("Cannot write default address") += "\n"); return NULL;
return nullptr;
}
} }
walletInstance->SetBestChain(chainActive.GetLocator()); walletInstance->SetBestChain(chainActive.GetLocator());

4
src/wallet/wallet.h

@ -807,8 +807,6 @@ public:
std::map<CTxDestination, CAddressBookData> mapAddressBook; std::map<CTxDestination, CAddressBookData> mapAddressBook;
CPubKey vchDefaultKey;
std::set<COutPoint> setLockedCoins; std::set<COutPoint> setLockedCoins;
const CWalletTx* GetWalletTx(const uint256& hash) const; const CWalletTx* GetWalletTx(const uint256& hash) const;
@ -1038,8 +1036,6 @@ public:
return setInternalKeyPool.size() + setExternalKeyPool.size(); return setInternalKeyPool.size() + setExternalKeyPool.size();
} }
bool SetDefaultKey(const CPubKey &vchPubKey);
//! signify that a particular wallet feature is now used. this may change nWalletVersion and nWalletMaxVersion if those are lower //! signify that a particular wallet feature is now used. this may change nWalletVersion and nWalletMaxVersion if those are lower
bool SetMinVersion(enum WalletFeature, CWalletDB* pwalletdbIn = nullptr, bool fExplicit = false); bool SetMinVersion(enum WalletFeature, CWalletDB* pwalletdbIn = nullptr, bool fExplicit = false);

17
src/wallet/walletdb.cpp

@ -130,11 +130,6 @@ bool CWalletDB::WriteOrderPosNext(int64_t nOrderPosNext)
return WriteIC(std::string("orderposnext"), nOrderPosNext); return WriteIC(std::string("orderposnext"), nOrderPosNext);
} }
bool CWalletDB::WriteDefaultKey(const CPubKey& vchPubKey)
{
return WriteIC(std::string("defaultkey"), vchPubKey);
}
bool CWalletDB::ReadPool(int64_t nPool, CKeyPool& keypool) bool CWalletDB::ReadPool(int64_t nPool, CKeyPool& keypool)
{ {
return batch.Read(std::make_pair(std::string("pool"), nPool), keypool); return batch.Read(std::make_pair(std::string("pool"), nPool), keypool);
@ -452,7 +447,14 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
} }
else if (strType == "defaultkey") else if (strType == "defaultkey")
{ {
ssValue >> pwallet->vchDefaultKey; // We don't want or need the default key, but if there is one set,
// we want to make sure that it is valid so that we can detect corruption
CPubKey vchPubKey;
ssValue >> vchPubKey;
if (!vchPubKey.IsValid()) {
strErr = "Error reading wallet database: Default Key corrupt";
return false;
}
} }
else if (strType == "pool") else if (strType == "pool")
{ {
@ -522,7 +524,6 @@ bool CWalletDB::IsKeyType(const std::string& strType)
DBErrors CWalletDB::LoadWallet(CWallet* pwallet) DBErrors CWalletDB::LoadWallet(CWallet* pwallet)
{ {
pwallet->vchDefaultKey = CPubKey();
CWalletScanState wss; CWalletScanState wss;
bool fNoncriticalErrors = false; bool fNoncriticalErrors = false;
DBErrors result = DB_LOAD_OK; DBErrors result = DB_LOAD_OK;
@ -565,7 +566,7 @@ DBErrors CWalletDB::LoadWallet(CWallet* pwallet)
{ {
// losing keys is considered a catastrophic error, anything else // losing keys is considered a catastrophic error, anything else
// we assume the user can live with: // we assume the user can live with:
if (IsKeyType(strType)) if (IsKeyType(strType) || strType == "defaultkey")
result = DB_CORRUPT; result = DB_CORRUPT;
else else
{ {

2
src/wallet/walletdb.h

@ -191,8 +191,6 @@ public:
bool WriteOrderPosNext(int64_t nOrderPosNext); bool WriteOrderPosNext(int64_t nOrderPosNext);
bool WriteDefaultKey(const CPubKey& vchPubKey);
bool ReadPool(int64_t nPool, CKeyPool& keypool); bool ReadPool(int64_t nPool, CKeyPool& keypool);
bool WritePool(int64_t nPool, const CKeyPool& keypool); bool WritePool(int64_t nPool, const CKeyPool& keypool);
bool ErasePool(int64_t nPool); bool ErasePool(int64_t nPool);

2
test/functional/keypool-topup.py

@ -69,7 +69,7 @@ class KeypoolRestoreTest(BitcoinTestFramework):
assert_equal(self.nodes[1].listtransactions()[0]['category'], "receive") assert_equal(self.nodes[1].listtransactions()[0]['category'], "receive")
# Check that we have marked all keys up to the used keypool key as used # Check that we have marked all keys up to the used keypool key as used
assert_equal(self.nodes[1].validateaddress(self.nodes[1].getnewaddress())['hdkeypath'], "m/0'/0'/111'") assert_equal(self.nodes[1].validateaddress(self.nodes[1].getnewaddress())['hdkeypath'], "m/0'/0'/110'")
if __name__ == '__main__': if __name__ == '__main__':
KeypoolRestoreTest().main() KeypoolRestoreTest().main()

4
test/functional/wallet-hd.py

@ -54,7 +54,7 @@ class WalletHDTest(BitcoinTestFramework):
for i in range(num_hd_adds): for i in range(num_hd_adds):
hd_add = self.nodes[1].getnewaddress() hd_add = self.nodes[1].getnewaddress()
hd_info = self.nodes[1].validateaddress(hd_add) hd_info = self.nodes[1].validateaddress(hd_add)
assert_equal(hd_info["hdkeypath"], "m/0'/0'/"+str(i+1)+"'") assert_equal(hd_info["hdkeypath"], "m/0'/0'/"+str(i)+"'")
assert_equal(hd_info["hdmasterkeyid"], masterkeyid) assert_equal(hd_info["hdmasterkeyid"], masterkeyid)
self.nodes[0].sendtoaddress(hd_add, 1) self.nodes[0].sendtoaddress(hd_add, 1)
self.nodes[0].generate(1) self.nodes[0].generate(1)
@ -83,7 +83,7 @@ class WalletHDTest(BitcoinTestFramework):
for _ in range(num_hd_adds): for _ in range(num_hd_adds):
hd_add_2 = self.nodes[1].getnewaddress() hd_add_2 = self.nodes[1].getnewaddress()
hd_info_2 = self.nodes[1].validateaddress(hd_add_2) hd_info_2 = self.nodes[1].validateaddress(hd_add_2)
assert_equal(hd_info_2["hdkeypath"], "m/0'/0'/"+str(_+1)+"'") assert_equal(hd_info_2["hdkeypath"], "m/0'/0'/"+str(_)+"'")
assert_equal(hd_info_2["hdmasterkeyid"], masterkeyid) assert_equal(hd_info_2["hdmasterkeyid"], masterkeyid)
assert_equal(hd_add, hd_add_2) assert_equal(hd_add, hd_add_2)
connect_nodes_bi(self.nodes, 0, 1) connect_nodes_bi(self.nodes, 0, 1)

Loading…
Cancel
Save