Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 23 commits into from Mar 29, 2017

Conversation

@jonasschnelli
Copy link
Member

jonasschnelli commented Dec 6, 2016

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 jonasschnelli added the Wallet label Dec 6, 2016

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Dec 6, 2016

Interesting assertion error in CWallet::CreateRawTransaction

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

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Dec 6, 2016

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

qa/rpc-tests/keypool.py Outdated
# the next one should fail
try:
addr = nodes[0].getrawchangeaddress()
raise AssertionError('Keypool should be exhausted after three addresses')
raise AssertionError('Keypool should be exhausted after three internal addresses (20%)')

This comment has been minimized.

@paveljanik

paveljanik Dec 8, 2016

Contributor

Still three?

qa/rpc-tests/keypool.py Outdated
# the next one should fail
try:
addr = nodes[0].getnewaddress()
raise AssertionError('Keypool should be exhausted after three external addresses (ceil rounded 80% of six)')

This comment has been minimized.

@paveljanik

paveljanik Dec 8, 2016

Contributor

three?

This comment has been minimized.

@instagibbs

instagibbs Jan 9, 2017

Member

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

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Dec 10, 2016

Once #9295 is merged, travis should succeed.

Shall we find out?

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_split branch Dec 11, 2016

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Dec 11, 2016

Rebased.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_split branch Dec 12, 2016

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Dec 12, 2016

Travis succeeds now.

@luke-jr
Copy link
Member

luke-jr left a comment

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

Does this implement a BIP that should be mentioned?

src/wallet/wallet.cpp Outdated
// 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(externalChainChildKey, BIP32_HARDENED_KEY_LIMIT+(int)internal);

This comment has been minimized.

@luke-jr

luke-jr Dec 22, 2016

Member

Prefer to explicitly add (internal ? 1 : 0)

src/wallet/wallet.cpp Outdated
externalChainChildKey.Derive(childKey, hdChain.nExternalChainCounter | BIP32_HARDENED_KEY_LIMIT);
metadata.hdKeypath = "m/0'/0'/" + std::to_string(hdChain.nExternalChainCounter) + "'";
externalChainChildKey.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)) + "'";

This comment has been minimized.

@luke-jr

luke-jr Dec 22, 2016

Member

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

src/wallet/wallet.cpp Outdated
// Top up key pool
unsigned int nTargetSize;
if (kpSize > 0)
nTargetSize = kpSize;
else
nTargetSize = max(GetArg("-keypool", DEFAULT_KEYPOOL_SIZE), (int64_t) 0);

while (setKeyPool.size() < (nTargetSize + 1))
// count amount of available keys (internal, external)
// try to generate 80% external keys

This comment has been minimized.

@luke-jr

luke-jr Dec 22, 2016

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Dec 22, 2016

Author Member

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.

@@ -87,7 +87,7 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const
return &(it->second);
}

CPubKey CWallet::GenerateNewKey()
CPubKey CWallet::GenerateNewKey(bool internal)

This comment has been minimized.

@luke-jr

luke-jr Dec 22, 2016

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 10, 2017

Author Member

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.

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

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.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_split branch 2 times, most recently Jan 6, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Jan 6, 2017

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 jonasschnelli force-pushed the jonasschnelli:2016/12/hd_split branch Jan 9, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Jan 9, 2017

Updated with a small nit-fix (#9294 (comment)) and a fix for wallet-dump.py RPC test. Travis should succeed now.

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_split branch Jan 9, 2017

@instagibbs
Copy link
Member

instagibbs left a comment

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.

src/wallet/wallet.cpp Outdated

// 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) + "'";
externalChainChildKey.Derive(childKey, (internal ? hdChain.nInternalChainCounter : hdChain.nExternalChainCounter) | BIP32_HARDENED_KEY_LIMIT);

This comment has been minimized.

@instagibbs

instagibbs Jan 9, 2017

Member

It's called externalChainChildKey but it could be either.

src/wallet/wallet.cpp Outdated
@@ -1296,7 +1299,7 @@ bool CWallet::SetHDMasterKey(const CPubKey& pubkey)
LOCK(cs_wallet);

// ensure this wallet.dat can only be opened by clients supporting HD

This comment has been minimized.

@instagibbs

instagibbs Jan 9, 2017

Member

update comment

src/wallet/wallet.cpp Outdated
throw runtime_error(std::string(__func__) + ": read failed");
amountI += tmpKeypool.fInternal;
amountE += !tmpKeypool.fInternal;
// don't flag keypool keys internal if we don't use HD

This comment has been minimized.

@instagibbs

instagibbs Jan 9, 2017

Member

/HD/HD split/?

src/wallet/wallet.cpp Outdated
// 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);

This comment has been minimized.

@instagibbs

instagibbs Jan 9, 2017

Member

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).

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 10, 2017

Author Member

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.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 14, 2017

Contributor

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?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 16, 2017

Author Member

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?

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 16, 2017

Contributor

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%.

src/wallet/wallet.cpp Outdated
@@ -3129,7 +3151,11 @@ bool CReserveKey::GetReservedKey(CPubKey& pubkey, bool internal)
if (nIndex != -1)
vchPubKey = keypool.vchPubKey;
else {
return false;
if (pwallet->IsLocked())

This comment has been minimized.

@instagibbs

instagibbs Jan 9, 2017

Member

Seems like a separate optimization?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 10, 2017

Author Member

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.

src/wallet/rpcwallet.cpp Outdated
obj.push_back(Pair("keypoolsize", (int64_t)kpExternalSize));
CKeyID masterKeyID = pwalletMain->GetHDChain().masterKeyID;
if (!masterKeyID.IsNull())
obj.push_back(Pair("keypoolsize_hd_internal", (int64_t)(pwalletMain->GetKeyPoolSize() - kpExternalSize)));

This comment has been minimized.

@instagibbs

instagibbs Jan 9, 2017

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 10, 2017

Author Member

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).

This comment has been minimized.

@instagibbs

instagibbs Jan 10, 2017

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 10, 2017

Author Member

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.

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 13, 2017

Author Member

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

qa/rpc-tests/keypool.py Outdated
# the next one should fail
try:
addr = nodes[0].getnewaddress()
raise AssertionError('Keypool should be exhausted after three external addresses (ceil rounded 80% of six)')

This comment has been minimized.

@instagibbs

instagibbs Jan 9, 2017

Member

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

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_split branch Jan 10, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Jan 10, 2017

Fixed @instagibbs points. Squashed into a single commit.

@instagibbs

This comment has been minimized.

Copy link
Member

instagibbs commented Jan 10, 2017

utACK 4065d5fd44b9ce68a688c82353822a1cf49efe53

@jtimon

This comment has been minimized.

Copy link
Member

jtimon commented Jan 10, 2017

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 jonasschnelli force-pushed the jonasschnelli:2016/12/hd_split branch Jan 12, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Jan 12, 2017

Rebased.

src/wallet/walletdb.h Outdated
@@ -58,13 +61,16 @@ class CHDChain
{
READWRITE(this->nVersion);
READWRITE(nExternalChainCounter);
if (this->nVersion >= VERSION_HD_CHAIN_SPLIT)

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 16, 2017

Author Member

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.

This comment has been minimized.

@luke-jr

luke-jr Jan 18, 2017

Member

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.

@@ -87,7 +87,7 @@ const CWalletTx* CWallet::GetWalletTx(const uint256& hash) const
return &(it->second);
}

CPubKey CWallet::GenerateNewKey()
CPubKey CWallet::GenerateNewKey(bool internal)

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

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.

@@ -100,7 +100,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));

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 13, 2017

Author Member

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

This comment has been minimized.

@luke-jr

luke-jr Jan 13, 2017

Member

Maybe move it to private: then?

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 20, 2017

Contributor

Yea, really should be private, I think.

src/wallet/wallet.cpp Outdated

// 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);

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

Seems like this could be significantly simplified with:

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

This comment has been minimized.

@theuni

theuni Jan 13, 2017

Member

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

src/wallet/wallet.cpp Outdated
// 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);

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 13, 2017

Author Member

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 14, 2017

Contributor

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 14, 2017

Contributor

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 16, 2017

Author Member

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?).

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 16, 2017

Contributor

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.

src/wallet/wallet.h Outdated
CPubKey GenerateNewKey();
void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret);
CPubKey GenerateNewKey(bool internal = false);
void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal = false);

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 13, 2017

Author Member

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

This comment has been minimized.

@luke-jr

luke-jr Jan 13, 2017

Member

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

CKeyID masterKeyID; //!< master key hash160

static const int CURRENT_VERSION = 1;
static const int VERSION_HD_BASE = 1;

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

Missing logic to initialise this version.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 13, 2017

Author Member

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

src/wallet/rpcwallet.cpp Outdated
obj.push_back(Pair("walletversion", pwalletMain->GetVersion()));
obj.push_back(Pair("balance", ValueFromAmount(pwalletMain->GetBalance())));
obj.push_back(Pair("unconfirmed_balance", ValueFromAmount(pwalletMain->GetUnconfirmedBalance())));
obj.push_back(Pair("immature_balance", ValueFromAmount(pwalletMain->GetImmatureBalance())));
obj.push_back(Pair("txcount", (int)pwalletMain->mapWallet.size()));
obj.push_back(Pair("keypoololdest", pwalletMain->GetOldestKeyPoolTime()));
obj.push_back(Pair("keypoolsize", (int)pwalletMain->GetKeyPoolSize()));
obj.push_back(Pair("keypoolsize", (int64_t)kpExternalSize));

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

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.

src/wallet/rpcwallet.cpp Outdated
obj.push_back(Pair("keypoolsize", (int64_t)kpExternalSize));
CKeyID masterKeyID = pwalletMain->GetHDChain().masterKeyID;
if (!masterKeyID.IsNull())
obj.push_back(Pair("keypoolsize_hd_internal", (int64_t)(pwalletMain->GetKeyPoolSize() - kpExternalSize)));

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

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

src/wallet/rpcwallet.cpp Outdated
obj.push_back(Pair("keypoolsize", (int)pwalletMain->GetKeyPoolSize()));
obj.push_back(Pair("keypoolsize", (int64_t)kpExternalSize));
CKeyID masterKeyID = pwalletMain->GetHDChain().masterKeyID;
if (!masterKeyID.IsNull())

This comment has been minimized.

@luke-jr

luke-jr Jan 12, 2017

Member

Shouldn't this use CanSupportFeature(FEATURE_HD_SPLIT)?

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 13, 2017

Author Member

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

This comment has been minimized.

@luke-jr

luke-jr Jan 13, 2017

Member

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...

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Jan 13, 2017

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.

src/wallet/rpcwallet.cpp Outdated
" \"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"

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 14, 2017

Contributor

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

src/wallet/rpcwallet.cpp Outdated
@@ -2315,17 +2316,20 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
LOCK2(cs_main, pwalletMain->cs_wallet);

UniValue obj(UniValue::VOBJ);
size_t kpExternalSize = (int)pwalletMain->KeypoolCountExternalKeys();
obj.push_back(Pair("walletversion", pwalletMain->GetVersion()));
obj.push_back(Pair("balance", ValueFromAmount(pwalletMain->GetBalance())));
obj.push_back(Pair("unconfirmed_balance", ValueFromAmount(pwalletMain->GetUnconfirmedBalance())));
obj.push_back(Pair("immature_balance", ValueFromAmount(pwalletMain->GetImmatureBalance())));
obj.push_back(Pair("txcount", (int)pwalletMain->mapWallet.size()));
obj.push_back(Pair("keypoololdest", pwalletMain->GetOldestKeyPoolTime()));

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 14, 2017

Contributor

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 16, 2017

Author Member

Agree. But probably in a separate PR.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jan 16, 2017

Contributor

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

This comment has been minimized.

@ryanofsky

ryanofsky Jan 24, 2017

Contributor

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Jan 26, 2017

Author Member

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.

jonasschnelli added some commits Jan 16, 2017

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

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2016/12/hd_split branch from 78f2cb9 to 1b3b5c6 Mar 24, 2017

jonasschnelli added some commits Mar 24, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Mar 24, 2017

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

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Mar 27, 2017

re utACK 1df08d1

@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Mar 27, 2017

Added a commit to address the backward compatibility issue during wallet encryption mentioned by @TheBlueMatt (#9294 (comment)).
@TheBlueMatt: can you give it a re-test?

@@ -2437,18 +2438,23 @@ UniValue getwalletinfo(const JSONRPCRequest& request)
LOCK2(cs_main, pwallet->cs_wallet);

UniValue obj(UniValue::VOBJ);

size_t kpExternalSize = pwalletMain->KeypoolCountExternalKeys();

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 27, 2017

Contributor

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

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 27, 2017

Contributor

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.

if (!SetHDMasterKey(masterPubKey))
// preserve the old chains version to not break backward compatibility
CHDChain oldChain = GetHDChain();
if (!SetHDMasterKey(masterPubKey, &oldChain))

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 27, 2017

Contributor

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

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 28, 2017

Author Member

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.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 28, 2017

Contributor

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.

for(const int64_t& id : setKeyPool)
{
if (!walletdb.ReadPool(id, keypool)) {
throw std::runtime_error(std::string(__func__) + ": read failed");

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Mar 27, 2017

Contributor

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

This comment has been minimized.

@jonasschnelli
Fix rebase issue where pwalletMain was used instead of pwallet
Ser./Deser. nInternalChainCounter as last element
@jonasschnelli

This comment has been minimized.

Copy link
Member Author

jonasschnelli commented Mar 28, 2017

Fixed two points reported by @TheBlueMatt:

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

This comment has been minimized.

Copy link
Member

laanwj commented Mar 29, 2017

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

@laanwj laanwj merged commit 4115af7 into bitcoin:master Mar 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 29, 2017

Merge #9294: Use internal HD chain for change outputs (hd split)
4115af7 Fix rebase issue where pwalletMain was used instead of pwallet Ser./Deser. nInternalChainCounter as last element (Jonas Schnelli)
9382f04 Do not break backward compatibility during wallet encryption (Jonas Schnelli)
1df08d1 Add assertion for CanSupportFeature(FEATURE_HD_SPLIT) (Jonas Schnelli)
cd468d0 Define CWallet::DeriveNewChildKey() as private (Jonas Schnelli)
ed79e4f Optimize GetOldestKeyPoolTime(), return as soon as we have both oldest keys (Jonas Schnelli)
771a304 Make sure we set the wallets min version to FEATURE_HD_SPLIT at the very first point (Jonas Schnelli)
1b3b5c6 Slightly modify fundrawtransaction.py test (change getnewaddress() into getrawchangeaddress()) (Jonas Schnelli)
003e197 Remove FEATURE_HD_SPLIT bump TODO (Jonas Schnelli)
d9638e5 Overhaul the internal/external key derive switch (Jonas Schnelli)
1090502 Fix superfluous cast and code style nits in RPC wallet-hd.py test (Jonas Schnelli)
58e1483 CKeyPool avoid "catch (...)" in SerializationOp (Jonas Schnelli)
e138876 Only show keypoolsize_hd_internal if HD split is enabled (Jonas Schnelli)
add38d9 GetOldestKeyPoolTime: if HD & HD Chain Split is enabled, response max(oldest-internal-key, oldest-external-key) (Jonas Schnelli)
dd526c2 Don't switch to HD-chain-split during wallet encryption of non HD-chain-split wallets (Jonas Schnelli)
79df9df Switch to 100% for the HD internal keypool size (Jonas Schnelli)
bcafca1 Make sure we always generate one keypool key at minimum (Jonas Schnelli)
d0a627a Fix issue where CDataStream->nVersion was taken a CKeyPool record version (Jonas Schnelli)
9af8f00 Make sure we hand out keypool keys if HD_SPLIT is not enabled (Jonas Schnelli)
469a47b Make sure ReserveKeyFromKeyPool only hands out internal keys if HD_SPLIT is supported (Jonas Schnelli)
05a9b49 Fix wrong keypool internal size in RPC getwalletinfo help (Jonas Schnelli)
01de822 Removed redundant IsLocked() check in NewKeyPool() (Jonas Schnelli)
d59531d Immediately return setKeyPool's size if HD or HD_SPLIT is disabled or not supported (Jonas Schnelli)
02592f4 [Wallet] split the keypool in an internal and external part (Jonas Schnelli)

Tree-SHA512: 80d355d5e844b48c3163b56c788ab8b5b5285db0ceeb19858a3ef517d5a702afeca21dbae526d7b8fb4101c2a745af1d92bf557c40cf516780f17992bf678c1a

@laanwj laanwj removed this from Blockers in High-priority for review Mar 30, 2017

@ryanofsky ryanofsky referenced this pull request Jul 14, 2017

Closed

[WIP] [wallet] keypool restore #10830

0 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.