build: disable external-signer for Windows

fanquake commented at 2:05 pm on November 29, 2023: member

It’s come to light that Boost ASIO (a Boost Process sub dep) has in some instances, been quietly initialising our network stack on Windows (see PR #28486 and discussion in #28940).

This has been shielding a bug in our own code, but the larger issue is that Boost Process/ASIO is running code before main, and doing things like setting up networking. This undermines our own assumptions about how our binary works, happens before we run any sanity checks, and before we call our own code to setup networking. Note that ASIO also calls WSAStartup with version 2.0, whereas we call with 2.2.

It’s also not clear why a feature like external signer would have a dependency that would be doing anything network/socket related, given it only exists to spawn a local process.

See also the discussion in #24907. Note that the maintaince of Boost Process in general, has not really improved. For example, rather than fixing bugs like https://github.com/boostorg/process/issues/111, i.e, https://github.com/boostorg/process/pull/317, the maintainer chooses to just wrap exception causing overflows in try-catch blocks: https://github.com/boostorg/process/commit/0c42a58eacab6a96b19196e399307bad8a938a27. These changes get merged in large, unreviewed PRs, i.e https://github.com/boostorg/process/pull/319.

This PR disables external-signer on Windows for now. If, in future, someone changes how Boost Process works, or replaces it entirely with some properly reviewed and maintained code, we could reenable this feature on Windows.

DrahtBot commented at 2:05 pm on November 29, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, TheCharlatan

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:

#25972 (build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set 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.

DrahtBot added the label Build system on Nov 29, 2023

hebasto commented at 4:56 pm on November 29, 2023: member

The same for MSVC builds: https://github.com/bitcoin/bitcoin/blob/8cf2137dbeaa681f0178f1e2fd70b639e6d4f717/build_msvc/bitcoin_config.h.in#L44-L45 ?

fanquake force-pushed on Nov 29, 2023

fanquake marked this as ready for review on Nov 29, 2023

fanquake commented at 5:19 pm on November 29, 2023: member

The same for MSVC builds:

Added.

TheCharlatan commented at 5:12 pm on November 30, 2023: contributor

Concept ACK

hebasto approved

hebasto commented at 2:21 am on December 1, 2023: member

It’s come to light that Boost ASIO (a Boost Process sub dep) has in some instances, been quietly initialising our network stack on Windows (see PR #28486 and discussion in #28940).

This has been shielding a bug in our own code, but the larger issue is that Boost Process/ASIO is running code before main, and doing things like setting up networking. This undermines our own assumptions about how our binary works, happens before we run any sanity checks, and before we call our own code to setup networking. Note that ASIO also calls WSAStartup with version 2.0, whereas we call with 2.2.

It’s also not clear why a feature like external signer would have a dependency that would be doing anything network/socket related, given it only exists to spawn a local process.

See also the discussion in #24907. Note that the maintaince of Boost Process in general, has not really improved. For example, rather than fixing bugs like boostorg/process#111, i.e, boostorg/process#317, the maintainer chooses to just wrap exception causing overflows in try-catch blocks: boostorg/process@0c42a58. These changes get merged in large, unreviewed PRs, i.e boostorg/process#319.

Working closely with Boost.Process source recently I agree with every statement above. Sigh…

This PR disables external-signer on Windows for now. If, in future, someone changes how Boost Process works, or replaces it entirely with some properly reviewed and maintained code, we could reenable this feature on Windows.

I still believe that the “external signer” feature is extremely important for Bitcoin Core users, who prefer to keep their keys in dedicated devices. So I hope we shall manage to make #28981 (or something better) merged before the v27 release.

ACK 9ff589c60c931a8cb19cb984cc69a223186181ca.

Suggesting to rebase on top of the bitcoin/bitcoin#28938 and drop boost-process from vcpkg.json.

DrahtBot requested review from TheCharlatan on Dec 1, 2023

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

This is redundant in any case, because --enable-external-signer is
already in `BITCOIN_CONFIG_ALL`.

35537318a1

build: disable external-signer for Windows

It's come to light that Boost ASIO (a Boost Process sub dep) has in some
instances, been queitly initialising our network stack on Windows (see
PR #28486 and discussion in #28940).

This has been shielding a bug in our own code, but the larger issue
is that Boost Process/ASIO is running code before main, and doing things
like setting up networking. This undermines our own assumptions about
how our binary works, happens before we get to run any sanity checks,
and also runs before we call our own code to setup networking.

It's also not clear why a feature like external signer would have a
dependency that would be doing anything network/socket related, given it
only exists to spawn a local process.

308aec3e56

fanquake force-pushed on Dec 1, 2023

fanquake commented at 10:47 am on December 1, 2023: member

and drop boost-process from vcpkg.json.

Added.

hebasto approved

hebasto commented at 10:49 am on December 1, 2023: member

re-ACK 308aec3e5655327d98e0428d8205d246f24d6af5.

TheCharlatan approved

TheCharlatan commented at 11:48 am on December 1, 2023: contributor

ACK 308aec3e5655327d98e0428d8205d246f24d6af5

luke-jr changes_requested

luke-jr commented at 2:45 am on December 5, 2023: member

Let’s at least let builders explicitly enable it?

fanquake commented at 10:53 am on December 13, 2023: member

Let’s at least let builders explicitly enable it?

That would mostly defeat the point of this change, and leave us supporting code that we’ve deemed unsafe and unreviewed. If someone wants to do that, they are free to understand the risks, and patch their own code.

fanquake merged this on Dec 13, 2023

fanquake closed this on Dec 13, 2023

fanquake deleted the branch on Dec 13, 2023

hebasto commented at 5:16 am on February 8, 2024: member

As it seems that #28981 won’t make it into v27.0, I suggest reflecting the disabling of external signer support for Windows in the release notes. This will allow users who rely on it to skip the next release update.

hebasto added the label Needs release note on Feb 8, 2024

hebasto referenced this in commit 51bc1c7126 on Feb 27, 2024

Sjors commented at 4:23 pm on February 27, 2024: member

For historical context: we disabled Windows support for external signers in Januari 2022 with #24065. We re-enabled it in May 2023 in #25696. It was included in v26.0, but not mentioned in the release notes: https://bitcoincore.org/en/releases/26.0/

fanquake referenced this in commit dfbad09c60 on Feb 28, 2024

fanquake removed the label Needs release note on Mar 4, 2024

fanquake commented at 11:57 am on March 4, 2024: member

Added rel-note todo to the draft wiki.

build: disable external-signer for Windows #28967

Code Coverage

Reviews

Conflicts