Track keypool entries as internal vs external in memory #10235

TheBlueMatt · 2017-04-19T17:12:59Z

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 · 2017-04-19T17:23:03Z

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

NicolasDorier · 2017-04-20T03:41:17Z

src/wallet/wallet.cpp

I think you can also remove this one completely

rpcwallet uses it? Might as well leave it.

NicolasDorier · 2017-04-20T03:56:33Z

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

NicolasDorier · 2017-04-20T03:59:06Z

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

ryanofsky

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

ryanofsky · 2017-05-02T19:34:57Z

src/wallet/wallet.cpp

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.

ryanofsky · 2017-05-02T19:53:50Z

src/wallet/wallet.cpp

Could use iterator version of erase to avoid another lookup

ryanofsky · 2017-05-02T19:57:00Z

src/wallet/wallet.cpp

Maybe assert !setKeyPool.empty()

ryanofsky · 2017-05-02T20:06:49Z

src/wallet/wallet.cpp

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 · 2017-05-05T21:49:06Z

Addressed @ryanofsky's comments and rebased.

ryanofsky

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

ryanofsky · 2017-05-10T16:54:12Z

src/wallet/wallet.cpp

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.

Maybe GetOldestKeyBirthdayInPool or GetOldestKeyTimeInPool

ryanofsky · 2017-05-10T17:02:15Z

src/wallet/wallet.cpp

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 · 2017-06-08T18:14:44Z

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

jonasschnelli · 2017-06-08T18:16:39Z

src/wallet/wallet.cpp

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

jonasschnelli · 2017-06-08T18:21:37Z

src/wallet/wallet.cpp

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

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 · 2017-06-08T18:27:29Z

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

TheBlueMatt · 2017-06-21T15:41:22Z

Rebased and addressed all the comments, I believe.

NicolasDorier · 2017-06-22T01:37:28Z

utACK 435c272

ryanofsky

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

TheBlueMatt · 2017-07-10T17:45:00Z

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

ryanofsky · 2017-07-10T17:54:40Z

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.

laanwj · 2017-07-11T15:55:47Z

src/wallet/wallet.cpp

+                nEnd = *(--setInternalKeyPool.end()) + 1;
+            }
+            if (!setExternalKeyPool.empty()) {
+                nEnd = std::max(nEnd, *(--setExternalKeyPool.end()) + 1);


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

ryanofsky

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.

ryanofsky · 2017-07-11T17:56:18Z

src/wallet/wallet.cpp

s/uint64_t/int64_t/

TheBlueMatt · 2017-07-11T22:29:57Z

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

ryanofsky

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

morcos · 2017-07-12T20:13:12Z

utACK bdf6ba8
assuming we also merge #10795

morcos · 2017-07-12T20:07:09Z

src/wallet/wallet.cpp

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


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

sipa · 2017-07-13T00:05:05Z

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

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

sipa · 2017-07-15T01:28:46Z

utACK d40a72c, only change is the keypool size logging.

morcos · 2017-07-15T03:56:53Z

utACK d40a72c

@jonasschnelli

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

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

jonasschnelli · 2017-07-18T08:25:39Z

Post merge ACK

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 bitcoin#10235, should probably get a 15 tag. Tree-SHA512: c13a18a90f1076fb74307f2d64e9d80149811524c6bda259698ff2c65adaf8c6c3f2a3a07a5f4bf03251bc942ba8f5fd33a4427aa4256748c40b062991682caf

TheBlueMatt mentioned this pull request Apr 19, 2017

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

Closed

jonasschnelli added Refactoring Wallet labels Apr 19, 2017

TheBlueMatt force-pushed the 2017-04-wallet-more-keypool-cache branch 2 times, most recently from 2db2acd to 4a526d9 Compare April 20, 2017 03:21

NicolasDorier reviewed Apr 20, 2017

View reviewed changes

jonasschnelli mentioned this pull request May 4, 2017

Add HD wallet auto-restore functionality #10240

Closed

ryanofsky reviewed May 5, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-04-wallet-more-keypool-cache branch from 4a526d9 to f4390a7 Compare May 5, 2017 21:48

ryanofsky reviewed May 10, 2017

View reviewed changes

jonasschnelli reviewed Jun 8, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-04-wallet-more-keypool-cache branch from f4390a7 to 435c272 Compare June 21, 2017 15:41

ryanofsky reviewed Jun 22, 2017

View reviewed changes

laanwj reviewed Jul 11, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-04-wallet-more-keypool-cache branch 2 times, most recently from 022b557 to 1ee8c0f Compare July 11, 2017 16:08

TheBlueMatt mentioned this pull request Jul 11, 2017

No longer ever reuse keypool indexes #10795

Merged

ryanofsky reviewed Jul 11, 2017

View reviewed changes

src/wallet/wallet.cpp Outdated

Copy link

Contributor

ryanofsky Jul 11, 2017 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/uint64_t/int64_t/

TheBlueMatt force-pushed the 2017-04-wallet-more-keypool-cache branch from 1ee8c0f to bdf6ba8 Compare July 11, 2017 22:28

ryanofsky reviewed Jul 11, 2017

View reviewed changes

morcos approved these changes Jul 12, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-04-wallet-more-keypool-cache branch from bdf6ba8 to 6734c07 Compare July 14, 2017 15:19

TheBlueMatt added 3 commits July 14, 2017 21:25

Track keypool entries as internal vs external in memory

4a3fc35

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

Meet code style on lines changed in the previous commit

28301b9

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

d40a72c

TheBlueMatt force-pushed the 2017-04-wallet-more-keypool-cache branch from 6734c07 to d40a72c Compare July 15, 2017 01:25

sipa merged commit d40a72c into bitcoin:master Jul 15, 2017

Sjors mentioned this pull request Sep 21, 2019

Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) #16341

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Track keypool entries as internal vs external in memory #10235

Track keypool entries as internal vs external in memory #10235

Uh oh!

Conversation

TheBlueMatt commented Apr 19, 2017

Uh oh!

jonasschnelli commented Apr 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Apr 20, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented May 5, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jun 8, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jun 8, 2017

Uh oh!

TheBlueMatt commented Jun 21, 2017

Uh oh!

NicolasDorier commented Jun 22, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 10, 2017

Uh oh!

ryanofsky commented Jul 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 11, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

morcos commented Jul 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Apr 20, 2017 •

edited

Loading

ryanofsky Jul 11, 2017 •

edited

Loading