multiprocess: Add ipc::Context and ipc::capnp::Context structs #22218

ryanofsky · 2021-06-10T22:13:11Z

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.

jamesob

ACK 09e3804 (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.

Show signature data

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 09e3804d8fa1498264b6031f1332d5f83a99ff43 ([`jamesob/ackr/22218.1.ryanofsky.multiprocess_add_ipc_con`](https://github.com/jamesob/bitcoin/tree/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 this 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.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmDJF4oACgkQepNdrbLE
TwVufw/+PvXRX6TMSZgA4QJsWBYhtCoHYiwQDYcikZkVvOSqdIWymdJr4Y/McYKp
Awi32L5cjKAK26zYYFUYQIZyZYqYsk447CQLTYKkggsy4o9TpZwosJjz2Bq+9HUo
12yxmeMjHruAvR2pED/eEhBGaiBJy6MJDNPuUWqY163VO/uar0ktyUHSXjxvu+Rf
TiqKUBHo2uvtdWPphmpV+Xd/FhtJEvxmV66jNJgGWhVh+4xtWBMMfJTwOTVR/fkz
XE7FExR92610XW4NI7DM4vF3WbEjKRYRru3YDHZVh0J39yy9z4DvJeqBH1B4WJIm
YyITEt//f5bZE8LKM26+VROkArREvtbCHlY9GOF/XKd/7HbBlRzQfkVteEkae44h
AXQGzjB3D/YMsm97SoU2Kj8zeoo2K4jVugnazgLdgAw4A3xZB72AGkwu54xJ+J34
o9bwSbETo1Ay+GIwbRBrxMjTvreo+3Pk1SarINLbC30S1ya/JGiLy7bSYMFaoiaI
0GBz6VJ4dkpCFKxgs3fhOkP6PjxiZFFX/YHxidewVC2yd8NmFFjVt2Mtm3oXiIdh
l98Q4i2hzGYpLEAF7KarSM6VWRJkYA3lMt8/MXe1ix1dO1MFTiIQki9VtrvJ5LXc
BIvYVmmoWL39ERqtoiXjySd6as+9Bno7qx3b2myZ6u4ZWQYTKKw=
=B8HG
-----END PGP SIGNATURE-----

Show platform data

Tested on Linux-4.19.0-10-amd64-x86_64-with-glibc2.28

Configured 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

Compiled 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

Compiler version: clang version 7.0.1-8+deb10u2 (tags/RELEASE_701/final)

ryanofsky · 2021-06-15T21:52:31Z

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

Thanks these structs which are empty for now just get used in commit 2516d39 "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.

ariard

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 (bitcoin-core/libmultiprocess#50) would ease understanding of the chain calls between controlling processes. Also see question here : 2516d39#r53226160

ariard · 2021-07-08T14:04:48Z

src/ipc/context.h

+namespace ipc {
+//! Context struct used to give IPC protocol implementations or implementation
+//! hooks access to application state, in case they need to run extra code that
+//! that isn't needed within a single procoess, like code copying global state


nit: duplicated that

re: #22218 (comment)

nit: duplicated that

Thanks, fixed

ariard · 2021-07-08T15:11:15Z

src/interfaces/ipc.h

@@ -58,6 +62,9 @@ class Ipc
        addCleanup(typeid(Interface), &iface, std::move(cleanup));
    }

+    //! Context accessor.
+    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

//! 3. The controlling process calls local proxy interfaces::Init object methods (e.g `makeChain`) to //! make other proxy objects calling other remote interfaces. It can access spawned process state //! through its ipc::Protocol::context()`

re: #22218 (comment)

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

//! 3. The controlling process calls local proxy interfaces::Init object methods (e.g `makeChain`) to //! make other proxy objects calling other remote interfaces. It can access spawned process state //! 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.

ariard · 2021-07-08T15:42:22Z

src/ipc/context.h

+//! that isn't needed within a single procoess, like code copying global state
+//! from an existing process to a new process when it's initialized, or code
+//! dealing with shared objects that are created or destroyed remotely.
+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 (comment)

Should we prefix this structure to avoid reviewers confusion with process specific contexts i.e NodeContext, WalletContext ?

For e.g, IpcState or LowLevelContext ?

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.

ryanofsky

Updated 09e3804 -> 3e33d17 (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.

ryanofsky · 2021-07-09T15:55:20Z

src/interfaces/ipc.h

@@ -58,6 +62,9 @@ class Ipc
        addCleanup(typeid(Interface), &iface, std::move(cleanup));
    }

+    //! Context accessor.
+    virtual ipc::Context& context() = 0;


re: #22218 (comment)

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

//! 3. The controlling process calls local proxy interfaces::Init object methods (e.g `makeChain`) to //! make other proxy objects calling other remote interfaces. It can access spawned process state //! 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.

ryanofsky · 2021-07-09T15:55:32Z

src/ipc/context.h

+namespace ipc {
+//! Context struct used to give IPC protocol implementations or implementation
+//! hooks access to application state, in case they need to run extra code that
+//! that isn't needed within a single procoess, like code copying global state


re: #22218 (comment)

nit: duplicated that

Thanks, fixed

ryanofsky · 2021-07-09T16:13:13Z

src/ipc/context.h

+//! that isn't needed within a single procoess, like code copying global state
+//! from an existing process to a new process when it's initialized, or code
+//! dealing with shared objects that are created or destroyed remotely.
+struct Context


re: #22218 (comment)

Should we prefix this structure to avoid reviewers confusion with process specific contexts i.e NodeContext, WalletContext ?

For e.g, IpcState or LowLevelContext ?

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.

ariard · 2021-07-12T14:26:16Z

Code Review ACK 3e33d17

Thanks for taking the comment improvements suggestions, either here or on #10102

ryanofsky · 2021-07-21T19:09:20Z

Possible this could be ready for merge. It just has two ACKs, but it's only adding two empty structs and a method to access them

…ontext …

DrahtBot added Build system interfaces labels Jun 10, 2021

jamesob approved these changes Jun 15, 2021

View reviewed changes

ryanofsky mentioned this pull request Jun 15, 2021

Multiprocess bitcoin #10102

Draft

ariard reviewed Jul 8, 2021

View reviewed changes

ryanofsky force-pushed the pr/ipc-ctx branch from 09e3804 to 3e33d17 Compare July 9, 2021 17:07

ryanofsky commented Jul 9, 2021

View reviewed changes

maflcko merged commit ba15ab4 into bitcoin:master Jul 22, 2021

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 23, 2021

Merge bitcoin#22218: multiprocess: Add ipc::Context and ipc::capnp::C…

81003d0

…ontext …

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multiprocess: Add ipc::Context and ipc::capnp::Context structs #22218

multiprocess: Add ipc::Context and ipc::capnp::Context structs #22218

ryanofsky commented Jun 10, 2021

jamesob left a comment

ryanofsky commented Jun 15, 2021

ariard left a comment

ariard Jul 8, 2021

ryanofsky Jul 9, 2021

ariard Jul 8, 2021

ryanofsky Jul 9, 2021

ariard Jul 12, 2021

ariard Jul 8, 2021

ryanofsky Jul 9, 2021

ariard Jul 12, 2021

ryanofsky left a comment

ryanofsky Jul 9, 2021

ryanofsky Jul 9, 2021

ryanofsky Jul 9, 2021

ariard commented Jul 12, 2021 •

edited

Loading

ryanofsky commented Jul 21, 2021

multiprocess: Add ipc::Context and ipc::capnp::Context structs #22218

multiprocess: Add ipc::Context and ipc::capnp::Context structs #22218

Conversation

ryanofsky commented Jun 10, 2021

jamesob left a comment

Choose a reason for hiding this comment

ryanofsky commented Jun 15, 2021

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard commented Jul 12, 2021 • edited Loading

ryanofsky commented Jul 21, 2021

ariard commented Jul 12, 2021 •

edited

Loading