Use boost::asio::deadline_timer for walletpassphrase timeout

gavinandresen commented at 3:03 pm on May 7, 2013: contributor

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

BitcoinPullTester commented at 3:26 pm on May 7, 2013: none

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:

It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in contrib/test-scripts)
It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
It does not build on either Linux i386 or Win32 (via MinGW cross compile)
The test suite fails on either Linux i386 or Win32
The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

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/ Contact BlueMatt on freenode if something looks broken.

jgarzik commented at 3:38 pm on May 7, 2013: contributor

ACK

Use boost::asio::deadline_timer for walletpassphrase timeout

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

92f2c1fe0f

BitcoinPullTester commented at 4:29 pm on May 7, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/92f2c1fe0fe2905540b0435188988851145f92be 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.

in src/bitcoinrpc.cpp: in 92f2c1fe0f

851@@ -850,6 +852,26 @@ void StopRPCThreads()
852     delete rpc_io_service; rpc_io_service = NULL;
853 }
854 
855+void RPCRunHandler(const boost::system::error_code& err, boost::function<void(void)> func)
856+{
857+    if (!err)

laanwj commented at 7:14 pm on May 7, 2013:

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.

laanwj commented at 7:19 pm on May 7, 2013: member

Still have to test, but code changes are OK, nice cleanup

gmaxwell commented at 8:34 pm on May 7, 2013: contributor

So one consequence of this is:

#task 1 walletpassphrase foo 3600 // I need this for a long time sleep 3000 signrawtranaction ..

#task 2 walletpassphrase foo 30 //I don’t need this for long signrawtransaction ..

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.

gavinandresen commented at 9:01 pm on May 7, 2013: contributor

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

gmaxwell commented at 10:38 pm on May 7, 2013: contributor

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

in src/rpcwallet.cpp: in 92f2c1fe0f

1290@@ -1339,9 +1291,12 @@ Value walletpassphrase(const Array& params, bool fHelp)
1291             "walletpassphrase <passphrase> <timeout>\n"
1292             "Stores the wallet decryption key in memory for <timeout> seconds.");
1293 
1294-    NewThread(ThreadTopUpKeyPool, NULL);
1295-    int64* pnSleepTime = new int64(params[1].get_int64());
1296-    NewThread(ThreadCleanWalletPassphrase, pnSleepTime);
1297+    pwalletMain->TopUpKeyPool();

TheBlueMatt commented at 9:50 am on May 8, 2013:

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.

gavinandresen commented at 1:41 pm on May 8, 2013:

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….

TheBlueMatt commented at 1:55 pm on May 8, 2013:

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.

TheBlueMatt commented at 9:54 am on May 8, 2013: member

re: fixes #1961 were you able to reproduce the original issue to begin with?

jgarzik referenced this in commit 9c95a2e836 on May 30, 2013

jgarzik merged this on May 30, 2013

jgarzik closed this on May 30, 2013

gavinandresen deleted the branch on Nov 4, 2013

DrahtBot locked this on Sep 8, 2021

Use boost::asio::deadline_timer for walletpassphrase timeout #2625