The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

Concept NACK. I don't think this is a good idea for mainnet.

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

@kristapsk I modified the PR to 1cc6b1c which now keeps it as a non optional param but now if the user sets 0 then we will use the MAX_SLEEP_TIME

Seems fine to do this. While 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 0 to 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

Thanks @jonatack I added those changes and pushed a7bc5db

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:

print("wallet is locked")
with unlocked_wallet() as w:
  w.getnewaddress()
print("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.

@MarcoFalke Thanks for the reply. It still seems safer to keep it explicit, so an application developer or cli user doesn't shoot themselves in the foot. As mentioned in #28454 (comment), this change would violate behaving in a way that most users will expect it to behave, and in a less secure direction. So I'm quite hesitant here.

@kevkevinpal I think the OP is out of date.

Sorry not sure what you mean here

@kevkevinpal
The description of this PR no longer describes what has been implemented because this PR was changed to not allow 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 timeout to 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.

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

On a second thought, I wonder if someone uses 0 as a brittle alias for walletlock. Maybe not worth it to risk breaking that?

On a second thought, I wonder if someone uses 0 as a brittle alias for walletlock. 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

Removed non mainnet if block and switched to using -1 instead of 0 to set time as MAX_SLEEP_TIME in 0fc8db0

rebased to 76b1c4c and since this PR 28617 has been merged only the test-framework will be modified so I am not sure if this change is needed anymore.

Thoughts @maflcko? I am happy to close this

rebased to 76b1c4c and since this PR 28617 has been merged only the test-framework will be modified so I am not sure if this change is needed anymore.

Thoughts @maflcko? I am happy to close this

closing this pull request