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 |
|---|---|
| ACK | TheCharlatan, itornaza, achow101 |
| Stale ACK | Sjors |
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.
re: #30510 (comment)
Nit: it would be nice to give this the global
g_prefix (though it is not introduced in this PR).
Added :: prefix to clarify it is a global
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.
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.
pr/mine-types.5 -> pr/mine-types.6, compare) testing https://github.com/chaincodelabs/libmultiprocess/pull/110 fix to add explicit cmake generated file dependency and fix unreliable parallel build
Updated 3749b730171e93a0beb3dcc458d440ee7035b3b9 -> 61c7ad56818888b2ee07db02134456aaf3e9aedc (pr/mine-types.6 -> pr/mine-types.7, compare) incorporating previous fix into PR
324@@ -325,6 +325,14 @@ if(WITH_MULTIPROCESS)
325 $<TARGET_NAME_IF_EXISTS:bitcoin_wallet>
326 )
327 list(APPEND installable_targets bitcoin-node)
328+
329+ add_library(bitcoin_ipc_test STATIC EXCLUDE_FROM_ALL
src/test/CMakeFile.txt? A tl&dr of https://github.com/bitcoin/bitcoin/pull/30510/files#r1747656292
re: #30510 (review)
988dace: can you add a comment for why this isn’t in
src/test/CMakeFile.txt? A tl&dr of https://github.com/bitcoin/bitcoin/pull/30510/files#r1747656292
Thanks, added comment
330+ test/ipc_test.cpp
331+ )
332+ target_capnp_sources(bitcoin_ipc_test ${PROJECT_SOURCE_DIR}
333+ test/ipc_test.capnp
334+ )
335+ add_dependencies(bitcoin_ipc_test bitcoin_ipc_headers)
On Intel macOS 14.6.1 this doesn’t work for me (CI feels the same).
0git clean -dfx
1cmake -B build -DWITH_MULTIPROCESS=true
2...
3-- Configuring done (29.3s)
4CMake Error at src/CMakeLists.txt:335 (add_dependencies):
5 The dependency target "bitcoin_ipc_headers" of target "bitcoin_ipc_test"
6 does not exist.
re: #30510 (review)
On Intel macOS 14.6.1 this doesn’t work for me (CI feels the same).
0git clean -dfx 1cmake -B build -DWITH_MULTIPROCESS=true 2... 3-- Configuring done (29.3s) 4CMake Error at src/CMakeLists.txt:335 (add_dependencies): 5 The dependency target "bitcoin_ipc_headers" of target "bitcoin_ipc_test" 6 does not exist.
Thanks, good catch. The bitcoin_ipc_headers target didn’t exist because 357a6bc97b1937bce44e583f0ffab31703a94836 was buggy and specified an older commit hash before https://github.com/chaincodelabs/libmultiprocess/pull/110.
pr/mine-types.8 -> pr/mine-types.9, compare) adding suggested cmake comment, fixing depends package which pointed at an old libmultiprocess version causing https://cirrus-ci.com/task/4659675541536768, and resolving conflicts with #30509
332+ # bitcoin_ipc_test library target is defined here in src/CMakeLists.txt
333+ # instead of src/test/CMakeLists.txt so capnp files in src/test/ are able to
334+ # reference capnp files in src/ipc/capnp/ by relative path. The Cap'n Proto
335+ # compiler only allows importing by relative path when the importing and
336+ # imported files are underneath the same compilation source prefix, so the
337+ # source prefix must be src/, not src/test/
if(BUILD_TESTS)
re: #30510 (review)
Thanks, added if(BUILD_TESTS)
pr/mine-types.9 -> pr/mine-types.10, compare) skipping test library build when tests are disabled, as suggested
158+//! CustomBuildField function because ::capnp::Data inherits from
159+//! ::capnp::ArrayPtr, and libmultiprocess already provides a generic
160+//! CustomReadField function that can read from ::capnp::ArrayPtr into
161+//! std::vector.
162+template <typename LocalType, typename Value, typename Output>
163+void CustomBuildField(
The argument list is really difficult to read due to the nature of the arguments. However, I understand that it can be no better than that anyway!
To be fair, I think this could be significantly simpler now that we are using c++20, which supports concepts. In general there should be no reason to use std::enable_if anymore now that concepts are available. There’s a lot of older code like this that could be switched to use concepts, and I haven’t tried to take that on yet, but this could be a good place to start Thanks for pointing out the issue, and I’ll see any there are any simple improvements that can be made here.
re: #30510 (review)
Removed enable_if so this should be simpler now. Thanks for bringing this up!
ACK 1be749c771cd9fd80361ebb69c87482920b25cd1
Built and run all tests including the extended functional tests and all of them pass locally. Went through the code changes and read all comments of the pr mostly to educate myself better on the ipc domain. Thanks to all the reviewers and the author for being so explicit.
pr/mine-types.10 -> pr/mine-types.11, compare) switching to concepts instead of enable_if
ACK b95bb2179610183d9398d50d8c8fd24b1450ad4d
The changes are on src/ipc/capnp/common-types.h and greatly enhance code reuse, readability and clarity through the use of concepts. As per the standard review procedure, I rebuilt and run all the tests locally and all of them pass.
13+
14+interface Mining $Proxy.wrap("interfaces::Mining") {
15+ isTestChain @0 (context :Proxy.Context) -> (result: Bool);
16+ isInitialBlockDownload @1 (context :Proxy.Context) -> (result: Bool);
17+ getTipHash @2 (context :Proxy.Context) -> (result: Data);
18+ createNewBlock @3 (context :Proxy.Context, scriptPubKey: Data, options: BlockCreateOptions) -> (result: CBlockTemplate);
-> (result: BlockTemplate);
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30078136911
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.11 -> pr/mine-types.12, compare) due to silent conflict with #30440
pr/mine-types.12 -> pr/mine-types.13, compare) due to conflict with #30875
pr/mine-types.13 -> pr/mine-types.14, compare) addressing review comments to clarify code
28- static std::false_type test(...);
29-
30-public:
31- static constexpr bool value = decltype(test<T>(nullptr))::value;
32+concept Serializable = requires(T t) {
33+ decltype(t.Serialize(std::declval<std::nullptr_t&>()))();
re: #30510 (review)
974e041:
serialize.halready defines aconcept UnserializableandSerializable, introduced by @maflcko in #29056.
This is interesting. The concepts are not interchangeable because the definitions in #29056 check whether a particular class can be serialized and unserialized with a particular stream class, while the concepts here check with a class has serialize / unserialize methods accepting any stream.
I think the concepts in serialize.h could be used here, but would require extra template parameters to be passed which would be more verbose and repetitive.
Similarly, the concepts defined here could be moved serialize.h and used there but wouldn’t be as nice for that code, because they don’t check the stream type and have more complicated, less readable definitions.
I think I will experiment with reusing serialize.h concepts here and maybe go with that approach if it doesn’t look too bad. Otherwise it might be better to rename serialize.h concepts to SerializableWith, UnserializableWith to avoid confusion. I think either of these changes could be followups. Current PR should be ok since the concepts live in different namespaces.
re: #30510 (review)
Updated now to use concepts from serialiize.h instead of defining new ones
utACK d59f5a31a1b888051eb47e2f8a5fb8d901de1d10
I previously tested this with https://github.com/Sjors/bitcoin/pull/48, will try to do that again.
Would be nice to get a unified concept Serializable, see above.
For the most part it should be quite straightforward for other developers in the future to change src/ipc/capnp/mining.capnp when something changes in the Mining interface. I’ve been doing that myself.
The only part that seems (slightly) more involved is in 00a82f45004a887c2b58ed8da3db9280e0ec8b5c. If someone were to refactor BlockValidationState, they would have to also adjust its CustomBuildMessage and CustomReadMessage counterparts.
re: #30510 (comment)
Thanks for the review!
The only part that seems (slightly) more involved is in 00a82f4. If someone were to refactor
BlockValidationState, they would have to also adjust itsCustomBuildMessageandCustomReadMessagecounterparts.
I think it might be good idea to change testBlockValidity method anyway to return validation result in simpler form than the BlockValidationState struct. For example, we’ve been discussing calling the Mining interface from rust code that doesn’t have access to the corresponding c++ class and enums. Probably there is something that can be returned that would be more straightforward for rust code to interpret than mode and result ints and rejectReason / debugMessage strings.
130+//!
131+//! There is currently no corresponding ::capnp::Data CustomReadField function
132+//! that works using Spans, because the bitcoin classes in the codebase like
133+//! BaseHash and blob_blob that can converted /to/ Span don't currently have
134+//! Span constructors that allow them to be constructed /from/ Span. If they
135+//! did, it would simplify things. For example, a generic CustomReadField
In 246fd7faae85e871ba78101f1dee8d795437b8f6 “multiprocess: Add serialization code for vector”
Why not introduce Span constructors for those types instead of this field builder if that would simplify things?
re: #30510 (review)
In 246fd7f “multiprocess: Add serialization code for vector”
Why not introduce Span constructors for those types instead of this field builder if that would simplify things?
tl;dr: Adding Span constructors would not eliminate the need for this builder. They would just allow defining a reader that’s as generic as this builder and would eliminate the need for other more specialized readers.
I should trim down & clarify this comment. It was taken from #10102 where this CustomBuildField overload was used for a lot of different things. In this PR, the sole purpose of this builder is to make the BlockTemplate::getCoinbaseCommitment method callable over IPC since it returns a vector<char>:
0 virtual std::vector<unsigned char> getCoinbaseCommitment() = 0;
Specifically, this CustomBuildField overload is needed to convert the vector<char> value returned by this function into a ::capnp::Data value that can be passed over IPC.
Normally if a CustomBuildField overload is defined, a corresponding CustomReadField overload is also defined, because if CustomBuildField function is used to convert a C++ value to a Cap’n Proto value, then a CustomReadField function is usually need to convert the Cap’n Proto value back into a C++ value on the other side of the connection.
The comment is just saying that in the future it might make sense to define a corresponding CustomReadField function here, but currently it would be useless to try that because types that rely on this overload like base_blob and BaseHash don’t have span<char> constructors, and need their own specialized CustomReadField implementations which are defined in followup PRs. If those types had span constructors, a generic CustomReadField could be written to replace the specialized ones.
None of this is really relevant to this PR, only to followups #29409 and #10102. And actually base_blob does have a Span constructor now so this comment is also out of date.
re: #30510 (review)
Rewrote the comment so hopefully it is less confusing now.
135+//! did, it would simplify things. For example, a generic CustomReadField
136+//! function could be written that would allow dropping specialized
137+//! CustomReadField functions for types like PKHash.
138+//!
139+//! For the LocalType = vector<char> case, it's also not necessary to
140+//! have ::capnp::Data CustomReadField function corresponding to this
In 246fd7faae85e871ba78101f1dee8d795437b8f6 “multiprocess: Add serialization code for vector”
This paragraph is a little bit confusing to me since it kind of implies that this function is unnecessary, and if that were the case, why is it here?
re: #30510 (review)
This paragraph is a little bit confusing to me since it kind of implies that this function is unnecessary, and if that were the case, why is it here?
This is saying defining a CustomReadField function corresponding to this CustomBuildField function is unnecessary. Not that this CustomBuildField is unnecessary.
Specifically a CustomReadField function that converts a ::capnp::Data value into a vector<char> is not necessary because ::capnp::Data has .begin() and .end() members and libmultiprocess provides a CustomReadField overload that can convert any iterable capnp type into a std::vector with a compatible elements.
libmultiprocess does provide a CustomBuildField for std::vector, but it is less generic and only is able to convert std::vector values to ::capnp::List values, which are not the same as ::capnp::Data values. Reason for the builder being less generic than the reader is is that c++ provides a generic interface to iterate over collections, but not a generic way to insert into them.
I think in theory it might be possible to move this function to libmultiprocess, though. Could experiment with that.
re: #30510 (review)
I rewrote the comment and tweaked implementation of this to avoid use of non-standard Span class, so this could be dropped and moved to libmultiprocess in the future. But leaving that for a followup since it will require other changes to libmultiprocess.
37+ } else if (reader.getMode() == 1) {
38+ dest.Invalid(static_cast<BlockValidationResult>(reader.getResult()), reader.getRejectReason(), reader.getDebugMessage());
39+ } else if (reader.getMode() == 2) {
40+ assert(reader.getResult() == 0);
41+ dest.Error(reader.getRejectReason());
42+ assert(reader.getDebugMessage().size() == 0);
In 00a82f45004a887c2b58ed8da3db9280e0ec8b5c “multiprocess: Add serialization code for BlockValidationState”
There are some places where a BlockValidationState is first set to invalid with a corresponding result and debug message, before then being set to error later. Presumably if such a BlockValidateState were serialized then deserialized, these assertions would be hit. I don’t know whether that is actually an issue right now, but it does seem like a potential trap that we might stumble into later.
re: #30510 (review)
There are some places where a
BlockValidationStateis first set to invalid with a corresponding result and debug message, before then being set to error later. Presumably if such aBlockValidateStatewere serialized then deserialized, these assertions would be hit. I don’t know whether that is actually an issue right now, but it does seem like a potential trap that we might stumble into later.
I guess a way to handle this problem would be to change the corresponding CustomBuildMessage to not set the debug message when state isn’t supposed to contain a debug message. I think ideally though we would change the mining interface to avoid using the BlockValidationState type and instead use a simpler state representation as suggested #30510#pullrequestreview-2315917440
re: #30510 (review)
Added a comment to mining.capnp about getting rid of BlockValidationState here and returning something simpler from testBlockValidity
This update brings in the following changes:
https://github.com/chaincodelabs/libmultiprocess/pull/107 example: Remove manual client adding
https://github.com/chaincodelabs/libmultiprocess/pull/108 doc: Add comments for socket descriptor handling when forking
https://github.com/chaincodelabs/libmultiprocess/pull/109 example: Add missing thread.join() call so example can exit cleanly
https://github.com/chaincodelabs/libmultiprocess/pull/110 cmake: add target_capnp_sources headers target
This change is needed to allow generated capnp code in src/ipc/capnp/ to be
used in unit tests for better test coverage in upcoming commits.
Idea came from review comment by ion-
https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1757372497
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.
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>
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30364935299
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.14 -> pr/mine-types.15, compare) to fix silent conflict with #30409. Also made some improvements in response to recent comments.
Updated 3499c2ca4ba5ae70cebe84614498cc105e208f3d -> fbed31494af9acf6bd543801143a12399b2c6a1a (pr/mine-types.15 -> pr/mine-types.16, compare) fixing up some new comments.
Updated fbed31494af9acf6bd543801143a12399b2c6a1a -> 1a332817665f77f55090fa166930fec0e5500727 (pr/mine-types.16 -> pr/mine-types.17, compare) making more improvements to comments.
re-utACK fbed31494af9acf6bd543801143a12399b2c6a1a
I didn’t check if the new CustomBuildField and CustomReadField to serialize std::chrono is identical to that in #30437. If not, then I’ll test it next time I rebase https://github.com/Sjors/bitcoin/pull/48.
Thanks for the review!
I didn’t check if the new
CustomBuildFieldandCustomReadFieldto serializestd::chronois identical to that in #30437. If not, then I’ll test it next time I rebase Sjors#48.
They should be the same except the builder now has static asserts to make sure the capnp number type has enough range to represent the std::chrono duration type (that capnp Float64 type can represent MillisecondsDouble in this case).
ACK 1a332817665f77f55090fa166930fec0e5500727
Nice changes to the serialization code since my last review, re-using the existing concepts is great. I wonder if the Deserializable concept could just live in the serialize.h header. It seems like it could be generally useful?
My clang-format also wants to indent the requires expressions, but I don’t think that is necessarily more readable.
My
clang-tidyalso wants to indent therequiresexpressions, but I don’t think that is necessarily more readable.
Checking with the C++ Core Guidelines and Stroustrup’s books, while they do not mention anything about indenting require statements, they do indent it all through the guide like in this minimal example
ACK 1a332817665f77f55090fa166930fec0e5500727
Very clever rework to maximize code reuse while using already existing concepts. The code is now far easier to read. Built locally and all unit, and functional tests pass including the extended.
I wonder if the Deserializable concept could just live in the
serialize.hheader. It seems like it could be generally useful?
Definitely could be. Would be happy to move it if I need to update this again. Reason I didn’t move is after the last update is that it’s not really consistent with other concepts there, because it not used in that file, and it doesn’t take a stream parameter. But I could move it and make it more consistent.
My
clang-formatalso wants to indent therequiresexpressions, but I don’t think that is necessarily more readable.
Happy to update this too. I don’t have any strong preference and this code just came from asking chatgpt to translate previous enable_if code to use concepts.
I’m not sure, I’ll do another post-merge look.
Update: looks like I copy-pasted the wrong thing, because locally I did have 1a332817665f77f55090fa166930fec0e5500727. The range-diff compared to last week looks the same as what I saw yesterday.
Post merge re-utACK 1a332817665f77f55090fa166930fec0e5500727.