doc: Add multiprocess design doc #28978
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
There was a problem hiding this comment.
Updated 66e19e7 -> f566245 (pr/ipcdoc.6 -> pr/ipcdoc.7, compare) fixing broken links and making small edits.
There was a problem hiding this comment.
Updated f566245 -> 837c53d (pr/ipcdoc.7 -> pr/ipcdoc.8, compare) adding suggested future enhancement and making a few other tweaks
There was a problem hiding this comment.
ACK 837c53d
For me this can already be merged as is but I also have a list of nits if you retouch. I have made myself familiar multiprocess a while ago but not looked at it recently. For me this was a nice refresher and I definitely think it's helpful for newcomers.
A few more ideas of what might be interesting to add, but they are not essential and may also be done in a follow-up or just be ignored:
- Could mention potential security implications (or lack thereof) of using
capnpandlibmultiprocess(at least as long as it's not maintained under the bitcoin-core org. - Similar to the usage example there could be an example of how a developer may go about changing something in the interfaces, mentioning common pitfalls etc.
- If possible maybe a visualization/diagram how the pieces of the architecture fit together could be helpful to grasp the high level quicker.
There was a problem hiding this comment.
Updated 837c53d -> fd98bee (pr/ipcdoc.8 -> pr/ipcdoc.9, compare) with suggested changes.
Updated fd98bee -> c80bd16 (pr/ipcdoc.9 -> pr/ipcdoc.10, compare) with more edits and tweaks.
re: #28978 (review)
Approach ACK. I'm still working on my understanding of multiprocess but this document was very helpful already in its current state, no major suggestions at this point.
Thank you, applied your suggestions, and improved the example flow section based on your feedback.
re: #28978 (review)
ACK 837c53d
For me this can already be merged as is but I also have a list of nits if you retouch. I have made myself familiar multiprocess a while ago but not looked at it recently. For me this was a nice refresher and I definitely think it's helpful for newcomers.
A few more ideas of what might be interesting to add, but they are not essential and may also be done in a follow-up or just be ignored:
- Could mention potential security implications (or lack thereof) of using
capnpandlibmultiprocess(at least as long as it's not maintained under the bitcoin-core org.
Great idea, added security consideration section addressing these things.
- Similar to the usage example there could be an example of how a developer may go about changing something in the interfaces, mentioning common pitfalls etc.
I think developer notes, particularly the Internal interface guidelines section will be a good place for this type of information. I added another link to it in the Interface Definition Maintenance section here. Probably the developer notes will need to have a little more information added, but that depends on more code being merged so should probably be left for future PRs.
- If possible maybe a visualization/diagram how the pieces of the architecture fit together could be helpful to grasp the high level quicker.
Yeah some visualizations would probably help. I added 3 simple diagrams, showing process/socket connections, code generation, and a call diagram, so hopefully these are helpful.
Also split up existing multiprocess documentation into design and usage sections
There was a problem hiding this comment.
Updated c80bd16 -> 91dc48c (pr/ipcdoc.10 -> pr/ipcdoc.11, compare) fixing table tag and reference heading.
I think this PR gets another ACK it could be merged to add the new documentation, and improvements could be implemented in a followup PR.
|
ACK 91dc48c |
There was a problem hiding this comment.
ACK 91dc48c - left a couple of improvements but agreed that iterating in future PRs is better.
| ``` | ||
| cd <BITCOIN_SOURCE_DIRECTORY> | ||
| make -C depends NO_QT=1 MULTIPROCESS=1 | ||
| CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure |
There was a problem hiding this comment.
nit: Perhaps useful to parameterize the host platform to highlight it's configurable? E.g.
HOST_PLATFORM="x86_64-pc-linux-gnu"
cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
CONFIG_SITE=$PWD/depends/$HOST_PLATFORM/share/config.site ./configureThere was a problem hiding this comment.
re: #28978 (comment)
nit: Perhaps useful to parameterize the host platform to highlight it's configurable? E.g.
Thanks I split it into a separate variable and clarified how it could be set in #10102.
| - In the generated code, we have C++ client subclasses that inherit from the abstract classes in [`src/interfaces/`](../../src/interfaces/). These subclasses are the workhorses of the IPC mechanism. | ||
| - They implement all the methods of the interface, marshalling arguments into a structured format, sending them as requests to the IPC server via a UNIX socket, and handling the responses. | ||
| - These subclasses effectively mask the complexity of IPC, presenting a familiar C++ interface to developers. | ||
| - Internally, the client subclasses generated by the `mpgen` tool wrap [client classes generated by Cap'n Proto](https://capnproto.org/cxxrpc.html#clients), and use them to send IPC requests. |
There was a problem hiding this comment.
Perhaps useful to elaborate on what the mpgen wrapping adds to the Cap'n Proto output?
There was a problem hiding this comment.
re: #28978 (comment)
Perhaps useful to elaborate on what the
mpgenwrapping adds to the Cap'n Proto output?
Great suggestion, done in #10102. The main difference is mpgen clients subclasses have a high-level blocking interface, while capnp client subclasses require you to do asynchronous and pass arguments and return values in capnp message format.
|
|
||
| An alternate approach could use custom [C++ Attributes](https://en.cppreference.com/w/cpp/language/attributes) embedded in interface declarations to automatically generate `.capnp` files from C++ headers. This has not been pursued because parsing C++ headers is more complicated than parsing Cap’n Proto interface definitions, especially portably on multiple platforms. | ||
|
|
||
| In the meantime, the developer guide [Internal interface guidelines](developer-notes.md#internal-interface-guidelines) can provide guidance on keeping interfaces consistent and functional and avoiding compile errors. |
There was a problem hiding this comment.
dead link
| In the meantime, the developer guide [Internal interface guidelines](developer-notes.md#internal-interface-guidelines) can provide guidance on keeping interfaces consistent and functional and avoiding compile errors. | |
| In the meantime, the developer guide [Internal interface guidelines](../developer-notes.md#internal-interface-guidelines) can provide guidance on keeping interfaces consistent and functional and avoiding compile errors. |
There was a problem hiding this comment.
| - Upon receiving the request, the Cap'n Proto dispatching code in the `bitcoin-node` process calls the `getBlockHash` method of the `Chain` [server class](#c-server-classes-in-generated-code). | ||
| - The server class is automatically generated by the `mpgen` tool from the [`chain.capnp`](https://github.com/ryanofsky/bitcoin/blob/pr/ipc/src/ipc/capnp/chain.capnp) file in [`src/ipc/capnp/`](../../src/ipc/capnp/). | ||
| - The `getBlockHash` method of the generated `Chain` server subclass in `bitcoin-wallet` receives a Cap’n Proto request object with the `height` parameter, and calls the `getBlockHash` method on its local `Chain` object with the provided `height`. | ||
| - When the call returns, it encapsulates the return value in a Cap’n Proto response, which it sends back to the `bitcoin-wallet` process, |
There was a problem hiding this comment.
| - When the call returns, it encapsulates the return value in a Cap’n Proto response, which it sends back to the `bitcoin-wallet` process, | |
| - When the call returns, it encapsulates the return value in a Cap’n Proto response, which it sends back to the `bitcoin-wallet` process. |
There was a problem hiding this comment.
|
|
||
| - **Cap’n Proto struct**: A structured data format used in Cap’n Proto, similar to structs in C++, for organizing and transporting data across different processes. | ||
|
|
||
| - **client class (in generated code)**: A C++ class generated from a Cap’n Proto interface which inherits from a Bitcoin core abstract class, and implements each virtual method to send IPC requests to another process. (see also [components section](#c-client-subclasses-in-generated-code)) |
There was a problem hiding this comment.
nit
| - **client class (in generated code)**: A C++ class generated from a Cap’n Proto interface which inherits from a Bitcoin core abstract class, and implements each virtual method to send IPC requests to another process. (see also [components section](#c-client-subclasses-in-generated-code)) | |
| - **client class (in generated code)**: A C++ class generated from a Cap’n Proto interface which inherits from a Bitcoin Core abstract class, and implements each virtual method to send IPC requests to another process. (see also [components section](#c-client-subclasses-in-generated-code)) |
There was a problem hiding this comment.
|
ACK 91dc48c |
|
Post-merge ACK. This doc is very helpful, thanks @ryanofsky! |
All suggested by stickies-v <stickies-v@protonmail.com> bitcoin#28978 (review) Co-authored-by: stickies-v <stickies-v@protonmail.com>
There was a problem hiding this comment.
re: #28978 (review)
left a couple of improvements but agreed that iterating in future PRs is better.
Thanks for the close reading and suggestions. I implemented them for now in the combined PR #10102 commit ee4b913 and will split it out into another PR at some point.
| - In the generated code, we have C++ client subclasses that inherit from the abstract classes in [`src/interfaces/`](../../src/interfaces/). These subclasses are the workhorses of the IPC mechanism. | ||
| - They implement all the methods of the interface, marshalling arguments into a structured format, sending them as requests to the IPC server via a UNIX socket, and handling the responses. | ||
| - These subclasses effectively mask the complexity of IPC, presenting a familiar C++ interface to developers. | ||
| - Internally, the client subclasses generated by the `mpgen` tool wrap [client classes generated by Cap'n Proto](https://capnproto.org/cxxrpc.html#clients), and use them to send IPC requests. |
There was a problem hiding this comment.
re: #28978 (comment)
Perhaps useful to elaborate on what the
mpgenwrapping adds to the Cap'n Proto output?
Great suggestion, done in #10102. The main difference is mpgen clients subclasses have a high-level blocking interface, while capnp client subclasses require you to do asynchronous and pass arguments and return values in capnp message format.
|
|
||
| An alternate approach could use custom [C++ Attributes](https://en.cppreference.com/w/cpp/language/attributes) embedded in interface declarations to automatically generate `.capnp` files from C++ headers. This has not been pursued because parsing C++ headers is more complicated than parsing Cap’n Proto interface definitions, especially portably on multiple platforms. | ||
|
|
||
| In the meantime, the developer guide [Internal interface guidelines](developer-notes.md#internal-interface-guidelines) can provide guidance on keeping interfaces consistent and functional and avoiding compile errors. |
There was a problem hiding this comment.
| - Upon receiving the request, the Cap'n Proto dispatching code in the `bitcoin-node` process calls the `getBlockHash` method of the `Chain` [server class](#c-server-classes-in-generated-code). | ||
| - The server class is automatically generated by the `mpgen` tool from the [`chain.capnp`](https://github.com/ryanofsky/bitcoin/blob/pr/ipc/src/ipc/capnp/chain.capnp) file in [`src/ipc/capnp/`](../../src/ipc/capnp/). | ||
| - The `getBlockHash` method of the generated `Chain` server subclass in `bitcoin-wallet` receives a Cap’n Proto request object with the `height` parameter, and calls the `getBlockHash` method on its local `Chain` object with the provided `height`. | ||
| - When the call returns, it encapsulates the return value in a Cap’n Proto response, which it sends back to the `bitcoin-wallet` process, |
There was a problem hiding this comment.
|
|
||
| - **Cap’n Proto struct**: A structured data format used in Cap’n Proto, similar to structs in C++, for organizing and transporting data across different processes. | ||
|
|
||
| - **client class (in generated code)**: A C++ class generated from a Cap’n Proto interface which inherits from a Bitcoin core abstract class, and implements each virtual method to send IPC requests to another process. (see also [components section](#c-client-subclasses-in-generated-code)) |
There was a problem hiding this comment.
| ``` | ||
| cd <BITCOIN_SOURCE_DIRECTORY> | ||
| make -C depends NO_QT=1 MULTIPROCESS=1 | ||
| CONFIG_SITE=$PWD/depends/x86_64-pc-linux-gnu/share/config.site ./configure |
There was a problem hiding this comment.
re: #28978 (comment)
nit: Perhaps useful to parameterize the host platform to highlight it's configurable? E.g.
Thanks I split it into a separate variable and clarified how it could be set in #10102.
All suggested by stickies-v <stickies-v@protonmail.com> bitcoin#28978 (review) Co-authored-by: stickies-v <stickies-v@protonmail.com>
All suggested by stickies-v <stickies-v@protonmail.com> bitcoin#28978 (review) Co-authored-by: stickies-v <stickies-v@protonmail.com>
All suggested by stickies-v <stickies-v@protonmail.com> bitcoin#28978 (review) Co-authored-by: stickies-v <stickies-v@protonmail.com>
Add multiprocess design doc and existing multiprocess documentation into design and usage sections.
Links to rendered markdown:
https://github.com/ryanofsky/bitcoin/blob/pr/ipcdoc/doc/design/multiprocess.md
https://github.com/ryanofsky/bitcoin/blob/pr/ipcdoc/doc/multiprocess.md
This PR is part of the process separation project.