Reintroduce external signer support for Windows

hebasto commented at 6:06 pm on April 14, 2024: member

This PR partially reverts:

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

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
ACK	Sjors, 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.

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.

hebasto force-pushed on Apr 14, 2024

fanquake commented at 10:42 am on April 17, 2024: member

There are no conflicts with #29865.

Looks like there are now?

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.

hebasto force-pushed on Apr 23, 2024

hebasto commented at 5:35 pm on April 23, 2024: member

Rebased.

hebasto force-pushed on Apr 23, 2024

DrahtBot added the label CI failed on Apr 23, 2024

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

DrahtBot removed the label CI failed on Apr 23, 2024

laanwj added the label Windows on Apr 24, 2024

laanwj added the label Wallet on Apr 24, 2024

hebasto force-pushed on Apr 24, 2024

hebasto commented at 4:34 pm on April 24, 2024: member

Rebased on top of the merged #29910.

hebasto added the label Needs CMake port on Apr 24, 2024

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.

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…

DrahtBot added the label Needs rebase on Apr 28, 2024

hebasto force-pushed on Apr 28, 2024

hebasto commented at 5:30 am on April 28, 2024: member

Rebased to resolve conflicts with the merged bitcoin/bitcoin#29774.

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 :)

DrahtBot removed the label Needs rebase on Apr 28, 2024

theStack commented at 5:05 pm on April 30, 2024: contributor

Concept ACK

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.

hebasto force-pushed on May 2, 2024

hebasto commented at 8:46 pm on May 2, 2024: member

Rebased on top of the merged #29961.

Sjors commented at 11:50 am on May 6, 2024: member

Briefly retested 83c6332dc7b8145b19d725830b26dc4ef21e4b61 which still works.

DrahtBot added the label Needs rebase on May 7, 2024

hebasto force-pushed on May 7, 2024

hebasto commented at 8:07 pm on May 7, 2024: member

Rebased due to the conflict with merged bitcoin/bitcoin#29494.

DrahtBot removed the label Needs rebase on May 7, 2024

DrahtBot added the label CI failed on May 7, 2024

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

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      |       ^~~

hebasto force-pushed on May 8, 2024

DrahtBot removed the label CI failed on May 8, 2024

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)

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.

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.

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.

fanquake referenced this in commit b47c393d8a on May 11, 2024

hebasto force-pushed on May 11, 2024

hebasto commented at 10:54 am on May 11, 2024: member

Rebased due to the conflict with merged bitcoin/bitcoin#30081.

DrahtBot added the label Needs rebase on May 30, 2024

hebasto force-pushed on May 30, 2024

hebasto commented at 8:57 am on May 30, 2024: member

Rebased to resolve a conflict with bitcoin/bitcoin#30049.

DrahtBot removed the label Needs rebase on May 30, 2024

DrahtBot added the label CI failed on May 30, 2024

DrahtBot removed the label CI failed on May 30, 2024

DrahtBot added the label Needs rebase on Aug 28, 2024

maflcko removed the label Needs CMake port on Aug 29, 2024

hebasto force-pushed on Aug 30, 2024

DrahtBot added the label CI failed on Aug 30, 2024

hebasto force-pushed on Aug 30, 2024

DrahtBot removed the label Needs rebase on Aug 30, 2024

hebasto force-pushed on Aug 30, 2024

DrahtBot removed the label CI failed on Aug 31, 2024

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

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

hebasto force-pushed on Sep 4, 2024

hebasto marked this as a draft on Sep 4, 2024

hebasto force-pushed on Sep 4, 2024

DrahtBot added the label CI failed on Sep 4, 2024

hebasto marked this as ready for review on Sep 4, 2024

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

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?

luke-jr changes_requested

DrahtBot removed the label CI failed on Sep 5, 2024

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.

hebasto marked this as a draft on Sep 5, 2024

hebasto force-pushed on Sep 5, 2024

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.

DrahtBot added the label CI failed on Sep 6, 2024

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.

DrahtBot added the label Needs rebase on Sep 24, 2024

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.

in src/util/subprocess.h:79 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.

luke-jr changes_requested

luke-jr referenced this in commit 1a2cefdb80 on Feb 26, 2025

luke-jr referenced this in commit e2d768731e on Feb 26, 2025

luke-jr referenced this in commit 36f3c2f5c9 on Feb 26, 2025

luke-jr referenced this in commit ad3dedaacb on Feb 26, 2025

luke-jr referenced this in commit 8c8249b545 on Feb 26, 2025

luke-jr referenced this in commit 54570647ca on Feb 26, 2025

luke-jr referenced this in commit c3c6c5a2c4 on Feb 26, 2025

luke-jr referenced this in commit 11036defba on Apr 4, 2025

luke-jr referenced this in commit 735fdb7852 on Apr 4, 2025

luke-jr referenced this in commit e1a0e4c7d8 on Apr 4, 2025

luke-jr referenced this in commit 700f989eee on Apr 4, 2025

luke-jr referenced this in commit 58d701b683 on Apr 4, 2025

luke-jr referenced this in commit 24aa3b9077 on Apr 4, 2025

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

hebasto force-pushed on Apr 22, 2025

hebasto commented at 4:24 pm on April 22, 2025: member

Rebased.

Picked @luke-jr’s suggestion.

hebasto marked this as ready for review on Apr 22, 2025

DrahtBot removed the label Needs rebase on Apr 22, 2025

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.

DrahtBot removed the label CI failed on Apr 22, 2025

hebasto force-pushed on Apr 22, 2025

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

in src/util/subprocess.h:85 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.

in src/util/subprocess.h:1073 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) ?

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.

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.

hebasto force-pushed on Apr 23, 2025

hebasto force-pushed on Apr 27, 2025

hebasto commented at 4:30 pm on April 27, 2025: member

Rebased on #32358 and drafted until the latter is landed.

hebasto marked this as a draft on Apr 27, 2025

hebasto force-pushed on May 1, 2025

hebasto requested review from luke-jr on May 1, 2025

hebasto referenced this in commit 53ccb75f0c on May 5, 2025

test: Reintroduce Windows support in `system_tests/run_command` test 6e5fc2bf9b

build: Re-enable external signer support for Windows 719fa9f4ef

ci: Drop `-DENABLE_EXTERNAL_SIGNER=ON` configure option

This option is now enabled by default.

3a18075aed

hebasto force-pushed on May 5, 2025

hebasto marked this as ready for review on May 5, 2025

hebasto commented at 11:40 am on May 5, 2025: member

Rebased and undrafted.

Sjors commented at 12:40 pm on May 7, 2025: member

ACK 3a18075aedd7cff6f06b5fe10966d618b6378701.

Windows 11 against HWI 3.1.0 on testnet4.

It crashes immediately after creating a new wallet from a Trezor model T.

This still happens, but it seems only on testnet4, e.g. not on mainnet and not on signet (didn’t try testnet3). It also happens on macOS 15.4.1, so it’s clearly unrelated. Update: it’s because https://github.com/bitcoin-core/HWI/pull/771 isn’t in the latest HWI release yet (v3.1.0).

On Windows 11 I was able to create a new wallet with the GUI, verify an address and make a transaction.

I did not test handling errors from HWI.

hebasto commented at 12:45 pm on May 7, 2025: member

@Sjors

ACK 8b73630.

Thank you for your review!

Could you please double-check the commit hash in your comment?

DrahtBot requested review from theStack on May 7, 2025

Sjors commented at 12:46 pm on May 7, 2025: member

@hebasto fixed, that was from a different PR, but I checked that I used the right commit for my Windows guix build.

0e2f572ad6ac20ed0b66c2db7a81997cf9e090c695364d88fda1c183efe2a2cbd  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/SHA256SUMS.part
1886647c8c0322b1a450cc97efc1bf6a41b5efc8bf02477bb22a694cea1314a42  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-codesigning.tar.gz
24a82a01d9a12fee247e33973480183bf8a29cde28bb5bce2ab576e81f29b8d9c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-debug.zip
31cda05c5313630e81bdacd655104850243cb98b123ab23376f7b0570f50bcf2c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-setup-unsigned.exe
47c58ea2a69bd6e1bc27d56e208693d8873a365c69669396d7167d0c4d2c9275c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-unsigned.zip

hebasto commented at 12:48 pm on May 7, 2025: member

Friendly ping @achow101 @theStack @luke-jr @sipsorcery :)

hebasto commented at 1:01 pm on May 7, 2025: member

Can we set “30.0” milestone here?

hebasto commented at 3:55 pm on May 7, 2025: member

My Guix build:

0aarch64
16b1411a20a83a7c8b791ff9ff738748b92d1a86fc7ac2337257ce418e0b966ca  guix-build-3a18075aedd7/output/dist-archive/bitcoin-3a18075aedd7.tar.gz
2e2f572ad6ac20ed0b66c2db7a81997cf9e090c695364d88fda1c183efe2a2cbd  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/SHA256SUMS.part
3886647c8c0322b1a450cc97efc1bf6a41b5efc8bf02477bb22a694cea1314a42  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-codesigning.tar.gz
44a82a01d9a12fee247e33973480183bf8a29cde28bb5bce2ab576e81f29b8d9c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-debug.zip
51cda05c5313630e81bdacd655104850243cb98b123ab23376f7b0570f50bcf2c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-setup-unsigned.exe
67c58ea2a69bd6e1bc27d56e208693d8873a365c69669396d7167d0c4d2c9275c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-unsigned.zip

theStack approved

theStack commented at 2:37 pm on May 9, 2025: contributor

Light ACK 3a18075aedd7cff6f06b5fe10966d618b6378701

Cross-compiled for Windows from a Linux machine using mingw-w64 and ran the involved unit and functional tests on a Windows 10 machine (needed to adapt the paths ./test/config.ini manually on the target system, like also done in the CI job):

 0>.\build\bin\test_bitcoin.exe --run_test=system_tests/run_command
 1Running 1 test case...
 2
 3*** No errors detected
 4
 5>python.exe .\build\test\functional\rpc_signer.py
 62025-05-09T14:17:19.535000Z TestFramework (INFO): PRNG seed is: 1291717054418451415
 72025-05-09T14:17:19.596000Z TestFramework (INFO): Initializing test directory C:\Users\thestack\AppData\Local\Temp\bitcoin_func_test_6_lj29gg
 82025-05-09T14:17:21.525000Z TestFramework (INFO): Stopping nodes
 92025-05-09T14:17:21.750000Z TestFramework (INFO): Cleaning up C:\Users\thestack\AppData\Local\Temp\bitcoin_func_test_6_lj29gg on exit
102025-05-09T14:17:21.750000Z TestFramework (INFO): Tests successful
11
12>python.exe .\build\test\functional\wallet_signer.py
132025-05-09T14:17:38.230000Z TestFramework (INFO): PRNG seed is: 5844070588042404732
142025-05-09T14:17:38.280000Z TestFramework (INFO): Initializing test directory C:\Users\thestack\AppData\Local\Temp\bitcoin_func_test_w0lar663
152025-05-09T14:17:39.823000Z TestFramework (INFO): Test walletdisplayaddress
162025-05-09T14:17:41.079000Z TestFramework (INFO): Prepare mock PSBT
172025-05-09T14:17:42.235000Z TestFramework (INFO): Test send using hww1
182025-05-09T14:17:42.507000Z TestFramework (INFO): Test sendall using hww1
192025-05-09T14:17:42.771000Z TestFramework (INFO): Prepare fee bumped mock PSBT
202025-05-09T14:17:42.776000Z TestFramework (INFO): Test bumpfee using hww1
212025-05-09T14:17:43.746000Z TestFramework (INFO): Test invalid external signer
222025-05-09T14:17:44.668000Z TestFramework (INFO): Test multiple external signers
232025-05-09T14:17:44.881000Z TestFramework (INFO): Stopping nodes
242025-05-09T14:17:45.038000Z TestFramework (INFO): Cleaning up C:\Users\thestack\AppData\Local\Temp\bitcoin_func_test_w0lar663 on exit
252025-05-09T14:17:45.038000Z TestFramework (INFO): Tests successful

Didn’t try any real HWI interaction. I think a release note makes sense here so that Windows users are aware they can externally sign again. A counterpart to https://github.com/bitcoin/bitcoin/blob/b070ce16966fe18eb68f886ab9ca02d1d05a2f2a/doc/release-notes/release-notes-27.0.md?plain=1#L96-L101

hebasto added the label Needs release note on May 9, 2025

hebasto added this to the milestone 30.0 on May 9, 2025

Reintroduce external signer support for Windows #29868

Code Coverage & Benchmarks

Reviews

Conflicts