wallet: Fix detection of symlinks on Windows by achow101 · Pull Request #34603 · bitcoin/bitcoin

achow101 · 2026-02-17T00:34:28Z

This PR implements symlink detection for Windows, since GCC doesn't support symlinks on Windows. This is used by the wallet's ListDatabases in order to detect symlinks and avoid recursing them. It's also used in wallet path validation since that is also checking whether paths are symlinks, and we should properly detect symlinks on Windows.

Under the hood, the IsSymlink function uses std::filesystem::is_symlink for non-Windows systems. On Windows, it checks a file's attributes to determine if it is a reparse point. Strictly speaking, this will also catch files that aren't symlinks, such as directories that are Onedrive mounts. I think it's fine to not be recursively searching such directories.

This is based on #34418 for the multiwallet test and CI changes that it enables. The last commit also removes the skipping of the symlink checks so that we are testing that the symlink handling behavior on Windows matches that on Linux and MacOS.

DrahtBot · 2026-02-17T00:35:07Z

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

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:

#34806 (refactor: logging: Various API improvements by ajtowns)
#34544 (wallet: Disallow wallet names that are paths including .. and . elements by achow101)
#34439 (qa: Drop recursive deletes from test code, add lint checks. by davidgumberg)
#34418 (qa: Make wallet_multiwallet.py Windows crossbuild-compatible by hodlinator)
#33186 (wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain) by w0xlt)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

windows -> Windows [Proper noun; "Windows" should be capitalized in documentation comment]
explicilty -> explicitly [Spelling error in comment]

^{2026-03-13 23:06:24}

fanquake · 2026-02-17T09:41:59Z

src/wallet/wallet.cpp

-          (path_type == fs::file_type::symlink && fs::is_directory(wallet_path)) ||
-          (path_type == fs::file_type::regular && fs::PathFromString(name).filename() == fs::PathFromString(name)))) {
+          (IsSymlink(wallet_path) && fs::is_directory(wallet_path)) ||
+          // Windows cross compile does not detect symlinks, so we need to explicilty check


Do symlinks work fine with LLVM/Clang? cc @hebasto (#31507).

When building on Windows either with MSVC or clang-cl, std::filesystem::is_symlink successfully detects file and folder symbolic links, but fails to detect junctions.

Since the in-code comment mentions "Windows cross compile", were you actually asking about the LLVM MinGW toolchain?

hebasto · 2026-02-18T15:19:24Z

src/util/fs_helpers.cpp

+    if (file_attrs == INVALID_FILE_ATTRIBUTES) {
+        throw fs::filesystem_error("Unable to get file attributes", fs::PathToString(path), std::make_error_code(std::errc::invalid_argument));
+    }
+    return (file_attrs & FILE_ATTRIBUTE_REPARSE_POINT) != 0;


f507a31

Should we additionally check against a Reparse Point Tag value?

I considered that, but didn't think any of the possible tag values were things that we actually want to support recursively scanning inside of.

Since the IsSymlink fucntion returns true for entities other than symlinks, we should probably add a clarifying comment.

The docstring states that it returns true for reparse points on windows.

luke-jr · 2026-02-18T17:37:19Z

test/functional/wallet_multiwallet.py

-        finally:
-            # Need to ensure access is restored for cleanup
-            os.chmod(wallet_dir('no_access'), stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)


Why is this removed? It's actually kind of annoying to have files that need to be chmod'd before deletion

It isn't, the chmod testing is significantly changed. This is from #34418 and review comments about it should be left there.

luke-jr · 2026-02-18T18:50:00Z

src/util/fs_helpers.cpp

 }
 #endif
+
+bool IsSymlink(const fs::path& path)


Might it be better to just override fs::is_symlink on Windows?

I considered that, but we aren't already using fs::is_symlink so I don't think it would give us any benefit. Also, since IsSymlink returns true for more than just symlinks, I thought it wouldn't be appropriate to be override the stdlib function.

luke-jr · 2026-02-18T19:14:11Z

src/wallet/db.cpp

+            if (IsSymlink(it->path())) {
+                LogWarning("Not recursively searching symlink/reparse point at '%s'", fs::PathToString(it->path()));
+                it.disable_recursion_pending();
+            }


Is it intentional that we still check the symlink itself for a wallet?

If so, should we not do so even if IsSymlink throws?

Is it intentional that we still check the symlink itself for a wallet?

Yes

If so, should we not do so even if IsSymlink throws?

status() and symlink_status() used below can also throw, so this is the same behavior.

src/wallet/wallet.cpp

test_scanning_sub_dir(): - Remove try/finally - we don't need to clean up after a failed test (done in this commit to maintain indentation). Regarding symlinks: bitcoin#31410 (comment) Kept some symlink creation which didn't disrupt Windows cross builds to make for a smaller diff and less cumbersome code. There is some hope of eventually getting better symlink support via bitcoin#34603. Co-authored-by: Ava Chow <github@achow101.com>

GCC lacks detection of symlinks on Windows. IsSymlink implements this detection on Windows, and utilizes std::filesystem::is_symlink for other systems.

When searching the wallets directory for wallets, we want to avoid recursing symlinks. Although recursive_directory_iterator already doesn't recurse symlinks, since GCC doesn't implement symlink detection on Windows, we need to use IsSymlink to detect symlinks on Windows in order to avoid recursing them.

Since GCC lacks symlink detection, we need to use IsSymlink to detect symlinks on Windows. Additionally, symlinks to files are reported as being regular files rather symlnks, so we need to explicitly check with IsSymlink that a regular file is not actually a symlink.

DrahtBot added the Wallet label Feb 17, 2026

DrahtBot added the CI failed label Feb 17, 2026

achow101 force-pushed the fix-listwallet-recursion-detect-symlinks branch from 0c76d21 to 677297e Compare February 17, 2026 03:00

fanquake reviewed Feb 17, 2026

View reviewed changes

This was referenced Feb 18, 2026

wallet: Disallow wallet names that are paths including .. and . elements #34544

Open

qa: Drop recursive deletes from test code, add lint checks. #34439

Open

qa: Make wallet_multiwallet.py Windows crossbuild-compatible #34418

Merged

hebasto reviewed Feb 18, 2026

View reviewed changes

luke-jr reviewed Feb 18, 2026

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

luke-jr reviewed Feb 18, 2026

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Feb 18, 2026

logging: replace -loglevel with -trace, expose trace logging via RPC #34038

Open

DrahtBot added the Needs rebase label Feb 19, 2026

DrahtBot mentioned this pull request Feb 19, 2026

wallet, rpc: remove settxfee and paytxfee #32138

Merged

achow101 force-pushed the fix-listwallet-recursion-detect-symlinks branch from 677297e to fda778d Compare February 24, 2026 23:12

DrahtBot removed the Needs rebase label Feb 25, 2026

DrahtBot mentioned this pull request Mar 4, 2026

Multiprocess bitcoin #10102

Draft

achow101 added 4 commits March 13, 2026 15:46

fs: Add IsSymlink which can check for Windows symlinks

892bed0

GCC lacks detection of symlinks on Windows. IsSymlink implements this detection on Windows, and utilizes std::filesystem::is_symlink for other systems.

test: Test multiwallet symlinks on Windows

9333d26

achow101 force-pushed the fix-listwallet-recursion-detect-symlinks branch from fda778d to 9333d26 Compare March 13, 2026 23:05

DrahtBot removed the CI failed label Mar 14, 2026

DrahtBot mentioned this pull request Mar 14, 2026

refactor: logging: Various API improvements #34806

Open

Conversation

achow101 commented Feb 17, 2026

Uh oh!

DrahtBot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DrahtBot commented Feb 17, 2026 •

edited

Loading