build: disable external-signer for Windows #28967
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
The same for MSVC builds: bitcoin/build_msvc/bitcoin_config.h.in Lines 44 to 45 in 8cf2137 ? |
fanquake
force-pushed
the
disable_external_signer_win
branch
2 times, most recently
from
02895a8
to
9ff589c
Compare
Added. |
|
Concept ACK |
There was a problem hiding this comment.
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 with2.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 9ff589c.
Suggesting to rebase on top of the #28938 and drop boost-process from vcpkg.json.
This is redundant in any case, because --enable-external-signer is already in `BITCOIN_CONFIG_ALL`.
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 bitcoin#28486 and discussion in bitcoin#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.
fanquake
force-pushed
the
disable_external_signer_win
branch
from
9ff589c
to
308aec3
Compare
Added. |
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. |
|
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. |
This code has been dead since bitcoin#28967. Required as a precondition for replacing Boost.Process with cpp-subprocess to make diff for this code meaningful and reviewable. The plan is to reintroduce Windows-specific code in this test simultaneously with enabling Windows support in cpp-subprocess.
|
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/ |
…un_command` 51bc1c7 test: Remove Windows-specific code from `system_tests/run_command` (Hennadii Stepanov) Pull request description: The removed code has been dead since #28967. Required as a precondition for replacing Boost.Process with [cpp-subprocess](#28981) to make diff for this code meaningful and reviewable. The plan is to reintroduce Windows-specific code in this test simultaneously with enabling Windows support in cpp-subprocess. ACKs for top commit: Sjors: utACK 51bc1c7 theStack: Code-review ACK 51bc1c7 Tree-SHA512: 0e3875c4dc20564332555633daf2227223b10dc3d052557635eced2734575d1e0252fb19e46ea6e6c47a15c51c345f70b6d437e33435abcd0e4fcf29edb50887
|
Added rel-note todo to the draft wiki. |
This code has been dead since bitcoin#28967. Required as a precondition for replacing Boost.Process with cpp-subprocess to make diff for this code meaningful and reviewable. The plan is to reintroduce Windows-specific code in this test simultaneously with enabling Windows support in cpp-subprocess.
This code has been dead since bitcoin#28967. Required as a precondition for replacing Boost.Process with cpp-subprocess to make diff for this code meaningful and reviewable. The plan is to reintroduce Windows-specific code in this test simultaneously with enabling Windows support in cpp-subprocess.
This code has been dead since bitcoin/bitcoin#28967. Required as a precondition for replacing Boost.Process with cpp-subprocess to make diff for this code meaningful and reviewable. The plan is to reintroduce Windows-specific code in this test simultaneously with enabling Windows support in cpp-subprocess.
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 with2.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.
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.