These are currently empty structs but they will be used to pass some function and object pointers from bitcoin application code to IPC hooks that run, for example, when a remote object is created or destroyed, or a new process is created.
This PR is part of the process separation project. The commit was first part of larger PR #10102.
These are currently empty structs but they will be used to pass some
function and object pointers from bitcoin application code to IPC hooks
that run, for example, when a remote object is created or destroyed, or
a new process is created.
ACK 09e3804d8fa1498264b6031f1332d5f83a99ff43 (jamesob/ackr/22218.1.ryanofsky.multiprocess_add_ipc_con)
Built and ran unittests locally. This looks like some benign plumbing for capnpnp IPC. I tried looking ahead at the parent PR (https://github.com/bitcoin/bitcoin/pull/10102/files#diff-ab470dae240c184802de8bbc39f63116f54c11e2dff50ef90421aecc50b04284) but I have to admit I can’t totally tell where or how this Context is being used. Maybe some additional details from @ryanofsky would help here. But there’s nothing obviously wrong with (or even complicated about) this changeset.
0-----BEGIN PGP SIGNED MESSAGE-----
1Hash: SHA512
2
3ACK 09e3804d8fa1498264b6031f1332d5f83a99ff43 ([`jamesob/ackr/22218.1.ryanofsky.multiprocess_add_ipc_con`](https://github.com/jamesob/bitcoin/tree/ackr/22218.1.ryanofsky.multiprocess_add_ipc_con))
4
5Built and ran unittests locally. This looks like some benign plumbing for capnpnp IPC. I tried looking ahead at the parent PR (https://github.com/bitcoin/bitcoin/pull/10102/files#diff-ab470dae240c184802de8bbc39f63116f54c11e2dff50ef90421aecc50b04284) but I have to admit I can't totally tell where this is being used. Maybe some additional details from [@ryanofsky](/bitcoin-bitcoin/contributor/ryanofsky/) would help here. But there's nothing obviously wrong with (or even complicated about) this changeset.
6-----BEGIN PGP SIGNATURE-----
7
8iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmDJF4oACgkQepNdrbLE
9TwVufw/+PvXRX6TMSZgA4QJsWBYhtCoHYiwQDYcikZkVvOSqdIWymdJr4Y/McYKp
10Awi32L5cjKAK26zYYFUYQIZyZYqYsk447CQLTYKkggsy4o9TpZwosJjz2Bq+9HUo
1112yxmeMjHruAvR2pED/eEhBGaiBJy6MJDNPuUWqY163VO/uar0ktyUHSXjxvu+Rf
12TiqKUBHo2uvtdWPphmpV+Xd/FhtJEvxmV66jNJgGWhVh+4xtWBMMfJTwOTVR/fkz
13XE7FExR92610XW4NI7DM4vF3WbEjKRYRru3YDHZVh0J39yy9z4DvJeqBH1B4WJIm
14YyITEt//f5bZE8LKM26+VROkArREvtbCHlY9GOF/XKd/7HbBlRzQfkVteEkae44h
15AXQGzjB3D/YMsm97SoU2Kj8zeoo2K4jVugnazgLdgAw4A3xZB72AGkwu54xJ+J34
16o9bwSbETo1Ay+GIwbRBrxMjTvreo+3Pk1SarINLbC30S1ya/JGiLy7bSYMFaoiaI
170GBz6VJ4dkpCFKxgs3fhOkP6PjxiZFFX/YHxidewVC2yd8NmFFjVt2Mtm3oXiIdh
18l98Q4i2hzGYpLEAF7KarSM6VWRJkYA3lMt8/MXe1ix1dO1MFTiIQki9VtrvJ5LXc
19BIvYVmmoWL39ERqtoiXjySd6as+9Bno7qx3b2myZ6u4ZWQYTKKw=
20=B8HG
21-----END PGP SIGNATURE-----
0Tested on Linux-4.19.0-10-amd64-x86_64-with-glibc2.28
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/lib/ccache/clang++ CC=/usr/lib/ccache/clang PYTHONPATH= --with-bignum=no --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion
3
4Compiled with /usr/bin/ccache /usr/lib/ccache/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_srcdir)=. -Wstack-protector -fstack-protector-all -fcf-protection=full -msse4 -msha -msse4.1 -msse4.2 i
5
6Compiler version: clang version 7.0.1-8+deb10u2 (tags/RELEASE_701/final)
This looks like some benign plumbing for capnpnp IPC. I tried looking ahead at the parent PR (#10102 (files)) but I have to admit I can’t totally tell where or how this
Contextis being used. Maybe some additional details from @ryanofsky would help here. But there’s nothing obviously wrong with (or even complicated about) this changeset.
Thanks these structs which are empty for now just get used in commit 2516d397d54b021e37d6e7628655fe7c54d2fb06 “Make bitcoin-gui spawn a bitcoin-node process” from #10102:
In that commit the context structs just get some function pointer members added so init code can hook into different multiprocess events. So for example with GUI and node code running in different processes, random number seed and logging init code has to run twice instead of once during startup (because it has to run once in each process) so ipc::Context gets an init_process callback to handle the initialization transparently in the multiprocess case, and bitcoin-qt and bitcoind code doesn’t have to change in the single process case.
6+#define BITCOIN_IPC_CONTEXT_H
7+
8+namespace ipc {
9+//! Context struct used to give IPC protocol implementations or implementation
10+//! hooks access to application state, in case they need to run extra code that
11+//! that isn't needed within a single procoess, like code copying global state
61@@ -58,6 +62,9 @@ class Ipc
62 addCleanup(typeid(Interface), &iface, std::move(cleanup));
63 }
64
65+ //! Context accessor.
66+ virtual ipc::Context& context() = 0;
I think this virtual method definition would gain to be more documented.
AFAIU, it allows a controlling process to access spawned process own IpcImpl.context() and as such trigger callback like init_process you’re pointing to in 2516d39.
So as suggestions, maybe comment could be “Allow to access spawned process context from controlling process flow” and the high-level documentation of class Ipc could be extended on point 3, like
0//! 3. The controlling process calls local proxy interfaces::Init object methods (e.g `makeChain`) to
1//! make other proxy objects calling other remote interfaces. It can access spawned process state
2//! through its ipc::Protocol::context()`
re: #22218 (review)
I think this virtual method definition would gain to be more documented.
Thanks, updated this comment. The use of the context struct is already documented in the struct definition so I just referenced the struct definition here to not repeat the same information two places. Also the purpose of the struct becomes more obvious in followup commits that add documented struct members.
AFAIU, it allows a controlling process to access spawned process own
IpcImpl.context()and as such trigger callback likeinit_processyou’re pointing to in 2516d39.So as suggestions, maybe comment could be “Allow to access spawned process context from controlling process flow” and the high-level documentation of
class Ipccould be extended on point 3, like0//! 3. The controlling process calls local proxy interfaces::Init object methods (e.g `makeChain`) to 1//! make other proxy objects calling other remote interfaces. It can access spawned process state 2//! through its ipc::Protocol::context()`
This is all true, but it is very low level information that I think is covered in comments more close to the relevant code. The point of the Context struct is just to give IPC hooks access to bitcoin application state. The bitcoin application calls IPC functions and also defines some hooks that run on IPC events (when a socket is disconnected, when a special parameter like Common.GlobalArgs passed, etc). The hooks need to access bitcoin application state. They access it through the context struct.
Thanks, updated this comment. The use of the context struct is already documented in the struct definition so I just referenced the struct definition here to not repeat the same information two places.
Well I think it’s more personal preference, but I like when the documentation on how to use an interface and its related methods are near to the code. In the present case, explain to a lambda user how to achieve the purpose of accessing application state through the context() method, even if the internal working are low-level. That said, we can reorg comment/documentation once the wider #10102 is landed and reviewers have a better grasp of the overall picture.
9+//! Context struct used to give IPC protocol implementations or implementation
10+//! hooks access to application state, in case they need to run extra code that
11+//! that isn't needed within a single procoess, like code copying global state
12+//! from an existing process to a new process when it's initialized, or code
13+//! dealing with shared objects that are created or destroyed remotely.
14+struct Context
Should we prefix this structure to avoid reviewers confusion with process specific contexts i.e NodeContext, WalletContext ?
For e.g, IpcState or LowLevelContext ?
re: #22218 (review)
Should we prefix this structure to avoid reviewers confusion with process specific contexts i.e
NodeContext,WalletContext?For e.g,
IpcStateorLowLevelContext?
It would help to know a specific example of how someone might interpret the current names and make a mistake or a wrong assumption, because I don’t understand what is less confusing about these suggestions.
I am using the word “context” in the same way in wallet, node, and IPC code. I think “context” is different than “state” because can “state” refer can refer to any type of data including temporary data, while “context” refers to longer lasting environmental data. Like data from a high-level outer environment that is is passed through low level code, but the low level code just treats as opaque. So “context” seems more appropriate than “state”. And LowLevelContext seems incorrect because the context struct is holding higher-level bitcoin-specific data that the lower-level IPC code is treats as opaque. It is high level data, not low level data.
On namespaces, I am using a namespace structure that mirrors the directory structure, so code in src/interfaces/ is in the interfaces:: namespace, code in src/ipc/ is in the ipc:: namespace, code in src/ipc/capnp/ is in the ipc::capnp:: namespace, code in src/util/ is in the util:: namespace, etc. This means when you see a reference to interfaces::Node in the code you can know where to find the definition and where it fits in the source code organization in a way that would not be possible if the class were called NodeInterface. Similarly, the struct name ipc::Context reveals a little more about that struct than the name IpcContext would.
I am using the word “context” in the same way in wallet, node, and IPC code. I think “context” is different than “state” because can “state” refer can refer to any type of data including temporary data, while “context” refers to longer lasting environmental data. Like data from a high-level outer environment that is is passed through low level code, but the low level code just treats as opaque. So “context” seems more appropriate than “state”. And LowLevelContext seems incorrect because the context struct is holding higher-level bitcoin-specific data that the lower-level IPC code is treats as opaque. It is high level data, not low level data.
Thanks for the explanation, I agree it’s servicing the same purpose than NodeContext/WalletContext, namely avoiding repeated variables/parameters declarations and proliferating usage of globals and “context” is a good pick. Prefixing would be still be better imho, like ApplicationContext/ServerContext to hint the lack of abstract/subclass relationships between current Context and NodeContext.
Concept ACK
I’m fine with the state of current PR, though feel free to take comments to make the documentation a bit better. I believe that’s yet another case where documenting well libmultiprocess (https://github.com/chaincodelabs/libmultiprocess/issues/50) would ease understanding of the chain calls between controlling processes. Also see question here : https://github.com/bitcoin/bitcoin/commit/2516d397d54b021e37d6e7628655fe7c54d2fb06#r53226160
Updated 09e3804d8fa1498264b6031f1332d5f83a99ff43 -> 3e33d170cc0a8f386791777f3cc597e2bd0bf2ee (pr/ipc-ctx.1 -> pr/ipc-ctx.2, compare) with suggested comment improvements.
Will also push an update to #10102 rebased on top of this with more comment improvements that were discussed here.
Labels
Build system
interfaces