qa: Fix wallet_multiwallet.py
#31410
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31410. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
c37c6b5 to
b58c8fe
Compare
b58c8fe to
0a1a7f7
Compare
0a1a7f7 to
c012743
Compare
maflcko
left a comment
There was a problem hiding this comment.
review ACK c012743 🎑
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK c0127439597ee9d09b8e117be7d6226a68b45721 🎑
tf/up7OMkfp/8WkQfPI/JfkrY0jnqtfRAg2T09eD2yTw/BRFey4pUZPMlJu+Y93FYLisAf28Ld2wEeMnGisIBQ==
|
Friendly ping a Python connoisseur @stickies-v :) |
| if platform.system() != 'Windows' and os.geteuid() != 0: | ||
| os.mkdir(wallet_dir('no_access')) | ||
| os.chmod(wallet_dir('no_access'), 0) | ||
| try: | ||
| with self.nodes[0].assert_debug_log(expected_msgs=['Error scanning']): | ||
| walletlist = self.nodes[0].listwalletdir()['wallets'] | ||
| finally: | ||
| # Need to ensure access is restored for cleanup | ||
| os.chmod(wallet_dir('no_access'), stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR) | ||
| assert_equal(sorted(map(lambda w: w['name'], walletlist)), sorted(in_wallet_dir)) |
There was a problem hiding this comment.
Making this check conditional on platform is undocumented in the commit message, and seems orthogonal to the goal of testing errors individually. I think it would be best to split this out in a separate commit and document it?
There was a problem hiding this comment.
I don't think these changes can be meaningfully separated. Applying either change independently would break the test or render it incomplete.
The commit message has been updated though.
| assert_equal(sorted(map(lambda w: w['name'], walletlist)), sorted(in_wallet_dir)) | ||
| # 2. "Too many levels of symbolic links" error. | ||
| # This test cannot be conducted robustly on Windows | ||
| # because it depends on the build toolchain: |
There was a problem hiding this comment.
In c012743:
because it depends on the build toolchain:
Even if this is the case, is the best option to just to do nothing? Shouldn't we atleast test the known behaviour for our release build, so we'd catch if this changes (is fixed?) in future? Otherwise my understanding is if this changes again, it's just going to be hidden/ignored.
There was a problem hiding this comment.
I guess the check could be limited to exclude the msvc build?
There was a problem hiding this comment.
Shouldn't we atleast test the known behaviour for our release build...?
As documented in the code comment:
A cross-compiled bitcoind.exe parses self_walletdat_symlink without errors.
Therefore, the release build won't trigger the "Error scanning" message, and this test would fail on the master branch.
There was a problem hiding this comment.
I guess the check could be limited to exclude the msvc build?
That would make the most sense to me.
There was a problem hiding this comment.
Therefore, the release build won't trigger the "Error scanning" message, and this test would fail on the master branch.
Sure, so you can check that that is happening when testing on Windows.
There was a problem hiding this comment.
I guess the check could be limited to exclude the msvc build?
Unfortunately, I can't find a simple way to detect whether binaries were built with MSVC without adding extra complexity to the Python code.
There was a problem hiding this comment.
I'd be fine with a hacky check that only runs on windows and checks for a magic msvc binary string in open(exe_path, "rb").read(), but anything is fine and this is not a blocker from my side.
c012743 to
e717fb5
Compare
|
Only change is the commit msg re-ACK e717fb5 🍣 Show signatureSignature: |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
UPD. Further investigation is needed. |
There was a problem hiding this comment.
Code review e717fb5
Guix cross-build failure
Attempted to run Guix build e717fb5 cross-built for Windows, but it fails already on test "0" / line 133:
bitcoin/test/functional/wallet_multiwallet.py
Lines 129 to 133 in e717fb5
Log
$ BITCOIND=/c/Users/hodlinator/Downloads/bitcoind-e717fb5a429d.exe build/test/functional/wallet_multiwallet.py
2025-02-08T22:43:58.813000Z TestFramework (INFO): PRNG seed is: 7271806868081798699
2025-02-08T22:43:58.862000Z TestFramework (INFO): Initializing test directory C:\Users\HODLIN~1\AppData\Local\Temp\bitcoin_func_test_fj5a7zvd
2025-02-08T22:44:00.455000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "C:\Users\hodlinator\bitcoin\build\test\functional\test_framework\test_framework.py", line 135, in main
self.run_test()
~~~~~~~~~~~~~^^
File "C:\Users\hodlinator\bitcoin\build\test\functional\wallet_multiwallet.py", line 133, in run_test
assert_equal(sorted(map(lambda w: w['name'], walletlist)), sorted(in_wallet_dir))
~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "C:\Users\hodlinator\bitcoin\build\test\functional\test_framework\util.py", line 77, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(['default_wallet', 'recursive_dir_symlink\\wallets\\default_wallet',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\default_wallet',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\default_wallet',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\default_wallet',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\sub\\w5',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w1',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w2',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w3',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w7',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w7_symlink',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\sub\\w5',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w1',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w2',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w3',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w7',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w7_symlink',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\sub\\w5',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w1',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w2',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w3',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w7',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w7_symlink',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\sub\\w5',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w1',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w2',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w3',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w7',
'recursive_dir_symlink\\wallets\\recursive_dir_symlink\\wallets\\w7_symlink',
'recursive_dir_symlink\\wallets\\sub\\w5',
'recursive_dir_symlink\\wallets\\w', 'recursive_dir_symlink\\wallets\\w1', 'recursive_dir_symlink\\wallets\\w2',
'recursive_dir_symlink\\wallets\\w3', 'recursive_dir_symlink\\wallets\\w7', 'recursive_dir_symlink\\wallets\\w7_symlink',
'sub\\w5', 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink']
== ['default_wallet', 'sub\\w5', 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink'])
2025-02-08T22:44:00.507000Z TestFramework (INFO): Stopping nodes
Should that not be working?
C++ behavior difference when iterating directories?
I managed to come up with 584e2fc, which adds extra error checking that appears to be missing from the wallet/db.cpp code. (Edit: later merged as #32736).
After adding that, I was able to re-implement permission checking for Windows in 0f67bd65ec3536ba618e6ae79931a68a2517f375 (for native builds, cross compiled seem to fail earlier as noted above). It does use ACLs since I hadn't seen #31410 (comment) until I was pretty much done with the implementation (didn't want to bias my review). Might be something for a follow-up PR.
Windows Developer Mode parenthesis
First thing I attempted on Windows was to run the wallet_multiwallet.py test on the current parent of this PR (ebe4cac):
$ BITCOIND=/c/Users/hodlinator/bitcoin/build/src/Release/bitcoind.exe build/test/functional/wallet_multiwallet.py
2025-02-08T12:32:02.546000Z TestFramework (INFO): PRNG seed is: 1953116584645958224
2025-02-08T12:32:02.566000Z TestFramework (INFO): Initializing test directory C:\Users\HODLIN~1\AppData\Local\Temp\bitcoin_func_test_tqrp64pv
2025-02-08T12:32:03.168000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
File "C:\Users\hodlinator\bitcoin\build\test\functional\test_framework\test_framework.py", line 135, in main
self.run_test()
~~~~~~~~~~~~~^^
File "C:\Users\hodlinator\bitcoin\build\test\functional\wallet_multiwallet.py", line 85, in run_test
os.symlink('w7', wallet_dir('w7_symlink'))
~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
OSError: [WinError 1314] A required privilege is not held by the client: 'w7' -> 'C:\\Users\\HODLIN~1\\AppData\\Local\\Temp\\bitcoin_func_test_tqrp64pv\\node0\\regtest\\wallets\\w7_symlink'
Was able to find a solution on StackOverflow - activated Developer Mode in Windows settings to avoid it. After that the test runs fine using native-built Windows binaries (without changes from this PR). Later I changed my user to be Administrator, might have solved it too.
e717fb5 to
9025657
Compare
…ively 25b56fd ci: Test cross-built Windows executables on Windows natively (Hennadii Stepanov) 3501bca ci: Move "Windows cross" job from Cirrus CI to GHA CI (Hennadii Stepanov) f861919 ci: Use `bash` by default for all platforms (Hennadii Stepanov) Pull request description: This PR enables on the CI tests of cross-compiled Windows binaries on Windows. It is important to have such tests in CI because the release binaries for Windows are also cross-compiled. Two functional tests, `wallet_migration.py` and `wallet_multiwallet.py`, are temporarily disabled. They require fixes, such as #31410, and adjustments for error message handling. Re-enabling these tests will be addressed in follow-up PRs. Resolves #31071. ACKs for top commit: davidgumberg: tested reACK 25b56fd hodlinator: re-ACK 25b56fd willcl-ark: utACK 25b56fd maflcko: review-only ACK 25b56fd 🍎 Tree-SHA512: fb9150807b7ebb248e8f4fe7b16e5179251e7be9336459287787f27e542583d73d937e6969667fd836378b676bb9be7f66756dc1abca8a01364bc9ee3e3720a5
|
Needs rebase to remove the TODO? |
9025657 to
5fca92d
Compare
272cd09 log: Use warning level while scanning wallet dir (MarcoFalke) 1777644 qa, wallet: Verify warning when failing to scan (Hodlinator) 893e51f wallet: Correct dir iteration error handling (Hodlinator) Pull request description: Make wallet DB properly detect and report failure to scan wallet directory. Seems to have been broken since moving from Boost to `std::filesystem`. Found while reviewing: #31410 (review) ACKs for top commit: achow101: ACK 272cd09 maflcko: re-ACK 272cd09 🍽 rkrux: tACK 272cd09 Tree-SHA512: 969afde2e37f885ed0c823dc36d2dbeaa0378639849c6a26f8ac67b4f1997eea95bbcae6d58aef5b716807210f37eb166c0cda7ba1d6caffd34249970833af3a
|
Would be nice to rebase this to fix the code-merge-conflicts, even if it remains in draft and even if the test remains in a failing state. |
This change ensures that each condition potentially triggering the "Error scanning" log message is tested independently, avoiding false positives. Additionally, the "Permission denied" error test is now performed conditionally, skipping scenarios where it is not applicable (e.g., when running as root or on Windows).
80034e7 to
6055935
Compare
Rebased. |
maflcko
left a comment
There was a problem hiding this comment.
the rebase looks correct. (ci obviously still fails, as expected)
re-ACK 6055935 🏣
Show signature
Signature:
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 605593547aa196204b1e5b8f7857abacd36906b2 🏣
12YfZ+I1LPx+9Su229E0oRkkR/3IFZSHFkgapE4PSB4kMY9MauKAzKmb2Nryy1azcj7xhYTwV+w0XZzQtL2pAw==
| # 1. "Permission denied" error. | ||
| if platform.system() != 'Windows': | ||
| if os.geteuid() == 0: | ||
| self.log.warning('Skipping "permission denied"-test requiring non-root user.') | ||
| else: |
There was a problem hiding this comment.
nit in 8f22dd5: Would be nice to use the same logic flow like above in this test:
self.log.info("Verify warning is emitted when failing to scan a wallet in the wallets directory")
if platform.system() == 'Windows':
self.log.warning('Skipping test involving chmod as Windows does not support it.')
elif os.geteuid() == 0:
self.log.warning('Skipping test involving chmod as it requires a non-root user.')
else:
...|
cc @achow101 |
|
Finally got around to debugging the actual issue. The issue has to do with how The problem is that GCC does not implement symlink detection for Windows (https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/src/filesystem/ops-common.h;h=ba377905a2e90f7baf30c900b090f1f732397e08;hb=refs/heads/releases/gcc-13#l124), so Unfortunately, because we can't detect symlinks on mingw, several other test cases in |
|
What is the status of this? We now also have #34233, which is trying to re-enable all currently excluded tests. |
Unchanged from last. We know the cause, but no fixes have been proposed. |
|
@hebasto are you planning on working on a fix? Otherwise maybe we should close this, and move the relevent details into an issue, so that someone might pick it up. |
|
Since I volunteered in late October out-of-band to have a look at this, I today confirmed with @hebasto that he's fine with me taking over this issue, and will resume work on it next week if nothing unforeseen happens. Feel free to close this one, I will create a tracking issue next week, to be followed by my own PR. |
Fixes #31409.
The first commit prevents possible false positives by ensuring that each condition potentially causing the "Error scanning" log message is tested separately.
The second commit disables a problematic check on Windows.