From 02592f4c5e6d9416165deef96398ac7760f457d1 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Tue, 10 Jan 2017 16:45:30 +0100 Subject: [PATCH 01/23] [Wallet] split the keypool in an internal and external part --- src/wallet/rpcwallet.cpp | 31 ++++--- src/wallet/wallet.cpp | 127 +++++++++++++++++--------- src/wallet/wallet.h | 25 +++-- src/wallet/walletdb.h | 8 +- test/functional/fundrawtransaction.py | 3 + test/functional/keypool.py | 37 +++++--- test/functional/wallet-dump.py | 6 +- test/functional/wallet-hd.py | 19 ++++ 8 files changed, 180 insertions(+), 76 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 84e7eb60d..7ab01040f 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -221,7 +221,7 @@ UniValue getrawchangeaddress(const JSONRPCRequest& request) CReserveKey reservekey(pwallet); CPubKey vchPubKey; - if (!reservekey.GetReservedKey(vchPubKey)) + if (!reservekey.GetReservedKey(vchPubKey, true)) throw JSONRPCError(RPC_WALLET_KEYPOOL_RAN_OUT, "Error: Keypool ran out, please call keypoolrefill first"); reservekey.KeepKey(); @@ -2418,16 +2418,17 @@ UniValue getwalletinfo(const JSONRPCRequest& request) "Returns an object containing various wallet state info.\n" "\nResult:\n" "{\n" - " \"walletversion\": xxxxx, (numeric) the wallet version\n" - " \"balance\": xxxxxxx, (numeric) the total confirmed balance of the wallet in " + CURRENCY_UNIT + "\n" - " \"unconfirmed_balance\": xxx, (numeric) the total unconfirmed balance of the wallet in " + CURRENCY_UNIT + "\n" - " \"immature_balance\": xxxxxx, (numeric) the total immature balance of the wallet in " + CURRENCY_UNIT + "\n" - " \"txcount\": xxxxxxx, (numeric) the total number of transactions in the wallet\n" - " \"keypoololdest\": xxxxxx, (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool\n" - " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated\n" - " \"unlocked_until\": ttt, (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n" - " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" - " \"hdmasterkeyid\": \"\" (string) the Hash160 of the HD master pubkey\n" + " \"walletversion\": xxxxx, (numeric) the wallet version\n" + " \"balance\": xxxxxxx, (numeric) the total confirmed balance of the wallet in " + CURRENCY_UNIT + "\n" + " \"unconfirmed_balance\": xxx, (numeric) the total unconfirmed balance of the wallet in " + CURRENCY_UNIT + "\n" + " \"immature_balance\": xxxxxx, (numeric) the total immature balance of the wallet in " + CURRENCY_UNIT + "\n" + " \"txcount\": xxxxxxx, (numeric) the total number of transactions in the wallet\n" + " \"keypoololdest\": xxxxxx, (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool\n" + " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated\n" + " \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (change outputs, 10% of the -keypoolsize target, only appears if HD is enabled)\n" + " \"unlocked_until\": ttt, (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n" + " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" + " \"hdmasterkeyid\": \"\" (string) the Hash160 of the HD master pubkey\n" "}\n" "\nExamples:\n" + HelpExampleCli("getwalletinfo", "") @@ -2437,18 +2438,22 @@ UniValue getwalletinfo(const JSONRPCRequest& request) LOCK2(cs_main, pwallet->cs_wallet); UniValue obj(UniValue::VOBJ); + size_t kpExternalSize = (int)pwallet->KeypoolCountExternalKeys(); obj.push_back(Pair("walletversion", pwallet->GetVersion())); obj.push_back(Pair("balance", ValueFromAmount(pwallet->GetBalance()))); obj.push_back(Pair("unconfirmed_balance", ValueFromAmount(pwallet->GetUnconfirmedBalance()))); obj.push_back(Pair("immature_balance", ValueFromAmount(pwallet->GetImmatureBalance()))); obj.push_back(Pair("txcount", (int)pwallet->mapWallet.size())); obj.push_back(Pair("keypoololdest", pwallet->GetOldestKeyPoolTime())); - obj.push_back(Pair("keypoolsize", (int)pwallet->GetKeyPoolSize())); + obj.push_back(Pair("keypoolsize", (int64_t)kpExternalSize)); + CKeyID masterKeyID = pwallet->GetHDChain().masterKeyID; + if (!masterKeyID.IsNull()) { + obj.push_back(Pair("keypoolsize_hd_internal", (int64_t)(pwallet->GetKeyPoolSize() - kpExternalSize))); + } if (pwallet->IsCrypted()) { obj.push_back(Pair("unlocked_until", pwallet->nRelockTime)); } obj.push_back(Pair("paytxfee", ValueFromAmount(payTxFee.GetFeePerK()))); - CKeyID masterKeyID = pwallet->GetHDChain().masterKeyID; if (!masterKeyID.IsNull()) obj.push_back(Pair("hdmasterkeyid", masterKeyID.GetHex())); return obj; diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 445e40b04..71c97a4e8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -85,7 +85,7 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const return &(it->second); } -CPubKey CWallet::GenerateNewKey() +CPubKey CWallet::GenerateNewKey(bool internal) { AssertLockHeld(cs_wallet); // mapKeyMetadata bool fCompressed = CanSupportFeature(FEATURE_COMPRPUBKEY); // default to compressed public keys if we want 0.6.0 wallets @@ -98,7 +98,7 @@ CPubKey CWallet::GenerateNewKey() // use HD key derivation if HD was enabled during wallet creation if (IsHDEnabled()) { - DeriveNewChildKey(metadata, secret); + DeriveNewChildKey(metadata, secret, (CanSupportFeature(FEATURE_HD_SPLIT) ? internal : false)); } else { secret.MakeNewKey(fCompressed); } @@ -118,13 +118,13 @@ CPubKey CWallet::GenerateNewKey() return pubkey; } -void CWallet::DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret) +void CWallet::DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal) { // for now we use a fixed keypath scheme of m/0'/0'/k CKey key; //master key seed (256bit) CExtKey masterKey; //hd master key CExtKey accountKey; //key at m/0' - CExtKey externalChainChildKey; //key at m/0'/0' + CExtKey chainChildKey; //key at m/0'/0' (external) or m/0'/1' (internal) CExtKey childKey; //key at m/0'/0'/' // try to get the master key @@ -137,19 +137,22 @@ void CWallet::DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret) // use hardened derivation (child keys >= 0x80000000 are hardened after bip32) masterKey.Derive(accountKey, BIP32_HARDENED_KEY_LIMIT); - // derive m/0'/0' - accountKey.Derive(externalChainChildKey, BIP32_HARDENED_KEY_LIMIT); + // derive m/0'/0' (external chain) OR m/0'/1' (internal chain) + accountKey.Derive(chainChildKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0)); // derive child key at next index, skip keys already known to the wallet do { // always derive hardened keys // childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649 - externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT); - metadata.hdKeypath = "m/0'/0'/" + std::to_string(hdChain.nExternalChainCounter) + "'"; + chainChildKey.Derive(childKey, (internal ? hdChain.nInternalChainCounter : hdChain.nExternalChainCounter) | BIP32_HARDENED_KEY_LIMIT); + metadata.hdKeypath = "m/0'/" + std::string(internal ? "1'/"+ std::to_string(hdChain.nInternalChainCounter) : "0'/" + std::to_string(hdChain.nExternalChainCounter)) + "'"; metadata.hdMasterKeyID = hdChain.masterKeyID; // increment childkey index - hdChain.nExternalChainCounter++; + if (internal) + hdChain.nInternalChainCounter++; + else + hdChain.nExternalChainCounter++; } while (HaveKey(childKey.key.GetPubKey().GetID())); secret = childKey.key; @@ -799,7 +802,7 @@ bool CWallet::GetAccountPubkey(CPubKey &pubKey, std::string strAccount, bool bFo // Generate a new key if (bForceNew) { - if (!GetKeyFromPool(account.vchPubKey)) + if (!GetKeyFromPool(account.vchPubKey, false)) return false; SetAddressBook(account.vchPubKey.GetID(), strAccount, "receive"); @@ -1304,8 +1307,8 @@ bool CWallet::SetHDMasterKey(const CPubKey& pubkey) { LOCK(cs_wallet); - // ensure this wallet.dat can only be opened by clients supporting HD - SetMinVersion(FEATURE_HD); + // ensure this wallet.dat can only be opened by clients supporting HD with chain split + SetMinVersion(FEATURE_HD_SPLIT); // store the keyid (hash160) together with // the child index counter in the database @@ -2445,7 +2448,7 @@ bool CWallet::CreateTransaction(const std::vector& vecSend, CWalletT // Reserve a new key pair from key pool CPubKey vchPubKey; bool ret; - ret = reservekey.GetReservedKey(vchPubKey); + ret = reservekey.GetReservedKey(vchPubKey, true); if (!ret) { strFailReason = _("Keypool ran out, please call keypoolrefill first"); @@ -2899,18 +2902,31 @@ bool CWallet::NewKeyPool() if (IsLocked()) return false; - int64_t nKeys = std::max(GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t)0); - for (int i = 0; i < nKeys; i++) - { - int64_t nIndex = i+1; - walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey())); - setKeyPool.insert(nIndex); - } - LogPrintf("CWallet::NewKeyPool wrote %d new keys\n", nKeys); + TopUpKeyPool(); + LogPrintf("CWallet::NewKeyPool rewrote keypool\n"); } return true; } +size_t CWallet::KeypoolCountExternalKeys() +{ + AssertLockHeld(cs_wallet); // setKeyPool + + CWalletDB walletdb(strWalletFile); + + // count amount of external keys + size_t amountE = 0; + for(const int64_t& id : setKeyPool) + { + CKeyPool tmpKeypool; + if (!walletdb.ReadPool(id, tmpKeypool)) + throw std::runtime_error(std::string(__func__) + ": read failed"); + amountE += !tmpKeypool.fInternal; + } + + return amountE; +} + bool CWallet::TopUpKeyPool(unsigned int kpSize) { { @@ -2919,8 +2935,6 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) if (IsLocked()) return false; - CWalletDB walletdb(strWalletFile); - // Top up key pool unsigned int nTargetSize; if (kpSize > 0) @@ -2928,21 +2942,39 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) else nTargetSize = std::max(GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0); - while (setKeyPool.size() < (nTargetSize + 1)) + // count amount of available keys (internal, external) + // make sure the keypool of external keys fits the user selected target (-keypool) + // generate +20% internal keys (minimum 2 keys) + int64_t amountExternal = KeypoolCountExternalKeys(); + int64_t amountInternal = setKeyPool.size() - amountExternal; + int64_t targetInternal = max((int64_t)ceil(nTargetSize * 0.2), (int64_t) 2); + int64_t missingExternal = max( (int64_t)(nTargetSize - amountExternal), (int64_t) 0); + int64_t missingInternal = max(targetInternal - amountInternal, (int64_t) 0); + + if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT)) + { + // don't create extra internal keys + missingInternal = 0; + } + bool internal = false; + CWalletDB walletdb(strWalletFile); + for (int64_t i = missingInternal + missingExternal; i--;) { int64_t nEnd = 1; + if (i < missingInternal) + internal = true; if (!setKeyPool.empty()) nEnd = *(--setKeyPool.end()) + 1; - if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey()))) + if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(internal), internal))) throw std::runtime_error(std::string(__func__) + ": writing generated key failed"); setKeyPool.insert(nEnd); - LogPrintf("keypool added key %d, size=%u\n", nEnd, setKeyPool.size()); + LogPrintf("keypool added key %d, size=%u, internal=%d\n", nEnd, setKeyPool.size(), internal); } } return true; } -void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool) +void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool internal) { nIndex = -1; keypool.vchPubKey = CPubKey(); @@ -2958,14 +2990,24 @@ void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool) CWalletDB walletdb(strWalletFile); - nIndex = *(setKeyPool.begin()); - setKeyPool.erase(setKeyPool.begin()); - if (!walletdb.ReadPool(nIndex, keypool)) - throw std::runtime_error(std::string(__func__) + ": read failed"); - if (!HaveKey(keypool.vchPubKey.GetID())) - throw std::runtime_error(std::string(__func__) + ": unknown key in key pool"); - assert(keypool.vchPubKey.IsValid()); - LogPrintf("keypool reserve %d\n", nIndex); + // try to find a key that matches the internal/external filter + for(const int64_t& id : setKeyPool) + { + CKeyPool tmpKeypool; + if (!walletdb.ReadPool(id, tmpKeypool)) + throw std::runtime_error(std::string(__func__) + ": read failed"); + if (!HaveKey(tmpKeypool.vchPubKey.GetID())) + throw std::runtime_error(std::string(__func__) + ": unknown key in key pool"); + if (!IsHDEnabled() || tmpKeypool.fInternal == internal) + { + nIndex = id; + keypool = tmpKeypool; + setKeyPool.erase(id); + assert(keypool.vchPubKey.IsValid()); + LogPrintf("keypool reserve %d\n", nIndex); + return; + } + } } } @@ -2990,17 +3032,17 @@ void CWallet::ReturnKey(int64_t nIndex) LogPrintf("keypool return %d\n", nIndex); } -bool CWallet::GetKeyFromPool(CPubKey& result) +bool CWallet::GetKeyFromPool(CPubKey& result, bool internal) { int64_t nIndex = 0; CKeyPool keypool; { LOCK(cs_wallet); - ReserveKeyFromKeyPool(nIndex, keypool); + ReserveKeyFromKeyPool(nIndex, keypool, internal); if (nIndex == -1) { if (IsLocked()) return false; - result = GenerateNewKey(); + result = GenerateNewKey(internal); return true; } KeepKey(nIndex); @@ -3205,12 +3247,12 @@ std::set CWallet::GetAccountAddresses(const std::string& strAcco return result; } -bool CReserveKey::GetReservedKey(CPubKey& pubkey) +bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal) { if (nIndex == -1) { CKeyPool keypool; - pwallet->ReserveKeyFromKeyPool(nIndex, keypool); + pwallet->ReserveKeyFromKeyPool(nIndex, keypool, internal); if (nIndex != -1) vchPubKey = keypool.vchPubKey; else { @@ -3629,7 +3671,7 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) throw std::runtime_error(std::string(__func__) + ": Storing master key failed"); } CPubKey newDefaultKey; - if (walletInstance->GetKeyFromPool(newDefaultKey)) { + if (walletInstance->GetKeyFromPool(newDefaultKey, false)) { walletInstance->SetDefaultKey(newDefaultKey); if (!walletInstance->SetAddressBook(walletInstance->vchDefaultKey.GetID(), "", "receive")) { InitError(_("Cannot write default address") += "\n"); @@ -3890,10 +3932,11 @@ CKeyPool::CKeyPool() nTime = GetTime(); } -CKeyPool::CKeyPool(const CPubKey& vchPubKeyIn) +CKeyPool::CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn) { nTime = GetTime(); vchPubKey = vchPubKeyIn; + fInternal = internalIn; } CWalletKey::CWalletKey(int64_t nExpires) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ae4321eef..bf6c5a2a8 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -86,6 +86,10 @@ enum WalletFeature FEATURE_COMPRPUBKEY = 60000, // compressed public keys FEATURE_HD = 130000, // Hierarchical key derivation after BIP32 (HD Wallet) + + //TODO: FEATURE_HD_SPLIT needs to be bumped to 140000 once we branch of 0.14 // + FEATURE_HD_SPLIT = 139900, // Wallet with HD chain split (change outputs will use m/0'/1'/k) + FEATURE_LATEST = FEATURE_COMPRPUBKEY // HD is optional, use FEATURE_COMPRPUBKEY as latest version }; @@ -96,9 +100,10 @@ class CKeyPool public: int64_t nTime; CPubKey vchPubKey; + bool fInternal; // for change outputs CKeyPool(); - CKeyPool(const CPubKey& vchPubKeyIn); + CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn); ADD_SERIALIZE_METHODS; @@ -109,6 +114,13 @@ public: READWRITE(nVersion); READWRITE(nTime); READWRITE(vchPubKey); + if (nVersion >= FEATURE_HD_SPLIT) + READWRITE(fInternal); + else + { + if (ser_action.ForRead()) + fInternal = false; + } } }; @@ -774,8 +786,8 @@ public: * keystore implementation * Generate a new key */ - CPubKey GenerateNewKey(); - void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret); + CPubKey GenerateNewKey(bool internal = false); + void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal = false); //! Adds a key to the store, and saves it to disk. bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey) override; //! Adds a key to the store, without saving it to disk (used by LoadWallet) @@ -883,11 +895,12 @@ public: static CAmount GetRequiredFee(unsigned int nTxBytes); bool NewKeyPool(); + size_t KeypoolCountExternalKeys(); bool TopUpKeyPool(unsigned int kpSize = 0); - void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool); + void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool internal); void KeepKey(int64_t nIndex); void ReturnKey(int64_t nIndex); - bool GetKeyFromPool(CPubKey &key); + bool GetKeyFromPool(CPubKey &key, bool internal = false); int64_t GetOldestKeyPoolTime(); void GetAllReserveKeys(std::set& setAddress) const; @@ -1063,7 +1076,7 @@ public: } void ReturnKey(); - bool GetReservedKey(CPubKey &pubkey); + bool GetReservedKey(CPubKey &pubkey, bool internal = false); void KeepKey(); void KeepScript() { KeepKey(); } }; diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 4d7dfb727..016296f25 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -46,9 +46,12 @@ class CHDChain { public: uint32_t nExternalChainCounter; + uint32_t nInternalChainCounter; CKeyID masterKeyID; //!< master key hash160 - static const int CURRENT_VERSION = 1; + static const int VERSION_HD_BASE = 1; + static const int VERSION_HD_CHAIN_SPLIT = 2; + static const int CURRENT_VERSION = VERSION_HD_CHAIN_SPLIT; int nVersion; CHDChain() { SetNull(); } @@ -58,6 +61,8 @@ public: { READWRITE(this->nVersion); READWRITE(nExternalChainCounter); + if (this->nVersion >= VERSION_HD_CHAIN_SPLIT) + READWRITE(nInternalChainCounter); READWRITE(masterKeyID); } @@ -65,6 +70,7 @@ public: { nVersion = CHDChain::CURRENT_VERSION; nExternalChainCounter = 0; + nInternalChainCounter = 0; masterKeyID.SetNull(); } }; diff --git a/test/functional/fundrawtransaction.py b/test/functional/fundrawtransaction.py index 1dc00f2ba..a28ff9465 100755 --- a/test/functional/fundrawtransaction.py +++ b/test/functional/fundrawtransaction.py @@ -467,6 +467,8 @@ class RawTransactionsTest(BitcoinTestFramework): # drain the keypool self.nodes[1].getnewaddress() + self.nodes[1].getrawchangeaddress() + self.nodes[1].getrawchangeaddress() inputs = [] outputs = {self.nodes[0].getnewaddress():1.1} rawTx = self.nodes[1].createrawtransaction(inputs, outputs) @@ -476,6 +478,7 @@ class RawTransactionsTest(BitcoinTestFramework): #refill the keypool self.nodes[1].walletpassphrase("test", 100) + self.nodes[1].keypoolrefill(8) #need to refill the keypool to get an internal change address self.nodes[1].walletlock() assert_raises_jsonrpc(-13, "walletpassphrase", self.nodes[1].sendtoaddress, self.nodes[0].getnewaddress(), 1.2) diff --git a/test/functional/keypool.py b/test/functional/keypool.py index cee58563f..26585fc4a 100755 --- a/test/functional/keypool.py +++ b/test/functional/keypool.py @@ -27,28 +27,38 @@ class KeyPoolTest(BitcoinTestFramework): wallet_info = nodes[0].getwalletinfo() assert(addr_before_encrypting_data['hdmasterkeyid'] != wallet_info['hdmasterkeyid']) assert(addr_data['hdmasterkeyid'] == wallet_info['hdmasterkeyid']) - assert_raises_jsonrpc(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress) - # put three new keys in the keypool + # put six (plus 2) new keys in the keypool (100% external-, +20% internal-keys, 2 in min) nodes[0].walletpassphrase('test', 12000) - nodes[0].keypoolrefill(3) + nodes[0].keypoolrefill(6) nodes[0].walletlock() + wi = nodes[0].getwalletinfo() + assert_equal(wi['keypoolsize_hd_internal'], 2) + assert_equal(wi['keypoolsize'], 6) - # drain the keys + # drain the internal keys + nodes[0].getrawchangeaddress() + nodes[0].getrawchangeaddress() addr = set() - addr.add(nodes[0].getrawchangeaddress()) - addr.add(nodes[0].getrawchangeaddress()) - addr.add(nodes[0].getrawchangeaddress()) - addr.add(nodes[0].getrawchangeaddress()) - # assert that four unique addresses were returned - assert(len(addr) == 4) # the next one should fail assert_raises_jsonrpc(-12, "Keypool ran out", nodes[0].getrawchangeaddress) + # drain the external keys + addr.add(nodes[0].getnewaddress()) + addr.add(nodes[0].getnewaddress()) + addr.add(nodes[0].getnewaddress()) + addr.add(nodes[0].getnewaddress()) + addr.add(nodes[0].getnewaddress()) + addr.add(nodes[0].getnewaddress()) + assert(len(addr) == 6) + # the next one should fail + assert_raises_jsonrpc(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress) + # refill keypool with three new addresses nodes[0].walletpassphrase('test', 1) nodes[0].keypoolrefill(3) + # test walletpassphrase timeout time.sleep(1.1) assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0) @@ -57,9 +67,14 @@ class KeyPoolTest(BitcoinTestFramework): nodes[0].generate(1) nodes[0].generate(1) nodes[0].generate(1) - nodes[0].generate(1) assert_raises_jsonrpc(-12, "Keypool ran out", nodes[0].generate, 1) + nodes[0].walletpassphrase('test', 100) + nodes[0].keypoolrefill(100) + wi = nodes[0].getwalletinfo() + assert_equal(wi['keypoolsize_hd_internal'], 20) + assert_equal(wi['keypoolsize'], 100) + def __init__(self): super().__init__() self.setup_clean_chain = False diff --git a/test/functional/wallet-dump.py b/test/functional/wallet-dump.py index b819b72b7..7eaa8090b 100755 --- a/test/functional/wallet-dump.py +++ b/test/functional/wallet-dump.py @@ -88,7 +88,7 @@ class WalletDumpTest(BitcoinTestFramework): read_dump(tmpdir + "/node0/wallet.unencrypted.dump", addrs, None) assert_equal(found_addr, test_addr_count) # all keys must be in the dump assert_equal(found_addr_chg, 50) # 50 blocks where mined - assert_equal(found_addr_rsv, 90 + 1) # keypool size (TODO: fix off-by-one) + assert_equal(found_addr_rsv, 90*1.2) # 90 keys plus 20% internal keys #encrypt wallet, restart, unlock and dump self.nodes[0].encryptwallet('test') @@ -102,8 +102,8 @@ class WalletDumpTest(BitcoinTestFramework): found_addr, found_addr_chg, found_addr_rsv, hd_master_addr_enc = \ read_dump(tmpdir + "/node0/wallet.encrypted.dump", addrs, hd_master_addr_unenc) assert_equal(found_addr, test_addr_count) - assert_equal(found_addr_chg, 90 + 1 + 50) # old reserve keys are marked as change now - assert_equal(found_addr_rsv, 90 + 1) # keypool size (TODO: fix off-by-one) + assert_equal(found_addr_chg, 90*1.2 + 50) # old reserve keys are marked as change now + assert_equal(found_addr_rsv, 90*1.2) if __name__ == '__main__': WalletDumpTest().main () diff --git a/test/functional/wallet-hd.py b/test/functional/wallet-hd.py index c40662dc3..b17f78843 100755 --- a/test/functional/wallet-hd.py +++ b/test/functional/wallet-hd.py @@ -42,6 +42,11 @@ class WalletHDTest(BitcoinTestFramework): masterkeyid = self.nodes[1].getwalletinfo()['hdmasterkeyid'] assert_equal(len(masterkeyid), 40) + #create an internal key + change_addr = self.nodes[1].getrawchangeaddress() + change_addrV= self.nodes[1].validateaddress(change_addr); + assert_equal(change_addrV["hdkeypath"], "m/0'/1'/0'") #first internal child key + # Import a non-HD private key in the HD wallet non_hd_add = self.nodes[0].getnewaddress() self.nodes[1].importprivkey(self.nodes[0].dumpprivkey(non_hd_add)) @@ -65,6 +70,11 @@ class WalletHDTest(BitcoinTestFramework): self.nodes[0].sendtoaddress(non_hd_add, 1) self.nodes[0].generate(1) + #create an internal key (again) + change_addr = self.nodes[1].getrawchangeaddress() + change_addrV= self.nodes[1].validateaddress(change_addr); + assert_equal(change_addrV["hdkeypath"], "m/0'/1'/1'") #second internal child key + self.sync_all() assert_equal(self.nodes[1].getbalance(), num_hd_adds + 1) @@ -90,6 +100,15 @@ class WalletHDTest(BitcoinTestFramework): #connect_nodes_bi(self.nodes, 0, 1) assert_equal(self.nodes[1].getbalance(), num_hd_adds + 1) + #send a tx and make sure its using the internal chain for the changeoutput + txid = self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 1) + outs = self.nodes[1].decoderawtransaction(self.nodes[1].gettransaction(txid)['hex'])['vout']; + keypath = "" + for out in outs: + if out['value'] != 1: + keypath = self.nodes[1].validateaddress(out['scriptPubKey']['addresses'][0])['hdkeypath'] + + assert(keypath[0:7] == "m/0'/1'") if __name__ == '__main__': WalletHDTest().main () From d59531ddfc09f705c2ef3a33c41d6d910756da18 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 16 Jan 2017 08:56:37 +0100 Subject: [PATCH 02/23] Immediately return setKeyPool's size if HD or HD_SPLIT is disabled or not supported --- src/wallet/wallet.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 71c97a4e8..349dff4f2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2912,6 +2912,10 @@ size_t CWallet::KeypoolCountExternalKeys() { AssertLockHeld(cs_wallet); // setKeyPool + // immediately return setKeyPool's size if HD or HD_SPLIT is disabled or not supported + if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT)) + return setKeyPool.size(); + CWalletDB walletdb(strWalletFile); // count amount of external keys From 01de822c8dbc92d8039dd17de9193d72eb7fadd2 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 16 Jan 2017 08:57:31 +0100 Subject: [PATCH 03/23] Removed redundant IsLocked() check in NewKeyPool() --- src/wallet/wallet.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 349dff4f2..15dabad7f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2899,10 +2899,9 @@ bool CWallet::NewKeyPool() walletdb.ErasePool(nIndex); setKeyPool.clear(); - if (IsLocked()) + if (!TopUpKeyPool()) { return false; - - TopUpKeyPool(); + } LogPrintf("CWallet::NewKeyPool rewrote keypool\n"); } return true; From 05a9b493eb93605c8d147634aecfd8bc92006847 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 16 Jan 2017 09:00:23 +0100 Subject: [PATCH 04/23] Fix wrong keypool internal size in RPC getwalletinfo help --- src/wallet/rpcwallet.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 7ab01040f..4e91c0be9 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -1852,7 +1852,7 @@ UniValue gettransaction(const JSONRPCRequest& request) " \"fee\": x.xxx, (numeric) The amount of the fee in " + CURRENCY_UNIT + ". This is negative and only available for the \n" " 'send' category of transactions.\n" " \"abandoned\": xxx (bool) 'true' if the transaction has been abandoned (inputs are respendable). Only available for the \n" - " 'send' category of transactions.\n" + " 'send' category of transactions.\n" " }\n" " ,...\n" " ],\n" @@ -2424,8 +2424,8 @@ UniValue getwalletinfo(const JSONRPCRequest& request) " \"immature_balance\": xxxxxx, (numeric) the total immature balance of the wallet in " + CURRENCY_UNIT + "\n" " \"txcount\": xxxxxxx, (numeric) the total number of transactions in the wallet\n" " \"keypoololdest\": xxxxxx, (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool\n" - " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated\n" - " \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (change outputs, 10% of the -keypoolsize target, only appears if HD is enabled)\n" + " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated (excluding internal-/chnage-output keys)\n" + " \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (change outputs, 20% of the -keypoolsize target, only appears if HD is enabled)\n" " \"unlocked_until\": ttt, (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n" " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" " \"hdmasterkeyid\": \"\" (string) the Hash160 of the HD master pubkey\n" From 469a47b7601bddeba052415efe95d0e8b0a7b05e Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 16 Jan 2017 09:05:27 +0100 Subject: [PATCH 05/23] Make sure ReserveKeyFromKeyPool only hands out internal keys if HD_SPLIT is supported --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 15dabad7f..2707567e8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3001,7 +3001,7 @@ void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool int throw std::runtime_error(std::string(__func__) + ": read failed"); if (!HaveKey(tmpKeypool.vchPubKey.GetID())) throw std::runtime_error(std::string(__func__) + ": unknown key in key pool"); - if (!IsHDEnabled() || tmpKeypool.fInternal == internal) + if (!IsHDEnabled() || (tmpKeypool.fInternal == internal && CanSupportFeature(FEATURE_HD_SPLIT))) { nIndex = id; keypool = tmpKeypool; From 9af8f00a7530934d4c9c64eb6c2676ac80b24ace Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 16 Jan 2017 11:08:00 +0100 Subject: [PATCH 06/23] Make sure we hand out keypool keys if HD_SPLIT is not enabled --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2707567e8..9c5172366 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3001,7 +3001,7 @@ void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool int throw std::runtime_error(std::string(__func__) + ": read failed"); if (!HaveKey(tmpKeypool.vchPubKey.GetID())) throw std::runtime_error(std::string(__func__) + ": unknown key in key pool"); - if (!IsHDEnabled() || (tmpKeypool.fInternal == internal && CanSupportFeature(FEATURE_HD_SPLIT))) + if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT) || tmpKeypool.fInternal == internal) { nIndex = id; keypool = tmpKeypool; From d0a627a53aa8cc5cd190f54c95b888dff88a4d37 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 16 Jan 2017 11:10:12 +0100 Subject: [PATCH 07/23] Fix issue where CDataStream->nVersion was taken a CKeyPool record version --- src/wallet/wallet.cpp | 1 + src/wallet/wallet.h | 15 ++++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 9c5172366..488e6690c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3933,6 +3933,7 @@ bool CWallet::BackupWallet(const std::string& strDest) CKeyPool::CKeyPool() { nTime = GetTime(); + fInternal = false; } CKeyPool::CKeyPool(const CPubKey& vchPubKeyIn, bool internalIn) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index bf6c5a2a8..d93830f08 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -114,12 +114,17 @@ public: READWRITE(nVersion); READWRITE(nTime); READWRITE(vchPubKey); - if (nVersion >= FEATURE_HD_SPLIT) - READWRITE(fInternal); - else - { - if (ser_action.ForRead()) + if (ser_action.ForRead()) { + try { + READWRITE(fInternal); + } + catch (...) { + /* flag as external address if we can't read the internal boolean */ fInternal = false; + } + } + else { + READWRITE(fInternal); } } }; From bcafca1077cf789ba79d16501a8cbb5d692bf8e6 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 16 Jan 2017 11:22:30 +0100 Subject: [PATCH 08/23] Make sure we always generate one keypool key at minimum --- src/wallet/wallet.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 488e6690c..e7ac918b2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2950,9 +2950,9 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) // generate +20% internal keys (minimum 2 keys) int64_t amountExternal = KeypoolCountExternalKeys(); int64_t amountInternal = setKeyPool.size() - amountExternal; - int64_t targetInternal = max((int64_t)ceil(nTargetSize * 0.2), (int64_t) 2); - int64_t missingExternal = max( (int64_t)(nTargetSize - amountExternal), (int64_t) 0); - int64_t missingInternal = max(targetInternal - amountInternal, (int64_t) 0); + int64_t targetInternal = std::max((int64_t)ceil(nTargetSize * 0.2), (int64_t) 2); + int64_t missingExternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - amountExternal, (int64_t) 0); + int64_t missingInternal = std::max(targetInternal - amountInternal, (int64_t) 0); if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT)) { From 79df9df3482d71391dee1dd59054aed8f7bbfa6b Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Tue, 17 Jan 2017 08:55:30 +0100 Subject: [PATCH 09/23] Switch to 100% for the HD internal keypool size --- src/wallet/rpcwallet.cpp | 4 ++-- src/wallet/wallet.cpp | 6 ++---- test/functional/fundrawtransaction.py | 1 - test/functional/keypool.py | 10 +++++++--- test/functional/wallet-dump.py | 6 +++--- 5 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 4e91c0be9..f07c187c8 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2424,8 +2424,8 @@ UniValue getwalletinfo(const JSONRPCRequest& request) " \"immature_balance\": xxxxxx, (numeric) the total immature balance of the wallet in " + CURRENCY_UNIT + "\n" " \"txcount\": xxxxxxx, (numeric) the total number of transactions in the wallet\n" " \"keypoololdest\": xxxxxx, (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool\n" - " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated (excluding internal-/chnage-output keys)\n" - " \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (change outputs, 20% of the -keypoolsize target, only appears if HD is enabled)\n" + " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated (only counts external keys)\n" + " \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (used for change outputs, only appears if HD is enabled otherwise there is no need for internal keys)\n" " \"unlocked_until\": ttt, (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n" " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" " \"hdmasterkeyid\": \"\" (string) the Hash160 of the HD master pubkey\n" diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e7ac918b2..7ad48e37b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2946,13 +2946,11 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize) nTargetSize = std::max(GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0); // count amount of available keys (internal, external) - // make sure the keypool of external keys fits the user selected target (-keypool) - // generate +20% internal keys (minimum 2 keys) + // make sure the keypool of external and internal keys fits the user selected target (-keypool) int64_t amountExternal = KeypoolCountExternalKeys(); int64_t amountInternal = setKeyPool.size() - amountExternal; - int64_t targetInternal = std::max((int64_t)ceil(nTargetSize * 0.2), (int64_t) 2); int64_t missingExternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - amountExternal, (int64_t) 0); - int64_t missingInternal = std::max(targetInternal - amountInternal, (int64_t) 0); + int64_t missingInternal = std::max(std::max((int64_t) nTargetSize, (int64_t) 1) - amountInternal, (int64_t) 0); if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT)) { diff --git a/test/functional/fundrawtransaction.py b/test/functional/fundrawtransaction.py index a28ff9465..6bb7d7f33 100755 --- a/test/functional/fundrawtransaction.py +++ b/test/functional/fundrawtransaction.py @@ -468,7 +468,6 @@ class RawTransactionsTest(BitcoinTestFramework): # drain the keypool self.nodes[1].getnewaddress() self.nodes[1].getrawchangeaddress() - self.nodes[1].getrawchangeaddress() inputs = [] outputs = {self.nodes[0].getnewaddress():1.1} rawTx = self.nodes[1].createrawtransaction(inputs, outputs) diff --git a/test/functional/keypool.py b/test/functional/keypool.py index 26585fc4a..cb9ab688d 100755 --- a/test/functional/keypool.py +++ b/test/functional/keypool.py @@ -29,17 +29,21 @@ class KeyPoolTest(BitcoinTestFramework): assert(addr_data['hdmasterkeyid'] == wallet_info['hdmasterkeyid']) assert_raises_jsonrpc(-12, "Error: Keypool ran out, please call keypoolrefill first", nodes[0].getnewaddress) - # put six (plus 2) new keys in the keypool (100% external-, +20% internal-keys, 2 in min) + # put six (plus 2) new keys in the keypool (100% external-, +100% internal-keys, 1 in min) nodes[0].walletpassphrase('test', 12000) nodes[0].keypoolrefill(6) nodes[0].walletlock() wi = nodes[0].getwalletinfo() - assert_equal(wi['keypoolsize_hd_internal'], 2) + assert_equal(wi['keypoolsize_hd_internal'], 6) assert_equal(wi['keypoolsize'], 6) # drain the internal keys nodes[0].getrawchangeaddress() nodes[0].getrawchangeaddress() + nodes[0].getrawchangeaddress() + nodes[0].getrawchangeaddress() + nodes[0].getrawchangeaddress() + nodes[0].getrawchangeaddress() addr = set() # the next one should fail assert_raises_jsonrpc(-12, "Keypool ran out", nodes[0].getrawchangeaddress) @@ -72,7 +76,7 @@ class KeyPoolTest(BitcoinTestFramework): nodes[0].walletpassphrase('test', 100) nodes[0].keypoolrefill(100) wi = nodes[0].getwalletinfo() - assert_equal(wi['keypoolsize_hd_internal'], 20) + assert_equal(wi['keypoolsize_hd_internal'], 100) assert_equal(wi['keypoolsize'], 100) def __init__(self): diff --git a/test/functional/wallet-dump.py b/test/functional/wallet-dump.py index 7eaa8090b..8876f935a 100755 --- a/test/functional/wallet-dump.py +++ b/test/functional/wallet-dump.py @@ -88,7 +88,7 @@ class WalletDumpTest(BitcoinTestFramework): read_dump(tmpdir + "/node0/wallet.unencrypted.dump", addrs, None) assert_equal(found_addr, test_addr_count) # all keys must be in the dump assert_equal(found_addr_chg, 50) # 50 blocks where mined - assert_equal(found_addr_rsv, 90*1.2) # 90 keys plus 20% internal keys + assert_equal(found_addr_rsv, 90*2) # 90 keys plus 100% internal keys #encrypt wallet, restart, unlock and dump self.nodes[0].encryptwallet('test') @@ -102,8 +102,8 @@ class WalletDumpTest(BitcoinTestFramework): found_addr, found_addr_chg, found_addr_rsv, hd_master_addr_enc = \ read_dump(tmpdir + "/node0/wallet.encrypted.dump", addrs, hd_master_addr_unenc) assert_equal(found_addr, test_addr_count) - assert_equal(found_addr_chg, 90*1.2 + 50) # old reserve keys are marked as change now - assert_equal(found_addr_rsv, 90*1.2) + assert_equal(found_addr_chg, 90*2 + 50) # old reserve keys are marked as change now + assert_equal(found_addr_rsv, 90*2) if __name__ == '__main__': WalletDumpTest().main () From dd526c2a2d2f0f7427047891a5e94b4e6c18b190 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Tue, 17 Jan 2017 09:07:48 +0100 Subject: [PATCH 10/23] Don't switch to HD-chain-split during wallet encryption of non HD-chain-split wallets --- src/wallet/wallet.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7ad48e37b..768781079 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1306,10 +1306,6 @@ CPubKey CWallet::GenerateNewHDMasterKey() bool CWallet::SetHDMasterKey(const CPubKey& pubkey) { LOCK(cs_wallet); - - // ensure this wallet.dat can only be opened by clients supporting HD with chain split - SetMinVersion(FEATURE_HD_SPLIT); - // store the keyid (hash160) together with // the child index counter in the database // as a hdchain object @@ -3670,6 +3666,9 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) CPubKey masterPubKey = walletInstance->GenerateNewHDMasterKey(); if (!walletInstance->SetHDMasterKey(masterPubKey)) throw std::runtime_error(std::string(__func__) + ": Storing master key failed"); + + // ensure this wallet.dat can only be opened by clients supporting HD with chain split + walletInstance->SetMinVersion(FEATURE_HD_SPLIT); } CPubKey newDefaultKey; if (walletInstance->GetKeyFromPool(newDefaultKey, false)) { From add38d9b83fe570a16ccda8e3c2642cb3b0b1f2f Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Tue, 17 Jan 2017 09:43:12 +0100 Subject: [PATCH 11/23] GetOldestKeyPoolTime: if HD & HD Chain Split is enabled, response max(oldest-internal-key, oldest-external-key) --- src/wallet/wallet.cpp | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 768781079..7da45587b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3056,9 +3056,27 @@ int64_t CWallet::GetOldestKeyPoolTime() if (setKeyPool.empty()) return GetTime(); - // load oldest key from keypool, get time and return CKeyPool keypool; CWalletDB walletdb(strWalletFile); + + if (IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT)) + { + // if HD & HD Chain Split is enabled, response max(oldest-internal-key, oldest-external-key) + int64_t now = GetTime(); + int64_t oldest_external = now, oldest_internal = now; + + for(const int64_t& id : setKeyPool) + { + if (!walletdb.ReadPool(id, keypool)) + throw std::runtime_error(std::string(__func__) + ": read failed"); + if (keypool.fInternal && keypool.nTime < oldest_internal) + oldest_internal = keypool.nTime; + else if (!keypool.fInternal && keypool.nTime < oldest_external) + oldest_external = keypool.nTime; + } + return std::max(oldest_internal, oldest_external); + } + // load oldest key from keypool, get time and return int64_t nIndex = *(setKeyPool.begin()); if (!walletdb.ReadPool(nIndex, keypool)) throw std::runtime_error(std::string(__func__) + ": read oldest key in keypool failed"); From e138876f0abc6e13a8240f99e4143712fc02ec92 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Tue, 17 Jan 2017 15:50:11 +0100 Subject: [PATCH 12/23] Only show keypoolsize_hd_internal if HD split is enabled --- src/wallet/rpcwallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index f07c187c8..62aefc4bc 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2425,7 +2425,7 @@ UniValue getwalletinfo(const JSONRPCRequest& request) " \"txcount\": xxxxxxx, (numeric) the total number of transactions in the wallet\n" " \"keypoololdest\": xxxxxx, (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool\n" " \"keypoolsize\": xxxx, (numeric) how many new keys are pre-generated (only counts external keys)\n" - " \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (used for change outputs, only appears if HD is enabled otherwise there is no need for internal keys)\n" + " \"keypoolsize_hd_internal\": xxxx, (numeric) how many new keys are pre-generated for internal use (used for change outputs, only appears if the wallet is using this feature, otherwise external keys are used)\n" " \"unlocked_until\": ttt, (numeric) the timestamp in seconds since epoch (midnight Jan 1 1970 GMT) that the wallet is unlocked for transfers, or 0 if the wallet is locked\n" " \"paytxfee\": x.xxxx, (numeric) the transaction fee configuration, set in " + CURRENCY_UNIT + "/kB\n" " \"hdmasterkeyid\": \"\" (string) the Hash160 of the HD master pubkey\n" @@ -2447,7 +2447,7 @@ UniValue getwalletinfo(const JSONRPCRequest& request) obj.push_back(Pair("keypoololdest", pwallet->GetOldestKeyPoolTime())); obj.push_back(Pair("keypoolsize", (int64_t)kpExternalSize)); CKeyID masterKeyID = pwallet->GetHDChain().masterKeyID; - if (!masterKeyID.IsNull()) { + if (!masterKeyID.IsNull() && pwallet->CanSupportFeature(FEATURE_HD_SPLIT)) { obj.push_back(Pair("keypoolsize_hd_internal", (int64_t)(pwallet->GetKeyPoolSize() - kpExternalSize))); } if (pwallet->IsCrypted()) { From 58e148333eb4b7ad87154a8b4d45ff0ebc3c9591 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Tue, 17 Jan 2017 16:04:16 +0100 Subject: [PATCH 13/23] CKeyPool avoid "catch (...)" in SerializationOp --- src/wallet/wallet.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d93830f08..4b40da168 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -118,7 +118,7 @@ public: try { READWRITE(fInternal); } - catch (...) { + catch (std::ios_base::failure&) { /* flag as external address if we can't read the internal boolean */ fInternal = false; } From 1090502c3e90f3f24fdc6c1e74f62d7669697b31 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 26 Jan 2017 20:52:57 +0100 Subject: [PATCH 14/23] Fix superfluous cast and code style nits in RPC wallet-hd.py test --- src/wallet/rpcwallet.cpp | 3 ++- test/functional/wallet-hd.py | 8 ++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 62aefc4bc..c715da8e9 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2438,7 +2438,8 @@ UniValue getwalletinfo(const JSONRPCRequest& request) LOCK2(cs_main, pwallet->cs_wallet); UniValue obj(UniValue::VOBJ); - size_t kpExternalSize = (int)pwallet->KeypoolCountExternalKeys(); + + size_t kpExternalSize = pwalletMain->KeypoolCountExternalKeys(); obj.push_back(Pair("walletversion", pwallet->GetVersion())); obj.push_back(Pair("balance", ValueFromAmount(pwallet->GetBalance()))); obj.push_back(Pair("unconfirmed_balance", ValueFromAmount(pwallet->GetUnconfirmedBalance()))); diff --git a/test/functional/wallet-hd.py b/test/functional/wallet-hd.py index b17f78843..64a6c9278 100755 --- a/test/functional/wallet-hd.py +++ b/test/functional/wallet-hd.py @@ -42,7 +42,7 @@ class WalletHDTest(BitcoinTestFramework): masterkeyid = self.nodes[1].getwalletinfo()['hdmasterkeyid'] assert_equal(len(masterkeyid), 40) - #create an internal key + # create an internal key change_addr = self.nodes[1].getrawchangeaddress() change_addrV= self.nodes[1].validateaddress(change_addr); assert_equal(change_addrV["hdkeypath"], "m/0'/1'/0'") #first internal child key @@ -70,7 +70,7 @@ class WalletHDTest(BitcoinTestFramework): self.nodes[0].sendtoaddress(non_hd_add, 1) self.nodes[0].generate(1) - #create an internal key (again) + # create an internal key (again) change_addr = self.nodes[1].getrawchangeaddress() change_addrV= self.nodes[1].validateaddress(change_addr); assert_equal(change_addrV["hdkeypath"], "m/0'/1'/1'") #second internal child key @@ -100,7 +100,7 @@ class WalletHDTest(BitcoinTestFramework): #connect_nodes_bi(self.nodes, 0, 1) assert_equal(self.nodes[1].getbalance(), num_hd_adds + 1) - #send a tx and make sure its using the internal chain for the changeoutput + # send a tx and make sure its using the internal chain for the changeoutput txid = self.nodes[1].sendtoaddress(self.nodes[0].getnewaddress(), 1) outs = self.nodes[1].decoderawtransaction(self.nodes[1].gettransaction(txid)['hex'])['vout']; keypath = "" @@ -108,7 +108,7 @@ class WalletHDTest(BitcoinTestFramework): if out['value'] != 1: keypath = self.nodes[1].validateaddress(out['scriptPubKey']['addresses'][0])['hdkeypath'] - assert(keypath[0:7] == "m/0'/1'") + assert_equal(keypath[0:7], "m/0'/1'") if __name__ == '__main__': WalletHDTest().main () From d9638e5aa46cd7e5fce4392f5333bbc556014160 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 26 Jan 2017 21:02:55 +0100 Subject: [PATCH 15/23] Overhaul the internal/external key derive switch --- src/wallet/wallet.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7da45587b..733afd49c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -145,17 +145,19 @@ void CWallet::DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool inter // always derive hardened keys // childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649 - chainChildKey.Derive(childKey, (internal ? hdChain.nInternalChainCounter : hdChain.nExternalChainCounter) | BIP32_HARDENED_KEY_LIMIT); - metadata.hdKeypath = "m/0'/" + std::string(internal ? "1'/"+ std::to_string(hdChain.nInternalChainCounter) : "0'/" + std::to_string(hdChain.nExternalChainCounter)) + "'"; - metadata.hdMasterKeyID = hdChain.masterKeyID; - // increment childkey index - if (internal) + if (internal) { + chainChildKey.Derive(childKey, hdChain.nInternalChainCounter | BIP32_HARDENED_KEY_LIMIT); + metadata.hdKeypath = "m/0'/1'/" + std::to_string(hdChain.nInternalChainCounter) + "'"; hdChain.nInternalChainCounter++; - else + } + else { + chainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT); + metadata.hdKeypath = "m/0'/0'/" + std::to_string(hdChain.nExternalChainCounter) + "'"; hdChain.nExternalChainCounter++; + } } while (HaveKey(childKey.key.GetPubKey().GetID())); secret = childKey.key; - + metadata.hdMasterKeyID = hdChain.masterKeyID; // update the chain model in the database if (!CWalletDB(strWalletFile).WriteHDChain(hdChain)) throw std::runtime_error(std::string(__func__) + ": Writing HD chain model failed"); From 003e1974986d62fa7c8112009d6fa1ee5871550e Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Thu, 26 Jan 2017 21:04:05 +0100 Subject: [PATCH 16/23] Remove FEATURE_HD_SPLIT bump TODO --- src/wallet/wallet.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 4b40da168..21c7f47f9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -87,7 +87,6 @@ enum WalletFeature FEATURE_HD = 130000, // Hierarchical key derivation after BIP32 (HD Wallet) - //TODO: FEATURE_HD_SPLIT needs to be bumped to 140000 once we branch of 0.14 // FEATURE_HD_SPLIT = 139900, // Wallet with HD chain split (change outputs will use m/0'/1'/k) FEATURE_LATEST = FEATURE_COMPRPUBKEY // HD is optional, use FEATURE_COMPRPUBKEY as latest version From 1b3b5c6f8fa6aff93935319888bf0cfd242359dd Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Wed, 1 Feb 2017 09:12:13 +0100 Subject: [PATCH 17/23] Slightly modify fundrawtransaction.py test (change getnewaddress() into getrawchangeaddress()) --- test/functional/fundrawtransaction.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/fundrawtransaction.py b/test/functional/fundrawtransaction.py index 6bb7d7f33..69f2c29f5 100755 --- a/test/functional/fundrawtransaction.py +++ b/test/functional/fundrawtransaction.py @@ -646,7 +646,7 @@ class RawTransactionsTest(BitcoinTestFramework): if out['value'] > 1.0: changeaddress += out['scriptPubKey']['addresses'][0] assert(changeaddress != "") - nextaddr = self.nodes[3].getnewaddress() + nextaddr = self.nodes[3].getrawchangeaddress() # frt should not have removed the key from the keypool assert(changeaddress == nextaddr) From 771a304ffe3c074a8ca1cdfb83d08379a5516e88 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Fri, 24 Mar 2017 10:53:35 +0100 Subject: [PATCH 18/23] Make sure we set the wallets min version to FEATURE_HD_SPLIT at the very first point --- src/wallet/wallet.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 733afd49c..0a0687975 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3682,13 +3682,14 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) { // Create new keyUser and set as default key if (GetBoolArg("-usehd", DEFAULT_USE_HD_WALLET) && !walletInstance->IsHDEnabled()) { + + // ensure this wallet.dat can only be opened by clients supporting HD with chain split + walletInstance->SetMinVersion(FEATURE_HD_SPLIT); + // generate a new master key CPubKey masterPubKey = walletInstance->GenerateNewHDMasterKey(); if (!walletInstance->SetHDMasterKey(masterPubKey)) throw std::runtime_error(std::string(__func__) + ": Storing master key failed"); - - // ensure this wallet.dat can only be opened by clients supporting HD with chain split - walletInstance->SetMinVersion(FEATURE_HD_SPLIT); } CPubKey newDefaultKey; if (walletInstance->GetKeyFromPool(newDefaultKey, false)) { From ed79e4f497a9a7747b006a5cb04d4173a6bbc1c5 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Fri, 24 Mar 2017 10:54:48 +0100 Subject: [PATCH 19/23] Optimize GetOldestKeyPoolTime(), return as soon as we have both oldest keys --- src/wallet/wallet.cpp | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0a0687975..d7d2927ba 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3069,12 +3069,18 @@ int64_t CWallet::GetOldestKeyPoolTime() for(const int64_t& id : setKeyPool) { - if (!walletdb.ReadPool(id, keypool)) + if (!walletdb.ReadPool(id, keypool)) { throw std::runtime_error(std::string(__func__) + ": read failed"); - if (keypool.fInternal && keypool.nTime < oldest_internal) + } + if (keypool.fInternal && keypool.nTime < oldest_internal) { oldest_internal = keypool.nTime; - else if (!keypool.fInternal && keypool.nTime < oldest_external) + } + else if (!keypool.fInternal && keypool.nTime < oldest_external) { oldest_external = keypool.nTime; + } + if (oldest_internal != now && oldest_external != now) { + break; + } } return std::max(oldest_internal, oldest_external); } From cd468d07d5dc3bc953f3fa07784aa842ecfd6b9f Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Fri, 24 Mar 2017 10:57:31 +0100 Subject: [PATCH 20/23] Define CWallet::DeriveNewChildKey() as private --- src/wallet/wallet.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 21c7f47f9..0b762cd89 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -118,7 +118,8 @@ public: READWRITE(fInternal); } catch (std::ios_base::failure&) { - /* flag as external address if we can't read the internal boolean */ + /* flag as external address if we can't read the internal boolean + (this will be the case for any wallet before the HD chain split version) */ fInternal = false; } } @@ -663,6 +664,9 @@ private: /* the HD chain data model (external chain counters) */ CHDChain hdChain; + /* HD derive new child key (on internal or external chain) */ + void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal = false); + bool fFileBacked; std::set setKeyPool; @@ -791,7 +795,6 @@ public: * Generate a new key */ CPubKey GenerateNewKey(bool internal = false); - void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal = false); //! Adds a key to the store, and saves it to disk. bool AddKeyPubKey(const CKey& key, const CPubKey &pubkey) override; //! Adds a key to the store, without saving it to disk (used by LoadWallet) From 1df08d1580fbab9e58017cf6c1ecb73550bf6ed7 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Fri, 24 Mar 2017 10:57:55 +0100 Subject: [PATCH 21/23] Add assertion for CanSupportFeature(FEATURE_HD_SPLIT) --- src/wallet/wallet.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d7d2927ba..4e625f64d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -138,6 +138,7 @@ void CWallet::DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool inter masterKey.Derive(accountKey, BIP32_HARDENED_KEY_LIMIT); // derive m/0'/0' (external chain) OR m/0'/1' (internal chain) + assert(internal ? CanSupportFeature(FEATURE_HD_SPLIT) : true); accountKey.Derive(chainChildKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0)); // derive child key at next index, skip keys already known to the wallet From 9382f0425e87b30e4621e0e23a99d6e880ec2200 Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Mon, 27 Mar 2017 09:51:55 +0200 Subject: [PATCH 22/23] Do not break backward compatibility during wallet encryption --- src/wallet/wallet.cpp | 10 ++++++++-- src/wallet/wallet.h | 6 ++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4e625f64d..a99817958 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -639,7 +639,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) if (IsHDEnabled()) { CKey key; CPubKey masterPubKey = GenerateNewHDMasterKey(); - if (!SetHDMasterKey(masterPubKey)) + // preserve the old chains version to not break backward compatibility + CHDChain oldChain = GetHDChain(); + if (!SetHDMasterKey(masterPubKey, &oldChain)) return false; } @@ -1306,13 +1308,17 @@ CPubKey CWallet::GenerateNewHDMasterKey() return pubkey; } -bool CWallet::SetHDMasterKey(const CPubKey& pubkey) +bool CWallet::SetHDMasterKey(const CPubKey& pubkey, CHDChain *possibleOldChain) { LOCK(cs_wallet); // store the keyid (hash160) together with // the child index counter in the database // as a hdchain object CHDChain newHdChain; + if (possibleOldChain) { + // preserve the old chains version + newHdChain.nVersion = possibleOldChain->nVersion; + } newHdChain.masterKeyID = pubkey.GetID(); SetHDChain(newHdChain, false); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 0b762cd89..ccede6009 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1055,8 +1055,10 @@ public: /* Generates a new HD master key (will not be activated) */ CPubKey GenerateNewHDMasterKey(); - /* Set the current HD master key (will reset the chain child index counters) */ - bool SetHDMasterKey(const CPubKey& key); + /* Set the current HD master key (will reset the chain child index counters) + If possibleOldChain is provided, the parameters from the old chain (version) + will be preserved. */ + bool SetHDMasterKey(const CPubKey& key, CHDChain *possibleOldChain = nullptr); }; /** A key allocated from the key pool. */ From 4115af7ac78971617c4fb80b7db336ff6bf254db Mon Sep 17 00:00:00 2001 From: Jonas Schnelli Date: Tue, 28 Mar 2017 09:18:20 +0200 Subject: [PATCH 23/23] Fix rebase issue where pwalletMain was used instead of pwallet Ser./Deser. nInternalChainCounter as last element --- src/wallet/rpcwallet.cpp | 2 +- src/wallet/walletdb.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index c715da8e9..22b1e3522 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2439,7 +2439,7 @@ UniValue getwalletinfo(const JSONRPCRequest& request) UniValue obj(UniValue::VOBJ); - size_t kpExternalSize = pwalletMain->KeypoolCountExternalKeys(); + size_t kpExternalSize = pwallet->KeypoolCountExternalKeys(); obj.push_back(Pair("walletversion", pwallet->GetVersion())); obj.push_back(Pair("balance", ValueFromAmount(pwallet->GetBalance()))); obj.push_back(Pair("unconfirmed_balance", ValueFromAmount(pwallet->GetUnconfirmedBalance()))); diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 016296f25..271c1d66c 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -61,9 +61,9 @@ public: { READWRITE(this->nVersion); READWRITE(nExternalChainCounter); + READWRITE(masterKeyID); if (this->nVersion >= VERSION_HD_CHAIN_SPLIT) READWRITE(nInternalChainCounter); - READWRITE(masterKeyID); } void SetNull()