CCryptoKeyStore::Unlock has a loop to attempt decrypting each key which only executes once, likely due to a simple mistake when the code was originally written.
This patch fixes the behavior by making it check all keys. It also adds a fatal assertion in the case some decrypt but some do not, since that indicates that the wallet is in some kind of really bad state.
This may make unlocking noticeably slower on wallets with many keys.
CCryptoKeyStore::Unlock has a loop to attempt decrypting each key which
only executes once, likely due to a simple mistake when the code was
originally written.
This patch fixes the behavior by making it check all keys. It also adds
a fatal assertion in the case some decrypt but some do not, since that
indicates that the wallet is in some kind of really bad state.
This may make unlocking noticeably slower on wallets with many keys.
If people don't think that checking all keys and handling the error in this way is stupid, I'll go ahead and check with a huge wallet to verify that the slowdown isn't excessive. At least with a 100 key keypool it's not noticeable.
This seems very sensible to me. What is the biggest size of wallet that is known to have been used?
Checking just one key was always the original intention, as far as I remember.
This will require an EC multiplication per key (and GetPubKey may be doing more than one...).
Sorry for the dumb question, but what does checking all the keys in this case get us? If it makes wallets with lots of keys even slower, we'd need to have a good reason to do this.
@sipa Bluematt seemed to think otherwise wrt intention. :) @laanwj it would catch any case where the public keys and the secret keys in the wallet didn't agree with each other, either due to corruption or due to malicious modification (e.g. so that newly issued keys were really someone elses private keys). How much thats worth I dunno. I don't have that much of an opinion: The existing code is brain damanged, either it should check more than one or the loop should be removed. I don't have that much of an opinion which we do, which is why I was asking what people thought before I went into benchmarking it on wallets with lots of keys.
Right. Checking all the keys is a good wallet integrity check. But it doesn't seem necessary to me to check all the keys every time that the wallet is unlocked. One idea would be to do it the first time, or to even add an argument to walletpassphrase for a 'deep' validation.
First time makes sense, I don't like adding more api surface. I'll go and check it on a huge wallet and verify that it's not too disruptive and if not I'll make it check all just once.
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4011_f36754839b5462b330b92fa488c467cc4b38a028/ 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.
I'm still of the opinion that it is not acceptable to do the decoding and checking all keys in the wallet every time that walletpassphrase is called, which would be the effect of this. This command is very frequently used, sometimes for every transaction.
So let's just go with doing this only the first time.
Is there an easy way to make PVS-Studio happy without the burden of checking every time? Or make this a santiy check on startup?
Sure, add a boolean flag to CCryptoKeyStore::Unlock whether to do a full check. Do the early return only if that is not set.
Meh, just static boolean fAlreadyChecked = false; set to true after the first run. Early return if its true.
That might be a good compromise. The tension I see is that the performance is important, but if you can't actually spend what you think you can thats very bad and ought to be detected (e.g. as a result of corruption). If the once-per-run test matt proposes is too slow, A compromise would be checking the keypool (or at least the near term keypool) only might also be reasonable.
A once-per-run check is fine.
Subsumed by Bluematt's