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. |
|
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.
re-ACK 37cd2c0
Changes since last ACK:
git range-diff ad01440~6..ad01440 37cd2c0764~9..37cd2c0764
Address PR feedback adding a line to interface docs, add mining interface to the GUI for testing, and refactors the test commits.
I rebased this PR at this commit on to #32345 and pummeled the IPC-RPC interface for an hour with no crash, so I think it's important that those libmutliprocess changes get merged as well, and probably should be merged first?
Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 37cd2c076434e7acbdbb20996cf87afb2cb5bc84
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmiUsx0ACgkQ5+KYS2KJ
yToNXg//byf9IRhhpTQ39dvrI4qC9R7ZLo8fCp9yQPP+7Is6mwSVlfVtfRfXlRqa
EGCxTxQttN2hcWyyqdt2+gIQDP/ytROECGWTs7iR24jJcXE01JmM0N42zNKfi949
kUIwVDtLJcN45gEKjVf1DUH/kUAB63VQLAimqcop15uqc0ZFYynxEktyox2/yAEq
MWQ04ctpF3NR2LccIY0pdR7n3zgKe7HWmAVQgh8/AdlxawM0wKMmFheQXF/7jVhM
yYKKkY1Zst/02GmQw3Kfe2mLOmFol0L6EuGuCrQzTSRV9/mlV1Ba8GiGGbjlfwWp
KPQ9ILwTQfCn0LqQ1o2DWuqvCnYIKjajbDcaTd5VO0p1pnQRtYo8NfYVgXz99snG
AeVRyBQO9gvj39nI4PgH9kLK3uW+bxIGwEHRPuAYrhkOGPStUgZa9EAB9M1C6Sry
KVs9l89KZL+XgC1u6DMe8WQUXnBBNtDvp53czw7n6GJv0R4C6pZSiI6qZ5VJ4ouq
67F3vkaKN2GA6mqH2JYYpDL26UiraSW/cg/sXRaGPxLhp/SDzxRSER4/zOxB0uZr
aRNoMRZM9LGkMEkVuZSmBejsAZyxzIQhiJSToqnw0JFqnc7jV524+NoW2r11b1yv
+YNhVjVhXQSgdZ1/XIsAvuhwCYIJdIAuoHw8sJwJzLtFJ4Z2RhI=
=Yr0Y
-----END PGP SIGNATURE-----
pinheadmz's public key is on openpgp.org
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.
Expose ENABLE_IPC build option to function tests so new tests for bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 can test IPC-only features.
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in bitcoin#30437 and `bitcoin-cli` in bitcoin#32297 to find the `bitcoin` binary and call `bitcoin -m` to start nodes with IPC support. This way the new tests can run whenever the ENABLE_IPC build option is enabled, instead of only running when the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
With this change, tests can specify `self.extra_init = [{ipcbind: True}]` to
start a node listening on an IPC socket, instead of needing to choose which
node binary to invoke and what `self.extra_args=[["-ipcbind=..."]]` value to
pass to it.
The eliminates boilerplate code bitcoin#30437 (interface_ipc_mining.py), bitcoin#32297
(interface_ipc_cli.py), and bitcoin#33201 (interface_ipc.py) previously needed in
their test setup.
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.
Expose ENABLE_IPC build option to function tests so new tests for bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 can test IPC-only features.
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in bitcoin#30437 and `bitcoin-cli` in bitcoin#32297 to find the `bitcoin` binary and call `bitcoin -m` to start nodes with IPC support. This way the new tests can run whenever the ENABLE_IPC build option is enabled, instead of only running when the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
Expose ENABLE_IPC build option to function tests so new tests for bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 can test IPC-only features.
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in bitcoin#30437 and `bitcoin-cli` in bitcoin#32297 to find the `bitcoin` binary and call `bitcoin -m` to start nodes with IPC support. This way the new tests can run whenever the ENABLE_IPC build option is enabled, instead of only running when the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
With this change, tests can specify `self.extra_init = [{ipcbind: True}]` to
start a node listening on an IPC socket, instead of needing to choose which
node binary to invoke and what `self.extra_args=[["-ipcbind=..."]]` value to
pass to it.
The eliminates boilerplate code bitcoin#30437 (interface_ipc_mining.py), bitcoin#32297
(interface_ipc_cli.py), and bitcoin#33201 (interface_ipc.py) previously needed in
their test setup.
Expose ENABLE_IPC build option to function tests so new tests for bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 can test IPC-only features.
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in bitcoin#30437 and `bitcoin-cli` in bitcoin#32297 to find the `bitcoin` binary and call `bitcoin -m` to start nodes with IPC support. This way the new tests can run whenever the ENABLE_IPC build option is enabled, instead of only running when the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
With this change, tests can specify `self.extra_init = [{ipcbind: True}]` to
start a node listening on an IPC socket, instead of needing to choose which
node binary to invoke and what `self.extra_args=[["-ipcbind=..."]]` value to
pass to it.
The eliminates boilerplate code bitcoin#30437 (interface_ipc_mining.py), bitcoin#32297
(interface_ipc_cli.py), and bitcoin#33201 (interface_ipc.py) previously needed in
their test setup.
Expose ENABLE_IPC build option to function tests so new tests for bitcoin-mine in bitcoin#30437 and bitcoin-cli in bitcoin#32297 can test IPC-only features.
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in bitcoin#30437 and `bitcoin-cli` in bitcoin#32297 to find the `bitcoin` binary and call `bitcoin -m` to start nodes with IPC support. This way the new tests can run whenever the ENABLE_IPC build option is enabled, instead of only running when the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
With this change, tests can specify `self.extra_init = [{ipcbind: True}]` to
start a node listening on an IPC socket, instead of needing to choose which
node binary to invoke and what `self.extra_args=[["-ipcbind=..."]]` value to
pass to it.
The eliminates boilerplate code bitcoin#30437 (interface_ipc_mining.py), bitcoin#32297
(interface_ipc_cli.py), and bitcoin#33201 (interface_ipc.py) previously needed in
their test setup.
…coin` binary test: Provide path to `bitcoin` binary Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in bitcoin#30437 and `bitcoin-cli` in bitcoin#32297 to find the `bitcoin` binary and call `bitcoin -m` to start nodes with IPC support. This way the new tests can run whenever the ENABLE_IPC build option is enabled, instead of only running when the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in bitcoin#30437 and `bitcoin-cli` in bitcoin#32297 to find the `bitcoin` binary and call `bitcoin -m` to start nodes with IPC support. This way the new tests can run whenever the ENABLE_IPC build option is enabled, instead of only running when the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
Set new `BitcoinTestFramework.binary_paths.bitcoin_bin` property with path to the `bitcoin` wrapper binary. This allows new tests for `bitcoin-mine` in bitcoin#30437 and `bitcoin-cli` in bitcoin#32297 to find the `bitcoin` binary and call `bitcoin -m` to start nodes with IPC support. This way the new tests can run whenever the ENABLE_IPC build option is enabled, instead of only running when the `BITCOIN_CMD` environment variable is set to `bitcoin -m`
With this change, tests can specify `self.extra_init = [{ipcbind: True}]` to
start a node listening on an IPC socket, instead of needing to choose which
node binary to invoke and what `self.extra_args=[["-ipcbind=..."]]` value to
pass to it.
The eliminates boilerplate code bitcoin#30437 (interface_ipc_mining.py), bitcoin#32297
(interface_ipc_cli.py), and bitcoin#33201 (interface_ipc.py) previously needed in
their test setup.
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.