Change setKeyPool to hold flexible entries #10238

jonasschnelli commented at 1:41 PM on April 20, 2017: contributor

Currently, we keep only keypool indexes (a int64_t) in memory. If we need the CKeyID of 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_t to KeypoolCache. 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 an IsMine or IsFromMe tx).

There are no direct performance optimisations implemented in this PR.

jonasschnelli added the label Wallet on Apr 20, 2017

jonasschnelli force-pushed on Apr 20, 2017

in src/wallet/wallet.h:699 in a4ed0d712b outdated

 699 | @@ -700,7 +700,16 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 700 |  
 701 |      bool fFileBacked;
 702 |  
 703 | -    std::set<int64_t> setKeyPool;

ryanofsky commented at 5:24 PM on May 1, 2017:

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.

jonasschnelli commented at 12:42 PM on May 4, 2017:

Yes. But IMO holding a flexible data-structure (KeypoolCache) allows us to be more flexible in future. Though no strong opinion.

ryanofsky commented at 7:28 PM on May 4, 2017:

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>

jonasschnelli commented at 7:37 PM on May 4, 2017:

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

in src/wallet/wallet.h:704 in a4ed0d712b outdated

 699 | @@ -700,7 +700,16 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 700 |  
 701 |      bool fFileBacked;
 702 |  
 703 | -    std::set<int64_t> setKeyPool;
 704 | +    // class for holding a keypool-key-entry in memory
 705 | +    class KeypoolCache {
 706 | +    public:
 707 | +        int64_t nIndex;
 708 | +        CKeyID keyId;

ryanofsky commented at 5:37 PM on May 1, 2017:

Note: keyId member is not actually used anywhere in this PR. (Fine, just pointing out to help review).

jonasschnelli commented at 12:35 PM on May 4, 2017:

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.

paveljanik commented at 6:35 AM on May 4, 2017: contributor

Rebase needed.

jonasschnelli force-pushed on May 4, 2017

jonasschnelli commented at 12:42 PM on May 4, 2017: contributor

Rebased.

in src/wallet/wallet.h:699 in 2e614359d5 outdated

 695 | @@ -696,7 +696,18 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 696 |      /* HD derive new child key (on internal or external chain) */
 697 |      void DeriveNewChildKey(CKeyMetadata& metadata, CKey& secret, bool internal = false);
 698 |  
 699 | -    std::set<int64_t> setKeyPool;
 700 | +    bool fFileBacked;

NicolasDorier commented at 1:04 PM on May 4, 2017:

why is it added ? where is it used ?

jonasschnelli commented at 1:09 PM on May 4, 2017:

Oh. That's a rebase issue. fFileBackand was removed in the meantime, Will remove it here.

jonasschnelli commented at 1:13 PM on May 4, 2017:

Removed.

in src/wallet/wallet.cpp:3331 in 2e614359d5 outdated

3324 | @@ -3324,10 +3325,10 @@ void CWallet::GetAllReserveKeys(std::set<CKeyID>& setAddress) const
3325 |      CWalletDB walletdb(*dbw);
3326 |  
3327 |      LOCK2(cs_main, cs_wallet);
3328 | -    BOOST_FOREACH(const int64_t& id, setKeyPool)
3329 | +    for (const KeypoolCache &poolPair : setKeyPool)
3330 |      {
3331 |          CKeyPool keypool;
3332 | -        if (!walletdb.ReadPool(id, keypool))
3333 | +        if (!walletdb.ReadPool(poolPair.nIndex, keypool))

NicolasDorier commented at 1:05 PM on May 4, 2017:

I am wondering if it is not better (minimum diff) to just change ReadPool signature to take KeyPoolCache instead of index.

Change setKeyPool from pure int64_t to a custom class for more flexible in-mem storage af1c4ec53a

in src/wallet/wallet.h:970 in 2e614359d5 outdated

 967 | @@ -957,7 +968,7 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface
 968 |      bool TopUpKeyPool(unsigned int kpSize = 0);
 969 |      void ReserveKeyFromKeyPool(int64_t& nIndex, CKeyPool& keypool, bool internal);
 970 |      void KeepKey(int64_t nIndex);
 971 | -    void ReturnKey(int64_t nIndex);
 972 | +    void ReturnKey(int64_t nIndex, const CKeyID &keyid);

NicolasDorier commented at 1:06 PM on May 4, 2017:

why not just take KeypoolCache ?

jonasschnelli commented at 1:09 PM on May 4, 2017:

Hmm.. then CReserveKey::Return key would have to create a KeypoolCache()?

NicolasDorier commented at 1:21 PM on May 4, 2017:

why ? I mean KeypoolCache is (int, KeyId) anyway. It is just strange to pass them as two distinct parameters here.

jonasschnelli force-pushed on May 4, 2017

ryanofsky commented at 5:55 PM on July 25, 2017: member

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:

Easiest way is probably just to just cache the result of GetAllReserveKeys during the rescan so it doesn't get called multiple times in the same scan.
Another way could be update this PR so GetAllReserveKeys() could work from memory without touching the database. This optimization is not exclusive with (1) above and both could be done together.
Another way could be to update this PR to store a bidirectional mapping from key id <-> key index instead of just forward mapping so GetAllReserveKeys() wouldn't have to do any work and could just return a reference to the reverse map. This could be done with boost multindex or just two plain maps.

jonasschnelli commented at 3:37 AM on October 5, 2017: contributor

No longer really useful, closing for now.

jonasschnelli closed this on Oct 5, 2017

TheBlueMatt commented at 9:23 PM on October 5, 2017: member

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.

DrahtBot locked this on Sep 8, 2021