bitcoin-cli: Add -ipcconnect option #32297
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32297. 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. LLM Linter (✨ experimental)Possible typos and grammar issues:
drahtbot_id_4_m |
|
Concept ACK, it'd be useful to have an utility that can talk to the node through IPC and making |
|
Concept ACK, as it can make communication between the CLI and the node simpler and configuration-free. |
|
concept ACK, cool idea and will review. Does this close #5029 (adding unix sockets to the bitcoind rpc api) or does it only apply to |
It uses capnproto, so it may or may not achieve goals of #5029 depending on what they are. This PR does make it possible to call RPC methods over a unix socket, but in order to do that you need to use capnproto which is not available everywhere. For example there wouldn't be an easy way for our python test framework to use the socket directly unless it depended on https://github.com/capnproto/pycapnp. So another solution that uses a another protocol or just uses http over the socket might be preferable to this approach. But the first commit here could be useful for supporting other approaches as well. |
There was a problem hiding this comment.
Updated df40f46 -> 538521a (pr/ipc-cli.2 -> pr/ipc-cli.3, compare) handling -rpcconnect option, adding a test, making various cleanups, adding comments, and fixing CI failures in non-ipc builds
Updated 538521a -> bd29de3 (pr/ipc-cli.3 -> pr/ipc-cli.4, compare) fixing compile error in intermediate commit, https://github.com/bitcoin/bitcoin/actions/runs/14538988101/job/40793056407?pr=32297, fixing functional test failure when socket path is too long https://cirrus-ci.com/task/6103052337283072, and adding new test to cover "bitcoin-cli was not built with IPC support" error
|
Concept ACK |
|
Looks like those are real typos that should be fixed:
|
|
This looks like it uses JSON-RPC inside Capnproto? Why not just use straight Capnproto? |
Yes it is skipping the HTTP part of the protocol but keeping the JSON part. The capnproto interface this exposes looks like: where |
Right, Say, representing the entire RPC protocol as cap'n'proto structures directly would be interesting (anologous to how c-lightning has a gRPC version of their RPC), but this would be a huge project for which the first step would be the formal definition of the RPC protocol (including specific-enough types) so that that can be automatically generated. And this would make it harder to make a cli tool, not easier, because the client needs to know the protocol to convert its command line to suitable calls. So sending the JSON inside the interface seems like a good, low-impact way to implement this tool. |
|
Concept ACK |
There was a problem hiding this comment.
Great review! I improved a number of things in response to your comments.
Rebased 9ff5bc7 -> 6bf727f (pr/ipc-cli.5 -> pr/ipc-cli.6, compare) due to conflict with #28710, also made a number of improvements to -ipcbind documentation and -rpcwait behavior and other code as suggested, also split commits and cleaned up bitcoin-cli build rule and new tests.
Rebased 6bf727f -> ad01440 (pr/ipc-cli.6 -> pr/ipc-cli.7, compare) due to conflict with #31375
Add ExecuteHTTPRPC to provide a way to execute an HTTP request without relying on HTTPRequest and libevent types. Behavior is not changing in any way, this is just moving code. This commit may be easiest to review using git's --color-moved option.
This allows `bitcoin-cli` to connect to the node via IPC instead TCP to execute RPC methods in an upcoming commit.
Add a MakeBasicInit() function so simpler standalone IPC clients like bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 that only initiate IPC connections without exposing any IPC interfaces themselves can to avoid needing to implement their own specialized interfaces::Init subclasses.
This implements an idea from Pieter Wuille <pieter@wuille.net> bitcoin#28722 (comment) to allow `bitcoin-cli` to connect to the node via IPC instead of TCP, if the `ENABLE_IPC` cmake option is enabled and the node has been started with `-ipcbind`. The feature can be tested with: build/bin/bitcoin-node -regtest -ipcbind=unix -debug=ipc build/bin/bitcoin-cli -regtest -ipcconnect=unix -getinfo The `-ipconnect` parameter can also be omitted, since this change also makes `bitcoin-cli` prefer IPC over HTTP by default, and falling back to HTTP if an IPC connection can't be established.
When an invalid socket path is passed to -ipcconnect, either because the path exceeds the maximum socket length, or the path includes a directory component which is not actually a directory, treat this the same as the same as the socket refusing connections or not existing, instead of treating it like a more serious I/O error and throwing a fatal exception. This is needed to avoid CI errors after the following commit which adds a functional test and uses -datadir paths exceeding the maximum socket length when running in CI.
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
Add a MakeBasicInit() function so simpler standalone IPC clients like bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 that only initiate IPC connections without exposing any IPC interfaces themselves can to avoid needing to implement their own specialized interfaces::Init subclasses.
Add a MakeBasicInit() function so simpler standalone IPC clients like bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 that only initiate IPC connections without exposing any IPC interfaces themselves can to avoid needing to implement their own specialized interfaces::Init subclasses.
Add a MakeBasicInit() function so simpler standalone IPC clients like bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 that only initiate IPC connections without exposing any IPC interfaces themselves can to avoid needing to implement their own specialized interfaces::Init subclasses. Two implementations of the MakeBasicInit() function are implemented, one returning a an Init object, and the other returning null. Having two implementations allows programs like bitcoin-cli to be linked with or without IPC support (based on the ENABLE_IPC option) without needing to be compiled differently.
Add a MakeBasicInit() function so simpler standalone IPC clients like bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 that only initiate IPC connections without exposing any IPC interfaces themselves can to avoid needing to implement their own specialized interfaces::Init subclasses.
Add a MakeBasicInit() function so simpler standalone IPC clients like bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 that only initiate IPC connections without exposing any IPC interfaces themselves can to avoid needing to implement their own specialized interfaces::Init subclasses.
Add a MakeBasicInit() function so simpler standalone IPC clients like bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 that only initiate IPC connections without exposing any IPC interfaces themselves can to avoid needing to implement their own specialized interfaces::Init subclasses.
| @@ -398,6 +399,16 @@ def run_test(self): | |||
| self.log.info("Test that only one of -addrinfo, -generate, -getinfo, -netinfo may be specified at a time") | |||
| assert_raises_process_error(1, "Only one of -getinfo, -netinfo may be specified", self.nodes[0].cli('-getinfo', '-netinfo').send_cli) | |||
|
|
|||
| if not self.is_ipc_enabled(): | |||
| # This test behavior when ENABLE_IPC if off. When it is on behavior | |||
There was a problem hiding this comment.
in interface_bitcoin_cli.py: “# This test behavior when ENABLE_IPC if off.” -> “# This tests behavior when ENABLE_IPC is off.” [missing verb “tests” and wrong “if” instead of “is” makes the comment ungrammatical]
There was a problem hiding this comment.
re-ACK ad01440
reviewed each commit again, built and ran tests on macos/arm64. Played with the feature on main, requesting large block data and waitforblockheight etc. Multiple requests can be executed in parallel over the same socket which is very cool, works with bitcoin-node and bitcoin-gui, tested both. Although waitforblockheight doesn't work with bitcoin-gui because EnsureMiner fails. That was a surprise to me but may just be expected.
Two non blocking nits
Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK ad0144022c2bbed6799bafa3ac8b80ac81dd7178
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmhRrOEACgkQ5+KYS2KJ
yTqgLhAA1fi5wv1y2LjeIPUqmzllmoBYnmlfI+Pki0cOFIH60DMhYLYrBc7gW61x
1U9G39GbscmOF6OO+z9viST62JlXTdhDhs3CieCaRX8n2QNGwnKRh8wO6jugybol
55XAvj5xJ7DwSzYJrQH8yMrzL/atHwkgC9ut0U4z2F3JgI0Pbt+PKhzxF7vwQFXB
BNH6xy0guuj2FEZ+jnicugpcABqKVtaypFZREs7EuESVfSCCTVbiuM2DLkbp1OKm
5aRnyifd9p0JNn71ApoJEcpx6shjWSo4fsVS26FmnW/+vyYKJ3y4wUctcLJSitVx
YtfeJ0wxlAa9PtkcHUkw/C0+DmW3aI1mK9N2rxXGIqOQ1fQVk270dp7nvpdcQbds
adhTRd/sm0kt9f83MwVwqeFWpuVoLEAQa2T6HTaVP9lSDvT+I3abZmR1dOwu8EG+
r0U0MrL5gMgOhLIbUXTBt9Heyo725ODaPiYJin/gmytAbL+WYJWG7hIardNg9vY/
74OtFw/Dd12DixO1IamkV0SiH7PqHpxnOZgJ+D+YLF0JilPXEpNOZ9CVFmX5++/D
VwOq+fEwo7l3c/CVt/7NRQrJTf8MeYkA2epVlBmUl8/ITihJ2qigQiqIBn91Pr5S
qt7HPHww2eRTYdqT+HHnTIkkFDywM5nToA5THA5Y+pyMgScVQ0U=
=UEB3
-----END PGP SIGNATURE-----
pinheadmz's public key is on openpgp.org
|
|
||
| namespace interfaces { | ||
| //! Interface giving clients ability to emulate RPC calls. | ||
| class Rpc |
There was a problem hiding this comment.
Could use a mention in src/interfaces/README.md
There was a problem hiding this comment.
| @@ -7,6 +7,7 @@ | |||
|
|
|||
| #include <functional> | |||
| #include <memory> | |||
| #include <string> | |||
There was a problem hiding this comment.
re: #32297 (comment)
unused?
IIUC, std::string does seem to be used in the Ipc::connectAddress and Ipc::listenAddress declarations below so keeping for now
A missing Init::makeMining implementation was causing internal code using the
mining interface (like the `waitforblockheight` RPC method) to not work when
running inside the `bitcoin-gui` binary. It was working the other bitcoin
binaries ('bitocind`, `bitcoin-qt`, and `bitcoin-node`) because they
implmemented `Init::makeMining` methods in commit
8ecb681 from bitcoin#30200, but the `bitcoin-gui`
init class was forgotten in that change.
This bug was reported by Matthew Zipkin <pinheadmz@gmail.com>
bitcoin#32297 (review)
There was a problem hiding this comment.
Thanks for the review and testing!
Updated ad01440 -> 9ffe57f (pr/ipc-cli.7 -> pr/ipc-cli.8, compare) with suggested bitcoin-node fix and documentation update.
re: #32297 (review)
Although waitforblockheight doesn't work with bitcoin-gui because EnsureMiner fails. That was a surprise to me but may just be expected.
Good catch. This is bug caused by not defining a makeMining method in 8ecb681 from #30200 which should be fixed by 9ffe57f in the latest push. Also I think the bug not specific to this PR and happens in master too with RPC.
| @@ -7,6 +7,7 @@ | |||
|
|
|||
| #include <functional> | |||
| #include <memory> | |||
| #include <string> | |||
There was a problem hiding this comment.
re: #32297 (comment)
unused?
IIUC, std::string does seem to be used in the Ipc::connectAddress and Ipc::listenAddress declarations below so keeping for now
|
|
||
| namespace interfaces { | ||
| //! Interface giving clients ability to emulate RPC calls. | ||
| class Rpc |
There was a problem hiding this comment.
re: #32297 (comment)
< 3D11 p dir="auto">Thanks! AddedCould use a mention in src/interfaces/README.md
This implements an idea from sipa in #28722 (comment) to allow
bitcoin-clito connect to the node via IPC instead of TCP, if the ENABLE_IPC cmake option is enabled and the node has been started with-ipcbind.This feature can be tested with:
The -ipconnect parameter can also be omitted, since this change also makes
bitcoin-cliprefer IPC over HTTP by default, and falling back to HTTP if an IPC connection can't be established.This PR is part of the process separation project.