common: remove `::runtime_error` from `RunCommandParseJSON` #35960

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:run_command_json changing 2 files +0 −16

fanquake commented at 4:32 PM on August 12, 2026: member

I don't think there's a code path that can reach RunCommandParseJSON if we compile with ENABLE_EXTERNAL_SIGNER=OFF. If there is a reason for having the code this way, it could be good to document.

This also requires more workarounds in #35911.
DrahtBot commented at 4:32 PM on August 12, 2026: 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/35960.



Reviews

See the guideline and AI policy for information on the review process.

Type Reviewers

ACK stickies-v, sedited, willcl-ark

Stale ACK w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.
DrahtBot added the label CI failed on Aug 12, 2026
DrahtBot commented at 5:56 PM on August 12, 2026: contributor


🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/31618062607/job/94185706375</sub> <sub>LLM reason (✨ experimental): CI failed due to a lint error (includes_build_config): src/common/run_command.cpp includes bitcoin-build-config.h but uses none of its symbols (unused include).</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

</details>
fanquake force-pushed on Aug 12, 2026
DrahtBot removed the label CI failed on Aug 12, 2026
w0xlt commented at 12:47 AM on August 13, 2026: contributor

ACK b0399cdb701887e3855915a82084758ab7119a03

Type	Reviewers
ACK	stickies-v, sedited, willcl-ark
Stale ACK	w0xlt

in src/common/run_command.cpp:13 in b0399cdb70 outdated

   8 |  
   9 |  #include <tinyformat.h>
  10 |  #include <univalue.h>
  11 |  #include <util/string.h>
  12 | -
  13 | -#ifdef ENABLE_EXTERNAL_SIGNER

stickies-v commented at 9:31 AM on August 14, 2026:

I don't think the tests should be gated either.

diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp
index 1b1d6bc75e..2fa4f812b7 100644
--- a/src/test/system_tests.cpp
+++ b/src/test/system_tests.cpp
@@ -10,19 +10,15 @@
 #include <test/util/setup_common.h>
 #include <univalue.h>
 #include <util/string.h>
+#include <util/subprocess.h>
 
 #include <cstdlib>
 #include <iostream>
+#include <string>
 #include <string_view>
 
-#ifdef ENABLE_EXTERNAL_SIGNER
-#include <util/subprocess.h>
-#endif // ENABLE_EXTERNAL_SIGNER
-
 #include <boost/test/unit_test.hpp>
 
-#include <string>
-
 namespace {
 // When set in the environment, test_bitcoin acts as a mock subprocess for the
 // run_command test below instead of running unit tests.
@@ -60,8 +56,6 @@ const bool g_maybe_run_mock_dispatcher_before_main{[]() {
 
 BOOST_FIXTURE_TEST_SUITE(system_tests, BasicTestingSetup)
 
-#ifdef ENABLE_EXTERNAL_SIGNER
-
 static std::vector<std::string> mock_executable(const std::string& name)
 {
 #if defined(WIN32)
@@ -127,6 +121,4 @@ BOOST_AUTO_TEST_CASE(run_command)
         BOOST_CHECK_EQUAL(success.get_bool(), true);
     }
 }
-#endif // ENABLE_EXTERNAL_SIGNER
-
 BOOST_AUTO_TEST_SUITE_END()

</details>

fanquake commented at 9:41 AM on August 14, 2026:

Thanks, added.

stickies-v approved
stickies-v commented at 9:33 AM on August 14, 2026: contributor

ACK b0399cdb701887e3855915a82084758ab7119a03

Doesn't make sense for that utility to be gated on ENABLE_EXTERNAL_SIGNER when it has no dependencies on external signers whatsoever. It seems like this is a weird artifact/leftover from 87a97941f667483bbf2ab00929e03a2199cb8a62. Since we have now replaced the boost dependency with util/subprocess.h, we can just remove the guards.

common: remove ::runtime_error from RunCommandParseJSON

I don't think there's a code path that can reach RunCommandParseJSON if
we compile with `-DENABLE_EXTERNAL_SIGNER=OFF`. This also requires more
workarounds in #35911.

Co-authored-by: stickies-v <stickies-v@protonmail.com>

8b5da677d7

fanquake force-pushed on Aug 14, 2026
stickies-v commented at 9:48 AM on August 14, 2026: contributor

re-ACK 8b5da677d7b0df5ef00b503b506bbfb68f9d436d
DrahtBot requested review from w0xlt on Aug 14, 2026
sedited approved
sedited commented at 2:18 PM on August 14, 2026: contributor

ACK 8b5da677d7b0df5ef00b503b506bbfb68f9d436d
willcl-ark approved
willcl-ark commented at 3:12 PM on August 14, 2026: member

ACK 8b5da677d7b0df5ef00b503b506bbfb68f9d436d

I see no reason to keep this (stale?) feature guard here.
fanquake merged this on Aug 14, 2026
fanquake closed this on Aug 14, 2026
fanquake deleted the branch on Aug 14, 2026

Contributors

Review Requested
w0xlt

Linked (view graph)

#35911 Warn on and add missing [[noreturn]]

common: remove `::runtime_error` from `RunCommandParseJSON` #35960

Code Coverage & Benchmarks

Reviews