Page MenuHomePhabricator

Fix unlocked_until in getwalletinfo rpc
Needs RevisionPublic

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

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
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
Branch
relocktime
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5222
Build 8507: Bitcoin ABC Teamcity Staging
Build 8506: arc lint + arc unit

Event Timeline

florian created this revision.Fri, Mar 1, 01:01
Owners added a reviewer: Restricted Owners Package.Fri, Mar 1, 01:01
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Mar 1, 01:01
Herald added a subscriber: schancel. · View Herald Transcript
Fabien accepted this revision.Fri, Mar 1, 08:11
This revision is now accepted and ready to land.Fri, Mar 1, 08:11
deadalnix requested changes to this revision.Fri, Mar 1, 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.Fri, Mar 1, 13:46
florian edited the test plan for this revision. (Show Details)Thu, Mar 14, 19:50
florian updated this revision to Diff 7709.Thu, Mar 14, 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)Thu, Mar 14, 19:53
florian added inline comments.Thu, Mar 14, 19:54
test/functional/wallet_encryption.py
17

blame the linter

Fabien added inline comments.Fri, Mar 15, 14:53
test/functional/wallet_encryption.py
62

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.Fri, Mar 22, 14:00
deadalnix added inline comments.
test/functional/wallet_encryption.py
62

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.Fri, Mar 22, 14:00