test: Add case for libstdc++ bug on Windows #36620

The code in BerkeleyRODatabase::Backup() relies on copy_file() with overwrite_existing working.

Upstream fixes:

• https://github.com/gcc-mirror/gcc/commit/017e3f89b081e4828a588a3bd27b5feacea042b7
• https://github.com/gcc-mirror/gcc/commit/35c98149a5c7af9159fa7615d8d827b3aaa3cc97
• https://github.com/gcc-mirror/gcc/commit/8b3e47b4099f319e38d357f7c646f51040a13d6e

Suggested in #36271 (comment).

Here is an example of the failure when cross-compiling for Windows using GCC 14.2.0:

./test/fs_tests.cpp(145): Entering test case "copy_file_overwrite_existing"
unknown location(0): fatal error: in "fs_tests/copy_file_overwrite_existing": std::filesystem::__cxx11::filesystem_error: filesystem error: cannot copy file: File exists [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\fs_tests\copy_file_overwrite_existing\c72b38273c1f6fce4336\a] [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\fs_tests\copy_file_overwrite_existing\c72b38273c1f6fce4336\b]
./test/fs_tests.cpp(155): last checkpoint

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

External sites

• Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/36620
• Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/36620.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/36620#author-ralph

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Here is an example of the failure when cross-compiling for Windows using GCC 14.2.0...

This can be done, for instance, with the following patch:

--- a/contrib/devtools/shell-win64-cross.nix
+++ b/contrib/devtools/shell-win64-cross.nix
@@ -16,9 +16,18 @@ let
   });
   toolchain = crossPkgs.stdenv.cc.targetPrefix;
   pthreads = crossPkgs.windows.pthreads;
+  gccVersion = "14.2.0";
   crossGcc = crossPkgs.buildPackages.gcc14;
-  gcc = crossGcc.override {
+  gcc = (crossGcc.override {
     cc = crossGcc.cc.overrideAttrs (old: {
+      version = gccVersion;
+      name = builtins.replaceStrings [ old.version ] [ gccVersion ] old.name;
+      postPatch = builtins.replaceStrings [ old.version ] [ gccVersion ] old.postPatch;
+      configureFlags = map (builtins.replaceStrings [ old.version ] [ gccVersion ]) old.configureFlags;
+      src = pkgs.fetchurl {
+        url = "mirror://gcc/releases/gcc-${gccVersion}/gcc-${gccVersion}.tar.xz";
+        sha256 = "a7b39bc69cbf9e25826c5a60ab26477001f7c08d85cec04bc0e29cabed6f3cc9";
+      };
       # Keep target pthread headers out of GCC's native build tools.
       env = old.env // {
         EXTRA_FLAGS_FOR_TARGET =
@@ -28,7 +37,9 @@ let
       };
     });
     extraPackages = [ pthreads ];
-  };
+  }).overrideAttrs (_: {
+    version = gccVersion;
+  });
 in
 
 pkgs.mkShellNoCC {

Could address the Ralph comment?

Can you elaborate on the motivation here? Why this bug specifically? Which users (compiling in what currently used environments) do we expect to hit this?

Can you elaborate on the motivation here? Why this bug specifically? Which users (compiling in what currently used environments) do we expect to hit this?

Even after #36042, users can hit this bug in a build environment we don’t control (e.g. a distro’s g++-mingw-w64-ucrt64 package) that formally meets our minimum requirements but ships an affected libstdc++.

Seems harmless to add the test. An alternative could be to reject g++-mingw less than 14.3 during configure?

Could address the Ralph comment?

Done.

Even after #36042, users can hit this bug in a build environment we don’t control

Thanks:

Why this bug specifically?" "Which users (compiling in what currently used environments) do we expect to hit this?" ?

It's not clear that adding a test here is that useful, unless you expect cross-compiling end-users are running the unit test binary on their systems before running bitcoind? Otherwise, as mentioned above, maybe it'd be better to reject the compiler outright.

It's also not clear what runtime failure / bug the user would be prevented from hitting (by seeing this test failure) if they happened to be using a broken compiler. If I understand correctly, does this mean that migration has likely never worked on windows with GCC-based cross-compiled binaries, but also nobody has ever reported it?

There are lots of compiler bugs, and we don't add tests for many of them, and the PR description / commit messages here don't really justify the changes, or explain how a user would be affected by this at runtime, if they used a broken compiler.

I have to admit that I can’t see a real-life scenario in which the bug affects users.

The code is reachable only when migrating a legacy BDB wallet, either via the migratewallet RPC command or via the GUI.

The dest argument of BerkeleyRODatabase::Backup() is constructed in MigrateLegacyToDescriptor() and includes a timestamp. If a migration attempt fails for any reason, the next attempt is very likely to write a differently named file. However, the bug can only be hit if the backup file has the same name.

It seems to me this is either:

• A compiler config not worth worrying about
• A compiler config worth explicitly blocking as suggested by @maflcko
• A compiler config we're worried about, in which case we could just remove the copy_options param from copy_file so that we can't hit this anywhere else.

Either way, adding a test that fails where we expect it to fail doesn't seem very helpful to me.

... we could just remove the copy_options param from copy_file so that we can't hit this anywhere else. @achow101

Could we?