Skip to content

Commit

Permalink
Merge bitcoin#28981: Replace Boost.Process with cpp-subprocess
Browse files Browse the repository at this point in the history
d5a7155 build: remove boost::process dependency for building external signer support (Sebastian Falbesoner)
70434b1 external_signer: replace boost::process with cpp-subprocess (Sebastian Falbesoner)
cc8b987 Add `cpp-subprocess` header-only library (Hennadii Stepanov)

Pull request description:

  Closes bitcoin#24907.

  This PR is based on **theStack**'s [work](bitcoin#24907 (comment)).

  The `subprocess.hpp` header has been sourced from the [upstream repo](https://github.com/arun11299/cpp-subprocess) with the only modification being the removal of convenience functions, which are not utilized in our codebase.

  Windows-related changes will be addressed in subsequent follow-ups.

ACKs for top commit:
  achow101:
    reACK d5a7155
  Sjors:
    re-tACK d5a7155
  theStack:
    Light re-ACK d5a7155
  fanquake:
    ACK d5a7155 - with the expectation that this code is going to be maintained as our own. Next PRs should:

Tree-SHA512: d7fb6fecc3f5792496204190afb7d85b3e207b858fb1a75efe483c05260843b81b27d14b299323bb667c990e87a07197059afea3796cf218ed8b614086bd3611
  • Loading branch information
fanquake committed Apr 10, 2024
2 parents e319569 + d5a7155 commit 0a9cfd1
Show file tree
Hide file tree
Showing 8 changed files with 2,017 additions and 82 deletions.
62 changes: 10 additions & 52 deletions configure.ac
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,9 @@ AC_ARG_ENABLE([werror],
[enable_werror=no])

AC_ARG_ENABLE([external-signer],
[AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is auto, requires Boost::Process)])],
[AS_HELP_STRING([--enable-external-signer],[compile external signer support (default is yes)])],
[use_external_signer=$enableval],
[use_external_signer=auto])
[use_external_signer=yes])

AC_LANG_PUSH([C++])

Expand Down Expand Up @@ -1414,56 +1414,14 @@ if test "$use_boost" = "yes"; then
fi
fi

if test "$use_external_signer" != "no"; then
AC_MSG_CHECKING([whether Boost.Process can be used])
TEMP_CXXFLAGS="$CXXFLAGS"
dnl Boost 1.78 requires the following workaround.
dnl See: https://github.com/boostorg/process/issues/235
CXXFLAGS="$CXXFLAGS -Wno-error=narrowing"
TEMP_CPPFLAGS="$CPPFLAGS"
CPPFLAGS="$CPPFLAGS $BOOST_CPPFLAGS"
TEMP_LDFLAGS="$LDFLAGS"
dnl Boost 1.73 and older require the following workaround.
LDFLAGS="$LDFLAGS $PTHREAD_CFLAGS"
AC_LINK_IFELSE([AC_LANG_PROGRAM([[
#define BOOST_PROCESS_USE_STD_FS
#include <boost/process.hpp>
]],[[
namespace bp = boost::process;
bp::opstream stdin_stream;
bp::ipstream stdout_stream;
bp::child c("dummy", bp::std_out > stdout_stream, bp::std_err > stdout_stream, bp::std_in < stdin_stream);
stdin_stream << std::string{"test"} << std::endl;
if (c.running()) c.terminate();
c.wait();
c.exit_code();
]])],
[have_boost_process="yes"],
[have_boost_process="no"])
LDFLAGS="$TEMP_LDFLAGS"
CPPFLAGS="$TEMP_CPPFLAGS"
CXXFLAGS="$TEMP_CXXFLAGS"
AC_MSG_RESULT([$have_boost_process])
if test "$have_boost_process" = "yes"; then
case $host in
dnl Boost Process for Windows uses Boost ASIO. Boost ASIO performs
dnl pre-main init of Windows networking libraries, which we do not
dnl want.
*mingw*)
use_external_signer="no"
;;
*)
use_external_signer="yes"
AC_DEFINE([ENABLE_EXTERNAL_SIGNER], [1], [Define if external signer support is enabled])
AC_DEFINE([BOOST_PROCESS_USE_STD_FS], [1], [Defined to avoid Boost::Process trying to use Boost Filesystem])
;;
esac
else
if test "$use_external_signer" = "yes"; then
AC_MSG_ERROR([External signing is not supported for this Boost version])
fi
use_external_signer="no";
fi
case $host in
dnl Re-enable it after enabling Windows support in cpp-subprocess.
*mingw*)
use_external_signer="no"
;;
esac
if test "$use_external_signer" = "yes"; then
AC_DEFINE([ENABLE_EXTERNAL_SIGNER], [1], [Define if external signer support is enabled])
fi
AM_CONDITIONAL([ENABLE_EXTERNAL_SIGNER], [test "$use_external_signer" = "yes"])

Expand Down
1 change: 1 addition & 0 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,7 @@ BITCOIN_CORE_H = \
util/sock.h \
util/spanparsing.h \
util/string.h \
util/subprocess.hpp \
util/syserror.h \
util/task_runner.h \
util/thread.h \
Expand Down
25 changes: 10 additions & 15 deletions src/common/run_command.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,39 +12,34 @@
#include <univalue.h>

#ifdef ENABLE_EXTERNAL_SIGNER
#include <boost/process.hpp>
#include <util/subprocess.hpp>
#endif // ENABLE_EXTERNAL_SIGNER

UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in)
{
#ifdef ENABLE_EXTERNAL_SIGNER
namespace bp = boost::process;
namespace sp = subprocess;

UniValue result_json;
bp::opstream stdin_stream;
bp::ipstream stdout_stream;
bp::ipstream stderr_stream;
std::istringstream stdout_stream;
std::istringstream stderr_stream;

if (str_command.empty()) return UniValue::VNULL;

bp::child c(
str_command,
bp::std_out > stdout_stream,
bp::std_err > stderr_stream,
bp::std_in < stdin_stream
);
auto c = sp::Popen(str_command, sp::input{sp::PIPE}, sp::output{sp::PIPE}, sp::error{sp::PIPE});
if (!str_std_in.empty()) {
stdin_stream << str_std_in << std::endl;
c.send(str_std_in);
}
stdin_stream.pipe().close();
auto [out_res, err_res] = c.communicate();
stdout_stream.str(std::string{out_res.buf.begin(), out_res.buf.end()});
stderr_stream.str(std::string{err_res.buf.begin(), err_res.buf.end()});

std::string result;
std::string error;
std::getline(stdout_stream, result);
std::getline(stderr_stream, error);

c.wait();
const int n_error = c.exit_code();
const int n_error = c.retcode();
if (n_error) throw std::runtime_error(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s\n", str_command, n_error, error));
if (!result_json.read(result)) throw std::runtime_error("Unable to parse JSON: " + result);

Expand Down
8 changes: 4 additions & 4 deletions src/external_signer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,12 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalS

UniValue ExternalSigner::DisplayAddress(const std::string& descriptor) const
{
return RunCommandParseJSON(m_command + " --fingerprint \"" + m_fingerprint + "\"" + NetworkArg() + " displayaddress --desc \"" + descriptor + "\"");
return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " displayaddress --desc " + descriptor);
}

UniValue ExternalSigner::GetDescriptors(const int account)
{
return RunCommandParseJSON(m_command + " --fingerprint \"" + m_fingerprint + "\"" + NetworkArg() + " getdescriptors --account " + strprintf("%d", account));
return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " getdescriptors --account " + strprintf("%d", account));
}

bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::string& error)
Expand All @@ -93,8 +93,8 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str
return false;
}

const std::string command = m_command + " --stdin --fingerprint \"" + m_fingerprint + "\"" + NetworkArg();
const std::string stdinStr = "signtx \"" + EncodeBase64(ssTx.str()) + "\"";
const std::string command = m_command + " --stdin --fingerprint " + m_fingerprint + NetworkArg();
const std::string stdinStr = "signtx " + EncodeBase64(ssTx.str());

const UniValue signer_result = RunCommandParseJSON(command, stdinStr);

Expand Down
14 changes: 5 additions & 9 deletions src/test/system_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#include <univalue.h>

#ifdef ENABLE_EXTERNAL_SIGNER
#include <boost/process.hpp>
#include <util/subprocess.hpp>
#endif // ENABLE_EXTERNAL_SIGNER

#include <boost/test/unit_test.hpp>
Expand All @@ -34,20 +34,16 @@ BOOST_AUTO_TEST_CASE(run_command)
BOOST_CHECK(result.isNull());
}
{
const UniValue result = RunCommandParseJSON("echo \"{\"success\": true}\"");
const UniValue result = RunCommandParseJSON("echo {\"success\": true}");
BOOST_CHECK(result.isObject());
const UniValue& success = result.find_value("success");
BOOST_CHECK(!success.isNull());
BOOST_CHECK_EQUAL(success.get_bool(), true);
}
{
// An invalid command is handled by Boost
const int expected_error{2};
BOOST_CHECK_EXCEPTION(RunCommandParseJSON("invalid_command"), boost::process::process_error, [&](const boost::process::process_error& e) {
BOOST_CHECK(std::string(e.what()).find("RunCommandParseJSON error:") == std::string::npos);
BOOST_CHECK_EQUAL(e.code().value(), expected_error);
return true;
});
// An invalid command is handled by cpp-subprocess
const std::string expected{"execve failed: "};
BOOST_CHECK_EXCEPTION(RunCommandParseJSON("invalid_command"), subprocess::CalledProcessError, HasReason(expected));
}
{
// Return non-zero exit code, no output to stderr
Expand Down
Loading

0 comments on commit 0a9cfd1

Please sign in to comment.