Track keypool entries as internal vs external in memory

TheBlueMatt commented at 5:12 pm on April 19, 2017: member

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.

jonasschnelli commented at 5:23 pm on April 19, 2017: contributor

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

jonasschnelli added the label Refactoring on Apr 19, 2017

jonasschnelli added the label Wallet on Apr 19, 2017

TheBlueMatt force-pushed on Apr 20, 2017

in src/wallet/wallet.cpp:3103 in 4a526d9d6b outdated

2984@@ -2975,25 +2985,8 @@ bool CWallet::NewKeyPool()
2985 
2986 size_t CWallet::KeypoolCountExternalKeys()

NicolasDorier commented at 3:41 am on April 20, 2017:

I think you can also remove this one completely

TheBlueMatt commented at 1:30 pm on April 20, 2017:

rpcwallet uses it? Might as well leave it.

NicolasDorier commented at 3:56 am on April 20, 2017: contributor

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

NicolasDorier commented at 3:59 am on April 20, 2017: contributor

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

in src/wallet/wallet.cpp:3051 in 8c7901c33e outdated

3047@@ -3051,30 +3048,29 @@ void CWallet::ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool int
3048         if (!IsLocked())
3049             TopUpKeyPool();
3050 
3051+        bool fInternal = IsHDEnabled() && CanSupportFeature(FEATURE_HD_SPLIT) && internal;

ryanofsky commented at 7:34 pm on May 2, 2017:

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.

in src/wallet/wallet.cpp:3061 in 8c7901c33e outdated

3073-                assert(keypool.vchPubKey.IsValid());
3074-                LogPrintf("keypool reserve %d\n", nIndex);
3075-                return;
3076-            }
3077+        nIndex = *setKeyPool.begin();
3078+        setKeyPool.erase(nIndex);

ryanofsky commented at 7:53 pm on May 2, 2017:

Could use iterator version of erase to avoid another lookup

in src/wallet/wallet.cpp:3248 in 8c7901c33e outdated

3117@@ -3118,46 +3118,34 @@ bool CWallet::GetKeyFromPool(CPubKey& result, bool internal)
3118     return true;
3119 }
3120 
3121+static int64_t GetOldestKeyInPool(const std::set<int64_t>& setKeyPool, CWalletDB& walletdb) {
3122+    CKeyPool keypool;
3123+    int64_t nIndex = *(setKeyPool.begin());

ryanofsky commented at 7:57 pm on May 2, 2017:

Maybe assert !setKeyPool.empty()

in src/wallet/wallet.cpp:3146 in 8c7901c33e outdated

3171-    return keypool.nTime;
3172+    if (!setInternalKeyPool.empty()) {
3173+        oldestKey = std::max(GetOldestKeyInPool(setInternalKeyPool, walletdb), oldestKey);
3174+    }
3175+    if (!setExternalKeyPool.empty()) {
3176+        oldestKey = std::max(GetOldestKeyInPool(setExternalKeyPool, walletdb), oldestKey);

ryanofsky commented at 8:06 pm on May 2, 2017:

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

ryanofsky commented at 8:45 pm on May 5, 2017: member

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

TheBlueMatt force-pushed on May 5, 2017

TheBlueMatt commented at 9:49 pm on May 5, 2017: member

Addressed @ryanofsky’s comments and rebased.

in src/wallet/wallet.cpp:3099 in aec3732b8e outdated

3100-{
3101-    LOCK(cs_wallet);
3102-
3103-    // if the keypool is empty, return <NOW>
3104-    if (setKeyPool.empty())
3105+static int64_t GetOldestKeyInPool(const std::set<int64_t>& setKeyPool, CWalletDB& walletdb) {

ryanofsky commented at 4:54 pm on May 10, 2017:

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 commented at 6:22 pm on June 8, 2017:

Maybe GetOldestKeyBirthdayInPool or GetOldestKeyTimeInPool

in src/wallet/wallet.cpp:3117 in aec3732b8e outdated

3144+int64_t CWallet::GetOldestKeyPoolTime()
3145+{
3146+    LOCK(cs_wallet);
3147+
3148+    CWalletDB walletdb(*dbw);
3149+    int64_t oldestKey = -1;

ryanofsky commented at 5:02 pm on May 10, 2017:

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 commented at 5:08 pm on May 10, 2017: member

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

ryanofsky commented at 6:14 pm on June 8, 2017: member

@jonasschnelli, do you want to review this? It seems like it simplifies some things and could interact nicely with #10240 and #10238.

in src/wallet/wallet.cpp:2989 in f4390a7c11 outdated

2985@@ -2993,8 +2986,8 @@ bool CWallet::TopUpKeyPool(unsigned int kpSize)
2986 
2987         // count amount of available keys (internal, external)
2988         // make sure the keypool of external and internal keys fits the user selected target (-keypool)
2989-        int64_t amountExternal = KeypoolCountExternalKeys();
2990-        int64_t amountInternal = setKeyPool.size() - amountExternal;
2991+        int64_t amountExternal = setExternalKeyPool.size();

jonasschnelli commented at 6:16 pm on June 8, 2017:

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

in src/wallet/wallet.cpp:3185 in f4390a7c11 outdated

3062-                LogPrintf("keypool reserve %d\n", nIndex);
3063-                return;
3064-            }
3065+        auto it = setKeyPool.begin();
3066+        nIndex = *it;
3067+        setKeyPool.erase(it);

jonasschnelli commented at 6:21 pm on June 8, 2017:

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

TheBlueMatt commented at 3:39 pm on June 21, 2017:

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 commented at 6:27 pm on June 8, 2017: contributor

Reviewed. utACK f4390a7c11d7bdaf772b2fd44ee11fdbc1b85d86 modulo nits (although merge seems okay at this stage).

TheBlueMatt force-pushed on Jun 21, 2017

TheBlueMatt commented at 3:41 pm on June 21, 2017: member

Rebased and addressed all the comments, I believe.

NicolasDorier commented at 1:37 am on June 22, 2017: contributor

utACK 435c2722289162f852090ca9f6a66df7392cf036

ryanofsky commented at 5:31 pm on June 22, 2017: member

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

TheBlueMatt commented at 5:45 pm on July 10, 2017: member

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

ryanofsky commented at 5:54 pm on July 10, 2017: member

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.

in src/wallet/wallet.cpp:3147 in 435c272228 outdated

3145+
3146+            if (!setInternalKeyPool.empty()) {
3147+                nEnd = *(--setInternalKeyPool.end()) + 1;
3148+            }
3149+            if (!setExternalKeyPool.empty()) {
3150+                nEnd = std::max(nEnd, *(--setExternalKeyPool.end()) + 1);

laanwj commented at 3:55 pm on July 11, 2017:

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

TheBlueMatt force-pushed on Jul 11, 2017

in src/wallet/wallet.cpp:3109 in 1ee8c0f186 outdated

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

ryanofsky commented at 5:56 pm on July 11, 2017:

s/uint64_t/int64_t/

ryanofsky commented at 6:06 pm on July 11, 2017: member

utACK 1ee8c0f1868c8e5ba2f22549ba8965e212867ff1, 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.

TheBlueMatt force-pushed on Jul 11, 2017

TheBlueMatt commented at 10:29 pm on July 11, 2017: member

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

ryanofsky commented at 10:46 pm on July 11, 2017: member

utACK bdf6ba87dd1cb04d4aa33662287285a0481e78c4. Last commit now just switches –end to rbegin.

in src/wallet/wallet.cpp:3158 in bdf6ba87dd outdated

3158+            if (internal) {
3159+                setInternalKeyPool.insert(nEnd);
3160+            } else {
3161+                setExternalKeyPool.insert(nEnd);
3162+            }
3163+            LogPrintf("keypool added key %d, size=%u, internal=%d\n", nEnd, setInternalKeyPool.size() + setExternalKeyPool.size(), internal);

morcos commented at 8:07 pm on July 12, 2017:

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

TheBlueMatt commented at 3:20 pm on July 14, 2017:

Done

morcos commented at 8:13 pm on July 12, 2017: member

utACK bdf6ba8 assuming we also merge #10795

morcos approved

sipa commented at 0:05 am on July 13, 2017: member

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

TheBlueMatt force-pushed on Jul 14, 2017

Track keypool entries as internal vs external in memory

This resolves a super minor performance regressions in several
keypool-handling functions

4a3fc35629

Meet code style on lines changed in the previous commit 28301b9780

Clarify *(--.end()) iterator semantics in CWallet::TopUpKeyPool d40a72ccbb

TheBlueMatt force-pushed on Jul 15, 2017

sipa commented at 1:28 am on July 15, 2017: member

utACK d40a72ccbb71d61b43cbf4d222ca2ab5d3ca7510, only change is the keypool size logging.

morcos commented at 3:56 am on July 15, 2017: member

utACK d40a72c

sipa merged this on Jul 15, 2017

sipa closed this on Jul 15, 2017

sipa referenced this in commit 5cfdda2503 on Jul 15, 2017

laanwj referenced this in commit 7b6e8bc442 on Jul 18, 2017

jonasschnelli commented at 8:25 am on July 18, 2017: contributor

Post merge ACK

PastaPastaPasta referenced this in commit 4bab9cb2d0 on Sep 16, 2019

PastaPastaPasta referenced this in commit b8c1d66fb9 on Sep 18, 2019

barrystyle referenced this in commit 9e562ac2a1 on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021

Track keypool entries as internal vs external in memory #10235