test: update tool_wallet coverage for unexpected writes to wallet

jonatack commented at 6:56 pm on July 20, 2023: contributor

Create file permission utilities in the functional test framework based on #27850 and #15687.
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/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 commented at 6:56 pm on July 20, 2023: contributor

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.

DrahtBot added the label Tests on Jul 20, 2023

in test/functional/tool_wallet.py:217 in d6c11d4d10 outdated

210@@ -211,13 +211,19 @@ def test_tool_wallet_info(self):
211         self.log.info('Calling wallet tool info, testing output')
212         #
213         # TODO: Wallet tool info should work with wallet file permissions set to
214-        # read-only without raising:
215-        # "Error loading wallet.dat. Is wallet being used by another process?"
216+        # read-only without raising as follows:
217+        #
218+        # Legacy wallets:
219+        # "Error loading . Is wallet being used by another process?"

maflcko commented at 7:04 pm on July 20, 2023:

Instead of putting the error message in a static comment, it may be better to check that the error message is thrown at runtime, and only keep the comment that it shouldn’t throw.

jonatack commented at 8:40 pm on July 20, 2023:

Thanks for reviewing! Done.

jonatack force-pushed on Jul 20, 2023

DrahtBot added the label CI failed on Jul 20, 2023

jonatack force-pushed on Jul 20, 2023

DrahtBot removed the label CI failed on Jul 21, 2023

jonatack commented at 1:38 am on July 21, 2023: contributor

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

fanquake commented at 8:53 am on August 1, 2023: member

Will rebase once that PR is merged.

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

jonatack marked this as a draft on Aug 1, 2023

DrahtBot added the label Needs rebase on Aug 24, 2023

jonatack commented at 5:54 pm on August 27, 2023: contributor

Will rebase and update once #27850 is merged.

jonatack force-pushed on Sep 29, 2023

DrahtBot added the label CI failed on Sep 29, 2023

jonatack force-pushed on Sep 29, 2023

DrahtBot removed the label Needs rebase on Sep 29, 2023

jonatack force-pushed on Sep 30, 2023

DrahtBot removed the label CI failed on Sep 30, 2023

jonatack force-pushed on Sep 30, 2023

jonatack marked this as ready for review on Oct 1, 2023

jonatack commented at 1:31 am on October 1, 2023: contributor

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.

DrahtBot added the label Needs rebase on Oct 2, 2023

jonatack force-pushed on Oct 2, 2023

jonatack commented at 1:15 pm on October 2, 2023: contributor

Rebased following the merge of #28545.

DrahtBot removed the label Needs rebase on Oct 2, 2023

in test/functional/feature_reindex_readonly.py:53 in d801f465a9 outdated

48-                    self.log.warning(f"stderr: {e.stderr}")
49-                if os.getuid() == 0:
50-                    self.log.warning("Return early on Linux under root, because chattr failed.")
51-                    self.log.warning("This should only happen due to missing capabilities in a container.")
52-                    self.log.warning("Make sure to --cap-add LINUX_IMMUTABLE if you want to run this test.")
53-                    return

maflcko commented at 6:26 pm on October 3, 2023:

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

jonatack commented at 12:21 pm on October 4, 2023:

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 force-pushed on Oct 4, 2023

jonatack force-pushed on Oct 9, 2023

jonatack commented at 0:09 am on October 10, 2023: contributor

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

DrahtBot added the label CI failed on Oct 16, 2023

Add file permission utility functions to test framework 774a7c7e19

Update tool_wallet tests for unexpected writes to wallet

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.

3ceb3131a9

Update feature_reindex_readonly.py to use file permission utils c3eb7c6cd3

Update wallet_multiwallet.py to use file permission utils

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.

6b20d56a3a

jonatack force-pushed on Oct 16, 2023

jonatack commented at 7:16 pm on October 16, 2023: contributor

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 removed the label CI failed on Oct 16, 2023

DrahtBot added the label Needs rebase on Oct 24, 2023

DrahtBot commented at 9:59 am on October 24, 2023: contributor

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

in test/functional/test_framework/util.py:489 in 6b20d56a3a

484+    if readonly:
485+        set_chmod(filename, mode)
486+    if platform.system() == "Linux":
487+        attr = "+i" if readonly else "-i"
488+        try:
489+            subprocess.run(["chattr", attr, filename], capture_output=readonly, check=readonly)

jonatack commented at 8:13 pm on October 25, 2023:

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

DrahtBot commented at 0:00 am on January 23, 2024: contributor

⌛ 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 commented at 4:20 pm on April 8, 2024: contributor

Closing to be able to re-open it.

jonatack closed this on Apr 8, 2024

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

Code Coverage

Reviews

Conflicts