util: add RunCommandParseJSON #15382

Sjors · 2019-02-11T15:09:31Z

Prerequisite for external signer support in #16546. Big picture overview in this gist.

This adds a new dependency boost process. This is part of Boost since 1.64 which is part of depends. Because the minimum Boost version is 1.47, this functionality is skipped for older versions of Boost.

Use ./configure --with-boost-process to opt in, which checks for the presence of Boost::Process.

We add UniValue runCommandParseJSON(const std::string& strCommand) to system.{h,cpp} which calls an arbitrary command and processes the JSON returned by it. This is currently only called by the test suite.

~~For testing purposes this adds a new regtest-only RPC method runcommand, as well as test/mocks/command.py used by functional tests.~~ (this is no longer the case)

TODO:

review boost process in RFC including boost-process #15440

Sjors · 2019-02-11T15:13:51Z

See earlier discussion in #14912 (review) for different approaches like _popen or boost process.

cc @practicalswift @promag @ken2812221 @ryanofsky

jgarzik · 2019-02-11T15:32:47Z

This C++11 python-inspired subprocess module may or may not be of relevant interest, as it avoids Boost in favor of straight C++11: http://templated-thoughts.blogspot.com/2016/03/sub-processing-with-modern-c.html

Sjors · 2019-02-12T08:16:28Z

@jgarzik thanks! 1600 lines of code seems a bit overkill, but on the other hand it looks very clean and presumably better than anything I would come up with.

It currently doesn't have Windows support, but they're open to patches.

Speaking of UniValue :-) I think I need to reorder the Makefile to prevent some Travis instances from failing with undefined reference to UniValue::read(char const*, unsigned int).

practicalswift · 2019-02-12T10:14:24Z

The -regtest requirement is very important: could add a test case triggering the
"runcommand for regression testing (-regtest mode) only" code path?

Sjors · 2019-02-12T10:24:13Z

@practicalswift agreed, but I don't know if the functional test framework can launch a Testnet node without triggering a real IBD. I suppose I can configure it to not connect to any peers.

maflcko · 2019-02-12T16:20:36Z

src/rpc/misc.cpp

+    if (request.fHelp || request.params.size() != 1)
+        throw std::runtime_error(
+            RPCHelpMan{"runcommand",
+                "\nRun command and parse JSON from stdout (-regtest only)\n",


Why is this RPC exposed? For testing purposes? It should be a unit test in that case.

It's restricted to regtest (similar to setmocktime). Main reasons I didn't build a unit test:

no idea how

potentially creates even more cross-platform headaches

runCommand is also only tested via functional tests (via blocknotify, etc)

potentially creates even more cross-platform headaches

What you mean? Because of the command itself?

@Sjors how about supporting -exec "cmd" in bitcoin_test binary (or in any other test binary), then that could be used in the functional test (instead of this RPC).

I think it's ok to have this RPC only enabled in regtest. If you wanted to add more protection against misuse, you could disable it without gArgs.GetBoolArg("-enable-unsafe-runcommand-rpc", false) or similar.

If you did want to drop the RPC and just write a unit test for runCommandParseJSON, I don't think it would be difficult to do by just having the unit test write a temporary json file and then calling runCommandParseJSON with cat path_to_file on unix and type path_to_file on windows.

Sjors · 2019-02-13T08:19:19Z

AppVeyor seems unhappy about the RPCHelpMan syntax:

[C:\projects\bitcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj]
25c:\projects\bitcoin\src\leveldb\db\skiplist.h(360): warning C4312: 'reinterpret_cast': conversion from 'int' to 'void *' of greater size [C:\projects\bitcoin\build_msvc\libleveldb\libleveldb.vcxproj]
26c:\projects\bitcoin\src\rpc\misc.cpp(371): error C2440: 'initializing': cannot convert from 'initializer list' to 'RPCHelpMan' [C:\projects\bitcoin\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]
27c:\projects\bitcoin\src\rpc\misc.cpp(379): error C2512: 'std::runtime_error': no appropriate default constructor available [C:\projects\bitcoin\build_msvc\libbitcoin_server\libbitcoin_server.vcxproj]
28Command exited with code 1
29

practicalswift · 2019-02-13T10:55:31Z

test/functional/mocks/command.py

+parser.set_defaults(func=command)
+
+if len(sys.argv) == 1:
+   args = parser.parse_args(['-h'])


Wrong indentation :-)

test/functional/mocks/command.py

src/rpc/misc.cpp

Sjors · 2019-02-13T18:03:08Z

I switched to boost::process, which should give us Windows support. It's included with the depends Boost 1.64. Because the minimum Boost version is 1.47, this functionality is just not compiled for older versions of Boost.

Performing a series of link checks for a Boost component that is header-only doesn't make much sense, and currently means we just have another confusing Boost macro in our tree. I'm not sure why this was originally done this way; maybe Sjors or luke-jr can elaborate (bitcoin#15382 (929cda5))? The macro also has the side-effect of producing confusing error messages. i.e in bitcoin#20744, the CI is currently failing with: ```bash checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes checking for boostlib >= 1.58.0 (105800)... yes checking whether the Boost::Process library is available... yes configure: error: Could not find a version of the Boost::Process library! ``` This isn't useful, given there is no such thing as a `Boost::Process` library. This PR just removes the macro entirely, but maintains a `--with-boost-process` (defaulting to off), flag to configure. Hopefully this will also be removed, in favour of `--enable-disable-external-signer` if/when bitcoin#16546 is merged.

7bf04e3 build: remove mostly pointless BOOST_PROCESS macro (fanquake) Pull request description: Performing a series of link checks for a Boost component that is header-only doesn't make much sense, and currently means we just have another confusing Boost macro in our tree. I'm not sure why this was originally done this way; maybe Sjors or luke-jr can elaborate (#15382 (929cda5))? The macro also has the side-effect of producing confusing error messages. i.e in #20744, the CI is currently failing with: ```bash checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes checking for boostlib >= 1.58.0 (105800)... yes checking whether the Boost::Process library is available... yes configure: error: Could not find a version of the Boost::Process library! ``` This isn't useful, given there is no such thing as a `Boost::Process` library. This PR just removes the macro entirely, but maintains a `--with-boost-process` (defaulting to off), flag to configure. Hopefully this will also be removed, in favour of `--enable/disable-external-signer` if/when #16546 is merged. ACKs for top commit: laanwj: ACK 7bf04e3 Tree-SHA512: b270a0250f32df2078f986c165b8977967d8c06df80bf2773f3442f74b395a3bfa6544af1024d9b6524d90d47a0f6304194b3aced0e2ecb88e75916da945ccb6

7bf04e3 build: remove mostly pointless BOOST_PROCESS macro (fanquake) Pull request description: Performing a series of link checks for a Boost component that is header-only doesn't make much sense, and currently means we just have another confusing Boost macro in our tree. I'm not sure why this was originally done this way; maybe Sjors or luke-jr can elaborate (bitcoin#15382 (929cda5))? The macro also has the side-effect of producing confusing error messages. i.e in bitcoin#20744, the CI is currently failing with: ```bash checking for boostlib >= 1.58.0 (105800) lib path in "/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/lib"... yes checking for boostlib >= 1.58.0 (105800)... yes checking whether the Boost::Process library is available... yes configure: error: Could not find a version of the Boost::Process library! ``` This isn't useful, given there is no such thing as a `Boost::Process` library. This PR just removes the macro entirely, but maintains a `--with-boost-process` (defaulting to off), flag to configure. Hopefully this will also be removed, in favour of `--enable/disable-external-signer` if/when bitcoin#16546 is merged. ACKs for top commit: laanwj: ACK 7bf04e3 Tree-SHA512: b270a0250f32df2078f986c165b8977967d8c06df80bf2773f3442f74b395a3bfa6544af1024d9b6524d90d47a0f6304194b3aced0e2ecb88e75916da945ccb6

contains only 8314c23

…ration changes 88a91e2 [build] AppVeyor: clean cache when build configuration changes (Sjors Provoost) Pull request description: AppVeyor builds started starting failing on master after I cleaned the cache in bitcoin#15382. In addition, it appeared that a new dependency (boost-process) wasn't getting added in that PR without at least cleaning the vcpkg cache. Tree-SHA512: 1ad87bf6ca866cc20db04682cdf7572b59d22a7eaf346f390fc476c5e28bc5422733277fd765e5c9fd2ea88107b52fccd13f1f7e55493f567c4c4a1c16d7cb3a

…ge list 29eb039 Moves vcpkg list to a text file and updates the appveyor job and readme to use it. (Aaron Clauson) Pull request description: bitcoin#17364 attempted to save a couple of minutes by skipping the `vcpkg` steps if the vcpkg install directory was already cached. The discussion in bitcoin#15382 highlights the approach used in bitcoin#17364 does not accommodate adding a new package. ~~This PR improves the approach to individually check whether each vcpg package is installed rather than checking for the existence of the vcpkg install directory.~~ This PR moves the list of required vcpkg packages into a separate file and uses changes to that file to invalidate the appveyor cache. Whenever the cache is invalidated the vcpkg sources will be updated, the vcpkg binary built and the required packages installed from the latest port files. ACKs for top commit: MarcoFalke: ACK 29eb039 Tree-SHA512: 0c2a170f4e4b47ca0f9cef14f1e3892001b441a6d84f50bf5fd8a26bc4cdbd9358dfce7ef180d37150262e849650e9857d6b2bcd686964b963c3de6cd708a2f3

contains only 8314c23

Sjors force-pushed the 2019/02/runCommandParseJSON branch from 6bdabd7 to c284e7d Compare February 11, 2019 15:13

fanquake added RPC/REST/ZMQ Utils/log/libs labels Feb 11, 2019

Sjors force-pushed the 2019/02/runCommandParseJSON branch 4 times, most recently from 7050b82 to 44ee037 Compare February 12, 2019 09:45

Sjors force-pushed the 2019/02/runCommandParseJSON branch 3 times, most recently from 0ae6e68 to e2f681b Compare February 12, 2019 16:14

maflcko reviewed Feb 12, 2019

View reviewed changes

Sjors force-pushed the 2019/02/runCommandParseJSON branch 4 times, most recently from a05525a to 35cf444 Compare February 12, 2019 18:47

practicalswift reviewed Feb 13, 2019

View reviewed changes

test/functional/mocks/command.py Outdated Show resolved Hide resolved

maflcko reviewed Feb 13, 2019

View reviewed changes

src/rpc/misc.cpp Outdated Show resolved Hide resolved

Sjors force-pushed the 2019/02/runCommandParseJSON branch 5 times, most recently from 7e84a5a to a194e3c Compare February 13, 2019 18:01

Rspigler mentioned this pull request May 12, 2021

Enable external signer support by default, reduce #ifdef #21935

Merged

Sjors mentioned this pull request Jun 15, 2021

Boost::ASIO (used by Boost::Process) uses (falsely?) deprecated std::allocator #22254

Closed

kwvg pushed a commit to kwvg/dash that referenced this pull request Aug 24, 2021

partial bitcoin#15382: add RunCommandParseJSON

5038ef1

contains only 8314c23

kwvg pushed a commit to kwvg/dash that referenced this pull request Aug 24, 2021

partial bitcoin#15382: add RunCommandParseJSON

d5cb98e

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 24, 2021

partial bitcoin#15382: add RunCommandParseJSON

bb610a6

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 24, 2021

partial bitcoin#15382: add RunCommandParseJSON

175d271

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 24, 2021

partial bitcoin#15382: add RunCommandParseJSON

a72edd4

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 24, 2021

partial bitcoin#15382: add RunCommandParseJSON

9ccab34

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 25, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

27a5a7a

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 25, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

b79ec3b

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 26, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

3e1a723

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 26, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

04f6408

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 26, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

22003fa

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 27, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

309b7f5

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Aug 30, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

03c291e

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Sep 1, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

e7b607b

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Sep 1, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

fe6007e

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Sep 2, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

382580b

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Sep 3, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

9807be8

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Sep 3, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

f001a56

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Sep 3, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

f7ad75f

contains only 8314c23

kwvg added a commit to kwvg/dash that referenced this pull request Sep 4, 2021

partial merge bitcoin#15382: add RunCommandParseJSON

ad70450

contains only 8314c23

Sjors mentioned this pull request Apr 18, 2022

Awesome multisig PR labyrinth guide #24861

Open

17 tasks

gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 2, 2022

partial merge bitcoin#15382: add RunCommandParseJSON

826d3db

contains only 8314c23

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: add RunCommandParseJSON #15382

util: add RunCommandParseJSON #15382

Sjors commented Feb 11, 2019 •

edited

Sjors commented Feb 11, 2019

jgarzik commented Feb 11, 2019

Sjors commented Feb 12, 2019 •

edited

practicalswift commented Feb 12, 2019

Sjors commented Feb 12, 2019

maflcko Feb 12, 2019

Sjors Feb 12, 2019 •

edited

promag Sep 28, 2019

promag Oct 10, 2019

ryanofsky Oct 31, 2019

Sjors commented Feb 13, 2019

practicalswift Feb 13, 2019

Sjors commented Feb 13, 2019

util: add RunCommandParseJSON #15382

util: add RunCommandParseJSON #15382

Conversation

Sjors commented Feb 11, 2019 • edited

Sjors commented Feb 11, 2019

jgarzik commented Feb 11, 2019

Sjors commented Feb 12, 2019 • edited

practicalswift commented Feb 12, 2019

Sjors commented Feb 12, 2019

maflcko Feb 12, 2019

Choose a reason for hiding this comment

Sjors Feb 12, 2019 • edited

Choose a reason for hiding this comment

promag Sep 28, 2019

Choose a reason for hiding this comment

promag Oct 10, 2019

Choose a reason for hiding this comment

ryanofsky Oct 31, 2019

Choose a reason for hiding this comment

Sjors commented Feb 13, 2019

practicalswift Feb 13, 2019

Choose a reason for hiding this comment

Sjors commented Feb 13, 2019

Sjors commented Feb 11, 2019 •

edited

Sjors commented Feb 12, 2019 •

edited

Sjors Feb 12, 2019 •

edited