multiprocess: Add capnp wrapper for Chain interface #29409

ryanofsky commented at 1:54 PM on February 8, 2024: contributor

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.

This is based on https://github.com/bitcoin-core/libmultiprocess/pull/243. The non-base commits are:

DrahtBot commented at 1:54 PM on February 8, 2024: contributor

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, willcl-ark
Stale ACK	cbergqvist, sedited

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34020 (mining: add getTransactions(ByWitnessID) IPC methods by Sjors)

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.

ryanofsky marked this as ready for review on Feb 8, 2024

DrahtBot added the label CI failed on Feb 8, 2024

DrahtBot commented at 3:41 PM on February 8, 2024: contributor

🚧 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

ryanofsky force-pushed on Feb 8, 2024

DrahtBot removed the label CI failed on Feb 8, 2024

in src/ipc/capnp/common-types.h:224 in eeb441ae7c outdated

 219 | +template <typename LocalType, typename Value, typename Output>
 220 | +void CustomBuildField(TypeList<util::Result<LocalType>>, Priority<1>, InvokeContext& invoke_context, Value&& value,
 221 | +                      Output&& output)
 222 | +{
 223 | +    auto result = output.init();
 224 | +    if (value) {

cbergqvist commented at 10:12 PM on February 26, 2024:

Nit: compiles with value.has_value() here, which is clearer than operator bool(). But maybe you were intending to support types which don't have .has_value()?

ryanofsky commented at 3:25 PM on February 28, 2024:

re: #29409 (review)

Nit: compiles with value.has_value() here, which is clearer than operator bool(). But maybe you were intending to support types which don't have .has_value()?

I mostly just don't agree it is clearer and I like the "if value, use value" pattern from c++ and python. I think it removes noise and makes code easier to read, also makes it easier for code to evolve, for example updating code using a plain pointer to switch to smart pointer.

In this case I am looking forward to this code working with ResultPtr from #26022, where operator bool and has_value have different meanings. Operator bool in that case is true if a pointer is contained AND the pointer value is not null. has_value is true in that case if a pointer is contained, whether or not it is null. After #26022, this code would look something like:

if (value) {
     BuildField(..., ValueAccessor, *value);
}
if (!value.has_value()) {
    BuildField(..., FailureAccessor, value.GetFailure());
}
BuildField(..., ErrorsAccessor, value.GetErrors());
BuildField(..., WarningsAccessor, value.GetWarnings());

And if value is a ResultPtr the first line would need to be if (value) not if value.has_value() to work.

in src/ipc/capnp/chain-types.h:15 in 2fd10424c0 outdated

  10 | +#include <ipc/capnp/handler.capnp.proxy-types.h>
  11 | +#include <interfaces/chain.h>
  12 | +#include <policy/fees.h>
  13 | +#include <rpc/server.h>
  14 | +
  15 | +#include <any>

cbergqvist commented at 8:33 AM on February 27, 2024:

Builds fine without #include <any> using g++ on Linux?

ryanofsky commented at 3:26 PM on February 28, 2024:

re: #29409 (review)

Builds fine without #include <any> using g++ on Linux?

Good catch, I think this must have been left over from a previous version of this file. Removed now.

in src/ipc/capnp/common-types.h:338 in 2fd10424c0 outdated

 333 | +}
 334 | +
 335 | +//! Generic ::capnp::Data field builder for any class that a Span can be
 336 | +//! constructed from, particularly BaseHash and base_blob classes and
 337 | +//! subclasses. It's also used to serialize vector<unsigned char>
 338 | +//! GCSFilter::ElementSet set elements.

cbergqvist commented at 1:51 PM on February 28, 2024:

Should this say ..."vector<unsigned char> and GCSFilter::ElementSet set elements."?

ryanofsky commented at 3:30 PM on February 28, 2024:

re: #29409 (review)

Should this say ..."vector<unsigned char> and GCSFilter::ElementSet set elements."?

I think it's technically correct because the the vectors are the set elements but I changed the comment to make it clearer.

cbergqvist commented at 1:56 PM on February 28, 2024: contributor

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 force-pushed on Feb 28, 2024

ryanofsky commented at 4:14 PM on February 28, 2024: contributor

Thanks for the review!

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

cbergqvist approved

cbergqvist commented at 4:58 PM on February 28, 2024: contributor

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

DrahtBot added the label CI failed on Feb 28, 2024

in src/ipc/capnp/common-types.h:42 in eeb441ae7c outdated

  37 | +//! Construct a ParamStream wrapping data stream with serialization parameters
  38 | +//! needed to pass transaction and address objects between bitcoin processes.
  39 | +template<typename S>
  40 | +auto Wrap(S& s)
  41 | +{
  42 | +    return ParamsStream{s, TX_WITH_WITNESS, CAddress::V2_NETWORK};

ariard commented at 2:54 AM on March 7, 2024:

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.

ryanofsky commented at 4:29 PM on July 26, 2024:

re: #29409 (review)

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.

ariard commented at 2:57 AM on March 7, 2024: none

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.

DrahtBot marked this as a draft on Apr 17, 2024

DrahtBot added the label Needs rebase on May 15, 2024

ryanofsky force-pushed on Jun 11, 2024

ryanofsky marked this as ready for review on Jun 11, 2024

DrahtBot removed the label Needs rebase on Jun 11, 2024

ryanofsky force-pushed on Jun 11, 2024

DrahtBot removed the label CI failed on Jun 12, 2024

sedited commented at 10:14 AM on July 5, 2024: contributor

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 commented at 4:38 PM on July 8, 2024: contributor

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 force-pushed on Jul 26, 2024

in src/ipc/capnp/common-types.h:211 in 198fbd295b outdated

 206 | +{
 207 | +    BuildField(TypeList<std::string>(), invoke_context, output, std::string(value.what()));
 208 | +}
 209 | +
 210 | +template <typename Input, typename ReadDest>
 211 | +decltype(auto) CustomReadField(TypeList<UniValue::type_error>, Priority<1>, InvokeContext& invoke_context,

sedited commented at 4:19 PM on July 26, 2024:

Just a question: From what I gathered so far UniValue is required in these interfaces for error handling (like here) and to pass around the ban map. Is that correct? It seems unfortunate to me that UniValue types end up in the interfaces and need to be handled here. Is the intention to eventually refactor this?

ryanofsky commented at 5:11 PM on July 26, 2024:

re: #29409 (review)

Just a question: From what I gathered so far UniValue is required in these interfaces for error handling (like here) and to pass around the ban map. Is that correct? It seems unfortunate to me that UniValue types end up in the interfaces and need to be handled here. Is the intention to eventually refactor this?

In general there are lots of ways current interfaces are not ideal and can be improved over time, to improve performance (especially to reduce round trips), reduce dependencies, and provide more flexibility.

On UniValue specifically, handling UniValue exceptions could go away if wallet processes would start their own RPC servers on their own ports instead of getting RPC requests forwarded to them by the bitcoin-node process. That wouldn't be hard to do, but it would be a UI change, and there's a chicken and egg problem because it doesn't really make sense to make wallets listen on different ports before wallets are able to run in separate processes.

UniValue is also used to pass settings between processes. For example when GUI settings are changed, the GUI calls the updateRwSetting() function which takes a univalue parameter to save the setting. In this case I think it makes sense for the interface to use univalues, since the settings are saved as JSON, though of course alternatives are possible and might be more ideal.

For the ban map, maybe univalue is not ideal for some reason, but it also might be better than defining a new serialization format. I don't see a problem with the current approach in any of these cases, but making changes is always possible

ryanofsky commented at 4:34 PM on July 26, 2024: contributor

Updated ab6b795f218b2074a9c9c05fcc94ec37eccca5a5 -> eec0d31850aa1f7c520a46c417320f480f23fb1c (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#pullrequestreview-1921347149

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 https://github.com/Sjors/bitcoin/pull/48 and #30437 use it for the mining interface

ryanofsky marked this as a draft on Jul 26, 2024

ryanofsky force-pushed on Jul 26, 2024

DrahtBot added the label CI failed on Jul 26, 2024

DrahtBot commented at 5:17 PM on July 26, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27972388234

<details><summary>Hints</summary>

Make sure to run all tests locally, according to the documentation.

The failure may 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.

</details>

ryanofsky removed the label CI failed on Aug 6, 2024

josibake commented at 10:40 AM on August 7, 2024: member

Small nit: can you update the description with what you mention in #29409#pullrequestreview-2202254574 , regarding #30509 ? I was surprised to see this PR in draft and it took me a bit of digging in the comments to learn why. It's more clear when reading the "process separation" issue, but I think it would also be helpful to mention in this PR description.

ryanofsky commented at 2:43 PM on August 7, 2024: contributor

re: #29409 (comment)

Thanks! Added following to the description:

Note: The changes in this PR allow a new bitcoin-node process to be started with an -ipcfd=<n> argument specifying a file descriptor from a socketpair, allowing another process to control it over the socketpair. This PR by itself is fairly inflexible but combined with #30509 it allows existing bitcoin-node processes to be controlled over a unix socket. This PR is also a dependency of #10102 which allows the wallet code to run in a separate process and use the Chain interface.

hebasto added the label Needs CMake port on Aug 16, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

maflcko removed the label Needs CMake port on Sep 3, 2024

achow101 referenced this in commit df3f63ccfa on Sep 9, 2024

ryanofsky force-pushed on Sep 19, 2024

ryanofsky commented at 10:56 AM on September 19, 2024: contributor

Rebased 43dc39eed47d83b2b7e72c911198bbdd401c78d8 -> 65c4edda94ead5c20969681f8337fd6a735182cc (pr/ipc-chain.7 -> pr/ipc-chain.8, compare) due to conflicts with #30697 and #30454 Rebased 65c4edda94ead5c20969681f8337fd6a735182cc -> 968f14fc7a634f65f4399dc042a37d4a39f2c703 (pr/ipc-chain.8 -> pr/ipc-chain.9, compare) after base PR #30510 merged

DrahtBot removed the label Needs rebase on Sep 19, 2024

DrahtBot added the label Needs rebase on Sep 25, 2024

ryanofsky force-pushed on Sep 26, 2024

ryanofsky marked this as ready for review on Sep 26, 2024

DrahtBot removed the label Needs rebase on Sep 26, 2024

josibake commented at 5:16 PM on September 26, 2024: member

Concept ACK

Thanks for the rebase, I'll be digging into this next week!

in src/ipc/capnp/chain.capnp:23 in 968f14fc7a outdated

  18 | +interface Chain $Proxy.wrap("interfaces::Chain") {
  19 | +    destroy @0 (context :Proxy.Context) -> ();
  20 | +    getHeight @1 (context :Proxy.Context) -> (result :Int32, hasResult :Bool);
  21 | +    getBlockHash @2 (context :Proxy.Context, height :Int32) -> (result :Data);
  22 | +    haveBlockOnDisk @3 (context :Proxy.Context, height :Int32) -> (result :Bool);
  23 | +    getTipLocator @4 (context :Proxy.Context) -> (result :Data);

sedited commented at 12:17 PM on November 8, 2024:

Looking at the usage of this endpoint I am wondering if it is even necessary? I.e. instead of getting it first and using it for chainStateFlushed, the chainStateFlushed implementation could just do that internally.

More generally I have been wondering if it would be a good idea to eventually strongly type these Data blobs (and similarly the enums)? For example here, wrapping it in a BlockLocator type. I don't think this is relevant as long as libmultiprocess is used to create the c++ wrappers, which do have the full data types, but I am not sure how other languages would do this if they were used as clients to the interfaces eventually. Do you know how this could look like?

ryanofsky commented at 1:55 PM on November 12, 2024:

re: #29409 (review)

Looking at the usage of this endpoint I am wondering if it is even necessary? I.e. instead of getting it first and using it for chainStateFlushed, the chainStateFlushed implementation could just do that internally.

Current usages are a little unusual because chainStateFlushed() is used in the wallet both to receive notifications from the node about when to flush, but also internally in the wallet to flush its own data. I'm not sure if I understand the exact code change you are suggesting, but it could be reasonable and I also have a PR #29652 which removes getTipLocator and getActiveChainLocator.

More generally I have been wondering if it would be a good idea to eventually strongly type these Data blobs (and similarly the enums)? [...]

Yes all that makes sense. The tradeoff is just that defining capnproto structs for all the serialized classes adds more code that will need to be maintained, so I think it should probably be done selectively whenever use cases arise, probably by adding new versions of methods using new types. These changes can be implemented incrementally as needed, instead of all up front.

in src/ipc/capnp/chain.capnp:37 in 968f14fc7a outdated

  32 | +    findCommonAncestor @13 (context :Proxy.Context, blockHash1 :Data, blockHash2 :Data, ancestor :FoundBlockParam, block1 :FoundBlockParam, block2 :FoundBlockParam) -> (ancestor :FoundBlockResult, block1 :FoundBlockResult, block2 :FoundBlockResult, result :Bool);
  33 | +    findCoins @14 (context :Proxy.Context, coins :List(Common.Pair(Data, Data))) -> (coins :List(Common.Pair(Data, Data)));
  34 | +    guessVerificationProgress @15 (context :Proxy.Context, blockHash :Data) -> (result :Float64);
  35 | +    hasBlocks @16 (context :Proxy.Context, blockHash :Data, minHeight :Int32, maxHeight: Int32, hasMaxHeight :Bool) -> (result :Bool);
  36 | +    isRBFOptIn @17 (context :Proxy.Context, tx :Data) -> (result :Int32);
  37 | +    isInMempool @18 (context :Proxy.Context, tx :Data) -> (result :Bool);

sedited commented at 12:43 PM on November 8, 2024:

Nit: Shouldn't this be called txid?

ryanofsky commented at 1:59 PM on November 12, 2024:

re: #29409 (review)

Nit: Shouldn't this be called txid?

Nice catch, fixed! Might be a good idea to make sure these names are correct, even though they are just documentation and don't affect behavior.

in src/ipc/capnp/chain.capnp:68 in 968f14fc7a outdated

  63 | +    rpcRunLater @44 (context :Proxy.Context, name :Text, fn: RunLaterCallback, seconds: Int64) -> ();
  64 | +    getSetting @45 (context :Proxy.Context, name :Text) -> (result :Text);
  65 | +    getSettingsList @46 (context :Proxy.Context, name :Text) -> (result :List(Text));
  66 | +    getRwSetting @47 (context :Proxy.Context, name :Text) -> (result :Text);
  67 | +    updateRwSetting @48 (context :Proxy.Context, name :Text, update: SettingsUpdateCallback) -> (result :Bool);
  68 | +    overwriteRwSetting @49 (context :Proxy.Context, name :Text, value :Text, write :Bool) -> (result :Bool);

sedited commented at 12:54 PM on November 8, 2024:

How does this write argument here map to the action argument in the c++ interface? They seem to have different types.

ryanofsky commented at 3:58 PM on November 12, 2024:

re: #29409 (review)

How does this write argument here map to the action argument in the c++ interface? They seem to have different types.

This is a bug, and it should have been caught by the libmultiprocess library. https://github.com/chaincodelabs/libmultiprocess/pull/120 adds a static_assert to ensure that it would result in a compile error if a specified capnproto type is ever not compatible with a c++ enum type.

in src/ipc/capnp/chain.capnp:78 in 968f14fc7a outdated

  73 | +
  74 | +interface ChainNotifications $Proxy.wrap("interfaces::Chain::Notifications") {
  75 | +    destroy @0 (context :Proxy.Context) -> ();
  76 | +    transactionAddedToMempool @1 (context :Proxy.Context, tx :Data) -> ();
  77 | +    transactionRemovedFromMempool @2 (context :Proxy.Context, tx :Data, reason :Int32) -> ();
  78 | +    blockConnected @3 (context :Proxy.Context, role: UInt32, block :BlockInfo) -> ();

sedited commented at 12:57 PM on November 8, 2024:

Why is the role typed as UInt32 when the other enums are typed as Int32?

ryanofsky commented at 4:02 PM on November 12, 2024:

re: #29409 (review)

Why is the role typed as UInt32 when the other enums are typed as Int32?

Now changed this to Int32 to be consistent. Probably UInt32 was used because this parameter was added at a later time than all the other code was written. The exact integer type used to represent the enum doesn't actually matter as long as it is wide enough to hold all the enum values, and as long as round-trip static casts from the enum type to the int type and back return the same enum value. After https://github.com/chaincodelabs/libmultiprocess/pull/120 it should also be a compile error if incompatible types are used

in src/ipc/capnp/chain.capnp:144 in 968f14fc7a outdated

 139 | +    id @0 :Text;
 140 | +    method @1 :Text $Proxy.name("strMethod");
 141 | +    params @2 :Text;
 142 | +    mode @3 :Int32;
 143 | +    uri @4 :Text $Proxy.name("URI");
 144 | +    authUser @5 :Text;

sedited commented at 1:05 PM on November 8, 2024:

Should this include the version as well at this point?

ryanofsky commented at 4:05 PM on November 12, 2024:

re: #29409 (review)

Should this include the version as well at this point?

Yes, good catch. Added the version field and added a comment to JSONRPCRequest struct to prevent this type of bug in the future. I think this bug did not matter too much in practice because this capnp struct is only used to forward RPC requests from the node to the wallet and wallet shouldn't respond to requests differently based on JSONRPC version, but it is still a bug in the serialization that should be fixed.

ryanofsky force-pushed on Nov 12, 2024

ryanofsky commented at 4:27 PM on November 12, 2024: contributor

Updated 968f14fc7a634f65f4399dc042a37d4a39f2c703 -> 34d3e2a6eaaab9de5328c3e64739f1392696c7db (pr/ipc-chain.9 -> pr/ipc-chain.10, compare) with review suggestions and fixes.

Thanks for the review!

sedited approved

sedited commented at 11:47 AM on November 19, 2024: contributor

lgtm ACK 34d3e2a6eaaab9de5328c3e64739f1392696c7db

DrahtBot requested review from josibake on Nov 19, 2024

ryanofsky commented at 8:33 PM on December 4, 2024: contributor

Added 1 commit 34d3e2a6eaaab9de5328c3e64739f1392696c7db -> 395d5eed0475558de2c2a66914d5a5814ce38f3c (pr/ipc-chain.10 -> pr/ipc-chain.11, compare) exposing the Chain interface so it is easier to experiment with. Updated 395d5eed0475558de2c2a66914d5a5814ce38f3c -> 64b833854a34d87cde4e0ca4173d75012c401a7a (pr/ipc-chain.11 -> pr/ipc-chain.12, compare) fixing FoundBlock locator serialization to fix wallet_backup.py --descriptors test failure in followup #10102 https://cirrus-ci.com/task/6269118947524608?logs=ci#L2529 in new test added #30678

in src/ipc/capnp/chain.capnp:80 in 395d5eed04 outdated

  75 | +    destroy @0 (context :Proxy.Context) -> ();
  76 | +    transactionAddedToMempool @1 (context :Proxy.Context, tx :Data) -> ();
  77 | +    transactionRemovedFromMempool @2 (context :Proxy.Context, tx :Data, reason :Int32) -> ();
  78 | +    blockConnected @3 (context :Proxy.Context, role: UInt32, block :BlockInfo) -> ();
  79 | +    blockDisconnected @4 (context :Proxy.Context, block :BlockInfo) -> ();
  80 | +    updatedBlockTip @5 (context :Proxy.Context) -> ();

darosior commented at 2:50 PM on December 5, 2024:

What is the use for an updatedBlockTip notification with no information about the new tip?

sedited commented at 10:10 PM on December 9, 2024:

Adding this here for context: I guess one of the benefits of the Notifications and NotificationsProxy is avoiding raw pointer types in these interfaces. I went through the exercise of removing the pointer types from the validation interface before. Maybe this could eliminate the need for the logic in the NotificationsProxy in the first place. Not having to handle pointer types is also useful for the kernel API, where conveying the semantics of these pointers in the notifications to the using developer is tricky.

ryanofsky force-pushed on Dec 6, 2024

darosior commented at 7:58 PM on December 6, 2024: member

In order to test this PR i implemented a PoC of a Rust wallet (using BDK) consuming the Chain interface introduced here. I think it's also a good example of what multiprocess enables. See https://github.com/darosior/core_bdk_wallet.

in src/ipc/capnp/chain.capnp:42 in 64b833854a outdated

  37 | +    isInMempool @18 (context :Proxy.Context, txid :Data) -> (result :Bool);
  38 | +    hasDescendantsInMempool @19 (context :Proxy.Context, txid :Data) -> (result :Bool);
  39 | +    broadcastTransaction @20 (context :Proxy.Context, tx: Data, maxTxFee :Int64, relay :Bool) -> (error: Text, result :Bool);
  40 | +    getTransactionAncestry @21 (context :Proxy.Context, txid :Data) -> (ancestors :UInt64, descendants :UInt64, ancestorsize :UInt64, ancestorfees :Int64);
  41 | +    calculateIndividualBumpFees @22 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result: List(Common.PairInt64(Data)));
  42 | +    calculateCombinedBumpFee @23 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result :Int64, hasResult :Bool);

darosior commented at 9:18 PM on December 9, 2024:

Why are the CFeeRate serialized as Data whereas the other CAmount are Int64?

darosior commented at 10:12 PM on December 9, 2024: member

tested ACK 395d5eed0475558de2c2a66914d5a5814ce38f3c

I've tested this with the Rust wallet PoC i mentioned earlier: #29409 (comment). This tool exercises the following methods: getHeight, getBlockHash, hasBlocks, findAncestorByHeight, findAncestorByHash, findCommonAncestor, initMessage, showProgress and handleNotifications. It also exercises all the ChainNotifications events.

I tested numerous scenarii with this tool on regtest (some are documented here). I then proceeded to scan some existing Signet wallets while syncing a Signet node, as well as performing a rescan on a synced Signet node. I did not find any issue with this PR in all those tests (no crash, i get accurate wallet information and events under various conditions). I found a crash in #10102, but it's specific to the new logic introduced there and i could not reproduce it using this PR.

I cursory-reviewed the rest of the interface not exercised in my Rust wallet and did not find any issue.

sedited approved

sedited commented at 10:25 PM on December 9, 2024: contributor

Re-ACK 64b833854a34d87cde4e0ca4173d75012c401a7a

fixing FoundBlock locator serialization to fix wallet_backup.py --descriptors test failure in followup

I should have seen that one though :| - good that those tests exist!

DrahtBot requested review from darosior on Dec 9, 2024

darosior commented at 4:30 PM on December 16, 2024: member

I found another segmentation fault in updating my previous ACK to the latest tip of this PR. Here are minimal steps to reproduce:

Run a bitcoin-node from this PR. I tested on regtest with: ./multiprocbuild/src/bitcoin-node -regtest -datadir=$PWD/datadir_bdk_wallet -server=0 -ipcbind=unix -debug=ipc
Have a client connect. I used my tool for this. See usage here. The command i ran: cargo run -- ../bitcoin/datadir_bdk_wallet/regtest/node.sock.
Stop the bitcoin-node process. It will hang waiting for the client to exit.
Stop the client. The bitcoin-node process will segfault.

Here are the debug logs of bitcoin-node from when the client connected to the segfault.

<summary>Click to see logs</summary>

2024-12-16T16:19:22Z Loading 1 mempool transactions from file...
2024-12-16T16:19:22Z Loading addresses from DNS seed dummySeed.invalid.
2024-12-16T16:19:22Z Imported mempool transactions from file: 1 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
2024-12-16T16:19:22Z initload thread exit                                                                 
2024-12-16T16:19:22Z 0 addresses found from DNS seeds
2024-12-16T16:19:22Z dnsseed thread exit                                                                  
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#1](/bitcoin-bitcoin/1/) Init.construct$Params ()
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#1](/bitcoin-bitcoin/1/) Init.construct$Results (threadMap = <external capability>)
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#2](/bitcoin-bitcoin/2/) Init.makeChain$Params (context = (thread = <external capability>))
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#2](/bitcoin-bitcoin/2/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#2](/bitcoin-bitcoin/2/) Init.makeChain$Results (result = <external capability>)
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#3](/bitcoin-bitcoin/3/) Chain.initMessage$Params (context = (thread = <external capability>), message = "Oxydation of the Bitcoin Core wallet in progress..")
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#3](/bitcoin-bitcoin/3/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
2024-12-16T16:19:26Z init message: Oxydation of the Bitcoin Core wallet in progress..
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#3](/bitcoin-bitcoin/3/) Chain.initMessage$Results ()
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#4](/bitcoin-bitcoin/4/) Chain.showProgress$Params (context = (thread = <external capability>), title = "BDK Core startup", progress = 1, resumePossible = false)
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#4](/bitcoin-bitcoin/4/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#4](/bitcoin-bitcoin/4/) Chain.showProgress$Results ()
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#5](/bitcoin-bitcoin/5/) Chain.getHeight$Params (context = (thread = <external capability>))
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#5](/bitcoin-bitcoin/5/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#5](/bitcoin-bitcoin/5/) Chain.getHeight$Results (result = 113, hasResult = true)
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#6](/bitcoin-bitcoin/6/) Chain.getBlockHash$Params (context = (thread = <external capability>), height = 113)
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#6](/bitcoin-bitcoin/6/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#6](/bitcoin-bitcoin/6/) Chain.getBlockHash$Results (result = "H\\3678\\307#\\020|\\020\\017\\340\\2469\\330\\356\\335\\277\\341dOB{\\220\\327\\354[\\361\\036\\233\\301\\035DD")
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#7](/bitcoin-bitcoin/7/) Chain.showProgress$Params (context = (thread = <external capability>), title = "BDK Core startup", progress = 100, resumePossible = true)
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#7](/bitcoin-bitcoin/7/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#7](/bitcoin-bitcoin/7/) Chain.showProgress$Results ()
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server recv request  [#8](/bitcoin-bitcoin/8/) Chain.handleNotifications$Params (context = (thread = <external capability>), notifications = <external capability>)
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server post request  [#8](/bitcoin-bitcoin/8/) {bitcoin-node-8814/b-capnp-loop-8842 (from )}
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server send response [#8](/bitcoin-bitcoin/8/) Chain.handleNotifications$Results (result = <external capability>)
2024-12-16T16:19:26Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages7HandlerEEE
^C2024-12-16T16:19:27Z tor: Thread interrupt                                                              
2024-12-16T16:19:27Z Shutdown: In progress...                                                             
2024-12-16T16:19:27Z addcon thread exit                                                                   
2024-12-16T16:19:27Z opencon thread exit                                                                  
2024-12-16T16:19:27Z torcontrol thread exit                                                               
2024-12-16T16:19:27Z net thread exit                                                                      
2024-12-16T16:19:27Z msghand thread exit                                                                  
2024-12-16T16:19:27Z DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat started
2024-12-16T16:19:27Z DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)
2024-12-16T16:19:27Z scheduler thread exit                                                                
2024-12-16T16:19:27Z Writing 1 mempool transactions to file...
2024-12-16T16:19:27Z Writing 0 unbroadcast transactions to file.
2024-12-16T16:19:27Z Dumped mempool: 0.000s to copy, 0.002s to dump, 289 bytes dumped to file
2024-12-16T16:19:27Z Flushed fee estimates to fee_estimates.dat.
2024-12-16T16:19:27Z [ipc] {bitcoin-node-8814/b-shutoff-8814} IPC client first request from current thread, constructing waiter
2024-12-16T16:19:27Z [ipc] {bitcoin-node-8814/b-shutoff-8814} IPC client send ChainNotifications.chainStateFlushed$Params (context = (thread = <external capability>, callbackThread = <external capability>), role = 0, locator = "\\200\\021\\001\\000\\022H\\3678\\307#\\020|\\020\\017\\340\\2469\\330\\356\\335\\277\\341dOB{\\220\\327\\354[\\361\\036\\233\\301\\035DD\\tGm\\237]YbZDV9\\254\\252f\\205\\001\\323K\\375!\\340\\t\\367\\375\\346\\264W\\207\\000\\2101\\035:\\314\\233\\246\\275\\207\\v\\\\\\\\\\343\\202\\026\\274\\037\\213\\314\\303V\\2116<*+\\352\\255\\026\\204M\\332\\340\\nT\\306\\360Pa\\304\\002\\024\\356\\247\\220\\263\\257l\\230\\350\\362\\027Yl\\300\\316lk\\232\\f\\333\\235,\\234\\336\\204+JE\\263>\\237\\177\\305z\\017\\252\\311@\\341e\\026\\300dk\\305\\212V\\334\\377\\004cb\\217p\\n\\307w\\0046\\2261|\\024\\\\N\\336\\223G\\263~n\\271\\256\\264\\346\\227\\301\\032\\301\\030\\bw\\241Z.\\302\\261\\027 \\005\\374B\\357R\\231\\237Ct\\324\\232I\\322\\370\\303T\\253\\205\\354\\233\\337\\031C\\3215O\\3746\\016\\360b &\\300\\201\\244U\\221\\306 Pql+c\\212\\2524\\314a\\t\\216\\275=\\230b\\207ouv[\\365(\\3...
2024-12-16T16:19:27Z [ipc] {bitcoin-node-8814/b-shutoff-8814} IPC client recv ChainNotifications.chainStateFlushed$Results ()
2024-12-16T16:19:27Z Shutdown: done                                                                       
2024-12-16T16:19:32Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages5ChainEEE
2024-12-16T16:19:32Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server: socket disconnected.
2024-12-16T16:19:32Z [ipc] {bitcoin-node-8814/b-capnp-loop-8816} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages4InitEEE
Segmentation fault

</details>

ryanofsky referenced this in commit 621a04a70f on Jan 13, 2025

Big621 approved

DrahtBot added the label Needs rebase on Jan 29, 2025

pseudoramdom commented at 7:21 PM on February 10, 2025: none

Hi @ryanofsky! I'm new to this and I'm looking to test the feature on macOS. Couple of questions -

Can I use the branch as is or would I need to rebase on master since I see some related changes like #31740, #31552 landed on master.
Do I need to install libmultiprocess from source or is MULTIPROCESS=1 needed? I see some related work in #31741 . Would I benefit from that?

I tried building on this branch rebased on master and I got the following errors while building.

make -C depends HOST=aarch64-apple-darwin MULTIPROCESS=1 NO_QT=1
cmake -B multiprocbuild/ --toolchain=depends/$HOST_PLATFORM/toolchain.cmake -DWITH_MULTIPROCESS=ON

<details> <summary>Build config</summary>

Configure summary
=================
Executables:
  bitcoind ............................ ON
  bitcoin-node (multiprocess) ......... ON
  bitcoin-qt (GUI) .................... OFF
  bitcoin-gui (GUI, multiprocess) ..... OFF
  bitcoin-cli ......................... ON
  bitcoin-tx .......................... ON
  bitcoin-util ........................ ON
  bitcoin-wallet ...................... ON
  bitcoin-chainstate (experimental) ... OFF
  libbitcoinkernel (experimental) ..... OFF
Optional features:
  wallet support ...................... ON
   - descriptor wallets (SQLite) ...... ON
   - legacy wallets (Berkeley DB) ..... ON
  external signer ..................... ON
  ZeroMQ .............................. ON
  USDT tracing ........................ OFF
  QR code (GUI) ....................... OFF
  DBus (GUI, Linux only) .............. OFF
Tests:
  test_bitcoin ........................ ON
  test_bitcoin-qt ..................... OFF
  bench_bitcoin ....................... OFF
  fuzz binary ......................... OFF

Cross compiling ....................... TRUE, for Darwin, aarch64
C++ compiler .......................... Clang 19.1.7, /opt/homebrew/opt/llvm/bin/clang++
CMAKE_BUILD_TYPE ...................... RelWithDebInfo
Preprocessor defined macros ........... OBJC_OLD_DISPATCH_PROTOTYPES=0
C++ compiler flags .................... -pipe -std=c++20 -mmacos-version-min=13.0 -arch arm64 -O2 -O2 -g -std=c++20 -fPIC -fdebug-prefix-map=/Users/pseudoramdom/Developer/Projects/bitcoin/src=. -fmacro-prefix-map=/Users/pseudoramdom/Developer/Projects/bitcoin/src=. -Wall -Wextra -Wgnu -Wformat -Wformat-security -Wvla -Wshadow-field -Wthread-safety -Wloop-analysis -Wredundant-decls -Wunused-member-function -Wdate-time -Wconditional-uninitialized -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wdocumentation -Wself-assign -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -mbranch-protection=bti
Linker flags .......................... -pipe -std=c++20 -mmacos-version-min=13.0 -arch arm64 -O2 -O2 -g -L/opt/homebrew/opt/llvm/lib -Wl,-dead_strip -Wl,-dead_strip_dylibs -Wl,-headerpad_max_install_names -fstack-protector-all -Wl,-fixup_chains -fPIE -Xlinker -pie

NOTE: The summary above may not exactly match the final applied build flags
      if any additional CMAKE_* or environment variables have been modified.
      To see the exact flags applied, build with the --verbose option.

Attempt to harden executables ......... ON
Treat compiler warnings as errors ..... OFF
Use ccache for compiling .............. OFF


-- Configuring done (9.5s)
-- Generating done (0.2s)
-- Build files have been written to: /Users/pseudoramdom/Developer/Projects/bitcoin/multiprocbuild

</details>

<details> <summary>Sample build errors</summary>

In file included from /Users/pseudoramdom/Developer/Projects/bitcoin/src/test/ipc_test_types.h:8:
/Users/pseudoramdom/Developer/Projects/bitcoin/src/ipc/capnp/common-types.h:218:25: error: no member named 'ReadDestValue' in namespace 'mp'
  218 |                     mp::ReadDestValue(error));
      |                     ~~~~^

In file included from /Users/pseudoramdom/Developer/Projects/bitcoin/multiprocbuild/src/ipc/capnp/common.capnp.proxy-types.h:7:
/Users/pseudoramdom/Developer/Projects/bitcoin/src/ipc/capnp/common-types.h:218:25: error: no member named 'ReadDestValue' in namespace 'mp'
  218 |                     mp::ReadDestValue(error));
      |                     ~~~~^

/Users/pseudoramdom/Developer/Projects/bitcoin/src/ipc/capnp/chain.cpp:131:9: error: no member named 'BuildOne' in namespace 'mp'
  131 |     mp::BuildOne<0>(mp::TypeList<interfaces::BlockInfo>(), invoke_context, builder, block);
      |     ~~~~^

</details>

P.S. let me know if this isn't the right place to discuss this.

ryanofsky commented at 7:53 PM on February 10, 2025: contributor

re: #29409 (comment)

Can I use the branch as is or would I need to rebase on master

Hi @pseudoramdom, definitely it should be easiest to just check out this branch and use it as is. The errors that you are seeing with the branch rebased happen due to https://github.com/chaincodelabs/libmultiprocess/pull/135, and require some code changes here to be compatible. I should be able to update this branch in the next day or two in case you do need other changes from master.

Do I need to install libmultiprocess from source or is MULTIPROCESS=1 needed?

These are two different alternatives described in https://github.com/bitcoin/bitcoin/blob/master/doc/multiprocess.md#installation.

The first one described there is to use just the use depends system (the depends system is a mini-package manager used to download dependencies and build releases) with option MULTIPROCESS=1

The second one described there is to install capnproto and libmultiprocess manually.

A third approach implemented in #31741 should make all of this a lot easier because libmultiprocess sources will be included as a subtree and won't have to be downloaded separately. But #31741 won't be compatible with this PR until this PR is rebased.

P.S. let me know if this isn't the right place to discuss this.

Anywhere is fine with me, and feel free to open new issues in https://github.com/chaincodelabs/libmultiprocess/issues/new if you have any questions or feedback. Thanks for trying this.

ryanofsky force-pushed on Feb 11, 2025

ryanofsky commented at 3:44 AM on February 11, 2025: contributor

Rebased 64b833854a34d87cde4e0ca4173d75012c401a7a -> 4fa7f72cb9d8f0290f56293989ec6ea950162c5b (pr/ipc-chain.12 -> pr/ipc-chain.13, compare) due to various conflicts. @pseudoramdom, with this update, errors you posted should be resolved

ryanofsky commented at 7:43 PM on February 11, 2025: contributor

re: #29409 (comment)

I found another segmentation fault in updating my previous ACK to the latest tip of this PR. Here are minimal steps to reproduce

This seems closely related to the broken connection during node shutdown causing unclean shutdown issue reported earlier so added a note about this there: https://github.com/chaincodelabs/libmultiprocess/issues/123#issuecomment-2651851492

DrahtBot removed the label Needs rebase on Feb 12, 2025

DrahtBot added the label interfaces on Feb 12, 2025

ryanofsky referenced this in commit 1543f2565d on Apr 24, 2025

ryanofsky referenced this in commit 813dd4a534 on Apr 25, 2025

ryanofsky referenced this in commit a324fc01f2 on Apr 28, 2025

ryanofsky referenced this in commit 8ca7049cea on Apr 28, 2025

in src/ipc/capnp/chain.capnp:91 in 4fa7f72cb9 outdated

  86 | +    destroy @0 (context :Proxy.Context) -> ();
  87 | +    registerRpcs @1 (context :Proxy.Context) -> ();
  88 | +    verify @2 (context :Proxy.Context) -> (result :Bool);
  89 | +    load @3 (context :Proxy.Context) -> (result :Bool);
  90 | +    start @4 (context :Proxy.Context, scheduler :Void) -> ();
  91 | +    flush @5 (context :Proxy.Context) -> ();

zaidmstrr commented at 10:24 AM on May 24, 2025:

This new PR https://github.com/bitcoin/bitcoin/pull/32438/commits/fa62a013a558338dc6ee5fb4cfd6fc7c782c301b removes this flush() from the codebase. I think it should also be removed from chain.capnp otherwise it will give build errors.

ryanofsky referenced this in commit 8b96229da5 on Jun 3, 2025

ryanofsky force-pushed on Jun 3, 2025

ryanofsky commented at 8:09 PM on June 3, 2025: contributor

Rebased 4fa7f72cb9d8f0290f56293989ec6ea950162c5b -> f92422e308ec33e2b211b866e218efacc77a4f7f (pr/ipc-chain.13 -> pr/ipc-chain.14, compare) due to silent conflicts with #32438 and #32641

DrahtBot added the label CI failed on Jun 3, 2025

DrahtBot commented at 10:19 PM on June 3, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/43415683148 LLM reason (✨ experimental): The CI failure is caused by a failed lint check due to detection of a touched subtree directory without a subtree merge.

<details><summary>Hints</summary>

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.

</details>

ryanofsky referenced this in commit 283c572ab1 on Jun 5, 2025

ryanofsky referenced this in commit 6f7c60f636 on Jun 5, 2025

ryanofsky referenced this in commit a1ca07692b on Jun 5, 2025

ryanofsky referenced this in commit 49cf256dd7 on Jun 5, 2025

ryanofsky referenced this in commit 6f00a28c1e on Jun 6, 2025

ryanofsky referenced this in commit 59030c68cb on Jun 11, 2025

ryanofsky referenced this in commit 615eede7c1 on Jun 13, 2025

ryanofsky referenced this in commit 81a812c496 on Jun 13, 2025

ryanofsky referenced this in commit 0007ee7dad on Jun 16, 2025

ryanofsky referenced this in commit 05a0a9eeaf on Jun 16, 2025

ryanofsky referenced this in commit 4fc3bfbb19 on Jun 16, 2025

ryanofsky referenced this in commit d7013b3055 on Jun 16, 2025

ryanofsky referenced this in commit 19585fb560 on Jun 24, 2025

ryanofsky referenced this in commit cca41f9b50 on Jun 24, 2025

Sjors referenced this in commit 8285c6a920 on Jun 27, 2025

ryanofsky referenced this in commit 18eabb3306 on Jul 1, 2025

ryanofsky referenced this in commit 71a6a25dc7 on Jul 1, 2025

ryanofsky referenced this in commit 25285bcada on Jul 1, 2025

ryanofsky referenced this in commit d9e9e1eb73 on Jul 1, 2025

ryanofsky referenced this in commit 2f5b860af4 on Jul 1, 2025

ryanofsky referenced this in commit accdd52e7c on Jul 1, 2025

ryanofsky referenced this in commit ff1c9278e7 on Jul 1, 2025

zaidmstrr commented at 12:03 PM on July 21, 2025: contributor

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

in src/ipc/capnp/chain.cpp:140 in f92422e308 outdated

 135 | +                        const ipc::capnp::messages::BlockInfo::Reader& reader,
 136 | +                        ::capnp::Void builder,
 137 | +                        std::function<void(const interfaces::BlockInfo&)>&& fn)
 138 | +{
 139 | +    // Pointer fields are annotated with Proxy.skip because code generator can't
 140 | +    // figure out pointer lifetimes to be able to implementat a ReadField

maflcko commented at 1:35 PM on July 21, 2025:

implementat -> implement [spelling error in comment “…able to implementat a ReadField…”]

ryanofsky force-pushed on Jul 24, 2025

ryanofsky commented at 4:19 PM on July 24, 2025: contributor

Rebased f92422e308ec33e2b211b866e218efacc77a4f7f -> c0d9515a3aef468bf4c5949c419ab1c9bab0dfa3 (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 commented at 8:18 PM on July 24, 2025: member

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

ryanofsky force-pushed on Jul 28, 2025

ryanofsky commented at 2:29 PM on July 28, 2025: contributor

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 https://github.com/bitcoin-core/libmultiprocess/pull/181 which is part of #32345 but not currently part of master. Implemented a different workaround for now.

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

DrahtBot removed the label CI failed on Jul 28, 2025

ryanofsky referenced this in commit 1045600c38 on Aug 8, 2025

ryanofsky referenced this in commit 7f65aac78b on Aug 8, 2025

Sjors referenced this in commit 7ae4c2d151 on Aug 12, 2025

janus referenced this in commit 1b74ca2e8a on Sep 15, 2025

sedited commented at 12:43 PM on September 24, 2025: contributor

I tried rebasing this PR, but I'm getting a bunch of compilation errors. Can you rebase it @ryanofsky?

ryanofsky force-pushed on Sep 24, 2025

ryanofsky commented at 2:01 PM on September 24, 2025: contributor

Rebased b372063241fbd34d27d062f3e438a3e425a9dfc5 -> 7ba7ec9d08927f244da8d5c1a7a1d7ced3239465 (pr/ipc-chain.16 -> pr/ipc-chain.17, compare) due to conflicts with #33169 and #32345

sedited commented at 9:20 AM on September 29, 2025: contributor

Thanks for the rebase :)

I updated darosior's core_bdk_wallet to use this newest version and I ran into a few crashes again. This is the one I managed to get a reproducible backtrace for:

<details> <summary>Crash backtrace + ipc debug logging</summary>

2025-09-29T11:18:20Z [ipc] {bitcoin-node-165737/b-capnp-loop-165742} IPC server post request  [#32](/bitcoin-bitcoin/32/) {bitcoin-node-165737/b-capnp-loop-166141 (from )}
2025-09-29T11:18:20Z [ipc] {bitcoin-node-165737/b-capnp-loop-165742} IPC server send response [#32](/bitcoin-bitcoin/32/) Chain.handleNotifications$Results (result = <external capability>)
2025-09-29T11:18:20Z [ipc] {bitcoin-node-165737/b-capnp-loop-165742} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages7HandlerEEE
2025-09-29T11:18:20Z [ipc] {bitcoin-node-165737/b-capnp-loop-165873} IPC client destroy N2mp11ProxyClientIN3ipc5capnp8messages18ChainNotificationsEEE
2025-09-29T11:18:20Z [ipc] {bitcoin-node-165737/b-capnp-loop-165873} IPC client first request from current thread, constructing waiter
2025-09-29T11:18:20Z [ipc] {bitcoin-node-165737/b-capnp-loop-165873} IPC client send ChainNotifications.destroy$Params (context = (thread = <external capability>, callbackThread = <external capability>))
2025-09-29T11:18:20Z [ipc] {bitcoin-node-165737/b-capnp-loop-165742} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages5ChainEEE
2025-09-29T11:18:20Z [ipc] IPC client method call interrupted by disconnect.
terminate called after throwing an instance of 'ipc::Exception'
  what():  IPC client method call interrupted by disconnect.
[Thread 0x7ffed37fe6c0 (LWP 166141) exited]
2025-09-29T11:18:20Z [ipc] {bitcoin-node-165737/b-capnp-loop-165742} IPC server: socket disconnected.
2025-09-29T11:18:20Z [ipc] {bitcoin-node-165737/b-capnp-loop-165742} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages4InitEEE

Thread 45 "b-capnp-loop" received signal SIGABRT, Aborted.
[Switching to Thread 0x7ffed2ffd6c0 (LWP 165873)]
__pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
warning: 44	./nptl/pthread_kill.c: No such file or directory
(gdb) bt
[#0](/bitcoin-bitcoin/0/)  __pthread_kill_implementation (no_tid=0, signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:44
[#1](/bitcoin-bitcoin/1/)  __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
[#2](/bitcoin-bitcoin/2/)  __GI___pthread_kill (threadid=<optimized out>, signo=signo@entry=6) at ./nptl/pthread_kill.c:89
[#3](/bitcoin-bitcoin/3/)  0x00007ffff724527e in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
[#4](/bitcoin-bitcoin/4/)  0x00007ffff72288ff in __GI_abort () at ./stdlib/abort.c:79
[#5](/bitcoin-bitcoin/5/)  0x00007ffff76a5ff5 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
[#6](/bitcoin-bitcoin/6/)  0x00007ffff76bb0da in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
[#7](/bitcoin-bitcoin/7/)  0x00007ffff76a5a55 in std::terminate() () from /lib/x86_64-linux-gnu/libstdc++.so.6
[#8](/bitcoin-bitcoin/8/)  0x000055555563c47f in __clang_call_terminate ()
[#9](/bitcoin-bitcoin/9/)  0x0000555555b39999 in mp::ProxyClientBase<ipc::capnp::messages::ChainNotifications, interfaces::Chain::Notifications>::~ProxyClientBase (this=0x7ffed40cfe60)
    at ./ipc/libmultiprocess/include/mp/proxy-io.h:468
[#10](/bitcoin-bitcoin/10/) 0x0000555555b36e9f in mp::ProxyClient<ipc::capnp::messages::ChainNotifications>::~ProxyClient (this=0x28769)
    at /home/drgrid/bitcoin/build_dev_mode_clang/src/ipc/capnp/chain.capnp.proxy-types.c++:12
[#11](/bitcoin-bitcoin/11/) 0x000055555575637e in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x7ffed40049a0)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:346
[#12](/bitcoin-bitcoin/12/) std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:1071
[#13](/bitcoin-bitcoin/13/) std::__shared_ptr<interfaces::Chain::Notifications, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:1524
[#14](/bitcoin-bitcoin/14/) node::(anonymous namespace)::NotificationsProxy::~NotificationsProxy (this=<optimized out>) at ./node/interfaces.cpp:456
[#15](/bitcoin-bitcoin/15/) 0x0000555555755fbc in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x7ffed4000d80)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:346
[#16](/bitcoin-bitcoin/16/) std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:1071
[#17](/bitcoin-bitcoin/17/) std::__shared_ptr<node::(anonymous namespace)::NotificationsProxy, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:1524
[#18](/bitcoin-bitcoin/18/) std::__shared_ptr<node::(anonymous namespace)::NotificationsProxy, (__gnu_cxx::_Lock_policy)2>::reset (this=0x7ffed4005a50)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:1642
[#19](/bitcoin-bitcoin/19/) node::(anonymous namespace)::NotificationsHandlerImpl::disconnect (this=0x7ffed4005a40) at ./node/interfaces.cpp:496
[#20](/bitcoin-bitcoin/20/) 0x0000555555755e47 in node::(anonymous namespace)::NotificationsHandlerImpl::~NotificationsHandlerImpl (this=0x7ffed4005a40) at ./node/interfaces.cpp:491
[#21](/bitcoin-bitcoin/21/) node::(anonymous namespace)::NotificationsHandlerImpl::~NotificationsHandlerImpl (this=0x28769) at ./node/interfaces.cpp:491
[#22](/bitcoin-bitcoin/22/) 0x0000555555b49cb7 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x7ffed4004d00)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:346
[#23](/bitcoin-bitcoin/23/) std::__shared_count<(__gnu_cxx::_Lock_policy)2>::~__shared_count (this=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:1071
[#24](/bitcoin-bitcoin/24/) std::__shared_ptr<interfaces::Handler, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:1524
[#25](/bitcoin-bitcoin/25/) std::__shared_ptr<interfaces::Handler, (__gnu_cxx::_Lock_policy)2>::reset (this=0x7fffe801c3e0)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/shared_ptr_base.h:1642
[#26](/bitcoin-bitcoin/26/) mp::ProxyServerBase<ipc::capnp::messages::Handler, interfaces::Handler>::~ProxyServerBase()::{lambda()#1}::operator()() (this=0x7fffe801c3e0)
    at ./ipc/libmultiprocess/include/mp/proxy-io.h:514
[#27](/bitcoin-bitcoin/27/) std::__invoke_impl<void, mp::ProxyServerBase<ipc::capnp::messages::Handler, interfaces::Handler>::~ProxyServerBase()::{lambda()#1}&>(std::__invoke_other, mp::ProxyServerBase<ipc::capnp::messages::Handler, interfaces::Handler>::~ProxyServerBase()::{lambda()#1}&) (__f=...) at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:61
[#28](/bitcoin-bitcoin/28/) std::__invoke_r<void, mp::ProxyServerBase<ipc::capnp::messages::Handler, interfaces::Handler>::~ProxyServerBase()::{lambda()#1}&>(mp::ProxyServerBase<ipc::capnp::messages::Handler, interfaces::Handler>::~ProxyServerBase()::{lambda()#1}&) (__fn=...) at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:111
[#29](/bitcoin-bitcoin/29/) std::_Function_handler<void (), mp::ProxyServerBase<ipc::capnp::messages::Handler, interfaces::Handler>::~ProxyServerBase()::{lambda()#1}>::_M_invoke(std::_Any_data const&) (
    __functor=...) at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:290
[#30](/bitcoin-bitcoin/30/) 0x0000555555f41979 in std::function<void ()>::operator()() const (this=0x7ffed2ffca70) at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591
[#31](/bitcoin-bitcoin/31/) mp::Unlock<mp::Lock, std::function<void ()> const&>(mp::Lock&, std::function<void ()> const&) (lock=..., callback=...) at ./ipc/libmultiprocess/include/mp/util.h:209
[#32](/bitcoin-bitcoin/32/) 0x0000555555f3f56f in mp::EventLoop::startAsyncThread()::$_0::operator()() const (this=<optimized out>) at ./ipc/libmultiprocess/src/mp/proxy.cpp:298
[#33](/bitcoin-bitcoin/33/) std::__invoke_impl<void, mp::EventLoop::startAsyncThread()::$_0>(std::__invoke_other, mp::EventLoop::startAsyncThread()::$_0&&) (__f=...)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:61
[#34](/bitcoin-bitcoin/34/) std::__invoke<mp::EventLoop::startAsyncThread()::$_0>(mp::EventLoop::startAsyncThread()::$_0&&) (__fn=...)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:96
[#35](/bitcoin-bitcoin/35/) std::thread::_Invoker<std::tuple<mp::EventLoop::startAsyncThread()::$_0> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) (this=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_thread.h:292
[#36](/bitcoin-bitcoin/36/) std::thread::_Invoker<std::tuple<mp::EventLoop::startAsyncThread()::$_0> >::operator()() (this=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_thread.h:299
[#37](/bitcoin-bitcoin/37/) std::thread::_State_impl<std::thread::_Invoker<std::tuple<mp::EventLoop::startAsyncThread()::$_0> > >::_M_run() (this=<optimized out>)
    at /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_thread.h:244
[#38](/bitcoin-bitcoin/38/) 0x00007ffff76ecdb4 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
[#39](/bitcoin-bitcoin/39/) 0x00007ffff729caa4 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:447
[#40](/bitcoin-bitcoin/40/) 0x00007ffff7329c6c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78

</details>

ryanofsky commented at 11:09 AM on September 29, 2025: contributor

Thanks for the rebase :)

I updated darosior's core_bdk_wallet to use this newest version and I ran into a few crashes again. This is the one I managed to get a reproducible backtrace for:

Unfortunately, I don't think this is a meaningful backtrace, since it is just showing the trace of a thread at the time it receives a SIGPIPE signal (Thread 3 "b-capnp-loop" received signal SIGPIPE, Broken pipe.), which happens fairly normally during disconnections. It's usually best to suppress these signals in GDB with gdb -ex "handle SIGPIPE nostop noprint pass" ...

sedited commented at 11:20 AM on September 29, 2025: contributor

Err, right. Updated the description with your suggested trace filtering.

ryanofsky commented at 2:17 PM on September 29, 2025: contributor

Err, right. Updated the description with your suggested trace filtering.

Thanks created https://github.com/bitcoin-core/libmultiprocess/issues/219 to track this (feel free to create new issues directly in the future). Thanks for updating the rust client, too!

ryanofsky force-pushed on Sep 30, 2025

ryanofsky commented at 1:58 PM on September 30, 2025: contributor

Updated 7ba7ec9d08927f244da8d5c1a7a1d7ced3239465 -> 7a526a161fdb31a04b0afaaa112a137b9a595977 (pr/ipc-chain.17 -> pr/ipc-chain.18, compare) to fix clang-tidy errors https://github.com/bitcoin/bitcoin/actions/runs/17978986033/job/51139663003?pr=29409

Updated 7a526a161fdb31a04b0afaaa112a137b9a595977 -> 2ae18d058562dd494f88328c6907c31c5d68b04e (pr/ipc-chain.18 -> pr/ipc-chain.19, compare) to fix silent conflict with #32660

Rebased 2ae18d058562dd494f88328c6907c31c5d68b04e -> 2763976d96663fcf013832cfbf771fd4050924ee (pr/ipc-chain.19 -> pr/ipc-chain.20, compare)

Rebased 2763976d96663fcf013832cfbf771fd4050924ee -> d9efd1e49d1df154970b6a60229eedde3ba7cffe (pr/ipc-chain.20 -> pr/ipc-chain.21, compare) due to silent conflicts with #33567 and #33218

in src/ipc/capnp/chain.capnp:5 in 7a526a161f outdated

   0 | @@ -0,0 +1,192 @@
   1 | +# Copyright (c) 2024 The Bitcoin Core developers
   2 | +# Distributed under the MIT software license, see the accompanying
   3 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 | +
   5 | +@0x94f21a4864bd2c65;

l0rinc commented at 5:09 PM on September 30, 2025:

Could we add a simple explanation comment for these salts?

ryanofsky commented at 1:09 PM on December 12, 2025:

re: #29409 (review)

Could we add a simple explanation comment for these salts?

Not sure about this. Is there a comment you would suggest? I feel like ids are built into the syntax of Cap'n Proto and ubiquitous. If you aren't familiar with Cap'n Proto, I think a comment here would probably just distract you from more semantically relevant parts of the file and leave you confused. Commenting on IDs in capnproto seems like commenting on #ifndef include guards at the top of c++ header files. It would be hard to know where to begin or what information would be helpful.

in src/ipc/capnp/chain.capnp:20 in 7a526a161f outdated

  15 | +using Common = import "common.capnp";
  16 | +using Handler = import "handler.capnp";
  17 | +
  18 | +interface Chain $Proxy.wrap("interfaces::Chain") {
  19 | +    destroy @0 (context :Proxy.Context) -> ();
  20 | +    getHeight @1 (context :Proxy.Context) -> (result :Int32, hasResult :Bool);

l0rinc commented at 5:26 PM on September 30, 2025:

I wonder how schema evolution is handled by Cap'n Proto - what happens if a field is deprecated and removed or new values are inserted, do I have to check all existing values to make sure I can find a new valid id?

https://capnproto.org/language.html#evolving-your-protocol claims:

New fields, enumerants, and methods may be added to structs, enums, and interfaces, respectively, as long as each new member’s number is larger than all previous members. Similarly, new fields may be added to existing groups and unions.
New parameters may be added to a method. The new parameters must be added to the end of the parameter list and must have default values.
Members can be re-arranged in the source code, so long as their numbers stay the same.

So I guess if you change any value and deprecate the old one you will have to add the new version with max + 1 - wanted to suggest that we could add 10 opportunities between values like:

    destroy [@10](/bitcoin-bitcoin/contributor/10/) ...
    getHeight [@20](/bitcoin-bitcoin/contributor/20/)...

so that when we change destroy to return a bool or add a new method, we can do it as:

    construct [@0](/bitcoin-bitcoin/contributor/0/) ...
    destroy [@10](/bitcoin-bitcoin/contributor/10/) ...  # deprecated
    destruct [@11](/bitcoin-bitcoin/contributor/11/) ...
    getHeight [@20](/bitcoin-bitcoin/contributor/20/)...

but it appears that's not possible.

ryanofsky commented at 1:26 PM on December 12, 2025:

re: #29409 (review)

I wonder how schema evolution is handled by Cap'n Proto - what happens if a field is deprecated and removed or new values are inserted, do I have to check all existing values to make sure I can find a new valid id?

Yes you do. You need to pick the next unused ID. I think it actually works pretty well in practice, especially if the id's are mostly in order, but it's also possible to include # Next unused id: 42 comments if they are really out of order and it's hard to find the next one.

we could add 10 opportunities between values [...] but it appears that's not possible

Yes like you point out, gaps are not allowed. Technically I think the compiler could allow gaps for interfaces, but for structs it couldn't work because the order fields are added to the struct defines its binary layout. For interfaces the restriction also seems reasonable to make interfaces consistent with structs, and make it possible to see what order methods were added over time.

Since it's also possible to move and rename deprecated fields, this restriction doesn't get in the way of making capnp files readble. Renaming also lets you break source compatibility and require source code to be updated, while maintaining binary compatibility so old binaries and code using previous definitions continue to work. In general, I think the system is very flexible and functions pretty nicely.

ryanofsky force-pushed on Oct 21, 2025

ryanofsky force-pushed on Oct 22, 2025

ryanofsky force-pushed on Nov 20, 2025

DrahtBot added the label CI failed on Nov 20, 2025

in src/ipc/capnp/chain.capnp:3 in d9efd1e49d

   0 | @@ -0,0 +1,196 @@
   1 | +# Copyright (c) 2024 The Bitcoin Core developers
   2 | +# Distributed under the MIT software license, see the accompanying
   3 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.

l0rinc commented at 4:52 PM on November 22, 2025:

super-nit: I assume the copyright headers are similar to other code in the repo:

# Copyright (c) 2024-present The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or https://opensource.org/license/mit.

ryanofsky commented at 1:31 PM on December 12, 2025:

re: #29409 (review)

super-nit: I assume the copyright headers are similar to other code in the repo:

Nice catch, fixed this up

DrahtBot removed the label CI failed on Nov 24, 2025

in src/ipc/capnp/chain.capnp:41 in d9efd1e49d

  36 | +    hasDescendantsInMempool @17 (context :Proxy.Context, txid :Data) -> (result :Bool);
  37 | +    broadcastTransaction @18 (context :Proxy.Context, tx: Data, maxTxFee :Int64, broadcastMethod :Int32) -> (error: Text, result :Bool);
  38 | +    getTransactionAncestry @19 (context :Proxy.Context, txid :Data) -> (ancestors :UInt64, descendants :UInt64, ancestorsize :UInt64, ancestorfees :Int64);
  39 | +    calculateIndividualBumpFees @20 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result: List(Common.PairInt64(Data)));
  40 | +    calculateCombinedBumpFee @21 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result :Int64, hasResult :Bool);
  41 | +    getPackageLimits @22 (context :Proxy.Context) -> (ancestors :UInt64, descendants :UInt64);

l0rinc commented at 12:01 PM on November 25, 2025:

May be deliberate, but the parameters in https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/interfaces/chain.h#L267 are 32 bit

ryanofsky commented at 1:32 PM on December 12, 2025:

re: #29409 (review)

May be deliberate, but the parameters in [...] are 32 bit

Good catch. Updated these. Using a bigger type was harmless but not intentional. (It is a compiler error if you use a smaller type.)

in src/ipc/capnp/chain.capnp:75 in d9efd1e49d outdated

  70 | +}
  71 | +
  72 | +interface ChainNotifications $Proxy.wrap("interfaces::Chain::Notifications") {
  73 | +    destroy @0 (context :Proxy.Context) -> ();
  74 | +    transactionAddedToMempool @1 (context :Proxy.Context, tx :Data) -> ();
  75 | +    transactionRemovedFromMempool @2 (context :Proxy.Context, tx :Data, reason :Int32) -> ();

l0rinc commented at 12:02 PM on November 25, 2025:

Original has enums for reason https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/interfaces/chain.h#L323

should we make the param UInt32 like we did one line below for ChainstateRole

    transactionRemovedFromMempool [@2](/bitcoin-bitcoin/contributor/2/) (context :Proxy.Context, tx :Data, reason :UInt32) -> ();

Or vice versa, since Int32 seems more common (though UInt32 may make more sense).

But looking at #29409 (review), you mentioned:

After bitcoin-core/libmultiprocess#120 it should also be a compile error if incompatible types are used

which was merged since - shouldn't it be a compile error now?

ryanofsky commented at 1:35 PM on December 12, 2025:

re: #29409 (review)

should we make the param UInt32 like we did one line below for ChainstateRole

Nice find noticing the inconsistency. Both types should be safe because casts between signed and unsigned types are well-defined. I tend to prefer signed types for enums so if the enums use -1, -2, -3 values they will be displayed nicely. But unsigned types could be better for some enums, particularly bitmasks. I updated chainstate role enums variables to be signed for consistency.

in src/ipc/capnp/chain.capnp:90 in d9efd1e49d

  85 | +    verify @2 (context :Proxy.Context) -> (result :Bool);
  86 | +    load @3 (context :Proxy.Context) -> (result :Bool);
  87 | +    start @4 (context :Proxy.Context, scheduler :Void) -> ();
  88 | +    stop @5 (context :Proxy.Context) -> ();
  89 | +    setMockTime @6 (context :Proxy.Context, time :Int64) -> ();
  90 | +    schedulerMockForward @7 (context :Proxy.Context, time :Int64) -> ();

l0rinc commented at 12:10 PM on November 25, 2025:

The original param name is delta_seconds (where it doesn't make sense, since it's typed), but here it would, since it's not obvious what Int64 should contain otherwise (could be epoch millis as well as far as I can tell).

https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/interfaces/chain.h#L426

ryanofsky commented at 1:38 PM on December 12, 2025:

re: #29409 (review)

The original param name is delta_seconds

Thanks renamed to deltaSeconds for consistency. In general parameter names don't matter very much but it is good to be consistent.

in src/ipc/capnp/chain.capnp:126 in d9efd1e49d outdated

 121 | +   actor @2 :ActorCallback;
 122 | +   argNames @3 :List(RPCArg);
 123 | +   uniqueId @4 :Int64 $Proxy.name("unique_id");
 124 | +}
 125 | +
 126 | +struct RPCArg {

l0rinc commented at 12:18 PM on November 25, 2025:

If this corresponds to https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/rpc/server.h#L50 we might want to ment why this one doesn't have a corresponding proxy:

# transport-only struct corresponding to `std::pair<std::string, bool>` in `CRPCCommand` constructor
struct RPCArg {

ryanofsky commented at 1:39 PM on December 12, 2025:

re: #29409 (review)

we might want to ment why this one doesn't have a corresponding proxy:

Good suggestion, added a version of this comment

in src/ipc/capnp/chain.capnp:143 in d9efd1e49d

 138 | +
 139 | +struct JSONRPCRequest $Proxy.wrap("JSONRPCRequest") {
 140 | +    id @0 :Text;
 141 | +    method @1 :Text $Proxy.name("strMethod");
 142 | +    params @2 :Text;
 143 | +    mode @3 :UInt32;

l0rinc commented at 12:19 PM on November 25, 2025:

same, should likely be

    mode [@3](/bitcoin-bitcoin/contributor/3/) :Int32;

ryanofsky commented at 1:40 PM on December 12, 2025:

re: #29409 (review)

same, should likely be

Thanks, updated. Agree signed type is a little nicer here (though both types should be safe)

in src/ipc/capnp/chain.capnp:140 in d9efd1e49d outdated

 135 | +struct HelpResult {
 136 | +    message @0 :Text;
 137 | +}
 138 | +
 139 | +struct JSONRPCRequest $Proxy.wrap("JSONRPCRequest") {
 140 | +    id @0 :Text;

l0rinc commented at 12:20 PM on November 25, 2025:

Is this an accurate mapping of https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/rpc/request.h#L55 ?

ryanofsky commented at 1:41 PM on December 12, 2025:

re: #29409 (review)

Is this an accurate mapping of std::optional<UniValue>

Yes, it is. The UniValue fields is serialized as JSON text here. And Cap'n Proto allows text fields to be null so std::nullopt value is just represented by an null text field.

in src/ipc/capnp/chain.capnp:150 in d9efd1e49d

 145 | +    authUser @5 :Text;
 146 | +    peerAddr @6 :Text;
 147 | +    version @7: Int32 $Proxy.name("m_json_version");
 148 | +}
 149 | +
 150 | +interface RunLaterCallback $Proxy.wrap("ProxyCallback<std::function<void()>>") {

l0rinc commented at 12:21 PM on November 25, 2025:

Is this still used after #29409 (comment)?

ryanofsky commented at 1:42 PM on December 12, 2025:

re: #29409 (review)

Is this still used after #29409 (comment)?

No, thanks for pointing this out. Dropped this in latest push

in src/ipc/capnp/chain.capnp:173 in d9efd1e49d

 168 | +    hash @0 :Data;
 169 | +    height @1 :Int32;
 170 | +    time @2 :Int64;
 171 | +    maxTime @3 :Int64;
 172 | +    mtpTime @4 :Int64;
 173 | +    inActiveChain @5 :Int64;

l0rinc commented at 12:23 PM on November 25, 2025:

in FoundBlockParam this is a bool https://github.com/bitcoin/bitcoin/blob/d9efd1e49d1df154970b6a60229eedde3ba7cffe/src/ipc/capnp/chain.capnp#L161 also in https://github.com/bitcoin/bitcoin/blob/5fe753b56f450b054c42227c5df8346c72447490/src/interfaces/chain.h#L72

ryanofsky commented at 1:43 PM on December 12, 2025:

re: #29409 (review)

in FoundBlockParam this is a bool

I think you probably meant to link to FoundBlockResult not FoundBlockParam but good catch: Int64 was wasteful and Bool is better. Now updated.

in src/ipc/capnp/chain.capnp:93 in d9efd1e49d outdated

  88 | +    stop @5 (context :Proxy.Context) -> ();
  89 | +    setMockTime @6 (context :Proxy.Context, time :Int64) -> ();
  90 | +    schedulerMockForward @7 (context :Proxy.Context, time :Int64) -> ();
  91 | +}
  92 | +
  93 | +struct FeeCalculation $Proxy.wrap("FeeCalculation") {

l0rinc commented at 12:26 PM on November 25, 2025:

best_height seems to be missing: https://github.com/bitcoin/bitcoin/blob/1a7fb5eeeef3575c1e7c27915c9b98695191299d/src/policy/fees/block_policy_estimator.h#L98 introduced in https://github.com/bitcoin/bitcoin/commit/1a7fb5eeeef3575c1e7c27915c9b98695191299d#diff-722dfe5b8bcfb83b1d2f2c9b10800a873325eac8dfc343239f6c851220508c4eR98

ryanofsky commented at 1:44 PM on December 12, 2025:

re: #29409 (review)

best_height seems to be missing:

Great catch! This is a real bug. Fixed and added comment to the C++ struct to prevent these getting out of sync in the future.

in src/interfaces/chain.h:369 in 9100cb97e5 outdated

 365 | @@ -366,7 +366,7 @@ class Chain
 366 |      //! support for writing null values to settings.json.
 367 |      //! Depending on the action returned by the update function, this will either
 368 |      //! update the setting in memory or write the updated settings to disk.
 369 | -    virtual bool updateRwSetting(const std::string& name, const SettingsUpdate& update_function) = 0;
 370 | +    virtual bool updateRwSetting(const std::string& name, SettingsUpdate update_function) = 0;

l0rinc commented at 1:04 PM on November 25, 2025:

Do we have to wait for SettingsValue to be exposed for the other consts to be removed as well? Looks a bit weird that this isn't consted anymore, while other similar ones still are: https://github.com/bitcoin/bitcoin/blob/5336bcd5784925cd722cff4b9c28e58c64c0b708/src/interfaces/node.h#L111-L116

ryanofsky commented at 1:47 PM on December 12, 2025:

re: #29409 (review)

Do we have to wait for SettingsValue to be exposed for the other consts to be removed as well?

It looks like const references there could be removed, but did you mean to link to a different interface file? The node.h interface is used by the GUI, unlike the chain.h interface used by wallet. But it could be changed in a separate PR and I noted some other ideas for cleaning up the settings interfaces previously in #30697 (comment) and #30828#pullrequestreview-2297231104

in src/rpc/request.h:65 in 9100cb97e5 outdated

  60 | @@ -61,6 +61,8 @@ class JSONRPCRequest
  61 |      std::string peerAddr;
  62 |      std::any context;
  63 |      JSONRPCVersion m_json_version = JSONRPCVersion::V1_LEGACY;
  64 | +    // Note: If you add fields to this struct, you should also update the
  65 | +    // JSONRPCRequest struct in ipc/capnp/chain.capnp.

l0rinc commented at 1:07 PM on November 25, 2025:

can't we auto-detect that via reflection maybe?

ryanofsky commented at 1:49 PM on December 12, 2025:

re: #29409 (review)

can't we auto-detect that via reflection maybe?

Yes there are different ways that could be detected automatically, not even requiring reflection necessarily. Like you could declare a duplicate struct and assert the sizes of the structs are the same. I think it would actually be possible for the code generator to do this automatically.

I do think the comment here makes sense even with additional enforcement, though since it should be helpful to anyone updating this code or reviewing changes to it.

in src/ipc/capnp/common-types.h:50 in d9efd1e49d

  45 | +inline T ToBlob(const kj::ArrayPtr<const kj::byte>& array)
  46 | +{
  47 | +    return T({array.begin(), array.begin() + array.size()});
  48 | +}
  49 | +
  50 | +//! Convert base_blob to kj::ArrayPtr.

l0rinc commented at 1:26 PM on November 25, 2025:

while ToArray seems to be used mostly for uint256, there are lots of DataStream calls to it which is not base_blob

//! Convert contiguous byte data (e.g. base_blob, DataStream) to kj::ArrayPtr.

ryanofsky commented at 1:52 PM on December 12, 2025:

re: #29409 (review)

while ToArray seems to be used mostly for uint256, there are lots of DataStream calls to it which is not base_blob

Thanks took your suggestion and also updated the ToBlob comment

in src/ipc/capnp/chain.cpp:199 in d9efd1e49d outdated

 194 | +
 195 | +void ProxyServerMethodTraits<ipc::capnp::messages::ChainClient::StartParams>::invoke(ChainContext& context)
 196 | +{
 197 | +    // This method is never called because ChainClient::Start is overridden by
 198 | +    // WalletLoader::Start. The custom implementation is needed just because
 199 | +    // the CScheduler& argument this is supposed to pass is not serializable.

l0rinc commented at 1:33 PM on November 25, 2025:

is this still accurate?

ryanofsky commented at 1:54 PM on December 12, 2025:

re: #29409 (review)

is this still accurate?

Yes the ChainClient::start method does still take a scheduler parameter (so the node and wallet can share a scheduler.

https://github.com/bitcoin/bitcoin/blob/938d7aacabd0bb3784bb3e529b1ed06bb2891864/src/interfaces/chain.h#L417

This could probably be changed, but the PR is trying to avoid changing the interface or changing behavior at all.

l0rinc changes_requested

l0rinc commented at 1:43 PM on November 25, 2025: contributor

Rebased locally and did a first pass through the changes. It seems to me the capnp wrapper needs some updates (missing field, 32/64 bit params, param rename, bool vs uint64, enums as unsigned, optional mapping, unused declarations, comments, etc).

Unrelated nit: while checking the code I noticed that RemoveCvRef and std::remove_cv_t<std::remove_reference_t can likely be changed to std::remove_cvref_t in a few places (should be done in a different PR, if valid)

DrahtBot added the label Needs rebase on Dec 4, 2025

ryanofsky force-pushed on Dec 12, 2025

DrahtBot removed the label Needs rebase on Dec 12, 2025

ryanofsky commented at 2:02 PM on December 12, 2025: contributor

Thanks for the close review, and sorry for the delay responding and updating this. You found a lot of annoying discrepencies and a real bug in the estimateSmartFee best_height return value, which should all be fixed now.

Rebased d9efd1e49d1df154970b6a60229eedde3ba7cffe -> 89cf624fe811d90d43f22d4519d859716d3ea958 (pr/ipc-chain.21 -> pr/ipc-chain.22, compare) fixing conflict with #33774 and making suggested updates

Rebased 89cf624fe811d90d43f22d4519d859716d3ea958 -> b2cbdd3c3297c5981895c7546fbfcbf1ea14fd02 (pr/ipc-chain.22 -> pr/ipc-chain.23, compare)

DrahtBot added the label CI failed on Dec 12, 2025

DrahtBot removed the label CI failed on Dec 12, 2025

ryanofsky force-pushed on Dec 16, 2025

bitcoin deleted a comment on Dec 21, 2025

ryanofsky force-pushed on Jan 6, 2026

ryanofsky commented at 8:59 PM on January 6, 2026: contributor

Rebased b2cbdd3c3297c5981895c7546fbfcbf1ea14fd02 -> b7b6d6c316221f86914316e82e593fc35df1c92d (pr/ipc-chain.23 -> pr/ipc-chain.24, compare) due to silent conflict with #30214

Rebased b7b6d6c316221f86914316e82e593fc35df1c92d -> 46bc59f581bd90f3b0f73be86a913afadc022bb0 (pr/ipc-chain.24 -> pr/ipc-chain.25, compare) due to conflict with #34568 and moving struct definitions from chain > common due to node interface now using them with bitcoin-core/gui#807

Updated 46bc59f581bd90f3b0f73be86a913afadc022bb0 -> 5c2b520f6b7a2e14048df093c834410077c250bb (pr/ipc-chain.25 -> pr/ipc-chain.26, compare) to fix missing include https://github.com/bitcoin/bitcoin/actions/runs/22499750543/job/65183484335?pr=29409

Rebased 5c2b520f6b7a2e14048df093c834410077c250bb -> e66150d2e4833ad2d6cf4955b3c793449c326494 (pr/ipc-chain.26 -> pr/ipc-chain.27, compare) including https://github.com/bitcoin-core/libmultiprocess/pull/247 to fix sanitizer CI failure (https://github.com/bitcoin/bitcoin/actions/runs/22501264518/job/65188721708?pr=29409#step:11:2216) caused by #34660 and LLVM bug

Updated e66150d2e4833ad2d6cf4955b3c793449c326494 -> 22f2c9a0d8817276c5738271b6389f47a9023809 (pr/ipc-chain.27 -> pr/ipc-chain.28, compare) cherry-picking fix from #34422 to fix macos CI error https://github.com/bitcoin/bitcoin/actions/runs/22555221663/job/65331380982?pr=29409

Rebased 22f2c9a0d8817276c5738271b6389f47a9023809 -> 6e3ecc8f05b8ae7fc405fea353652c120a210fbe (pr/ipc-chain.28 -> pr/ipc-chain.29, compare) due to conflicts

Rebased 6e3ecc8f05b8ae7fc405fea353652c120a210fbe -> 6833d5f2b49fed8182160c5996ae94d7dfb476a1 (pr/ipc-chain.29 -> pr/ipc-chain.30, compare) to fix tidy error https://github.com/bitcoin/bitcoin/actions/runs/23811319241/job/69398846880

DrahtBot added the label Needs rebase on Feb 20, 2026

ryanofsky force-pushed on Feb 27, 2026

DrahtBot removed the label Needs rebase on Feb 27, 2026

ryanofsky force-pushed on Feb 27, 2026

DrahtBot added the label CI failed on Feb 27, 2026

DrahtBot commented at 7:47 PM on February 27, 2026: contributor

🚧 At least one of the CI tasks failed. Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/22499750543/job/65183484335 LLM reason (✨ experimental): Cap'n Proto generated proxy-types are referencing incomplete types (e.g., FeeCalculation/EstimationResult), causing compilation errors.

<details><summary>Hints</summary>

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.

</details>

ryanofsky referenced this in commit f61af48721 on Mar 1, 2026

ryanofsky force-pushed on Mar 1, 2026

ryanofsky force-pushed on Mar 2, 2026

DrahtBot removed the label CI failed on Mar 2, 2026

DrahtBot added the label Needs rebase on Mar 3, 2026

sedited commented at 5:34 PM on March 4, 2026: contributor

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

ryanofsky referenced this in commit 22bec918c9 on Mar 9, 2026

xyzconstant referenced this in commit 7e15751902 on Mar 13, 2026

xyzconstant referenced this in commit 4c1a9480f5 on Mar 13, 2026

xyzconstant referenced this in commit b77a815e2c on Mar 13, 2026

xyzconstant referenced this in commit f2b5868277 on Mar 14, 2026

xyzconstant referenced this in commit cc1ae9915b on Mar 14, 2026

xyzconstant referenced this in commit 3914820da5 on Mar 14, 2026

ryanofsky force-pushed on Mar 31, 2026

DrahtBot removed the label Needs rebase on Mar 31, 2026

Squashed 'src/ipc/libmultiprocess/' changes from 70f632bda8f..6d796edac65

6d796edac65 tidy fix: modernize-use-nullptr
c206f8328c7 mpgen: support primitive std::optional struct fields
46df71115ed mpgen refactor: add AccessorType function
a4b85dabd84 mpgen refactor: Move field handling code to FieldList class

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: 6d796edac652292a020bd976cffaa866c9d140f4

c4c677ce91

Merge commit 'c4c677ce917b418249e915b9f67663e75a5a6847' into pr/ipc-chain 5467330635

Add capnp serialization code for bitcoin types

- 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

2312d50a72

Add capnp wrapper for Handler interface 5b6d662d48

Add capnp wrapper for Chain interface 1c9dcf2f39

multiprocess: Expose Chain interface

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

6833d5f2b4

DrahtBot added the label CI failed on Mar 31, 2026

ryanofsky force-pushed on Mar 31, 2026

DrahtBot removed the label CI failed on Mar 31, 2026

Sjors commented at 10:23 AM on April 1, 2026: member

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 commented at 1:32 PM on April 20, 2026: contributor

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

<details> <summary>Click for backtrace</summary>

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](/bitcoin-bitcoin/0/)  0x00007ffff73298cb in __GI___writev (iovcnt=2, iov=0x7ffff61fc130, fd=26) at ../sysdeps/unix/sysv/linux/writev.c:26
[#1](/bitcoin-bitcoin/1/)  __GI___writev (fd=26, iov=0x7ffff61fc130, iovcnt=2) at ../sysdeps/unix/sysv/linux/writev.c:24
[#2](/bitcoin-bitcoin/2/)  0x00007ffff7c8b987 in ?? () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
[#3](/bitcoin-bitcoin/3/)  0x00007ffff7c8c394 in ?? () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
[#4](/bitcoin-bitcoin/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](/bitcoin-bitcoin/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](/bitcoin-bitcoin/6/)  0x00007ffff7da4865 in capnp::MessageStream::writeMessages(kj::ArrayPtr<capnp::MessageAndFds>) () from /lib/x86_64-linux-gnu/libcapnp-rpc-1.0.1.so
[#7](/bitcoin-bitcoin/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](/bitcoin-bitcoin/8/)  0x00007ffff7e03e4a in ?? () from /lib/x86_64-linux-gnu/libcapnp-rpc-1.0.1.so
[#9](/bitcoin-bitcoin/9/)  0x00007ffff7c4161d in kj::_::TransformPromiseNodeBase::get(kj::_::ExceptionOrValue&) () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
[#10](/bitcoin-bitcoin/10/) 0x00007ffff7c4245d in kj::_::ChainPromiseNode::fire() () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
[#11](/bitcoin-bitcoin/11/) 0x00007ffff7c38509 in kj::EventLoop::turn() () from /lib/x86_64-linux-gnu/libkj-async-1.0.1.so
[#12](/bitcoin-bitcoin/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](/bitcoin-bitcoin/13/) 0x00005555561d6220 in kj::Promise<unsigned long>::wait (this=<optimized out>, waitScope=..., location=...) at /usr/include/kj/async-inl.h:1357
[#14](/bitcoin-bitcoin/14/) 0x00005555561d2bd2 in mp::EventLoop::loop (this=0x555556b9e378) at /home/drgrid/bitcoin/src/ipc/libmultiprocess/src/mp/proxy.cpp:244
[#15](/bitcoin-bitcoin/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](/bitcoin-bitcoin/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](/bitcoin-bitcoin/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](/bitcoin-bitcoin/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](/bitcoin-bitcoin/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](/bitcoin-bitcoin/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](/bitcoin-bitcoin/21/) 0x00007ffff76ecdb4 in ?? () from /lib/x86_64-linux-gnu/libstdc++.so.6
[#22](/bitcoin-bitcoin/22/) 0x00007ffff729caa4 in start_thread (arg=<optimized out>) at ./nptl/pthread_create.c:447
[#23](/bitcoin-bitcoin/23/) 0x00007ffff7329c6c in clone3 () at ../sysdeps/unix/sysv/linux/x86_64/clone3.S:78
(gdb)

</details>

ryanofsky commented at 3:38 PM on April 21, 2026: contributor

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

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

in src/ipc/capnp/chain.capnp:92 in 6833d5f2b4

  87 | +    load @3 (context :Proxy.Context) -> (result :Bool);
  88 | +    start @4 (context :Proxy.Context, scheduler :Void) -> ();
  89 | +    stop @5 (context :Proxy.Context) -> ();
  90 | +    setMockTime @6 (context :Proxy.Context, time :Int64) -> ();
  91 | +    schedulerMockForward @7 (context :Proxy.Context, deltaSeconds :Int64) -> ();
  92 | +}

xyzconstant commented at 6:23 PM on April 27, 2026:

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.

ryanofsky commented at 4:12 PM on May 7, 2026:

re: #29409 (review)

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.

xyzconstant commented at 6:04 PM on May 7, 2026:

I get it now, thanks you!

xyzconstant referenced this in commit 2cb0524688 on May 1, 2026

xyzconstant referenced this in commit a710a5fb9d on May 5, 2026

xyzconstant referenced this in commit 53f21459ea on May 11, 2026

Sjors referenced this in commit 62845e278b on May 12, 2026

Sjors referenced this in commit 328a8324a6 on May 12, 2026

Sjors commented at 7:56 PM on May 12, 2026: member

Here's the (partially implemented) Rust bindings: https://github.com/2140-dev/bitcoin-capnp-types/pull/22

And https://github.com/Sjors/electrs/pull/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)

in src/ipc/capnp/chain.capnp:37 in 1c9dcf2f39

  32 | +    guessVerificationProgress @13 (context :Proxy.Context, blockHash :Data) -> (result :Float64);
  33 | +    hasBlocks @14 (context :Proxy.Context, blockHash :Data, minHeight :Int32, maxHeight: Int32, hasMaxHeight :Bool) -> (result :Bool);
  34 | +    isRBFOptIn @15 (context :Proxy.Context, tx :Data) -> (result :Int32);
  35 | +    isInMempool @16 (context :Proxy.Context, txid :Data) -> (result :Bool);
  36 | +    hasDescendantsInMempool @17 (context :Proxy.Context, txid :Data) -> (result :Bool);
  37 | +    broadcastTransaction @18 (context :Proxy.Context, tx: Data, maxTxFee :Int64, broadcastMethod :Int32) -> (error: Text, result :Bool);

willcl-ark commented at 10:51 AM on May 20, 2026:

In 1c9dcf2f397fa0b58cad936bde2feaaefa532745

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.

willcl-ark commented at 10:57 AM on May 20, 2026:

In 1c9dcf2f397fa0b58cad936bde2feaaefa532745

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.

ryanofsky commented at 12:36 PM on May 20, 2026:

re: #29409 (review)

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 c6638fa7c5e97f9fd7a5ea8feb29f8caeac788bd. Another way is to make method implementations handle invalid values better, for example by throwing exceptions instead of asserting.

in src/ipc/capnp/chain.capnp:38 in 1c9dcf2f39

  33 | +    hasBlocks @14 (context :Proxy.Context, blockHash :Data, minHeight :Int32, maxHeight: Int32, hasMaxHeight :Bool) -> (result :Bool);
  34 | +    isRBFOptIn @15 (context :Proxy.Context, tx :Data) -> (result :Int32);
  35 | +    isInMempool @16 (context :Proxy.Context, txid :Data) -> (result :Bool);
  36 | +    hasDescendantsInMempool @17 (context :Proxy.Context, txid :Data) -> (result :Bool);
  37 | +    broadcastTransaction @18 (context :Proxy.Context, tx: Data, maxTxFee :Int64, broadcastMethod :Int32) -> (error: Text, result :Bool);
  38 | +    getTransactionAncestry @19 (context :Proxy.Context, txid :Data) -> (ancestors :UInt64, descendants :UInt64, ancestorsize :UInt64, ancestorfees :Int64);

willcl-ark commented at 10:54 AM on May 20, 2026:

In 1c9dcf2f397fa0b58cad936bde2feaaefa532745

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

ryanofsky commented at 12:44 PM on May 20, 2026:

re: #29409 (review)

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

in src/ipc/capnp/chain.capnp:42 in 1c9dcf2f39

  37 | +    broadcastTransaction @18 (context :Proxy.Context, tx: Data, maxTxFee :Int64, broadcastMethod :Int32) -> (error: Text, result :Bool);
  38 | +    getTransactionAncestry @19 (context :Proxy.Context, txid :Data) -> (ancestors :UInt64, descendants :UInt64, ancestorsize :UInt64, ancestorfees :Int64);
  39 | +    calculateIndividualBumpFees @20 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result: List(Common.PairInt64(Data)));
  40 | +    calculateCombinedBumpFee @21 (context :Proxy.Context, outpoints :List(Data), targetFeerate :Data) -> (result :Int64, hasResult :Bool);
  41 | +    getPackageLimits @22 (context :Proxy.Context) -> (ancestors :UInt32, descendants :UInt32);
  42 | +    checkChainLimits @23 (context :Proxy.Context, tx :Data) -> (result :Common.ResultVoid);

willcl-ark commented at 10:58 AM on May 20, 2026:

In 1c9dcf2f397fa0b58cad936bde2feaaefa532745

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.

ryanofsky commented at 1:10 PM on May 20, 2026:

re: #29409 (review)

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.

in src/ipc/capnp/chain.cpp:195 in 1c9dcf2f39

 190 | +    result->m_context.cleanup_fns.emplace_back([rpc_command = rpc_command.release()] { delete rpc_command; });
 191 | +    results.setResult(kj::mv(result));
 192 | +    return {};
 193 | +}
 194 | +
 195 | +void ProxyServerMethodTraits<ipc::capnp::messages::ChainClient::StartParams>::invoke(ChainContext& context)

willcl-ark commented at 11:12 AM on May 20, 2026:

In 1c9dcf2f397fa0b58cad936bde2feaaefa532745

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.

ryanofsky commented at 2:18 PM on May 20, 2026:

re: #29409 (review)

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 (review), 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.

willcl-ark commented at 11:19 AM on May 20, 2026: member

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.

ryanofsky commented at 2:20 PM on May 20, 2026: contributor

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