Reintroduce external signer support for Windows by hebasto · Pull Request #29868 · bitcoin/bitcoin

hebasto · 2024-04-14T18:06:35Z

This PR partially reverts:

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

DrahtBot · 2024-04-14T18:06:38Z

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, laanwj

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32566 (Use subprocess library for notifications by laanwj)

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.

fanquake · 2024-04-17T10:42:24Z

There are no conflicts with #29865.

Looks like there are now?

hebasto · 2024-04-18T15:30:49Z

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 · 2024-04-23T17:35:34Z

Rebased.

DrahtBot · 2024-04-23T19:48:59Z

🚧 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}

hebasto · 2024-04-24T16:34:21Z

Rebased on top of the merged #29910.

Sjors · 2024-04-26T08:47:55Z

 /*
  ~Popen()
  {
-#ifdef __USING_WINDOWS__


#29961 removes this, that's the only conflict I found.

Sjors · 2024-04-26T09:50:23Z

Tested the Guix build on Windows 11.

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

hebasto · 2024-04-28T05:30:11Z

Rebased to resolve conflicts with the merged #29774.

hebasto · 2024-04-28T05:33:11Z

cc @achow101 @theStack

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

cc @sipsorcery :)

theStack · 2024-04-30T17:05:48Z

Concept ACK

hebasto · 2025-04-22T16:24:35Z

Rebased.

Picked @luke-jr's suggestion.

Sjors · 2025-04-23T09:51:10Z

Tested 86c7c65 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

2025-04-23T09:50:08Z Using SQLite Version 3.46.1
2025-04-23T09:50:08Z Using wallet C:\Users\sjors\AppData\Roaming\Bitcoin\testnet4\wallets\TrezorT
2025-04-23T09:50:08Z init message: Loading walletâ€¦
2025-04-23T09:50:08Z [TrezorT] Wallet file version = 10500, last client version = 299900
2025-04-23T09:50:08Z [TrezorT] Legacy Wallet Keys: 0 plaintext, 0 encrypted, 0 w/ metadata, 0 total.
2025-04-23T09:50:08Z [TrezorT] Descriptors: 0, Descriptor Keys: 0 plaintext, 0 encrypted, 0 total.
2025-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:

9f6a6c496222c312ec6a53e870a8ff53d8374142544e6b77bb2428c42d49cfbc  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/SHA256SUMS.part
4ad397b829a5f011b7f72dce2448755123ae11042ebb80e5e292f8078b494420  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/bitcoin-86c7c65e2fe7-win64-codesigning.tar.gz
e51655e88f45c9adb851125d8242f63dbddc0bc8164ab1e8e9a5ffaa5aa67100  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/bitcoin-86c7c65e2fe7-win64-debug.zip
9d6e6d2794defe412b1608bac2269b62284ac0d093479612821e98328984c5ab  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/bitcoin-86c7c65e2fe7-win64-setup-unsigned.exe
c25f8353f4e9bdfeb7e45853881e3e34027847d55ba5003b2f92c85ef47f16be  guix-build-86c7c65e2fe7/output/x86_64-w64-mingw32/bitcoin-86c7c65e2fe7-win64-unsigned.zip

hebasto · 2025-04-27T16:30:25Z

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

cd95c9d subprocess: check and handle fcntl(F_GETFD) failure (Tomás Andróil) b7288de subprocess: Proper implementation of wait() on Windows (Haowen Liu) 7423214 subprocess: Do not escape double quotes for command line arguments on Windows (Hennadii Stepanov) bb9ffea subprocess: Explicitly define move constructor of Streams class (Shunsuke Shimizu) 174bd43 subprocess: Avoid leaking POSIX name aliases beyond `subprocess.h` (Hennadii Stepanov) 7997b76 subprocess: Fix cross-compiling with mingw toolchain (Hennadii Stepanov) 6476304 subprocess: Get Windows return code in wait() (Haowen Liu) d3f511b subprocess: Fix string_arg when used with rref (Haowen Liu) 2fd3f2f subprocess: Fix memory leaks (Haoran Peng) Pull request description: Most of these changes were developed during work on #29868 and #32342 and have since been upstreamed. As they are now merged, this PR backports them to our `src/util/subprocess.h` header. Required for #29868. A list of the backported PRs: - arun11299/cpp-subprocess#106 - arun11299/cpp-subprocess#110 - arun11299/cpp-subprocess#109 - arun11299/cpp-subprocess#99 - arun11299/cpp-subprocess#112 - arun11299/cpp-subprocess#107 - arun11299/cpp-subprocess#113 - arun11299/cpp-subprocess#116 - arun11299/cpp-subprocess#117 The following PRs were skipped for backporting: - arun11299/cpp-subprocess#108 because we are not planning to support this feature. - arun11299/cpp-subprocess#101 because that change has been already landed in #29849. ACKs for top commit: theStack: Light ACK cd95c9d laanwj: Code review re-ACK cd95c9d Tree-SHA512: f9b60b932957d2e1cad1d87f2ad8bb68c97136e9735eb78547018a42cc50c4652750367f29462eadb0512c27db1dd8a7d4b17a2f0aeab62b3dbf86db5f51a61c

This option is now enabled by default.

hebasto · 2025-05-05T11:40:09Z

Rebased and undrafted.

Sjors · 2025-05-07T12:40:42Z

ACK 3a18075.

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 bitcoin-core/HWI#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 · 2025-05-07T12:45:14Z

@Sjors

ACK 8b73630.

Thank you for your review!

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

Sjors · 2025-05-07T12:46:50Z

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

e2f572ad6ac20ed0b66c2db7a81997cf9e090c695364d88fda1c183efe2a2cbd  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/SHA256SUMS.part
886647c8c0322b1a450cc97efc1bf6a41b5efc8bf02477bb22a694cea1314a42  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-codesigning.tar.gz
4a82a01d9a12fee247e33973480183bf8a29cde28bb5bce2ab576e81f29b8d9c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-debug.zip
1cda05c5313630e81bdacd655104850243cb98b123ab23376f7b0570f50bcf2c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-setup-unsigned.exe
7c58ea2a69bd6e1bc27d56e208693d8873a365c69669396d7167d0c4d2c9275c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-unsigned.zip

hebasto · 2025-05-07T12:48:05Z

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

hebasto · 2025-05-07T13:01:11Z

Can we set "30.0" milestone here?

hebasto · 2025-05-07T15:55:21Z

My Guix build:

aarch64
6b1411a20a83a7c8b791ff9ff738748b92d1a86fc7ac2337257ce418e0b966ca  guix-build-3a18075aedd7/output/dist-archive/bitcoin-3a18075aedd7.tar.gz
e2f572ad6ac20ed0b66c2db7a81997cf9e090c695364d88fda1c183efe2a2cbd  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/SHA256SUMS.part
886647c8c0322b1a450cc97efc1bf6a41b5efc8bf02477bb22a694cea1314a42  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-codesigning.tar.gz
4a82a01d9a12fee247e33973480183bf8a29cde28bb5bce2ab576e81f29b8d9c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-debug.zip
1cda05c5313630e81bdacd655104850243cb98b123ab23376f7b0570f50bcf2c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-setup-unsigned.exe
7c58ea2a69bd6e1bc27d56e208693d8873a365c69669396d7167d0c4d2c9275c  guix-build-3a18075aedd7/output/x86_64-w64-mingw32/bitcoin-3a18075aedd7-win64-unsigned.zip

theStack

Light ACK 3a18075

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

>.\build\bin\test_bitcoin.exe --run_test=system_tests/run_command
Running 1 test case...

*** No errors detected

>python.exe .\build\test\functional\rpc_signer.py
2025-05-09T14:17:19.535000Z TestFramework (INFO): PRNG seed is: 1291717054418451415
2025-05-09T14:17:19.596000Z TestFramework (INFO): Initializing test directory C:\Users\thestack\AppData\Local\Temp\bitcoin_func_test_6_lj29gg
2025-05-09T14:17:21.525000Z TestFramework (INFO): Stopping nodes
2025-05-09T14:17:21.750000Z TestFramework (INFO): Cleaning up C:\Users\thestack\AppData\Local\Temp\bitcoin_func_test_6_lj29gg on exit
2025-05-09T14:17:21.750000Z TestFramework (INFO): Tests successful

>python.exe .\build\test\functional\wallet_signer.py
2025-05-09T14:17:38.230000Z TestFramework (INFO): PRNG seed is: 5844070588042404732
2025-05-09T14:17:38.280000Z TestFramework (INFO): Initializing test directory C:\Users\thestack\AppData\Local\Temp\bitcoin_func_test_w0lar663
2025-05-09T14:17:39.823000Z TestFramework (INFO): Test walletdisplayaddress
2025-05-09T14:17:41.079000Z TestFramework (INFO): Prepare mock PSBT
2025-05-09T14:17:42.235000Z TestFramework (INFO): Test send using hww1
2025-05-09T14:17:42.507000Z TestFramework (INFO): Test sendall using hww1
2025-05-09T14:17:42.771000Z TestFramework (INFO): Prepare fee bumped mock PSBT
2025-05-09T14:17:42.776000Z TestFramework (INFO): Test bumpfee using hww1
2025-05-09T14:17:43.746000Z TestFramework (INFO): Test invalid external signer
2025-05-09T14:17:44.668000Z TestFramework (INFO): Test multiple external signers
2025-05-09T14:17:44.881000Z TestFramework (INFO): Stopping nodes
2025-05-09T14:17:45.038000Z TestFramework (INFO): Cleaning up C:\Users\thestack\AppData\Local\Temp\bitcoin_func_test_w0lar663 on exit
2025-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

bitcoin/doc/release-notes/release-notes-27.0.md

Lines 96 to 101 in b070ce1

    
           External Signing 
        
           ---------------- 
        
           - Support for external signing on Windows has been disabled. It will be re-enabled 
        
             once the underlying dependency (Boost Process), has been replaced with a different 
        
             library. (#28967)

laanwj

Code review and lightly tested ACK 3a18075

fanquake reviewed Apr 14, 2024

View reviewed changes

Comment thread ci/test/00_setup_env_win64.sh Outdated

hebasto force-pushed the 240414-win-subprocess branch from cf6a4b2 to 5e50b3d Compare April 14, 2024 18:17

fanquake mentioned this pull request Apr 19, 2024

refactor: Rename subprocess.hpp to follow our header name conventions #29910

Merged

hebasto force-pushed the 240414-win-subprocess branch from c05204c to 718e11c Compare April 23, 2024 17:34

hebasto force-pushed the 240414-win-subprocess branch from 718e11c to a0f3fc9 Compare April 23, 2024 19:48

DrahtBot added the CI failed label Apr 23, 2024

DrahtBot removed the CI failed label Apr 23, 2024

laanwj added Windows Wallet labels Apr 24, 2024

hebasto force-pushed the 240414-win-subprocess branch from a0f3fc9 to 0202082 Compare April 24, 2024 16:33

hebasto added the Needs CMake port label Apr 24, 2024

DrahtBot mentioned this pull request Apr 25, 2024

refactor: remove remaining unused code from cpp-subprocess #29961

Merged

Sjors reviewed Apr 26, 2024

View reviewed changes

DrahtBot added the Needs rebase label Apr 28, 2024

hebasto force-pushed the 240414-win-subprocess branch from 0202082 to db73ac8 Compare April 28, 2024 05:29

DrahtBot removed the Needs rebase label Apr 28, 2024

hebasto force-pushed the 240414-win-subprocess branch from 5541ef0 to ed41f61 Compare April 22, 2025 16:23

hebasto marked this pull request as ready for review April 22, 2025 16:28

DrahtBot removed the Needs rebase label Apr 22, 2025

maflcko reviewed Apr 22, 2025

View reviewed changes

Comment thread src/test/system_tests.cpp Outdated

DrahtBot removed the CI failed label Apr 22, 2025

hebasto force-pushed the 240414-win-subprocess branch from ed41f61 to 86c7c65 Compare April 22, 2025 22:02

fanquake reviewed Apr 23, 2025

View reviewed changes

Comment thread src/util/subprocess.h Outdated

Comment thread src/util/subprocess.h

Comment thread src/util/subprocess.h Outdated

Comment thread CMakeLists.txt Outdated

hebasto force-pushed the 240414-win-subprocess branch from 86c7c65 to f9e26d7 Compare April 23, 2025 20:27

hebasto mentioned this pull request Apr 27, 2025

subprocess: Backport upstream changes #32358

Merged

hebasto force-pushed the 240414-win-subprocess branch from f9e26d7 to 7806529 Compare April 27, 2025 16:28

hebasto marked this pull request as draft April 27, 2025 16:30

hebasto force-pushed the 240414-win-subprocess branch from 7806529 to 73ab0a4 Compare May 1, 2025 21:31

hebasto requested a review from luke-jr May 1, 2025 21:31

hebasto added 3 commits May 5, 2025 12:37

test: Reintroduce Windows support in system_tests/run_command test

6e5fc2b

build: Re-enable external signer support for Windows

719fa9f

ci: Drop -DENABLE_EXTERNAL_SIGNER=ON configure option

3a18075

This option is now enabled by default.

theStack approved these changes May 9, 2025

View reviewed changes

laanwj mentioned this pull request May 19, 2025

Use subprocess library for notifications #32566

Closed

laanwj approved these changes May 20, 2025

View reviewed changes

	External Signing
	----------------

	- Support for external signing on Windows has been disabled. It will be re-enabled
	once the underlying dependency (Boost Process), has been replaced with a different
	library. (#28967)

Uh oh!

Conversation

hebasto commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

fanquake commented Apr 17, 2024

Uh oh!

hebasto commented Apr 18, 2024

Uh oh!

hebasto commented Apr 23, 2024

Uh oh!

DrahtBot commented Apr 23, 2024

Uh oh!

hebasto commented Apr 24, 2024

Uh oh!

Sjors Apr 26, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Apr 28, 2024

Uh oh!

hebasto commented Apr 28, 2024

Uh oh!

theStack commented Apr 30, 2024

Uh oh!

hebasto commented Apr 22, 2025

Uh oh!

Uh oh!

Sjors commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hebasto commented Apr 27, 2025

Uh oh!

hebasto commented May 5, 2025

Uh oh!

Sjors commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented May 7, 2025

Uh oh!

Sjors commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented May 7, 2025

Uh oh!

hebasto commented May 7, 2025

Uh oh!

hebasto commented May 7, 2025

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

hebasto commented Apr 14, 2024 •

edited

Loading

DrahtBot commented Apr 14, 2024 •

edited

Loading

Sjors commented Apr 26, 2024 •

edited

Loading

Sjors commented Apr 23, 2025 •

edited

Loading

Sjors commented May 7, 2025 •

edited

Loading

Sjors commented May 7, 2025 •

edited

Loading