Reintroduce external signer support for Windows #29868

pull hebasto wants to merge 10 commits into bitcoin:master from hebasto:240414-win-subprocess changing 4 files +101 −59
  1. hebasto commented at 6:06 pm on April 14, 2024: member

    Based on #32358.

    Partially reverts:

    After this PR, we can proceed to actually remove the unused code from src/util/subprocess.hpp.

  2. DrahtBot commented at 6:06 pm on April 14, 2024: contributor

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

    Code Coverage & Benchmarks

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK theStack

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    No conflicts as of last run.

  3. in ci/test/00_setup_env_win64.sh:19 in cf6a4b2c0a outdated 
    15@@ -16,4 +16,4 @@ export GOAL="deploy"
16 # Prior to 11.0.0, the mingw-w64 headers were missing noreturn attributes, causing warnings when
17 # cross-compiling for Windows. https://sourceforge.net/p/mingw-w64/bugs/306/
18 # https://github.com/mingw-w64/mingw-w64/commit/1690994f515910a31b9fb7c7bd3a52d4ba987abe
19-export BITCOIN_CONFIG="--enable-reduce-exports --disable-gui-tests CXXFLAGS=-Wno-return-type"
20+export BITCOIN_CONFIG="--enable-reduce-exports --enable-external-signer --disable-gui-tests CXXFLAGS=-Wno-return-type"
    fanquake commented at 6:15 pm on April 14, 2024:

    ci: remove –enable-external-signer from win64 job

    Why this is being reverted? This is already applied globally to the CI (and I’d say pointlessly so).

    hebasto commented at 6:17 pm on April 14, 2024:
    Thanks! Dropped.
    fanquake commented at 6:19 pm on April 14, 2024:
    Great, can you remove it from the global config here as well. Given it’s not required or actually testing anything.
    hebasto commented at 7:54 pm on April 14, 2024:

    Great, can you remove it from the global config here as well. Given it’s not required or actually testing anything.

    I agree. Implemented.

  4. hebasto force-pushed on Apr 14, 2024
  5. fanquake commented at 10:42 am on April 17, 2024: member

    There are no conflicts with #29865.

    Looks like there are now?

  6. hebasto commented at 3:30 pm on April 18, 2024: member

    There are no conflicts with #29865.

    Looks like there are now?

    Well, there is a conflict in the code, which is being removed in #29865. It won’t be a problem for either PR if the other one goes first.

  7. hebasto force-pushed on Apr 23, 2024
  8. hebasto commented at 5:35 pm on April 23, 2024: member
    Rebased.
  9. hebasto force-pushed on Apr 23, 2024
  10. DrahtBot added the label CI failed on Apr 23, 2024
  11. DrahtBot commented at 7:48 pm on April 23, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/24165845040

  12. DrahtBot removed the label CI failed on Apr 23, 2024
  13. laanwj added the label Windows on Apr 24, 2024
  14. laanwj added the label Wallet on Apr 24, 2024
  15. hebasto force-pushed on Apr 24, 2024
  16. hebasto commented at 4:34 pm on April 24, 2024: member
    Rebased on top of the merged #29910.
  17. hebasto added the label Needs CMake port on Apr 24, 2024
  18. in src/util/subprocess.h:1015 in 0202082747 outdated 
    1007@@ -1012,7 +1008,7 @@ class Popen
1008 /*
1009   ~Popen()
1010   {
1011-#ifdef __USING_WINDOWS__
    Sjors commented at 8:47 am on April 26, 2024:
    #29961 removes this, that’s the only conflict I found.
  19. Sjors commented at 9:50 am on April 26, 2024: member

    Tested the Guix build on Windows 11.

    Code changes look reasonable, but Windows stuff is not my expertise…

  20. DrahtBot added the label Needs rebase on Apr 28, 2024
  21. hebasto force-pushed on Apr 28, 2024
  22. hebasto commented at 5:30 am on April 28, 2024: member
    Rebased to resolve conflicts with the merged bitcoin/bitcoin#29774.
  23. hebasto commented at 5:33 am on April 28, 2024: member

    cc @achow101 @theStack

    Code changes look reasonable, but Windows stuff is not my expertise…

    cc @sipsorcery :)

  24. DrahtBot removed the label Needs rebase on Apr 28, 2024
  25. theStack commented at 5:05 pm on April 30, 2024: contributor
    Concept ACK
  26. in src/test/system_tests.cpp:34 in db73ac8c51 outdated 
    28@@ -29,25 +29,44 @@ BOOST_AUTO_TEST_CASE(dummy)
29 
30 BOOST_AUTO_TEST_CASE(run_command)
31 {
32+#ifdef WIN32
33+    // https://www.winehq.org/pipermail/wine-devel/2008-September/069387.html
34+    auto hntdll = GetModuleHandleA("ntdll.dll");
35+    assert(hntdll);
36+    const bool wine_runtime = GetProcAddress(hntdll, "wine_get_version");
    sipsorcery commented at 6:55 pm on May 2, 2024:
    This check will fail on Windows. I’m guessing that’s expected but just checking?
    hebasto commented at 7:07 pm on May 2, 2024:
    The goal is to detect that binaries, which are cross-compiled for Windows, are running in Wine.
    sipsorcery commented at 7:31 pm on May 2, 2024:
    Does that mean this PR is intended to be a fix for Windows or for wine?
    hebasto commented at 8:30 pm on May 2, 2024:
    For both. Wine is used to run test suite on Linux after cross-compiling for Windows. This need roots from the fact that Wine and Windows runtimes produce different error messages: https://github.com/bitcoin/bitcoin/blob/db73ac8c517f91ed5175b3635eb37ed56852ff91/src/test/system_tests.cpp#L80
    hebasto commented at 8:32 pm on May 2, 2024:
    FWIW, the same behaviour was before #29489.
  27. hebasto force-pushed on May 2, 2024
  28. hebasto commented at 8:46 pm on May 2, 2024: member
    Rebased on top of the merged #29961.
  29. Sjors commented at 11:50 am on May 6, 2024: member
    Briefly retested 83c6332dc7b8145b19d725830b26dc4ef21e4b61 which still works.
  30. DrahtBot added the label Needs rebase on May 7, 2024
  31. hebasto force-pushed on May 7, 2024
  32. hebasto commented at 8:07 pm on May 7, 2024: member
    Rebased due to the conflict with merged bitcoin/bitcoin#29494.
  33. DrahtBot removed the label Needs rebase on May 7, 2024
  34. DrahtBot added the label CI failed on May 7, 2024
  35. DrahtBot commented at 9:29 pm on May 7, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin/bitcoin/runs/24699627882

  36. fanquake commented at 3:21 am on May 8, 2024: member

    https://github.com/bitcoin/bitcoin/pull/29868/checks?check_run_id=24699627882:

    0In file included from common/run_command.cpp:13:
1./util/subprocess.h: In member function ‘int subprocess::Popen::wait()’:
2./util/subprocess.h:1062:7: error: unused variable ‘ret’ [-Werror=unused-variable]
3 1062 |   int ret = WaitForSingleObject(process_handle_, INFINITE);
4      |       ^~~
  37. hebasto force-pushed on May 8, 2024
  38. DrahtBot removed the label CI failed on May 8, 2024
  39. Sjors commented at 7:07 am on May 9, 2024: member

    I assume you added b69b9e85e98cac2c7585c9b613185dc6c80320cc because it no longer uses an external dependency?

    Is there something you can remove from cpp-subprocess based on your comment? #29961 (comment)

  40. fanquake commented at 8:18 am on May 9, 2024: member

    I assume you added https://github.com/bitcoin/bitcoin/commit/b69b9e85e98cac2c7585c9b613185dc6c80320cc because it no longer uses an external dependency?

    It’s there because setting something that is already on, to on, is pointless, and doesn’t test anything. In either case, external signer is also not special, so it’s not clear why, as a feature, it’s being explcitly enabled in the global CI config.

  41. hebasto commented at 10:20 am on May 9, 2024: member

    @Sjors

    Is there something you can remove from cpp-subprocess based on your comment? #29961 (comment)

    Popen::poll() and Popen::pid() might go.

  42. hebasto commented at 10:28 am on May 9, 2024: member

    In either case, external signer is also not special…

    At some point, we might want to get rid of the #ifdef ENABLE_EXTERNAL_SIGNER all together.

  43. fanquake referenced this in commit b47c393d8a on May 11, 2024
  44. hebasto force-pushed on May 11, 2024
  45. hebasto commented at 10:54 am on May 11, 2024: member
    Rebased due to the conflict with merged bitcoin/bitcoin#30081.
  46. DrahtBot added the label Needs rebase on May 30, 2024
  47. hebasto force-pushed on May 30, 2024
  48. hebasto commented at 8:57 am on May 30, 2024: member
    Rebased to resolve a conflict with bitcoin/bitcoin#30049.
  49. DrahtBot removed the label Needs rebase on May 30, 2024
  50. DrahtBot added the label CI failed on May 30, 2024
  51. DrahtBot removed the label CI failed on May 30, 2024
  52. DrahtBot added the label Needs rebase on Aug 28, 2024
  53. maflcko removed the label Needs CMake port on Aug 29, 2024
  54. hebasto force-pushed on Aug 30, 2024
  55. hebasto force-pushed on Aug 30, 2024
  56. DrahtBot added the label CI failed on Aug 30, 2024
  57. hebasto force-pushed on Aug 30, 2024
  58. DrahtBot removed the label Needs rebase on Aug 30, 2024
  59. hebasto force-pushed on Aug 30, 2024
  60. DrahtBot removed the label CI failed on Aug 31, 2024
  61. in src/test/system_tests.cpp:78 in dc08930ec6 outdated 
    72@@ -54,8 +73,13 @@ BOOST_AUTO_TEST_CASE(run_command)
73     }
74     {
75         // Return non-zero exit code, with error message for stderr
76+#ifdef WIN32
77+        const std::string command{"cmd.exe /c dir nosuchfile"};
78+        const std::string expected{wine_runtime ? "File not found." : "File Not Found"};
    maflcko commented at 7:37 am on September 2, 2024:

    Not sure. Won’t this lead to more issue of the kind of #30608? If yes, it would be good to fix this first, and then extend the tests.

    Why not simply call python with python -c 'import sys; print("err", file=sys.stderr); sys.exit(2)'?

    maflcko commented at 6:50 pm on September 3, 2024:
    Fixed in https://github.com/bitcoin/bitcoin/pull/30788
  62. Sjors commented at 11:47 am on September 2, 2024: member

    Tested dc08930ec68b465420cc5e1157e532fcb5cf8c87 again on Windows.

    x86_64 guix hashes

     001d11755aaf73fd378b563afa5f54286fbe9c8e2a3b6259a61d21b7cbb5bf9f4  guix-build-dc08930ec68b/output/aarch64-linux-gnu/SHA256SUMS.part
 12baf27e4f865ed23b97d7f353bbcf6f8fd5629c8d7cec26ee02b048406d3418b  guix-build-dc08930ec68b/output/aarch64-linux-gnu/bitcoin-dc08930ec68b-aarch64-linux-gnu-debug.tar.gz
 2a686ed4a50b371a3ef2d20791e4fdf1f1258404f1012efc132a906808835628b  guix-build-dc08930ec68b/output/aarch64-linux-gnu/bitcoin-dc08930ec68b-aarch64-linux-gnu.tar.gz
 3dbea3292cca17491164781477fdc6c34e189456156aecc58cfe07763989a09b9  guix-build-dc08930ec68b/output/arm-linux-gnueabihf/SHA256SUMS.part
 485f6d8f7377f1210c460b49766fe29075dbd54c4ab0a23c93d7d81605e82cf58  guix-build-dc08930ec68b/output/arm-linux-gnueabihf/bitcoin-dc08930ec68b-arm-linux-gnueabihf-debug.tar.gz
 538730c75787366e4b9719123bcac53db01cce8c22701bc994a8e34b5d8c17415  guix-build-dc08930ec68b/output/arm-linux-gnueabihf/bitcoin-dc08930ec68b-arm-linux-gnueabihf.tar.gz
 66ce75b2084be1e1afcddc3353ba67b5f848bbeb24639beba3f5e82ba4d79679f  guix-build-dc08930ec68b/output/arm64-apple-darwin/SHA256SUMS.part
 7bfb4d7c4d12a8bd24c1d3d0806c3ef41693211efa991138f92cc47e6b81f0b85  guix-build-dc08930ec68b/output/arm64-apple-darwin/bitcoin-dc08930ec68b-arm64-apple-darwin-unsigned.tar.gz
 8673bfb079316b5f793401f10038739fe7947f33c41013ebc1cfbea53ee6f6949  guix-build-dc08930ec68b/output/arm64-apple-darwin/bitcoin-dc08930ec68b-arm64-apple-darwin-unsigned.zip
 94f5884559b3f880e7152b6b320b8f400079ee9ec413caf25ddc682c4a489e691  guix-build-dc08930ec68b/output/arm64-apple-darwin/bitcoin-dc08930ec68b-arm64-apple-darwin.tar.gz
107e60b134b8b59271cc4df1b31506f73ee02bf19e28207e40989190f18e4bb68d  guix-build-dc08930ec68b/output/dist-archive/bitcoin-dc08930ec68b.tar.gz
11f94e934d10242c09488235fdfd3ce14b4ed9fa3148c75e33fa1e4cdf59db6cd1  guix-build-dc08930ec68b/output/powerpc64-linux-gnu/SHA256SUMS.part
1280562f673316e701ed3b601fc0fb8f9af39eb1e7d17edbdc436db6e108bc8031  guix-build-dc08930ec68b/output/powerpc64-linux-gnu/bitcoin-dc08930ec68b-powerpc64-linux-gnu-debug.tar.gz
13967d0b7d316d19a3bc7496aaa0162c65c235bdd14b490786f45026b8ad08cb4d  guix-build-dc08930ec68b/output/powerpc64-linux-gnu/bitcoin-dc08930ec68b-powerpc64-linux-gnu.tar.gz
144bc31242d17bc8653ab647699f101b3073c1e364ce2d2a8685694fa4df39948f  guix-build-dc08930ec68b/output/riscv64-linux-gnu/SHA256SUMS.part
158fe6e6def3df24dcd20b6bd52857fbb28c84753a3db2816522521ab7787c97b7  guix-build-dc08930ec68b/output/riscv64-linux-gnu/bitcoin-dc08930ec68b-riscv64-linux-gnu-debug.tar.gz
16cea272797e4f1528903efe6b56d2fc77cf3564e67133a9bc674169f0cac13d98  guix-build-dc08930ec68b/output/riscv64-linux-gnu/bitcoin-dc08930ec68b-riscv64-linux-gnu.tar.gz
17312ac2af7056f6b807d5143be9f4814bbbe2f947e2f09cd41b066ebf74879861  guix-build-dc08930ec68b/output/x86_64-apple-darwin/SHA256SUMS.part
187d71cd457dc5d85ec9c38fe9779535c5a70fb9a3420a06719be16b3d265174f9  guix-build-dc08930ec68b/output/x86_64-apple-darwin/bitcoin-dc08930ec68b-x86_64-apple-darwin-unsigned.tar.gz
19e055caed7299e4dbc9dc7e03f19d4a3e92224099223760e49e66b9c9186953fb  guix-build-dc08930ec68b/output/x86_64-apple-darwin/bitcoin-dc08930ec68b-x86_64-apple-darwin-unsigned.zip
207b776bef6e3e99bf124b03c074649f9a82881afd84db2c3baed063996e57149b  guix-build-dc08930ec68b/output/x86_64-apple-darwin/bitcoin-dc08930ec68b-x86_64-apple-darwin.tar.gz
217a138f4fa30b3654a21ecc4db58788eeaf91d053c3bb5b8e991c0cc40578212d  guix-build-dc08930ec68b/output/x86_64-linux-gnu/SHA256SUMS.part
2235435eeddcd6b5185b6f8a5cf665ca143c9b0e1b38d7a30cbb6fe4e04d92e861  guix-build-dc08930ec68b/output/x86_64-linux-gnu/bitcoin-dc08930ec68b-x86_64-linux-gnu-debug.tar.gz
235ded07c774f55349ea892ed030acbe11bd5823b2956bf08f7aac43abd66b39f2  guix-build-dc08930ec68b/output/x86_64-linux-gnu/bitcoin-dc08930ec68b-x86_64-linux-gnu.tar.gz
240c4dd1a1c1cbe8def8cb13240b0456d4d094c9639d6533de530d606b4f3b2586  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/SHA256SUMS.part
2559c27d3edc82b84cff8af895ebf80b250a9e3b0c2ef08b5a1cb1720fbe913113  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/bitcoin-dc08930ec68b-win64-debug.zip
26f533c0800645b8704c528bd3f13d65558964aeff820dc8ff21360b1847dc774b  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/bitcoin-dc08930ec68b-win64-setup-unsigned.exe
27030082937bb216365d5bfea074ae8ba3290268d122718cc04f675bc6c0881f7d  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/bitcoin-dc08930ec68b-win64-unsigned.tar.gz
288d6f24a1aaf2079a3c2e5562678bfd8513d741b6b27ef5432b7b78b4325def91  guix-build-dc08930ec68b/output/x86_64-w64-mingw32/bitcoin-dc08930ec68b-win64.zip
  63. hebasto force-pushed on Sep 4, 2024
  64. hebasto marked this as a draft on Sep 4, 2024
  65. hebasto force-pushed on Sep 4, 2024
  66. DrahtBot added the label CI failed on Sep 4, 2024
  67. hebasto marked this as ready for review on Sep 4, 2024
  68. in src/test/system_tests.cpp:29 in 53d426e563 outdated 
    38         const UniValue result = RunCommandParseJSON("");
39         BOOST_CHECK(result.isNull());
40     }
41     {
42+#ifdef WIN32
43+        const UniValue result = RunCommandParseJSON("cmd.exe /c echo {\"success\": true}");
    luke-jr commented at 11:40 pm on September 4, 2024:
    Why is an extra cmd.exe needed?
    hebasto commented at 6:44 pm on September 5, 2024:

    It is not “extra”, but required. Consider this output:

    0C:\Users\hebasto>where echo
1INFO: Could not find files for the given pattern(s).
2
3C:\Users\hebasto>where cmd
4C:\Windows\System32\cmd.exe
  69. in src/test/system_tests.cpp:83 in 53d426e563 outdated 
    78+        std::string expected;
79+        if (wine_runtime) {
80+            command = "cmd.exe /c dir nosuchfile";
81+            expected = "File not found.";
82+        } else {
83+            command = "py -3 -c 'import sys; print(\"err\", file=sys.stderr); sys.exit(2)'";
    luke-jr commented at 11:41 pm on September 4, 2024:
    Seems like echo err 1>&2 should be portable?
  70. luke-jr changes_requested
  71. DrahtBot removed the label CI failed on Sep 5, 2024
  72. in src/test/system_tests.cpp:81 in 53d426e563 outdated 
    72@@ -54,8 +73,20 @@ BOOST_AUTO_TEST_CASE(run_command)
73     }
74     {
75         // Return non-zero exit code, with error message for stderr
76+#ifdef WIN32
77+        std::string command;
78+        std::string expected;
79+        if (wine_runtime) {
80+            command = "cmd.exe /c dir nosuchfile";
81+            expected = "File not found.";
    maflcko commented at 4:56 am on September 5, 2024:
    Please don’t re-introduce https://github.com/bitcoin/bitcoin/issues/30608
    hebasto commented at 4:27 pm on April 22, 2025:
    Reworked.
  73. hebasto marked this as a draft on Sep 5, 2024
  74. hebasto force-pushed on Sep 5, 2024
  75. hebasto force-pushed on Sep 5, 2024
  76. DrahtBot commented at 1:20 am on September 6, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29748644551

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  77. DrahtBot added the label CI failed on Sep 6, 2024
  78. in src/test/system_tests.cpp:85 in 5541ef02f7 outdated 
    81+            command = "cmd.exe /c \"echo err 1>&2 && exit 1\"";
82+            expected = "err";
83+        } else {
84+            command = "cmd.exe /c \"echo err 1>&2 && exit 1\"";
85+            expected = "err";
86+        }
    luke-jr commented at 4:50 pm on September 6, 2024:
    These are identical now. wine_runtime seems no longer needed at all?
    hebasto commented at 4:25 pm on April 22, 2025:
    Thanks! Dropped.
  79. DrahtBot added the label Needs rebase on Sep 24, 2024
  80. Sjors commented at 10:49 am on January 16, 2025: member
    Can you rebase and address @luke-jr’s comment? I’m happy to retest after that.
  81. in src/util/subprocess.h:75 in 5541ef02f7 outdated 
    69-  #include <Windows.h>
70+#ifdef WIN32
71+  #include <windows.h>
72   #include <io.h>
73   #include <cwchar>
74
    luke-jr commented at 5:06 pm on February 25, 2025:
    Just below here… the #define for close/open/fileno breaks using those names in other contexts (eg, C++ stdlib includes, potentially boost, whatever file includes this one, etc). At a minimum, we should probably define them as subprocess_(open|close|fileno) and define those to open/close/fileno for non-Windows too.
    hebasto commented at 4:25 pm on April 22, 2025:
    Thanks! Your commit has been picked.
  82. luke-jr changes_requested
  83. luke-jr referenced this in commit 1a2cefdb80 on Feb 26, 2025
  84. luke-jr referenced this in commit e2d768731e on Feb 26, 2025
  85. luke-jr referenced this in commit 36f3c2f5c9 on Feb 26, 2025
  86. luke-jr referenced this in commit ad3dedaacb on Feb 26, 2025
  87. luke-jr referenced this in commit 8c8249b545 on Feb 26, 2025
  88. luke-jr referenced this in commit 54570647ca on Feb 26, 2025
  89. luke-jr referenced this in commit c3c6c5a2c4 on Feb 26, 2025
  90. luke-jr referenced this in commit 11036defba on Apr 4, 2025
  91. luke-jr referenced this in commit 735fdb7852 on Apr 4, 2025
  92. luke-jr referenced this in commit e1a0e4c7d8 on Apr 4, 2025
  93. luke-jr referenced this in commit 700f989eee on Apr 4, 2025
  94. luke-jr referenced this in commit 58d701b683 on Apr 4, 2025
  95. luke-jr referenced this in commit 24aa3b9077 on Apr 4, 2025
  96. luke-jr commented at 8:57 pm on April 4, 2025: member
    Feel free to clone/squash rebasing and fixes from: https://github.com/bitcoin/bitcoin/compare/master...luke-jr:bitcoin:hww_windows-29
  97. hebasto force-pushed on Apr 22, 2025
  98. hebasto commented at 4:24 pm on April 22, 2025: member

    Rebased.

    Picked @luke-jr’s suggestion.

  99. hebasto marked this as ready for review on Apr 22, 2025
  100. DrahtBot removed the label Needs rebase on Apr 22, 2025
  101. in src/test/system_tests.cpp:86 in ed41f613a7 outdated 
    82         BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, HasReason("Unable to parse JSON: {"));
83     }
84-    // Test std::in
85+#ifndef WIN32
86     {
87+        // Test std::in
    maflcko commented at 5:32 pm on April 22, 2025:
    std::in → stdin
    hebasto commented at 10:02 pm on April 22, 2025:
    Thanks! Done.
  102. DrahtBot removed the label CI failed on Apr 22, 2025
  103. hebasto force-pushed on Apr 22, 2025
  104. Sjors commented at 9:51 am on April 23, 2025: member

    Tested 86c7c65e2fe7a33f47e08d7c344dbee44ea1e379 on Windows 11 against HWI 3.1.0 on testnet4.

    It crashes immediately after creating a new wallet from a Trezor model T. The debug log shows nothing useful, except that it didn’t import descriptors. cc @achow101

    02025-04-23T09:50:08Z Using SQLite Version 3.46.1
12025-04-23T09:50:08Z Using wallet C:\Users\sjors\AppData\Roaming\Bitcoin\testnet4\wallets\TrezorT
22025-04-23T09:50:08Z init message: Loading wallet…
32025-04-23T09:50:08Z [TrezorT] Wallet file version = 10500, last client version = 299900
42025-04-23T09:50:08Z [TrezorT] Legacy Wallet Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total.
52025-04-23T09:50:08Z [TrezorT] Descriptors: 0, Descriptor Keys: 0 plaintext, 0 encrypted, 0 total.
62025-04-23T09:50:08Z [TrezorT] Setting minversion to 169900

    The output of hwi.exe --fingerprint=.... getdescriptors looks fine. Since HWI constructs the descriptors, it’s probably not a device specific error, though I didn’t test with other devices.

    The wallet ends up blank with no descriptors.

    Guix hashes:

    09f6a6c496222c312ec6a53e870a8ff53d8374142544e6b77bb2428c42d49cfbc  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/SHA256SUMS.part
14ad397b829a5f011b7f72dce2448755123ae11042ebb80e5e292f8078b494420  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/bitcoin-86c7c65e2fe7-win64-codesigning.tar.gz
2e51655e88f45c9adb851125d8242f63dbddc0bc8164ab1e8e9a5ffaa5aa67100  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/bitcoin-86c7c65e2fe7-win64-debug.zip
39d6e6d2794defe412b1608bac2269b62284ac0d093479612821e98328984c5ab  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/bitcoin-86c7c65e2fe7-win64-setup-unsigned.exe
4c25f8353f4e9bdfeb7e45853881e3e34027847d55ba5003b2f92c85ef47f16be  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/bitcoin-86c7c65e2fe7-win64-unsigned.zip
  105. in src/util/subprocess.h:81 in 86c7c65e2f outdated 
    73   #include <cwchar>
74 
75-  #define close _close
76-  #define open _open
77-  #define fileno _fileno
78+  #define subprocess_close _close
    fanquake commented at 9:58 am on April 23, 2025:
    In c475135c07e588f656090664c7e48c8a73f2305c: Is this going to be upstreamed?
    hebasto commented at 4:01 pm on April 24, 2025:
    Upstreamed in https://github.com/arun11299/cpp-subprocess/pull/112.
  106. in src/util/subprocess.h:1059 in 86c7c65e2f outdated 
    1044+#ifdef WIN32
1045   int ret = WaitForSingleObject(process_handle_, INFINITE);
1046+  if (ret != WAIT_OBJECT_0) return -1;
1047 
1048-  return 0;
1049+  DWORD dretcode_;
    fanquake commented at 9:59 am on April 23, 2025:
    In ae3ddd322ca61f6bc7c4f5d6c372057069e6c5ea: is this going to be upstreamed / aligned with the upstream code (https://github.com/arun11299/cpp-subprocess/pull/109) ?
  107. in src/util/subprocess.h:172 in 86c7c65e2f outdated 
    168@@ -169,7 +169,7 @@ namespace util
169     //
170 
171     if (force == false && argument.empty() == false &&
172-        argument.find_first_of(L" \t\n\v\"") == argument.npos) {
    fanquake commented at 10:00 am on April 23, 2025:
    In ce950ec20fa0d246d2fd44f562a864d3ff83d399 “Fix quote issue on Windows “: This isn’t in a Windows block, so I’m guessing it doesn’t have any effect on other OS’s? Is this also going to be upstreamed?
    hebasto commented at 4:00 pm on April 24, 2025:
    Upstreamed in https://github.com/arun11299/cpp-subprocess/pull/113.
  108. in CMakeLists.txt:441 in 86c7c65e2f outdated 
    437@@ -438,6 +438,7 @@ if(MSVC)
438   try_append_cxx_flags("/wd4715" TARGET warn_interface SKIP_LINK)
439   try_append_cxx_flags("/wd4805" TARGET warn_interface SKIP_LINK)
440   target_compile_definitions(warn_interface INTERFACE
441+    _CRT_NONSTDC_NO_WARNINGS
    fanquake commented at 10:03 am on April 23, 2025:
    In ce9d47987be2b6a72fc87cc42a1c31a6f1766d52: This is adding suppressions for warnings but doesn’t mention what they are, or explain why we aren’t fixing the code?
    hebasto commented at 9:57 pm on April 23, 2025:
    Reworked and upstreamed the fix in https://github.com/arun11299/cpp-subprocess/pull/112.
  109. hebasto force-pushed on Apr 23, 2025
  110. subprocess: Fix string_arg when used with rref 
    When passing in a rvalue reference, compiler
considers it ambiguous between std::string and
std::string&&. Making one of them take a lvalue
reference makes compilers correctly pick the right
one depending on whether the passed in value binds
to a rvalue or lvalue reference.

Github-Pull: arun11299/cpp-subprocess#110
Rebased-From: 2d8a8eebb03e509840e2c3c755d1abf32d930f33
    9888d0f511
  111. subprocess: Get Windows return code in wait() 
    Currently, wait() returns 0 on windows regardless
of the actual return code of processes.

Github-Pull: arun11299/cpp-subprocess#109
Rebased-From: 04b015a8e52ead4d8bb5f0eb486419c77e418a17
    9e8992bf8b
  112. subprocess: Fix cross-compiling with mingw toolchain 
    Github-Pull: arun11299/cpp-subprocess#99
Rebased-From: 34033d03deacfdba892a708b7d8092b4d9e5e889
    7ecb4e430a
  113. subprocess: Avoid leaking POSIX name aliases beyond `subprocess.h` 
    The commit a32c0f3df4b6bcd1d2e93f19e8f380bb890cd507 introduced code to
silence MSVC's "warning C4996: The POSIX name for this item is
deprecated."

However, it exhibits several issues:
1. The aliases may leak into code outside the `subprocess.hpp` header.
2. They are unnecessarily applied when using the MinGW-w64 toolchain.
3. The fix is incomplete: downstream projects still see C4996 warnings.
4. The implementation lacks documentation.

This change addresses all of the above shortcomings.

Github-Pull: arun11299/cpp-subprocess#112
Rebased-From: 778543b2f2ca7f5d1c4f0241b635bbb265d750dd

Co-authored-by: Luke Dashjr <luke-jr+git@utopios.org>
    df7a52241f
  114. subprocess: Do not escape double quotes for command line arguments on Windows 
    * refactor: Guard `util::quote_argument()` with `#ifdef __USING_WINDOWS__`

The `util::quote_argument()` function is specific to Windows and is used
in code already guarded by `#ifdef __USING_WINDOWS__`.

* Do not escape double quotes for command line arguments on Windows

This change fixes the handling of double quotes and aligns the behavior
with Python's `Popen` class. For example:
```
>py -3
>>> import subprocess
>>> p = subprocess.Popen("cmd.exe /c dir \"C:\\Program Files\"", stdout=subprocess.PIPE, text=True)
>>> print(f"Captured stdout:\n{stdout}")
```

Currently, the same command line processed by the `quote_argument()`
function looks like `cmd.exe /c dir "\"C:\Program" "Files\""`, which is
broken.

With this change, it looks correct: `cmd.exe /c dir "C:\Program Files"`.

Github-Pull: arun11299/cpp-subprocess#113
Rebased-From: ed313971c04ac10dc006104aba07d016ffc6542a
    bf8230d525
  115. subprocess: Proper implementation of wait() on Windows 
    This commit makes sure:
1. WaitForSingleObject returns with expected
code before proceeding.
2. Process handle is properly closed.

Github-Pull: arun11299/cpp-subprocess#116
Rebased-From: 625a8775791e62736f20f3fa3e6cc4f1b24aa89a
    e25ec7300c
  116. subprocess, refactor: Replace custom `__USING_WINDOWS__` with `WIN32` 
    The `WIN32` macro is used across the entire code base to designate
building for Windows.
    7835579d6d
  117. test: Reintroduce Windows support in `system_tests/run_command` test 15e5125258
  118. build: Re-enable external signer support for Windows c008b87fec
  119. ci: Drop `-DENABLE_EXTERNAL_SIGNER=ON` configure option 
    This option is now enabled by default.
    7806529dd4
  120. hebasto force-pushed on Apr 27, 2025
  121. hebasto commented at 4:30 pm on April 27, 2025: member
    Rebased on #32358 and drafted until the latter is landed.
  122. hebasto marked this as a draft on Apr 27, 2025


hebasto DrahtBot fanquake Sjors theStack sipsorcery maflcko luke-jr

Labels
Wallet Windows

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: 2025-04-28 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me