Use internal HD chain for change outputs (hd split)

jonasschnelli commented at 10:08 am on December 6, 2016: contributor

Main changes

This pull request will result in using m/0'/1'/k' for internal keys (change outputs) while keeping m/0'/0'/k' for external keys (getnewaddress).

There is still a single keypool. All keypool elements (CKeyPool) have now a flag (fInternal).
~~The keypool will be filled with 80% external keys (round ceil).~~
The keypool will be filled with 100% external keys + 20% (round ceil) internal keys.
getwalletinfo additionally reports keypoolsize_hd_internal (amount of internal key in the

Compatibility:

This hd split change will only affect new wallets. Current 0.13 HD wallets will continue to only use the external chain.
Using a 0.14 wallet in <0.14 is not possible (hd chain split requires 0.14)
Using a 0.13 wallet in 0.14 will result in continue using only the external chain (only m/0'/0'/k').

This change also fixes the keypool +1 offset.

jonasschnelli added the label Wallet on Dec 6, 2016

jonasschnelli added this to the milestone 0.14.0 on Dec 6, 2016

laanwj commented at 10:44 am on December 6, 2016: member

Interesting assertion error in CWallet::CreateRawTransaction

 0stderr:
 1bitcoind: ../../src/wallet/wallet.cpp:2395: bool CWallet::CreateTransaction(const std::vector<CRecipient>&, CWalletTx&, CReserveKey&, CAmount&, int&, std::string&, const CCoinControl*, bool): Assertion `ret' failed.
 2  File "/home/travis/build/bitcoin/bitcoin/qa/rpc-tests/test_framework/test_framework.py", line 145, in main
 3    self.run_test()
 4  File "/home/travis/build/bitcoin/bitcoin/build/../qa/rpc-tests/fundrawtransaction.py", line 498, in run_test
 5    fundedTx = self.nodes[1].fundrawtransaction(rawTx)
 6  File "/home/travis/build/bitcoin/bitcoin/qa/rpc-tests/test_framework/coverage.py", line 49, in __call__
 7    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
 8  File "/home/travis/build/bitcoin/bitcoin/qa/rpc-tests/test_framework/authproxy.py", line 150, in __call__
 9    response = self._request('POST', self.__url.path, postdata.encode('utf-8'))
10  File "/home/travis/build/bitcoin/bitcoin/qa/rpc-tests/test_framework/authproxy.py", line 130, in _request
11    self.__conn.request(method, path, postdata, headers)
12  File "/usr/lib/python3.4/http/client.py", line 1125, in request
13    self._send_request(method, url, body, headers)
14  File "/usr/lib/python3.4/http/client.py", line 1163, in _send_request
15    self.endheaders(body)
16  File "/usr/lib/python3.4/http/client.py", line 1121, in endheaders
17    self._send_output(message_body)
18  File "/usr/lib/python3.4/http/client.py", line 951, in _send_output
19    self.send(msg)
20  File "/usr/lib/python3.4/http/client.py", line 886, in send
21    self.connect()
22  File "/usr/lib/python3.4/http/client.py", line 863, in connect
23    self.timeout, self.source_address)
24  File "/usr/lib/python3.4/socket.py", line 512, in create_connection
25    raise err
26  File "/usr/lib/python3.4/socket.py", line 503, in create_connection
27    sock.connect(sa)

jonasschnelli commented at 12:53 pm on December 6, 2016: contributor

Travis is failing because it did what it is supposed to: finding bugs. Once #9295 is merged, travis should succeed.

in qa/rpc-tests/keypool.py: in 324c7f5818 outdated

54-        assert(len(addr) == 4)
55         # the next one should fail
56         try:
57             addr = nodes[0].getrawchangeaddress()
58-            raise AssertionError('Keypool should be exhausted after three addresses')
59+            raise AssertionError('Keypool should be exhausted after three internal addresses (20%)')

paveljanik commented at 1:39 pm on December 8, 2016:

Still three?

in qa/rpc-tests/keypool.py: in 324c7f5818 outdated

68+        addr.add(nodes[0].getnewaddress())
69+        assert(len(addr) == 6)
70+        # the next one should fail
71+        try:
72+            addr = nodes[0].getnewaddress()
73+            raise AssertionError('Keypool should be exhausted after three external addresses (ceil rounded 80% of six)')

paveljanik commented at 1:40 pm on December 8, 2016:

three?

instagibbs commented at 3:13 pm on January 9, 2017:

also, this is 100%, not 80% of 6. 100% vs 20%, not 80% vs 20%.

gmaxwell commented at 7:20 pm on December 10, 2016: contributor

Once #9295 is merged, travis should succeed.

Shall we find out?

jonasschnelli force-pushed on Dec 11, 2016

jonasschnelli commented at 10:16 am on December 11, 2016: contributor

Rebased.

jonasschnelli force-pushed on Dec 12, 2016

jonasschnelli commented at 8:37 am on December 12, 2016: contributor

Travis succeeds now.

in src/wallet/wallet.cpp: in 0971c35319 outdated

139@@ -140,19 +140,22 @@ void CWallet::DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret)
140     // use hardened derivation (child keys >= 0x80000000 are hardened after bip32)
141     masterKey.Derive(accountKey, BIP32_HARDENED_KEY_LIMIT);
142 
143-    // derive m/0'/0'
144-    accountKey.Derive(externalChainChildKey, BIP32_HARDENED_KEY_LIMIT);
145+    // derive m/0'/0' (external chain) OR m/0'/1' (internal chain)
146+    accountKey.Derive(externalChainChildKey, BIP32_HARDENED_KEY_LIMIT+(int)internal);

luke-jr commented at 7:19 pm on December 22, 2016:

Prefer to explicitly add (internal ? 1 : 0)

in src/wallet/wallet.cpp: in 0971c35319 outdated

151         // childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
152         // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
153-        externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
154-        metadata.hdKeypath = "m/0'/0'/" + std::to_string(hdChain.nExternalChainCounter) + "'";
155+        externalChainChildKey.Derive(childKey, (internal ? hdChain.nInternalChainCounter : hdChain.nExternalChainCounter) | BIP32_HARDENED_KEY_LIMIT);
156+        metadata.hdKeypath = "m/0'/" + std::string(internal ? "1'/"+ std::to_string(hdChain.nInternalChainCounter) : "0'/" + std::to_string(hdChain.nExternalChainCounter)) + "'";

luke-jr commented at 8:09 pm on December 22, 2016:

Seems like it would be better to just access the specific counter once with a post-increment, and use variables here.

in src/wallet/wallet.cpp: in 0971c35319 outdated

2840         else
2841             nTargetSize = max(GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);
2842 
2843-        while (setKeyPool.size() < (nTargetSize + 1))
2844+        // count amount of available keys (internal, external)
2845+        // try to generate 80% external keys

luke-jr commented at 8:13 pm on December 22, 2016:

IMO this is unintuitive ~~and can lead to loss~~. If users set a keypool size of 100, they expect to be able to safely generate 100 addresses between backups. I usually round down to 90 in advice to give some breathing room, but 80% here will break even that.

Therefore, the full keypool size should be ensured on both chains.

Edit: Forgot this was HD. Less critical in that case.

jonasschnelli commented at 8:34 pm on December 22, 2016:

Yes. This is a discussion point and I’d like to get others feedback (maybe @gmaxwell and @sipa). The question is, if you want a keypool of 100 external + 50 (TBD) internals = total 150 keys, if you set the keypool to 100.

IMO user given values should be respected. If the user chooses keypool=100, the keypool should contain 100 keys. But as said, no strong opinion on that.

in src/wallet/wallet.cpp: in 0971c35319 outdated

86@@ -87,7 +87,7 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const
87     return &(it->second);
88 }
89 
90-CPubKey CWallet::GenerateNewKey()
91+CPubKey CWallet::GenerateNewKey(bool internal)

luke-jr commented at 8:16 pm on December 22, 2016:

IMO it would be better to use an enum for internal vs external rather than a bool.

jonasschnelli commented at 3:29 pm on January 10, 2017:

IMO it would be better to use an enum for internal vs external rather than a bool.

Why? Either its internal or external. Once we have more configurable HD key derivation (ext-pubkey-derivation, hd-multisig) we could switch to a enum or a more flexible design.

luke-jr commented at 8:21 pm on January 12, 2017:

To avoid implicit casting when/if the type changes for this parameter. Also makes it clearer at the call-locations what the parameter is specifying.

luke-jr changes_requested

luke-jr commented at 8:16 pm on December 22, 2016: member

I don’t see code for upgrading existing HD wallets.

Does this implement a BIP that should be mentioned?

jonasschnelli force-pushed on Jan 6, 2017

jonasschnelli commented at 1:49 pm on January 6, 2017: contributor

Overhauled after recommendation from @sipa and @luke-jr.

The amount of external pre-generated keys now back at 100% from -keypool=(default 100).
The amount of internal pre-generated keys is +20% from -keypool=(default 100).
Pre-generate two internal keys at minimum

… this now results in always have 120% keys pre-generated (20% internal key)

jonasschnelli force-pushed on Jan 9, 2017

jonasschnelli commented at 7:46 am on January 9, 2017: contributor

Updated with a small nit-fix (https://github.com/bitcoin/bitcoin/pull/9294#discussion_r93689981) and a fix for wallet-dump.py RPC test. Travis should succeed now.

jonasschnelli force-pushed on Jan 9, 2017

in src/wallet/wallet.cpp: in 76c883cd39 outdated

150         // always derive hardened keys
151         // childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
152         // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
153-        externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
154-        metadata.hdKeypath = "m/0'/0'/" + std::to_string(hdChain.nExternalChainCounter) + "'";
155+        externalChainChildKey.Derive(childKey, (internal ? hdChain.nInternalChainCounter : hdChain.nExternalChainCounter) | BIP32_HARDENED_KEY_LIMIT);

instagibbs commented at 2:45 pm on January 9, 2017:

It’s called externalChainChildKey but it could be either.

in src/wallet/wallet.cpp: in 76c883cd39 outdated

1298@@ -1296,7 +1299,7 @@ bool CWallet::SetHDMasterKey(const CPubKey& pubkey)
1299     LOCK(cs_wallet);
1300 
1301     // ensure this wallet.dat can only be opened by clients supporting HD

instagibbs commented at 2:46 pm on January 9, 2017:

update comment

in src/wallet/wallet.cpp: in 76c883cd39 outdated

2858-            CKeyPool tmpKeypool;
2859-            if (!walletdb.ReadPool(id, tmpKeypool))
2860-                throw runtime_error(std::string(__func__) + ": read failed");
2861-            amountI += tmpKeypool.fInternal;
2862-            amountE += !tmpKeypool.fInternal;
2863+            // don't flag keypool keys internal if we don't use HD

instagibbs commented at 2:50 pm on January 9, 2017:

/HD/HD split/?

in src/wallet/wallet.cpp: in 76c883cd39 outdated

2847+        // count amount of available keys (internal, external)
2848+        // make sure the keypool of external keys fits the user selected target (-keypool)
2849+        // generate +20% internal keys (minimum 2 keys)
2850+        int64_t amountExternal = KeypoolCountExternalKeys();
2851+        int64_t amountInternal = setKeyPool.size() - amountExternal;
2852+        int64_t targetInternal = max((int64_t)ceil(nTargetSize * 0.2), (int64_t) 2);

instagibbs commented at 2:54 pm on January 9, 2017:

I find it a bit confusing that this value goes from 0(non-existant) to -keypool then to -keypool*.2. Perhaps have it start at 0 on previous commit, then set it to non-0 here (unless there is reasoning).

jonasschnelli commented at 3:51 pm on January 10, 2017:

I find it a bit confusing that this value goes from 0(non-existant) to -keypool then to -keypool*.2. Perhaps have it start at 0 on previous commit, then set it to non-0 here (unless there is reasoning).

Fixed. I decided to squash the two commit into a single one because the current PR is a non-splittable change.

TheBlueMatt commented at 2:38 am on January 14, 2017:

Can we just set the target for both internal and external to nTargetSize? It seems super non-user-friendly that the setting which used to mean “can create this many keys/transactions” now means “can create this many keys, or 1/5th as many transactions”. I dont think we care all that much about the performance hit, do we?

jonasschnelli commented at 10:24 am on January 16, 2017:

Some weeks back we discusses that and we came up with something that we should create less internal keys. But I’m fine with 100%/100%. Any objections?

TheBlueMatt commented at 9:02 pm on January 16, 2017:

I’d be fine with reducing the internal keypool significantly once we have rescan logic that can regenerate keys, but until then I think we absolutely need to have it be 100%.

in src/wallet/wallet.cpp: in 76c883cd39 outdated

3150@@ -3129,7 +3151,11 @@ bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal)
3151         if (nIndex != -1)
3152             vchPubKey = keypool.vchPubKey;
3153         else {
3154-            return false;
3155+            if (pwallet->IsLocked())

instagibbs commented at 2:56 pm on January 9, 2017:

Seems like a separate optimization?

jonasschnelli commented at 3:41 pm on January 10, 2017:

Right. This is no longer required because we always derive at minimum 2 internal keys. And ReserveKeyFromKeyPool will topup the keypool. Will remove that part.

in src/wallet/rpcwallet.cpp: in 76c883cd39 outdated

2310     obj.push_back(Pair("keypoololdest", pwalletMain->GetOldestKeyPoolTime()));
2311-    obj.push_back(Pair("keypoolsize",   (int)pwalletMain->GetKeyPoolSize()));
2312+    obj.push_back(Pair("keypoolsize", (int64_t)kpExternalSize));
2313+    CKeyID masterKeyID = pwalletMain->GetHDChain().masterKeyID;
2314+    if (!masterKeyID.IsNull())
2315+        obj.push_back(Pair("keypoolsize_hd_internal",   (int64_t)(pwalletMain->GetKeyPoolSize() - kpExternalSize)));

instagibbs commented at 3:05 pm on January 9, 2017:

Might make sense to explicitly count both types and use those counts if we foresee a future with more possible types rather than implicitly count total - external. You’d know better than me how likely that is.

jonasschnelli commented at 3:28 pm on January 10, 2017:

I think it make sense to keep this as it is: keypoolsize should be alined with the -keypool conf. That’s why keypoolsize in getwalletinfo responses only the external key-count in the keypool (-keypool conf arg also defines the external key count).

instagibbs commented at 4:01 pm on January 10, 2017:

Sorry, I agree with what you’re saying I just wasn’t clear enough. I suppose it’s along the lines of @luke-jr ’s comment about enum, keeping flexible for the future. Not a blocker, just a suggestion. That can be deferred.

jonasschnelli commented at 4:05 pm on January 10, 2017:

Right. This is generally a good idea. IMO the next possible HD extensions (far away) are maybe xpub watch-only-wallets, flexible keypath, hd-multisig. I can’t imagine any change the would require a third (or more then two) type(s) during key derivation as well as in the keypool.

luke-jr commented at 9:03 pm on January 12, 2017:

"keypoolsizes": {"internal": N, "external": N} sounds like a good idea here.

jonasschnelli commented at 7:18 am on January 13, 2017:

I though about that, but this would break the API while the current PRs change does not.

instagibbs commented at 3:19 pm on January 9, 2017: member

You might also want to have a test that ensures the ceil behavior is correct for the 100vs20 split as I think the tests only check evenly divisible or the min value of 2.

jonasschnelli force-pushed on Jan 10, 2017

jonasschnelli commented at 3:51 pm on January 10, 2017: contributor

Fixed @instagibbs points. Squashed into a single commit.

instagibbs commented at 4:04 pm on January 10, 2017: member

utACK 4065d5fd44b9ce68a688c82353822a1cf49efe53

jtimon commented at 6:16 pm on January 10, 2017: contributor

I’m not sure how this would work together with #8723 but at a first glance at the code, it seems to simplify things: concept ACK. Can you explain more about its interaction with #8723 and maybe suggest one of them to focus review on first?

jonasschnelli force-pushed on Jan 12, 2017

jonasschnelli commented at 7:46 pm on January 12, 2017: contributor

Rebased.

in src/wallet/walletdb.h: in 0a7b70aa7b outdated

60@@ -58,13 +61,16 @@ class CHDChain
61     {
62         READWRITE(this->nVersion);
63         READWRITE(nExternalChainCounter);
64+        if (this->nVersion >= VERSION_HD_CHAIN_SPLIT)

luke-jr commented at 8:19 pm on January 12, 2017:

This should set nInternalChainCounter for old versions - maybe use std::numeric_limits<uint32_t>::max() (and make it private, with all accessors throwing if it’s not a sufficient version)?

Also, braces, please.

jonasschnelli commented at 10:16 am on January 16, 2017:

Where do you see the need to set the nInternalChainCounter for old versions here? Old Versions will always have the nInternalChainCounter set the 0 over SetNull() during the constructor call.

A new chain (which then will enable VERSION_HD_CHAIN_SPLIT) can only be created when a new HD master key will be set which again will enable FEATURE_HD_SPLIT.

luke-jr commented at 5:49 pm on January 18, 2017:

It should be setup to prevent accidental usage. If any code were to attempt to use it without a wallet that supports it, we want to throw an error/exception, not silently generate keys we may not be able to recover.

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

 99@@ -100,7 +100,7 @@ CPubKey CWallet::GenerateNewKey()
100 
101     // use HD key derivation if HD was enabled during wallet creation
102     if (IsHDEnabled()) {
103-        DeriveNewChildKey(metadata, secret);
104+        DeriveNewChildKey(metadata, secret, (CanSupportFeature(FEATURE_HD_SPLIT) ? internal : false));

luke-jr commented at 8:22 pm on January 12, 2017:

Is there a reason to do this check here, rather than inside DeriveNewChildKey?

jonasschnelli commented at 7:20 am on January 13, 2017:

A design question, but IMO DeriveNewChildKey should execute the derivation and not care about wallet versions and supported features.

luke-jr commented at 5:14 pm on January 13, 2017:

Maybe move it to private: then?

TheBlueMatt commented at 11:57 pm on March 20, 2017:

Yea, really should be private, I think.

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

150         // always derive hardened keys
151         // childIndex | BIP32_HARDENED_KEY_LIMIT = derive childIndex in hardened child-index-range
152         // example: 1 | BIP32_HARDENED_KEY_LIMIT == 0x80000001 == 2147483649
153-        externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
154-        metadata.hdKeypath = "m/0'/0'/" + std::to_string(hdChain.nExternalChainCounter) + "'";
155+        chainChildKey.Derive(childKey, (internal ? hdChain.nInternalChainCounter : hdChain.nExternalChainCounter) | BIP32_HARDENED_KEY_LIMIT);

luke-jr commented at 8:24 pm on January 12, 2017:

Seems like this could be significantly simplified with:

0uint32_t &nChainCounter = (internal ? hdChain.nInternalChainCounter : hdChain.nExternalChainCounter);

theuni commented at 6:46 pm on January 13, 2017:

Since almost everything here is different depending on internal/external, I think it’d be clearer to just make it if (internal) and separate the behaviors

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

1296@@ -1294,8 +1297,8 @@ bool CWallet::SetHDMasterKey(const CPubKey& pubkey)
1297 {
1298     LOCK(cs_wallet);
1299 
1300-    // ensure this wallet.dat can only be opened by clients supporting HD
1301-    SetMinVersion(FEATURE_HD);
1302+    // ensure this wallet.dat can only be opened by clients supporting HD with chain split
1303+    SetMinVersion(FEATURE_HD_SPLIT);

luke-jr commented at 8:26 pm on January 12, 2017:

I think this won’t work if the user specifies a HD-but-not-split wallet version for -walletupgrade?

jonasschnelli commented at 7:22 am on January 13, 2017:

I think we should no longer allow non split HD versions.

TheBlueMatt commented at 2:34 am on January 14, 2017:

This also forcibly upgrades the user’s wallet (if from pre-split HD version) if they encrypt their wallet, which I think is unaccptable.

TheBlueMatt commented at 3:11 am on January 14, 2017:

Note that if you change this need to set the newHdChain’s version to the appropriate value prior to SetHDChain.

jonasschnelli commented at 10:26 am on January 16, 2017:

We could… one question is: if the users encrypt his 0.13 wallet (HD enabled but no chain split) in 0.14. Do we want to keep the non-hd split or do we want to enforce an upgrade to HD_Split. The later seems preferable from the users perspective (Is there a reason to not split the HD chains if we can?).

TheBlueMatt commented at 8:18 pm on January 16, 2017:

I believe it is policy to not upgrade a user’s wallet unless they have specifically requested we do so, so, no, I dont think we should use HD_Split at that point.

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

2879-            int64_t nIndex = i+1;
2880-            walletdb.WritePool(nIndex, CKeyPool(GenerateNewKey()));
2881-            setKeyPool.insert(nIndex);
2882-        }
2883-        LogPrintf("CWallet::NewKeyPool wrote %d new keys\n", nKeys);
2884+        TopUpKeyPool();

luke-jr commented at 8:29 pm on January 12, 2017:

Probably should check the return value here. Since TopUpKeyPool also checks IsLocked, perhaps replace it above?

jonasschnelli commented at 7:23 am on January 13, 2017:

The program can never reach this point in IsLocked() state because we return at L2871.

luke-jr commented at 5:13 pm on January 13, 2017:

TopUpKeyPool could return false for other reasons as well. There’s no need to check IsLocked at L2871; it’s redundant. So just remove that check and do:

0if (!TopUpKeyPool()) {
1    return false;
2}

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

2885+        LogPrintf("CWallet::NewKeyPool rewrote keypool\n");
2886     }
2887     return true;
2888 }
2889 
2890+size_t CWallet::KeypoolCountExternalKeys()

luke-jr commented at 8:32 pm on January 12, 2017:

Maybe KeypoolCountKeys(bool internal)?

jonasschnelli commented at 7:24 am on January 13, 2017:

Again a design question. setKeyPool.size() - KeypoolCountExternalKeys() = internal key count.

luke-jr commented at 5:12 pm on January 13, 2017:

That’s going to break as soon as we have more than internal/external key pools…

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

2893+
2894+    CWalletDB walletdb(strWalletFile);
2895+
2896+    // count amount of external keys
2897+    size_t amountE = 0;
2898+    for(const int64_t& id : setKeyPool)

luke-jr commented at 8:34 pm on January 12, 2017:

Iterating over the entire keypool, even if small (which is not guaranteed), seems excessive to do for every keypool top-up… especially if we’re reading the wallet db (ie, I/O) for each key.

jonasschnelli commented at 7:26 am on January 13, 2017:

I think we have no other reliable way here and the performance impact should be little. BDB should be pretty fast for that operation and we are only opening the DB once.

luke-jr commented at 5:12 pm on January 13, 2017:

Is there some reason not to have a vector or map of separate setKeyPools? (With the current setKeyPool a special case for external keys)

TheBlueMatt commented at 3:23 am on January 14, 2017:

Additionally, this is called at the entry to getwalletinfo, even with HD disabled. While I agree with @luke-jr that there should probably be two separate setKeyPools, at a minimum this function needs an if (!IsHDEnabled()) return setKeyPool.size(); to avoid causing massive slowdown for folks who are using non-hd wallets with massive keypools (to avoid constant backups).

jonasschnelli commented at 7:39 am on January 17, 2017:

Okay. This is fixed now (https://github.com/bitcoin/bitcoin/pull/9294/files#diff-b2bb174788c7409b671c46ccc86034bdR2886).

luke-jr commented at 5:53 pm on January 18, 2017:

That commit merely implements a fix for non-split-HD wallets, not for new wallets…

TheBlueMatt commented at 6:12 pm on January 18, 2017:

See comments later in the PR, the performance measurements that @jonasschnelli did indicate there is little to no issue for most uses…if we found out this is not true later, we can fix it as a regression-fix.

luke-jr commented at 8:19 pm on January 18, 2017:

Okay, I expected changing this would be a wallet file format change, but upon further investigation I see it is not.

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

2920+        // make sure the keypool of external keys fits the user selected target (-keypool)
2921+        // generate +20% internal keys (minimum 2 keys)
2922+        int64_t amountExternal = KeypoolCountExternalKeys();
2923+        int64_t amountInternal = setKeyPool.size() - amountExternal;
2924+        int64_t targetInternal = max((int64_t)ceil(nTargetSize * 0.2), (int64_t) 2);
2925+        int64_t missingExternal = max( (int64_t)(nTargetSize - amountExternal), (int64_t) 0);

luke-jr commented at 8:39 pm on January 12, 2017:

Shouldn’t the cast be directly on nTargetSize, rather than the result of the subtraction?

TheBlueMatt commented at 3:36 am on January 14, 2017:

This should be max(max(nTargetSize, 1) - amountExternal, 0) to avoid violating some of the assumptions about at least one keypool entry being available after TopUpKeyPool returns.

TheBlueMatt commented at 10:57 pm on January 16, 2017:

I believe this is a bug, which would need fixing prior to merge.

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

2925+        int64_t missingExternal = max( (int64_t)(nTargetSize - amountExternal), (int64_t) 0);
2926+        int64_t missingInternal = max(targetInternal - amountInternal, (int64_t) 0);
2927+
2928+        if (!IsHDEnabled() || !CanSupportFeature(FEATURE_HD_SPLIT))
2929+        {
2930+            // don't flag keypool keys internal if we don't use HD

luke-jr commented at 8:40 pm on January 12, 2017:

Also don’t create extra internal keys (already current code behaviour, but comment could be clearer).

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

2930+            // don't flag keypool keys internal if we don't use HD
2931+            missingInternal = 0;
2932+        }
2933+        bool internal = true;
2934+        CWalletDB walletdb(strWalletFile);
2935+        for(unsigned int i = 0; i < missingInternal + missingExternal; i++)

luke-jr commented at 8:43 pm on January 12, 2017:

Better to count down and avoid adding every iteration:

0for (int64_t i = missingInternal + missingExternal; i--; )

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

2933+        bool internal = true;
2934+        CWalletDB walletdb(strWalletFile);
2935+        for(unsigned int i = 0; i < missingInternal + missingExternal; i++)
2936         {
2937             int64_t nEnd = 1;
2938+            internal = (i < missingInternal);

luke-jr commented at 8:45 pm on January 12, 2017:

(Assuming counting down, and internal initialised to false above:)

0if (i < missingInternal) {
1    internal = true;
2}

in src/wallet/wallet.cpp: in 0a7b70aa7b outdated

2973+        {
2974+            CKeyPool tmpKeypool;
2975+            if (!walletdb.ReadPool(id, tmpKeypool))
2976+                throw runtime_error(std::string(__func__) + ": read failed");
2977+            if (!HaveKey(tmpKeypool.vchPubKey.GetID()))
2978+                throw runtime_error(std::string(__func__) + ": unknown key in key pool");

luke-jr commented at 8:48 pm on January 12, 2017:

These exceptions used to be thrown after the key was removed from the set, allowing a retry to succeed. That is no longer the case here, potentially leading to irrecoverable situations. I’m unsure if this is good or bad.

jonasschnelli commented at 7:34 pm on January 26, 2017:

Yes. There are arguments for and against throwing before or after removing. If we throw here, you already ran into (more) significant issues.

in src/wallet/wallet.h: in 0a7b70aa7b outdated

115@@ -111,6 +116,13 @@ class CKeyPool
116             READWRITE(nVersion);
117         READWRITE(nTime);
118         READWRITE(vchPubKey);
119+        if (nVersion >= FEATURE_HD_SPLIT)
120+            READWRITE(fInternal);
121+        else
122+        {
123+            if (ser_action.ForRead())
124+                fInternal = false;

luke-jr commented at 8:55 pm on January 12, 2017:

Maybe throw an exception if !ser_action.ForRead() && fInternal?

Either way, please add braces.

in src/wallet/wallet.h: in 0a7b70aa7b outdated

723@@ -712,8 +724,8 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
724      * keystore implementation
725      * Generate a new key
726      */
727-    CPubKey GenerateNewKey();
728-    void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret);
729+    CPubKey GenerateNewKey(bool internal = false);
730+    void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal = false);

luke-jr commented at 8:56 pm on January 12, 2017:

Do we want internal to have a default? Should it be false? IIRC currently new keys default to change unless added to the address book, so this default seems to contradict the status quo.

jonasschnelli commented at 7:31 am on January 13, 2017:

I have set the default to false to keep the exiting behaviour.

luke-jr commented at 5:10 pm on January 13, 2017:

My point is that the existing behaviour is closer to true than false.

in src/wallet/walletdb.h: in 0a7b70aa7b outdated

45@@ -46,9 +46,12 @@ class CHDChain
46 {
47 public:
48     uint32_t nExternalChainCounter;
49+    uint32_t nInternalChainCounter;
50     CKeyID masterKeyID; //!< master key hash160
51 
52-    static const int CURRENT_VERSION = 1;
53+    static const int VERSION_HD_BASE        = 1;

luke-jr commented at 8:58 pm on January 12, 2017:

Missing logic to initialise this version.

jonasschnelli commented at 7:32 am on January 13, 2017:

VERSION_HD_BASE is only there to identify versions that do not support HD SPLIT (first HD version introduced in 0.13).

in src/wallet/rpcwallet.cpp: in 0a7b70aa7b outdated

2322     obj.push_back(Pair("unconfirmed_balance", ValueFromAmount(pwalletMain->GetUnconfirmedBalance())));
2323     obj.push_back(Pair("immature_balance",    ValueFromAmount(pwalletMain->GetImmatureBalance())));
2324     obj.push_back(Pair("txcount",       (int)pwalletMain->mapWallet.size()));
2325     obj.push_back(Pair("keypoololdest", pwalletMain->GetOldestKeyPoolTime()));
2326-    obj.push_back(Pair("keypoolsize",   (int)pwalletMain->GetKeyPoolSize()));
2327+    obj.push_back(Pair("keypoolsize", (int64_t)kpExternalSize));

luke-jr commented at 9:02 pm on January 12, 2017:

keypoolsize should probably be the lower of the two and deprecated. There is no reason to assume prior use didn’t look at it for info on change keys.

in src/wallet/rpcwallet.cpp: in 0a7b70aa7b outdated

2324     obj.push_back(Pair("txcount",       (int)pwalletMain->mapWallet.size()));
2325     obj.push_back(Pair("keypoololdest", pwalletMain->GetOldestKeyPoolTime()));
2326-    obj.push_back(Pair("keypoolsize",   (int)pwalletMain->GetKeyPoolSize()));
2327+    obj.push_back(Pair("keypoolsize", (int64_t)kpExternalSize));
2328+    CKeyID masterKeyID = pwalletMain->GetHDChain().masterKeyID;
2329+    if (!masterKeyID.IsNull())

luke-jr commented at 9:04 pm on January 12, 2017:

Shouldn’t this use CanSupportFeature(FEATURE_HD_SPLIT)?

jonasschnelli commented at 7:18 am on January 13, 2017:

I think always reporting keypoolsize_hd_internal (with 0 in non HD-SPLIT case) can make parsing simpler.

luke-jr commented at 5:09 pm on January 13, 2017:

Then always report it? Right now it’s conditional on something more or less irrelevant.

But I’m not sure it makes sense to ever report 0 when there are indeed keys that will be used for internal outputs…

luke-jr changes_requested

jonasschnelli commented at 7:38 am on January 13, 2017: contributor

Added a commit that addresses some of @luke-jr points. Maybe we can try to not bike-shed this with to many design and code-style nits (braces, etc.) otherwise we will very likely delay this for 0.15.

in src/wallet/rpcwallet.cpp: in 12fcde55b7 outdated

2311+            "  \"unconfirmed_balance\": xxx,      (numeric) the total unconfirmed balance of the wallet in " + CURRENCY_UNIT + "\n"
2312+            "  \"immature_balance\": xxxxxx,      (numeric) the total immature balance of the wallet in " + CURRENCY_UNIT + "\n"
2313+            "  \"txcount\": xxxxxxx,              (numeric) the total number of transactions in the wallet\n"
2314+            "  \"keypoololdest\": xxxxxx,         (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool\n"
2315+            "  \"keypoolsize\": xxxx,             (numeric) how many new keys are pre-generated\n"
2316+            "  \"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"

TheBlueMatt commented at 3:18 am on January 14, 2017:

I think the 10% number is wrong? current code is 20, I believe? (and I think it should be changed to 100).

in src/wallet/rpcwallet.cpp: in 12fcde55b7 outdated

2320     obj.push_back(Pair("walletversion", pwalletMain->GetVersion()));
2321     obj.push_back(Pair("balance",       ValueFromAmount(pwalletMain->GetBalance())));
2322     obj.push_back(Pair("unconfirmed_balance", ValueFromAmount(pwalletMain->GetUnconfirmedBalance())));
2323     obj.push_back(Pair("immature_balance",    ValueFromAmount(pwalletMain->GetImmatureBalance())));
2324     obj.push_back(Pair("txcount",       (int)pwalletMain->mapWallet.size()));
2325     obj.push_back(Pair("keypoololdest", pwalletMain->GetOldestKeyPoolTime()));

TheBlueMatt commented at 3:20 am on January 14, 2017:

I think we really need to update this - keypoololdest is used to know when your backup has been invalidated. I know technically now that we have HD wallets arent invalidated, but until we have rebuilding-from-chain implemented I think we really need to make sure keypoololdest is still useable as “if your backup is older than this date, backup again”.

This means keypoololdest needs to be max(oldest internal keypool, oldest external keypool) isntead.

jonasschnelli commented at 8:01 am on January 16, 2017:

Agree. But probably in a separate PR.

TheBlueMatt commented at 9:05 pm on January 16, 2017:

Not doing this would break some uses of walletbackup, I think, so I wouldnt be too happy with this slipping into a different release.

ryanofsky commented at 7:16 pm on January 24, 2017:

Maybe add a TODO comment (though this does seem to me like a desirable change to include in this PR).

jonasschnelli commented at 7:32 pm on January 26, 2017:

Maybe add a TODO comment (though this does seem to me like a desirable change to include in this PR).

This is fixed in the current version.

in src/wallet/rpcwallet.cpp: in 12fcde55b7 outdated

2310+            "  \"balance\": xxxxxxx,              (numeric) the total confirmed balance of the wallet in " + CURRENCY_UNIT + "\n"
2311+            "  \"unconfirmed_balance\": xxx,      (numeric) the total unconfirmed balance of the wallet in " + CURRENCY_UNIT + "\n"
2312+            "  \"immature_balance\": xxxxxx,      (numeric) the total immature balance of the wallet in " + CURRENCY_UNIT + "\n"
2313+            "  \"txcount\": xxxxxxx,              (numeric) the total number of transactions in the wallet\n"
2314+            "  \"keypoololdest\": xxxxxx,         (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool\n"
2315+            "  \"keypoolsize\": xxxx,             (numeric) how many new keys are pre-generated\n"

TheBlueMatt commented at 3:25 am on January 14, 2017:

Maybe also mention that keypoolsize_hd_internal is NOT included in this count, as one might assume that external_keypool = keypoolsize - keypoolsize_hd_internal from these docs.

in src/wallet/wallet.cpp: in 12fcde55b7 outdated

2975+            CKeyPool tmpKeypool;
2976+            if (!walletdb.ReadPool(id, tmpKeypool))
2977+                throw runtime_error(std::string(__func__) + ": read failed");
2978+            if (!HaveKey(tmpKeypool.vchPubKey.GetID()))
2979+                throw runtime_error(std::string(__func__) + ": unknown key in key pool");
2980+            if (!IsHDEnabled() || tmpKeypool.fInternal == internal)

TheBlueMatt commented at 3:53 am on January 14, 2017:

You need to check if internal is enabled here, not just hd enabled, I think. GetReservedKey is used in a few places with internal set to true without checking if internal is enabled, and if its not, you could end up never succeeding in getting a key, since HD might be enabled, but they never find an internal key.

in src/wallet/wallet.h: in 12fcde55b7 outdated

115@@ -111,6 +116,13 @@ class CKeyPool
116             READWRITE(nVersion);
117         READWRITE(nTime);
118         READWRITE(vchPubKey);
119+        if (nVersion >= FEATURE_HD_SPLIT)

TheBlueMatt commented at 4:02 am on January 14, 2017:

I do not believe the serializer version is set to the wallet’s version. I believe it is the one set in src/wallet/db.h - CLIENT_VERSION.

jonasschnelli force-pushed on Jan 16, 2017

jonasschnelli commented at 10:33 am on January 16, 2017: contributor

Added a couple of fixes:

Most important, as @TheBlueMatt has discovered, the nVersion handling of CKeyPool was wrong. I have fixed it with a try{} to serialise the fInternal boolean.
Return setKeyPool.size() if hd or hd-chain-split is disabled
Removed redundant IsLocked() check
Fixed wrong keypool internal size in RPC help
Make sure we hand out keypool keys if HD_SPLIT is not enabled

I did not cover design-changes and non-important nits in order to get this merged today before the feature freeze. We can fix things and make it more elegant after a possible merge. I also not changed the keypool distribution (still 100%/20%). Can be done after the freeze and once we all agree whether we’d like to have 100/20 or 100/100.

btcdrak commented at 8:39 pm on January 16, 2017: contributor

ACK e34676e

in src/wallet/rpcwallet.cpp: in e34676ef42 outdated

2310+            "  \"balance\": xxxxxxx,              (numeric) the total confirmed balance of the wallet in " + CURRENCY_UNIT + "\n"
2311+            "  \"unconfirmed_balance\": xxx,      (numeric) the total unconfirmed balance of the wallet in " + CURRENCY_UNIT + "\n"
2312+            "  \"immature_balance\": xxxxxx,      (numeric) the total immature balance of the wallet in " + CURRENCY_UNIT + "\n"
2313+            "  \"txcount\": xxxxxxx,              (numeric) the total number of transactions in the wallet\n"
2314+            "  \"keypoololdest\": xxxxxx,         (numeric) the timestamp (seconds since Unix epoch) of the oldest pre-generated key in the key pool\n"
2315+            "  \"keypoolsize\": xxxx,             (numeric) how many new keys are pre-generated (excluding internal-/chnage-output keys)\n"

TheBlueMatt commented at 9:00 pm on January 16, 2017:

s/chnage/change/

TheBlueMatt commented at 9:08 pm on January 16, 2017:

Also, these docs are super confusing - the way I read it, if I have HD enabled I’ll never have a keypool for change-output keys, which isnt true - they’re just included here.

jonasschnelli commented at 8:47 am on January 17, 2017: contributor

Added three fixes for issues found by @TheBlueMatt and @luke-jr :

Switched to 100%/100% for the keypool distribution, also, make sure we always generate at least 1 key for each chain
Added a fix that ensures that we don’t accidentally upgrade a NON hd-chain-split wallet to hd-chain-split wallet during encryption (=HD reseed).
Fix GetOldestKeyPoolTime to response max(oldest_internal_key, oldest_external_key).

jonasschnelli force-pushed on Jan 17, 2017

TheBlueMatt commented at 3:15 pm on January 17, 2017: member

utACK a874b75a180a9e967a3d936cf46df11f8531b70a modulo performance concerns, but I’m more than happy to fix those up post-merge if needed, as they should be pretty easy to address

jonasschnelli commented at 3:43 pm on January 17, 2017: contributor

Performance on my Core i7 2.9 GHZ with a 5'000 keypool wallet.

This PR (results actually in 5k internal and 5k external keys = 10k keys) getnewaddress: real 0m0.219s

0.13 (5k keys, no chain split) getnewaddress: real 0m0.104s

luke-jr approved

luke-jr commented at 8:21 pm on January 18, 2017: member

utACK. It’s not perfect, but seems to be good enough.

in src/wallet/rpcwallet.cpp: in a874b75a18 outdated

2315@@ -2315,17 +2316,20 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
2316     LOCK2(cs_main, pwalletMain->cs_wallet);
2317 
2318     UniValue obj(UniValue::VOBJ);
2319+    size_t kpExternalSize = (int)pwalletMain->KeypoolCountExternalKeys();

ryanofsky commented at 6:11 pm on January 19, 2017:

(int) cast superfluous

in qa/rpc-tests/wallet-hd.py: in a874b75a18 outdated

33@@ -34,6 +34,11 @@ def run_test (self):
34         masterkeyid = self.nodes[1].getwalletinfo()['hdmasterkeyid']
35         assert_equal(len(masterkeyid), 40)
36 
37+        #create an internal key

ryanofsky commented at 6:13 pm on January 19, 2017:

Nit: new comments don’t follow existing style in test (space after #, capitalized)

in qa/rpc-tests/wallet-hd.py: in a874b75a18 outdated

 97+        outs = self.nodes[1].decoderawtransaction(self.nodes[1].gettransaction(txid)['hex'])['vout'];
 98+        keypath = ""
 99+        for out in outs:
100+            if out['value'] != 1:
101+                keypath = self.nodes[1].validateaddress(out['scriptPubKey']['addresses'][0])['hdkeypath']
102+

ryanofsky commented at 6:13 pm on January 19, 2017:

trailing whitespace here

in qa/rpc-tests/wallet-hd.py: in a874b75a18 outdated

 98+        keypath = ""
 99+        for out in outs:
100+            if out['value'] != 1:
101+                keypath = self.nodes[1].validateaddress(out['scriptPubKey']['addresses'][0])['hdkeypath']
102+        
103+        assert(keypath[0:7] == "m/0'/1'")

ryanofsky commented at 6:15 pm on January 19, 2017:

maybe use assert_equal

in qa/rpc-tests/keypool.py: in 9df9d2c2e1 outdated

80@@ -66,13 +81,18 @@ def run_test(self):
81         nodes[0].generate(1)
82         nodes[0].generate(1)
83         nodes[0].generate(1)
84-        nodes[0].generate(1)

ryanofsky commented at 6:46 pm on January 19, 2017:

Note: I was confused about why 4 generates were needed here previously to use up only 3 keys. But I think it was because TopupKeyPool used to create (nTargetSize + 1) keys instead of nTargetSize keys, which is fixed now.

btcdrak commented at 7:03 pm on January 19, 2017: contributor

utACK a874b75

in src/wallet/wallet.h: in a874b75a18 outdated

87@@ -88,6 +88,10 @@ enum WalletFeature
88     FEATURE_COMPRPUBKEY = 60000, // compressed public keys
89 
90     FEATURE_HD = 130000, // Hierarchical key derivation after BIP32 (HD Wallet)
91+
92+    //TODO: FEATURE_HD_SPLIT needs to be bumped to 140000 once we branch of 0.14 //

gmaxwell commented at 7:19 pm on January 19, 2017:

I’m completely confused by this? why would the version of the wallet change due to a branch here? won’t that break compatibility with wallets generated by git master and cause funds loss for them?

jonasschnelli commented at 7:44 pm on January 19, 2017:

We can keep 139999 for 0.14 to make it compatible with wallets created before branching of 0.14 made with this PR. Unsure what’s best practice here…

jonasschnelli commented at 8:25 pm on January 19, 2017:

from IRC

0[21:24:20]  <sipa>	[20:47:45] yeah, there is a race there
1[21:24:20]  <sipa>	[20:48:00] that's always the case when a feature needs to be tied to a version number
2[21:24:20]  <morcos>	[20:48:18] i don't see any problem leaving FEATURE_HD_SPLIT = 139900
3[21:24:20]  <jonasschnelli>	[20:48:21] sipa: keeping in 139999 looks bad but is efficient?
4[21:24:20]  <morcos>	[20:48:24] that seems the correct way to do it
5[21:24:20]  <jonasschnelli>	[20:48:41] Agree with morcos 
6[21:24:20]  <sipa>	[20:48:54] a better way would be to disentangle the wallet version number for the software version number
7[21:24:20]  <sipa>	[20:49:09] so the wallet version can just be bumped in the same PR as the feature is introduced

ryanofsky commented at 7:36 pm on January 24, 2017:

Sounds like you just need to remove the TODO.

jonasschnelli commented at 7:51 pm on January 26, 2017:

I think if we branch of 0.14 we would need to bump it to 1499000. If someone was using this patch in production… he’s.. in serious troubles anyways. :)

MarcoFalke commented at 7:54 pm on January 26, 2017:

It should not be required to bump it when it is in master, because some people are using master “in production” (i.e. on the main net).

in src/wallet/wallet.cpp: in d15e02b11a outdated

3046+            else if (!keypool.fInternal && keypool.nTime < oldest_external)
3047+                oldest_external = keypool.nTime;
3048+        }
3049+        return std::max(oldest_internal, oldest_external);
3050+    }
3051+    // load oldest key from keypool, get time and return

ryanofsky commented at 7:25 pm on January 19, 2017:

Could you add a comment mentioning why in this case it is safe to assume key with the lowest index is the oldest key, while in the above case, you need to loop through the keys to find the oldest one. It’s not clear (at least to me) why FEATURE_HD_SPLIT would affect key order.

jonasschnelli commented at 8:20 am on January 20, 2017:

I haven’t change this part,… but I feel with you that this is very likely a bug we carry around a long time. Haven’t verified it, but returning a CKeyPool done in (void CWallet::ReturnKey(int64_t nIndex)) will mess up the order assumption. If is a bug, we should fix it,… but maybe in a different PR.

jonasschnelli commented at 9:40 am on January 20, 2017:

I debugged that a little bit and it seems that this assumption (first item in setKeyPool is always oldest key) holds.

The std::set setKeyPool (always ordered) contains int64_t. CKeyPool instances remember that nIndex (index of the setKeyPool int64.
New CKeyPools will always insert int64_t nEnd = *(--setKeyPool.end()) + 1;.
Returning keys will keep the date order because the returned will be inserted at the position where it has been taken out.

The only problem I can see is when someone overflows int64 by creating 2^63+1 keys. But, well,… yes.

ryanofsky commented at 7:09 pm on January 24, 2017:

So it seems like there is no reason for having a FEATURE_HD_SPLIT special case here. Could your remove either the new block of code or the old block of code, and just keep one implementation?

jonasschnelli commented at 7:50 pm on January 26, 2017:

We still need the FEATURE_HD_SPLIT special case here. If don’t want to return just the oldest key in that case. Reason:

Assume people use GetOldestKeyPoolTime() to determine if it’s time for a new backup (difficult question when it’s HD anyways, but that actually what @TheBlueMatt came up in https://github.com/bitcoin/bitcoin/pull/9294/files#r96107555)
Assume your key in the keypool at index 0 (=oldest key) is an external key.
Now assume you call getrawchangeaddress multiple times and you only and completely refill your internal keypool.
In that case, you want to return the oldest internal key because its very likely newer then your oldest external key.

But maybe I’m wrong.

ryanofsky commented at 8:36 pm on January 26, 2017:

I think you’re right. Actually I was just confused because I misread std::max as std::min and couldn’t figure out what the new code was doing differently!

ryanofsky commented at 7:37 pm on January 19, 2017: member

utACK a874b75a180a9e967a3d936cf46df11f8531b70a. Suggested some minor changes, probably for a future PR since this needs to be merged soon.

jonasschnelli removed this from the milestone 0.14.0 on Jan 20, 2017

jonasschnelli added this to the milestone 0.15.0 on Jan 20, 2017

jonasschnelli commented at 7:12 pm on January 20, 2017: contributor

Removed 0.14 tag (added to 0.15 milestone). Let’s hope we can get this in after the 0.14 branch-off

ryanofsky approved

ryanofsky commented at 7:49 pm on January 24, 2017: member

a874b75a180a9e967a3d936cf46df11f8531b70a still looks good, though it would be nice if some of the outstanding comments here could be addressed in this PR or a followup.

jonasschnelli commented at 8:05 pm on January 26, 2017: contributor

Fixed most (all?) of @ryanofsky nits
Fixed @theuni’s internal/external design nit.
Removed the bump to 140000 TODO

ghost commented at 6:10 am on January 27, 2017: none

This split sounds interesting, I wanted to know about the motivation for this, but did not find any information neither in this PR nor by searching in the internet. Could You provide some information/link about motivation for this PR ?

sipa commented at 6:34 am on January 27, 2017: member

@wodry BIP32 specifies the use of separate chains for external addresses (those you give out to receive payments on) and internal addresses (used for sending change back to yourself). The initial BIP32 implementation in Bitcoin Core 0.13 did not implement this, and only had a single chain for both. That means you’re unable to distinguish what are payments and change in a potential future version that can autonomously recover from a wallet backup.

jonasschnelli commented at 8:11 am on February 1, 2017: contributor

Had to rebase after #9377 and slightly modify the fundrawtransaction.py test (change getnewaddress() into getrawchangeaddress()).

jonasschnelli force-pushed on Feb 1, 2017

jonasschnelli commented at 10:19 am on March 2, 2017: contributor

Is anything holding back this PR, is there anything left to do?

jonasschnelli force-pushed on Mar 3, 2017

jonasschnelli commented at 2:57 pm on March 3, 2017: contributor

Needed rebase (caused by #8775).

jonasschnelli force-pushed on Mar 6, 2017

laanwj commented at 3:34 pm on March 9, 2017: member

Is anything holding back this PR, is there anything left to do?

Eh yes we should merge this soon. Sorry, (again) needs rebase though, probably trivial for the std:: changes in #9643

jonasschnelli force-pushed on Mar 10, 2017

jonasschnelli commented at 8:07 am on March 10, 2017: contributor

Rebased.

laanwj commented at 8:52 am on March 10, 2017: member

Locally here (Ubuntu 16.04, x86_64) it fails the wallet-dump.py test:

 0wallet-dump.py:
 12017-03-10 09:49:52.887000 TestFramework (INFO): Initializing test directory /tmp/testb91k5t1l/479
 22017-03-10 09:49:55.124000 TestFramework (ERROR): Assertion failed
 3Traceback (most recent call last):
 4  File "/store/orion/projects/bitcoin/bitcoin/qa/rpc-tests/test_framework/test_framework.py", line 147, in main
 5    self.run_test()
 6  File "/home/orion/projects/bitcoin/bitcoin/qa/rpc-tests/wallet-dump.py", line 91, in run_test
 7    assert_equal(found_addr_rsv, 90*2) # 90 keys plus 100% internal keys
 8  File "/store/orion/projects/bitcoin/bitcoin/qa/rpc-tests/test_framework/util.py", line 476, in assert_equal
 9    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
10AssertionError: not(90 == 180)
112017-03-10 09:49:55.125000 TestFramework (INFO): Stopping nodes
122017-03-10 09:49:57.249000 TestFramework (WARNING): Not cleaning up dir /tmp/testb91k5t1l/479
132017-03-10 09:49:57.250000 TestFramework (ERROR): Test failed. Test logging available at /tmp/testb91k5t1l/479/test_framework.log
14
15Pass: False, Duration: 4 s

Edit: Solved after deleting qa/cache.

NicolasDorier commented at 12:48 pm on March 10, 2017: contributor

utACK 52874f378d3ba3050fddd3a3af0c5347077e2893

I do not really like the O(N) disk hit on the number of key in the pool. As I am dealing with lots of keys, and machines with crappy latency. But I might fix that in a separate PR.

NicolasDorier commented at 6:23 am on March 14, 2017: contributor

Removing my ACK for now, this would make ReserveKey really too slow. I will propose a commit to fix that this week. I think there should be 2 keypools or at least a way to get the external count without iterating over the whole keypool, whichever is the most easy. I will play around.

in src/wallet/wallet.cpp: in 52874f378d outdated

2957+        bool internal = false;
2958+        CWalletDB walletdb(strWalletFile);
2959+        for (int64_t i = missingInternal + missingExternal; i--;)
2960         {
2961             int64_t nEnd = 1;
2962+            if (i < missingInternal)

NicolasDorier commented at 6:28 am on March 14, 2017:

if you use internal = i % 2 == 0; instead, then at least ReserveKeyFromPool would not have to iterates over all the external keys to find one which match the filter when the filter search for internal key.

NicolasDorier commented at 7:26 am on March 17, 2017:

Actually no, my answer would not do. But you can still not clutter internal and external together.

jonasschnelli force-pushed on Mar 16, 2017

jonasschnelli commented at 2:46 pm on March 16, 2017: contributor

Needed rebase.

I think the performance is acceptable (see comparison further down). Improving the performance could be done after this PR. It’s already packed.

A keypool=5000 comparison between this PR and Master (2.9 GHz Intel Core i7, OSX, SSD).

This PR

(Keep in mind that they keypool is now x2, 5000 ext. and 5000 int. key)

 0$ time ./src/bitcoin-cli --regtest --datadir=../dummy_intkp/ getnewaddress
 1n3yNEDDgo1k512LXBF8k9h2cSCfvsqKPac
 2
 3real	0m0.204s
 4user	0m0.003s
 5sys	0m0.003s
 6
 7$ time ./src/bitcoin-cli --regtest --datadir=../dummy_intkp/ getrawchangeaddress
 8mnVRynjpdTtLovncFDWPn2dkqrLnij5Kf9
 9
10real	0m0.271s
11user	0m0.004s
12sys	0m0.003s

Master

 0$ time ./src/bitcoin-cli --regtest --datadir=../dummy_master/ getnewaddress
 1mmzkFkya299T4ebushyoHye48c5xJmd7GC
 2
 3real	0m0.089s
 4user	0m0.004s
 5sys	0m0.004s
 6
 7$ time ./src/bitcoin-cli --regtest --datadir=../dummy_master/ getrawchangeaddress
 8mwZbWTps1EgZoJoF59XR8meWQUA3rNdb2i
 9
10real	0m0.071s
11user	0m0.003s
12sys	0m0.003s

NicolasDorier commented at 7:28 am on March 17, 2017: contributor

@jonasschnelli well this is on SSD. My hosted VM does not have that. Can you at least do something where I commented on the if (i < missingInternal) ? You clutter internal and external together which make a very bad worst case scenario when there is a scan of the pool based on the filter.

jonasschnelli commented at 3:39 pm on March 17, 2017: contributor

@NicolasDorier: this PR is open since a couple of month (dec 2016) and I don’t want to add any form of optimisations (only fix real problems and it seems to me that the performance is still acceptable/while not good). We really should do this after this has been merged. Adding more optimisation will cause another round of reviewing (and we already did a couple of them).

ryanofsky commented at 5:40 pm on March 17, 2017: member

utACK 78f2cb926495114354d835a2ba344558384c89f9

Confirmed there were no significant changes in the first 13 commits which were rebased since my previous ACK. The rebased commits just added std:: namespace prefixes and updated various python rpc error checks.

All of the changes in the 4 new commits also looked good. The only one of these that was a change in behavior rather than simple cleanup was the change to fundrawtransaction.py, which seemed fine (though it would be good if the commit message mentioned the reason for the change).

NicolasDorier commented at 2:16 pm on March 18, 2017: contributor

utACK 78f2cb9 @jonasschnelli understood, knowing that this PR blocks mine I would also like to see it merged as soon as possible. Will fix the performance issues myself after.

in src/wallet/wallet.h:121 in 78f2cb9264 outdated

112@@ -109,6 +113,18 @@ class CKeyPool
113             READWRITE(nVersion);
114         READWRITE(nTime);
115         READWRITE(vchPubKey);
116+        if (ser_action.ForRead()) {
117+            try {
118+                READWRITE(fInternal);
119+            }
120+            catch (std::ios_base::failure&) {
121+                /* flag as external address if we can't read the internal boolean */

TheBlueMatt commented at 7:47 pm on March 20, 2017:

nit: I believe comment should point out that this is expected of any old wallets.

in src/wallet/wallet.cpp:3077 in 78f2cb9264 outdated

3072+            if (!walletdb.ReadPool(id, keypool))
3073+                throw std::runtime_error(std::string(__func__) + ": read failed");
3074+            if (keypool.fInternal && keypool.nTime < oldest_internal)
3075+                oldest_internal = keypool.nTime;
3076+            else if (!keypool.fInternal && keypool.nTime < oldest_external)
3077+                oldest_external = keypool.nTime;

TheBlueMatt commented at 11:01 pm on March 20, 2017:

Maybe add an additional if (oldest_internal != now && oldest_external != now) break; ?

jonasschnelli commented at 9:40 am on March 24, 2017:

Ah. Right. It’s guaranteed that the setKeyPool iterates always from the oldest to the newest. Will add.

in src/wallet/wallet.cpp:3690 in 78f2cb9264 outdated

3684@@ -3626,9 +3685,12 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile)
3685             CPubKey masterPubKey = walletInstance->GenerateNewHDMasterKey();
3686             if (!walletInstance->SetHDMasterKey(masterPubKey))
3687                 throw std::runtime_error(std::string(__func__) + ": Storing master key failed");
3688+
3689+            // ensure this wallet.dat can only be opened by clients supporting HD with chain split

TheBlueMatt commented at 11:13 pm on March 20, 2017:

Technically this should happen before SetHDMasterKey, I believe (otherwise it maybe a race?)

in src/wallet/walletdb.h:54 in 78f2cb9264 outdated

50     CKeyID masterKeyID; //!< master key hash160
51 
52-    static const int CURRENT_VERSION = 1;
53+    static const int VERSION_HD_BASE        = 1;
54+    static const int VERSION_HD_CHAIN_SPLIT = 2;
55+    static const int CURRENT_VERSION        = VERSION_HD_CHAIN_SPLIT;

TheBlueMatt commented at 11:18 pm on March 20, 2017:

By defaulting to VERSION_HD_CHAIN_SPLIT, encrypting a wallet will auto-upgrade the wallet without asking the user, I believe this is technically a bug.

jonasschnelli commented at 9:53 am on March 24, 2017:

It would upgrade to the new CHDChain serialisation format (including the nInternalChainCounter), but it would not upgrade to use internal keys, right? Is that problematic? If so, I can add a fix that keeps the serialisation version of CHDChain during encryption.

TheBlueMatt commented at 9:22 pm on March 24, 2017:

It would break opening in old wallets either way - because the nInternalChainCounter is added in the middle instead of the end the deserialization would come back with garbage.

in qa/rpc-tests/fundrawtransaction.py:480 in 78f2cb9264 outdated

476@@ -476,6 +477,7 @@ def run_test(self):
477 
478         #refill the keypool
479         self.nodes[1].walletpassphrase("test", 100)
480+        self.nodes[1].keypoolrefill(8) #need to refill the keypool to get an internal change address

TheBlueMatt commented at 11:36 pm on March 20, 2017:

Why is this required? Doesnt walletpassphrase TopUpKeyPool?

in src/wallet/wallet.cpp:141 in 78f2cb9264 outdated

136@@ -137,22 +137,27 @@ void CWallet::DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret)
137     // use hardened derivation (child keys >= 0x80000000 are hardened after bip32)
138     masterKey.Derive(accountKey, BIP32_HARDENED_KEY_LIMIT);
139 
140-    // derive m/0'/0'
141-    accountKey.Derive(externalChainChildKey, BIP32_HARDENED_KEY_LIMIT);
142+    // derive m/0'/0' (external chain) OR m/0'/1' (internal chain)
143+    accountKey.Derive(chainChildKey, BIP32_HARDENED_KEY_LIMIT+(internal ? 1 : 0));

TheBlueMatt commented at 11:58 pm on March 20, 2017:

Maybe assert(CanSupportFeature()) here?

TheBlueMatt commented at 11:58 pm on March 20, 2017: member

Didn’t look too closely at tests

TheBlueMatt commented at 11:58 pm on March 20, 2017: member

Needs rebase.

laanwj added this to the "Blockers" column in a project

[Wallet] split the keypool in an internal and external part 02592f4c5e

Immediately return setKeyPool's size if HD or HD_SPLIT is disabled or not supported d59531ddfc

Removed redundant IsLocked() check in NewKeyPool() 01de822c8d

Fix wrong keypool internal size in RPC getwalletinfo help 05a9b493eb

Make sure ReserveKeyFromKeyPool only hands out internal keys if HD_SPLIT is supported 469a47b760

Make sure we hand out keypool keys if HD_SPLIT is not enabled 9af8f00a75

Fix issue where CDataStream->nVersion was taken a CKeyPool record version d0a627a53a

Make sure we always generate one keypool key at minimum bcafca1077

Switch to 100% for the HD internal keypool size 79df9df348

Don't switch to HD-chain-split during wallet encryption of non HD-chain-split wallets dd526c2a2d

GetOldestKeyPoolTime: if HD & HD Chain Split is enabled, response max(oldest-internal-key, oldest-external-key) add38d9b83

Only show keypoolsize_hd_internal if HD split is enabled e138876f0a

CKeyPool avoid "catch (...)" in SerializationOp 58e148333e

Fix superfluous cast and code style nits in RPC wallet-hd.py test 1090502c3e

Overhaul the internal/external key derive switch d9638e5aa4

Remove FEATURE_HD_SPLIT bump TODO 003e197498

Slightly modify fundrawtransaction.py test (change getnewaddress() into getrawchangeaddress()) 1b3b5c6f8f

jonasschnelli force-pushed on Mar 24, 2017

Make sure we set the wallets min version to FEATURE_HD_SPLIT at the very first point 771a304ffe

Optimize GetOldestKeyPoolTime(), return as soon as we have both oldest keys ed79e4f497

Define CWallet::DeriveNewChildKey() as private cd468d07d5

Add assertion for CanSupportFeature(FEATURE_HD_SPLIT) 1df08d1580

jonasschnelli commented at 10:05 am on March 24, 2017: contributor

Addressed @TheBlueMatt’s nits (except #9294 (review) which I’m not sure if this is a concern or not)

NicolasDorier commented at 6:00 am on March 27, 2017: contributor

re utACK 1df08d1

Do not break backward compatibility during wallet encryption 9382f0425e

jonasschnelli commented at 7:53 am on March 27, 2017: contributor

Added a commit to address the backward compatibility issue during wallet encryption mentioned by @TheBlueMatt (https://github.com/bitcoin/bitcoin/pull/9294#discussion_r107040500). @TheBlueMatt: can you give it a re-test?

in src/wallet/rpcwallet.cpp:2442 in 9382f0425e outdated

2437@@ -2437,18 +2438,23 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
2438     LOCK2(cs_main, pwallet->cs_wallet);
2439 
2440     UniValue obj(UniValue::VOBJ);
2441+
2442+    size_t kpExternalSize = pwalletMain->KeypoolCountExternalKeys();

TheBlueMatt commented at 9:10 pm on March 27, 2017:

Oops, should be pwallet, not pwalletMain now :).

in src/wallet/walletdb.h:65 in 9382f0425e outdated

60@@ -58,13 +61,16 @@ class CHDChain
61     {
62         READWRITE(this->nVersion);
63         READWRITE(nExternalChainCounter);
64+        if (this->nVersion >= VERSION_HD_CHAIN_SPLIT)
65+            READWRITE(nInternalChainCounter);

TheBlueMatt commented at 9:19 pm on March 27, 2017:

In addition to the fix for backward compat where you set the version, can we make this more robust and flip the order? Generally adding new fields for (de-)serialization we should be adding them to the end.

in src/wallet/wallet.cpp:644 in 9382f0425e outdated

638@@ -633,7 +639,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
639         if (IsHDEnabled()) {
640             CKey key;
641             CPubKey masterPubKey = GenerateNewHDMasterKey();
642-            if (!SetHDMasterKey(masterPubKey))
643+            // preserve the old chains version to not break backward compatibility
644+            CHDChain oldChain = GetHDChain();
645+            if (!SetHDMasterKey(masterPubKey, &oldChain))

TheBlueMatt commented at 9:28 pm on March 27, 2017:

Why not SetHDMasterKey(int nHDChainVersion)? Then you can just do CanSupportFeature(FEATURE_HD_SPLIT) ? CHDChain::VERSION_HD_CHAIN_SPLIT : CHDChain::VERSION_HD_BASE.

jonasschnelli commented at 7:13 am on March 28, 2017:

I though hiding the internals at this point would be preferable. IMO CWallet::encryptWallet should not have direct knowhow of the CHDChain version handling. I’d prefer passing in the old chain and let it be handled through SetHDMasterKey. Also, in future maybe other attributes need to be preserved. Not sure although.

TheBlueMatt commented at 5:58 pm on March 28, 2017:

It kinda feels like mixing models, then. Wallet checks for FEATURE_HD_SPLIT everywhere before it updates the nInternalCounts, ie it seems to think its responsible for doing the checks, only to have CHDChain hide some stuff like version, but then you have to copy the version? Either we move logic into CHDChain (as @luke-jr suggested previously, have CHDChain know what version means and assert if you try to use the internal counter with a version that doesnt support it, which I assume some folks would object to), or all the logic should be in CWallet.

in src/wallet/wallet.cpp:3080 in 9382f0425e outdated

3076+        int64_t oldest_external = now, oldest_internal = now;
3077+
3078+        for(const int64_t& id : setKeyPool)
3079+        {
3080+            if (!walletdb.ReadPool(id, keypool)) {
3081+                throw std::runtime_error(std::string(__func__) + ": read failed");

TheBlueMatt commented at 9:43 pm on March 27, 2017:

nit: the old version does an assert(keypool.vchPubKey.IsValid()); which is nice to have…not necessary to add, but would be cool.

jonasschnelli commented at 7:16 am on March 28, 2017:

For the keypool oldest it’s further down L3026 (at https://github.com/bitcoin/bitcoin/pull/9294/files#diff-b2bb174788c7409b671c46ccc86034bdL3026)

Fix rebase issue where pwalletMain was used instead of pwallet

Ser./Deser. nInternalChainCounter as last element

4115af7ac7

jonasschnelli commented at 7:20 am on March 28, 2017: contributor

Fixed two points reported by @TheBlueMatt:

Move Ser./Deser. of nInternalChainCounter to be the last element
Fix a rebase issue where pwalletMain was used instead of pWallet.

laanwj commented at 10:46 am on March 29, 2017: member

utACK 4115af7 I think this has enough ACKs. Let’s merge this and fix the rest, which seems to be less critical, later.

laanwj merged this on Mar 29, 2017

laanwj closed this on Mar 29, 2017

laanwj referenced this in commit f34cdcbd80 on Mar 29, 2017

laanwj removed this from the "Blockers" column in a project

MarcoFalke locked this on Sep 8, 2021

Use internal HD chain for change outputs (hd split) #9294

Main changes

Compatibility:

This PR

Master