Fix unlocked_until in getwalletinfo rpc
ClosedPublic
Actions

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

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 reviewer: deadalnix. · View Herald Transcript

Herald added a subscriber: schancel. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5113: Diff 7548.Mar 1 2019, 01:14

Fabien accepted this revision.Mar 1 2019, 08:11

This revision is now accepted and ready to land.Mar 1 2019, 08:11

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

Added functional test

florian edited the test plan for this revision. (Show Details)Mar 14 2019, 19:53

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

florian added inline comments.Mar 14 2019, 19:54

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

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5222: Diff 7709.Mar 14 2019, 20:07

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).

Add mocktime to avoid time dependencies.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

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.