build: Re-enable external signer on Windows #25696

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:220725-boost changing 3 files +57 −44
  1. hebasto commented at 1:41 pm on July 25, 2022: member

    As https://github.com/boostorg/process/issues/207 has been resolved, it is possible now to re-enable external signer on Windows when cross-compiling.

    Guix build hashes:

     078f69ea7e0dbc8338981a92c0352220ccd7c2272d8cbff6a3b082a1412a935c5  guix-build-1a0d8e178c7b/output/aarch64-linux-gnu/SHA256SUMS.part
 1ee17456ec818ddf5a175182508966e622573ccb518807cca43a40fa1dceda092  guix-build-1a0d8e178c7b/output/aarch64-linux-gnu/bitcoin-1a0d8e178c7b-aarch64-linux-gnu-debug.tar.gz
 25080551bde379c746cc67b10429aef33b9f9e49d2d4e21ee1c3bfd9c1c845d46  guix-build-1a0d8e178c7b/output/aarch64-linux-gnu/bitcoin-1a0d8e178c7b-aarch64-linux-gnu.tar.gz
 3dfab220ce76a40bf7dcf07aab352a616a91b516503639455fe7e1b137bad3e85  guix-build-1a0d8e178c7b/output/arm-linux-gnueabihf/SHA256SUMS.part
 4516ceb822571a8bd88fe107dca434ef596b1e4328ccbda1d51e1d482d3050396  guix-build-1a0d8e178c7b/output/arm-linux-gnueabihf/bitcoin-1a0d8e178c7b-arm-linux-gnueabihf-debug.tar.gz
 521325380638f817107c203b9a1aedb808d1a4a2b4041493753ca4cbf19aa4f2c  guix-build-1a0d8e178c7b/output/arm-linux-gnueabihf/bitcoin-1a0d8e178c7b-arm-linux-gnueabihf.tar.gz
 6cf48ed78fcfceaeb3610ccf22326d735a129dcbf9d50b557b3de359169aefdfd  guix-build-1a0d8e178c7b/output/arm64-apple-darwin/SHA256SUMS.part
 7d4d51e136148bac6a20bb3adb402c499967647736acb420bfdeb71603aba57da  guix-build-1a0d8e178c7b/output/arm64-apple-darwin/bitcoin-1a0d8e178c7b-arm64-apple-darwin-unsigned.dmg
 895bb62d24f860e08a392ddb74d5860ccf27e8baa183e6749af877d26a3bd6b0b  guix-build-1a0d8e178c7b/output/arm64-apple-darwin/bitcoin-1a0d8e178c7b-arm64-apple-darwin-unsigned.tar.gz
 968da4c92f37bb802df37141af194f47c16da1d84f77a0fbb1016013ae0338502  guix-build-1a0d8e178c7b/output/arm64-apple-darwin/bitcoin-1a0d8e178c7b-arm64-apple-darwin.tar.gz
106704e38c2d3f11321403797598d05f062648fec6f2d76900ba250dab481e29da  guix-build-1a0d8e178c7b/output/dist-archive/bitcoin-1a0d8e178c7b.tar.gz
1164b936bc90d1e01fe8f276511edc9bb945dcebe70332aa37d3a786348443b8e7  guix-build-1a0d8e178c7b/output/powerpc64-linux-gnu/SHA256SUMS.part
123d03532e54b6e42498ea240c86b8567e94fd462f56087b869c3d6f09e2dde878  guix-build-1a0d8e178c7b/output/powerpc64-linux-gnu/bitcoin-1a0d8e178c7b-powerpc64-linux-gnu-debug.tar.gz
13c5843d79a58b0a864fe723458dab4eee54ad11f4b1f7960975b086eeedc0d541  guix-build-1a0d8e178c7b/output/powerpc64-linux-gnu/bitcoin-1a0d8e178c7b-powerpc64-linux-gnu.tar.gz
14f861ff519bd5e3d6d5ce1646ee0a06bcef1288ddb804a4a600e4dbfe5d5be521  guix-build-1a0d8e178c7b/output/powerpc64le-linux-gnu/SHA256SUMS.part
155f477da21980dbcf9696081903dc1ba8a3f79ce3579641d208e69a6f598c8eb9  guix-build-1a0d8e178c7b/output/powerpc64le-linux-gnu/bitcoin-1a0d8e178c7b-powerpc64le-linux-gnu-debug.tar.gz
16b3757b11c614136934158acea5139e8abd0c5c9cdfda72ae44db436f21716b33  guix-build-1a0d8e178c7b/output/powerpc64le-linux-gnu/bitcoin-1a0d8e178c7b-powerpc64le-linux-gnu.tar.gz
171c21bdb17fe3436e685e88c62423e630fe2b3c41dd00025a99fd80d97817ac2f  guix-build-1a0d8e178c7b/output/riscv64-linux-gnu/SHA256SUMS.part
18f36ae98473f086ae8f0dc66223b5ec407d57dc4d8d45ae284401520ff5c0b273  guix-build-1a0d8e178c7b/output/riscv64-linux-gnu/bitcoin-1a0d8e178c7b-riscv64-linux-gnu-debug.tar.gz
191603e4d0e869eb47a1dc2d26b67772d0016d90f7ba5e50d2009365cc02cb8169  guix-build-1a0d8e178c7b/output/riscv64-linux-gnu/bitcoin-1a0d8e178c7b-riscv64-linux-gnu.tar.gz
20f86ef652102f022827b70477bffa0a44008c6300cf62ca7b3595146cf2ed91ba  guix-build-1a0d8e178c7b/output/x86_64-apple-darwin/SHA256SUMS.part
21f84d435d8e4709bf29bc7ac7ed8dc6b8af4077cef05e520b468b2896ce10876a  guix-build-1a0d8e178c7b/output/x86_64-apple-darwin/bitcoin-1a0d8e178c7b-x86_64-apple-darwin-unsigned.dmg
22af2aab969b7ed7aeea0e02adbcc9e3b438086bf76b6bfc36146c53e05a27bd57  guix-build-1a0d8e178c7b/output/x86_64-apple-darwin/bitcoin-1a0d8e178c7b-x86_64-apple-darwin-unsigned.tar.gz
2332a5109ba28ab74ff66238e6a8f8a04e455ebce382a3be287df92a227818fe72  guix-build-1a0d8e178c7b/output/x86_64-apple-darwin/bitcoin-1a0d8e178c7b-x86_64-apple-darwin.tar.gz
24377462e9a96f4aba72c915dd5df5159a4301a1fa8ed0ee48faa6c71573de80c3  guix-build-1a0d8e178c7b/output/x86_64-linux-gnu/SHA256SUMS.part
25a3bf62e828d2350a483b2d16205014f66e8884597b0b72e178042a958c548336  guix-build-1a0d8e178c7b/output/x86_64-linux-gnu/bitcoin-1a0d8e178c7b-x86_64-linux-gnu-debug.tar.gz
2666cda980188ea1941a7d66c8b03c447580af33db55abe3bbe3581823ae0534a3  guix-build-1a0d8e178c7b/output/x86_64-linux-gnu/bitcoin-1a0d8e178c7b-x86_64-linux-gnu.tar.gz
272117f0dd9baeb4d585f841592e94c088f4487bf2008b8f281d0c3ceee92ff6cc  guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/SHA256SUMS.part
28d40d5dec3287f467c42232c05d82f7fb538cda34bd2e63ff7e1876f471c3a790  guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/bitcoin-1a0d8e178c7b-win64-debug.zip
2992dcc92765fbc07b1cc8258bfa69280541e1b4553cc41fed18672c2c6931d5c0  guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/bitcoin-1a0d8e178c7b-win64-setup-unsigned.exe
30a6dd9b4d29f21d3a18cf64556cb03446ef17bf801eb6ac257b65d27cbd95080f  guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/bitcoin-1a0d8e178c7b-win64-unsigned.tar.gz
31a4022e595d955198f73530473ef8e90a708746089ee2dd27de794176873330c1  guix-build-1a0d8e178c7b/output/x86_64-w64-mingw32/bitcoin-1a0d8e178c7b-win64.zip
  2. DrahtBot added the label Build system on Jul 25, 2022
  3. in configure.ac:1489 in 94bbce8f4b outdated 
    1523+  TEMP_CPPFLAGS="$CPPFLAGS"
1524+  CPPFLAGS="$CPPFLAGS $BOOST_CPPFLAGS"
1525+  TEMP_LDFLAGS="$LDFLAGS"
1526+  dnl Boost 1.73 and older require the following workaround.
1527+  LDFLAGS="$LDFLAGS $PTHREAD_CFLAGS"
1528+  AC_LINK_IFELSE([AC_LANG_PROGRAM([[#include <boost/process.hpp>]])],
    luke-jr commented at 5:55 pm on July 25, 2022:

    This test is too simplistic/fragile and will fail to detect compatibility correctly.

    See the test I have in #25111

    hebasto commented at 1:39 pm on July 26, 2022:
    Thank you @luke-jr! Cherry-picked your commit from #25111.
    luke-jr commented at 5:24 pm on July 26, 2022:
    Note there were 2 fixup commits as well. I think a7fa0da8611eb35ecd24284392f062a8ec47d037 might not be applicable here, but we probably need 06548df560f61c9deb5be09f72dd079833ec3c6b?
    hebasto commented at 9:41 am on July 27, 2022:

    Note there were 2 fixup commits as well. I think a7fa0da might not be applicable here

    For sure, since v1.78.0. See https://github.com/boostorg/process/pull/171.

    but we probably need 06548df?

    That issue has also been fixed since v1.78.0. See https://github.com/boostorg/process/pull/215.

    luke-jr commented at 2:18 pm on July 27, 2022:
    We support Boost down to 1.64.0 currently on non-Windows.
    hebasto commented at 7:22 pm on July 27, 2022:
    Changes in wchar_t.hpp look Windows-specific. FWIW, the first commit compiles with depends (boost 1.77) successfully. @luke-jr Could you point at a configuration which fails?
  4. luke-jr changes_requested
  5. DrahtBot commented at 6:26 pm on July 25, 2022: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK Sjors, achow101
    Stale ACK jarolrod

    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:

    • #24742 (build: prune Boost headers in depends by fanquake)

    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.

  6. hebasto force-pushed on Jul 25, 2022
  7. hebasto force-pushed on Jul 26, 2022
  8. fanquake commented at 2:10 pm on July 27, 2022: member

    [Similar to #25111. I’m still ~0 on this change](/bitcoin-bitcoin/25111/#pullrequestreview-970602390), but this approach is an improvement.

    Given #24907, I don’t think we should be leaning back into more Boost code usage. Outside of that, it’s pretty clear that Boost Process currently has no or minimal support for mingw-w64 builds. It looks like they don’t even have a mingw-w64 build in their CI (only MSVC). Windows fixes, some that we’ve upstreamed, also remain ignored, i.e https://github.com/boostorg/process/pull/237, https://github.com/boostorg/process/pull/239, https://github.com/boostorg/process/pull/240.

    I assume if we make this change, external signing on Windows is only going to be supported with Boost 1.80.0 and later? If so, I assume we can remove https://github.com/bitcoin/bitcoin/blob/207a22877330709e4462e6092c265ab55c8653ac/src/test/system_tests.cpp#L10?

  9. fanquake requested review from Sjors on Jul 27, 2022
  10. hebasto commented at 7:47 pm on July 27, 2022: member

    @fanquake

    I assume if we make this change, external signing on Windows is only going to be supported with Boost 1.80.0 and later? If so, I assume we can remove

    https://github.com/bitcoin/bitcoin/blob/207a22877330709e4462e6092c265ab55c8653ac/src/test/system_tests.cpp#L10

    Correct. See #25723.

  11. fanquake referenced this in commit 62c864605a on Jul 28, 2022
  12. hebasto force-pushed on Jul 28, 2022
  13. hebasto commented at 11:03 am on July 28, 2022: member
    Rebased on top of #25723.
  14. sidhujag referenced this in commit 59b247d7b0 on Jul 29, 2022
  15. in depends/patches/boost/mingw32_process.patch:1 in 0444353ba7 outdated 
    0@@ -0,0 +1,15 @@
1+Use Boost.Process with mingw32
    fanquake commented at 10:05 am on August 1, 2022:
    Is this patch from upstream? If so, can you add the upstream commit details / links here? If not, are you planning on upstreaming it? The fact that we still have to patch Process out the box, just to make it work, isn’t encouraging. Maybe these fixes can make it into 1.80.0.
    hebasto commented at 8:44 am on August 2, 2022:

    If not, are you planning on upstreaming it?

    https://github.com/boostorg/process/pull/264

  16. Sjors commented at 8:27 am on August 5, 2022: member
    I can’t test on Windows. Bumping the Boost version in depends should probably be its own PR? (or at least indicated in the PR description)
  17. fanquake commented at 9:35 am on August 19, 2022: member

    Bumping the Boost version in depends should probably be its own PR?

    See #25873.

  18. hebasto force-pushed on Aug 19, 2022
  19. hebasto marked this as ready for review on Aug 19, 2022
  20. hebasto commented at 10:47 am on August 19, 2022: member

    Updated 0444353ba7e97b53ff7f4ed7f06d67ed36466912 -> 9077916135ddabc07f105a77da8eb2bf8de73469 (pr25696.04 -> pr25696.05):

  21. jarolrod commented at 4:29 am on September 14, 2022: member

    concept ack

    guix hashes

    x86:

    0a4a230812d5e8281be4c1a96689ca64e917110a780bd96b254122d2588dba3d0  guix-build-86cb0b7168a2/output/dist-archive/bitcoin-86cb0b7168a2.tar.gz
12ad6d601394435946daf51e000af396ebbc89dfee8a10a76698ba2084c88b9a7  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/SHA256SUMS.part
2d6f2efe1e0b5be2ad4b465c078d36cdf636713049a60a2b424c3a9a16a667c1a  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/bitcoin-86cb0b7168a2-win64-debug.zip
3b29537c5bdc6d1cbd03be251f039d61f8f5215853fd47d776d05d85907aeb838  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/bitcoin-86cb0b7168a2-win64-setup-unsigned.exe
46349e6a21671ec8f1dbcd76a6246ec0300384864d198bb768dce1d853b2bc115  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/bitcoin-86cb0b7168a2-win64-unsigned.tar.gz
552192555fde11182b50758fd23db112e7a6a152e7c78b935da45ab2840021b61  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/bitcoin-86cb0b7168a2-win64.zip

    arm64:

    0a4a230812d5e8281be4c1a96689ca64e917110a780bd96b254122d2588dba3d0  guix-build-86cb0b7168a2/output/dist-archive/bitcoin-86cb0b7168a2.tar.gz
12ad6d601394435946daf51e000af396ebbc89dfee8a10a76698ba2084c88b9a7  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/SHA256SUMS.part
2d6f2efe1e0b5be2ad4b465c078d36cdf636713049a60a2b424c3a9a16a667c1a  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/bitcoin-86cb0b7168a2-win64-debug.zip
3b29537c5bdc6d1cbd03be251f039d61f8f5215853fd47d776d05d85907aeb838  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/bitcoin-86cb0b7168a2-win64-setup-unsigned.exe
46349e6a21671ec8f1dbcd76a6246ec0300384864d198bb768dce1d853b2bc115  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/bitcoin-86cb0b7168a2-win64-unsigned.tar.gz
552192555fde11182b50758fd23db112e7a6a152e7c78b935da45ab2840021b61  guix-build-86cb0b7168a2/output/x86_64-w64-mingw32/bitcoin-86cb0b7168a2-win64.zip
  22. fanquake referenced this in commit 3c537f1cc8 on Sep 21, 2022
  23. hebasto force-pushed on Sep 21, 2022
  24. hebasto commented at 10:40 am on September 21, 2022: member
    Rebased 9077916135ddabc07f105a77da8eb2bf8de73469 -> 75b51ed60350d63e1216be21f345bb297e697374 (pr25696.05 -> pr25696.06) on top of the merged #25873.
  25. sidhujag referenced this in commit a7834914f5 on Sep 23, 2022
  26. Sjors commented at 9:26 am on September 23, 2022: member
    I may have a Windows dual boot system in a month or so, so I can test stuff like this.
  27. jarolrod approved
  28. jarolrod commented at 4:57 am on October 14, 2022: member

    ACK 75b51ed60350d63e1216be21f345bb297e697374

    I tested that external signer works again on windows. The external signer setting is now show in the GUI. I was able to connect an external signer, communicate with HWI, and create a new wallet. I confirmed the correct descriptors were passed, I was able to generate new addresses, receive testnet bitcoin, and sign & broadcast a transaction.

  29. in src/test/system_tests.cpp:82 in 75b51ed603 outdated 
    84     }
85     {
86         // Return non-zero exit code, with error message for stderr
87 #ifdef WIN32
88         const std::string command{"cmd.exe /c dir nosuchfile"};
89+#ifdef _MSC_VER
    Sjors commented at 11:35 am on October 26, 2022:
    75b51ed60350d63e1216be21f345bb297e697374: Under what circumstances is this not defined? Maybe add a comment.
    hebasto commented at 11:49 am on November 23, 2022:
    No longer relevant, I guess.
  30. in depends/patches/boost/mingw32_process.patch:4 in 75b51ed603 outdated 
    0@@ -0,0 +1,21 @@
1+Fix compiling for MinGW-w64 using std::filesystem 
2+
3+Upstream commit:
4+ - https://github.com/boostorg/process/commit/a7b65bfc44f4af3a9d584fe9347fde0944c0167a
    Sjors commented at 12:16 pm on October 26, 2022:
    This has been merged. Do we know which release it’s in? (so we can drop the patch once the depends version is bumped)
    hebasto commented at 11:49 am on November 23, 2022:
    Updated.
  31. Sjors commented at 12:24 pm on October 26, 2022: member

    Testing 75b51ed60350d63e1216be21f345bb297e697374 rebased on master…

    External signer works on Windows 11 when using the Guix build. I can also see that the it won’t build the external signer if I drop the last commit.

    The patch matches what’s upstreamed to Boost.

    However, in PowerShell as well the regular Command Prompt the tests fail:

    0 .\test_bitcoin.exe
1Running 534 test cases...
2test/system_tests.cpp(66): error: in "system_tests/run_command": check what.find(expected) != std::string::npos has failed
3test/system_tests.cpp(99): error: in "system_tests/run_command": check what.find(expected) != std::string::npos has failed
  32. hebasto commented at 12:27 pm on October 26, 2022: member

    @Sjors

    However, in PowerShell as well the regular Command Prompt the tests fail:

    Is your Windows localized? See #26368.

  33. Sjors commented at 1:38 pm on October 26, 2022: member

    It is, I’ll try that fix.

    Meanwhile tACK 75b51ed60350d63e1216be21f345bb297e697374

  34. hebasto force-pushed on Oct 28, 2022
  35. hebasto marked this as a draft on Oct 28, 2022
  36. hebasto force-pushed on Oct 28, 2022
  37. hebasto marked this as ready for review on Oct 28, 2022
  38. hebasto commented at 1:55 pm on October 28, 2022: member
    Rebased 75b51ed60350d63e1216be21f345bb297e697374 -> 86cb0b7168a2e011e5e2f6ac2a605db64b0525ba (pr25696.06 -> pr25696.08) on top of the merged #26377.
  39. Sjors commented at 3:00 pm on October 31, 2022: member

    Tested 86cb0b7168a2e011e5e2f6ac2a605db64b0525ba using guix build. External signer works, but tests fail:

    0.\bitcoin-86cb0b7168a2\bin\test_bitcoin.exe
1Running 534 test cases...
2test/system_tests.cpp(61): error: in "system_tests/run_command": check e.code().value() == expected_error has failed [2 != 6]
3test/system_tests.cpp(94): error: in "system_tests/run_command": check what.find(expected) != std::string::npos has failed
4
5*** 2 failures are detected in the test module "Bitcoin Core Test Suite"

    Tried both PowerShell and the regular Command Prompt.

  40. hebasto force-pushed on Nov 23, 2022
  41. hebasto marked this as a draft on Nov 23, 2022
  42. hebasto commented at 11:48 am on November 23, 2022: member

    Updated 86cb0b7168a2e011e5e2f6ac2a605db64b0525ba -> 3e8160b1a27bda6ca41f2b8a577e5a7764ebefae (pr25696.08 -> pr25696.09):

  43. hebasto force-pushed on Nov 23, 2022
  44. fanquake referenced this in commit 911a40ead2 on Jan 6, 2023
  45. configure: Detect compatibility of Boost.Process rather than hardcode non-Windows 989451d068
  46. build: Re-enable external signer on Windows 1a0d8e178c
  47. hebasto force-pushed on Jan 6, 2023
  48. hebasto marked this as ready for review on Jan 6, 2023
  49. hebasto commented at 10:57 am on January 6, 2023: member

    Rebased bfd8f07293c66afdd5e8b91f99724e2d95a2ba36 -> 1a0d8e178c7b9a3e94d14f94b77305802ebdc93b (pr25696.10 -> pr25696.11) on top of the #26557.

    Ready for (final?) reviewing :)

  50. Sjors commented at 3:34 pm on January 6, 2023: member

    Tests pass now.

    tACK 1a0d8e178c7b9a3e94d14f94b77305802ebdc93b

    Also checked that with 989451d0689543b25ee6bf1d5b82c863d583597b Boost:Process is still detected correctly on Intel macOS.

  51. sidhujag referenced this in commit 7cfcf6e023 on Jan 6, 2023
  52. hebasto commented at 4:08 pm on January 25, 2023: member

    @achow101 @luke-jr @jarolrod

    Mind looking into this PR?

  53. achow101 commented at 2:00 am on March 9, 2023: member
    ACK 1a0d8e178c7b9a3e94d14f94b77305802ebdc93b
  54. DrahtBot requested review from jarolrod on Mar 9, 2023
  55. achow101 merged this on Mar 9, 2023
  56. achow101 closed this on Mar 9, 2023
  57. fanquake commented at 7:47 am on March 9, 2023: member
    post-merge Concept NACK. I don’t know why we are leaning back into this.
  58. hebasto commented at 9:53 am on March 9, 2023: member

    I don’t know why we are leaning back into this.

    Because using external signers is one of the best security options for users who run their Bitcoin Core software on Windows.

    post-merge Concept NACK.

    Better alternatives are welcome.

  59. hebasto deleted the branch on Mar 9, 2023
  60. sidhujag referenced this in commit 05882abaf8 on Mar 9, 2023
  61. PastaPastaPasta referenced this in commit 2f4ff5ae57 on Oct 27, 2023
  62. PastaPastaPasta referenced this in commit eecba4c49e on Dec 6, 2023
  63. bitcoin locked this on Mar 8, 2024


hebasto luke-jr DrahtBot fanquake Sjors jarolrod achow101


jarolrod

Labels
Build system

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