/bitcoin

Track keypool entries as internal vs external in memory #10235

Merged
merged 3 commits into from Jul 15, 2017

Conversation

Reviewers

@laanwj laanwj

@jonasschnelli jonasschnelli

@NicolasDorier NicolasDorier

@ryanofsky ryanofsky

@morcos morcos

Assignees
No one assigned
Labels
Refactoring Wallet
Projects
None yet
Milestone
No milestone
7 participants
@TheBlueMatt @jonasschnelli @NicolasDorier @ryanofsky @morcos @sipa @laanwj
@TheBlueMatt
Contributor

TheBlueMatt commented Apr 19, 2017

This is an alternative version of #10184. As @jonasschnelli points out there, the performance regressions are pretty minimal, but given that this is a pretty simple, mechanical change, its probably worth doing.

TheBlueMatt referenced this pull request Apr 19, 2017

Closed

[Wallet] Worst case performance improvement on KeyPool filtering #10184

@jonasschnelli
Member

jonasschnelli commented Apr 19, 2017

Concept ACK.
Ideally we should have been implemented this in the first place, but the code diff was already huge without this.

@jonasschnelli jonasschnelli added Refactoring Wallet labels Apr 19, 2017

src/wallet/wallet.cpp
@@ -2975,25 +2985,8 @@ bool CWallet::NewKeyPool()
size_t CWallet::KeypoolCountExternalKeys()
@NicolasDorier

NicolasDorier Apr 20, 2017

Member

I think you can also remove this one completely

@TheBlueMatt

TheBlueMatt Apr 20, 2017

Contributor

rpcwallet uses it? Might as well leave it.

@NicolasDorier
Member

NicolasDorier commented Apr 20, 2017 • edited

I really like this PR. This is simpler than before, utACK. You should remove KeypoolCountExternalKeys() though.
@NicolasDorier
Member

NicolasDorier commented Apr 20, 2017

@jonasschnelli the line count is almost the same. But it was better to not break the reviews, this was already a big PR.

jonasschnelli referenced this pull request May 4, 2017

Closed

Add HD wallet auto-restore functionality #10240

@ryanofsky

ryanofsky reviewed May 5, 2017
View changes

(These review comments are actually from Tuesday. Apparently I forgot to press the submit button.)

src/wallet/wallet.cpp
@@ -3051,30 +3048,29 @@ void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool int
if (!IsLocked())
TopUpKeyPool();
+ bool fInternal = IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT) && internal;
@ryanofsky

ryanofsky May 5, 2017

Contributor

Having both internal and fInternal variables in the same scope with slightly different meanings seems error prone. Consider renaming one or both. I might go with requestedInternal and actuallyInternal.

src/wallet/wallet.cpp
- return;
- }
+ nIndex = *setKeyPool.begin();
+ setKeyPool.erase(nIndex);
@ryanofsky

ryanofsky May 5, 2017

Contributor

Could use iterator version of erase to avoid another lookup

src/wallet/wallet.cpp
@@ -3118,46 +3118,34 @@ bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
return true;
}
+static int64_t GetOldestKeyInPool(const std::set<int64_t>& setKeyPool, CWalletDB& walletdb) {
+ CKeyPool keypool;
+ int64_t nIndex = *(setKeyPool.begin());
@ryanofsky

ryanofsky May 5, 2017

Contributor

Maybe assert !setKeyPool.empty()

src/wallet/wallet.cpp
+ oldestKey = std::max(GetOldestKeyInPool(setInternalKeyPool, walletdb), oldestKey);
+ }
+ if (!setExternalKeyPool.empty()) {
+ oldestKey = std::max(GetOldestKeyInPool(setExternalKeyPool, walletdb), oldestKey);
@ryanofsky

ryanofsky May 5, 2017

Contributor

This new code doesn't seem equivalent to the old code. Previously, if HD_SPLIT were true but there were 0 internal keys, it would return current timestamp now. Now it will return oldest external key time. Also vice versa (swapping internal/external).

@TheBlueMatt
Contributor

TheBlueMatt commented May 5, 2017

Addressed @ryanofsky's comments and rebased.
@ryanofsky

ryanofsky reviewed May 10, 2017
View changes

utACK f4390a7. Changes since previous review: rebase, fInternal variable renames, iterator erase call, GetOldestKeyPoolTime bugfix.

src/wallet/wallet.cpp
-
- // if the keypool is empty, return <NOW>
- if (setKeyPool.empty())
+static int64_t GetOldestKeyInPool(const std::set<int64_t>& setKeyPool, CWalletDB& walletdb) {
@ryanofsky

ryanofsky May 10, 2017

Contributor

In commit "Track keypool entries as internal vs external in memory"

The previous name GetOldestKeyPoolTime might be better name for this since it is returning a time.

@jonasschnelli

jonasschnelli Jun 8, 2017

Member

Maybe GetOldestKeyBirthdayInPool or GetOldestKeyTimeInPool

src/wallet/wallet.cpp
+ LOCK(cs_wallet);
+
+ CWalletDB walletdb(*dbw);
+ int64_t oldestKey = -1;
@ryanofsky

ryanofsky May 10, 2017

Contributor

In commit "Track keypool entries as internal vs external in memory"

You could initialize this to GetOldest(external) to save a line of code and get rid of the -1 magic value.

@ryanofsky
Contributor

ryanofsky commented Jun 8, 2017

@jonasschnelli, do you want to review this? It seems like it simplifies some things and could interact nicely with #10240 and #10238.
src/wallet/wallet.cpp
@@ -2993,8 +2986,8 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
// count amount of available keys (internal, external)
// make sure the keypool of external and internal keys fits the user selected target (-keypool)
- int64_t amountExternal = KeypoolCountExternalKeys();
- int64_t amountInternal = setKeyPool.size() - amountExternal;
+ int64_t amountExternal = setExternalKeyPool.size();
@jonasschnelli

jonasschnelli Jun 8, 2017

Member

nit: amountExternal is not longer required, setExternalKeyPool.size() can be used directly at L2991+.

src/wallet/wallet.cpp
- }
+ auto it = setKeyPool.begin();
+ nIndex = *it;
+ setKeyPool.erase(it);
@jonasschnelli

jonasschnelli Jun 8, 2017

Member

Ideally setKeyPool.erase() happens after the DB read and HaveKey/fInternal check? No?

@TheBlueMatt

TheBlueMatt Jun 21, 2017

Contributor

If there are any issues with that key, best to get it out of the pool so that future calls succeed (though, really, those should be fatal StartShutdown() issues, not just runtime_errors that the RPC interface will return).

@jonasschnelli
Member

jonasschnelli commented Jun 8, 2017

Reviewed.
utACK f4390a7 modulo nits (although merge seems okay at this stage).
@TheBlueMatt
Contributor

TheBlueMatt commented Jun 21, 2017

Rebased and addressed all the comments, I believe.
@NicolasDorier
Member

NicolasDorier commented Jun 22, 2017

utACK 435c272
@ryanofsky

ryanofsky reviewed Jun 22, 2017
View changes

utACK 435c272. Rebased and has the 3 suggested changes.

@TheBlueMatt
Contributor

TheBlueMatt commented Jul 10, 2017

Would be nice to get this for 15 to avoid some minor performance regressions.
@ryanofsky
Contributor

ryanofsky commented Jul 10, 2017

Would be nice to get this for 15 to avoid some minor performance regressions.

This seems like it could be merged now. I don't see anything that would hold it up.
src/wallet/wallet.cpp
+ nEnd = *(--setInternalKeyPool.end()) + 1;
+ }
+ if (!setExternalKeyPool.empty()) {
+ nEnd = std::max(nEnd, *(--setExternalKeyPool.end()) + 1);
@laanwj

laanwj Jul 11, 2017

Owner

This looks really strange, why the infix subtract to an iterator here?

TheBlueMatt referenced this pull request Jul 11, 2017

Merged

No longer ever reuse keypool indexes #10795

@ryanofsky

ryanofsky reviewed Jul 11, 2017
View changes

utACK 1ee8c0f, but IMO the new commit makes the code worse. Before you could clearly see what TopUpKeyPool was doing if either of the sets were empty. Now you have to look inside the definition of a dodgy GetHighestSetElement function in another part of the code to find out what it returns when a set is empty and what implications that has for the max value. Would be better to drop this commit and just replace *--end() with *rbegin() in the original code, I think.

Only change since last review is the new commit.

src/wallet/wallet.cpp
@@ -3106,6 +3106,12 @@ size_t CWallet::KeypoolCountExternalKeys()
return setExternalKeyPool.size();
}
+// Get highest element in a uint64_t set (eg keypools), or 0 if its empty
@ryanofsky

ryanofsky Jul 11, 2017 • edited

Contributor

s/uint64_t/int64_t/

@TheBlueMatt
Contributor

TheBlueMatt commented Jul 11, 2017

@ryanofsky OK, I restored it closer to where it was. That code goes away in #10795 anyway.
@ryanofsky

ryanofsky reviewed Jul 11, 2017
View changes

utACK bdf6ba8. Last commit now just switches --end to rbegin.

@morcos
Contributor

morcos commented Jul 12, 2017

utACK bdf6ba8
assuming we also merge #10795
@morcos

morcos approved these changes Jul 12, 2017
View changes

src/wallet/wallet.cpp
+ } else {
+ setExternalKeyPool.insert(nEnd);
+ }
+ LogPrintf("keypool added key %d, size=%u, internal=%d\n", nEnd, setInternalKeyPool.size() + setExternalKeyPool.size(), internal);
@morcos

morcos Jul 12, 2017

Contributor

nit: print external and internal sizes or only size for keypool you're adding to

@TheBlueMatt

TheBlueMatt Jul 14, 2017

Contributor

Done

@sipa
Owner

sipa commented Jul 13, 2017

utACK bdf6ba8. I started reviewing the first commit, made two review comments, and your two further commits exactly fixed those issues :)
@sipa
Owner

sipa commented Jul 15, 2017

utACK d40a72c, only change is the keypool size logging.
@morcos
Contributor

morcos commented Jul 15, 2017

utACK d40a72c

@sipa sipa merged commit d40a72c into bitcoin:master Jul 15, 2017
1 check passed

1 check passed

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

@sipa sipa added a commit that referenced this pull request Jul 15, 2017

@sipa sipa Merge #10235: Track keypool entries as internal vs external in memory 
d40a72c Clarify *(--.end()) iterator semantics in CWallet::TopUpKeyPool (Matt Corallo)
28301b9 Meet code style on lines changed in the previous commit (Matt Corallo)
4a3fc35 Track keypool entries as internal vs external in memory (Matt Corallo)

Pull request description:

  This is an alternative version of #10184. As @jonasschnelli points out there, the performance regressions are pretty minimal, but given that this is a pretty simple, mechanical change, its probably worth doing.

Tree-SHA512: e83f9ebf2998f8164d1b2eebe5e6dcdeadea8c30b7612861f830758c08bf4093cd6a67b3bcfa9cfcb139e5e0b106fc8898a975fc69f334981aefc756568ab613
This commit was signed with a verified signature.
@sipa sipa Pieter Wuille
GPG key ID: A636E97631F767E0 Learn about signing commits
5cfdda2

@laanwj laanwj added a commit that referenced this pull request Jul 18, 2017

@laanwj laanwj Merge #10795: No longer ever reuse keypool indexes 
1fc8c3d No longer ever reuse keypool indexes (Matt Corallo)

Pull request description:

  This fixes an issue where you could reserve a keypool entry, then
  top up the keypool, writing out a new key at the given index, then
  return they key from the pool. This isnt likely to cause issues,
  but given there is no reason to ever re-use keypool indexes
  (they're 64 bits...), best to avoid it alltogether.

  Builds on #10235, should probably get a 15 tag.

Tree-SHA512: c13a18a90f1076fb74307f2d64e9d80149811524c6bda259698ff2c65adaf8c6c3f2a3a07a5f4bf03251bc942ba8f5fd33a4427aa4256748c40b062991682caf
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
7b6e8bc
@jonasschnelli
Member

jonasschnelli commented Jul 18, 2017

Post merge ACK
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment