test: update tool_wallet coverage for unexpected writes to wallet #28116

jonatack · 2023-07-20T18:56:34Z

Create file permission utilities in the functional test framework based on test: Add unit & functional test coverage for blockstore #27850 and test: tool wallet test coverage for unexpected writes to wallet #15687.
PR test: tool wallet test coverage for unexpected writes to wallet #15687 added test coverage in test/functional/tool_wallet.py to reproduce unexpected writes to the wallet file, as described in issue Feature request: bitcoin-wallet tool: don't modify files unless requested #15608:
- wallet tool info unexpectedly writes to the wallet file if the wallet file permissions are read/write
- wallet tool info raises if the wallet file permissions are read-only
Update these tests/docs for the following changes:
- addition of descriptor wallets
- CI migration away from Appveyor
- allow running tests that need to change file permissions on Linux using chattr
Instead of describing the error messages for legacy BDB wallets and descriptor SQLite wallets in a comment, check that the error message is thrown at runtime and document that it shouldn't throw.
Update feature_reindex_readonly.py to use the file permission utilities.
Improve wallet_multiwallet.py coverage, while updating to use the file permission utilities, which enables the test to work on Linux and provides logging and error handling. Also add a log entry for the test.

DrahtBot · 2023-07-20T18:56:37Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28660 (test: enable reindex readonly test on *BSD by pinheadmz)
#28550 (Covenant tools softfork by jamesob)
#10102 (Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

test/functional/tool_wallet.py

jonatack · 2023-07-21T01:38:12Z

Pulled in 2 commits from #27850 by @pinheadmz that this is now based on. Will rebase once that PR is merged.

fanquake · 2023-08-01T08:53:32Z

Will rebase once that PR is merged.

Mark as draft for now then, if based on another PR?

jonatack · 2023-08-27T17:54:12Z

Will rebase and update once #27850 is merged.

jonatack · 2023-10-01T01:31:29Z

Rebased and reworked following the merge of #27850 by extracting the chmod+chattr code to a test framework utility (also using the changes in #28545), then using that for the test files that need to change permissions.

jonatack · 2023-10-02T13:15:09Z

Rebased following the merge of #28545.

maflcko · 2023-10-03T18:26:41Z

test/functional/feature_reindex_readonly.py

-                    self.log.warning("Return early on Linux under root, because chattr failed.")
-                    self.log.warning("This should only happen due to missing capabilities in a container.")
-                    self.log.warning("Make sure to --cap-add LINUX_IMMUTABLE if you want to run this test.")
-                    return


removing this return will just re-introduce the bug that was fixed when adding it, no?

Thank you for having a look! Is this better now? git range-diff 04265ba d801f46 571eb17 (rebased for an unrelated CI failure now fixed in master).

An alternative would be to fail loudly by raising.

jonatack · 2023-10-10T00:09:30Z

Rebased for an unrelated CI failure normally now fixed in master.

PR 15687 added test coverage in test/functional/tool_wallet.py to reproduce unexpected writes to the wallet file, as described in issue 15608: - wallet tool info unexpectedly writes to the wallet file if the wallet file permissions are read/write - wallet tool info raises if the wallet file permissions are read-only Update these tests for two changes since PR 15687: - the CI migration away from Appveyor - the addition of descriptor wallets Instead of describing the error messages for legacy BDB wallets and descriptor SQLite wallets in a comment, check that the error message is thrown at runtime and document that it shouldn't throw.

which enables the test to work on linux and provides the logging and error handling contained in the utils. Also add a log entry for the test.

jonatack · 2023-10-16T19:16:50Z

Rebased and updated a line due to a python linter conflict following the merge of #28392.

@pinheadmz, @maflcko, @achow101, @ns-xvrn you might be interested in the changes here.

DrahtBot · 2023-10-24T09:59:57Z

🐙 This pull request conflicts with the target branch and needs rebase.

jonatack · 2023-10-25T20:13:57Z

test/functional/test_framework/util.py

+    if platform.system() == "Linux":
+        attr = "+i" if readonly else "-i"
+        try:
+            subprocess.run(["chattr", attr, filename], capture_output=readonly, check=readonly)


Note to address this feedback while updating and rebasing for the merge of that pull: #28660 (comment)

DrahtBot · 2024-01-23T00:00:01Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

jonatack · 2024-04-08T16:20:23Z

Closing to be able to re-open it.

DrahtBot added the Tests label Jul 20, 2023

maflcko reviewed Jul 20, 2023

View reviewed changes

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

jonatack force-pushed the 2023-07-tool-wallet-test-readonly-updates branch from d6c11d4 to 079337c Compare July 20, 2023 20:39

DrahtBot added the CI failed label Jul 20, 2023

jonatack force-pushed the 2023-07-tool-wallet-test-readonly-updates branch from 079337c to 4a5c004 Compare July 20, 2023 23:02

DrahtBot removed the CI failed label Jul 21, 2023

jonatack mentioned this pull request Jul 21, 2023

test: Add unit & functional test coverage for blockstore #27850

Merged

DrahtBot mentioned this pull request Jul 21, 2023

ci: Start with clean env #27976

Merged

jonatack marked this pull request as draft August 1, 2023 16:09

DrahtBot added the Needs rebase label Aug 24, 2023

jonatack force-pushed the 2023-07-tool-wallet-test-readonly-updates branch 2 times, most recently from 171a82e to 209142f Compare September 29, 2023 21:31

DrahtBot added the CI failed label Sep 29, 2023

jonatack force-pushed the 2023-07-tool-wallet-test-readonly-updates branch 2 times, most recently from 1a99a9e to 7534d69 Compare September 29, 2023 22:21

DrahtBot removed the Needs rebase label Sep 29, 2023

This was referenced Sep 30, 2023

test: Avoid test failure on Linux root without cap-add LINUX_IMMUTABLE #28545

Merged

Multiprocess bitcoin #10102

Draft

jonatack force-pushed the 2023-07-tool-wallet-test-readonly-updates branch 4 times, most recently from 6bba596 to 5e0445c Compare September 30, 2023 19:31

DrahtBot mentioned this pull request Sep 30, 2023

Covenant tools softfork #28550

Closed

DrahtBot removed the CI failed label Sep 30, 2023

jonatack force-pushed the 2023-07-tool-wallet-test-readonly-updates branch from 5e0445c to 3ac8635 Compare September 30, 2023 21:51

jonatack marked this pull request as ready for review October 1, 2023 01:04

DrahtBot added the Needs rebase label Oct 2, 2023

jonatack force-pushed the 2023-07-tool-wallet-test-readonly-updates branch from 3ac8635 to d801f46 Compare October 2, 2023 13:14

DrahtBot removed the Needs rebase label Oct 2, 2023

maflcko reviewed Oct 3, 2023

View reviewed changes

jonatack force-pushed the 2023-07-tool-wallet-test-readonly-updates branch 2 times, most recently from ccf2bfd to 571eb17 Compare October 9, 2023 22:27

DrahtBot added the CI failed label Oct 16, 2023

jonatack added 4 commits October 16, 2023 13:03

Add file permission utility functions to test framework

774a7c7

Update feature_reindex_readonly.py to use file permission utils

c3eb7c6

Update wallet_multiwallet.py to use file permission utils

6b20d56

which enables the test to work on linux and provides the logging and error handling contained in the utils. Also add a log entry for the test.

jonatack force-pushed the 2023-07-tool-wallet-test-readonly-updates branch from 571eb17 to 6b20d56 Compare October 16, 2023 19:11

DrahtBot removed the CI failed label Oct 16, 2023

jonatack mentioned this pull request Oct 16, 2023

test: enable reindex readonly test on *BSD #28660

Merged

DrahtBot added the Needs rebase label Oct 24, 2023

jonatack commented Oct 25, 2023

View reviewed changes

jonatack closed this Apr 8, 2024

bitcoin locked and limited conversation to collaborators Apr 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: update tool_wallet coverage for unexpected writes to wallet #28116

test: update tool_wallet coverage for unexpected writes to wallet #28116

Uh oh!

jonatack commented Jul 20, 2023 •

edited

Loading

Uh oh!

DrahtBot commented Jul 20, 2023 •

edited

Loading

Uh oh!

Uh oh!

jonatack commented Jul 21, 2023

Uh oh!

fanquake commented Aug 1, 2023

Uh oh!

jonatack commented Aug 27, 2023

Uh oh!

jonatack commented Oct 1, 2023

Uh oh!

jonatack commented Oct 2, 2023

Uh oh!

maflcko Oct 3, 2023

Uh oh!

jonatack Oct 4, 2023 •

edited

Loading

Uh oh!

jonatack commented Oct 10, 2023

Uh oh!

jonatack commented Oct 16, 2023

Uh oh!

DrahtBot commented Oct 24, 2023

Uh oh!

jonatack Oct 25, 2023

Uh oh!

DrahtBot commented Jan 23, 2024

Uh oh!

jonatack commented Apr 8, 2024

Uh oh!

Uh oh!

test: update tool_wallet coverage for unexpected writes to wallet #28116

test: update tool_wallet coverage for unexpected writes to wallet #28116

Uh oh!

Conversation

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

Uh oh!

Uh oh!

DrahtBot commented Jul 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

jonatack commented Jul 21, 2023

Uh oh!

fanquake commented Aug 1, 2023

Uh oh!

jonatack commented Aug 27, 2023

Uh oh!

jonatack commented Oct 1, 2023

Uh oh!

jonatack commented Oct 2, 2023

Uh oh!

maflcko Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

jonatack Oct 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack commented Oct 10, 2023

Uh oh!

jonatack commented Oct 16, 2023

Uh oh!

DrahtBot commented Oct 24, 2023

Uh oh!

jonatack Oct 25, 2023

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 23, 2024

Uh oh!

jonatack commented Apr 8, 2024

Uh oh!

Uh oh!

jonatack commented Jul 20, 2023 •

edited

Loading

DrahtBot commented Jul 20, 2023 •

edited

Loading

jonatack Oct 4, 2023 •

edited

Loading