[Wallet] slighly refactor GetOldestKeyPoolTime() #7816

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2016/04/wallet_oldest_key changing 1 files +12 −5

jonasschnelli commented at 1:57 PM on April 5, 2016: contributor

Not sure if this solution is much better. We don't keep keypool keys in memory (only a 64bit identifier), therefore a DB operation is required. Another solution would be to do a in-mem cache of the oldest key.

This solution, at least, does not retain and release the key.
jonasschnelli added the label Refactoring on Apr 5, 2016
jonasschnelli added the label Wallet on Apr 5, 2016
laanwj commented at 4:14 PM on April 5, 2016: member

This is meant to fix #7794

in src/wallet/wallet.cpp:None in a77bb1046c outdated

2615 | -    CKeyPool keypool;
2616 | -    ReserveKeyFromKeyPool(nIndex, keypool);
2617 | -    if (nIndex == -1)
2618 | +    LOCK(cs_wallet);
2619 | +
2620 | +    // is the keypool is empty, return <NOW>

paveljanik commented at 6:48 PM on April 5, 2016:

Typo: if

paveljanik commented at 6:53 PM on April 5, 2016: contributor

tests fail with:

Initializing test directory /var/folders/65/fn0h49r55k7779vg1b_h461r0000gn/T/testfGlvKm
JSONRPC error: Error: Keypool ran out, please call keypoolrefill first
  File "/private/tmp/bitcoin-7816/qa/rpc-tests/test_framework/test_framework.py", line 135, in main
    self.run_test()
  File "./keypool.py", line 83, in run_test
    nodes[0].generate(1)
  File "/private/tmp/bitcoin-7816/qa/rpc-tests/test_framework/coverage.py", line 50, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/private/tmp/bitcoin-7816/qa/rpc-tests/test_framework/authproxy.py", line 139, in __call__
    raise JSONRPCException(response['error'])
Stopping nodes
Cleaning up
Failed

in src/wallet/wallet.cpp:None in a77bb1046c outdated
```
2626 | +    CKeyPool keypool;
2627 | +    CWalletDB walletdb(strWalletFile);
2628 | +    int64_t nIndex = *(setKeyPool.begin());
2629 | +    setKeyPool.erase(setKeyPool.begin());
2630 | +    if (!walletdb.ReadPool(nIndex, keypool))
2631 | +        throw runtime_error("GetOldestKeyPoolTime(): read oldest key in keypool failed");
```
paveljanik commented at 6:55 PM on April 5, 2016:

How do you know it is GetOldestKeyPoolTime who failed?

Edit: Ah, please use __func__ ;-))

jonasschnelli commented at 7:10 AM on April 6, 2016:

Thanks. But lets keep GetOldestKeyPoolTime(): for now and let it open if someone wants to change the whole wallet sources to make use of __func__. Mixing would be worse IMO.

paveljanik commented at 7:32 AM on April 6, 2016:

Sorry, this is like using TABs to indent and say that it is OK to use TABs, and later someone could reformat it completely with clang-format. I thought we already agreed that in new code, we should follow our best practice (and as you see on my example above, it can even fool people). And it is shorter ;-)
jonasschnelli force-pushed on Apr 6, 2016
jonasschnelli commented at 7:12 AM on April 6, 2016: contributor

Fixed the bug (accidentally erase keypool key) that caused the test to fail. Fixed @paveljanik typo/nit.
paveljanik commented at 7:40 AM on April 6, 2016: contributor
So if the keypool is empty, you are returning NOW. RPC getinfo's help says/expects the key to exist though:
```
  "keypoololdest": xxxxxx,    (numeric) the timestamp (seconds since GMT epoch) of the oldest pre-generated key in the key pool
```
This is a slight change in the semantic, but I think it is OK.
paveljanik commented at 8:15 AM on April 6, 2016: contributor

minor nit: commit message typo slighly-> slightly (sorry for noticing all these unimportant things, please bear with me...).
[Wallet] slightly refactor GetOldestKeyPoolTime() 9f7336b457
jonasschnelli force-pushed on Apr 6, 2016
jonasschnelli commented at 9:07 AM on April 6, 2016: contributor

@paveljanik:

So if the keypool is empty, you are returning NOW

I think this was already the case before. ReserveKeyFromKeyPool returns -1 if the keypool is empty which should have lead to return GetTime()? Or am I wrong'

Thanks. Fixed the commit typo. I'm happy if you point to nits/typos.
laanwj commented at 10:25 AM on April 19, 2016: member

Yes, the return GetTime(); was already there. If this was written from scratch it could be argued if returning the current time is the sensible thing to do, but let's not change this API.
laanwj commented at 10:40 AM on April 19, 2016: member

tACK 9f7336b Can confirm that this solves #7794.
laanwj merged this on Apr 22, 2016
laanwj closed this on Apr 22, 2016
laanwj referenced this in commit 0c95ebce7e on Apr 22, 2016
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels