multiprocess: Add capnp wrapper for Chain interface by ryanofsky · Pull Request #29409 · bitcoin/bitcoin

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 and AI policy for information on the review process.

Type	Reviewers
Concept ACK	josibake, darosior, willcl-ark
Stale ACK	cbergqvist, sedited

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

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.

ryanofsky

Thanks for the review!

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

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

+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.

sedited · 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

+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.

sedited · 2026-03-04T17:34:32Z

I'm curious to check if this #29409 (comment) is now fixed once this is rebased.

Sjors · 2026-04-01T10:23:57Z

I think we need to consider if shipping capnp files for this (complete) interface is the right move. But in order to not distract from the code review here, I opened a separate issue - that turned into a bit of an essay: #34981

sedited · 2026-04-20T13:32:09Z

Updated darosior's core_bdk_wallet again, and bitcoin-node still crashes on disconnect:

Click for backtrace

2026-04-20T13:27:37Z [ipc] {bitcoin-node-589794/b-capnp-loop-589798} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages5ChainEEE
[Thread 0x7fff54ff96c0 (LWP 590165) exited]

Thread 3 "b-capnp-loop" received signal SIGPIPE, Broken pipe.
[Switching to Thread 0x7ffff61fe6c0 (LWP 589798)]
0x00007ffff73298cb in __GI___writev (iovcnt=2, iov=0x7ffff61fc130, fd=26) at ../sysdeps/unix/sysv/linux/writev.c:26
warning: 26	../sysdeps/unix/sysv/linux/writev.c: No such file or directory
(gdb) bt
#0  0x00007ffff73298cb in __GI___writev (iovcnt=2, iov=0x7ffff61fc130, fd=26) at ../sysdeps/unix/sysv/linux/writev.c:26
#1  __GI___writev (fd=26, iov=0x7ffff61fc130, iovcnt=2) at ../sysdeps/unix/sysv/linux/writev.c:24
#2  0x00007ffff7c8b987 in ?? () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
#3  0x00007ffff7c8c394 in ?? () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
#4  0x00007ffff7da696f in capnp::writeMessages(kj::AsyncOutputStream&, kj::ArrayPtr<kj::ArrayPtr<kj::ArrayPtr<capnp::word const> const> >) ()
   from /lib/x86_64-linux-gnu/libcapnp-rpc-1.0.1.so
#5  0x00007ffff7da6e1c in capnp::BufferedMessageStream::writeMessages(kj::ArrayPtr<kj::ArrayPtr<kj::ArrayPtr<capnp::word const> const> >) ()
   from /lib/x86_64-linux-gnu/libcapnp-rpc-1.0.1.so
#6  0x00007ffff7da4865 in capnp::MessageStream::writeMessages(kj::ArrayPtr<capnp::MessageAndFds>) () from /lib/x86_64-linux-gnu/libcapnp-rpc-1.0.1.so
#7  0x00007ffff7df9419 in capnp::TwoPartyVatNetwork::OutgoingMessageImpl::send()::{lambda()#1}::operator()() const::{lambda()#1}::operator()() const ()
   from /lib/x86_64-linux-gnu/libcapnp-rpc-1.0.1.so
#8  0x00007ffff7e03e4a in ?? () from /lib/x86_64-linux-gnu/libcapnp-rpc-1.0.1.so
#9  0x00007ffff7c4161d in kj::_::TransformPromiseNodeBase::get(kj::_::ExceptionOrValue&) () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
#10 0x00007ffff7c4245d in kj::_::ChainPromiseNode::fire() () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
#11 0x00007ffff7c38509 in kj::EventLoop::turn() () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
#12 0x00007ffff7c3e6c9 in kj::_::waitImpl(kj::Own<kj::_::PromiseNode, kj::_::PromiseDisposer>&&, kj::_::ExceptionOrValue&, kj::WaitScope&, kj::SourceLocation) ()
   from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
#13 0x00005555561d6220 in kj::Promise<unsigned long>::wait (this=<optimized out>, waitScope=..., location=...) at /usr/include/kj/async-inl.h:1357
#14 0x00005555561d2bd2 in mp::EventLoop::loop (this=0x555556b9e378) at /home/drgrid/bitcoin/src/ipc/libmultiprocess/src/mp/proxy.cpp:244
#15 0x0000555555ae97b4 in ipc::capnp::(anonymous namespace)::CapnpProtocol::startLoop(char const*)::{lambda()#1}::operator()() const (this=0x555556b9fff8)
    at /home/drgrid/bitcoin/src/ipc/capnp/protocol.cpp:136
#16 std::__invoke_impl<void, ipc::capnp::(anonymous namespace)::CapnpProtocol::startLoop(char const*)::{lambda()#1}>(std::__invoke_other, ipc::capnp::(anonymous namespace)::CapnpProtocol::startLoop(char const*)::{lambda()#1}&&) (__f=...) at /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:61
#17 std::__invoke<ipc::capnp::(anonymous namespace)::CapnpProtocol::startLoop(char const*)::{lambda()#1}>(ipc::capnp::(anonymous namespace)::CapnpProtocol::startLoop(char const*)::{lambda()#1}&&) (__fn=...) at /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:96
#18 std::thread::_Invoker<std::tuple<ipc::capnp::(anonymous namespace)::CapnpProtocol::startLoop(char const*)::{lambda()#1}> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) (
    this=0x555556b9fff8) at /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_thread.h:292
#19 std::thread::_Invoker<std::tuple<ipc::capnp::(anonymous namespace)::CapnpProtocol::startLoop(char const*)::{lambda()#1}> >::operator()() (this=0x555556b9fff8)
    at /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_thread.h:299
#20 std::thread::_State_impl<std::thread::_Invoker<std::tuple<ipc::capnp::(anonymous namespace)::CapnpProtocol::startLoop(char const*)::{lambda()#1}> > >::_M_run() (this=0x555556b9fff0)
    at /usr/lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_thread.h:244
#21 0x00007ffff76ecdb4 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
#22 0x00007ffff729caa4 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:447
#23 0x00007ffff7329c6c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
(gdb)

ryanofsky · 2026-04-21T15:38:20Z

Updated darosior's core_bdk_wallet again, and bitcoin-node still crashes on disconnect:

Thanks, this is expected and bitcoin-core/libmultiprocess#219 was created earlier to track this. Rust wallet is not disconnecting cleanly and some destructors need to be marked noexcept to prevent this

xyzconstant · 2026-04-27T18:23:23Z

+interface ChainClient $Proxy.wrap("interfaces::ChainClient") {
+    destroy @0 (context :Proxy.Context) -> ();
+    registerRpcs @1 (context :Proxy.Context) -> ();
+    verify @2 (context :Proxy.Context) -> (result :Bool);
+    load @3 (context :Proxy.Context) -> (result :Bool);
+    start @4 (context :Proxy.Context, scheduler :Void) -> ();
+    stop @5 (context :Proxy.Context) -> ();
+    setMockTime @6 (context :Proxy.Context, time :Int64) -> ();
+    schedulerMockForward @7 (context :Proxy.Context, deltaSeconds :Int64) -> ();
+}


Out of scope for this PR, just raising this out of curiosity:

I'm working on an IPC extractor for peer-observer, a non-wallet client that consumes bitcoin-node's interfaces for monitoring. While extending it to cover the Chain interface exposed in this PR, I started to think whether such a tool could fit the ChainClient contract. The doc-comment on ChainClient specifically states this:

Interface to let node manage chain clients (wallets, or maybe tools for monitoring and analysis in the future).

Three of those methods feel pretty wallet-shaped: registerRpcs(), verify() and load(). A non-wallet implementer wouldn't necessarily expose RPCs or have saved state to load/verify.

Would it be worth reducing ChainClient to just lifecycle + mock-time hooks and moving the rest to WalletLoader? Genuinely curious whether there's prior context I'm missing.

re: #29409 (comment)

I'm working on an IPC extractor for peer-observer, a non-wallet client that consumes bitcoin-node's interfaces for monitoring. While extending it to cover the Chain interface exposed in this PR, I started to think whether such a tool could fit the ChainClient contract. The doc-comment on ChainClient specifically states this:

Interface to let node manage chain clients (wallets, or maybe tools for monitoring and analysis in the future).

It's a good question. The doc comment is too ambiguous and should be improved. ChainClient declares methods that bitcoin-node calls and wallets and indexes can implement. It's used in #10102 to let bitcoin-node start a bitcoin-wallet process and control it. In the future the node could also start other processes (indexes, etc) and use this interface to control them. ChainClient probably isn't as likely to be useful for Chain users connecting via IPC as opposed to Chain users being spawned by the node.

I get it now, thanks you!

Sjors · 2026-05-12T19:56:10Z

Here's the (partially implemented) Rust bindings: 2140-dev/bitcoin-capnp-types#22

And Sjors/electrs#1 is a proof-of-concept / demo that modifies Electrs to use IPC instead of RPC and p2p for a bunch of things, notably:

fetching historical blocks (previously used local p2p getblock)
transaction broadcast (previously used send_raw_transaction RPC)
tip updates (previously used inv)
header sync (previously used p2p, though it now gets the whole block via the chain interface, so that's not very efficient)
mempool sync (previously used getrawmempool)

willcl-ark

Concept ACK.

Exposing interfaces::Chain over IPC seems very useful, especially for downstream users. I have been using a version of this branch myself on a node "dashboard" as some dog-fooding at: https://tracing.fish.foo

Long term, I think IPC should become the preferred integration boundary for programmatic interaction with Bitcoin Core. I'd like to see us move more users away from linking into node internals, and eventually maybe even from RPC/REST/ZMQ! when IPC can provide a cleaner interface.

The overall approach of reusing the existing interfaces layer makes sense to me. I will continue to test and review a little more. Left a few nits inline.

willcl-ark · 2026-05-20T10:51:17Z

+    isRBFOptIn @15 (context :Proxy.Context, tx :Data) -> (result :Int32);
+    isInMempool @16 (context :Proxy.Context, txid :Data) -> (result :Bool);
+    hasDescendantsInMempool @17 (context :Proxy.Context, txid :Data) -> (result :Bool);
+    broadcastTransaction @18 (context :Proxy.Context, tx: Data, maxTxFee :Int64, broadcastMethod :Int32) -> (error: Text, result :Bool);


In 1c9dcf2

broadcastmethod is switched on (e.g. here) with no default case, as is our convention. but we don't check incoming value is mapped to the enum safely.

In 1c9dcf2

Also what happens here is tx is a null pointer (allowed by capnp spec AFAIU)? I think we will immediately try to deference it in BroadcastTransaction and segfault.

re: #29409 (comment)

broadcastmethod is switched on (e.g. here) with no default case, as is our convention. but we don't check incoming value is mapped to the enum safely.

Also what happens here is tx is a null pointer (allowed by capnp spec AFAIU)? I think we will immediately try to deference it in BroadcastTransaction and segfault.

IIUC, the concern in both of these cases is that non-C++ clients calling this interface (like rust or python clients) could call this method but leave the tx argument unset or leave the broadcastMethod argument unset, and then the implementation of this method could crash or fail if it is not checking for invalid values.

This is a legitimate concern, and it's not specific to this method or to these parameters. In C++, the compiler enforces at each call site that explicit values need to be passed for all parameters which don't have defaults. But in capnproto, all parameters are optional because fields without explicit defaults have implicit defaults of 0 or null.

The Chain interface was designed to be called by C++ clients, and with C++ clients this issue does not occur because clients don't need to interact with cap'nproto directly. They can call C++ methods with all normal language restrictions enforced, and the calls are forwarded over capnproto.

But if non-c++ other clients are used, they either need to be careful when they omit parameters, or the interface needs to be changed to allow parameters to be safely omitted. One way to do this is to assign default values in interfaces like c6638fa. Another way is to make method implementations handle invalid values better, for example by throwing exceptions instead of asserting.

willcl-ark · 2026-05-20T10:54:01Z

+    isInMempool @16 (context :Proxy.Context, txid :Data) -> (result :Bool);
+    hasDescendantsInMempool @17 (context :Proxy.Context, txid :Data) -> (result :Bool);
+    broadcastTransaction @18 (context :Proxy.Context, tx: Data, maxTxFee :Int64, broadcastMethod :Int32) -> (error: Text, result :Bool);
+    getTransactionAncestry @19 (context :Proxy.Context, txid :Data) -> (ancestors :UInt64, descendants :UInt64, ancestorsize :UInt64, ancestorfees :Int64);


In 1c9dcf2

even though these are positional, I think we can rename descendants -> cluster_count post-cluster mempool (and as this is public facing).

re: #29409 (comment)

even though these are positional, I think we can rename descendants -> cluster_count post-cluster mempool (and as this is public facing).

Good catch. It looks like the parameter was renamed in commit 957ae23 and this PR was not updated. Currently there is checking to make sure capnproto parameter types and c++ types are compatible (compilation will fail otherwise) but there it isn't checked that parameters have the same names so sometimes names can get out of sync.

willcl-ark · 2026-05-20T10:58:22Z

+    calculateIndividualBumpFees @20 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result: List(Common.PairInt64(Data)));
+    calculateCombinedBumpFee @21 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result :Int64, hasResult :Bool);
+    getPackageLimits @22 (context :Proxy.Context) -> (ancestors :UInt32, descendants :UInt32);
+    checkChainLimits @23 (context :Proxy.Context, tx :Data) -> (result :Common.ResultVoid);


In 1c9dcf2

I wonder if this tx isn't the same as 1c9dcf2 (this PR) ?

CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition is going to try and dereference.

re: #29409 (comment)

I wonder if this tx isn't the same as 1c9dcf2 (this PR) ?

CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition is going to try and dereference.

Yes this is the same issue. checkChainLimits calls CheckPolicyLimits which calls StageAddition which does not check whether tx is null before dereferencing.

And In this case, the issue applies to C++ clients as well as rust and python clients because C++ clients can also call checkChainLimits with a null pointer.

Depending on what the goal is, there are various ways this could be addressed. One way would be to change checkChainLimits to throw an exception if a null pointer is passed.

In general it's not a realistic goal for this PR to ensure that it's safe to call all Chain methods with all possible arguments and ensure that the node will not crash, but it should be good to do where feasible.

willcl-ark · 2026-05-20T11:12:16Z

+    return {};
+}
+
+void ProxyServerMethodTraits<ipc::capnp::messages::ChainClient::StartParams>::invoke(ChainContext& context)


In 1c9dcf2

This assert is dead code only because ChainClient is not exposed by this PR.

The comment says it is overridden by WalletLoader::start, but would this not still abort if ever called?

Why not omit start from the exposed base interface or make this throw a protocol error instead of asserting.

re: #29409 (comment)

In 1c9dcf2

This assert is dead code only because ChainClient is not exposed by this PR.

ChainClient is actually exposed by this PR. But depending on what we want the goal of this PR it doesn't need to be. I tried to describe purpose of ChainClient better in a recent comment #29409 (comment), but basically it is not something you would implement if you are an external process connecting to the node, it is only something you would implement if you are implementing a process you would want to be started by the node. In #10102, bitcoin-node spawns a bitcoin-wallet process, and bitcoin-wallet implements the ChainClient interfaces, so the node can manage it and forward RPCs and shut it down.

Original goal of this PR was to add chain.capnp to the codebase so #10102 is easier to review. But if the goal of this PR is now really to expose the Chain interface over IPC, the ChainClient capnproto definition can be dropped from this.

The comment says it is overridden by WalletLoader::start, but would this not still abort if ever called?

I don't think there is a realistic way this could ever be called. This is here as workaround for the fact the libmultiprocess (and capnproto) don't have separate options for generating client and server classes. So if you declare any interface, client and server classes will be produced and they need to compile. Libmultiprocess can't generate ChainClient::start server method that compiles because it takes a CScheduler& argument and there isn't a way to share schedulers across processes. In #10102, the WalletLoader implementation of start just creates a separate scheduler for the wallet process to use.

More ideally, there might be a way to mark ChainClient as an abstract class that is allowed to be called but never implemented directly as a server object, only inherited from and implement that way. Capnproto doesn't provide this but libmultiprocess could define it's own $Abstract annotation that would do this.

Why not omit start from the exposed base interface or make this throw a protocol error instead of asserting.

This could throw an exception instead of assert. It shouldn't matter very much because I don't think there's a way this could be accidentally called.

Another way to get rid of this might be drop the CScheduler parameter and just have the wallet and node use different schedulers instead of the same one.

ryanofsky

I spent way too long looking into the ChainClient::start thing, but thanks for the questions and putting the interface to work!

ryanofsky · 2026-05-20T12:36:47Z

+    isRBFOptIn @15 (context :Proxy.Context, tx :Data) -> (result :Int32);
+    isInMempool @16 (context :Proxy.Context, txid :Data) -> (result :Bool);
+    hasDescendantsInMempool @17 (context :Proxy.Context, txid :Data) -> (result :Bool);
+    broadcastTransaction @18 (context :Proxy.Context, tx: Data, maxTxFee :Int64, broadcastMethod :Int32) -> (error: Text, result :Bool);


re: #29409 (comment)

broadcastmethod is switched on (e.g. here) with no default case, as is our convention. but we don't check incoming value is mapped to the enum safely.

Also what happens here is tx is a null pointer (allowed by capnp spec AFAIU)? I think we will immediately try to deference it in BroadcastTransaction and segfault.

IIUC, the concern in both of these cases is that non-C++ clients calling this interface (like rust or python clients) could call this method but leave the tx argument unset or leave the broadcastMethod argument unset, and then the implementation of this method could crash or fail if it is not checking for invalid values.

This is a legitimate concern, and it's not specific to this method or to these parameters. In C++, the compiler enforces at each call site that explicit values need to be passed for all parameters which don't have defaults. But in capnproto, all parameters are optional because fields without explicit defaults have implicit defaults of 0 or null.

The Chain interface was designed to be called by C++ clients, and with C++ clients this issue does not occur because clients don't need to interact with cap'nproto directly. They can call C++ methods with all normal language restrictions enforced, and the calls are forwarded over capnproto.

But if non-c++ other clients are used, they either need to be careful when they omit parameters, or the interface needs to be changed to allow parameters to be safely omitted. One way to do this is to assign default values in interfaces like c6638fa. Another way is to make method implementations handle invalid values better, for example by throwing exceptions instead of asserting.

ryanofsky · 2026-05-20T12:44:46Z

+    isInMempool @16 (context :Proxy.Context, txid :Data) -> (result :Bool);
+    hasDescendantsInMempool @17 (context :Proxy.Context, txid :Data) -> (result :Bool);
+    broadcastTransaction @18 (context :Proxy.Context, tx: Data, maxTxFee :Int64, broadcastMethod :Int32) -> (error: Text, result :Bool);
+    getTransactionAncestry @19 (context :Proxy.Context, txid :Data) -> (ancestors :UInt64, descendants :UInt64, ancestorsize :UInt64, ancestorfees :Int64);


re: #29409 (comment)

even though these are positional, I think we can rename descendants -> cluster_count post-cluster mempool (and as this is public facing).

Good catch. It looks like the parameter was renamed in commit 957ae23 and this PR was not updated. Currently there is checking to make sure capnproto parameter types and c++ types are compatible (compilation will fail otherwise) but there it isn't checked that parameters have the same names so sometimes names can get out of sync.

ryanofsky · 2026-05-20T13:10:08Z

+    calculateIndividualBumpFees @20 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result: List(Common.PairInt64(Data)));
+    calculateCombinedBumpFee @21 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result :Int64, hasResult :Bool);
+    getPackageLimits @22 (context :Proxy.Context) -> (ancestors :UInt32, descendants :UInt32);
+    checkChainLimits @23 (context :Proxy.Context, tx :Data) -> (result :Common.ResultVoid);


re: #29409 (comment)

I wonder if this tx isn't the same as 1c9dcf2 (this PR) ?

CTxMemPool::ChangeSet::TxHandle CTxMemPool::ChangeSet::StageAddition is going to try and dereference.

Yes this is the same issue. checkChainLimits calls CheckPolicyLimits which calls StageAddition which does not check whether tx is null before dereferencing.

And In this case, the issue applies to C++ clients as well as rust and python clients because C++ clients can also call checkChainLimits with a null pointer.

Depending on what the goal is, there are various ways this could be addressed. One way would be to change checkChainLimits to throw an exception if a null pointer is passed.

In general it's not a realistic goal for this PR to ensure that it's safe to call all Chain methods with all possible arguments and ensure that the node will not crash, but it should be good to do where feasible.

ryanofsky · 2026-05-20T14:18:41Z

+    return {};
+}
+
+void ProxyServerMethodTraits<ipc::capnp::messages::ChainClient::StartParams>::invoke(ChainContext& context)


re: #29409 (comment)

In 1c9dcf2

This assert is dead code only because ChainClient is not exposed by this PR.

ChainClient is actually exposed by this PR. But depending on what we want the goal of this PR it doesn't need to be. I tried to describe purpose of ChainClient better in a recent comment #29409 (comment), but basically it is not something you would implement if you are an external process connecting to the node, it is only something you would implement if you are implementing a process you would want to be started by the node. In #10102, bitcoin-node spawns a bitcoin-wallet process, and bitcoin-wallet implements the ChainClient interfaces, so the node can manage it and forward RPCs and shut it down.

Original goal of this PR was to add chain.capnp to the codebase so #10102 is easier to review. But if the goal of this PR is now really to expose the Chain interface over IPC, the ChainClient capnproto definition can be dropped from this.

The comment says it is overridden by WalletLoader::start, but would this not still abort if ever called?

I don't think there is a realistic way this could ever be called. This is here as workaround for the fact the libmultiprocess (and capnproto) don't have separate options for generating client and server classes. So if you declare any interface, client and server classes will be produced and they need to compile. Libmultiprocess can't generate ChainClient::start server method that compiles because it takes a CScheduler& argument and there isn't a way to share schedulers across processes. In #10102, the WalletLoader implementation of start just creates a separate scheduler for the wallet process to use.

More ideally, there might be a way to mark ChainClient as an abstract class that is allowed to be called but never implemented directly as a server object, only inherited from and implement that way. Capnproto doesn't provide this but libmultiprocess could define it's own $Abstract annotation that would do this.

Why not omit start from the exposed base interface or make this throw a protocol error instead of asserting.

This could throw an exception instead of assert. It shouldn't matter very much because I don't think there's a way this could be accidentally called.

Another way to get rid of this might be drop the CScheduler parameter and just have the wallet and node use different schedulers instead of the same one.

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

DrahtBot · 2026-07-14T01:49:44Z

🚧 At least one of the CI tasks failed.
_{Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/29296250380/job/86970302095}
_{LLM reason (✨ experimental): CI failed at C++ compile time because the IPC capnp proxy client couldn’t instantiate BuildPrimitive for CBlockLocator/Coin (templates failed due to incomplete/unsupported Coin serialization/deserialization).}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

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

ryanofsky mentioned this pull request Feb 8, 2024

Multiprocess tracking issue #28722

Open

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

Comment thread src/ipc/capnp/common-types.h

Comment thread src/ipc/capnp/chain-types.h Outdated

Comment thread src/ipc/capnp/common-types.h Outdated

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

Comment thread src/ipc/capnp/common-types.h

Comment thread src/ipc/capnp/chain-types.h Outdated

Comment thread src/ipc/capnp/common-types.h Outdated

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

DrahtBot mentioned this pull request Mar 2, 2026

mining: add submitBlock to IPC Mining interface #34644

Merged

xyzconstant mentioned this pull request Mar 13, 2026

Add IPC extractor peer-observer/peer-observer#379

Merged

Sjors mentioned this pull request Apr 1, 2026

multiprocess: expose existing interfaces or design new ones? #34981

Open

xyzconstant reviewed Apr 27, 2026

View reviewed changes

ViniciusCestarii mentioned this pull request May 14, 2026

Add std::unordered_set support and a helper BuildList to dedup list build handlers bitcoin-core/libmultiprocess#277

Merged

zaidmstrr mentioned this pull request May 18, 2026

refactor: update schema and include path zaidmstrr/bitcoin-rpc-multiprocess#1

Merged

willcl-ark reviewed May 20, 2026

View reviewed changes

ryanofsky commented May 20, 2026

View reviewed changes

xyzconstant mentioned this pull request May 20, 2026

Extend ipc-extractor to test bitcoin/bitcoin#29409 peer-observer/peer-observer#441

Draft

edilmedeiros mentioned this pull request Jun 4, 2026

Add support for Bitcoin Core IPC interface ClubeBitcoinUnB/BlockSci#8

Open

xyzconstant mentioned this pull request Jun 5, 2026

Add makePool method on ThreadMap bitcoin-core/libmultiprocess#283

Merged

This was referenced Jun 9, 2026

type-number: fix clang-tidy modernize-use-nullptr bitcoin-core/libmultiprocess#292

Merged

IPC interface for LND support 2140-dev/bitcoin#24

Open

DrahtBot mentioned this pull request Jun 12, 2026

RFC: consensus: Make CAmount a class #35511

Open

This was referenced Jun 30, 2026

ipc: support per-address max-connections options on -ipcbind #35037

Open

Update libmultiprocess subtree to add ThreadMap.makePool method #35661

Merged

ryanofsky and others added 4 commits July 13, 2026 17:53

Add capnp serialization code for bitcoin types

16a55c7

- 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

84e9b8a

Add capnp wrapper for Chain interface

72c2513

multiprocess: Expose Chain interface

2eb914c

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sedited 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!

Choose a reason for hiding this comment

Uh oh!

sedited commented Mar 4, 2026

Uh oh!

Sjors commented Apr 1, 2026

Uh oh!

sedited commented Apr 20, 2026

Uh oh!

ryanofsky commented Apr 21, 2026

Uh oh!

xyzconstant Apr 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented May 12, 2026

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ryanofsky commented Feb 8, 2024 •

edited

Loading

DrahtBot commented Feb 8, 2024 •

edited

Loading

xyzconstant Apr 27, 2026 •

edited

Loading

ryanofsky May 20, 2026 •

edited

Loading

ryanofsky May 20, 2026 •

edited

Loading