Drop support for passing fs::path directly to std::ifstream and std::ofstream constructors and open() functions, because as reported by hebasto in #33545, after https://wg21.link/lwg3430 there is no way this can continue to work in windows builds, and there are already compile errors compiling for windows with newer versions of libc++.
Instead, add an fs::path::std_path() method that returns std::filesystem::path references and use it where needed.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33550.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Concept ACK.
Maybe consider removing these lines now:https://github.com/bitcoin/bitcoin/blob/f39d3e1a2f2b9cdcf0e77942fd69eb92a58ce77f/src/util/fs.h#L79-L81 ?
CI fails:
/home/admin/actions-runner/_work/_temp/src/wallet/test/init_test_fixture.cpp:39:21: error: conversion function from 'mapped_type' (aka 'fs::path') to 'const string' (aka 'const basic_string<char>') invokes a deleted function
39 | std::ofstream f{m_walletdir_path_cases["file"]};
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/admin/actions-runner/_work/_temp/src/util/fs.h:65:5: note: 'operator basic_string' has been explicitly marked deleted here
65 | operator string_type() const = delete;
| ^
/cxx_build/include/c++/v1/fstream:1303:63: note: passing argument to parameter '__s' here
1303 | _LIBCPP_HIDE_FROM_ABI explicit basic_ofstream(const string& __s, ios_base::openmode __mode = ios_base::out);
| ^
1 error generated.
review ACK f39d3e1a2f2b9cdcf0e77942fd69eb92a58ce77f, but CI fails
As reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545,
newer libc++ versions implementing https://wg21.link/lwg3430 will no longer
implicitly convert `fs::path` objects to `std::filesystem::path` objects when
constructing `std::ifstream` and `std::ofstream` types.
This is not a problem in Unix systems since `fs::path` objects use
`std::string` as their native string type, but it causes compile errors on
Windows which use `std::wstring` as their string type, since `fstream`s can't
be constructed from `wstring`s.
Fix the windows libc++ compile errors by adding a new `fs::path::std_path()`
method and using it construct `fstream`s more portably.
Additionally, delete `fs::path`'s implicit `native_string` conversion so these
errors will not go undetected in the future, even though there is not currently
a CI job testing Windows libc++ builds.
<!-- begin push-2 -->
Updated f39d3e1a2f2b9cdcf0e77942fd69eb92a58ce77f -> b0113afd44b4c7c0d0da9883424bd2978de3d18c (pr/winstream.1 -> pr/winstream.2, compare)<!-- end --> to fix CI failures caused by missing std_path replacement
re: #33550#pullrequestreview-3305372031
Maybe consider removing these lines now:
Nice suggestion. Attempted this in new commit.
<!-- begin push-3 -->
Added 1 commits b0113afd44b4c7c0d0da9883424bd2978de3d18c -> d405c76d2e32e957a72c042f1fb82fd0cfcc8e6d (pr/winstream.2 -> pr/winstream.3, compare)<!-- end --> simplifying fs::path as suggested
re: #33550 (review)
Maybe consider removing these lines now:
Nice suggestion. Attempted this in new commit.
Added 1 commits b0113af -> d405c76 (
pr/winstream.2->pr/winstream.3, compare) simplifying fs::path as suggested
Thanks for taking this.
I assume we still need fs::path::filename() though.
Tested b0113afd44b4c7c0d0da9883424bd2978de3d18c by cross-compiling for x86_64-w64-mingw32 and aarch64-w64-mingw32 targets using the LLVM MinGW toolchain.
74 | @@ -65,11 +75,6 @@ class path : public std::filesystem::path 75 | const std::u8string& utf8_str{std::filesystem::path::u8string()}; 76 | return std::string{utf8_str.begin(), utf8_str.end()}; 77 | } 78 | - 79 | - // Required for path overloads in <fstream>. 80 | - // See https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=96e0367ead5d8dcac3bec2865582e76e2fbab190 81 | - path& make_preferred() { std::filesystem::path::make_preferred(); return *this; }
This might require a bit more adjustments:
--- a/src/common/settings.cpp
+++ b/src/common/settings.cpp
@@ -78,7 +78,7 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
if (!fs::exists(path)) return true;
std::ifstream file;
- file.open(path);
+ file.open(path.std_path());
if (!file.is_open()) {
errors.emplace_back(strprintf("%s. Please check permissions.", fs::PathToString(path)));
return false;
@@ -133,7 +133,7 @@ bool WriteSettings(const fs::path& path,
out.pushKVEnd(value.first, value.second);
}
std::ofstream file;
- file.open(path);
+ file.open(path.std_path());
if (file.fail()) {
errors.emplace_back(strprintf("Error: Unable to open settings file %s for writing", fs::PathToString(path)));
return false;
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -114,7 +114,7 @@ GenerateAuthCookieResult GenerateAuthCookie(const std::optional<fs::perms>& cook
if (filepath_tmp.empty()) {
return GenerateAuthCookieResult::DISABLED; // -norpccookiefile
}
- file.open(filepath_tmp);
+ file.open(filepath_tmp.std_path());
if (!file.is_open()) {
LogWarning("Unable to open cookie authentication file %s for writing", fs::PathToString(filepath_tmp));
return GenerateAuthCookieResult::ERR;
@@ -153,7 +153,7 @@ bool GetAuthCookie(std::string *cookie_out)
if (filepath.empty()) {
return true; // -norpccookiefile
}
- file.open(filepath);
+ file.open(filepath.std_path());
if (!file.is_open())
return false;
std::getline(file, cookie);
--- a/src/test/settings_tests.cpp
+++ b/src/test/settings_tests.cpp
@@ -45,7 +45,7 @@ inline std::ostream& operator<<(std::ostream& os, const std::pair<std::string, c
inline void WriteText(const fs::path& path, const std::string& text)
{
std::ofstream file;
- file.open(path);
+ file.open(path.std_path());
file << text;
}
--- a/src/wallet/dump.cpp
+++ b/src/wallet/dump.cpp
@@ -37,7 +37,7 @@ bool DumpWallet(const ArgsManager& args, WalletDatabase& db, bilingual_str& erro
return false;
}
std::ofstream dump_file;
- dump_file.open(path);
+ dump_file.open(path.std_path());
if (dump_file.fail()) {
error = strprintf(_("Unable to open %s for writing"), fs::PathToString(path));
return false;
I assume we still need
fs::path::filename()though.
I think we could overload it but it seems a little consistent to do that without a clear reason and without also overloading parent_path. For now just dropped some code in bitcoin.cpp which was using it.
These overloads were needed to allow passing `fs::path` objects directly to
libstdc++'s `fstream` constructors, but after the previous commit, there is no
longer any remaining code that does pass `fs::path` objects to `fstream`
constructors. Writing new code which does this is also discouraged because the
standard has been updated in https://wg21.link/lwg3430 to disallow it.
Dropping these also means its no longer possible to pass `fs::path` arguments
directly to `fstream::open` in libstdc++, which is somewhat unfortunate but not
a big loss because it is already not possible to pass them to the constructor.
So this commit updates `fstream::open` calls.
Additionally, this change required updates to src/bitcoin.cpp since it was
relying on the overloaded filename() method.
<!-- begin push-4 -->
Updated d405c76d2e32e957a72c042f1fb82fd0cfcc8e6d -> c864a4c1940d682f7eb6fdb3b91b18d638b59330 (pr/winstream.3 -> pr/winstream.4, compare)<!-- end --> with CI fixes for second commit
ACK c864a4c1940d682f7eb6fdb3b91b18d638b59330.
... even though there is not currently a CI job testing Windows libc++ builds.
Added to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.
Added to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.
Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):
test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec"
test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}"
...
Added to the nightly builds in https://github.com/hebasto/bitcoin-core-nightly/pull/74.
Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):
test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec" test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}" test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}" ...
Correct. I'm looking into them.
However, I believe those failures are not related to this PR changes.
Added to the nightly builds in hebasto/bitcoin-core-nightly#74.
Looks like there are failing unit tests (https://github.com/hebasto/bitcoin-core-nightly/actions/runs/18313967330/job/52151177201#step:6:401):
test\util_string_tests.cpp(43): Entering test case "ConstevalFormatString_NumSpec" test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}" test/util_string_tests.cpp(40): error: in "util_string_tests/ConstevalFormatString_NumSpec": exception "const char*" raised as expected: validation on the raised exception through predicate "HasReason{error}" ...
The issue appears to be caused by this commit.
Friendly ping @l0rinc @hodlinator as co-authors of that commit :)
I'm on it, it seems related to the discussion in the original PR which already had some Windows build problems: #30546 (review)
I have reproduced the error in https://github.com/l0rinc/bitcoin-core-nightly/pull/2 with @hebasto's nightly clone redirected to l0rinc/master.
I pushed a potential fix that adds a const char * overload to HasReason in https://github.com/l0rinc/bitcoin/pull/43 - tested through https://github.com/l0rinc/bitcoin-core-nightly/pull/1.
Also added an alternative which replaces all remaining const char * throws with std::runtime_error in https://github.com/l0rinc/bitcoin/pull/44 - tested through https://github.com/l0rinc/bitcoin-core-nightly/pull/3.
Based on your feedback and on what the CI gods think, I will push one of them as a PR.
Also note that the windows build indicates additional warnings (unused vars because of ifndef __USING_WINDOWS__)
<details> <summary>Details</summary>
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/randomenv.cpp:61:15: warning: '__p__environ' redeclared without 'dllimport' attribute: previous 'dllimport' ignored [-Winconsistent-dllimport]
61 | extern char** environ; // NOLINT(readability-redundant-declaration): Necessary on some platforms
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:656:17: note: expanded from macro 'environ'
656 | #define environ _environ
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:225:21: note: expanded from macro '_environ'
225 | #define _environ (* __p__environ())
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:221:27: note: previous declaration is here
221 | _CRTIMP char ***__cdecl __p__environ(void);
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/stdlib.h:221:3: note: previous attribute is here
221 | _CRTIMP char ***__cdecl __p__environ(void);
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/llvm_mingw_toolchain/x86_64-w64-mingw32/include/_mingw.h:52:40: note: expanded from macro '_CRTIMP'
52 | # define _CRTIMP __attribute__ ((__dllimport__))
|
and
In file included from /home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/exec.cpp:8:
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/subprocess.h:759:10: warning: private field 'parent_' is not used [-Wunused-private-field]
759 | Popen* parent_ = nullptr;
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/subprocess.h:760:7: warning: private field 'err_wr_pipe_' is not used [-Wunused-private-field]
760 | int err_wr_pipe_ = -1;
| ^
/home/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/src/util/subprocess.h:1038:7: warning: private field 'child_pid_' is not used [-Wunused-private-field]
1038 | int child_pid_ = -1;
| ^
</details>
We should likely fix these as well - I don't mind pushing a separate PR for this after the other one.
I've submitted an issue to https://github.com/mstorsjo/llvm-mingw. @l0rinc
I have reproduced the error in l0rinc/bitcoin-core-nightly#2 with @hebasto's nightly clone redirected to
l0rinc/master.I pushed a potential fix that adds a
const char *overload toHasReasonin l0rinc#43 - tested through l0rinc/bitcoin-core-nightly#1.Also added an alternative which replaces all remaining
const char *throws withstd::runtime_errorin l0rinc#44 - tested through l0rinc/bitcoin-core-nightly#3.Based on your feedback and on what the CI gods think, I will push one of them as a PR.
I believe that this is also a good point. Because of array-to-pointer conversion, I'd lean to the second option you mentioned.
Also note that the windows build indicates additional warnings (unused vars because of
ifndef __USING_WINDOWS__)We should likely fix these as well - I don't mind pushing a separate PR for this after the other one.
Yes. These warnings are unrelated to the current PR changes, and I'd prefer to see them fixed in separated PR(s).
I'd lean to the second option you mentioned
Thanks, closed the other ones and opened #33569 - the dedicated nightly build passed: https://github.com/l0rinc/bitcoin-core-nightly/pull/3
I'd prefer to see them fixed in separated PR(s)
Pushed #33570 as well for extern char** environ - but the one in subprocess.h seems more involved, will leave that to someone else.
Code review ACK c864a4c1940d682f7eb6fdb3b91b18d638b59330
It bothers me a bit that the first commit doesn't compile, but it seems fixed in the second one - I'm fine with merging as long as the second commit is part of the PR.
I have tried reproducing the error of removing make_preferred and filename on master, but I'm not actually getting any failure - but it doesn't look dangerous, I'm personally fine with removing those, they don't seem to be necessary anymore.
<details> <summary>Details</summary>
root@i9-ssd /mnt/my_storage/bitcoin # cat /usr/include/c++/15/fstream | grep -C2 make_preferred
// Enable if _Path is a filesystem::path or experimental::filesystem::path
template<typename _Path, typename _Result = _Path, typename _Path2
= decltype(std::declval<_Path&>().make_preferred().filename())>
using _If_fs_path = enable_if_t<is_same_v<_Path, _Path2>, _Result>;
#endif // C++17
root@i9-ssd /mnt/my_storage/bitcoin # git log -1
commit 0eeae4d174a41c3fc2eae41e76b929fa3114d6f3 (HEAD, origin/master, origin/HEAD)
Merge: d30f149360 879c21045e
Author: merge-script <fanquake@gmail.com>
Date: Sun Oct 19 15:45:47 2025 +0100
Merge bitcoin/bitcoin#33625: Update secp256k1 subtree to latest master
3cbf7cb3e6ac51492b354732bddbb4f58ce97ed3 Squashed 'src/secp256k1/' changes from b9313c6e1a..d543c0d917 (fanquake)
Pull request description:
Updates the subtree to https://github.com/bitcoin-core/secp256k1/commit/d543c0d917a76a201578948701cc30ef336e0fe6
Related to [#33284](/bitcoin-bitcoin/33284/).
ACKs for top commit:
hebasto:
ACK 879c21045eba64b3dc875f6f2c2c579abecde1d0.
janb84:
ACK 879c21045eba64b3dc875f6f2c2c579abecde1d0
Tree-SHA512: 1802cd84959b5c935170792f458651f30431fe8340ead7966ff381c1c0c3a9f6c21bbb8dd96a07482ffed49642ded49e80b61802e688b8351956b111dffd5a78
root@i9-ssd /mnt/my_storage/bitcoin # git diff
diff --git a/src/util/fs.h b/src/util/fs.h
index 7c313e2072..24f4943048 100644
--- a/src/util/fs.h
+++ b/src/util/fs.h
@@ -65,11 +65,6 @@ public:
const std::u8string& utf8_str{std::filesystem::path::u8string()};
return std::string{utf8_str.begin(), utf8_str.end()};
}
-
- // Required for path overloads in <fstream>.
- // See https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=96e0367ead5d8dcac3bec2865582e76e2fbab190
- path& make_preferred() { std::filesystem::path::make_preferred(); return *this; }
- path filename() const { return std::filesystem::path::filename(); }
};
static inline path u8path(const std::string& utf8_str)
</details>
I have also tried reverting std::ofstream fout{file}; and the deleted method does indeed catch it, so the implementation seems correct regardless of whether it fixes the issue - which I have reproduced with @hebasto's nightly branch anyway.
I have also tested why the auto was needed in wrapper_dir - it seems to be related to mixing paths for the / - seems fine to me.
Thanks for the review!
It bothers me a bit that the first commit doesn't compile
It should compile. Github just seems to show a red x next to the commit because the job was cancelled when the second commit was pushed.
I have tried reproducing the error of removing make_preferred and filename on master, but I'm not actually getting any failure
I think the failure only happened on windows. The workaround was introduced here: #20744 (comment)
208 | @@ -209,7 +209,7 @@ static void ExecCommand(const std::vector<const char*>& args, std::string_view w 209 | 210 | // Try to resolve any symlinks and figure out the directory containing the wrapper executable. 211 | std::error_code ec; 212 | - fs::path wrapper_dir{fs::weakly_canonical(wrapper_path, ec)}; 213 | + auto wrapper_dir{fs::weakly_canonical(wrapper_path, ec)};
nit in c864a4c1940d682f7eb6fdb3b91b18d638b59330: I understand that you probably used the std namespace here and then used auto to silence the linter? However, this seems to go in the wrong direction. If we think our fs:: namespace should be preferred over the std:: fs one, we should try to use it as much as possible and only call std_path when needed. I guess restoring fs::path here would require some overloads in the wrapper class, or restoring some fs::path constructors below. But this is just a nit or follow-up idea, and maybe this was intentional and I am missing something.
It was intentional just to minimize verbosity of the code and size of the diff, but it's true this goes in direction of using std::filesystem::path instead of fs::path, and maybe we don't want that.
To me, it seems good to prefer using fs::path instead of std::filesystem::path, but using std::filesystem::path is not always avoidable anyway, and it is at least safe to use as long as not doing conversions to or from arbitrary char strings, so it's ok to tolerate in some places
re-ACK c864a4c1940d682f7eb6fdb3b91b18d638b59330 🌥
<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 c864a4c1940d682f7eb6fdb3b91b18d638b59330 🌥
0D0v6O8aoNqJXCgjkkFNkvQkFzEvahdV0ADufk2lsrbmdkWx9WCf6o0r/LORbJuEwFehUzATQ8meSoax1jCVBw==
</details>