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
rSTAGING095183ecff21: Fix unlocked_until in getwalletinfo rpc
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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mar 1 2019, 01:01
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)

Added functional test

test/functional/wallet_encryption.py
17 ↗(On Diff #7709)

blame the linter

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 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 marked an inline comment as done.

Add mocktime to avoid time dependencies.

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.

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