qa: Fix `wallet_multiwallet.py` #31410

pull hebasto wants to merge 3 commits into bitcoin:master from hebasto:241203-multiwallet changing 2 files +29 −12
  1. hebasto commented at 1:37 PM on December 3, 2024: member

    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.

  2. hebasto added the label Tests on Dec 3, 2024
  3. DrahtBot commented at 1:37 PM on December 3, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31410.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko
    Approach ACK stickies-v

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #34240 (ci: Run feature_unsupported_utxo_db.py on Windows by maflcko)
    • #34168 (qa: Require --exclude for each excluded test by hebasto)
    • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. hebasto force-pushed on Dec 3, 2024
  5. DrahtBot added the label CI failed on Dec 3, 2024
  6. in test/functional/wallet_multiwallet.py:132 in b58c8fe005 outdated 
     125 | @@ -129,15 +126,23 @@ def wallet_file(name):
 126 |          for wallet_name in to_load:
 127 |              self.nodes[0].loadwallet(wallet_name)
 128 |  
 129 | +        # Tests for possible scanning errors:
 130 | +        # 1. "Permission denied" error.
 131 |          os.mkdir(wallet_dir('no_access'))
 132 |          os.chmod(wallet_dir('no_access'), 0)
    maflcko commented at 2:36 PM on December 3, 2024:

    Would it be possible to achieve the same with icacls on Windows?

    hebasto commented at 2:48 PM on December 3, 2024:

    Via subprocess?

    hebasto commented at 9:12 PM on December 3, 2024:

    I'm not sure if testing the scenario with the modified ACL on Windows is worth the added complexity of the test script.

  7. hebasto force-pushed on Dec 3, 2024
  8. hebasto marked this as ready for review on Dec 3, 2024
  9. hebasto added the label Wallet on Dec 3, 2024
  10. maflcko removed the label CI failed on Dec 4, 2024
  11. in test/functional/wallet_multiwallet.py:147 in fd8d7c367f outdated 
     148 | +            assert_equal(sorted(map(lambda w: w['name'], walletlist)), sorted(in_wallet_dir))
 149 | +        # 2. "Too many levels of symbolic links" error.
 150 | +        os.mkdir(wallet_dir('self_walletdat_symlink'))
 151 | +        os.symlink('wallet.dat', wallet_dir('self_walletdat_symlink/wallet.dat'))
 152 | +        with self.nodes[0].assert_debug_log(expected_msgs=['Error scanning']):
 153 | +            walletlist = self.nodes[0].listwalletdir()['wallets']
    maflcko commented at 7:30 AM on December 4, 2024:

    Could do a third listwalletdir without the symlink to check that the test is working by checking for absence of error scanning?

    hebasto commented at 9:41 AM on December 4, 2024:

    Thanks! Done.

  12. hebasto force-pushed on Dec 4, 2024
  13. maflcko commented at 8:48 PM on December 5, 2024: member

    review ACK c0127439597ee9d09b8e117be7d6226a68b45721 🎑

    <details><summary>Show signature</summary>

    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==

    </details>

  14. maflcko added this to the milestone 29.0 on Dec 18, 2024
  15. hebasto commented at 11:55 AM on December 20, 2024: member

    Friendly ping a Python connoisseur @stickies-v :)

  16. Countrymom71 approved
  17. in test/functional/wallet_multiwallet.py:144 in 3f53cc8b8c outdated 
     147 | +                with self.nodes[0].assert_debug_log(expected_msgs=['Error scanning']):
 148 | +                    walletlist = self.nodes[0].listwalletdir()['wallets']
 149 | +            finally:
 150 | +                # Need to ensure access is restored for cleanup
 151 | +                os.chmod(wallet_dir('no_access'), stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
 152 | +            assert_equal(sorted(map(lambda w: w['name'], walletlist)), sorted(in_wallet_dir))
    stickies-v commented at 11:22 AM on January 16, 2025:

    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?

    hebasto commented at 11:20 AM on January 21, 2025:

    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.

  18. stickies-v commented at 11:50 AM on January 16, 2025: contributor

    Approach ACK c0127439597ee9d09b8e117be7d6226a68b45721

    I'm not familiar enough with symlinks and build toolchains to have an opinion on c0127439597ee9d09b8e117be7d6226a68b45721, but it seems reasonable.

  19. in test/functional/wallet_multiwallet.py:165 in c012743959 outdated 
     145 | +                # Need to ensure access is restored for cleanup
 146 | +                os.chmod(wallet_dir('no_access'), stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
 147 | +            assert_equal(sorted(map(lambda w: w['name'], walletlist)), sorted(in_wallet_dir))
 148 | +        # 2. "Too many levels of symbolic links" error.
 149 | +        # This test cannot be conducted robustly on Windows
 150 | +        # because it depends on the build toolchain:
    fanquake commented at 12:05 PM on January 16, 2025:

    In c0127439597ee9d09b8e117be7d6226a68b45721:

    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.

    maflcko commented at 12:16 PM on January 16, 2025:

    I guess the check could be limited to exclude the msvc build?

    hebasto commented at 11:26 AM on January 21, 2025:

    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.

    fanquake commented at 11:26 AM on January 21, 2025:

    I guess the check could be limited to exclude the msvc build?

    That would make the most sense to me.

    fanquake commented at 11:28 AM on January 21, 2025:

    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.

    hebasto commented at 4:06 PM on January 29, 2025:

    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.

    maflcko commented at 9:23 AM on February 4, 2025:

    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.

  20. hebasto force-pushed on Jan 21, 2025
  21. maflcko commented at 11:24 AM on January 21, 2025: member

    Only change is the commit msg

    re-ACK e717fb5a429d9d00e008fa1eb2b85179050be8fe 🍣

    <details><summary>Show signature</summary>

    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 e717fb5a429d9d00e008fa1eb2b85179050be8fe 🍣
70Rky7BeBYMJwMIgU5Z47QQaP7eIFFxizwukIZAlazkuNY6DYE5ckOBE/wtHkl58rSQdK2QEsSU14lp+IPPVAA==

    </details>

  22. DrahtBot requested review from stickies-v on Jan 21, 2025
  23. Countrymom71 commented at 3:04 PM on January 29, 2025: none

    I need help on Cloudflare seeing my wallet. I have tried setting up all the domains and things to keep it secure and reroute things and until I get it right I can't see my account with crypto. I only want to use to to keep my crypto secure and send and receive. I have no interest in anything else on the computer I am old and don't understand all the computer lingo I just need some simple easy way to get to my account and use it pay my fee and that's it. The other stuff on here makes me want to scream It's confusing. I even got a younger girl to come help me and paid her. She wasn't much help you can see. I believe if I had a number to call and someone could talk me thru a lil I would figure it out and life would be grand. I have done a lot and so did the girl I hired but something must be wrong somewhere. Please help me or tell me who to call thanks

  24. hebasto commented at 8:57 PM on February 1, 2025: member

    ~FWIW, the Windows UCRT-linked cross-compiled builds still need this fix.~

    UPD. Further investigation is needed.

  25. hodlinator commented at 11:19 PM on February 8, 2025: contributor

    Code review e717fb5a429d9d00e008fa1eb2b85179050be8fe

    Guix cross-build failure

    Attempted to run Guix build e717fb5a429d9d00e008fa1eb2b85179050be8fe cross-built for Windows, but it fails already on test "0" / line 133:

    https://github.com/bitcoin/bitcoin/blob/e717fb5a429d9d00e008fa1eb2b85179050be8fe/test/functional/wallet_multiwallet.py#L129-L133

    <details><summary>Log</summary>

    $ 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

    </details>

    Should that not be working?

    C++ behavior difference when iterating directories?

    I managed to come up with 584e2fcd943ede052aa3641befae121454ae091e, 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 (review) until I was pretty much done with the implementation (didn't want to bias my review). Might be something for a follow-up PR.

    <details><summary>

    Windows Developer Mode parenthesis

    </summary>

    First thing I attempted on Windows was to run the wallet_multiwallet.py test on the current parent of this PR (ebe4cac38bf6c510b00b8e080acab079c54016d6):

    $ 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.

    </details>

  26. in test/functional/wallet_multiwallet.py:135 in e717fb5a42 outdated 
     133 | +        # 0. Baseline, no errors.
 134 | +        with self.nodes[0].assert_debug_log(expected_msgs=[], unexpected_msgs=['Error scanning']):
 135 | +            walletlist = self.nodes[0].listwalletdir()['wallets']
 136 | +        assert_equal(sorted(map(lambda w: w['name'], walletlist)), sorted(in_wallet_dir))
 137 | +        # 1. "Permission denied" error.
 138 | +        if platform.system() != 'Windows' and os.geteuid() != 0:
    hodlinator commented at 10:00 AM on February 10, 2025:

    nit: Could add a warning when skipping the test because of user permissions:

            if platform.system() != 'Windows':
            if os.geteuid() == 0:
                self.log.warning('Skipping "permission denied"-test requiring non-root user.')
            else:

    (Ran into something similar on my own branch as sanitizer-tests apparently run with root permissions (CI log)).

    hebasto commented at 1:25 PM on February 10, 2025:

    Thanks! Applied.

  27. hebasto force-pushed on Feb 10, 2025
  28. hebasto commented at 3:33 PM on February 10, 2025: member

    Attempted to run Guix build e717fb5 cross-built for Windows, but it fails already on test "0" / line 133:

    Is this failure introduced by this PR, or does it occur on the master branch as well?

    Was able to find a solution on StackOverflow - activated Developer Mode in Windows settings to avoid it.

    There are many ways to enable the "Create symbolic links" privilege (SeCreateSymbolicLinkPrivilege).

  29. hodlinator commented at 10:52 PM on February 10, 2025: contributor

    Attempted to run Guix build e717fb5 cross-built for Windows, but it fails already on test "0" / line 133:

    Is this failure introduced by this PR, or does it occur on the master branch as well?

    I think the way cross-built binaries fail was new thanks to your introduced "0. Baseline, no errors."-test.

    With the same executable (e717fb5a429d = this PR which only includes Python changes) on master (ebe4cac38bf6c510b00b8e080acab079c54016d6 Python scripts), I get a different error, since os.chmod() has no effect on Windows, the expected scanning error is not emitted:

    $ BITCOIND=/c/Users/hodlinator/Downloads/bitcoind-e717fb5a429d.exe build/test/functional/wallet_multiwallet.py
2025-02-10T21:40:56.073000Z TestFramework (INFO): PRNG seed is: 4150940621330997535
2025-02-10T21:40:56.093000Z TestFramework (INFO): Initializing test directory C:\Users\HODLIN~1\AppData\Local\Temp\bitcoin_func_test_pszwxbng
2025-02-10T21:40:59.598000Z 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 135, in run_test
    with self.nodes[0].assert_debug_log(expected_msgs=['Error scanning']):
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Program Files\WindowsApps\PythonSoftwareFoundation.Python.3.13_3.13.752.0_x64__qbz5n2kfra8p0\Lib\contextlib.py", line 148, in __exit__
    next(self.gen)
    ~~~~^^^^^^^^^^
  File "C:\Users\hodlinator\bitcoin\build\test\functional\test_framework\test_node.py", line 520, in assert_debug_log
    self._raise_assertion_error('Expected messages "{}" does not partially match log:\n\n{}\n\n'.format(str(expected_msgs), print_log))
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Users\hodlinator\bitcoin\build\test\functional\test_framework\test_node.py", line 198, in _raise_assertion_error
    raise AssertionError(self._node_msg(msg))
AssertionError: [node 0] Expected messages "['Error scanning']" does not partially match log:
  30. maflcko commented at 7:23 AM on February 11, 2025: member

    re-ACK 9025657baaf99fcf630cc1a37baec11b072196fa only change is adding a warning log for the skipped test 🥋

    <details><summary>Show signature</summary>

    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 9025657baaf99fcf630cc1a37baec11b072196fa only change is adding a warning log for the skipped test 🥋
8JwK2lTF2PaJReWtjE7KYXRP5D3CeQdUUGaK/bgJ/HSQX1wVVCUIdRe+nyLSRUJ7rivC1UC0wOT6Hh8/2Y3tDw==

    </details>

  31. hodlinator commented at 7:52 PM on February 11, 2025: contributor

    Just reproduced the same failure using binaries produced by CI cross-compilation setup from #31176 as I had before using the Guix cross-compiled build. Good to see that the two have the same behavior... but worrying that nobody else is getting this.

    $ BITCOINCLI=/c/Users/hodlinator/Downloads/18274b6-bins/bitcoin-cli.exe BITCOIND=/c/Users/hodlinator/Downloads/18274b6-bins/bitcoind.exe build/test/functional/wallet_multiwallet.py
2025-02-11T19:42:46.959000Z TestFramework (INFO): PRNG seed is: 4624503638077972153
2025-02-11T19:42:47.013000Z TestFramework (INFO): Initializing test directory C:\Users\HODLIN~1\AppData\Local\Temp\bitcoin_func_test_14_2asnv
2025-02-11T19:42:48.521000Z 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-11T19:42:48.573000Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
  32. glozow requested review from achow101 on Feb 13, 2025
  33. glozow assigned achow101 on Feb 13, 2025
  34. achow101 commented at 9:03 PM on February 13, 2025: member

    I am able to replicate @hodlinator's errors with cross built binaries.

  35. hebasto commented at 9:14 PM on February 13, 2025: member

    Is this failure introduced by this PR, or does it occur on the master branch as well?

    I think the way cross-built binaries fail was new thanks to your introduced "0. Baseline, no errors."-test.

    Right.

    Even with an additional commit that fixes another check in this test, some of the subsequent checks reveal their flaws. Fixing these issues will require removing non-portable behaviour in the wallet code. I'm testing a branch with additional fixes.

    In the meantime, converting this PR to a draft.

  36. hebasto marked this as a draft on Feb 13, 2025
  37. hebasto removed this from the milestone 29.0 on Feb 13, 2025
  38. fanquake referenced this in commit 3358b1d105 on Mar 31, 2025
  39. maflcko commented at 10:57 AM on April 1, 2025: member

    Needs rebase to remove the TODO?

  40. hebasto force-pushed on Apr 2, 2025
  41. achow101 referenced this in commit 482d255376 on Jun 20, 2025
  42. DrahtBot added the label Needs rebase on Jun 20, 2025
  43. maflcko commented at 8:42 AM on June 21, 2025: member

    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.

  44. qa: Test scanning errors individually
    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).
    8f22dd5e4c
  45. qa: Disable self-symlink test on Windows and document it 8fa3e95e33
  46. ci: Enable `wallet_multiwallet.py` in "Windows, test cross-built" job 605593547a
  47. hebasto force-pushed on Jul 10, 2025
  48. DrahtBot removed the label Needs rebase on Jul 10, 2025
  49. hebasto commented at 3:07 PM on July 10, 2025: member

    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.

    Rebased.

  50. in test/functional/wallet_multiwallet.py:153 in 8f22dd5e4c outdated 
     156 | +        assert_equal(sorted(map(lambda w: w['name'], walletlist)), sorted(in_wallet_dir))
 157 | +        # 1. "Permission denied" error.
 158 | +        if platform.system() != 'Windows':
 159 | +            if os.geteuid() == 0:
 160 | +                self.log.warning('Skipping "permission denied"-test requiring non-root user.')
 161 | +            else:
    maflcko commented at 4:19 PM on July 10, 2025:

    nit in 8f22dd5e4c4b116ad873e464ea7f75caaef3476d: 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:
...
  51. maflcko commented at 4:22 PM on July 10, 2025: member

    the rebase looks correct. (ci obviously still fails, as expected)

    re-ACK 605593547aa196204b1e5b8f7857abacd36906b2 🏣

    <details><summary>Show signature</summary>

    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==

    </details>

  52. achow101 requested review from hodlinator on Oct 22, 2025
  53. maflcko commented at 3:32 PM on October 22, 2025: member

    cc @achow101

  54. achow101 commented at 9:33 PM on November 19, 2025: member

    Finally got around to debugging the actual issue.

    The issue has to do with how std::filesystem::recursive_directory_iterator handles symlinks. The docs for the default options that we use say that symlinks are not recursed into for searching. Instead, when we find symlink directories, we look for a wallet.dat file, but we won't search any of the subdirectories.

    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 recursive_directory_iterator ends up recursing the symlinked directory. This recursion is the cause of the test failure as the test creates a recursive symlink - it symlinks to the wallet directory from within the wallet directory itself.

    Unfortunately, because we can't detect symlinks on mingw, several other test cases in wallet_multiwallet.py fail since they are specifically testing things with symlinks.

  55. fanquake commented at 9:59 AM on January 9, 2026: member

    What is the status of this? We now also have #34233, which is trying to re-enable all currently excluded tests.

  56. achow101 commented at 12:35 AM on January 10, 2026: member

    What is the status of this?

    Unchanged from last. We know the cause, but no fixes have been proposed.

  57. fanquake commented at 3:36 PM on January 10, 2026: member

    @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.

  58. hodlinator commented at 12:38 PM on January 12, 2026: contributor

    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.

  59. hebasto closed this on Jan 12, 2026
  60. hodlinator commented at 12:12 PM on January 24, 2026: contributor

    Started looking further into which parts of the test fail with cross-built binaries, but not ready create a new PR yet. I'm considering having explicit wallet_multiwallet.py --non-windows and wallet_multiwallet.py --common to make it clear that a subset of tests are run.

    The already existing #31409 should be sufficient to track this for now.

  61. hodlinator referenced this in commit 7c7c81bfff on Jan 27, 2026
  62. hodlinator referenced this in commit 00f145d355 on Jan 27, 2026
  63. hodlinator referenced this in commit 467d4899e6 on Jan 27, 2026
  64. hodlinator referenced this in commit a36aae2f40 on Jan 27, 2026
  65. hodlinator commented at 12:37 PM on January 27, 2026: contributor

    New PR up for review: #34418

Contributors
hebastoDrahtBotmaflckoCountrymom71stickies-vfanquakehodlinatorachow101


achow101stickies-vhodlinator

Labels
WalletTests
Linked (view graph)
#31176 ci: Test cross-built Windows executables on Windows natively#31409 qa: Broken `wallet_multiwallet.py`#32736 wallet: Correct dir iteration error handling#34418 qa: Make wallet_multiwallet.py Windows crossbuild-compatible
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-21 03:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me