test: tool wallet test coverage for unexpected writes to wallet #15687

jonatack · 2019-03-28T17:22:26Z

This pull request adds test coverage in test/functional/tool_wallet.py to reproduce unexpected writes to the wallet as described in #15608 and serve as a benchmark for fixing the issue:

Wallet tool info unexpectedly writes to the wallet file if the wallet file permissions are read/write.
Wallet tool info raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only.

Goals:

Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix. Add debug-level logging for sanity checking and commented-out assertions to be uncommented when fixing the issue. Add the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue.
Add info log messages as there are currently none in the test file.
Split the tests out to separate functions as per review feedback.

Thanks to Marco Falke for pointing me in the right direction.

test/functional/tool_wallet.py

DrahtBot · 2019-03-28T18:48:06Z

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

Conflicts

No conflicts as of last run.

test/functional/tool_wallet.py

Tests were added with bitcoin/bitcoin#15687 so now update todo to resolving the issue.

jonatack · 2019-04-05T11:19:20Z

Lowered the buffer size for the shasum function to 16Kb and hoisted that value to a constant. Rebased.

This should be ready for review.

test/functional/tool_wallet.py

laanwj · 2019-06-17T13:45:39Z

Looks good to me
ACK 96e69a2

maflcko

some style-nits

test/functional/tool_wallet.py

jonatack · 2019-07-02T20:19:57Z

Linter is complaining about unused code because it is in the commented-out tests and Appveyor seems to keep file perms at 666. Reworking.

jonatack · 2019-07-08T10:10:33Z

Thanks for the review @laanwj and @MarcoFalke. I've pushed rebased commits that address your feedback. Are you able to rereview/retest?

and update code comments as per Python PEP 8 style guide.

as per Marco Falke review suggestion.

This commit adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in bitcoin#15608: - wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write. - wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only. 1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix in the form of commented-out assertions to be uncommented when testing/fixing. 2. Provisionally extend the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue. 3. Add some logging for sanity checking. ------ Changes after rebase: 5. Make wallet_path an instance method instead of a function in tool_wallet.py as per Marco Falke review suggestion. 6. Assert wallet permissions instead of logging them in tool_wallet.py. This ran into an issue with Appveyor keeping permissions at 666 so allowed for 666 as a workaround. 7. Change the added logging from info to debug level. 8. More helpful assertions order in tool_wallet.py#assert_tool_output. This change makes #assert_tool_output raise "Error loading wallet.dat. Is wallet being used by another process?" rather than a less-helpful message when debugging the read-only wallet permissions issue.

laanwj · 2019-07-08T12:56:46Z

code review ACK 7195fa7

…o wallet 7195fa7 test: Tool wallet test coverage for unexpected writes to wallet (Jon Atack) 3bf2b3a test: Split tool_wallet.py test into subtests (Jon Atack) 1eb13f0 test: Add log messages to test/functional/tool_wallet.py (Jon Atack) Pull request description: This pull request adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in #15608 and serve as a benchmark for fixing the issue: - Wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write. - Wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only. Goals: 1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix. Add debug-level logging for sanity checking and commented-out assertions to be uncommented when fixing the issue. Add the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue. 2. Add info log messages as there are currently none in the test file. 3. Split the tests out to separate functions as per review feedback. Thanks to Marco Falke for pointing me in the right direction. ACKs for top commit: laanwj: code review ACK 7195fa7 Tree-SHA512: 16a41cce989c8f819cf5b02c6cf8ea84653ede2738fb402f6c36cf4dc075b424dff3e2c73a1cfa1ec9c75f614675baecc71e588845a2596db06ba0957db2df7b

jonatack · 2019-07-08T16:29:58Z

The lone Travis failure appears to be an unrelated wait_until timeout > 60 sec in feature_block.py.

…rites to wallet 7195fa7 test: Tool wallet test coverage for unexpected writes to wallet (Jon Atack) 3bf2b3a test: Split tool_wallet.py test into subtests (Jon Atack) 1eb13f0 test: Add log messages to test/functional/tool_wallet.py (Jon Atack) Pull request description: This pull request adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in bitcoin#15608 and serve as a benchmark for fixing the issue: - Wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write. - Wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only. Goals: 1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix. Add debug-level logging for sanity checking and commented-out assertions to be uncommented when fixing the issue. Add the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue. 2. Add info log messages as there are currently none in the test file. 3. Split the tests out to separate functions as per review feedback. Thanks to Marco Falke for pointing me in the right direction. ACKs for top commit: laanwj: code review ACK 7195fa7 Tree-SHA512: 16a41cce989c8f819cf5b02c6cf8ea84653ede2738fb402f6c36cf4dc075b424dff3e2c73a1cfa1ec9c75f614675baecc71e588845a2596db06ba0957db2df7b

Summary: * test: Add log messages to test/functional/tool_wallet.py and update code comments as per Python PEP 8 style guide. * test: Split tool_wallet.py test into subtests as per Marco Falke review suggestion. * test: Tool wallet test coverage for unexpected writes to wallet This commit adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in bitcoin/bitcoin#15608: - wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write. - wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only. 1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix in the form of commented-out assertions to be uncommented when testing/fixing. 2. Provisionally extend the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue. 3. Add some logging for sanity checking. ------ Changes after rebase: 5. Make wallet_path an instance method instead of a function in tool_wallet.py as per Marco Falke review suggestion. 6. Assert wallet permissions instead of logging them in tool_wallet.py. This ran into an issue with Appveyor keeping permissions at 666 so allowed for 666 as a workaround. 7. Change the added logging from info to debug level. 8. More helpful assertions order in tool_wallet.py#assert_tool_output. This change makes #assert_tool_output raise "Error loading wallet.dat. Is wallet being used by another process?" rather than a less-helpful message when debugging the read-only wallet permissions issue. This is a backport of Core [[bitcoin/bitcoin#15687 | PR15687]] Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D7706

…rites to wallet 7195fa7 test: Tool wallet test coverage for unexpected writes to wallet (Jon Atack) 3bf2b3a test: Split tool_wallet.py test into subtests (Jon Atack) 1eb13f0 test: Add log messages to test/functional/tool_wallet.py (Jon Atack) Pull request description: This pull request adds test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet as described in bitcoin#15608 and serve as a benchmark for fixing the issue: - Wallet tool `info` unexpectedly writes to the wallet file if the wallet file permissions are read/write. - Wallet tool `info` raises with "Error loading . Is wallet being used by another process?" if the wallet file permissions are read-only. Goals: 1. Reproduce the reported issue, define the current unexpected behavior, and add test coverage to guide a future fix. Add debug-level logging for sanity checking and commented-out assertions to be uncommented when fixing the issue. Add the same coverage to the wallet tool create test and the getwalletinfo test as regression tests while fixing the issue. 2. Add info log messages as there are currently none in the test file. 3. Split the tests out to separate functions as per review feedback. Thanks to Marco Falke for pointing me in the right direction. ACKs for top commit: laanwj: code review ACK 7195fa7 Tree-SHA512: 16a41cce989c8f819cf5b02c6cf8ea84653ede2738fb402f6c36cf4dc075b424dff3e2c73a1cfa1ec9c75f614675baecc71e588845a2596db06ba0957db2df7b

jonatack commented Mar 28, 2019

View reviewed changes

test/functional/tool_wallet.py Outdated Show resolved Hide resolved

maflcko reviewed Mar 28, 2019

View reviewed changes

test/functional/tool_wallet.py Outdated Show resolved Hide resolved

maflcko reviewed Mar 28, 2019

View reviewed changes

test/functional/tool_wallet.py Outdated Show resolved Hide resolved

DrahtBot added the Tests label Mar 28, 2019

jonatack force-pushed the tool-wallet-tests-for-unexpected-writes-to-wallet-file branch from 6d9b033 to 7abd455 Compare April 3, 2019 16:28

jonatack added a commit to jonatack/bitcoin-development that referenced this pull request Apr 4, 2019

Update completed todo item after submitting PR

a0dc1ab

Tests were added with bitcoin/bitcoin#15687 so now update todo to resolving the issue.

jonatack force-pushed the tool-wallet-tests-for-unexpected-writes-to-wallet-file branch from 7abd455 to 96e69a2 Compare April 5, 2019 10:41

laanwj reviewed Jun 17, 2019

View reviewed changes

test/functional/tool_wallet.py Show resolved Hide resolved

maflcko reviewed Jun 24, 2019

View reviewed changes

jonatack mentioned this pull request Jun 29, 2019

test: Make test cases separate functions #16293

Merged

jonatack force-pushed the tool-wallet-tests-for-unexpected-writes-to-wallet-file branch from 96e69a2 to 1b286d3 Compare July 2, 2019 19:40

jonatack force-pushed the tool-wallet-tests-for-unexpected-writes-to-wallet-file branch 3 times, most recently from d336321 to 497af0c Compare July 3, 2019 10:26

jonatack added 3 commits July 8, 2019 12:33

test: Add log messages to test/functional/tool_wallet.py

1eb13f0

and update code comments as per Python PEP 8 style guide.

test: Split tool_wallet.py test into subtests

3bf2b3a

as per Marco Falke review suggestion.

jonatack force-pushed the tool-wallet-tests-for-unexpected-writes-to-wallet-file branch from 497af0c to 7195fa7 Compare July 8, 2019 11:24

laanwj merged commit 7195fa7 into bitcoin:master Jul 8, 2019

jonatack deleted the tool-wallet-tests-for-unexpected-writes-to-wallet-file branch July 8, 2019 16:24

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: tool wallet test coverage for unexpected writes to wallet #15687

test: tool wallet test coverage for unexpected writes to wallet #15687

Uh oh!

jonatack commented Mar 28, 2019 •

edited

Loading

Uh oh!

Uh oh!

DrahtBot commented Mar 28, 2019 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

jonatack commented Apr 5, 2019

Uh oh!

Uh oh!

laanwj commented Jun 17, 2019

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jul 2, 2019 •

edited

Loading

Uh oh!

jonatack commented Jul 8, 2019

Uh oh!

laanwj commented Jul 8, 2019

Uh oh!

jonatack commented Jul 8, 2019

Uh oh!

Uh oh!

test: tool wallet test coverage for unexpected writes to wallet #15687

test: tool wallet test coverage for unexpected writes to wallet #15687

Uh oh!

Conversation

jonatack commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Mar 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

jonatack commented Apr 5, 2019

Uh oh!

Uh oh!

laanwj commented Jun 17, 2019

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jul 8, 2019

Uh oh!

laanwj commented Jul 8, 2019

Uh oh!

jonatack commented Jul 8, 2019

Uh oh!

Uh oh!

jonatack commented Mar 28, 2019 •

edited

Loading

DrahtBot commented Mar 28, 2019 •

edited

Loading

jonatack commented Jul 2, 2019 •

edited

Loading