`getrawchangeaddress` should fail when keypool exhausted + add tests #4347

laanwj commented at 12:57 PM on June 16, 2014: member

An user on IRC reported an issue where getrawchangeaddress keeps returning a single address when the keypool is exhausted. In my opinion this is unexpected behaviour.

In this commit:

Change CReserveKey to fail when running out of keys in the keypool.
Make getrawchangeaddress return RPC_WALLET_KEYPOOL_RAN_OUT when unable to create an address.
Add a Python RPC test for checking the keypool behaviour in combination with encrypted wallets.

The other uses of CReserveKey::GetReservedKey are:

In CWallet::CreateTransaction: right after unlocking the wallet, so no problem
In the internal miner in CreateNewBlockWithKey: the miner thread will exit when it cannot reserve a key. In This can happen with an encrypted wallet when the keypool is exhausted. As the internal miner is only used for testing, I think we can live with this.

in qa/rpc-tests/keypool.py:None in 8230467a1b outdated

  63 | +    # drain the keys
  64 | +    addr = set()
  65 | +    addr.add(nodes[0].getrawchangeaddress())
  66 | +    addr.add(nodes[0].getrawchangeaddress())
  67 | +    addr.add(nodes[0].getrawchangeaddress())
  68 | +    addr.add(nodes[0].getrawchangeaddress())

laanwj commented at 1:01 PM on June 16, 2014:

BTW I was a bit surprised here; I do a keypoolrefill(3) with no keys left, but it needs to request four keys to exhaust the pool. Not sure yet why.

sipa commented at 8:31 PM on June 21, 2014:

Perhaps some remaining artefact of a default key, requested before filling the keypool with N extra keys?

laanwj commented at 9:27 AM on June 22, 2014:

Right, that may be it - I want to debug this and know for sure before I merge this.

Regarding the 'default key', after this pull it is only used to detect whether this is the first run with a wallet. A later pull could remove it entirely.

laanwj commented at 1:32 PM on July 11, 2014:

OK, nothing sinister is going on here. One is added to the target size in TopUpKeypool. No idea why, but it's not harmful.

https://github.com/bitcoin/bitcoin/blob/master/src/wallet.cpp#L1765 :

        while (setKeyPool.size() < (nTargetSize + 1))

laanwj added the label Wallet on Jun 17, 2014

laanwj renamed this:
~~`getrawchangeaddress` should fail when keypool exhausted~~
`getrawchangeaddress` should fail when keypool exhausted + add tests
on Jun 18, 2014

sipa commented at 8:34 PM on June 21, 2014: member

Untested ACK

jgarzik commented at 8:38 PM on June 21, 2014: contributor

untested ACK

gmaxwell commented at 8:46 PM on June 21, 2014: contributor

I have a little concern that people will write code which doesn't handle this rare failure and when it happens simply fail to take change entirely... but I don't have anything better to suggest.

laanwj commented at 9:24 AM on June 22, 2014: member

@jgarzik The idea is that it should be possible to restart the mining thread with "setgenerate" after unlocking the wallet and generating a new keypool (still need to test this though). But requiring the entire process to shut down seems overly drastic. Anyhow who would be 'actively mining' using the internal miner and an encrypted wallet? I mean, I suppose the internal miner is still useful for testnet-in-a-box, but that doesn't require top security. @gmaxwell Well if you insist we can do it the careful way, make it an option then deprecate it over time. But if you don't take into account that RPC calls can fail that would be really strange, I doubt this is a very realistic concern.

gmaxwell commented at 12:15 AM on June 23, 2014: contributor

@laanwj I doubt deprecating it would help— it's just the kind of sharp interface which may break people from time to time even if they're amply warned. I don't think we should do any different, I mentioned the concern only for the chance that someone would have some brillant idea I was missing.

BitcoinPullTester commented at 10:24 AM on July 4, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4347_3b43f959e006ee880af60c3c0f33da9dd7d1ffc9/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

`getrawchangeaddress` should fail when keypool exhausted

An user on IRC reported an issue where `getrawchangeaddress`
keeps returning a single address when the keypool is exhausted.
In my opinion this is strange behaviour.

- Change CReserveKey to fail when running out of keys in the keypool.
- Make `getrawchangeaddress` return RPC_WALLET_KEYPOOL_RAN_OUT when
  unable to create an address.
- Add a Python RPC test for checking the keypool behaviour in combination
  with encrypted wallets.

6c37f7fd78

laanwj merged this on Jul 11, 2014

laanwj closed this on Jul 11, 2014

laanwj referenced this in commit d3165ed35a on Jul 11, 2014

MarcoFalke locked this on Sep 8, 2021