In this change we allow the timeout for walletpassphrase to be set to MAX_SLEEP_TIME, if set as -1 then we then use the MAX_SLEEP_TIME amount
context from PR 28403
added release notes for RPC Wallet
Also tests were modified to use the max timeout amount using timeout = -1
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | kristapsk |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
Concept NACK. I don’t think this is a good idea for mainnet.
Any specific reason why? A user can currently input a time larger than MAX_SLEEP_TIME and it would be set to it, we are just defaulting to it if no arg is added.
Or are you more concerned that a user wouldn’t notice the optional arg and unexpectedly have a command with a long timeout running?
There was also the suggestion of 0 being used instead of omitting the arg to signal max timeout
Or are you more concerned that a user wouldn’t notice the optional arg and unexpectedly have a command with a long timeout running?
Yes
0 is currently accepted, it should have no use case that someone can rely on. (If someone wanted to immediately re-lock, it seems easier to do it synchronously with the walletlock RPC, than to wait for the async callback and poll the locked-state in a loop)
(If someone wanted to immediately re-lock, it seems easier to do it synchronously with the walletlock RPC, than to wait for the async callback and poll the locked-state in a loop)
I wonder if there’s an argument around a JSON-RPC batch that unlocks, runs some method, and re-locks… Probably wouldn’t work today, but might be worth adding?
Would be good to add test coverage and a release note.
now if the user sets 0 then we will use the MAX_SLEEP_TIME
That said, I think this breaks https://en.wikipedia.org/wiki/Principle_of_least_astonishment (should behave in a way that most users will expect it to behave), and in a less secure direction.
I wonder if there’s an argument around a JSON-RPC batch that unlocks, runs some method, and re-locks… Probably wouldn’t work today, but might be worth adding?
Agree that this would be nice, but seems unrelated to this pull, or would the batch feature require this RPC?
That said, I think this breaks https://en.wikipedia.org/wiki/Principle_of_least_astonishment (should behave in a way that most users will expect it to behave), and in a less secure direction.
The same is used for the rpc client/server timeout, so I think it seems more consistent to map 0 to max. Though, if it is too controversial, any negative value can be mapped to max instead?
forced pushed to 7deb571
added release notes and changed tests in this commit to use timeout=0
That said, I think this breaks https://en.wikipedia.org/wiki/Principle_of_least_astonishment (should behave in a way that most users will expect it to behave), and in a less secure direction.
The same is used for the rpc client/server timeout, so I think it seems more consistent to map
0to max. Though, if it is too controversial, any negative value can be mapped to max instead?
I would be open to using a negative value eg(-1) since any negative value rn throws an error. But I do think if we’re using 0 else where we should either continue doing the same or change it there aswell
0@@ -0,0 +1,6 @@
1+RPC Wallet
2+----------
3+
4+- The `walletpassphrase` call will now use MAX_SLEEP_TIME
MAX_SLEEP_TIME isn’t a user-facing value
0- The `walletpassphrase` call now uses the value of 100,000,000 seconds (~3 years)
0@@ -0,0 +1,6 @@
1+RPC Wallet
2+----------
3+
4+- The `walletpassphrase` call will now use MAX_SLEEP_TIME
5+ if the user sets 0 for the timeout arg.
0 if the user passes a `timeout` value of `0`. (#28454)
What would be a use case for encrypting a wallet and then setting a 3-year timeout? Maybe I’m missing something, but it seems like doing so should be explicit (as it is currently). I would expect users to pass as short a value as possible, i.e. a few seconds or minutes.
Setting a high timeout allows the user to run a wallet operation (of unknown duration) and then immediately re-lock it after it finished, see #28454 (comment).
For example, this can be done inside a python context:
0print("wallet is locked")
1with unlocked_wallet() as w:
2 w.getnewaddress()
3print("wallet is now locked again")
Where unlocked_wallet unlocks the wallet and returns w that locks when leaving the context.
@kevkevinpal I think the OP is out of date.
Sorry not sure what you mean here
timeout to be optional.
@kevkevinpal The description of this PR no longer describes what has been implemented because this PR was changed to not allow
timeoutto be optional.
Thanks, updated the PR description to match the commit message
I think that it would also be fine to only implement this for testnet and regtest. The wallet locking too soon has been a frequently occurring problem with wallet tests. If this is required for the batch feature however, then that is another case.
Added if block to check if non-mainnet (let me know if you want to exclude signet for any reason). Also updated the help doc to say that this would only happen in non mainnet. Updated commit message to include info about this only working in non mainnet in 9be4f3e
59@@ -59,6 +60,12 @@ RPCHelpMan walletpassphrase()
60 }
61 // Clamp timeout
62 constexpr int64_t MAX_SLEEP_TIME = 100000000; // larger values trigger a macos/libevent bug?
63+
64+ // Allow sleep time to be set to max time by using zero in non mainnet
65+ if (Params().GetChainType() != ChainType::MAIN && nSleepTime == 0) {
I am not really a fan of writing test-only code in real code. This is even more true for the RPC interface.
Either this feature makes sense, in which case it should be done for mainnet, or it does not, in which case it shouldn’t be done at all.
0 as a brittle alias for walletlock. Maybe not worth it to risk breaking that?
On a second thought, I wonder if someone uses
0as a brittle alias forwalletlock. Maybe not worth it to risk breaking that?
I can switch it from 0 to -1 since negative values are not being used currently for timeout
MAX_SLEEP_TIME in 0fc8db0
In this change we allow the timeout for walletpassphrase to be
set to MAX_SLEEP_TIME,
if set as -1 then we then use the MAX_SLEEP_TIME amount
context from PR 28403
added release notes for RPC Wallet
Also tests were modified to use the max timeout amount using timeout = -1