[rpcwallet] Clamp walletpassphrase value at 100M seconds #12905

sdaftuar commented at 6:28 PM on April 6, 2018: member

Larger values seem to trigger a bug on macos+libevent (resulting in the rpc server stopping).

sdaftuar commented at 6:29 PM on April 6, 2018: member

This is a quick fix for #12375. Not sure if further reducing this value as I've proposed here is acceptable, or if we should be trying to solve this problem at a different level?

in src/wallet/rpcwallet.cpp:2352 in 45b645dbea outdated

2348 | @@ -2349,8 +2349,7 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
2349 |              "This is needed prior to performing transactions related to private keys such as sending bitcoins\n"
2350 |              "\nArguments:\n"
2351 |              "1. \"passphrase\"     (string, required) The wallet passphrase\n"
2352 | -            "2. timeout            (numeric, required) The time to keep the decryption key in seconds. Limited to at most 1073741824 (2^30) seconds.\n"
2353 | -            "                                          Any value greater than 1073741824 seconds will be set to 1073741824 seconds.\n"
2354 | +            "2. timeout            (numeric, required) The time to keep the decryption key in seconds; capped at 100000000.\n"

MarcoFalke commented at 6:43 PM on April 6, 2018:

nit: Could add a comment that the constant is about 3 years.

in src/wallet/rpcwallet.cpp:2386 in 45b645dbea outdated

2381 | @@ -2383,9 +2382,10 @@ UniValue walletpassphrase(const JSONRPCRequest& request)
2382 |      if (nSleepTime < 0) {
2383 |          throw JSONRPCError(RPC_INVALID_PARAMETER, "Timeout cannot be negative.");
2384 |      }
2385 | -    // Clamp timeout to 2^30 seconds
2386 | -    if (nSleepTime > (int64_t)1 << 30) {
2387 | -        nSleepTime = (int64_t)1 << 30;
2388 | +    // Clamp timeout
2389 | +    int64_t max_sleep_time = 100000000; // larger values trigger a macos/libevent bug?

MarcoFalke commented at 6:44 PM on April 6, 2018:

I think we currently use constexpr (without static) and UPPER_CASE for compile time constants.

in test/functional/wallet_encryption.py:67 in 45b645dbea outdated

  63 | @@ -64,14 +64,15 @@ def run_test(self):
  64 |          assert_raises_rpc_error(-8, "Timeout cannot be negative.", self.nodes[0].walletpassphrase, passphrase2, -10)
  65 |          # Check the timeout
  66 |          # Check a time less than the limit
  67 | -        expected_time = int(time.time()) + (1 << 30) - 600
  68 | -        self.nodes[0].walletpassphrase(passphrase2, (1 << 30) - 600)
  69 | +        max_value = 100000000

MarcoFalke commented at 6:44 PM on April 6, 2018:

nit: Constants should be UPPER_CASE

in test/functional/wallet_encryption.py:75 in 45b645dbea outdated

  74 |          assert_greater_than(expected_time + 5, actual_time) # 5 second buffer
  75 |          # Check a time greater than the limit
  76 | -        expected_time = int(time.time()) + (1 << 30) - 1
  77 | -        self.nodes[0].walletpassphrase(passphrase2, (1 << 33))
  78 | +        expected_time = int(time.time()) + max_value - 1
  79 | +        self.nodes[0].walletpassphrase(passphrase2, max_value + 1)

MarcoFalke commented at 6:47 PM on April 6, 2018:

Might want to be off by more than 1, since we have a "buffer" of 5? Maybe try off by 1000 or something?

MarcoFalke commented at 6:49 PM on April 6, 2018: member

utACK, just a ton of style nits.

Also, since you are touching the walletpassphrase code anyway, you might want to change the boost::bind into std::bind to preempt future pull request from doing that.

sdaftuar commented at 7:00 PM on April 6, 2018: member

Thanks @MarcoFalke, nits fixed in latest commit.

promag commented at 11:29 PM on April 6, 2018: member

utACK, also add release note?

sipa commented at 11:35 PM on April 6, 2018: member

Is the fact that wallets get locked after 3+ years really observable?

fanquake added the label Wallet on Apr 6, 2018

fanquake added the label RPC/REST/ZMQ on Apr 6, 2018

MarcoFalke added the label Needs backport on Apr 7, 2018

MarcoFalke removed the label Needs backport on Apr 7, 2018

laanwj commented at 8:38 AM on April 8, 2018: member

From a principled point of view I'd still prefer to never unlock if a value higher than defined threshold is given, instead of clamping. Because that's what users intend if they give a huge number like this. They don't expect the wallet to ever re-lock. But I also agree with @sipa that no one is going to notice this in practice.

utACK - don't think this requires specific mention in the release notes.

PierreRochard commented at 9:14 AM on April 8, 2018: contributor

Tested ACK 93e179853e89e14cf248a7e36aa51ed611727ed8 (on macOS).

My view is that we should throw an error if the value provided is > MAX_SLEEP_TIME and we should have an explicit boolean never_timeout parameter. But in terms of fixing this macOS-related bug the PR lgtm.

promag commented at 9:41 AM on April 8, 2018: member

Tested ACK 93e1798.

jonasschnelli commented at 1:50 PM on April 8, 2018: contributor

utACK 93e179853e89e14cf248a7e36aa51ed611727ed8

MarcoFalke commented at 2:14 PM on April 8, 2018: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

[rpcwallet] Clamp walletpassphrase value at 100M seconds

Larger values seem to trigger a bug on macos+libevent (resulting in the
rpc server stopping).

662d19ff72

Use std::bind instead of boost::bind to re-lock the wallet

Change suggested by Marco Falke.

2b2b96cd45

sdaftuar force-pushed on Apr 8, 2018

sdaftuar commented at 2:46 PM on April 8, 2018: member

Squashed https://github.com/bitcoin/bitcoin/commit/93e179853e89e14cf248a7e36aa51ed611727ed8 -> https://github.com/bitcoin/bitcoin/pull/12905/commits/2b2b96cd452bd08537310ce78bee9cbf6f43c10e

in src/wallet/rpcwallet.cpp:40 in 2b2b96cd45

  36 | @@ -37,6 +37,8 @@
  37 |  
  38 |  #include <univalue.h>
  39 |  
  40 | +#include <functional>

PierreRochard commented at 3:08 PM on April 8, 2018:

Why is this include needed?

MarcoFalke commented at 4:31 PM on April 8, 2018:

According to http://en.cppreference.com/w/cpp/utility/functional/bind, std::bind is "[d]efined in header <functional>".

MarcoFalke commented at 4:30 PM on April 8, 2018: member

utACK 2b2b96cd45 (Checked that it is the same code as the previous 93e179853e)

PierreRochard commented at 4:37 PM on April 8, 2018: contributor

reACK 2b2b96cd452bd08537310ce78bee9cbf6f43c10e

MarcoFalke merged this on Apr 8, 2018

MarcoFalke closed this on Apr 8, 2018

MarcoFalke referenced this in commit bd42b85e8b on Apr 8, 2018

MarcoFalke referenced this in commit 7e6b9bd7ac on Apr 20, 2018

MarcoFalke referenced this in commit 02ed4f62cb on May 24, 2018

MarcoFalke referenced this in commit 1fd36a4fa5 on May 24, 2018

MarcoFalke referenced this in commit 1627d30dc8 on May 29, 2018

MarcoFalke referenced this in commit cfc6f7413b on Jul 12, 2018

HashUnlimited referenced this in commit 581acdef12 on Jan 11, 2019

UdjinM6 referenced this in commit 45a8c39cde on Feb 29, 2020

PastaPastaPasta referenced this in commit b24951bc78 on Mar 4, 2020

ckti referenced this in commit a1cf301362 on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021