Add Cap’n Proto wrapper for the Mining interface introduced in #30200, and its associated types.
This PR combined with #30509 will allow a separate mining process, like the one being implemented in https://github.com/Sjors/bitcoin/pull/48, to connect to the node over IPC, and create, manage, and submit block templates. (#30437 shows another simpler demo of a process using the Mining interface.)
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.
| Type | Reviewers |
|---|---|
| Stale ACK | TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Is the idea to incorporate the interface changes proposed in #30440 and #30409 after they’re merged? And in the mean time keep them in #30437?
Maybe also link to #30437 in the description as it’s a more simple demo than https://github.com/Sjors/bitcoin/pull/48.
Is the idea to incorporate the interface changes proposed in #30440 and #30409 after they’re merged? And in the mean time keep them in #30437?
I could add them here. Just let me know what you prefer. I am planning to rebase #30437 on top of this, too.
Maybe also link to #30437 in the description as it’s a more simple demo than Sjors#48.
Thanks, added
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27810447228
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.
pr/mine-types.1 -> pr/mine-types.2, compare) to fix link error in https://github.com/bitcoin/bitcoin/pull/30510/checks?check_run_id=27810447228
Was able to simplify this by dropping CustomBuildMessage/CustomReadMessage functions and tests. It now requires the latest version of the libmultiprocess library though.
Rebased 79cfc0d198faecec598f923e6bf83ccf8dc09bbb -> 6cccb436557c58e0ccd21ffe0eaf31058f5cb799 (pr/mine-types.2 -> pr/mine-types.3, compare) dropping the first commit
119+ auto data = input.get();
120+ SpanReader stream({data.begin(), data.end()});
121+ // TODO: instead of always preferring Deserialize implementation over
122+ // Unserialize should prefer Deserializing when emplacing, unserialize when
123+ // updating. Can implement by adding read_dest.alreadyConstructed()
124+ // constexpr bool method in libmultiprocess.
re: #30510 (review)
I’m having a hard time reading this TODO. Can you try and reformulate it?
I’m removed the TODO since it’s a low-priority efficiency improvement. Basically, there are 2 ways to call CustomReadField and there are 2 ways to deserialize certain objects, and the code is not currently choosing the most efficient way to deserialize based on the way it is called.
To explain further, the CustomReadField function is responsible for converting one field of a capnproto struct into a C++ object. (This C++ object will be an input parameter passed to a function in the IPC server process, or it will be an output parameter, return value, or an exception to be thrown in the IPC client process.)
One way to call CustomReadField is to pass a ReadDestEmplace object as the read_dest parameter. The other way to call it is to pass a ReadDestValue object as the read_dest parameter.
You pass ReadDestValue when you want the CustomReadField implementation to read some capnpproto struct field and use the content to update an existing C++ object. ReadDestValue just contains a reference to the object that should be updated.
You pass ReadDestEmplace when there is no existing C++ object and you want the CustomReadField implementation to read a capnproto field and construct a new C++ object from it by calling an emplace function. ReadDestEmplace is needed if the C++ type is immutable like CTransaction and can’t be modified after is created. It is also useful for mutable objects if they are being used as std::map keys, because std::map keys are const, so it is less awkward and more efficient to call std::map::emplace than try to read the capnproto field into a temporary object and then try to move that object into the std::map.
As for the TODO, the TODO is just saying right now if a C++ class has both a deserialize_type constructor and an Unserialize method, the current CustomReadField implementation will always prefer calling the deserialize_type constructor. This works and is ideal if ReadDestEmplace is passed, but is not ideal if ReadDestValue is passed, because that requires the existing C++ object to be destroyed and reconstructed to call the deserialize_type constructor, when probably it would be more efficient to call the Unserialize method instead. So the TODO describes a libmultiprocess modification which would enable that.
121+ // TODO: instead of always preferring Deserialize implementation over
122+ // Unserialize should prefer Deserializing when emplacing, unserialize when
123+ // updating. Can implement by adding read_dest.alreadyConstructed()
124+ // constexpr bool method in libmultiprocess.
125+ auto wrapper{ipc::capnp::Wrap(stream)};
126+ return read_dest.construct(deserialize, wrapper);
g_ prefix (though it is not introduced in this PR).
re: #30510 (review)
Nit: it would be nice to give this the global
g_prefix (though it is not introduced in this PR).
It’s a good point but I think modifying serialize.h is outside the scope of this PR, so I will leave it alone here.
I’m not sure a g_ prefix is ideal since this is a constant, so all caps DESERIALIZE would probably communicate its purpose more accurately and be more in spirit of the the developer notes.
I think If I were changing this, I would eliminate the deserialize global entirely and rename struct deserialize_type just to struct Deserialize, replacing current uses of deserialize with Deserialize{}. The would avoid the ugliness of all-caps DESERIALIZE and also make clear this is a token parameter which doesn’t actually hold any information.
106 });
107 }
108
109+//! Overload multiprocess library's CustomReadField hook to allow any object
110+//! with an deserialize constructor to be read from a capnproto Data field or
111+//! returned from canproto interface. Use Priority<1> so this hook has medium
10@@ -11,4 +11,14 @@
11 #include <node/types.h>
12 #include <validation.h>
13
14+namespace mp {
15+// Custom serialization for BlockValidationState.
16+void CustomBuildMessage(InvokeContext& invoke_context,
17+ const BlockValidationState& src,
18+ ipc::capnp::messages::BlockValidationState::Builder&& builder);
19+void CustomReadMessage(InvokeContext& invoke_context,
re: #30510 (review)
Not directly related to this PR, but did these type hook methods get renamed since the docs were written? https://github.com/ryanofsky/bitcoin/blob/pr/ipc/doc/design/multiprocess.md#type-hooks-in-srcipccapnp-typesh
Good catch, updated the docs to mention these. The custom message hooks have actually been around a while but I moved them from bitcoin code to libmultiprocess code recently in https://github.com/chaincodelabs/libmultiprocess/pull/105. The message hooks are just simpler but less flexible alternatives to the field hooks, as described there.
0@@ -0,0 +1,12 @@
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+#ifndef BITCOIN_IPC_CAPNP_MINING_H
6+#define BITCOIN_IPC_CAPNP_MINING_H
7+
8+#include <interfaces/mining.h>
9+#include <ipc/capnp/mining.capnp.h>
mining.capnp, but in turn includes the auto-generated header from mining.capnp here. What is the purpose of this? Couldn’t the capnp file just directly include interfaces/mining.h? At least doing so compiles for me.
re: #30510 (review)
I’m a bit confused by this file. It is only included by
mining.capnp, but in turn includes the auto-generated header frommining.capnphere. What is the purpose of this? Couldn’t the capnp file just directly includeinterfaces/mining.h? At least doing so compiles for me.
I simplified this as suggested. I had just copied these mining interface files from one of the other interfaces (probably node or chain) and did not bother to simplify it.
The mining interface is pretty basic and does not require specializing the mp::ProxyServer and mp::ProxyClient classes like some other interfaces (to manage lifetimes of pointer and reference parameters passed through those interfaces). If it did, these definitions would go in mining.h because those specializations would be needed by the mining.capnp.proxy.h file generated by libmultiprocess. They could not go in mining-types.h because it depends on that file, which would cause a circular dependency.
If the mining interface did need to specialize the proxy client or proxy server classes, the includes would look like:
0graph BT
1 CH[mining.capnp.h]:::gencap
2 H[mining.h]
3 PH[mining.capnp.proxy.h]:::genmp
4 TH[mining-types.h]
5 PTH[mining.capnp.proxy-types.h]:::genmp
6
7 H --> CH
8 PH --> H
9 PH --> CH
10 PTH --> PH
11 TH --> PH
12 TH --> CH
13 PTH --> TH
14 PTH --> CH
15
16 classDef gencap fill:#ffecff,stroke:#937093,font-style:italic;
17 classDef genmp fill:#ecffff,stroke:#709393,font-style:italic;
with:
mining.capnp.h generated by Cap’n Proto and exposing capnp structs and interfaces to c++mining.h written manually and customizing mp::ProxyServer and mp::ProxyClient classes.mining.capnp.proxy.h generated by libmultiprocess and providing accessors for capnp types, using customizations from previous.mining-types.h written manually and customizing C++ type ↔️ Cap’n Proto type conversion, using accessors from previous.mining.capnp.proxy-types.h generated by libmultiprocess, providing type conversions for Capnp structs annotated with $Proxy.wrap, and using manual type conversions from previous70@@ -70,6 +71,13 @@ void IpcTest()
71 CTransactionRef tx2{foo->passTransaction(tx1)};
72 BOOST_CHECK(*Assert(tx1) == *Assert(tx2));
73
74+ BlockValidationState bs1;
75+ bs1.Invalid(BlockValidationResult::BLOCK_CHECKPOINT, "reject reason", "debug message");
76+ BlockValidationState bs2{foo->passBlockState(bs1)};
77+ BOOST_CHECK_EQUAL(static_cast<int>(bs1.GetResult()), static_cast<int>(bs2.GetResult()));
78+ BOOST_CHECK_EQUAL(bs1.GetRejectReason(), bs2.GetRejectReason());
79+ BOOST_CHECK_EQUAL(bs1.GetDebugMessage(), bs2.GetDebugMessage());
I think it would be nice to test some more invariants here:
0diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp
1index aabc0650b8..fa5273446f 100644
2--- a/src/test/ipc_test.cpp
3+++ b/src/test/ipc_test.cpp
4@@ -79,0 +80,3 @@ void IpcTest()
5+ BOOST_CHECK_EQUAL(bs1.IsValid(), bs2.IsValid());
6+ BOOST_CHECK_EQUAL(bs1.IsError(), bs2.IsError());
7+ BOOST_CHECK_EQUAL(bs1.IsInvalid(), bs2.IsInvalid());
8@@ -82,0 +86,9 @@ void IpcTest()
9+ BlockValidationState bs3;
10+ BlockValidationState bs4{foo->passBlockState(bs3)};
11+ BOOST_CHECK_EQUAL(bs3.IsValid(), bs4.IsValid());
12+ BOOST_CHECK_EQUAL(bs3.IsError(), bs4.IsError());
13+ BOOST_CHECK_EQUAL(bs3.IsInvalid(), bs4.IsInvalid());
14+ BOOST_CHECK_EQUAL(static_cast<int>(bs3.GetResult()), static_cast<int>(bs4.GetResult()));
15+ BOOST_CHECK_EQUAL(bs3.GetRejectReason(), bs4.GetRejectReason());
16+ BOOST_CHECK_EQUAL(bs3.GetDebugMessage(), bs4.GetDebugMessage());
Updated 6cccb436557c58e0ccd21ffe0eaf31058f5cb799 -> c538ec69f266b51c893a374a4bb82796ede3d7cb (pr/mine-types.3 -> pr/mine-types.4, compare) with review suggestions.
Thanks for the review and thoughtful questions. I wrote detailed answers in line but I will reuse the text & diagram in documentation for libmultiprocess so there’s a place to point to if similar questions come up again.
Thanks for all the explanations @ryanofsky , I’ll take some time to digest all of this, and read some more libmultiprocess code.
Thanks! Feel free to ping me if you have any questions. it will help me learn what things to work on documentation for.
16@@ -16,6 +17,14 @@
17
18 namespace ipc {
19 namespace capnp {
20+//! Construct a ParamStream wrapping data stream with serialization parameters
21+//! needed to pass transaction objects between bitcoin processes.
22+template<typename S>
23+auto Wrap(S& s)
24+{
25+ return ParamsStream{s, TX_WITH_WITNESS};
8523d19bb3f8c982c6e01a4635c98cd9ac386dea: I wonder if this causes a problem when handling a block template without a witness commitment.
If so, I might just have stratum v2 templates always set a coinbase witness.
re: #30510 (review)
8523d19: I wonder if this causes a problem when handling a block template without a witness commitment.
If so, I might just have stratum v2 templates always set a coinbase witness.
This just sets a serialization flag called allow_witness and fAllowWitness in SerializeTransaction that includes witness data when it is present instead of omitting it. It ensures that if you pass a CTransaction parameter over IPC, the value of the parameter will be unchanged after it is passed. I should add a comment explaining this better, since more serialization flags are added here later in #10102, and may have to be added in the future. But I don’t think there should be a particular concern for the mining interface.
16@@ -16,6 +17,14 @@
17
18 namespace ipc {
19 namespace capnp {
20+//! Construct a ParamStream wrapping data stream with serialization parameters
21+//! needed to pass transaction objects between bitcoin processes.
22+template<typename S>
template.
Add support for passing CTransaction and CTransactionRef types to IPC
functions.
These types can't be passed currently because IPC serialization code currently
only supports deserializing types that have an Unserialize() method, which
CTransaction does not, because it is supposed to represent immutable
transactions. Work around this by adding a CustomReadField overload that will
call CTransaction's deserialize_type constructor.
These types also can't be passed currently because serializing transactions
requires TransactionSerParams to be set. Fix this by setting TX_WITH_WITNESS as
default serialization parameters for IPC code.
This update brings in the following change which allows simplifying the
multiprocess code in bitcoin:
https://github.com/chaincodelabs/libmultiprocess/pull/105 types: Add Custom{Build,Read,Pass}Message hooks
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
Most improvements suggested by stickies-v <stickies-v@protonmail.com>
https://github.com/bitcoin/bitcoin/pull/28978#pullrequestreview-1800375604
Omission of CustomReadMessage and CustomBuildMessage was noticed by
TheCharlatan in
https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1708996040 and is
fixed here as well.
Co-authored-by: stickies-v <stickies-v@protonmail.com>
pr/mine-types.4 -> pr/mine-types.5, compare) updating for cmake
158@@ -159,14 +159,6 @@ if(ENABLE_WALLET)
159 endif()
160
161 if(WITH_MULTIPROCESS)
162- add_library(bitcoin_ipc_test STATIC EXCLUDE_FROM_ALL
163- ipc_test.cpp
164- )
165-
166- target_capnp_sources(bitcoin_ipc_test ${PROJECT_SOURCE_DIR}
Did you move these out of here because you’d otherwise get the following error:
0*** Uncaught exception ***
1capnp/compiler/node-translator.c++:1374: context: member.name = arg
2kj/filesystem.c++:315: failed: expected parts.size() > 0 [0 > 0]; can't use ".." to break out of starting directory
3stack: 59a492c93c94 59a492c93fc7 59a492c916b3 59a492a192f8 59a492b094db 59a492b08eb6 59a492b35fad 59a492b431c8 59a492b043ad 59a492b085a9 59a492b07e69 59a492b080d7 59a492b35b7f 59a492b36834 59a492b4302c 59a492b43495 59a492b587a6 59a492b5556b 59a492b4a2e1 59a492b42aa6 59a492b3fd6c 59a492b3ee13 59a492b1d565 59a492b1710c 59a492b05b7f 59a492b06909 59a492b06c1c 59a492b0bb7e 59a492b0c5b9 59a492a243e9 59a492a5aa22
re: #30510 (review)
Did you move these out of here because you’d otherwise get the following error:
Yes, that’s the reason. It doesn’t seem to be possible to access src/ipc/ files from the files in the src/test/ directory by writing ../ipc/ when the capnp compile source prefix is src/test/, but it does work if the source prefix is src/.
I think a different way to work around this would be to pass --import-path=/path/to/src to capnp compile and then change using Mining = import "../ipc/capnp/mining.capnp" to using Mining = import "/ipc/capnp/mining.capnp", but this would probably be more confusing.
--import-path argument seems akin to the -I option I’d pass to a compiler.
I just think if you see:
0using Mining = import "../ipc/capnp/mining.capnp";
it is very obvious what file is being imported while if you see
0using Mining = import "/ipc/capnp/mining.capnp"
it’s less clear what file is being imported because you don’t know what the root path is. This is also inconsistent with standard uses of the root path which have package prefixes “capnp” and “mp”:
0using Cxx = import "/capnp/c++.capnp";
1using Proxy = import "/mp/proxy.capnp"
We would also be polluting the root namespace with our own files (literally every file in src/) so could cause conflicts with other packages that are being imported.
If we could include a package name like:
0using Mining = import "/bitcoin/ipc/capnp/mining.capnp"
This approach would seem more reasonable to me, but I don’t think there is a way to do that with our current directory structure. I think just using relative paths is fine here and probably the way to go.
This is a confusing CI failure:
0-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-tx.1
1-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-cli.1
2-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoind.1
3-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-qt.1
4-- Installing: /ci_container_base/ci/scratch/out/share/man/man1/bitcoin-util.1
5+ false
https://cirrus-ci.com/task/6743063731634176
It’s also weird because followup PR #30437 which is based on this and includes the same changes shows this test passing.
This is a confusing CI failure:
I’m guessing you saw the actual error further up?
0In file included from /ci_container_base/src/test/ipc_test.cpp:7:
1/ci_container_base/ci/scratch/build-i686-pc-linux-gnu/src/test/ipc_test.capnp.h:18:10: fatal error: '../ipc/capnp/mining.capnp.h' file not found
2 18 | #include "../ipc/capnp/mining.capnp.h"
3 | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4[ 5%] Building CXX object CMakeFiles/minisketch.dir/src/minisketch/src/fields/generic_4bytes.cpp.o
5[ 5%] Building CXX object src/zmq/CMakeFiles/bitcoin_zmq.dir/zmqnotificationinterface.cpp.o
6[ 5%] Building CXX object CMakeFiles/minisketch.dir/src/minisketch/src/fields/generic_5bytes.cpp.o
7[ 5%] Building CXX object CMakeFiles/minisketch.dir/src/minisketch/src/fields/generic_6bytes.cpp.o
8[ 5%] Building CXX object CMakeFiles/leveldb.dir/src/leveldb/table/merger.cc.o
9[ 5%] Building CXX object CMakeFiles/minisketch.dir/src/minisketch/src/fields/generic_7bytes.cpp.o
10[ 6%] Building CXX object CMakeFiles/minisketch.dir/src/minisketch/src/fields/generic_8bytes.cpp.o
11[ 6%] Building CXX object CMakeFiles/leveldb.dir/src/leveldb/table/table.cc.o
12[ 6%] Linking CXX static library libminisketch.a
13[ 6%] Building CXX object CMakeFiles/leveldb.dir/src/leveldb/table/table_builder.cc.o
14[ 6%] Built target minisketch
15[ 6%] Generating capnp/mining.capnp.c++, capnp/mining.capnp.h, capnp/mining.capnp.proxy-client.c++, capnp/mining.capnp.proxy-types.h, capnp/mining.capnp.proxy-server.c++, capnp/mining.capnp.proxy-types.c++, capnp/mining.capnp.proxy.h
16[ 7%] Generating capnp/echo.capnp.c++, capnp/echo.capnp.h, capnp/echo.capnp.proxy-client.c++, capnp/echo.capnp.proxy-types.h, capnp/echo.capnp.proxy-server.c++, capnp/echo.capnp.proxy-types.c++, capnp/echo.capnp.proxy.h
171 error generated.
18make[2]: *** [src/CMakeFiles/bitcoin_ipc_test.dir/build.make:98: src/CMakeFiles/bitcoin_ipc_test.dir/test/ipc_test.cpp.o] Error 1
19make[1]: *** [CMakeFiles/Makefile2:1036: src/CMakeFiles/bitcoin_ipc_test.dir/all] Error 2
20make[1]: *** Waiting for unfinished jobs....
21[ 7%] Generating capnp/init.capnp.c++, capnp/init.capnp.h, capnp/init.capnp.proxy-client.c++, capnp/init.capnp.proxy-types.h, capnp/init.capnp.proxy-server.c++, capnp/init.capnp.proxy-types.c++, capnp/init.capnp.proxy.h
22[ 7%] Building CXX object src/zmq/CMakeFiles/bitcoin_zmq.dir/zmqpublishnotifier.cpp.o
23[ 7%] Building CXX object CMakeFiles/leveldb.dir/src/leveldb/table/two_level_iterator.cc.o
24[ 7%] Building CXX object CMakeFiles/leveldb.dir/src/leveldb/util/arena.cc.o
25[ 7%] Building CXX object src/ipc/CMakeFiles/bitcoin_ipc.dir/capnp/mining.cpp.o
26[ 7%] Building CXX object CMakeFiles/leveldb.dir/src/leveldb/util/bloom.cc.o
27[ 7%] Building CXX object CMakeFiles/leveldb.dir/src/leveldb/util/cache.cc.o
re: #30510 (comment)
I’m guessing you saw the actual error further up?
No thanks. I missed that. That is also confusing but at least it’s something to work with. I’m sure why it doesn’t happen in normal non-CI builds or seem to happen in the followup PR #30437. Will try to reproduce and fix
src/test/ipc_test.capnp.h file depends on the generated ipc/capnp/mining.capnp.h file, so can fail if these do not happen to be built in the right order. Will need to annotate that somehow.
Labels
CI failed