getrawchangeaddress should fail when keypool exhausted + add tests
#4347
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Perhaps some remaining artefact of a default key, requested before filling the keypool with N extra keys?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK, nothing sinister is going on here. One is added to the target size in TopUpKeypool. No idea why, but it's not harmful.
while (setKeyPool.size() < (nTargetSize + 1))
laanwj
changed the title
getrawchangeaddress should fail when keypool exhaustedgetrawchangeaddress should fail when keypool exhausted + add tests
|
Untested ACK |
|
untested ACK |
|
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. |
|
@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. @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. |
|
@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. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4347_3b43f959e006ee880af60c3c0f33da9dd7d1ffc9/ for binaries and test log. |
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.
6c37f7f `getrawchangeaddress` should fail when keypool exhausted (Wladimir J. van der Laan)
5 participants
An user on IRC reported an issue where
getrawchangeaddresskeeps returning a single address when the keypool is exhausted. In my opinion this is unexpected behaviour.In this commit:
getrawchangeaddressreturn RPC_WALLET_KEYPOOL_RAN_OUT when unable to create an address.The other uses of CReserveKey::GetReservedKey are:
CWallet::CreateTransaction: right after unlocking the wallet, so no problemCreateNewBlockWithKey: 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.