[Wallet] slighly refactor GetOldestKeyPoolTime()

[jonasschnelli]

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.

[laanwj]

This is meant to fix #7794

[paveljanik]

Typo: if

[paveljanik]

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

[paveljanik]

How do you know it is GetOldestKeyPoolTime who failed?

Edit: Ah, please use __func__ ;-))

[jonasschnelli]

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]

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]

Fixed the bug (accidentally erase keypool key) that caused the test to fail.
Fixed @paveljanik typo/nit.

[paveljanik]

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]

minor nit: commit message typo slighly-> slightly (sorry for noticing all these unimportant things, please bear with me...).

[jonasschnelli]

@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]

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]

tACK 9f7336b Can confirm that this solves #7794.