Page MenuHomePhabricator

Fix unlocked_until in getwalletinfo rpc
ClosedPublic

Authored by florian on Mar 1 2019, 01:01.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABC095183ecff21: Fix unlocked_until in getwalletinfo rpc
Summary

Getwalletinfo returned an uninitialized value for unlocked_until before the first walletpassphrase call, even though the wallet was locked and it should return 0.

Test Plan

make check
./wallet_encryption.py

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

florian created this revision.Mar 1 2019, 01:01
Owners added a reviewer: Restricted Owners Package.Mar 1 2019, 01:01
Herald added a reviewer: Restricted Project. · View Herald TranscriptMar 1 2019, 01:01
Herald added a subscriber: schancel. · View Herald Transcript
Fabien accepted this revision.Mar 1 2019, 08:11
This revision is now accepted and ready to land.Mar 1 2019, 08:11
deadalnix requested changes to this revision.Mar 1 2019, 13:46

getwalletinfo on an encrypted wallet before and after applying the patch.

That sounds like something that a RPC test test should do.

This revision now requires changes to proceed.Mar 1 2019, 13:46
florian edited the test plan for this revision. (Show Details)Mar 14 2019, 19:50
florian updated this revision to Diff 7709.Mar 14 2019, 19:52
florian edited the test plan for this revision. (Show Details)

Added functional test

florian edited the test plan for this revision. (Show Details)Mar 14 2019, 19:53
florian added inline comments.Mar 14 2019, 19:54
test/functional/wallet_encryption.py
17 ↗(On Diff #7709)

blame the linter

Fabien added inline comments.Mar 15 2019, 14:53
test/functional/wallet_encryption.py
62 ↗(On Diff #7709)

I'm not a fan of the assertion being time dependent. If for some reason the dumpprivkey rpc lasts more than a second the test will fail.
I suggest you to move set the time.time() as close as possible from the walletpassphrase call, or maybe enclose the call with 2 time.time() so you get a range which is always valid.

deadalnix requested changes to this revision.Mar 22 2019, 14:00
deadalnix added inline comments.
test/functional/wallet_encryption.py
62 ↗(On Diff #7709)

Yes, this needs to be reworked. Timing dependent assertions are to be avoided, especially since there are time mocking capabilities.

It would also be worth looking at what core does and backport that if they already fixed it.

This revision now requires changes to proceed.Mar 22 2019, 14:00
florian marked 2 inline comments as done.Mar 26 2019, 18:16
florian added inline comments.
test/functional/wallet_encryption.py
62 ↗(On Diff #7709)

I agree and I fixed it, but please note this test already had time dependencies. For instance, that assert on line 46 breaks if the rpc call on 45 takes more than 2 seconds to return. This is the reason I chose a 2 seconds tolerance on the diff you reviewed.

62 ↗(On Diff #7709)

Thanks for the mocktime idea, I fixed it. Core just fixed the bug on wallet.h (https://github.com/bitcoin/bitcoin/pull/9993).

florian updated this revision to Diff 7835.Mar 26 2019, 18:19
florian marked an inline comment as done.

Add mocktime to avoid time dependencies.

florian added inline comments.Mar 26 2019, 18:31
test/functional/wallet_encryption.py
63 ↗(On Diff #7835)

Since we are using mocktime=1 (!=0 to avoid an ambiguity with a locked wallet i.e. unlocked_until=0) , the returned value will be 1+84600 _unless_ 84600s have passed, in which case it'll return 0. The timer in bitcoind is a simple timeout, so mocktime doesn't change its behavior.

This is the reason I chose not to test unlocked_until in line 46, as a 2 s delay would still break the test even though we have mocktime now.

deadalnix accepted this revision.Mar 27 2019, 14:41
This revision is now accepted and ready to land.Mar 27 2019, 14:41
This revision was automatically updated to reflect the committed changes.