Use boost::asio::deadline_timer for walletpassphrase timeout #2625
Conversation
|
Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/eb477a63fefdc29db9d152a9189ca790e0cb2519 for binaries and test log. This could happen for one of several reasons:
If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here. 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/ |
|
ACK |
New method in bitcoinrpc: RunLater, that uses a map of deadline
timers to run a function later.
Behavior of walletpassphrase is changed; before, calling
walletpassphrase again before the lock timeout passed
would result in: Error: Wallet is already unlocked.
You would have to call lockwallet before walletpassphrase.
Now: the last walletpassphrase with correct password
wins, and overrides any previous timeout.
Fixes issue# 1961 which was caused by spawning too many threads.
Test plan:
Start with encrypted wallet, password 'foo'
NOTE:
python -c 'import time; print("%d"%time.time())'
... will tell you current unix timestamp.
Try:
walletpassphrase foo 600
getinfo
EXPECT: unlocked_until is about 10 minutes in the future
walletpassphrase foo 1
sleep 2
sendtoaddress mun74Bvba3B1PF2YkrF4NsgcJwHXXh12LF 11
EXPECT: Error: Please enter the wallet passphrase with walletpassphrase first.
walletpassphrase foo 600
walletpassphrase foo 0
getinfo
EXPECT: wallet is locked (unlocked_until is 0)
walletpassphrase foo 10
walletpassphrase foo 600
getinfo
EXPECT: wallet is unlocked until 10 minutes in future
walletpassphrase foo 60
walletpassphrase bar 600
EXPECT: Error, incorrect passphrase
getinfo
EXPECT: wallet still scheduled to lock 60 seconds from first (successful) walletpassphrase
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/92f2c1fe0fe2905540b0435188988851145f92be for binaries and test log. |
There was a problem hiding this comment.
Maybe add a comment that err will have a nonzero value when the timer was cancelled, I had to look this up as it was unclear to me why a timer handler would get an error passed in.
|
Still have to test, but code changes are OK, nice cleanup |
|
So one consequence of this is: #task 1 #task 2 Now depending on how these tasks race the second kills the first. This seems bad and I thought avoiding this was previously intentional. I think instead it should be doing max(new_unlocked_until, old_unlocked_until), and if someone wants to lock an unlocked wallet they can explicitly call walletlock. |
|
@gmaxwell : previously the second task's 'walletpassphrase' would fail with "error: wallet already unlocked." If you REALLY want the max() behavior, then I say have the tasks call getinfo to get unlocked_until and do that calculation yourself. I'm very much in favor of the RPC having the most obvious implementation, and strongly feel that "do the last thing I told you to do" is the most obvious behavior. |
|
@gavinandresen You can't do that calculation yourself though, because there is a potential race if you have multiple callers calling the RPC at once. E.g. if the shorter unlock calls getinfo first then the longer unlock happens before the shorter unlock the longer unlock's request will get ignored... and we have no way to facilitate exclusive access to the RPC. "Do the last thing" starts losing its meaning when there is concurrent access. It's not the end of the world— don't make concurrent accesses with different unlock lifetimes is also an answer... I'm just generally uneasy about behavior that may expose racy behavior in callers because it's really hard to debug. |
There was a problem hiding this comment.
This used to be the source of quite some lag when calling unlock, not sure if it matters, but thats why it was in a separate thread.
There was a problem hiding this comment.
Topping up the keypool in a separate thread adds indeterminism which scares me. I'd rather the state of the wallet be known when walletpassphrase returns ("full keypool, unlocked").
E.g. if the keypool is completely exhausted, and I do 'walletpassphrase' immediately followed by a 'send', will I always get a fresh 'change' key or not? Separate thread == maybe, maybe not....
There was a problem hiding this comment.
I believe using anything from keypool tries to top up keypool before it uses any keys anyway. Also, the topup keypool thread locks wallet first thing anyway, so unless your machine is under heavy load (or a shitty VPS), it should be pretty constant. Anyway, if you dont care about the performance, it doesnt matter much.
|
re: fixes #1961 were you able to reproduce the original issue to begin with? |
Use boost::asio::deadline_timer for walletpassphrase timeout
6 participants
New method in bitcoinrpc: RunLater, that uses a map of deadline
timers to run a function later.
Behavior of walletpassphrase is changed; before, calling
walletpassphrase again before the lock timeout passed
would result in: Error: Wallet is already unlocked.
You would have to call lockwallet before walletpassphrase.
Now: the last walletpassphrase with correct password
wins, and overrides any previous timeout.
Fixes issue# 1961 which was caused by spawning too many threads.
Test plan:
Start with encrypted wallet, password 'foo'
NOTE:
python -c 'import time; print("%d"%time.time())'
... will tell you current unix timestamp.
Try:
walletpassphrase foo 600
getinfo
EXPECT: unlocked_until is about 10 minutes in the future
walletpassphrase foo 1
sleep 2
sendtoaddress mun74Bvba3B1PF2YkrF4NsgcJwHXXh12LF 11
EXPECT: Error: Please enter the wallet passphrase with walletpassphrase first.
walletpassphrase foo 600
walletpassphrase foo 0
getinfo
EXPECT: wallet is locked (unlocked_until is 0)
walletpassphrase foo 10
walletpassphrase foo 600
getinfo
EXPECT: wallet is unlocked until 10 minutes in future
walletpassphrase foo 60
walletpassphrase bar 600
EXPECT: Error, incorrect passphrase
getinfo
EXPECT: wallet still scheduled to lock 60 seconds from first (successful) walletpassphrase