multiprocess: Add capnp wrapper for Chain interface #29409

ryanofsky · 2024-02-08T13:54:40Z

Changes making it possible to call interface::Chain over a socket.

Note: The changes in this PR allow a bitcoin-node process to be controlled over a socket. The socket can either be an UNIX socket created by specifying the -ipcbind option, or it can be a socketpair created by a controlling process and passed to bitcoin-node via the -ipcfd=<n> with a file descriptor number. This PR is a dependency of #10102 which allows the wallet code to run in a separate process and use the Chain interface.

This PR is part of the process separation project.

DrahtBot · 2024-02-08T13:54:43Z

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/29409.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	josibake, darosior
Stale ACK	TheCharlatan
User requested bot ignore	cbergqvist

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)

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.

DrahtBot · 2024-02-08T15:41:22Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/21365328047}

cbergqvist

I'm eager for multiprocess to get in, but unable to do in-depth reviews in tolerable time due to lack of domain-knowledge. Did a surface level review of 2fd1042. Hope you don't mind.

src/ipc/capnp/common-types.h

src/ipc/capnp/chain-types.h

src/ipc/capnp/common-types.h

ryanofsky

Thanks for the review!

Updated 2fd1042 -> 41831ab (pr/ipc-chain.2 -> pr/ipc-chain.3, compare) with suggested changes.

src/ipc/capnp/common-types.h

src/ipc/capnp/chain-types.h

src/ipc/capnp/common-types.h

cbergqvist

Thanks for incorporating some of my suggestions. Surface-level ACK of 41831ab. :)

ariard

being able to work with any socket (TCP or UNIX) makes a lot of sense.
you could have bitcoin-node on some secure computing host.
and then one or more bitcoin-walet on over-the-network hosts.

ariard · 2024-03-07T02:54:34Z

src/ipc/capnp/common-types.h

+template<typename S>
+auto Wrap(S& s)
+{
+    return ParamsStream{s, TX_WITH_WITNESS, CAddress::V2_NETWORK};


i think Wrap could be parameterized with any CAddress version.
e.g useful if someone wanna test the native crypto noise framework between bitcoin processes.
or in the future experiment with better crypto-primitives for bip324.

re: #29409 (comment)

i think Wrap could be parameterized with any CAddress version. e.g useful if someone wanna test the native crypto noise framework between bitcoin processes. or in the future experiment with better crypto-primitives for bip324.

For now the Wrap function is basically a workaround for not having a serialization option that says "serialize all the data in this object to a stream, I don't care about the specific serialization format." So the formats here just chosen to preserve all data, and don't really matter otherwise.

But I could imagine use-cases like you are suggesting where we may want to customize which serialization format is used depending on the IPC connection. I think it would not be that hard to do by adding parameters to Wrap, which is not called many places, or by writing different CustomReadField/BuildField overloads which do not call Wrap. For now though the easiest thing is for the wrapped stream to just hardcode parameters that serialize all the data.

TheCharlatan · 2024-07-05T10:14:06Z

I'm not sure what a good approach for reviewing this might be. How can I validate the changes without tests? Should I be running this as part of a parent pull request? The first two commits seem fairly mechanical, but the last one less so. The comments in the code are helpful, but some pointers in the PR description for what to look out for, or a broad description of the approaches taken would be helpful too. E.g. why is so much custom functionality introduced for findBlock?

ryanofsky · 2024-07-08T16:38:44Z

re: #29409 (comment)

Thanks for the questions.

I'm not sure what a good approach for reviewing this might be. How can I validate the changes without tests? Should I be running this as part of a parent pull request?

You could test this as part of #10102, and I should probably add some unit test code to ipc_test.cpp to make sure new runtime code in capnp/chain.cpp and capnp/common-types.h functions has test coverage.

Most of the code in the PR is evaluated at compile time though, so for example, if there is a mismatch in any of the interface declarations there will be build errors.

The first two commits seem fairly mechanical, but the last one less so. The comments in the code are helpful, but some pointers in the PR description for what to look out for, or a broad description of the approaches taken would be helpful too.

This is helpful feedback, and I will try to add more comments. I know it needs more comments, but it's hard for me to know where to add them without being asked because I'm too familiar with everything here. This PR is a case where "Don't spend more than a few seconds trying to understand it" review really advice applies, so if something doesn't make sense, you should just ask so I can clarify. If something doesn't make sense to you, it probably doesn't make sense to other people either.

E.g. why is so much custom functionality introduced for findBlock?

Will add a comment, but Chain::findBlock and the handful of other Chain methods taking FoundBlock input/output arguments need custom code because these are converted to FoundBlockParam and FoundBlockResult Cap'n Proto structs to make IPC calls, and these structs are recursive. Libmultiprocess has decent support for converting back and forth between C++ classes and capnp structs generally, but it would be hard for it support recursive structs. It also might have a hard time in this case because there is a not 1:1 mapping between fields in the c++ class and capnp structs. So the conversion for these types is completely custom.

Similar custom code also exists for the C++ BlockInfo struct which is complicated to translate to the Capnp BlockInfo struct, because it has internal pointers and references, so it has custom code because it can't really be converted automatically.

ryanofsky

Updated ab6b795 -> eec0d31 (pr/ipc-chain.5 -> pr/ipc-chain.6, compare), rebasing and dropping no longer needed code after libmultiprocess update, adding comments and tests, and splitting up commits. There is still more to do to splitting up commits and adding tests, however.

This PR now shares a little bit of code with #30510, so will get smaller if #30510 is merged. #30510 is also simpler and should be easier to review, so will mark this as a draft, although feedback is still welcome on this PR especially if there are questions or things that need more explanation.

re: #29409 (review)

being able to work with any socket (TCP or UNIX) makes a lot of sense. you could have bitcoin-node on some secure computing host. and then one or more bitcoin-walet on over-the-network hosts

Yes this PR does not actually expose a socket but #30509 does, and #19460 uses it for the wallet (and #19461 uses it for the gui, and Sjors#48 and #30437 use it for the mining interface

ryanofsky · 2024-07-26T16:29:15Z

src/ipc/capnp/common-types.h

+template<typename S>
+auto Wrap(S& s)
+{
+    return ParamsStream{s, TX_WITH_WITNESS, CAddress::V2_NETWORK};


re: #29409 (comment)

i think Wrap could be parameterized with any CAddress version. e.g useful if someone wanna test the native crypto noise framework between bitcoin processes. or in the future experiment with better crypto-primitives for bip324.

For now the Wrap function is basically a workaround for not having a serialization option that says "serialize all the data in this object to a stream, I don't care about the specific serialization format." So the formats here just chosen to preserve all data, and don't really matter otherwise.

But I could imagine use-cases like you are suggesting where we may want to customize which serialization format is used depending on the IPC connection. I think it would not be that hard to do by adding parameters to Wrap, which is not called many places, or by writing different CustomReadField/BuildField overloads which do not call Wrap. For now though the easiest thing is for the wrapped stream to just hardcode parameters that serialize all the data.

This fixes behavior reported by Antoine Poinsot <darosior@protonmail.com> bitcoin#29409 (comment) where if an IPC client is connected, the node will wait forever for it to disconnect before exiting.

zaidmstrr · 2025-07-21T12:03:37Z

The method rpcRunLater was removed in commit, thus there are some build errors.

src/ipc/capnp/chain.cpp

- Add capnp ToBlob, ToArray, Wrap, Serialize, and Unserialize helper functions - Add support for std::chrono::seconds capnp serialization - Add support for util::Result capnp serialization

Expose Chain interface to external processes spawning or connecting to bitcoin-node.

ryanofsky · 2025-07-24T16:19:51Z

Rebased f92422e -> c0d9515 (pr/ipc-chain.14 -> pr/ipc-chain.15, compare) to fix silent merge conflict with #32862 and spelling error. (Thanks zaidmstrr and maflcko for pointing these out!)

maflcko · 2025-07-24T20:18:18Z

For reference, the lint task still fails. Not sure if this is intentional or if you missed it.

ryanofsky · 2025-07-28T14:29:20Z

re: #29409 (comment)

For reference, the lint task still fails. Not sure if this is intentional or if you missed it.

Thanks! The lint task was failing in https://cirrus-ci.com/task/5047676548415488 because the subtree was modified. The subtree was modified to work around a bug fixed in bitcoin-core/libmultiprocess#181 which is part of #32345 but not currently part of master. Implemented a different workaround for now.

Updated c0d9515 -> b372063 (pr/ipc-chain.15 -> pr/ipc-chain.16, compare) to fix failing lint task

This fixes behavior reported by Antoine Poinsot <darosior@protonmail.com> bitcoin#29409 (comment) where if an IPC client is connected, the node will wait forever for it to disconnect before exiting.

ryanofsky mentioned this pull request Feb 8, 2024

Multiprocess tracking issue #28722

Open

87 tasks

ryanofsky marked this pull request as ready for review February 8, 2024 14:04

DrahtBot added the CI failed label Feb 8, 2024

ryanofsky force-pushed the pr/ipc-chain branch from 22e0549 to 2fd1042 Compare February 8, 2024 20:45

DrahtBot removed the CI failed label Feb 8, 2024

DrahtBot mentioned this pull request Feb 21, 2024

serialization: Support for multiple parameters #28929

Merged

cbergqvist reviewed Feb 28, 2024

View reviewed changes

src/ipc/capnp/common-types.h Show resolved Hide resolved

src/ipc/capnp/chain-types.h Outdated Show resolved Hide resolved

src/ipc/capnp/common-types.h Outdated Show resolved Hide resolved

ryanofsky force-pushed the pr/ipc-chain branch from 2fd1042 to 41831ab Compare February 28, 2024 16:08

ryanofsky commented Feb 28, 2024

View reviewed changes

src/ipc/capnp/common-types.h Show resolved Hide resolved

src/ipc/capnp/chain-types.h Outdated Show resolved Hide resolved

src/ipc/capnp/common-types.h Outdated Show resolved Hide resolved

cbergqvist approved these changes Feb 28, 2024

View reviewed changes

DrahtBot added the CI failed label Feb 28, 2024

ariard reviewed Mar 7, 2024

View reviewed changes

DrahtBot marked this pull request as draft April 17, 2024 17:34

DrahtBot added the Needs rebase label May 15, 2024

ryanofsky force-pushed the pr/ipc-chain branch from 41831ab to 6608b03 Compare June 11, 2024 01:53

ryanofsky marked this pull request as ready for review June 11, 2024 01:55

DrahtBot removed the Needs rebase label Jun 11, 2024

ryanofsky mentioned this pull request Jun 11, 2024

Multiprocess bitcoin #10102

Draft

ryanofsky force-pushed the pr/ipc-chain branch from 6608b03 to ab6b795 Compare June 11, 2024 19:10

DrahtBot removed the CI failed label Jun 12, 2024

DrahtBot mentioned this pull request Jul 12, 2024

ipc: add bitcoin-mine test program #30437

Open

ryanofsky mentioned this pull request Jul 23, 2024

multiprocess: Add -ipcbind option to bitcoin-node #30509

Merged

DrahtBot mentioned this pull request Jul 23, 2024

multiprocess: Add IPC wrapper for Mining interface #30510

Merged

ryanofsky force-pushed the pr/ipc-chain branch from ab6b795 to eec0d31 Compare July 26, 2024 16:05

ryanofsky commented Jul 26, 2024

View reviewed changes

ryanofsky marked this pull request as draft July 26, 2024 16:35

ryanofsky mentioned this pull request Jun 27, 2025

Tracepoint-like interface via libmultiprocess and IPC communication bitcoin-core/libmultiprocess#185

Closed

DrahtBot mentioned this pull request Jul 8, 2025

POC: IPC tracing interface #32898

Closed

maflcko reviewed Jul 21, 2025

View reviewed changes

src/ipc/capnp/chain.cpp Outdated Show resolved Hide resolved

zaidmstrr mentioned this pull request Jul 22, 2025

refactor: Moving removeTxFromMempool to mining interface braidpool/bitcoin#6

Merged

ryanofsky and others added 4 commits July 23, 2025 11:44

Add capnp serialization code for bitcoin types

b901d5e

- Add capnp ToBlob, ToArray, Wrap, Serialize, and Unserialize helper functions - Add support for std::chrono::seconds capnp serialization - Add support for util::Result capnp serialization

Add capnp wrapper for Handler interface

5bfa778

Add capnp wrapper for Chain interface

704ae74

multiprocess: Expose Chain interface

b372063

Expose Chain interface to external processes spawning or connecting to bitcoin-node.

ryanofsky force-pushed the pr/ipc-chain branch from f92422e to c0d9515 Compare July 24, 2025 16:18

ryanofsky force-pushed the pr/ipc-chain branch from c0d9515 to b372063 Compare July 28, 2025 14:25

DrahtBot removed the CI failed label Jul 28, 2025

zaidmstrr mentioned this pull request Jul 31, 2025

feat: Add valid template creator braidpool/braidpool#234

Merged

ryanofsky mentioned this pull request Aug 19, 2025

Add bitcoin-{node,gui} to release binaries for IPC #31802

Merged

multiprocess: Add capnp wrapper for Chain interface #29409

Are you sure you want to change the base?

multiprocess: Add capnp wrapper for Chain interface #29409

Uh oh!

Conversation

ryanofsky commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Feb 8, 2024

Uh oh!

cbergqvist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbergqvist left a comment

Choose a reason for hiding this comment

Uh oh!

ariard left a comment

Choose a reason for hiding this comment

Uh oh!

ariard Mar 7, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

TheCharlatan commented Jul 5, 2024

Uh oh!

ryanofsky commented Jul 8, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

zaidmstrr commented Jul 21, 2025

Uh oh!

Uh oh!

ryanofsky commented Jul 24, 2025

Uh oh!

maflcko commented Jul 24, 2025

Uh oh!

ryanofsky commented Jul 28, 2025

Uh oh!

Uh oh!

ryanofsky commented Feb 8, 2024 •

edited

Loading

DrahtBot commented Feb 8, 2024 •

edited

Loading