[rpcwallet] Clamp walletpassphrase value at 100M seconds #12905
Conversation
|
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? |
maflcko
left a comment
There was a problem hiding this comment.
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.
src/wallet/rpcwallet.cpp
Outdated
| "1. \"passphrase\" (string, required) The wallet passphrase\n" | ||
| "2. timeout (numeric, required) The time to keep the decryption key in seconds. Limited to at most 1073741824 (2^30) seconds.\n" | ||
| " Any value greater than 1073741824 seconds will be set to 1073741824 seconds.\n" | ||
| "2. timeout (numeric, required) The time to keep the decryption key in seconds; capped at 100000000.\n" |
There was a problem hiding this comment.
nit: Could add a comment that the constant is about 3 years.
src/wallet/rpcwallet.cpp
Outdated
| if (nSleepTime > (int64_t)1 << 30) { | ||
| nSleepTime = (int64_t)1 << 30; | ||
| // Clamp timeout | ||
| int64_t max_sleep_time = 100000000; // larger values trigger a macos/libevent bug? |
There was a problem hiding this comment.
I think we currently use constexpr (without static) and UPPER_CASE for compile time constants.
test/functional/wallet_encryption.py
Outdated
| # Check a time less than the limit | ||
| expected_time = int(time.time()) + (1 << 30) - 600 | ||
| self.nodes[0].walletpassphrase(passphrase2, (1 << 30) - 600) | ||
| max_value = 100000000 |
There was a problem hiding this comment.
nit: Constants should be UPPER_CASE
test/functional/wallet_encryption.py
Outdated
| expected_time = int(time.time()) + (1 << 30) - 1 | ||
| self.nodes[0].walletpassphrase(passphrase2, (1 << 33)) | ||
| expected_time = int(time.time()) + max_value - 1 | ||
| self.nodes[0].walletpassphrase(passphrase2, max_value + 1) |
There was a problem hiding this comment.
Might want to be off by more than 1, since we have a "buffer" of 5? Maybe try off by 1000 or something?
|
Thanks @MarcoFalke, nits fixed in latest commit. |
|
utACK, also add release note? |
|
Is the fact that wallets get locked after 3+ years really observable? |
|
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. utACK - don't think this requires specific mention in the release notes. |
|
Tested ACK 93e1798 (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 |
|
Tested ACK 93e1798. |
|
utACK 93e1798 |
|
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits |
Larger values seem to trigger a bug on macos+libevent (resulting in the rpc server stopping).
Change suggested by Marco Falke.
93e1798 to
2b2b96c
Compare
|
|
||
| #include <univalue.h> | ||
|
|
||
| #include <functional> |
There was a problem hiding this comment.
Why is this include needed?
There was a problem hiding this comment.
According to http://en.cppreference.com/w/cpp/utility/functional/bind, std::bind is "[d]efined in header <functional>".
|
reACK 2b2b96c |
2b2b96c Use std::bind instead of boost::bind to re-lock the wallet (Suhas Daftuar) 662d19f [rpcwallet] Clamp walletpassphrase value at 100M seconds (Suhas Daftuar) Pull request description: Larger values seem to trigger a bug on macos+libevent (resulting in the rpc server stopping). Tree-SHA512: 890f3b641f6c586e2f8f629a9d23bca6ceb8b237b285561aad488cb7adf941a21177d3129d0c2b8293c0a673cd8e401957dbe2b6b3b7c8c4e991bb411d260102
Larger values seem to trigger a bug on macos+libevent (resulting in the rpc server stopping). Github-Pull: bitcoin#12905 Rebased-From: 662d19f
Larger values seem to trigger a bug on macos+libevent (resulting in the rpc server stopping). Github-Pull: bitcoin#12905 Rebased-From: 662d19f
Larger values seem to trigger a bug on macos+libevent (resulting in the rpc server stopping). Github-Pull: bitcoin#12905 Rebased-From: 662d19f
… seconds 2b2b96c Use std::bind instead of boost::bind to re-lock the wallet (Suhas Daftuar) 662d19f [rpcwallet] Clamp walletpassphrase value at 100M seconds (Suhas Daftuar) Pull request description: Larger values seem to trigger a bug on macos+libevent (resulting in the rpc server stopping). Tree-SHA512: 890f3b641f6c586e2f8f629a9d23bca6ceb8b237b285561aad488cb7adf941a21177d3129d0c2b8293c0a673cd8e401957dbe2b6b3b7c8c4e991bb411d260102
Larger values seem to trigger a bug on macos+libevent (resulting in the rpc server stopping).