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.

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

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	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: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:

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

ariard commented at 2:57 am on March 7, 2024: member

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

Add capnp serialization code for bitcoin types

- Add capnp ToBlob, ToArray, Wrap, Serialize, and Unserialize helper functions
- Add support for CTransaction deserialization that requires deserialize
  constructor argument and cannot be unserialized into an existing object
  because it is immutable.
- Add support for std::chrono::seconds capnp serialization
- Add support for util::Result capnp serialization

e57c36cda3

Add capnp wrapper for Handler interface 3c99149473

Add capnp wrapper for Chain interface ab6b795f21

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

multiprocess: Add capnp wrapper for Chain interface #29409

Code Coverage

Reviews

Conflicts