Add type conversion hooks to allow UniValue objects, and objects that have CDataStream Serialize and Unserialize methods to be used as arguments and return values in Cap’nProto interface methods. Also add unit test to verify the hooks are working and data can be round-tripped correctly.
The non-test code in this PR was previously part of #10102 and has been split off for easier review, but the test code is new.
This PR is part of the process separation project.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
61+ // that strips cv references, to prevent this CustomBuildField overload from
62+ // taking precedence over more narrow overloads for specific LocalTypes.
63+ typename std::enable_if_t<ipc::capnp::Serializable<LocalType>::value &&
64+ std::is_same_v<LocalType, std::remove_cv_t<std::remove_reference_t<LocalType>>>>* enable = nullptr)
65+{
66+ CDataStream stream(SER_NETWORK, CLIENT_VERSION);
0 DataStream stream{};
The values are ignored either way, so it seems better to remove them. CDataStream will go away anyway, soon.
re: #28921 (review)
The values are ignored either way, so it seems better to remove them.
CDataStreamwill go away anyway, soon.
Thanks, will update. This previously was used to serialize transactions, but now with #28438 that is broken anyway, so I’ll need to find something different to do for transactions.
11+$Proxy.include("test/ipc_test.h");
12+$Proxy.includeTypes("ipc/capnp/common-types.h");
13+
14+interface FooInterface $Proxy.wrap("FooImplementation") {
15+ add @0 (a :Int32, b :Int32) -> (result :Int32);
16+ passOutPoint @1 (arg :Data) -> (result :Data);
This type of interface where we pass serialized objects (using our serialization) via capnp data obviously fits in quite well with our code base, but I am wondering if this is a nice design for external use? since now anybody using it needs capnp and our serialization format.
I might have missed prior discussion on this, is the plan to gradually convert more of the interface to native capnp types?
I really like this conversion glue by the way (i.e. $Proxy.wrap, etc.)! I’ve been working on a clone of https://github.com/google/libprotobuf-mutator for capnp, which I eventually would like to use to fuzz the multi-process interfaces. Conversion from capnp to c++ types is always a bit annoying, so your work on this could integrate quite well I think.
re: #28921 (review)
This type of interface where we pass serialized objects (using our serialization) via capnp data obviously fits in quite well with our code base, but I am wondering if this is a nice design for external use? since now anybody using it needs capnp and our serialization format.
I think the choice will vary on a case-by-case basis. If you look at chain.capnp, node.capnp, wallet.capnp you’ll see a lot of custom capnp structs being defined and used to send data, but types that support bitcoin serialization are just being passed as Data fields, and some types that support JSON serialization are being passed as Text fields. Using pre-existing serialization formats is nice because it saves code, but the resulting nested data format is not very pretty, and like you say it basically makes writing internal code easier at the expense of making writing external code harder. I think as long as we consider the API unstable and willing to break it, the best thing to do is start with a simple implementation and improve it later. There are other parts of the API that are unwieldy too (like basically the whole Chain interface) so I expect there will be a lot of chances to make improvements.
pr/ipcc.1 -> pr/ipcc.2, compare) fixing lint error and making minor cleanups
Updated 118d60e95cc5c8c0d5a88c0d16904355c7e0c5f6 -> 33d796411dd4d8098287e1307f1337b6e2eab3e7 (pr/ipcc.2 -> pr/ipcc.3, compare) replacing CDataStream with DataStream as suggested and making minor cleanups
Updated 33d796411dd4d8098287e1307f1337b6e2eab3e7 -> a506148b7a7058b4c60e5c497e70688134ee9a3f (pr/ipcc.3 -> pr/ipcc.4, compare) to try to fix MSVC link error https://github.com/bitcoin/bitcoin/actions/runs/6948636857/job/18905029537?pr=28921
58+ // qualified. If LocalType is cv or reference qualified, it is important to
59+ // fall back to lower-priority Priority<0> implementation of this function
60+ // that strips cv references, to prevent this CustomBuildField overload from
61+ // taking precedence over more narrow overloads for specific LocalTypes.
62+ std::enable_if_t<ipc::capnp::Serializable<LocalType>::value &&
63+ std::is_same_v<LocalType, std::remove_cv_t<std::remove_reference_t<LocalType>>>>* enable = nullptr)
I’m gonna try to summarize my understanding of this to make sure I actually got it right.
We want this specialization of CustomBuildField for (most) of our serializable types. So we can either define it manually for each type or we can make use of sfinae (like you do here) to automatically only create the specialization for each of the required serializable types.
We also want to be able to further overload CustomBuildField for serializable types that can’t use this generic specialization (e.g. if we need to use TX_WITH_WITNESS), so we specify Priority<1> such that another overload with Priority<2> would take precedence.
What I don’t understand is making this exclusive to non cv and reference qualified types.
If LocalType is cv or reference qualified, it is important to fall back to lower-priority Priority<0> … to prevent this CustomBuildField overload from taking precedence
As I understand it, this comment implies that (without && std::is_same_v<LocalType, std::remove_cv_t<std::remove_reference_t<LocalType>>>) the following would not take precedence, even though it has Priority<2>?
0template <typename LocalType, typename Output>
1void CustomBuildField(TypeList<LocalType>, Priority<2>, InvokeContext& invoke_context, const CTransaction& value, Output&& output)
2{
3 DataStream stream;
4 stream << TX_WITH_WITNESS(value);
5 auto result = output.init(stream.size());
6 memcpy(result.begin(), stream.data(), stream.size());
7}
As I understand it, this comment implies that (without
&& std::is_same_v<LocalType, std::remove_cv_t<std::remove_reference_t<LocalType>>>) the following would not take precedence, even though it hasPriority<2>?0template <typename LocalType, typename Output> 1void CustomBuildField(TypeList<LocalType>, Priority<2>, InvokeContext& invoke_context, const CTransaction& value, Output&& output)
Yes that’s right, but it would be more correct to say “would not reliably take precedence”. For example, in the case where a const CTransaction argument was passed then the Priority<2> function actually would take precedence, but if a non-const CTransaction were passed the precedence would be ambiguous. This is because if you passed a non-const CTransaction, the types of the two functions after template type deduction would be:
0void CustomBuildField(TypeList<LocalType>, Priority<1>, InvokeContext&, CTransaction&, Output&& output);
1void CustomBuildField(TypeList<LocalType>, Priority<2>, InvokeContext&, const CTransaction&, Output&& output);
So with a non-const CTransaction argument, it would be ambiguous whether to call the Priority<2> function that requires a const conversion, or a Priority<1> function not requiring a const-conversion, and there would be a compile error about the overload being ambiguous.
Code review ACK a506148b7a7058b4c60e5c497e70688134ee9a3f
CI failure looks spurious.
Add unit test to test IPC method calls and type conversion between bitcoin c++
types and capnproto messages.
Right now there are custom type hooks in bitcoin IPC code, so the test is
simple, but in upcoming commits, code will be added to convert bitcoin types to
capnproto messages, and the test will be expanded.
Allow any C++ object that has Serialize and Unserialize methods and can be
serialized to a bitcoin CDataStream to be converted to a capnproto Data field
and passed as arguments or return values to capnproto methods using the Data
type.
Extend IPC unit test to cover this and verify the serialization happens
correctly.
Extend IPC unit test to cover this and verify the serialization happens
correctly.
