Change setKeyPool to hold flexible entries #10238
Conversation
jonasschnelli
force-pushed
the
2017/04/keypool_fix_a
branch
from
cd818b4 to
a4ed0d7
Compare
src/wallet/wallet.h
Outdated
There was a problem hiding this comment.
I think it would be more straightforward to replace setKeyPool with a std::map<int64_t, CKeyID> instead of taking this approach which basically emulates a map using set entries with a custom < operator.
There was a problem hiding this comment.
Yes. But IMO holding a flexible data-structure (KeypoolCache) allows us to be more flexible in future. Though no strong opinion.
There was a problem hiding this comment.
holding a flexible data-structure (KeypoolCache) allows us to be more flexible in future
I don't have any problem with keeping KeypoolCache if it will be useful later.
What seems bad to me is using std::set with a custom comparison function to emulate a map. Would suggest removing nIndex and operator< from KeypoolCache and using std::map<int64_t, KeypoolCache>
There was a problem hiding this comment.
Fair point. The question is should we do it in this PR or later. But since we significant change the set's content, It should be changed now (which I will do soon).
src/wallet/wallet.h
Outdated
There was a problem hiding this comment.
Note: keyId member is not actually used anywhere in this PR. (Fine, just pointing out to help review).
There was a problem hiding this comment.
Yes. This is true, though, the keyId is relatively important for future performance improvements. We could add it later,.. but I guess it doesn't hurt to already have it there in this PR.
|
Rebase needed. |
jonasschnelli
force-pushed
the
2017/04/keypool_fix_a
branch
from
a4ed0d7 to
2e61435
Compare
|
Rebased. |
src/wallet/wallet.h
Outdated
There was a problem hiding this comment.
why is it added ? where is it used ?
There was a problem hiding this comment.
Oh. That's a rebase issue. fFileBackand was removed in the meantime, Will remove it here.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
I am wondering if it is not better (minimum diff) to just change ReadPool signature to take KeyPoolCache instead of index.
src/wallet/wallet.h
Outdated
There was a problem hiding this comment.
why not just take KeypoolCache ?
There was a problem hiding this comment.
Hmm.. then CReserveKey::Return key would have to create a KeypoolCache()?
There was a problem hiding this comment.
why ? I mean KeypoolCache is (int, KeyId) anyway. It is just strange to pass them as two distinct parameters here.
…le in-mem storage
jonasschnelli
force-pushed
the
2017/04/keypool_fix_a
branch
from
2e61435 to
af1c4ec
Compare
|
In #10882 it seems like looking up keypool index using key id is more useful than looking up key id using key index, so I'm not sure if there is a direct use for this PR and maybe it should be reconsidered. If the goal is to avoid reading all the unused keys from disk each time an IsMine transaction is encountered, I think there are three ways it could be done:
|
|
No longer really useful, closing for now. |
|
Hmm, I actually think this was a really nice change, though I guess it hits at the broader specifically-define-what-we-cache-from-db issue that has persisted for some time. |
5 participants
Currently, we keep only keypool indexes (a
int64_t) in memory.If we need the
CKeyIDof a keypool key (or if we need to know if it's an internal key), we need to load the whole CKeyPool entry from the database.This PR will change the in-memory set entries from
int64_ttoKeypoolCache.This essentially allows us to efficiently scan the
CKeyIDs of our keypool which is almost a pre-requirement if we want to have an efficient HD rescan (otherwise we would have to load the complete key pool each time we found anIsMineorIsFromMetx).There are no direct performance optimisations implemented in this PR.