Multiprocess bitcoin #10102
Draft
Multiprocess bitcoin #10102
+2,360
−89
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Oh. Nice. Conceptually I think this goes into the right direction, though, I'm not sure if this could end up being only a temporary in-between step that may end up being replaced. Though, I'm aware that capnp has an RPC layer. But this would introduce another API (RPC / ZMQ / REST and then capnp RPC). I'm not saying this is the wrong direction, but we should be careful about adding another API. Three questions:
|
Reason this is currently using capnp is not performance but convenience. Capnp provides a high level API that supports bidirectional, synchronous, and asynchronous calls out of the box and allows me to easily explore implementation choices in bitcoin-qt without having to worry about low level protocol details, write a lot of parameter packing/unpacking boilerplate, and implement things like long polling. Capnp could definitely be replaced by JSON-RPC, though, and I've gone out of my way to support this by not calling capnp functions or using capnp types or headers anywhere except the
It could, but I'm going out of my way right now specifically NOT to add yet another bitcoind public API that could add to the JSON-RPC/REST/ZMQ/-blocknotify/-walletnotify confusion. The IPC here doesn't happen over a TCP port or even a unix socket path but over an anonymous socketpair using an inherited file descriptor. (I haven't done a windows implementation yet but similar things are possible there). I'm trying to make the change completely internal for now and transparent to users. Bitcoin-qt should still be invoked the same way and behave the same way as before, starting its own node and wallet. It just will happen to do this internally now by forking a bitcoind executable rather than calling in-process functions. This change will not add any new command line or GUI options allowing bitcoin-qt to connect to bitcoinds other than the one it spawns internally. Adding these features and supporting new public APIs might be things we want to do in the future, but they would involve downsides and complications that I'm trying to avoid here.
It's not required here because this change doesn't expose any new socket or endpoint, but it could be supported. Capnp's security model is based on capabilities, so to add authentication, you would just define a factory function that takes credentials as parameters and returns a reference to an object exposing the appropriate functionality. |
|
I'm really uncomfortable with using capn proto, but fine enough for some example testing stuff! I'm a fan of this general approach (ignoring the use of capn proto) and I think we should have done something like it a long time ago. |
|
strong concept ACK, but if is feasible, would prefer usage of the existing RPC instead of capn'proto |
|
Concept ACK, nice.
Please, let's not turn this into a discussion of serialization and RPC frameworks. To be honest that's been one of the things that's putting me off of doing work like this. If you want to suggest what framework to use, please make a thorough investigation of what method would be best to use for our specific use case, and propose that, but let's not start throwing random "I'm not comfortable with X" comments. We already use google protocol buffers in the GUI for payment requests to in a way that would be the straightforward choice. I'm also happy you didn't choose some XML-based abomonation or ASN.1. But anyhow, not here. For this pull it's fine to use whatever RPC mechanism you're comfortable with.
I'm also perfectly fine with keeping the scope here to "communication between GUI and bitcoind". This is not the place for introducing another external interface. Might be an option at some point in the future, but for now process isolation is enough motivation. |
laanwj
reviewed
Mar 29, 2017
laanwj
reviewed
Mar 29, 2017
laanwj
reviewed
Mar 29, 2017
laanwj
reviewed
Mar 29, 2017
laanwj
reviewed
Mar 29, 2017
laanwj
reviewed
Mar 29, 2017
laanwj
reviewed
Mar 29, 2017
ryanofsky
commented
Apr 6, 2017
There was a problem hiding this comment.
Updated and rebased bf5f8ed -> 0ca73bc (pr/ipc.0 -> pr/ipc.1)
There's a lot of new changes here. More functions and callbacks have been wrapped, and there's now support for asynchronous calls that don't block event threads in the client and server.
At this point it would be very helpful to have code review for the main commit (0ca73bc "Add barebones IPC framework to bitcoin-qt and bitcoind"), because all the real infrastructure is now in place, and the main thing left to do is wrap up more functions and callbacks in IPC calls so the GUI can be functional.
|
Updated and rebased 0ca73bc -> 5e28c2f (pr/ipc.1 -> pr/ipc.3) to avoid a conflict. Main addition is an expanded src/ipc/README.md file. Again it would be very helpful to have some code review for the main commit (5e28c2f "Add barebones IPC framework to bitcoin-qt and bitcoind"). Giving feedback on the README file would be an easy place to start. |
ryanofsky
force-pushed
the
pr/ipc
branch
2 times, most recently
from
5e28c2f to
dda3756
Compare
|
Updated 5e28c2f -> dda3756 (pr/ipc.3 -> pr/ipc.4) This implements two suggestions from @JeremyRubin:
|
|
@laanwj pointed out in IRC (https://botbot.me/freenode/bitcoin-core-dev/msg/83983170/) that this change could help make the GUI more responsive by preventing Qt event processing from getting blocked, which currently happens in the monolithic At the time in IRC, I didn't think this change could directly help gui responsiveness, because although it does move libbitcoin and LOCK calls out of the However, this doesn't have to be the case. The place where IPC calls currently block waiting for responses is the But the This would add more overhead and make the average IPC call a little slower. But it would avoid situations where an unexpectedly slow IPC call ties up the whole gui, so it might be worth doing anyway. |
|
@ryanofsky Yes, integrating the IPC event loop and Qt event loop would help responsiveness. |
|
@ryanofsky I'm not familiar with Qt or capnproto, but I don't understand what the move to a different process has to do with making things less blocking. Any changes in architecture that would result in less blocks should equally be possible within the same process. |
I don't understand the goal here. On itself, there seems little benefit in separating the GUI and the rest into separate processes if those two processes still depend on each other (this is different from separating the wallet from the node, for example, as there as security considerations there... but for that use case the easiest approach seems to just have a lightweight mode and running two instances). I think it would be awesome if bitcoin-qt could be started and stopped independently to control a bitcoind process in the background, but if that's not the intent, what is the purpose? |
Let's say there are 50 places where bitcoin-qt calls a libbitcoin function. That means there are 50 places to update if you want bitcoin-qt handle to events while the function calls are executing. WIth the IPC framework, there is only one place you have to update instead of 50 places (if you want to do this).
Ok, so you think the benefits are small, and I think they are more significant.
This is trivial once bitcoin-qt is controlling bitcoind across a socket. I'm just implementing the socket part first, without introducing new UI features for now. |
Ok, that's what I was missing. It wasn't clear to me that this was a just first step towards a more useful separation. |
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Apr 20, 2017
This doesn't crash currently because the method doesn't access any object members, but this behavior is fragile and incompatible with bitcoin#10102.
This was referenced Nov 7, 2024
|
Not sure if it's worth sharing here but hey what's an 1300th comment. Running Simply compiled with Expand diff to compile on Debian stablediff --git a/CMakeLists.txt b/CMakeLists.txt
index 6ae988ac90..ca0880b3cd 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -23,6 +23,7 @@ set(CLIENT_VERSION_BUILD 0)
set(CLIENT_VERSION_RC 0)
set(CLIENT_VERSION_IS_RELEASE "false")
set(COPYRIGHT_YEAR "2024")
+set(FOUND_LIBATOMIC TRUE)
# During the enabling of the CXX and CXXOBJ languages, we modify
# CMake's compiler/linker invocation strings by appending the content
diff --git a/src/test/ipc_test.cpp b/src/test/ipc_test.cpp
index 91eba9214f..af37434980 100644
--- a/src/test/ipc_test.cpp
+++ b/src/test/ipc_test.cpp
@@ -62,7 +62,7 @@ void IpcPipeTest()
auto connection_client = std::make_unique<mp::Connection>(loop, kj::mv(pipe.ends[0]));
auto foo_client = std::make_unique<mp::ProxyClient<gen::FooInterface>>(
- connection_client->m_rpc_system.bootstrap(mp::ServerVatId().vat_id).castAs<gen::FooInterface>(),
+ connection_client->m_rpc_system->bootstrap(mp::ServerVatId().vat_id).castAs<gen::FooInterface>(),
connection_client.get(), /* destroy_connection= */ false);
foo_promise.set_value(std::move(foo_client));
disconnect_client = [&] { loop.sync([&] { connection_client.reset(); }); };Just starting The command i used to start it is With a simple hacked up client which constructs an Now if the client requested a thread before stopping, this becomes a segfault (still only when stopping Here is some logs. Logs for just starting and stopping `bitcoin-node`Logs for when the client constructs `Init`, creates a thread, and stops |
|
re: #10102 (comment)
Thanks for the report! Was able to reproduce this and created issue chaincodelabs/libmultiprocess#123 to track it. |
Needed because BlockConnected notifications are a lot slower with the wallet running in separate process.
Make default constructor more generic so it doesn't only work with void types.
Spawn node subprocess instead of running node code internally
Spawn wallet subprocess instead of running wallet code internally
Add .wallet/.gui suffixes to log files created by bitcoin-gui and bitcoin-wallet processes so they don't clash with bitcoin-node log file.
|
Rebased a295345 -> 5b9a953 ( |
ryanofsky
force-pushed
the
pr/ipc
branch
2 times, most recently
from
b5b0664 to
b57c897
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a draft PR because it is based on #29409. The non-base commits are:
392296298a03test: Increase feature_block.py and feature_taproot.py timeouts4fd334e5e3e2test: Fix multiprocess test for unclean shutdown on kill9f979fcc615autil: Add util::Result workaround to be compatible with libmultiprocess9e49448266f4multiprocess: Add capnp serialization code for bitcoin types7c4f3d367866multiprocess: Add capnp wrapper for Wallet interface1aeff1518df2multiprocess: Add capnp wrapper for Node interface6880553d09admultiprocess: Make bitcoin-gui spawn a bitcoin-node process19fd233b78dbmultiprocess: Make bitcoin-node spawn a bitcoin-wallet process7e20199b54damultiprocess: Add debug.log .wallet/.gui suffixes3ec1db0e1635doc: Multiprocess misc doc and comment updatesb57c89793ec0combine_logs: Handle multiprocess wallet log filesThis PR adds an
--enable-multiprocessconfigure option which builds newbitcoin-node,bitcoin-wallet, andbitcoin-guiexecutables with relevant functionality isolated into different processes. See doc/design/multiprocess.md for usage details and future plans.The change is implemented by adding a new
Initinterface that spawns new wallet and node subprocesses that can be controlled over a socketpair by callingNode,Wallet, andChainClientmethods. When running with split processes, you can see the IPC messages going back and forth in-debug=1output. Followup PR's #19460 and #19461 add-ipcbindand-ipcconnectoptions that allow more flexibility in how processes are connected.The IPC protocol used is Cap'n Proto, but this could be swapped out for another protocol. Cap'n Proto types and libraries are only accessed in the src/ipc/capnp/ directory, and not in any public headers or other parts of bitcoin code.
Slides from a presentation describing this change are available on google drive. Demo code used in the presentation was from an older version this PR (tag ipc.21, commits).
This PR is part of the process separation project.