Fixes #12100
Makes the timeout be clamped to 2^30 seconds to avoid the issue with sign flipping with large timeout values and thus relocking the wallet instantly. Unlocking for at most ~34 years should be sufficient.
Also checks that the timeout is not negative to avoid instant relocks.
2297@@ -2298,6 +2298,14 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
2298 // Alternately, find a way to make request.params[0] mlock()'d to begin with.
2299 strWalletPass = request.params[0].get_str().c_str();
2300
2301+ // Get the timeout
2302+ int32_t nSleepTime = request.params[1].get_int();
2303+ pwallet->nRelockTime = GetTime() + nSleepTime;
2297@@ -2298,6 +2298,14 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
2298 // Alternately, find a way to make request.params[0] mlock()'d to begin with.
2299 strWalletPass = request.params[0].get_str().c_str();
2300
2301+ // Get the timeout
2302+ int32_t nSleepTime = request.params[1].get_int();
int, not int32_t (they’re not guaranteed to be the same)
2302+ int nSleepTime = request.params[1].get_int();
2303+ // Timeout cannot be negative, otherwise it will relock immediately
2304+ if (nSleepTime < 0) {
2305+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative.");
2306+ }
2307+ pwallet->nRelockTime = GetTime() + nSleepTime;
Fixed.
nRelockTime isn’t actually used for anything except displaying stuff in getwalletinfo so this wouldn’t actually be a problem.
2297@@ -2298,6 +2298,13 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
2298 // Alternately, find a way to make request.params[0] mlock()'d to begin with.
2299 strWalletPass = request.params[0].get_str().c_str();
2300
2301+ // Get the timeout
2302+ int nSleepTime = request.params[1].get_int();
2303+ // Timeout cannot be negative, otherwise it will relock immediately
2304+ if (nSleepTime < 0) {
(You’re currently allowing zero here.)
Right.
It doesn’t matter either way IMO.
55@@ -56,6 +56,11 @@ def run_test(self):
56 assert_raises_rpc_error(-14, "wallet passphrase entered was incorrect", self.nodes[0].walletpassphrase, passphrase, 10)
57 self.nodes[0].walletpassphrase(passphrase2, 10)
58 assert_equal(privkey, self.nodes[0].dumpprivkey(address))
59+ self.nodes[0].walletlock()
60+
61+ # Test timeout bounds
62+ assert_raises_rpc_error(-8, "Timeout cannot be negative.", self.nodes[0].walletpassphrase, passphrase, -10)
63+ assert_raises_rpc_error(-1, "JSON integer out of range", self.nodes[0].walletpassphrase, passphrase, 2147483648)
From user experience point of view when we have “seconds” defined timeout I would expect times maybe up to 1 day. Giving big number does not make sense.
If it is intended to allow days then the timeout should be specified in “days” and not “seconds”.
Look at the usability aspect. Anyway I don’t have a clue how it is used today in real life.
Giving big number does not make sense.
Yes it does. A common and recommended configuration on a system with an online wallet which would otherwise not be encrypted is to encrypt and then manually unlock any time the system is restarted. If we did not allow a setting which was effectively ‘until restart’ these systems would simply not use encryption.
This configuration saved localbitcoins loss of almost all their funds after a datacenter operator was social engineered, just as a single example. – Facilities social engineering has historically been one of the most common compromise vectors, and it almost always results in a system reboot.
If it is intended to allow days then the timeout should be specified in “days” and not “seconds”.
Different applications have different scales. More ordinary CLI user usage typical timeouts are just a few seconds or a few minutes.
What would be the point of allowing 0?
To trigger a keypool topup. (though I didn’t look to see if that would be guaranteed to work, but we should make it do so by performing the topup on unlock while still holding the lock)
I’d feel more comfortable using int64, but add a clamp (if value is over 100 years, set it to 100 years or so).
That sounds fine, in particular we don’t want people who have been happily using 999999999 to find it suddenly returning an error.
time_t type that is used for the timer can be a 32 bit signed integer on some systems so this will avoid sign flipping with such systems.
"2. timeout (numeric, required) The time to keep the decryption key in seconds.\n"
It looks like Travis test fails on 32bit architecture. There is something wrong as you get a lot of messages like:
JSONRPCException: Error: Please enter the wallet passphrase with walletpassphrase first.
I don’t know yet why you get it yet.
Also you can see that in the log printout there is a wrong value:
node1 2018-01-08 18:52:07.849526 queue run of timer lockwallet(wallet.dat) in -2147483648 seconds (using HTTP)
The number is -2147483648 is 1L«31 on 32bit architecture.
This faulty printout is probably caused by LogPrint string formatting in RPCRunLater:
LogPrint(BCLog::RPC, "queue run of timer %s in %i seconds (using %s)\n", name, nSeconds, timerInterface->Name());
63+ # Check the timeout
64+ # Check a time less than the limit
65+ expected_time = int(time.time()) + (1 << 31) - 600
66+ self.nodes[0].walletpassphrase(passphrase2, (1 << 31) - 600)
67+ actual_time = self.nodes[0].getwalletinfo()['unlocked_until']
68+ assert actual_time < expected_time + 5 and actual_time >= expected_time # 5 second buffer
expected_time <= actual_time < expected_time + 5
63+ # Check the timeout
64+ # Check a time less than the limit
65+ expected_time = int(time.time()) + (1 << 31) - 600
66+ self.nodes[0].walletpassphrase(passphrase2, (1 << 31) - 600)
67+ actual_time = self.nodes[0].getwalletinfo()['unlocked_until']
68+ assert expected_time <= actual_time < expected_time + 5 # 5 second buffer
Might want to use the provided assert_greater... helper methods to give a more verbose output in case of failure.
Current error shows only:
0File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet-encryption.py", line 68, in run_test
1 assert expected_time <= actual_time < expected_time + 5 # 5 second buffer
2AssertionError
and not by how much it was off.
2304+ if (nSleepTime < 0) {
2305+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative.");
2306+ }
2307+ // Clamp timeout to 2^31 seconds
2308+ if (nSleepTime > ((int64_t)1 << 31) - 1) {
2309+ nSleepTime = ((int64_t)1 << 31) - 1;
You could use the named constexpr std::numeric_limits<int32_t>::max().
Also, how does that take into account your observation that “the time_t type that is used for the timer can be a 32 bit signed integer on some systems”. (Considering that nSleepTime is multiplied by 1000 somewhere down in the call stack)
You could use the named constexpr
std::numeric_limits<int32_t>::max().
I’ll try that now.
Also, how does that take into account your observation that “the time_t type that is used for the timer can be a 32 bit signed integer on some systems”. (Considering that nSleepTime is multiplied by 1000 somewhere down in the call stack)
It’s multiplied by 1000 and then divided by 1000 before going into the time_t.
GetTime(), capping it at the max of int32_t is not enough?
wallet->nRelockTime which is only used for display in getwalletinfo.
2268@@ -2269,7 +2269,7 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
2269 "This is needed prior to performing transactions related to private keys such as sending bitcoins\n"
2270 "\nArguments:\n"
2271 "1. \"passphrase\" (string, required) The wallet passphrase\n"
2272- "2. timeout (numeric, required) The time to keep the decryption key in seconds.\n"
2273+ "2. timeout (numeric, required) The time to keep the decryption key in seconds. Limited to at most 2147483647 (2^31 - 1) seconds.\n"
Maybe you can add some more observability from C++ code to debug.log It can help to find what is wrong with tests on 32bit.
It looks like you get similar fault as original one but only on 32 bits now. From the log you can see that actual_time is 0. This is taken from pwallet->nRelockTime. So if it is 0 then the wallet is locked.
I don’t see how this can happen. It might be some other fault related to the timers handling?
I’ve tested a bit on the official release and I have found that the on 64bits it works fine for 9223372036854775 and lower timeout values and fails for 9223372036854776 and bigger timeout values (than again it works and fails in a cyclic way but I’have not found yet the cycle amplitude… maybe tomorrow but it is on the level of 2^53).
The point is that 9223372036854776 is nothing special when it comes to binary representation. This suggests that fault might be somewhere else.
timeval struct, yet the wallet is still locking immediately.
Clamps the timeout of walletpassphrase to 2^(30) seconds, which is
~34 years. Any number greater than that will be forced to be
2^(30). This avoids the sign flipping problem with large values which
can result in a negative time used.
Also perform bounds checks to ensure that the timeout is positive
to avoid immediate relocking of the wallet.
I still think we don’t understand the fault’s root cause very well. This 32bit failing doesn’t give a rest. I have tested this functionality on 32bit version (0.15.1 without your fixes). What I have found is:
libevent: event.c:3117: Assertion tv->tv_usec >= 0 failed in timeout_nextI suspect QTimer implementation but I have not succeeded to troubleshoot it yet.
Still open questions: Why with high numbers wallet it re-locked immediately? What is causing this behaviour? Which code exactly? Is it QTimer?
I still think we don’t understand the fault’s root cause very well.
That’s correct. I have no clue why it is failing, although I believe it has to do with floating point numbers. I’m pretty sure the issue has to do with the timeval struct, in particular tv.tv_sec’s type of time_t.
I suspect QTimer implementation but I have not succeeded to troubleshoot it yet.
We don’t use QTimers. Rather it uses libevent, so the issue lies in their implementation AFAICT.
libevent. I’ve mixed it up. There must be something wrong somewhere :)
We don’t use QTimers. Rather it uses libevent, so the issue lies in their implementation AFAICT.
In the most common case, yes. IIRC it does use QTimers when -server is false in the GUI: in that case libevent isn’t initialized. Multiple timer implementations can be registered through RPCSetTimerInterface.
Not sure why I didn’t think of this sooner: but another thing we can do instead of clamping the time when larger than a certain value, is to not use a timer at all in that case - interpret large numbers as ‘infinite’, unlocking forever (or until manually re-locked). Of course, this needs to be documented.
This avoids having to cope with timer edge-case behavior entirely.
2304+ // Timeout cannot be negative, otherwise it will relock immediately
2305+ if (nSleepTime < 0) {
2306+ throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative.");
2307+ }
2308+ // Clamp timeout to 2^30 seconds
2309+ if (nSleepTime > (int64_t)1 << 30) {
clamping doesn’t work very well on every platform
Where are you seeing this? It seems like current implementation should work on every platform. Agree that laanwj’s suggestion would make code a little cleaner, but the behavior for all practical purposes would seem identical to what is implemented here.
Also, regardless of anything, current PR seems like an improvement over status the quo.
Where are you seeing this?
The “magic value” was determined by “ok, std::numeric_limits<int32_t>::max() doesn’t work, so let’s go a bit lower and try again”. See
I got fed up with debugging this thing and just lowered the clamp to 2^30 seconds
Also, regardless of anything, current PR seems like an improvement over status the quo.
Agree.
clamping doesn’t work very well on every platform
We would still need some magic value where all of the values less than it are valid timeouts. Using std::numeric_limits<int32_t>::max() as the cutoff magic value for an infinite timeout would still be problematic as using certain values less than that would still result in invalid timeouts. Changing it from clamping to “infinite timeout cutoff” doesn’t resolve that problem.
Just to remind that we don’t know the root cause of this behaviour. Suspected is libevent library.
Described behaviour is that with big number wallet gets locked immediately so probably timeout is fired right after the command to set timer is given. I have observed also that sometimes this is few seconds delayed if bitcoind is heavily occupied with some tasks. This suggests event handling.
Find a root cause to have 100% safe solution. I have not debugged it to this level yet (lack of time)
0 <= nSleepTime <= 1,073,741,824 in wallet/rpcwallet.cpp is an int64_t, measured in seconds
pwallet->nRelockTime = GetTime()+nSleepTime in wallet/rpcwallet.cpp is an int64_t, so will only overflow when GetTime() returns 2**30*(2**33-1) which is about 292 million millenia away, and it’s only used for printing in any event.
nSleepTime gets passed to RPCRunLater which multiplies it by 1000 to convert to milliseconds and passes it to RPCTimerInterface::NewTimer as an int64_t. 0 <= 1000*nSleepTime <= 1,073,741,824,000, which fits in about 40 bits, so still doesn’t overflow.
RPCTimerInterface::NewTimer can be either QtRPCTimerInterface (using QTimers) or HTTPRPCTimerInterface (using libevent).
QtRPCTimerInterface passes the millisecond value to QTimer::start(), which in Qt5 accepts a std::chrono::milliseconds which is specced to be at least 45 bits, so is fine. In Qt4.8 it’s specced as an int, and if int maxes out at 2**31-1 will overflow if nSleepTime is 2147484 or above, which is about 24 days.
HTTPRPCTimerInterface makes an HTTPRPCTimer, which populates a timeval with millis/1000 and (millis%1000)*1000. The latter should be fine, the former will overflow if time_t maxes out at 2**31-1 and millis is at least 2**31*1000, ie nSleepTime >= 2**31 which is already forbidden. The timeval is then passed to libevent’s evtimer_add(), and libevent then turns the time into an absolute value using timeradd() from <sys/time.h> which will overflow if the final time overflows time_t, ie the Y2038 bug. This will happen if nSleepTime is greater than about 631,300,000 which is about 2**(29.2), decreasing every second as 2038 gets closer.
Trying on Debian/i386, I can confirm that libevent gets an event with a negative timeout in that scenario, which I would expect to behave badle. I can’t actually get the event to trigger, however, so I’m not sure if this can be made to cause the wallet to actually be re-locked early. (Checking it’s re-locked at the correct time doesn’t seem worthwhile, since time_t will have wrapped by then, and who knows what will have broken…)
So if this needs to be improved further, maybe a check along the lines of nRelockTime < std::numeric_limits<time_t>::max() could be added.
But as is, I agree this makes things better than the status-quo, so ACK 134cdc7cee3da7c554e40ad947a9cdcbb3069f13 .
2**31-1, I get a libevent timeout at time -631,298,485 which then gets handled immediately, causing the wallet to be relocked. I don’t know why libevent handles that timeout immediately, but appears happy to leave a timeout corresponding to -2**32 or just after Y2038 pending indefinitely.
Also, regardless of anything, current PR seems like an improvement over status the quo.
OK, I agree. I was just a bit wary of setting a timer that we assume will never trigger at all, might as well not set it, but indeed this improves on the issue. Can be improved later. utACK 134cdc7
