multiprocess: Add capnp wrapper for Chain interface

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.

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
ACK	TheCharlatan
Concept ACK	josibake
Ignored review	cbergqvist

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

Conflicts

No conflicts as of last run.

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:199 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:

0if (value) {
1     BuildField(..., ValueAccessor, *value);
2}
3if (!value.has_value()) {
4    BuildField(..., FailureAccessor, value.GetFailure());
5}
6BuildField(..., ErrorsAccessor, value.GetErrors());
7BuildField(..., 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 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:50 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: contributor

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

TheCharlatan 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:186 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,

TheCharlatan 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

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.

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

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

e7619a84bd

Add capnp wrapper for Handler interface 76fd4e56c4

Add capnp wrapper for Chain interface 34d3e2a6ea

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);

TheCharlatan 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);

TheCharlatan 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);

TheCharlatan 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) -> ();

TheCharlatan 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;

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

TheCharlatan approved

TheCharlatan commented at 11:47 am on November 19, 2024: contributor

lgtm ACK 34d3e2a6eaaab9de5328c3e64739f1392696c7db

DrahtBot requested review from josibake on Nov 19, 2024

multiprocess: Add capnp wrapper for Chain interface #29409

Code Coverage & Benchmarks

Reviews

Conflicts