This PR adds an --enable-multiprocess configure option which builds new bitcoin-node, bitcoin-wallet, and bitcoin-gui executables 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 Init interface that spawns new wallet and node subprocesses that can be controlled over a socketpair by calling Node, Wallet, and ChainClient methods. When running with split processes, you can see the IPC messages going back and forth in -debug=1 output. Followup PR's #19460 and #19461 add -ipcbind and -ipcconnect options 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).
jonasschnelli
commented at 7:38 AM on March 28, 2017:
contributor
Oh. Nice.
I expected much more code to achieve this.
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.
Because, it may be more effective to split the Qt/d part completely and let them communicate over the p2p protocol (SPV and eventually RPC). More effective because it would also allow to run Qt independent from a trusted full node (if not trusted, use mechanism like full block SPV, etc.).
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:
Would the performance be impractical if we would try to use the existing RPC API?
Could the capnp approach (or lets say IPC approach) be designed as a (or the) new API ("JSON RPC v2" and replacement for ZMQ)?
Does capnp provide a basic form of authentication? Would that even be required?
jonasschnelli added the label GUI on Mar 28, 2017
jonasschnelli added the label RPC/REST/ZMQ on Mar 28, 2017
ryanofsky
commented at 9:58 AM on March 28, 2017:
contributor
Would the performance be impractical if we would try to use the existing RPC API?
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 ipc/server.cpp and ipc/client.cpp files. No code outside of these two files has to change in order to move to a different protocol.
Could the capnp approach (or lets say IPC approach) be designed as a (or the) new API ("JSON RPC v2" and replacement for ZMQ)?
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.aspx) 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.
Does capnp provide a basic form of authentication? Would that even be required?
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.
gmaxwell
commented at 5:23 PM on March 28, 2017:
contributor
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.
dcousens
commented at 10:39 PM on March 28, 2017:
contributor
strong concept ACK, but if is feasible, would prefer usage of the existing RPC instead of capn'proto
laanwj
commented at 6:53 AM on March 29, 2017:
member
Concept ACK, nice.
I'm really uncomfortable with using capn proto, but fine enough for some example testing stuff!
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.
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.
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.
in
src/qt/walletmodel.cpp:712
in
bf5f8ed6f0outdated
Yep I guess most of these calls should be turned into async calls and not wait on a response synchronously blocking the GUI. Not necessarily in the first iteration of this, of course.
ryanofsky
commented at 10:35 AM on March 29, 2017:
Yep I guess most of these calls should be turned into async calls and not wait on a response synchronously blocking the GUI. Not necessarily in the first iteration of this, of course.
Another alternative in some of these cases is to consolidate many low level calls into fewer calls of a higher level interface.
I think that's not optional but a required element of making this asynchronous, otherwise there'd be a lot of roundtrips.
Edit: Though ofcourse one of the things cap'n'proto advertises with is that there is 'zero roundtrip overhead', because of the promise pipelining, but we don't want to depend too strongly on that.
in
src/ipc/messages.capnp:13
in
bf5f8ed6f0outdated
It can also expose multiple instances of one class?
Yes, the ipc::Node::wallet() method right now returns an ipc::Wallet interface wrapping pwalletMain, but it could support multiwallet by just adding an argument that indicates a different wallet object to return.
Should it pass through parameters? Most of the parameters to bitcoin-qt will actually be for the daemon. Or will you provide parameters in a later stage through IPC?
Should it pass through parameters? Most of the parameters to bitcoin-qt will actually be for the daemon. Or will you provide parameters in a later stage through IPC?
The change I'm working on now (not yet pushed) provides the parameters over IPC. It adds an ipc::Node::parseParameters method in client.h which calls ParseParameters() in bitcoind.
ParseParameters() is also called on the bitcoin-qt side in current change. This gets the job done, but could be improved later, since really the bitcoind global variables set by ParseParameters() should not even be linked into bitcoin-qt.
The thread group is completely remote in the case of IPC, isn't it? I guess this entire function should be done differently when IPC is used. E.g. send a shutdown command to the core, then have the shutdownResult event come from remote as response when done.
ryanofsky
commented at 10:02 AM on March 29, 2017:
The thread group is completely remote in the case of IPC, isn't it?
Yes the change I'm working on now adds threadGroup and scheduler members to NodeServer in ipc/server.cpp, removing the current instances in BitcoinCore.
in
src/qt/bitcoin.cpp:685
in
bf5f8ed6f0outdated
681 | @@ -676,7 +682,7 @@ int main(int argc, char *argv[])
682 | app.createOptionsModel(IsArgSet("-resetguisettings"));
683 |
684 | // Subscribe to global signals from core
685 | - uiInterface.InitMessage.connect(InitMessage); 686 | + FIXME_IMPLEMENT_IPC_VALUE(uiInterface).InitMessage.connect(InitMessage);
How would you handle uiInterface in this model, e.g. signals from the server to the client?
ryanofsky
commented at 10:16 AM on March 29, 2017:
How would you handle uiInterface in this model, e.g. signals from the server to the client?
This line needs to change to ipcNode.handleInitMessage(InitMessage). If you look at the ipc::Node::handleInitMessage implementation, it takes a std::function argument, creates a capnp InitMessageCallback::Server object that can invoke it, and sends a reference to that object over the IPC channel to bitcoind. bitcoind then calls uiInterface.InitMessage.connect with a handler that sends messages with the InitMessageCallback object.
So the server then effectively calls an object on the client when the notification happens? No polling/waiting involved? That's great.
Yes, though to be clear, there is still polling/waiting happening under the hood. It just gets handled by the capnp event loop, which waits for incoming IPC messages and dispatches to InitMessageCallback::Server::call and other server methods. The change I'm working on now (not yet pushed) spawns a new thread in ipc/client.cpp to run the event loop.
Yes, I understand that, but one of the problems with existing proposals for using RPC to communicate to the server had no way to handle asynchonous notifications, so needed to e.g. poll for new transactions every few seconds. This protocol clearly does support true bidirectional communication.
As for the capnp event loop, as most of the responses and notifications from the server involve updating the UI, couldn't we integrate that into Qt's event loop? Conceptually that'd be easier. Qt only allows GUI updates from a single thread, so if the capnp event loop is separete, everything will have to be separately ferried through Qt's signal mechanism to get to the GUI thread.
As for the capnp event loop, as most of the responses and notifications from the server involve updating the UI, couldn't we integrate that into Qt's event loop?
This locking (either of cs_main or wallet->cs_wallet) makes no sense when a remote core is used.
Yeah, the point of the FIXME_IMPLEMENT_IPC macro is really just to segfault and indicate places in the code which need to be updated to support IPC. Many of them are pretty nonsensical.
It's a bit of a shame that argument parsing doesn't work here yet so this needs to use manual parsing using C functions :/
I think I could change this to at least use GetArg like you are doing in your cloudabi code.
ryanofsky force-pushed on Apr 6, 2017
ryanofsky
commented at 11:55 PM on April 6, 2017:
contributor
Updated and rebased bf5f8ed6f0124d65af0198b473cb25513fe84325 -> 0ca73bc13c3457cd5c3244abfa9fa586d9137117 (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 (0ca73bc13c3457cd5c3244abfa9fa586d9137117 "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.
ryanofsky force-pushed on Apr 7, 2017
ryanofsky
commented at 8:51 PM on April 7, 2017:
contributor
Updated and rebased 0ca73bc13c3457cd5c3244abfa9fa586d9137117 -> 5e28c2fcc2757479d29ca83cd3256584ab908e48 (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 (5e28c2fcc2757479d29ca83cd3256584ab908e48 "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 on Apr 10, 2017
ryanofsky force-pushed on Apr 10, 2017
ryanofsky
commented at 10:25 PM on April 10, 2017:
contributor
This implements two suggestions from @JeremyRubin:
It includes a small commit demonstrating what it looks like to add a single new method to the API:
dda3756 Add ipc::Node::getNodeCount method. This should help give a clearer picture of the layers involved in implementing an IPC call.
Instead of adding Cap'n Proto code and modifying Qt code in a single commit, it includes a new early commit (1407a2b Add ipc::Node and ipc::Wallet interfaces that introduces new src/ipc/interfaces.h and src/ipc/interfaces.cpp files and ports Qt code to use them without any Cap'n Proto stuff. This shows the separation between Qt updates and IPC implementation details better and makes it easier to see how a different IPC system could be substituted in for Cap'n Proto. This commit could even be made into a separate PR.
ryanofsky
commented at 5:44 AM on April 14, 2017:
contributor
@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 bitcoin-qt when the main GUI thread makes a call to a slow libbitcoin function, or waits a long time for a cs_main or cs_wallet lock.
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 bitcoin-qt process and into the bitcoind process, it just replaces these calls with blocking IPCs that make the GUI equally unresponsive when they tie up the main GUI thread.
But the std::promise object used in that line could easily be replaced with a Qt-aware promise object that processes GUI events while the promise is blocked. (The Qt-aware promise implementation would check if it is being used on the main GUI thread, and if so use a local Qt event loop substituting
loop.exec() for std::future::get() and loop.quit() for std::promise::set_value().)
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.
laanwj
commented at 8:00 AM on April 14, 2017:
member
@ryanofsky Yes, integrating the IPC event loop and Qt event loop would help responsiveness.
Though I remember there were some issues in some cases with recursively calling into the Qt event loop (e.g. things need to be reentrant, deleteLater stuff runs earlier than expected, to keep in mind).
sipa
commented at 12:23 PM on April 17, 2017:
member
@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.
sipa
commented at 12:29 PM on April 17, 2017:
member
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.
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?
ryanofsky
commented at 2:15 PM on April 17, 2017:
contributor
Any changes in architecture that would result in less blocks should equally be possible within the same process.
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).
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.
Ok, so you think the benefits are small, and I think they are more significant.
I think it would be awesome if bitcoin-qt could be started and stopped independently to control a bitcoind process in the background,
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.
sipa
commented at 6:48 PM on April 17, 2017:
member
I think it would be awesome if bitcoin-qt could be started and stopped independently to control a bitcoind process in the background,
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 referenced this in commit 7ba55396a0 on Apr 20, 2017
ryanofsky force-pushed on Apr 20, 2017
ryanofsky force-pushed on Apr 20, 2017
ryanofsky referenced this in commit fb463d1717 on Apr 20, 2017
ryanofsky force-pushed on Apr 27, 2017
ryanofsky
commented at 6:01 PM on April 27, 2017:
contributor
As of 8f78f085976bcb0f9093f0b1b4c3c65110ec44aa (pr/ipc.7), this change is much more complete & functional. You can also now monitor the IPC traffic going back and forth between bitcoin-qt and bitcoind by setting the IPC_DEBUG environment variable (export IPC_DEBUG=1)
ryanofsky force-pushed on Apr 27, 2017
practicalswift referenced this in commit baf971cc8c on Apr 27, 2017
ryanofsky referenced this in commit c59ffa1968 on Apr 28, 2017
ryanofsky force-pushed on Apr 28, 2017
ryanofsky force-pushed on Apr 28, 2017
ryanofsky referenced this in commit 7e9ee9193f on May 4, 2017
ryanofsky referenced this in commit 6135fc0948 on May 5, 2017
ryanofsky referenced this in commit 333426d65d on May 9, 2017
ryanofsky force-pushed on May 9, 2017
ryanofsky referenced this in commit d944bd7a27 on May 17, 2017
ryanofsky force-pushed on May 17, 2017
ryanofsky force-pushed on May 23, 2017
ryanofsky force-pushed on May 24, 2017
ryanofsky force-pushed on May 25, 2017
ryanofsky renamed this: bitcoin-qt: spawn bitcoind and communicate over pipe (Experimental, WIP) bitcoin-qt: spawn bitcoind and communicate over pipe (Experimental, WIP, Depends on #10244) on May 31, 2017
ryanofsky force-pushed on Jun 2, 2017
ryanofsky force-pushed on Jun 2, 2017
luke-jr referenced this in commit 651d189f51 on Jun 15, 2017
ryanofsky force-pushed on Jun 19, 2017
ryanofsky force-pushed on Aug 25, 2017
ryanofsky force-pushed on Aug 25, 2017
ryanofsky force-pushed on Sep 21, 2017
ryanofsky force-pushed on Feb 14, 2018
ryanofsky renamed this: bitcoin-qt: spawn bitcoind and communicate over pipe (Experimental, WIP, Depends on #10244) [experimental] Multiprocess bitcoin on Feb 14, 2018
ryanofsky force-pushed on Feb 14, 2018
ryanofsky force-pushed on Feb 14, 2018
ryanofsky force-pushed on Feb 23, 2018
ryanofsky force-pushed on Feb 26, 2018
SmarterHomes referenced this in commit 0950e8ac40 on Mar 2, 2018
ryanofsky force-pushed on Mar 2, 2018
SmarterHomes referenced this in commit 7f09072241 on Mar 3, 2018
ryanofsky force-pushed on Mar 7, 2018
ryanofsky force-pushed on Mar 7, 2018
ryanofsky force-pushed on Mar 12, 2018
ryanofsky force-pushed on Mar 14, 2018
ryanofsky force-pushed on Mar 23, 2018
laanwj referenced this in commit 5f0c6a7b0e on Apr 5, 2018
ryanofsky force-pushed on Apr 6, 2018
ryanofsky force-pushed on Apr 7, 2018
ryanofsky force-pushed on Apr 9, 2018
ryanofsky force-pushed on Apr 10, 2018
ryanofsky force-pushed on Apr 10, 2018
ryanofsky force-pushed on Apr 11, 2018
ryanofsky force-pushed on Apr 17, 2018
ryanofsky force-pushed on Apr 17, 2018
maflcko added the label Needs rebase on Jun 6, 2018
ryanofsky force-pushed on Jul 18, 2018
DrahtBot removed the label Needs rebase on Jul 18, 2018
DrahtBot added the label Needs rebase on Jul 20, 2018
ryanofsky force-pushed on Aug 3, 2018
DrahtBot removed the label Needs rebase on Aug 3, 2018
DrahtBot added the label Needs rebase on Aug 6, 2018
ryanofsky force-pushed on Aug 7, 2018
ryanofsky force-pushed on Aug 7, 2018
DrahtBot removed the label Needs rebase on Aug 7, 2018
ryanofsky force-pushed on Aug 8, 2018
DrahtBot added the label Needs rebase on Aug 9, 2018
ryanofsky force-pushed on Aug 9, 2018
DrahtBot removed the label Needs rebase on Aug 9, 2018
in
doc/build-unix.md:108
in
006286e731outdated
104 | @@ -105,7 +105,7 @@ To build without GUI pass `--without-gui`.
105 |
106 | To build with Qt 5 you need the following:
107 |
108 | - sudo apt-get install libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools libprotobuf-dev protobuf-compiler 109 | + sudo apt-get install libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools libprotobuf-dev protobuf-compiler libcapnp-dev capnproto
Sjors
commented at 1:10 PM on August 11, 2018:
member
Concept ACK, ultimately being able to leave bitcoin-node running while starting the GUI on demand, while not having to wait for sync, would be quite nice. We could even have it run in bandwidth conserving mode if the GUI isn't on, but that's all for future PRs.
Should capnp be added to depends (in a future PR) as well?
DrahtBot added the label Needs rebase on Aug 13, 2018
ryanofsky force-pushed on Aug 14, 2018
DrahtBot removed the label Needs rebase on Aug 14, 2018
ryanofsky force-pushed on Aug 21, 2018
ryanofsky
commented at 7:33 PM on August 21, 2018:
contributor
Updated 85b23296891032875cbda7a3c70c3422ce04da15 -> 84af92e496f00608dff749e7b1372964cb20df42 (pr/ipc.46 -> pr/ipc.47) with various cleanups and fixes for macos.
Updated 84af92e496f00608dff749e7b1372964cb20df42 -> ca294aa8c036bf62d808450a4555a0806ff7227b (pr/ipc.47 -> pr/ipc.48) with fixes for appveyor / msvc.
Thanks for trying this out Sjors. The compiler errors & warnings you encountered should be fixed now, and I did some light ui testing on macos with src/bitcoin-gui -regtest -printtoconsole -debug
ryanofsky force-pushed on Aug 22, 2018
DrahtBot added the label Needs rebase on Aug 23, 2018
ryanofsky force-pushed on Aug 23, 2018
DrahtBot removed the label Needs rebase on Aug 23, 2018
DrahtBot added the label Needs rebase on Aug 25, 2018
Sjors
commented at 10:26 AM on August 26, 2018:
member
Compile errors are gone now.
The multiprocess=yes/no line in ./configure only showed up for me after running ./autogen.sh again. This was without using the --enable-multiprocess argument.
The documentation suggests multiprocess isn't on by default, but it is:
./configure --disable-bench --disable-zmq --with-miniupnpc=no --with-incompatible-bdb --with-qrencode
....
Options used to compile and link:
multiprocess = yes
I do still see a few warnings that appear related to this change:
interfaces/capnp/util.cpp:15:90: warning: 'syscall' is deprecated: first deprecated in macOS 10.12 - syscall(2) is unsupported; please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost(). [-Wdeprecated-declarations]
return strprintf("%s-%i/%s-%i", exe_name ? exe_name : "", getpid(), thread_name, int(syscall(SYS_gettid)));
^
/usr/include/unistd.h:745:6: note: 'syscall' has been explicitly marked deprecated here
int syscall(int, ...);
^
1 warning generated.
Should the test suite use these new binaries? In order to prevent the functional suite from taking forever to run, perhaps half of the default test suite nodes could use bitcoind and the other half bitcoin-node?
The bitcoin-gui executable is ignoring testnet=1 in bitcoin.conf. bitcoin-node does honor it. When launched with -testnet it correctly sees global options in bitcoin.conf., but it missed or incorrectly parses options for [test]. E.g. if I set dbcache=1000 under [test] the GUI shows this:
<img width="383" alt="schermafbeelding 2018-08-26 om 12 07 44" src="https://user-images.githubusercontent.com/10217/44627135-d741cd80-a928-11e8-9c1a-c73151ab8da9.png">
Should the log file specify which process is generating each entry? Or should each process have its own log file, where the master process just logs "started/stopped bitcoin-node process PID=...."?
I did some light GUI testing as well. Sending and receiving works. Using the console I'm getting the following error:
<img width="594" alt="schermafbeelding 2018-08-26 om 12 17 01" src="https://user-images.githubusercontent.com/10217/44627206-060c7380-a92a-11e8-9cfe-84842cff0f66.png">
getblockchaininfo and even getbalance do work, so that's weird.
createwallet "test" froze the app for me, and didn't create a file. When force quitting QT, it seems bitcoin-node keeps running. But then bitcoin-cli help just hangs. bitcoin-cli stop responds with Bitcoin server stopping but nothing happens in the log file and the process doesn't go away; I had to resort to kill -9.
ryanofsky
commented at 2:50 PM on August 27, 2018:
contributor
@Sjors, this is great! Thanks so much for testing this.
The documentation suggests multiprocess isn't on by default, but it is:
Since --enable-multiprocess configure option only builds new executables and has no effect on existing ones, the default value is auto rather than no. This seems like the most convenient behavior, but I coudl change it or try to document it better.
I do still see a few warnings that appear related to this change:
I also saw the syscall warning and plan to fix it. It's not serious problem, but a side effect seems to be that bad thread numbers are shown in the debug log.
Should the test suite use these new binaries? In order to prevent the functional suite from taking forever to run, perhaps half of the default test suite nodes could use bitcoind and the other half bitcoin-node?
I'm planning on adding --multiprocess and --gui options to the python test framework to make it easier to switch between bitcoin, bitcoin-qt, bitcoin-node, and bitcoin-gui executables. All 4 of these should work, though some tests fail with bitcoin-node right now. I think default test mode should use bitcoind but travis should be configured to test other binaries, too.
The bitcoin-gui executable is ignoring testnet=1 in bitcoin.conf. bitcoin-node does honor it. When launched with -testnet it correctly sees global options in bitcoin.conf., but it missed or incorrectly parses options for [test].
Interesting, this is not expected. Will debug.
Should the log file specify which process is generating each entry? Or should each process have its own log file, where the master process just logs "started/stopped bitcoin-node process PID=...."?
Each process has its own log file and I extended the combine_logs.py script to be able to merge them. The log files just have simple suffixes right now so are named debug.logdebug.log.wallet and debug.log.gui. The followup change I'm working on to add -ipconnect and -ipcbind options will allow multiple wallet and gui processes, so these names will no longer be unique and I'll have to add pid suffixes as well.
Writing to a common log file would be another option, but it would require some kind of locking and syncing, so I don't think the complexity would be worth it.
I did some light GUI testing as well.
Thanks! I'll look into the various problems you reported and make fixes.
Sjors
commented at 11:00 AM on August 28, 2018:
member
I think default test mode should use bitcoind but travis should be configured to test other binaries, too.
That makes sense, we shouldn't force developers / users to compile the other binaries.
Writing to a common log file would be another option, but it would require some kind of locking and syncing, so I don't think the complexity would be worth it.
Multiple log files seems fine by me, though when logging to console they would ideally be combined.
ryanofsky force-pushed on Aug 29, 2018
DrahtBot removed the label Needs rebase on Aug 29, 2018
DrahtBot added the label Needs rebase on Aug 30, 2018
ryanofsky force-pushed on Aug 30, 2018
DrahtBot removed the label Needs rebase on Aug 30, 2018
DrahtBot added the label Needs rebase on Aug 31, 2018
ryanofsky force-pushed on Aug 31, 2018
DrahtBot removed the label Needs rebase on Aug 31, 2018
in
src/interfaces/README.md:17
in
2e8bee7b47outdated
14 | * [`Handler`](handler.h) — returned by `handleEvent` methods on interfaces above and used to manage lifetimes of event handlers.
15 |
16 | -* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102). 17 | +* [`Init`](init.h) — used by multiprocess code to access other interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102). 18 | + 19 | +* [`Base`](base.h) — base interface class used by multiprocess code for bookeeping and cleanup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).
practicalswift
commented at 6:27 PM on September 2, 2018:
Typo found by codespell: bookeeping
ryanofsky
commented at 2:58 PM on September 4, 2018:
Fixed
in
src/interfaces/capnp/init.cpp:44
in
2e8bee7b47outdated
39 | +{ 40 | +public: 41 | + ShutdownLoop(EventLoop& loop) : m_loop(loop) {} 42 | + void onClose(Base& interface, bool remote) override 43 | + { 44 | + // FIXME: Shutdown can cause segfault right now because close/destuction
practicalswift
commented at 6:28 PM on September 2, 2018:
Typo found by codespell: destuction
ryanofsky
commented at 3:00 PM on September 4, 2018:
Fixed
in
src/interfaces/capnp/init.cpp:46
in
2e8bee7b47outdated
41 | + ShutdownLoop(EventLoop& loop) : m_loop(loop) {} 42 | + void onClose(Base& interface, bool remote) override 43 | + { 44 | + // FIXME: Shutdown can cause segfault right now because close/destuction 45 | + // seuqnce doesn't wait for server objects across the pipe to shut down, 46 | + // so e.g. things like handlers dont get a chance to get deleted in
practicalswift
commented at 6:29 PM on September 2, 2018:
Typo found by codespell: “dont” should be “don’t” or “do not” :-)
ryanofsky
commented at 3:00 PM on September 4, 2018:
Fixed
in
src/interfaces/capnp/util.h:71
in
2e8bee7b47outdated
66 | +struct TypeList 67 | +{ 68 | + static constexpr size_t size = sizeof...(Types); 69 | +}; 70 | + 71 | +//! Split TypeList into two halfs at position index.
practicalswift
commented at 6:30 PM on September 2, 2018:
Typo found by codespell: halfs should be halves :-)
ryanofsky
commented at 3:18 PM on September 4, 2018:
Fixed
in
src/interfaces/capnp/FIXME:6
in
2e8bee7b47outdated
0 | @@ -0,0 +1,61 @@ 1 | +- [ ] BA rename X.name to X.client or (X.impl/X.classMethod/X.callsMethod/X.wraps/X.serverName) and change X.name modifier to affect both client & server declarations instead of just server to remove ChainNotifications client overloads 2 | +- [ ] BB Get rid of FunctionTraits::Fields, ProxyMethodTraits::Fields move to ClientInvoke 3 | +- [ ] BC unify readfield forms. can have single ReadDest class with ReadDest::Return typedef, with ReadField taking ReadDest, RestDestArg... arguments and returning ReadDest::Construct(constructor arg, ReadDestArg...) and Return type either being real destination type or a proxy that emplaces into a vector, or a proxy that updates an existing variable 4 | + - could have ReadDest::Update(lambda) where lambda takes reference argument to object, this allows reading & updating for object types that can't be initialized by constructor. e.g. maps, sets, structs without unserializing constructors 5 | + - designgoal is just to have one ReadField per type, not confusing mix of readfieldupdate/emplace and maze of overloads to fall back to one when another isn't available 6 | + - other goal is to support return types. however i think this may be only useful for client side return values. server side value initialization in PassField could potentially use it to get rid of boost::optional, but this relies of type having move / copy constructor, even if it would be elided, and no guarantee of this for paramters
practicalswift
commented at 6:00 PM on September 3, 2018:
Typo found by codespell: paramters
ryanofsky
commented at 2:59 PM on September 4, 2018:
Fixed
in
src/interfaces/capnp/proxy-impl.h:779
in
2e8bee7b47outdated
practicalswift
commented at 7:38 AM on September 4, 2018:
Couldn't loop_ptr potentially be a dead pointer here?
ryanofsky
commented at 3:20 PM on September 4, 2018:
Couldn't loop_ptr potentially be a dead pointer here?
Good catch. It could happen if socket was immediately disconnected. Moved onDisconnect handler down to fix this.
ryanofsky force-pushed on Sep 4, 2018
ryanofsky
commented at 4:21 PM on September 4, 2018:
contributor
Using the console I'm getting the following error:
Rpc help error is fixed now. It was caused by signrawtransaction throwing a wallet not found exception during the help request because no wallets are loaded in the node process.
Rebased ca294aa8c036bf62d808450a4555a0806ff7227b -> dd9e4f6909a6aca0cf4b7c16a277b5099b2011cc (pr/ipc.48 -> pr/ipc.49)
Rebased dd9e4f6909a6aca0cf4b7c16a277b5099b2011cc -> 5c345a6fdbebd7579795f3798b65704fd2441987 (pr/ipc.49 -> pr/ipc.50)
Rebased 5c345a6fdbebd7579795f3798b65704fd2441987 -> 6bd29a0fc805c223b4f000f999db15bf0b327881 (pr/ipc.50 -> pr/ipc.51)
Rebased 6bd29a0fc805c223b4f000f999db15bf0b327881 -> 2e8bee7b4761a3e61e5266b51d6f7298e2b1bad3 (pr/ipc.51 -> pr/ipc.52)
Updated 2e8bee7b4761a3e61e5266b51d6f7298e2b1bad3 -> b65d9eca6e1d365fc63542b6961cfe89188bb083 (pr/ipc.52 -> pr/ipc.53) with travis link fix, and fixes for practicalswift comments.
Updated b65d9eca6e1d365fc63542b6961cfe89188bb083 -> 8d3df50476fa2f96cf2910238e5a2b3f8380b6e1 (pr/ipc.53 -> pr/ipc.54) to fix appveyor link error.
ryanofsky force-pushed on Sep 4, 2018
ryanofsky force-pushed on Sep 5, 2018
ryanofsky
commented at 2:53 PM on September 5, 2018:
contributor
The bitcoin-gui executable is ignoring testnet=1 in bitcoin.conf
This is fixed now. It was caused by a bad rebase of this PR, which neglected to call SetupServerArgs in the gui process after it was introduced in #13190.
leishman
commented at 8:38 PM on September 5, 2018:
chainParams unused?
ryanofsky
commented at 8:57 PM on September 5, 2018:
chainParams unused?
Will fix, but this is part of the base PR #10973 (cb0c6b42a096980152a467b98f3c1250f62d4e7e), so it would be better to comment on that PR.
leishman
commented at 9:01 PM on September 5, 2018:
Ah sorry. Will leave any new comments there.
in
src/rpc/rawtransaction.cpp:801
in
5ce9a948c8outdated
803 | for (const CTxIn& txin : mtx.vin) {
804 | - view.AccessCoin(txin.prevout); // Load entries from viewChain into view; can fail. 805 | + outputs.emplace_back(txin.prevout); 806 | + } 807 | + std::vector<Coin> coins = chain.findCoins(outputs); 808 | + for (int i = 0; i < coins.size(); ++i) {
leishman
commented at 8:42 PM on September 5, 2018:
perhaps change int to size_t to prevent comparison of integers with different sign warning?
ryanofsky
commented at 8:58 PM on September 5, 2018:
perhaps change int to size_t to prevent comparison of integers with different sign warning?
Will fix, but this change is part of the base PR #10973 (249bf5006184f81d77d40ee0ede0924c628bf33e), so it would be better to comment on that PR.
ryanofsky
commented at 6:57 PM on September 6, 2018:
contributor
createwallet "test" froze the app for me
This is fixed in pr/ipc.56
DrahtBot added the label Needs rebase on Sep 7, 2018
ryanofsky force-pushed on Sep 14, 2018
DrahtBot removed the label Needs rebase on Sep 14, 2018
DrahtBot added the label Needs rebase on Sep 15, 2018
fingera
commented at 7:30 AM on September 21, 2018:
contributor
help
gcc 7.3.0:
CXX interfaces/capnp/libbitcoin_util_a-messages.o
In file included from ../../src/interfaces/capnp/proxy.h:6:0,
from ../../src/interfaces/capnp/messages.h:5,
from ./interfaces/capnp/messages.capnp.proxy.h:6,
from ../../src/interfaces/capnp/messages-impl.h:9,
from ../../src/interfaces/capnp/messages.cpp:1:
../../src/interfaces/capnp/proxy.h: In instantiation of ‘struct interfaces::capnp::ListOutput<capnp::List<interfaces::capnp::messages::Pair<capnp::Text, capnp::List<capnp::Text> > > >’:
../../src/interfaces/capnp/proxy-impl.h:945:13: required from ‘void interfaces::capnp::BuildField(interfaces::capnp::TypeList<std::map<K, V, std::less<_Key>, std::allocator<std::pair<const _Key, _Tp> > > >, interfaces::capnp::Priority<1>, interfaces::capnp::InvokeContext&, Value&&, Output&&) [with KeyLocalType = std::__cxx11::basic_string<char>; ValueLocalType = std::vector<std::__cxx11::basic_string<char> >; Value = const std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >&; Output = interfaces::capnp::StructField<interfaces::capnp::Accessor<interfaces::capnp::FieldOverrideArgs, 19>, interfaces::capnp::messages::GlobalArgs::Builder>&]’
../../src/interfaces/capnp/proxy-impl.h:1060:15: required from ‘void interfaces::capnp::BuildField(interfaces::capnp::TypeList<const LocalType>, interfaces::capnp::Priority<0>, interfaces::capnp::InvokeContext&, Value&&, Output&&) [with LocalType = std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >; Value = const std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >&; Output = interfaces::capnp::StructField<interfaces::capnp::Accessor<interfaces::capnp::FieldOverrideArgs, 19>, interfaces::capnp::messages::GlobalArgs::Builder>&]’
../../src/interfaces/capnp/proxy-impl.h:1071:15: required from ‘void interfaces::capnp::BuildField(interfaces::capnp::TypeList<LocalType&>, interfaces::capnp::Priority<0>, interfaces::capnp::InvokeContext&, Value&&, Output&&, void*) [with LocalType = const std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >; Value = const std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >&; Output = interfaces::capnp::StructField<interfaces::capnp::Accessor<interfaces::capnp::FieldOverrideArgs, 19>, interfaces::capnp::messages::GlobalArgs::Builder>&]’
../../src/interfaces/capnp/proxy-impl.h:1098:15: required from ‘void interfaces::capnp::BuildOne(interfaces::capnp::TypeList<ValueLocalType>, interfaces::capnp::InvokeContext&, Value&&, Output&&, typename std::enable_if<(index < interfaces::capnp::ProxyType<LocalType>::fields)>::type*) [with long unsigned int index = 0; LocalType = interfaces::capnp::GlobalArgs; Value = const interfaces::capnp::GlobalArgs&; Output = interfaces::capnp::messages::GlobalArgs::Builder&; typename std::enable_if<(index < interfaces::capnp::ProxyType<LocalType>::fields)>::type = void]’
../../src/interfaces/capnp/proxy-impl.h:1119:16: required from ‘void interfaces::capnp::BuildField(interfaces::capnp::TypeList<LocalType>, interfaces::capnp::Priority<1>, interfaces::capnp::InvokeContext&, Value&&, Output&&, typename interfaces::capnp::ProxyType<LocalType>::Struct*) [with LocalType = interfaces::capnp::GlobalArgs; Value = const interfaces::capnp::GlobalArgs&; Output = interfaces::capnp::ValueField<interfaces::capnp::messages::GlobalArgs::Builder>; typename interfaces::capnp::ProxyType<LocalType>::Struct = interfaces::capnp::messages::GlobalArgs]’
../../src/interfaces/capnp/messages.cpp:52:109: required from here
../../src/interfaces/capnp/proxy.h:322:110: error: ‘B’ is not a class or namespace
template<typename B = Builder, typename Arg> auto set(Arg&& arg) const -> AUTO_RETURN(this->m_builder.B::set(m_index, std::forward<Arg>(arg)))
ryanofsky force-pushed on Sep 26, 2018
ryanofsky
commented at 8:56 PM on September 26, 2018:
contributor
Thanks for the report @fingera. The gcc compile errors should be fixed now (I've been testing with clang more than gcc recently).
Updated 5ce9a948c8ec47762d0a23d1cf7f2ce57386052b -> f896f063007aa3889caf9d14724357117eca78c1 (pr/ipc.55 -> pr/ipc.56) to fix createwallet and other wallet RPCs that use g_rpc_interfaces global
Rebased f896f063007aa3889caf9d14724357117eca78c1 -> c1cbca095f1bf10ca4370bfe99b798a5d1e9f268 (pr/ipc.56 -> pr/ipc.57)
Rebased c1cbca095f1bf10ca4370bfe99b798a5d1e9f268 -> 382cfabf523cd209adeb3ed9c5fd5d69fea284ab (pr/ipc.57 -> pr/ipc.58) with compile fixes for gcc
Rebased 382cfabf523cd209adeb3ed9c5fd5d69fea284ab -> ededfe8800fd2647851d4d42dd765cfcd8d69cde (pr/ipc.58 -> pr/ipc.59) due to conflict with #12246
Rebased ededfe8800fd2647851d4d42dd765cfcd8d69cde -> ee425e2ddff453a5436cffb1e16e5452d8e3892a (pr/ipc.59 -> pr/ipc.60) on top of base PR tag pr/wipc-sep.85
Rebased ee425e2ddff453a5436cffb1e16e5452d8e3892a -> 10ad449bb7c6ea3c2fea9f9a32e54e960c5acfd5 (pr/ipc.60 -> pr/ipc.61) on top of base PR tag pr/wipc-sep.87
Rebased 10ad449bb7c6ea3c2fea9f9a32e54e960c5acfd5 -> f2233bcfef4147e0268da581f797b1646b0d8447 (pr/ipc.61 -> pr/ipc.62) on top of base PR tag pr/wipc-sep.88
Rebased f2233bcfef4147e0268da581f797b1646b0d8447 -> da8719d39614e2673047721f9a7ea8e96587b000 (pr/ipc.62 -> pr/ipc.63) on top of base PR tag pr/wipc-sep.89
Rebased da8719d39614e2673047721f9a7ea8e96587b000 -> c5bc654324670631d32e530c4cf1dbf9af58841a (pr/ipc.63 -> pr/ipc.64) on top of base PR tag pr/wipc-sep.93 and fixing conflicts with #15101, #15114, and others
Rebased c5bc654324670631d32e530c4cf1dbf9af58841a -> cc23f7434c21e195609e4dff6b8839864a426715 (pr/ipc.64 -> pr/ipc.65) on top of base PR tag pr/wipc-sep.95 and fixing conflicts with #15153
Rebased cc23f7434c21e195609e4dff6b8839864a426715 -> c5dadccc5da6247975ee3884687a680bc46a9327 (pr/ipc.65 -> pr/ipc.66) on top of base PR tag pr/wipc-sep.96 after #15288 merge
Rebased c5dadccc5da6247975ee3884687a680bc46a9327 -> 191e240ce66208eb17ca743a6928cb20aa56041e (pr/ipc.66 -> pr/ipc.67) on top of base PR tag pr/wipc-sep.97 fixing conflict with #15531
Rebased 191e240ce66208eb17ca743a6928cb20aa56041e -> 3440513a45e94a5f1f0c67ab7409439afbbef673 (pr/ipc.67 -> pr/ipc.68) after #10973 merge
DrahtBot removed the label Needs rebase on Sep 26, 2018
DrahtBot added the label Needs rebase on Sep 26, 2018
ryanofsky force-pushed on Sep 27, 2018
DrahtBot removed the label Needs rebase on Sep 27, 2018
DrahtBot
commented at 9:14 PM on September 27, 2018:
contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)
#25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
LLM Linter (✨ experimental)
Possible typos and grammar issues:
File suffix to append for log files.. -> File suffix to append for log files. [Extra . makes the sentence look typo’d and slightly hurts readability.]
<sup>2026-03-31 22:18:17</sup>
Sjors
commented at 10:29 AM on September 28, 2018:
member
After upgrading to Mojave I'm having compile issues (as of ededfe8800fd2647851d4d42dd765cfcd8d69cde):
In file included from interfaces/capnp/proxy-codegen.cpp:4:
In file included from /usr/local/Cellar/capnp/0.7.0/include/capnp/blob.h:28:
/usr/local/Cellar/capnp/0.7.0/include/kj/common.h:36:4: error: "This code requires C++14. Either your compiler does not support it or it is not enabled."
#error "This code requires C++14. Either your compiler does not support it or it is not enabled."
^
/usr/local/Cellar/capnp/0.7.0/include/kj/common.h:39:6: error: "Pass -std=c++14 on the compiler command line to enable C++14."
#error "Pass -std=c++14 on the compiler command line to enable C++14."
^
In file included from interfaces/capnp/proxy-codegen.cpp:5:
In file included from /usr/local/Cellar/capnp/0.7.0/include/capnp/schema-parser.h:30:
In file included from /usr/local/Cellar/capnp/0.7.0/include/kj/filesystem.h:28:
/usr/local/Cellar/capnp/0.7.0/include/kj/function.h:264:3: error: 'auto' return without trailing return type; deduced return types are a C++14 extension
auto operator()(Params&&... params) {
^
/usr/local/Cellar/capnp/0.7.0/include/kj/function.h:268:3: error: 'auto' return without trailing return type; deduced return types are a C++14 extension
auto operator()(Params&&... params) const {
^
interfaces/capnp/proxy-codegen.cpp:10:10: fatal error: 'interfaces/capnp/proxy.capnp.h' file not found
DrahtBot added the label Needs rebase on Oct 8, 2018
Sjors
commented at 7:16 AM on October 9, 2018:
member
Compile works again for me. I do still get the syscall deprecated in macOS 10.12 warning I mentioned earlier. Some of the tests also throw a compiler warning.
rpc help, and testnet=1 in bitcoin.conf, command override in settings screen work for me now.
createwallet in the gui console crashes the gui. The node and wallet keep running, but won't shut down using bitcoin-cli stop.
Don't forget to update .gitignore. Nit: should bitcoin-gui be in src/qt ?
ryanofsky
commented at 7:54 AM on October 9, 2018:
contributor
Thanks for testing! I will test the createwallet method, fix git ignores, and clean up the disconnect handling so leftover processes won't stick around if one crashes.
On location of bitcoin-gui, all three executables are intentionally built in the same directory to make spawning processes simple. When bitcoin-gui launches bitcoin-node, it always looks for a bitcoin-node executable in the same directory. Same when bitcoin-node runs bitcoin-wallet. Trying to build and call executables in different directories would make things unnecessarily complicated, especially if the executables will eventually be installed in the same directory anyway.
ryanofsky force-pushed on Nov 1, 2018
ryanofsky
commented at 9:14 PM on November 1, 2018:
contributor
Didn't look in much detail, but these are warnings about signed/unsigned comparisons from checks like: BOOST_CHECK_EQUAL(reader.size(), 6);. Could be fixed by changing 6 to 6u, but I think these warnings must predate this PR.
re: #10102 (comment) shutdown/hang issues. I'm planning to look into these next.
DrahtBot removed the label Needs rebase on Nov 1, 2018
ismail120572
commented at 12:00 AM on November 2, 2018:
none
practicalswift
commented at 3:55 PM on November 2, 2018:
exe_path is not a parameter? :-)
in
src/wallet/wallet.cpp:2635
in
ee425e2ddfoutdated
2631 | @@ -2604,7 +2632,7 @@ bool CWallet::CreateTransaction(const std::vector<CRecipient>& vecSend, CTransac
2632 | // enough, that fee sniping isn't a problem yet, but by implementing a fix
2633 | // now we ensure code won't be written that makes assumptions about
2634 | // nLockTime that preclude a fix later.
2635 | - txNew.nLockTime = chainActive.Height();2636 | + txNew.nLockTime = locked_chain.getHeight().value_or(LOCKTIME_MAX);
practicalswift
commented at 3:56 PM on November 2, 2018:
Make this implicit conversion explicit since it changes signedness?
in
src/interfaces/node.cpp:55
in
ee425e2ddfoutdated
No, it's a overridden method called through the base class by the kj library.
in
src/init.h:55
in
ee425e2ddfoutdated
51 | @@ -42,7 +52,7 @@ bool AppInitParameterInteraction();
52 | * @note This can be done before daemonization. Do not call Shutdown() if this function fails.
53 | * @pre Parameters should be parsed and config file should be read, AppInitParameterInteraction should have been called.
54 | */
55 | -bool AppInitSanityChecks(); 56 | +bool AppInitSanityChecks(bool lock_data_dir=false);
practicalswift
commented at 4:07 PM on November 2, 2018:
Missing spaces around =
in
src/interfaces/base.cpp:1
in
ee425e2ddfoutdated
practicalswift
commented at 4:09 PM on November 2, 2018:
Missing whitespace before {.
in
src/interfaces/capnp/proxy-impl.h:252
in
ee425e2ddfoutdated
247 | + // all if the current thread is a remote thread created for a different 248 | + // IPC client, because in that case PassField code (below) will have set 249 | + // remote_thread to point to the calling thread. 250 | + auto request = invoke_context.loop.m_thread_map.makeThreadRequest(); 251 | + request.setName(invoke_context.client_thread.waiter->m_name); 252 | + remote_thread = request.send().getResult(); // Nonblocking due to capnp request piplineing.
practicalswift
commented at 4:10 PM on November 2, 2018:
Typo: pipelining
in
src/wallet/wallet.cpp:1724
in
ee425e2ddfoutdated
1768 | - if (pindex && fAbortRescan) {1769 | - WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", pindex->nHeight, progress_current);1770 | - } else if (pindex && ShutdownRequested()) {1771 | - WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", pindex->nHeight, progress_current);1772 | + if (!block_hash.IsNull() && fAbortRescan) {1773 | + WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", *block_height, progress_current);
practicalswift
commented at 9:23 PM on November 4, 2018:
The optional block_height could be uninitialized here?
in
src/wallet/wallet.cpp:1726
in
ee425e2ddfoutdated
1770 | - } else if (pindex && ShutdownRequested()) {1771 | - WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", pindex->nHeight, progress_current);1772 | + if (!block_hash.IsNull() && fAbortRescan) {1773 | + WalletLogPrintf("Rescan aborted at block %d. Progress=%f\n", *block_height, progress_current);1774 | + } else if (!block_hash.IsNull() && ShutdownRequested()) {1775 | + WalletLogPrintf("Rescan interrupted by shutdown request at block %d. Progress=%f\n", *block_height, progress_current);
practicalswift
commented at 9:24 PM on November 4, 2018:
Same here?
DrahtBot added the label Needs rebase on Nov 5, 2018
ryanofsky force-pushed on Nov 12, 2018
DrahtBot removed the label Needs rebase on Nov 12, 2018
DrahtBot added the label Needs rebase on Nov 13, 2018
ryanofsky force-pushed on Nov 26, 2018
DrahtBot removed the label Needs rebase on Nov 26, 2018
DrahtBot added the label Needs rebase on Nov 30, 2018
ryanofsky force-pushed on Dec 6, 2018
ryanofsky force-pushed on Jan 23, 2019
DrahtBot removed the label Needs rebase on Jan 23, 2019
in
src/interfaces/capnp/proxy-impl.h:387
in
c5bc654324outdated
practicalswift
commented at 8:47 AM on January 25, 2019:
Are arg1 and arg2 guaranteed to be valid here? Could have been moved from on the call on the line before?
meshcollider referenced this in commit 72ca72e637 on Jan 30, 2019
DrahtBot added the label Needs rebase on Jan 30, 2019
ryanofsky force-pushed on Feb 13, 2019
DrahtBot removed the label Needs rebase on Feb 13, 2019
DrahtBot added the label Needs rebase on Feb 15, 2019
maflcko referenced this in commit 45f434f44d on Mar 4, 2019
ryanofsky force-pushed on Mar 4, 2019
DrahtBot removed the label Needs rebase on Mar 4, 2019
ryanofsky force-pushed on Mar 5, 2019
DrahtBot added the label Needs rebase on Mar 6, 2019
meshcollider referenced this in commit 2607d960a0 on Mar 21, 2019
ryanofsky force-pushed on Mar 21, 2019
DrahtBot removed the label Needs rebase on Mar 21, 2019
DrahtBot added the label Needs rebase on Mar 27, 2019
ryanofsky force-pushed on May 24, 2019
DrahtBot removed the label Needs rebase on May 24, 2019
ryanofsky force-pushed on May 24, 2019
ryanofsky force-pushed on May 24, 2019
jb55
commented at 1:09 AM on May 26, 2019:
contributor
codegen seems broken for me: (gcc 7.4.0). I get random errors of these kind:
err1
CXX wallet/libbitcoin_wallet_tool_a-wallettool.o
GEN interfaces/capnp/test/foo.capnp.c++
*** Uncaught exception ***
kj/filesystem.c++:315: failed: expected parts.size() > 0; can't use ".." to break out of starting directory
stack: 7ffbcf3c71a0 7ffbcf3c9829 4098e3 7ffbcf55904d 7ffbcf559082 7ffbcf53208f 7ffbcf534296 7ffbcf5510a4 7ffbcf558893 7ffbcf558901 7ffbcf558c57 7ffbcf532974 7ffbcf532724 7ffbcf533921 7ffbcf5399fc 7ffbcf542aa5 7ffbcf5436d6 7ffbcf555452 7ffbcf55662a 7ffbcf55679f 7ffbcf55a606 7ffbcf55aa53 412466 412770 7ffbcf3e4c28 7ffbcf3e5d1a
make[2]: *** [Makefile:17337: interfaces/capnp/test/foo.capnp.c++] Error 1
make[2]: *** Waiting for unfinished jobs....
Generated test/data/base58_encode_decode.json.h
Generated test/data/key_io_invalid.json.h
Generated test/data/blockfilters.json.h
Generated test/data/key_io_valid.json.h
In file included from interfaces/capnp/config_bitcoin-node.cpp:1:0:
./interfaces/capnp/config.h:4:10: fatal error: interfaces/capnp/node.capnp.h: No such file or directory
#include <interfaces/capnp/node.capnp.h>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [Makefile:12585: interfaces/capnp/bitcoin_node-config_bitcoin-node.o] Error 1
In file included from interfaces/capnp/config_bitcoin-wallet.cpp:1:0:
./interfaces/capnp/config.h:4:10: fatal error: interfaces/capnp/node.capnp.h: No such file or directory
#include <interfaces/capnp/node.capnp.h>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
err 2
CXX interfaces/capnp/proxy_codegen-proxy-codegen.o
In file included from /nix/store/69fkv7ql25r8j496bbchnsvbp42izv8q-capnproto-0.7.0/include/capnp/blob.h:28:0,
from interfaces/capnp/proxy-codegen.cpp:4:
/nix/store/69fkv7ql25r8j496bbchnsvbp42izv8q-capnproto-0.7.0/include/kj/common.h:36:4: error: #error "This code requires C++14. Either your compiler does not support it or it is not enabled."
#error "This code requires C++14. Either your compiler does not support it or it is not enabled."
^~~~~
/nix/store/69fkv7ql25r8j496bbchnsvbp42izv8q-capnproto-0.7.0/include/kj/common.h:39:6: error: #error "Pass -std=c++14 on the compiler command line to enable C++14."
#error "Pass -std=c++14 on the compiler command line to enable C++14."
^~~~~
interfaces/capnp/proxy-codegen.cpp:10:10: fatal error: interfaces/capnp/proxy.capnp.h: No such file or directory
#include <interfaces/capnp/proxy.capnp.h>
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ryanofsky
commented at 7:41 AM on May 27, 2019:
contributor
ryanofsky
commented at 2:15 PM on May 28, 2019:
contributor
re: #10102 (comment)@jb55 Will try to update this PR shortly, but I posted build fixes for capnp 0.7.0 in 792963e32ceb081971935cf1aa10f88f9d3f3a1a (ipc-work.276)
ryanofsky force-pushed on Jun 1, 2019
ryanofsky force-pushed on Jun 1, 2019
ryanofsky force-pushed on Jun 1, 2019
ryanofsky force-pushed on Jun 1, 2019
DrahtBot added the label Needs rebase on Jun 6, 2019
ryanofsky force-pushed on Jun 8, 2019
DrahtBot removed the label Needs rebase on Jun 8, 2019
DrahtBot added the label Needs rebase on Jun 18, 2019
ryanofsky force-pushed on Jun 26, 2019
ryanofsky force-pushed on Jun 26, 2019
DrahtBot removed the label Needs rebase on Jun 26, 2019
DrahtBot added the label Needs rebase on Jun 27, 2019
ryanofsky force-pushed on Jun 27, 2019
DrahtBot removed the label Needs rebase on Jun 27, 2019
DrahtBot added the label Needs rebase on Jun 29, 2019
ryanofsky referenced this in commit 25140b7dcc on Jul 10, 2019
ryanofsky force-pushed on Jul 10, 2019
DrahtBot removed the label Needs rebase on Jul 10, 2019
ryanofsky referenced this in commit 8fd0743716 on Jul 12, 2019
ryanofsky force-pushed on Jul 12, 2019
ryanofsky referenced this in commit 724d2f7fe3 on Jul 15, 2019
ryanofsky referenced this in commit 45785e7f19 on Jul 15, 2019
ryanofsky referenced this in commit 5233494f3f on Jul 15, 2019
ryanofsky force-pushed on Jul 15, 2019
ryanofsky force-pushed on Jul 17, 2019
DrahtBot added the label Needs rebase on Jul 24, 2019
ryanofsky referenced this in commit c685fa9bed on Aug 6, 2019
ryanofsky referenced this in commit 8125688f6c on Aug 6, 2019
ryanofsky referenced this in commit f324c66615 on Aug 6, 2019
ryanofsky referenced this in commit dee0711538 on Aug 6, 2019
ryanofsky referenced this in commit 1938bdebc6 on Aug 6, 2019
ryanofsky force-pushed on Aug 6, 2019
ryanofsky force-pushed on Aug 6, 2019
ryanofsky referenced this in commit ffa6c808f8 on Aug 6, 2019
ryanofsky force-pushed on Aug 6, 2019
DrahtBot removed the label Needs rebase on Aug 6, 2019
DrahtBot added the label Needs rebase on Aug 12, 2019
ryanofsky force-pushed on Aug 20, 2019
DrahtBot removed the label Needs rebase on Aug 20, 2019
ryanofsky referenced this in commit 6208f3e7bd on Aug 20, 2019
DrahtBot added the label Needs rebase on Aug 23, 2019
ryanofsky referenced this in commit 7fbe1cac83 on Aug 28, 2019
ryanofsky force-pushed on Aug 28, 2019
DrahtBot removed the label Needs rebase on Aug 28, 2019
DrahtBot added the label Needs rebase on Aug 29, 2019
ryanofsky referenced this in commit fdc72aed96 on Aug 29, 2019
ryanofsky force-pushed on Aug 29, 2019
DrahtBot removed the label Needs rebase on Aug 29, 2019
ryanofsky referenced this in commit 94730885da on Aug 30, 2019
ryanofsky force-pushed on Aug 30, 2019
ryanofsky force-pushed on Aug 30, 2019
ryanofsky force-pushed on Aug 30, 2019
ryanofsky force-pushed on Aug 30, 2019
DrahtBot added the label Needs rebase on Sep 7, 2019
ryanofsky referenced this in commit 124bbcabe3 on Sep 9, 2019
ryanofsky referenced this in commit 7225ebebdf on Sep 10, 2019
ryanofsky referenced this in commit a3dc53304a on Sep 10, 2019
ryanofsky referenced this in commit 7b65a5356e on Sep 18, 2019
ryanofsky referenced this in commit 858a3f90fc on Sep 18, 2019
ryanofsky force-pushed on Sep 18, 2019
DrahtBot removed the label Needs rebase on Sep 18, 2019
ryanofsky force-pushed on Sep 19, 2019
DrahtBot added the label Needs rebase on Oct 2, 2019
laanwj referenced this in commit 471e5f8829 on Oct 30, 2019
ryanofsky referenced this in commit 616658f1cf on Jan 8, 2020
ryanofsky force-pushed on Jan 8, 2020
ryanofsky force-pushed on Jan 9, 2020
ryanofsky force-pushed on Jan 9, 2020
DrahtBot removed the label Needs rebase on Jan 9, 2020
ryanofsky referenced this in commit 2b3d6bf0e4 on Jan 10, 2020
ryanofsky force-pushed on Jan 10, 2020
jgarzik
commented at 9:30 PM on January 10, 2020:
contributor
concept ACK
practicalswift
commented at 9:39 AM on January 12, 2020:
contributor
Very strong concept ACK!
Has this PR reached a level where it could be assigned a milestone? :)
DrahtBot added the label Needs rebase on Jan 13, 2020
ryanofsky referenced this in commit 26cde384af on Jan 16, 2020
ryanofsky referenced this in commit 17bd1544b5 on Jan 24, 2020
ryanofsky referenced this in commit 30a4088a9a on Jan 24, 2020
ryanofsky force-pushed on Jan 24, 2020
DrahtBot removed the label Needs rebase on Jan 24, 2020
DrahtBot added the label Needs rebase on Jan 29, 2020
ryanofsky referenced this in commit 689424a679 on Feb 10, 2020
ryanofsky referenced this in commit e94785a54f on Feb 25, 2020
ryanofsky referenced this in commit a56950c54a on Feb 25, 2020
ryanofsky force-pushed on Feb 25, 2020
ryanofsky referenced this in commit 360fd9be3c on Feb 25, 2020
DrahtBot removed the label Needs rebase on Feb 25, 2020
ryanofsky referenced this in commit 96cb597325 on Feb 26, 2020
Sjors
commented at 1:27 PM on February 28, 2020:
member
Can you clarify if the stuff in src/interfaces/capnp/*.{h,cpp} is automatically generated or manual? If automatic, I would prefer to leave it out of the repo and have make produce them. Other than those files, I like how this PR is looking a lot leaner than it used to. The first 5 non-base commits can be separate PRs, leaving only 3.
Separating libmultiprocess into its own project also seems like a good call, even though it's quite small. I think we can kick any bike shedding about capnproto alternatives down the road while this feature is optional and marked experimental.
ryanofsky
commented at 7:08 PM on March 2, 2020:
contributor
Can you clarify if the stuff in src/interfaces/capnp/*.{h,cpp} is automatically generated or manual?
This is manual code, but it needs to be better documented and have boilerplate removed. It's all dealing with lifetime and serialization issues for specific classes and structs that were difficult to share between processes for one reason or another. There's a lot of boilerplate because I just dealt with corner cases individually as they came up, and refactoring should make it smaller.
The code in src/interfaces/capnp/*.{h,cpp} is the least stable part of this PR, but it isn't really where the core functionality is implemented. That's more in the src/interfaces/*.{h,cpp}, particularly in the new Init, LocalInit, IpcProcess, and IpcProtocol classes
DrahtBot added the label Needs rebase on Mar 5, 2020
hebasto
commented at 9:35 AM on March 5, 2020:
member
Concept ACK.
hebasto
commented at 11:26 AM on March 5, 2020:
member
Testing on Linux Mint 19.3:
an attempt to shutdown bitcoin-node using Ctrl+C causes an error:
$ ./src/bitcoin-node -testnet
...
2020-03-05T11:21:53Z [init] Shutdown: In progress...
2020-03-05T11:21:53Z [addcon] addcon thread exit
terminate called after throwing an instance of 'std::logic_error'
what(): clientInvoke call made after disconnect
Aborted (core dumped)
No such error during bitcoin-gui shutdown.
ryanofsky referenced this in commit 6aad158ce1 on Mar 5, 2020
ryanofsky force-pushed on Mar 5, 2020
ryanofsky
commented at 5:33 PM on March 5, 2020:
contributor
an attempt to shutdown bitcoin-node using Ctrl+C causes an error:
I can add a "Known Issues" section to the doc and mention this. This PR isn't adding signal handlers to spawned bitcoin-node and bitcoin-wallet processes so ctrl-c can kill them immediately and trigger sudden disconnects and IPC errors. I think adding simple signal handlers would be a good thing to do in a followup PR, and also in general recovering from sudden IPC disconnects is something that should be improved later.
In the meantime clean shutdowns with bitcoin-cli stop should work here if new code is working properly
Rebased 20ee129209ccf9f6e84893fe27a4dee99e7b5d5c -> 8c7f94f9dbc9c257afd7986068f0eeca7ee038a1 (pr/ipc.87 -> pr/ipc.88) on ipc-build.8
Rebased 8c7f94f9dbc9c257afd7986068f0eeca7ee038a1 -> 805addc1c2d06131c19675ceb342b7a91ddfebaa (pr/ipc.88 -> pr/ipc.89) on ipc-build.9 with unique_ptr build fixes
Rebased 805addc1c2d06131c19675ceb342b7a91ddfebaa -> 406278a8b8e68b79e3366629a52acb87ee671500 (pr/ipc.89 -> pr/ipc.90) on ipc-build.10 with chain wallet include fix
Updated 406278a8b8e68b79e3366629a52acb87ee671500 -> 908619d8b1f3490a06b3248bc0acd847818f24ed (pr/ipc.90 -> pr/ipc.91, compare) with make distdir fix
Updated 908619d8b1f3490a06b3248bc0acd847818f24ed -> 665348c7c54c934e247e18bfcf0f5b5435cc11d3 (pr/ipc.91 -> pr/ipc.92, compare) with disable wallet compile fix
Updated 665348c7c54c934e247e18bfcf0f5b5435cc11d3 -> adbf158a4c02d0dd8f488feae343c39f83e85383 (pr/ipc.92 -> pr/ipc.93, compare) with another disable wallet fix
Rebased adbf158a4c02d0dd8f488feae343c39f83e85383 -> 51c895d0169fc4c18a662b8cfc2b7526b2211d29 (pr/ipc.93 -> pr/ipc.94) on ipc-build.11
Updated 51c895d0169fc4c18a662b8cfc2b7526b2211d29 -> b6066d2b491fb32563b9b69a82f4736b68df9cc7 (pr/ipc.94 -> pr/ipc.95, compare) with travis/appveyor build fixes
Rebased b6066d2b491fb32563b9b69a82f4736b68df9cc7 -> d22e3f28d58f2ffd6df1eeaad4625eac673b7b0b (pr/ipc.95 -> pr/ipc.96) on pr/ipc-build.12 splitting commits and fixing various rebase conflicts
Squashed d22e3f28d58f2ffd6df1eeaad4625eac673b7b0b -> 4a0f0fd6a1a3d38cf5115d21ff0242964f0bba09 (pr/ipc.96 -> pr/ipc.97, compare) fixing commit order
Rebased 4a0f0fd6a1a3d38cf5115d21ff0242964f0bba09 -> bdd677c32ff38987fa71ae848b6565637c97550d (pr/ipc.97 -> pr/ipc.98) due to conflict with #16963
Rebased bdd677c32ff38987fa71ae848b6565637c97550d -> 2017cca9cff0b0c5bb7a59921ec7b5aece63ac8c (pr/ipc.98 -> pr/ipc.99) with travis/appveyor fixes on top of pr/ipc-count.1, pr/ipc-txup.1
Rebased 2017cca9cff0b0c5bb7a59921ec7b5aece63ac8c -> 004a611351a2a5bf81a4cfb706d0458576a54dc7 (pr/ipc.99 -> pr/ipc.100) fixing minor conflicts
Rebased 004a611351a2a5bf81a4cfb706d0458576a54dc7 -> 7dd8e269cc5ebb479f63aedea0c9994e89671a60 (pr/ipc.100 -> pr/ipc.101, compare) due to conflicts in base prs
Updated 7dd8e269cc5ebb479f63aedea0c9994e89671a60 -> 8f7b2b772d2a8941b34b54c5e3c345309eed83de (pr/ipc.101 -> pr/ipc.102, compare) on ipc-build.16, also lowercasing walletnotifications and adding boost::optional read/build overloads after they are removed from libmultiprocess
Rebased 8f7b2b772d2a8941b34b54c5e3c345309eed83de -> b70b060230eb792f9ce089b047e2ec01292372e7 (pr/ipc.102 -> pr/ipc.103, compare) due to conflicts with #18249, #18241, and #18249
Rebased b70b060230eb792f9ce089b047e2ec01292372e7 -> 0ffff86cffbd1048a435c55e9b6d19f4e0200fb2 (pr/ipc.103 -> pr/ipc.104, compare) for various conficts and with ReadFieldUpdate/ReadFieldNew merge
Rebased 0ffff86cffbd1048a435c55e9b6d19f4e0200fb2 -> 67eb575c5b071ea802bd2fa02b9022b92dd8d9f8 (pr/ipc.104 -> pr/ipc.105, compare) with msvc test_bitcoin-qt build fix, centos c++ old compiler fix, updated libmultiprocess fixing ThrowFn exception reading
Updated 67eb575c5b071ea802bd2fa02b9022b92dd8d9f8 -> 4d153cc8a61610da91488e2f250dac74c87fad32 (pr/ipc.105 -> pr/ipc.106, compare) with feature_config_args fix
Rebased 4d153cc8a61610da91488e2f250dac74c87fad32 -> 9e4ccac3a842e060b5862c73f4bb8cb2d741cb98 (pr/ipc.106 -> pr/ipc.107, compare) after #16367 merge and with shared_ptr callback support needed after #18338
Updated 9e4ccac3a842e060b5862c73f4bb8cb2d741cb98 -> eb8a19ce9e7ed863f3271f37d2d8684dac1c283b (pr/ipc.107 -> pr/ipc.108, compare) with multiwallet urlencode test fix
Rebased eb8a19ce9e7ed863f3271f37d2d8684dac1c283b -> 0ba0b3052ca916085fc5e93f5c26f9e4f14fa17d (pr/ipc.108 -> pr/ipc.109, compare) with support for FoundBlock passing in #17954 and after IPC build revert #18588
Rebased 0ba0b3052ca916085fc5e93f5c26f9e4f14fa17d -> aa38570fa5f1ceee5883a8170da60206051e05fd (pr/ipc.109 -> pr/ipc.110, compare) due to conflicts with #18571, #16426, and #17781
Rebased aa38570fa5f1ceee5883a8170da60206051e05fd -> fd810d2ae759091dadf39d1d6af3bf267e66bbfd (pr/ipc.110 -> pr/ipc.111, compare) after base prs merged and many conflicts with other PRs resolved
ryanofsky referenced this in commit 5fff177756 on Mar 5, 2020
ryanofsky referenced this in commit 9621f4469f on Mar 5, 2020
ryanofsky referenced this in commit 23b79a3555 on Mar 6, 2020
ryanofsky referenced this in commit 78f28a2655 on Mar 6, 2020
ryanofsky referenced this in commit 1c53c9ec9f on Mar 6, 2020
ryanofsky referenced this in commit 0c1e0c04c4 on Mar 6, 2020
ryanofsky referenced this in commit 1390103563 on Mar 6, 2020
ryanofsky referenced this in commit 1242ac8e31 on Mar 6, 2020
ryanofsky referenced this in commit 1e822cd493 on Mar 6, 2020
ryanofsky referenced this in commit 072df2ec6f on Mar 6, 2020
ryanofsky referenced this in commit ddea706fa7 on Mar 6, 2020
ryanofsky referenced this in commit 67e7fe7641 on Mar 6, 2020
ryanofsky referenced this in commit f2809b240e on Mar 6, 2020
ryanofsky referenced this in commit 51c9b1472f on Mar 6, 2020
ryanofsky referenced this in commit a16ea53d67 on Mar 6, 2020
ryanofsky referenced this in commit 5212bd926b on Mar 6, 2020
ryanofsky referenced this in commit c7b9338fd3 on Mar 6, 2020
ryanofsky referenced this in commit 46fa4ba52c on Mar 6, 2020
ryanofsky force-pushed on Mar 6, 2020
ryanofsky referenced this in commit 5df203c45f on Mar 6, 2020
ryanofsky referenced this in commit 720c59a564 on Mar 6, 2020
ryanofsky referenced this in commit 34d68548fa on Mar 6, 2020
ryanofsky referenced this in commit 4231e81ce6 on Mar 6, 2020
ryanofsky referenced this in commit 5c3803bd88 on Mar 6, 2020
ryanofsky referenced this in commit 3335df38bf on Mar 6, 2020
DrahtBot removed the label Needs rebase on Mar 7, 2020
ryanofsky referenced this in commit a9ff82406f on Mar 9, 2020
ryanofsky referenced this in commit 39d05cb2e7 on Mar 9, 2020
DrahtBot added the label Needs rebase on Mar 9, 2020
ryanofsky referenced this in commit 623b19deb3 on Mar 9, 2020
ryanofsky referenced this in commit 5d5899d07b on Mar 9, 2020
ryanofsky referenced this in commit 64764c2f5b on Mar 9, 2020
ryanofsky referenced this in commit 782c7ef202 on Mar 9, 2020
ryanofsky referenced this in commit 0a183e3f02 on Mar 9, 2020
ryanofsky referenced this in commit 07912bfbfb on Mar 9, 2020
ryanofsky referenced this in commit 519afbee0f on Mar 9, 2020
ryanofsky referenced this in commit 6a9e96fa5b on Mar 9, 2020
ryanofsky referenced this in commit d12892ad69 on Mar 9, 2020
ryanofsky referenced this in commit 493421a3d1 on Mar 9, 2020
ryanofsky referenced this in commit b812395298 on Mar 9, 2020
ryanofsky referenced this in commit a752430301 on Mar 9, 2020
ryanofsky referenced this in commit 4437ae0549 on Mar 9, 2020
ryanofsky referenced this in commit e565a694c7 on Mar 9, 2020
ryanofsky referenced this in commit b4bc14ce50 on Mar 20, 2020
ryanofsky referenced this in commit b80e677e35 on Mar 20, 2020
ryanofsky referenced this in commit 5389f3fa8e on Mar 20, 2020
ryanofsky referenced this in commit 69e3ebd8d4 on Mar 20, 2020
ryanofsky referenced this in commit 77e4b06572 on Mar 20, 2020
ryanofsky referenced this in commit 6ceb21909c on Mar 20, 2020
ryanofsky referenced this in commit 96dfe5ced6 on Mar 20, 2020
ryanofsky referenced this in commit 1dca9dc4c7 on Mar 20, 2020
ryanofsky referenced this in commit e43c923314 on Mar 20, 2020
ryanofsky referenced this in commit 005b0716a0 on Mar 20, 2020
ryanofsky referenced this in commit 0957095609 on Mar 20, 2020
ryanofsky referenced this in commit d9c8350933 on Mar 20, 2020
ryanofsky referenced this in commit b3f5eda1a2 on Mar 20, 2020
ryanofsky referenced this in commit 1fa2e9ab89 on Mar 20, 2020
ryanofsky referenced this in commit 6f3e1bdb97 on Mar 20, 2020
ryanofsky referenced this in commit 92a9448b66 on Mar 20, 2020
ryanofsky force-pushed on Mar 20, 2020
DrahtBot removed the label Needs rebase on Mar 20, 2020
maflcko referenced this in commit ac579ada7e on Mar 23, 2020
ryanofsky referenced this in commit cdd066d66b on Mar 25, 2020
ryanofsky force-pushed on Mar 25, 2020
ryanofsky referenced this in commit 671d33e0ed on Mar 25, 2020
ryanofsky referenced this in commit 671749e327 on Mar 25, 2020
ryanofsky force-pushed on Mar 25, 2020
sidhujag referenced this in commit d630e78524 on Mar 28, 2020
DrahtBot added the label Needs rebase on Mar 31, 2020
ryanofsky referenced this in commit e6e44eedd5 on Apr 6, 2020
ryanofsky referenced this in commit fedadaf084 on Apr 6, 2020
maflcko referenced this in commit 1b30761360 on Apr 10, 2020
maflcko referenced this in commit 99d6a5be8b on Apr 10, 2020
ryanofsky referenced this in commit 3f50cdc914 on Apr 10, 2020
ryanofsky referenced this in commit d04533c28e on Apr 10, 2020
ryanofsky force-pushed on Apr 10, 2020
ryanofsky force-pushed on Apr 10, 2020
DrahtBot removed the label Needs rebase on Apr 10, 2020
maflcko referenced this in commit 3eb8b1c392 on Apr 10, 2020
ryanofsky referenced this in commit 9fe489b76f on Apr 10, 2020
ryanofsky referenced this in commit 8928ebd6eb on Apr 10, 2020
DrahtBot added the label Needs rebase on Apr 11, 2020
sidhujag referenced this in commit 292df1690f on Apr 13, 2020
sidhujag referenced this in commit cb75c3b555 on Apr 13, 2020
sidhujag referenced this in commit c486c7458a on Apr 13, 2020
ryanofsky referenced this in commit f3ecfb553f on Apr 15, 2020
ryanofsky referenced this in commit 26c1dc9c2d on Apr 15, 2020
ryanofsky referenced this in commit fa4c9ff83c on Apr 15, 2020
ryanofsky force-pushed on Apr 15, 2020
ryanofsky referenced this in commit ae38954238 on Apr 15, 2020
ryanofsky referenced this in commit 165ec1f83d on Apr 15, 2020
DrahtBot removed the label Needs rebase on Apr 15, 2020
ryanofsky referenced this in commit bc5f6da84e on Apr 16, 2020
SmarterHomes referenced this in commit e88645f092 on Apr 17, 2020
SmarterHomes referenced this in commit 9746ac3b44 on Apr 17, 2020
SmarterHomes referenced this in commit 8f3b970b3f on Apr 17, 2020
SmarterHomes referenced this in commit ee710bddc0 on Apr 17, 2020
ryanofsky referenced this in commit 4f359eb256 on Apr 20, 2020
ryanofsky referenced this in commit cf333aba15 on Apr 25, 2020
DrahtBot added the label Needs rebase on May 1, 2020
ryanofsky referenced this in commit bf0a510981 on May 1, 2020
ryanofsky referenced this in commit ae9ee3c8fc on May 1, 2020
ryanofsky force-pushed on May 1, 2020
ryanofsky referenced this in commit 803fb13624 on May 1, 2020
ryanofsky referenced this in commit a9014cf50a on May 1, 2020
DrahtBot removed the label Needs rebase on May 1, 2020
JeremyRubin
commented at 8:07 PM on May 1, 2020:
contributor
Now that #17905 is merged this seems to be the fourth step in the multiprocess transformation.
What things need to happen in your view for this to proceed? From my view this seems like the step where we need to decide to use capnproto or not, and if not, what sort of replacement library. Personally I think using capnproto is a decent idea, as it's a well maintained project, and can save us from a lot of the common bugs we might otherwise run into if we rolled our own. OTOH it's a big codebase so we might want to tread carefully.
In particular, it seems that we're using CapnProto in a very limited way. CapnProto is designed to be usable as a layer between untrusted peers (e.g., at the network consensus layer). This isn't what's going on here. In our usage here it's only between trusted node components. Even without CapnProto, if your wallet user wants to mess with your node they can do some heavy damage, so I don't really see a new attack surface. In fact, I see less attack surface because now if someone pops your node handling network stuff they can't get to your private keys. That seems like a major improvement over the status quo, even at the expense of a new, complex, library.
Curious to hear what other's think and if we can move ahead with capnproto.
jb55
commented at 8:13 PM on May 1, 2020:
contributor
when it was last mentioned on IRC, the idea was to merge it as experimental with capnproto, with plans to eventually replace it when that makes sense.
ryanofsky referenced this in commit 1c51df5bab on May 6, 2020
DrahtBot added the label Needs rebase on May 7, 2020
ryanofsky referenced this in commit 0775109f55 on May 7, 2020
ryanofsky referenced this in commit 0ffa2052cf on May 7, 2020
ryanofsky referenced this in commit 09939485d3 on May 8, 2020
ryanofsky referenced this in commit 6d911a40cf on May 8, 2020
ryanofsky referenced this in commit 1bc9859d6a on May 13, 2020
ryanofsky referenced this in commit 72c2eff332 on May 13, 2020
ryanofsky referenced this in commit 5d1377b52b on May 13, 2020
jonasschnelli referenced this in commit a587f85853 on May 20, 2020
sidhujag referenced this in commit 381c70f27b on May 20, 2020
fanquake referenced this in commit 97b21b302a on May 21, 2020
sidhujag referenced this in commit 6a4320e7a9 on May 21, 2020
ryanofsky force-pushed on Jun 1, 2020
ryanofsky referenced this in commit 294105ebb1 on Jun 1, 2020
DrahtBot removed the label Needs rebase on Jun 1, 2020
in
test/functional/feature_block.py:86
in
b737e5397aoutdated
82 | @@ -83,6 +83,7 @@ def set_test_params(self):
83 | self.num_nodes = 1
84 | self.setup_clean_chain = True
85 | self.extra_args = [['-acceptnonstdtxn=1']] # This is a consensus block test, we don't care about tx policy
86 | + self.rpc_timeout = 1920
in commit b737e5397af4cedde1aec47f5b4e87c2521efdd0
Out of scope for this pr, but I am wondering if it wouldn't be easier to compile an increased rpc timeout factor into the test config file. This way, all tests are updated when compiled with ipc and it would avoid having to update them manually and individually.
Out of scope for this pr, but I am wondering if it wouldn't be easier to compile an increased rpc timeout factor into the test config file. This way, all tests are updated when compiled with ipc and it would avoid having to update them manually and individually.
That's a good idea, I didn't know about the timeout factor and it'd probably better to increase that if there are other timeouts outside this test. Some IPC performance problems are actually very easy to fix after you know they exist, though, so there might be some argument for increasing timeouts selectively instead of globally to identify these places, at least to start out with before it becomes a burden.
In that case it may be good to add comments that the long timeout is due to IPC.
I'm happy to add a specific comment if you want to suggest one, but I can't think of what would be appropriate to say here about IPC. IPC does slow things down generally, like running on a slow machine, and I think it's expected that if you run code in a new environment, you may need to adjust some parameters that didn't have enough tolerance.
It's true that there are a lot of places where we can change application code to have better performance over IPC (basically just places IPC methods are called in a repeatedly and we can reduce the number of round trips), but it wouldn't seem seem appropriate to put comments about it here in the middle of this one test.
Sjors
commented at 2:16 PM on June 2, 2020:
member
When launching src/bitcoin-gui I get an error message: Error: Error reading configuration file:. It trips over unknown setting names, even for a different network (e.g. -signer which is for an unmerged PR). src/qt/bitcoin-qt and src/bitcoin-node do not have this problem; they report unrecognised options in the log.
If I stop bitcoin-gui with ctrl+c it's not always very graceful, e.g. I got:
^C
libc++abi.dylib: terminating with uncaught exception of type interfaces::IpcException: kj::Exception: capnp/rpc.c++:2092: disconnected: Peer disconnected.
stack: 1084bf30a 106e09f40 106e0c090`
If I stop early it's also not very happy:
2020-06-02T14:06:07Z Using obfuscation key for /Users/sjors/Library/Application Support/Bitcoin/testnet3/blocks/index: 0000000000000000
^C2020-06-02T14:06:10Z Shutdown requested. Exiting.
Assertion failed: (nThreadsServicingQueue == 0), function ~CScheduler, file scheduler.cpp, line 18
DrahtBot removed the label Needs rebase on Sep 13, 2020
ryanofsky force-pushed on Sep 14, 2020
ryanofsky force-pushed on Sep 14, 2020
DrahtBot added the label Needs rebase on Sep 18, 2020
ryanofsky referenced this in commit c832c85561 on Sep 24, 2020
ryanofsky referenced this in commit d563dfa46a on Sep 24, 2020
ryanofsky referenced this in commit 4e7906269c on Sep 25, 2020
ryanofsky referenced this in commit 97e683d1a1 on Sep 25, 2020
ryanofsky force-pushed on Sep 26, 2020
DrahtBot removed the label Needs rebase on Sep 26, 2020
DrahtBot added the label Needs rebase on Sep 26, 2020
ryanofsky force-pushed on Sep 28, 2020
DrahtBot removed the label Needs rebase on Sep 28, 2020
ryanofsky referenced this in commit c915cea0db on Oct 1, 2020
ryanofsky referenced this in commit a8fbbd7364 on Oct 1, 2020
ryanofsky force-pushed on Oct 1, 2020
ryanofsky force-pushed on Oct 2, 2020
ryanofsky referenced this in commit 2f521b2c24 on Oct 2, 2020
ryanofsky referenced this in commit bf525a8d60 on Oct 21, 2020
promag
commented at 9:45 PM on October 23, 2020:
contributor
Outstanding work and effort! Not sure if it was mentioned, but while testing I've noticed it takes way longer to load big wallets. But this is something we could improve in different ways, not in the context of this PR.
Concept ACK.
DrahtBot added the label Needs rebase on Oct 27, 2020
janus referenced this in commit b5e8dd35ed on Nov 5, 2020
janus referenced this in commit ee612c6fce on Nov 15, 2020
ryanofsky referenced this in commit f663f7031c on Nov 25, 2020
ryanofsky referenced this in commit 7723ab68ca on Nov 25, 2020
ryanofsky referenced this in commit af792718de on Nov 25, 2020
ryanofsky force-pushed on Nov 25, 2020
DrahtBot removed the label Needs rebase on Nov 25, 2020
DrahtBot added the label Needs rebase on Dec 2, 2020
janus referenced this in commit ed6e35e3be on Dec 7, 2020
ryanofsky referenced this in commit d0abe4e56d on Dec 8, 2020
ryanofsky referenced this in commit 3fbbb9a640 on Dec 8, 2020
ryanofsky referenced this in commit 1d1688f600 on Dec 8, 2020
ryanofsky referenced this in commit 40fba17b1a on Dec 8, 2020
maflcko referenced this in commit e98d1d6740 on Dec 8, 2020
ryanofsky referenced this in commit 5a695fd248 on Dec 8, 2020
ryanofsky referenced this in commit cc392c014d on Dec 8, 2020
ryanofsky referenced this in commit c58ddd466c on Dec 11, 2020
ryanofsky referenced this in commit fe184ae2df on Dec 11, 2020
ryanofsky force-pushed on Dec 11, 2020
DrahtBot removed the label Needs rebase on Dec 11, 2020
ryanofsky force-pushed on Dec 11, 2020
ryanofsky force-pushed on Dec 11, 2020
ryanofsky
commented at 6:19 PM on December 11, 2020:
contributor
Fabcien referenced this in commit c2a9184653 on Dec 14, 2020
DrahtBot added the label Needs rebase on Dec 16, 2020
ryanofsky referenced this in commit a51b9f463d on Dec 18, 2020
ryanofsky referenced this in commit fe0a68197a on Dec 18, 2020
ryanofsky force-pushed on Dec 18, 2020
DrahtBot removed the label Needs rebase on Dec 18, 2020
DrahtBot added the label Needs rebase on Jan 7, 2021
Fabcien referenced this in commit 1d7377e74d on Jan 11, 2021
ryanofsky referenced this in commit 35d2091134 on Jan 28, 2021
ryanofsky referenced this in commit 94b8f19f98 on Jan 28, 2021
ryanofsky force-pushed on Jan 29, 2021
DrahtBot removed the label Needs rebase on Jan 29, 2021
ryanofsky referenced this in commit 8a6e870cbb on Jan 29, 2021
ryanofsky force-pushed on Jan 30, 2021
ryanofsky force-pushed on Feb 1, 2021
ryanofsky force-pushed on Feb 3, 2021
ryanofsky force-pushed on Feb 20, 2021
DrahtBot added the label Needs rebase on Mar 2, 2021
electorr
commented at 11:16 AM on March 4, 2021:
none
NACK
This is using a library produced by an outside entity that
addresses a specific use case of bitcoin.
Good intentions and all, bait-and-switch is a thing.
20 years into the future we have a company having control
of a part of the bitcoin code which does not get the same level
of audit as the bitcoin code itself but neither as external libraries
which are not targeted at bitcoin like zmq.
ryanofsky force-pushed on Mar 4, 2021
ryanofsky
commented at 2:39 PM on March 4, 2021:
contributor
Concerns about external libraries (including zmq) are legitimate and this is a reason multiprocess support is optional and will always be optional. Just like you can build versions of bitcoin without GUI or wallet support, and just like we distribute separate bitcoin-qt and bitcoind binaries so you can choose whether or not to use Qt, you will always be able to build bitcoin without multiprocess support. Also, you can also choose whether to use multiprocess binaries.
Multiprocess support is disabled by default in this PR (toggled by the --enable-multiprocess option), and even when it is enabled it just creates new bitcoin-gui, bitcoin-node, and bitcoin-wallet binaries without any effect on existing bitcoind and bitcoin-qt binaries.
Beyond the ability to disable multiprocess support, there is also the ability to enable it and replace it with a different implementation. For example, if you want multiprocess support, but would prefer not to rely on the external libmultiprocess library, you could implement a class overriding interfaces::Ipc with methods spawnProcess, serveProcess, connectAddress, listenAddress that use a different IPC protocol or are implemented with different libraries.
Rebased 585585c28f69cc232c2f9e6d1ae96c8d3a11eb5a -> a43b55a381b023800f9f5f68a2a56a1e374ab19f (pr/ipc.143 -> pr/ipc.144, compare) on #19160 pr/ipc-echo.27 due to conflicts with #20685 and #21015
Rebased a43b55a381b023800f9f5f68a2a56a1e374ab19f -> 26bc68baf4d221bfd45cb4c61556fd5851376e84 (pr/ipc.144 -> pr/ipc.145, compare) due to conflict with bitcoin-core/gui#233
Rebased 26bc68baf4d221bfd45cb4c61556fd5851376e84 -> 667affafa4b1a086746de9ef17c459e40380829b (pr/ipc.145 -> pr/ipc.146, compare) due to conflicts with #21007 and #21404
ryanofsky referenced this in commit f228b9bac4 on Mar 4, 2021
DrahtBot removed the label Needs rebase on Mar 4, 2021
DrahtBot added the label Needs rebase on Mar 8, 2021
ryanofsky referenced this in commit 9048c58e10 on Mar 9, 2021
ryanofsky force-pushed on Mar 9, 2021
DrahtBot removed the label Needs rebase on Mar 9, 2021
DrahtBot added the label Needs rebase on Mar 11, 2021
in
src/init/bitcoin-gui.cpp:47
in
26bc68baf4outdated
maflcko referenced this in commit 1e57d14d96 on Mar 15, 2021
ryanofsky force-pushed on Mar 23, 2021
DrahtBot removed the label Needs rebase on Mar 23, 2021
ariard
commented at 2:40 PM on March 26, 2021:
none
I've been following the instructions in doc/multiprocess.md, especially the ones in ##Installation
cd <BITCOIN_SOURCE_DIRECTORY>
make -C depends NO_QT=1 MULTIPROCESS=1
./configure --prefix=$PWD/depends/x86_64-pc-linux-gnu
make
src/bitcoin-node -regtest -printtoconsole -debug=ipc
BITCOIND=bitcoin-node test/functional/test_runner.py
But I'm hitting linker failures likewise :
/home/user/Bitcoin/bitcoin/src/ipc/capnp/node.cpp:112: undefined reference to `mp::ProxyClient<ipc::capnp::messages::Node>::customWalletClient()'
/usr/bin/ld: libbitcoin_ipc.a(libbitcoin_ipc_a-node.o): in function `mp::ProxyClientBase<ipc::capnp::messages::Node, interfaces::Node>::ProxyClientBase(ipc::capnp::messages::Node::Client, mp::Connection*, bool)::{lambda()#2}::operator()() const':
/home/user/Bitcoin/bitcoin/depends/x86_64-pc-linux-gnu/include/mp/proxy-io.h:392: undefined reference to `mp::ProxyClient<ipc::capnp::messages::Node>::destroy()'
/usr/bin/ld: libbitcoin_ipc.a(libbitcoin_ipc_a-init.capnp.proxy-client.o): in function `mp::ProxyClientBase<ipc::capnp::messages::WalletClient, interfaces::WalletClient>::ProxyClientBase(ipc::capnp::messages::WalletClient::Client, mp::Connection*, bool)::{lambda()#2}::operator()() const':
I'm investigating further, it might be local env config ?
Thanks for the bug report. So far I'm not able to reproduce the problem. Just checking out the current PR and following the instructions builds successfully for me (and also seems to be working on CI).
It's possible there are old build outputs on your system and using git-clean could help. Otherwise it'd be great if you opened a new issue https://github.com/chaincodelabs/libmultiprocess/issues/new including the complete error message (maybe with make V=1 to be able to see the linker command line). From the snippet above, it's not clear what executable is actually being linked or if the symbol should be expected to be defined there.
In the future feel free to post any questions or issues related to multiprocess stuff to https://github.com/chaincodelabs/libmultiprocess/issues/new. Even though libmultiprocess isn't bitcoin-specific, the issue tracker is a pretty quiet and reasonable place for discussion, even if issues are tangential. This PR thread is long and it's hard to have ongoing discussion about specific topics here.
ryanofsky referenced this in commit ddcfd078b6 on Mar 27, 2021
backpacker69 referenced this in commit b666892912 on Mar 28, 2021
backpacker69 referenced this in commit 415759cd01 on Mar 28, 2021
backpacker69 referenced this in commit 4e20eb5e2e on Mar 28, 2021
backpacker69 referenced this in commit 03726cf79a on Mar 28, 2021
achow101
commented at 7:45 PM on March 29, 2021:
member
I am not really a fan of overloading bitcoin-wallet to serve as the wallet process. Currently (in master) it is used solely as the wallet tool. It seems really weird to me that with this PR it's behavior is completely different depending on whether it is given -ipcfd. I would rather that the wallet process is a new binary instead of overloading the current bitcoin-wallet.
The last commit seems to be extremely monolithic. It would be nice to break it up into several smaller commits if possible.
ryanofsky
commented at 10:23 PM on March 29, 2021:
contributor
Thanks for checking this!
Tried to run bitcoin-gui and hit an assert:
I'll look into this some more, but feel free to post an issue https://github.com/chaincodelabs/libmultiprocess/issues/new just because it's easier to dig into bugs outside this long PR thread. I wonder if you are using a bitcoin.conf file or have log options defined that could be relevant to this.
I am not really a fan of overloading bitcoin-wallet to serve as the wallet process. Currently (in master) it is used solely as the wallet tool. It seems really weird to me that with this PR it's behavior is completely different depending on whether it is given -ipcfd. I would rather that the wallet process is a new binary instead of overloading the current bitcoin-wallet.
The idea here is to have one wallet tool instead of multiple tools. This is described a little in #19460 (feel free to follow up there). -ipcfd is an internal argument that users shouldn't care about, but #19460 adds an -ipcconnect option to control whether bitcoin-wallet tries to connect to the bitcoin-node IPC socket. The idea would be that
bitcoin-wallet -noipcconnect -wallet=mywallet listtransactions # List limited transaction info
bitcoin-wallet -ipcconnect -wallet=mywallet listtransactions # List full transaction info
bitcoin-wallet -ipcconnect=auto -wallet=mywallet listtransactions # List available transaction info
bitcoin-wallet -noipcconnect -wallet=mywallet serve # Run limited RPC server
bitcoin-wallet -ipcconnect -wallet=mywallet serve # Run full RPC server
bitcoin-wallet -ipcconnect=auto -wallet=mywallet serve # Run full server if possible, but fall back to limited
Default -ipcconnect option value might be a discussion point but in #19460 it is auto.
The last commit seems to be extremely monolithic. It would be nice to break it up into several smaller commits if possible.
Yes, that's the plan. It needs to be split up and also has a few hacks that need to be removed. (Some of the hacks in fact are related to logging and could be responsible for the issue you reported).
DrahtBot added the label Needs rebase on Mar 30, 2021
ryanofsky force-pushed on Apr 8, 2021
ryanofsky
commented at 2:47 AM on April 8, 2021:
contributor
Thanks, reproduced and fixed this now. I missed it because I'd had been testing ahead of this PR on the #19461 branch. Backporting some changes from #19461 fixed it.
DrahtBot removed the label Needs rebase on Apr 8, 2021
ryanofsky force-pushed on Apr 9, 2021
maflcko referenced this in commit 66fd3b28e8 on Apr 23, 2021
laanwj referenced this in commit ac219dcbcc on Apr 27, 2021
DrahtBot added the label Needs rebase on Apr 27, 2021
random-zebra referenced this in commit 4a470eda19 on Apr 28, 2021
random-zebra referenced this in commit 66f12d2558 on Apr 29, 2021
ryanofsky referenced this in commit 90369e5271 on Apr 30, 2021
ryanofsky force-pushed on Apr 30, 2021
ryanofsky
commented at 1:46 PM on April 30, 2021:
contributor
Base pr #19160 was merged, and I split up the big commit into smaller commits, so I think this is definitely in a reviewable state now, even though there still are some small remaining things I would like to clean up where there are todos in the comments.
Rebased 6e7a814da00c9f8ecaf7c57921515e61e0a037b6 -> 68fe5860cc786962b1de94295aac41574b46159c (pr/ipc.148 -> pr/ipc.149, compare) after #19160 merge, splitting up commits and making some minor cleanups.
Rebased 68fe5860cc786962b1de94295aac41574b46159c -> 21064594fd96a1ac20aeae175bb9edba431d1321 (pr/ipc.149 -> pr/ipc.150, compare) due to conflict with bitcoin-core/gui#4 and #22156, on top of bitcoin-core/gui#360 + #22215 + #22217 + #22218 + #22219
Rebased 21064594fd96a1ac20aeae175bb9edba431d1321 -> 334f1f90c951ef89a32a8f857a612f249c07c534 (pr/ipc.150 -> pr/ipc.151, compare) due to silent conflict with bitcoin-core/gui#4 causing cirrus failure https://cirrus-ci.com/task/6103801187794944
Updated 334f1f90c951ef89a32a8f857a612f249c07c534 -> 3515e22d78c7361fe06107721d3bf9616f909aaf (pr/ipc.151 -> pr/ipc.152, compare) to fix external signer cirrus error https://cirrus-ci.com/task/6579691483037696
Rebased 3515e22d78c7361fe06107721d3bf9616f909aaf -> a7c4732716bfd51540d6b070b2b8d9d6e1048aed (pr/ipc.152 -> pr/ipc.153, compare) due to conflict with #21935
Rebased a7c4732716bfd51540d6b070b2b8d9d6e1048aed -> e3d3a183e094aa5acd54ff387fede6c16486523f (pr/ipc.153 -> pr/ipc.154, compare) due to conflict with #22323
Rebased e3d3a183e094aa5acd54ff387fede6c16486523f -> 24dd84afd4414cbb20bb937bb95cfa661d1e2b78 (pr/ipc.154 -> pr/ipc.155, compare) due to conflict with #22320
Rebased 24dd84afd4414cbb20bb937bb95cfa661d1e2b78 -> 1326c4a9a443e4169822eab569be68e53c58de36 (pr/ipc.155 -> pr/ipc.156, compare) due to conflict with #22339
ryanofsky referenced this in commit a5d176c023 on Apr 30, 2021
DrahtBot removed the label Needs rebase on Apr 30, 2021
random-zebra referenced this in commit e2825e0b15 on May 8, 2021
random-zebra referenced this in commit df2c9ab116 on May 17, 2021
ryanofsky referenced this in commit 3287cff0b9 on May 19, 2021
ryanofsky referenced this in commit 26f46feffa on Jun 7, 2021
ryanofsky referenced this in commit fe45c37989 on Jun 7, 2021
ryanofsky referenced this in commit d7bafb5065 on Jun 7, 2021
DrahtBot added the label Needs rebase on Jun 9, 2021
ryanofsky referenced this in commit 93cc53a2b2 on Jun 10, 2021
maflcko referenced this in commit f66eceaecf on Jun 11, 2021
fanquake referenced this in commit 9795e8ec8c on Jun 12, 2021
sidhujag referenced this in commit 5b721bb042 on Jun 13, 2021
ryanofsky force-pushed on Jun 15, 2021
ryanofsky referenced this in commit 75ad998da9 on Jun 15, 2021
DrahtBot removed the label Needs rebase on Jun 15, 2021
ryanofsky force-pushed on Jun 16, 2021
ryanofsky force-pushed on Jun 16, 2021
DrahtBot added the label Needs rebase on Jun 17, 2021
ryanofsky force-pushed on Jun 17, 2021
DrahtBot removed the label Needs rebase on Jun 17, 2021
DrahtBot added the label Needs rebase on Jun 23, 2021
ryanofsky force-pushed on Jun 23, 2021
DrahtBot removed the label Needs rebase on Jun 23, 2021
DrahtBot added the label Needs rebase on Jun 24, 2021
ryanofsky force-pushed on Jun 24, 2021
DrahtBot removed the label Needs rebase on Jun 24, 2021
DrahtBot added the label Needs rebase on Jun 28, 2021
ryanofsky force-pushed on Jul 1, 2021
DrahtBot removed the label Needs rebase on Jul 1, 2021
ryanofsky referenced this in commit 2516d397d5 on Jul 8, 2021
ryanofsky force-pushed on Jul 9, 2021
ryanofsky
commented at 5:17 PM on July 9, 2021:
contributor
in
src/init/bitcoin-node.cpp:45
in
7df963de80outdated
38 | @@ -26,6 +39,15 @@ class BitcoinNodeInit : public interfaces::Init
39 | {
40 | m_node.args = &gArgs;
41 | m_node.init = this;
42 | + // Extra initialization code that runs when a bitcoin-node process is 43 | + // spawned by a bitcoin-gui process, after the ArgsManager configuration 44 | + // is transferred from the parent process to the child process. 45 | + m_ipc->context().init_process = [this] {
IIUC, it's going to be call at the first invocation on the server from the client as it's dependent on the whole serverInvoke()/MakeServerField()/etc call chain ? Sometimes it's hard to follow which objects are defined in bitcoin, which ones are in libmultiprocess and which ones are dynamically generated by mpgen. Though i agree it's fine for now, we can improve with time.
init_process is bitcoin-specific and libmultiprocess can't access it or reference it, and it isn't related to IPC method invocation (serverInvoke is a libmultiprocess internal function that bitcoin code shouldn't access and doesn't reference which translates an incoming IPC method call from a remote process to a C++ method call on a local object.)
init_process is a hack to deal with the fact that bitcoin code uses a bunch of global variables that depend on the gArgs global variable and they need to be initialized at some point.
When bitcoin is running with wallet & node & gui code in the same process, global variables only have to be initialized once by existing init code. But if the node process spawns a wallet process, or if the GUI process spawns a node process, then there are multiple copies of the global variables in different processes, and they have to be newly initialized in each spawned process, and existing init code is no longer sufficient. That is why the GlobalArgs class and init_process callback are introduced. GlobalArgs transfers gArgs state from the parent process to the child process and init_process sets up logging and other global variables that depend on gArgs.
Ideally there would be no global variables shared between node wallet and GUI code (they would move into NodeContext and WalletContext objects) and the init_process hack could go away. Short of that, I maybe rename init_process or you could think of it as something like init_bitcoin_shared_global_variables_in_newly_spawned_child_process_after_transferring_gargs
DrahtBot added the label Needs rebase on Jul 14, 2021
maflcko referenced this in commit ba15ab4990 on Jul 22, 2021
hebasto referenced this in commit 439e58c4d8 on Aug 12, 2021
stratospher referenced this in commit 34e8efb5bb on Aug 14, 2021
fanquake referenced this in commit 62cb4009c2 on Aug 18, 2021
ryanofsky force-pushed on Aug 18, 2021
DrahtBot removed the label Needs rebase on Aug 18, 2021
in
src/netaddress.h:552
in
aa05f698feoutdated
547 | + } else { 548 | + READWRITE(obj.netmask); 549 | + } 550 | + READWRITE(obj.valid); 551 | + // Mark invalid if the result doesn't pass sanity checking. 552 | + SER_READ(obj, if (obj.valid) obj.valid = obj.SanityCheck());
When you are re-introducing this, it might be good to cleanup the complexity in this method or maybe just use the existing json serialization?
Thanks, exposed json functions in #22848, and now using it in this PR
sidhujag referenced this in commit cfb4bf97a5 on Aug 20, 2021
meshcollider referenced this in commit e9d6eb1b80 on Aug 22, 2021
ryanofsky referenced this in commit b3f46e9058 on Aug 31, 2021
ryanofsky force-pushed on Aug 31, 2021
ryanofsky
commented at 4:17 PM on August 31, 2021:
contributor
Rebased bc918ffaaaf108a9bf49325cef7773ba3797feb4 -> d7c617e3580195398bbc34f1efcde548a834427e (pr/ipc.158 -> pr/ipc.159, compare) after #22215 and #22217 merges, on top of #22219 pr/ipc-make.6 and #22848 pr/ipc-banmap.1
switching to banman json serialization
fixing logging bug where different processes wrote to the same log file
fixing bitcoin-node bug where it ran wallet code internally instead of using the bitcoin-wallet process
changing node logging/wallet logging init order feature_logging.py test works unchanged with multiprocess binaries and so GUI does not block waiting for the wallet process to start
DrahtBot removed the label Needs rebase on Oct 6, 2021
DrahtBot added the label Needs rebase on Oct 15, 2021
PastaPastaPasta referenced this in commit 1ada50fee0 on Oct 23, 2021
PastaPastaPasta referenced this in commit 3e8670d81a on Oct 24, 2021
PastaPastaPasta referenced this in commit df0373cd76 on Oct 25, 2021
PastaPastaPasta referenced this in commit 1834aa9a9a on Oct 25, 2021
ryanofsky force-pushed on Oct 25, 2021
ryanofsky
commented at 7:20 PM on October 25, 2021:
contributor
Rebased 884d3e1dfa0d46472cd26093142331e51fb9f2ae -> b9b40ae00e6eef8487e197aff2a9a4a0d07bee12 (pr/ipc.161 -> pr/ipc.162, compare) due to conflict with #22937
Rebased b9b40ae00e6eef8487e197aff2a9a4a0d07bee12 -> eb01cec5c5e71f2dfa4b413ee3759bb78ecf8566 (pr/ipc.162 -> pr/ipc.163, compare) after merge of #23006
Updated eb01cec5c5e71f2dfa4b413ee3759bb78ecf8566 -> 03869abd23949dc4d4315d2b647acd5696f5a0db (pr/ipc.163 -> pr/ipc.164, compare) dropping ArgsManager private member hack
Rebased 03869abd23949dc4d4315d2b647acd5696f5a0db -> 4004e0b4a6e52016f36e0054d869ffdf9e719a1e (pr/ipc.164 -> pr/ipc.165, compare) due to conflicts due to conflicts with #23345 and #23474
Rebased 4004e0b4a6e52016f36e0054d869ffdf9e719a1e -> 4b255e1e938d254231d4455a1d4bf61794ba1511 (pr/ipc.165 -> pr/ipc.166, compare) due to conflict with #22364
Updated 4b255e1e938d254231d4455a1d4bf61794ba1511 -> eef402d2913fdb9f13af73f9fbc789611cf60360 (pr/ipc.166 -> pr/ipc.167, compare) to fix silent conflict with #23591
DrahtBot removed the label Needs rebase on Oct 25, 2021
maflcko referenced this in commit e77d9679fd on Oct 26, 2021
DrahtBot added the label Needs rebase on Oct 26, 2021
janus referenced this in commit f21dd521fa on Oct 29, 2021
ryanofsky force-pushed on Oct 29, 2021
DrahtBot removed the label Needs rebase on Oct 29, 2021
ryanofsky renamed this: [experimental] Multiprocess bitcoin Multiprocess bitcoin on Nov 1, 2021
PastaPastaPasta referenced this in commit 75fda781df on Nov 1, 2021
PastaPastaPasta referenced this in commit 051bdef7f5 on Nov 1, 2021
PastaPastaPasta referenced this in commit 48826b429d on Nov 3, 2021
ryanofsky force-pushed on Nov 9, 2021
janus referenced this in commit c16023118c on Nov 11, 2021
DrahtBot added the label Needs rebase on Nov 15, 2021
ryanofsky force-pushed on Nov 17, 2021
DrahtBot removed the label Needs rebase on Nov 17, 2021
pravblockc referenced this in commit a7a32011d3 on Nov 18, 2021
DrahtBot added the label Needs rebase on Nov 22, 2021
ryanofsky force-pushed on Nov 29, 2021
ryanofsky force-pushed on Nov 29, 2021
DrahtBot removed the label Needs rebase on Nov 29, 2021
DrahtBot added the label Needs rebase on Nov 30, 2021
in
src/ipc/capnp/common-types.h:158
in
eae99d609coutdated
153 | + typename std::enable_if<ipc::capnp::Deserializable<LocalType>::value>::type* enable = nullptr) 154 | +{ 155 | + assert(input.has()); 156 | + auto data = input.get(); 157 | + // Note: stream copy here is unnecessary, and can be avoided in the future 158 | + // when `VectorReader` from #12254 is added.
This is a required part of every .capnp file. It's basically a UUID that acts like a namespace for types defined in the file. It is generated by running capnp id, and is used as a form of namespacing to ensure that even if types from different files or different projects have the same names, their binary representations won't conflict and there wont be any ambiguities in the protocol if objects from different files are serialized together.
I could add a comment like # Random unique identifier for this Cap'n Proto schema file generated by "capnp id" to the top of every .capnp file if that would be helpful. Opting not to for now since it seems maybe too verbose like writing /* This is an include guard to prevent this header from being included twice. */ in a C header.
Sjors
commented at 8:46 AM on December 9, 2021:
member
eef402d looks pretty good. And so far I haven't been able to break things. Although I don't understand all of the IPC code involved, it seems well enough documented to know what to change if any of the interfaces or structs change.
The slowdown is noticable, e.g. when opening the wallet creation dialog from bitcoin-gui, but that can be ironed out in followups.
./interfaces/chain.h:227:18: note: overridden virtual function is here
virtual void initWarning(const bilingual_str& message) = 0;
^
In file included from ipc/capnp/node.cpp:9:
In file included from ./ipc/capnp/node-types.h:11:
In file included from ./ipc/capnp/wallet.capnp.proxy-types.h:6:
In file included from ./ipc/capnp/wallet.capnp.proxy.h:7:
In file included from ./ipc/capnp/wallet.h:9:
./ipc/capnp/chain.capnp.proxy.h:1414:26: warning: 'initError' overrides a member function but is not marked 'override' [-Wsuggest-override]
typename M35::Result initError(M35::Param<0> message);
Also a bunch of these:
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/memory:2644:9: warning: destructor called on non-final 'mp::ProxyClient<ipc::capnp::messages::NotifyHeaderTipCallback>' that has virtual functions but non-virtual destructor [-Wdelete-non-abstract-non-virtual-dtor]
__get_elem()->~_Tp();
The serialisation code in ipc/capnp/common.cpp look straight forward, but @achow101 can probably judge them better. Would it make sense to add tests to eae99d609c7a08fde35e723967d078904ad1cd05 with examples? I suppose we know it all works, because the end result works.
It would be nice if one can reduce the chatter for -debug=ipc, since it's hard to see you own actions through it, but again that can wait:
The serialisation code in ipc/capnp/common.cpp look straight forward, but @achow101 can probably judge them better. Would it make sense to add tests to eae99d6 with examples? I suppose we know it all works, because the end result works.
That's a good idea. I think it would be good to have tests for a few serialized types just to test the overall code path, and show how serialization code gets called. The existing functional tests do provide pretty good coverage for node <-> wallet serialization, but there is basically no coverage for the gui <-> node, gui <-> wallet serialization, so tests could help with coverage, too.
It would be nice if one can reduce the chatter for -debug=ipc, since it's hard to see you own actions through it, but again that can wait:
Yeah there are a few things that could be done to improve this. I think the most promising would be to change higher level polling code to use notifications instead, which would also cut down IPC traffic and improve performance. But also there could be options to log less verbosely without repeating information, or only selectively log some IPC calls.
DrahtBot removed the label Needs rebase on Dec 17, 2021
ryanofsky force-pushed on Dec 22, 2021
ryanofsky
commented at 6:24 PM on December 22, 2021:
contributor
I updated multiprocess and flood of warnings disappeared.
Perhaps you missed something during rebase on https://github.com/bitcoin-core/gui/pull/459. I'm able to consistently crash the bitcoin-gui (or the wallet process) when generating a new Taproot address. Let me know if you can't reproduce it, in which case I'll try to get a debugger to work again on macOS.
Wallet log:
2022-01-14T14:45:54Z [] {bitcoin-wallet-75194/0x11292c600} IPC server recv request [#546](/bitcoin-bitcoin/546/) Wallet.setAddressReceiveRequest$Params (context = (thread = <external capability>, callbackThread = <external capability>), dest = (), id = "8", value = "\\001\\000\\000\\000\\b\\000\\000\\000\\000\\000\\000\\000\\242\\214\\341a\\001\\000\\000\\000\\000\\000\\000\\000\\000\\000\\000\\000\\000\\000\\000\\000\\000")
2022-01-14T14:45:54Z [] {bitcoin-wallet-75194/0x11292c600} IPC server post request [#546](/bitcoin-bitcoin/546/) {bitcoin-wallet-75194/0x70000f53d000 (from bitcoin-node-75191/0x70000186b000 (from bitcoin-gui-75190/0x11ace6600))}
Gui log:
2022-01-14T14:45:54Z [main] GUI: Qt has caught an exception thrown from an event handler. Throwing
exceptions from an event handler is not supported in Qt.
You must not let any exception whatsoever propagate through Qt code.
If that is not possible, in Qt 5 you must at least reimplement
QCoreApplication::notify() and catch all exceptions there.
2022-01-14T14:45:54Z [main]
************************
EXCEPTION: N3ipc9ExceptionE
kj::Exception: capnp/rpc.c++:2413: disconnected: Peer disconnected.
stack: 10d1734ea 10d184242 10cd33590 10cd33ec0
bitcoin in Runaway exception
I also get an exception when I quit (Command + q) the gui immediately after launch, e.g. while it's still loading wallets and such. Combined log:
in
src/ipc/capnp/common-types.h:291
in
cdd239245coutdated
286 | +//! implementation would be able to detect the required argument type. This 287 | +//! could be cleaned up in the future by adding Span constructors to all these 288 | +//! classes and writing a ::capnp::Data CustomReadField function that is called 289 | +//! for any class with a single-argument Span constructor. 290 | +template <typename LocalType, typename Value, typename Output> 291 | +void CustomBuildField(
cdd2392 nit: it's a bit odd to introduce this here, since this commit doesn't use it.
Updated commit message, but the CustomBuildField definition for a generic span is added here because it's the inverse of CustomReadField below for PKHash. PKHash has .data() and .size() members so it can be converted to a span, but it doesn't have a span constructor, so it can't be initialized from a span right now.
DrahtBot added the label Needs rebase on Jan 31, 2022
in
src/net_processing.h:37
in
a5116a57a9outdated
31 | @@ -32,6 +32,9 @@ struct CNodeStateStats {
32 | uint64_t m_addr_processed = 0;
33 | uint64_t m_addr_rate_limited = 0;
34 | bool m_addr_relay_enabled{false};
35 | + // Note: If you add fields to this struct, you should also consider updating 36 | + // the getpeerinfo RPC (in rpc/net.cpp), and the IPC serialization code (in 37 | + // ipc/capnp/node.cpp and ipc/capnp/node.capnp).
It's unclear to me what need to be updated in ipc/capnp/node.cpp in case of new members added to CNodeStateStats ? Also for better clarity the object wrapper could be quoted NodeStateStats. Here and for CNodeStats and proxyType comments.
ryanofsky
commented at 3:54 PM on September 12, 2022:
It's unclear to me what need to be updated in ipc/capnp/node.cpp in case of new members added to CNodeStateStats ? Also for better clarity the object wrapper could be quoted NodeStateStats. Here and for CNodeStats and proxyType comments.
Thanks! Updated the comments to mention specific structs by name, and deleted the part about ipc/capnp/node.cpp (the code that this was referring to was automated away and no longer exists).
ariard
commented at 6:19 PM on April 12, 2022:
none
Do you still wish to land this PR in one block or are you considering to split in few chunks ?
Maybe the capnp wrappers for interfaces can go first, then the bitcoin-node spawning stubs and the bitcoin-wallet ones. That said, not sure if it makes easier to assert correctness of the new IPC serialization code.
Fabcien referenced this in commit 5e2fdfcbd4 on May 5, 2022
ryanofsky referenced this in commit a1d72012c7 on Aug 24, 2022
ryanofsky force-pushed on Aug 24, 2022
DrahtBot removed the label Needs rebase on Aug 24, 2022
DrahtBot added the label Needs rebase on Aug 30, 2022
ryanofsky referenced this in commit 6c6e0ee75a on Sep 8, 2022
ryanofsky force-pushed on Sep 8, 2022
ryanofsky referenced this in commit 8da740308f on Sep 8, 2022
ryanofsky referenced this in commit 9b587201f1 on Sep 8, 2022
ryanofsky force-pushed on Sep 8, 2022
DrahtBot removed the label Needs rebase on Sep 8, 2022
Sjors
commented at 1:14 PM on September 9, 2022:
member
Checked changes since review and tested 05e236772da0ed9396ce269b5fe8384b0de8e775 on macOS 12.5.1 using depends.
The receive address labels have disappeared in GUI, probably because it can no longer see which address the payment was received on. See e.g. this descriptor wallet:
<img width="832" alt="Schermafbeelding 2022-09-09 om 15 09 30" src="https://user-images.githubusercontent.com/10217/189357508-e90338c0-4e78-4643-9d4f-2e68e8e0d7dd.png">
It does still show labels for "sent to".
I still get a crash when generating a taproot address in the GUI:
2022-09-09T13:12:37Z [main] GUI: AddressTablePriv::updateEntry: Warning: Got CT_NEW, but entry is already in model
libc++abi: terminating with uncaught exception of type std::out_of_range: map::at: key not found
libc++abi: terminating with uncaught exception of type ipc::Exception: kj::Exception: capnp/rpc.c++:2092: disconnected: Peer disconnected.
stack: 104d0b010 10181cab0 10181ced0
zsh: segmentation fault src/bitcoin-gui
When shutting down the GUI very quickly after starting, it shuts down cleanly now.
ryanofsky referenced this in commit 1f0b8eb161 on Sep 12, 2022
ryanofsky force-pushed on Sep 12, 2022
ryanofsky
commented at 3:51 PM on September 12, 2022:
contributor
🙌 Thanks for testing! I've followed up with some fixes.
Taproot crash was caused by failing to serialize WitnessV1Taproot variant of CTxDestination. I fixed the problem and added an assertion to make the cause more obvious if CTxDestination is expanded again. I also tested sending and receiving and fixed problems with CScript and Coin serialization that were causing similar crashes. It does look like send and receive labels are displayed correctly in the transaction list now, but I'm not sure if this is because I fixed something, or am just not reproducing the bug. if you see more problems I'd like to know, and thanks again for testing!
Updated 05e236772da0ed9396ce269b5fe8384b0de8e775 -> d0bea139b7a4a158eea6fec62285646a826a62b9 (pr/ipc.176 -> pr/ipc.177, compare) with fixes
Sjors
commented at 7:04 AM on September 13, 2022:
member
@ryanofsky linter complains: Unrecognized address type. Serialization not implemented for
ryanofsky referenced this in commit 150f2fa5da on Sep 13, 2022
ryanofsky force-pushed on Sep 13, 2022
ryanofsky
commented at 7:21 PM on September 13, 2022:
contributor
@ryanofsky linter complains: Unrecognized address type. Serialization not implemented for
jonatack
commented at 12:21 PM on September 14, 2022:
member
Is this the current multiprocess pull to review? (If yes, will review. It builds cleanly for me. Thanks!)
JeremyRubin
commented at 4:18 PM on September 14, 2022:
contributor
@jonatack also have been curious about this -- it might make sense to open a new PR for it since GH's UX kinda craps the bed on being able to look at other's reviews once it's like this
<img width="188" alt="image" src="https://user-images.githubusercontent.com/886523/190208093-a11b1998-13af-46e0-bbb8-4d41b47e74bb.png">
ryanofsky
commented at 9:12 PM on September 14, 2022:
contributor
Is this the current multiprocess pull to review? (If yes, will review. It builds cleanly for me. Thanks!)
Yes this is working, and up to date, and split up into commits that should make it easier to review. I think all the information needed to review is covered or linked to in the PR description, and it's safe to ignore the older comment threads. But I can open a new PR if github's UX makes it difficult to review or comment on this one.
aureleoules
commented at 8:30 AM on September 15, 2022:
contributor
ryanofsky referenced this in commit a9587178bc on Sep 20, 2022
ryanofsky force-pushed on Sep 20, 2022
ryanofsky referenced this in commit e690496ea7 on Sep 20, 2022
ryanofsky force-pushed on Sep 20, 2022
ryanofsky
commented at 5:37 PM on September 20, 2022:
contributor
Updated ee5825aa8eb6bcce60db591b3c85c79253a76a31 -> a1b6968c4c2adb978a04386a2154f3f1b32b711b (pr/ipc.179 -> pr/ipc.180, compare) just updating some comments as suggested
ryanofsky referenced this in commit 17ad236d67 on Sep 26, 2022
ryanofsky force-pushed on Sep 26, 2022
ryanofsky referenced this in commit 40a92f0fa0 on Sep 26, 2022
ryanofsky
commented at 4:40 PM on September 26, 2022:
contributor
Rebased a1b6968c4c2adb978a04386a2154f3f1b32b711b -> d8a971115bcd058606d5185fcc5be46c82bb3b2a (pr/ipc.180 -> pr/ipc.181, compare) due to silent conflict with #25737
Squashed d8a971115bcd058606d5185fcc5be46c82bb3b2a -> f8d5af9518b02ee017a90a6e5aede3dff8e62a00 (pr/ipc.181 -> pr/ipc.182, compare)
ryanofsky referenced this in commit 4d84f9ba64 on Sep 26, 2022
ryanofsky force-pushed on Sep 26, 2022
achow101
commented at 7:14 PM on October 17, 2022:
member
libmultiprocess seems like it would better fit as a subtree (a la leveldb, secp256k1, etc.) rather than a dependency given that it is slightly more difficult to install than all of our other dependencies. It also should probably be moved under the bitcoin-core org.
jamesob
commented at 11:32 AM on October 25, 2022:
contributor
Concept ACK, will review this week.
DrahtBot added the label Needs rebase on Dec 6, 2022
ryanofsky referenced this in commit dfb7b92f02 on Jan 20, 2023
ryanofsky force-pushed on Jan 20, 2023
DrahtBot removed the label Needs rebase on Jan 20, 2023
maflcko
commented at 9:35 AM on January 21, 2023:
member
Not sure if related or not:
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_backup.py", line 217, in run_test
assert_equal(res0_rpc.getbalance(), balance0)
File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 56, in assert_equal
raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(47.89978700 == 46.19956280)
Yes, the IPC serialization code wasn't setting the interfaces::FoundBlock.found field, which caused wallets not to sync correctly after #26679. (The serialization bug present before #26679, but didn't have an effect until new code tried to use the missing field.)
Rebased f8d5af9518b02ee017a90a6e5aede3dff8e62a00 -> 05e44e7fd41b32d57379bb6a87c4bebb4a166f9c (pr/ipc.182 -> pr/ipc.183, compare) due to conflicts with #25957, #26752, #26672, and #26238
Rebased 05e44e7fd41b32d57379bb6a87c4bebb4a166f9c -> 5e90c75de0d13aedb7e7c06ebe5aa55b11bb398d (pr/ipc.183 -> pr/ipc.184, compare) fixing wallet_backup.py CI failure https://cirrus-ci.com/task/4576796809625600?logs=ci#L2695 caused by IPC FoundBlock bug exposed by #26679, and fixing wallet_fast_rescan.py failure caused by silent conflict with #25957
Rebased 5e90c75de0d13aedb7e7c06ebe5aa55b11bb398d -> 80926e1b4e63d241ac8b4922b2a2a3204a9c5859 (pr/ipc.184 -> pr/ipc.185, compare) to fix silent conflict with bitcoin-core/gui#697
DrahtBot added the label Needs rebase on Feb 28, 2023
ryanofsky force-pushed on Feb 28, 2023
DrahtBot removed the label Needs rebase on Feb 28, 2023
DrahtBot added the label Needs rebase on Mar 11, 2023
ryanofsky force-pushed on Mar 16, 2023
DrahtBot removed the label Needs rebase on Mar 16, 2023
DrahtBot added the label Needs rebase on Apr 11, 2023
ryanofsky force-pushed on Apr 13, 2023
DrahtBot removed the label Needs rebase on Apr 13, 2023
PastaPastaPasta referenced this in commit a094d50f33 on Apr 15, 2023
PastaPastaPasta referenced this in commit 00f58c0a2e on Apr 15, 2023
PastaPastaPasta referenced this in commit f5557bed67 on Apr 16, 2023
PastaPastaPasta referenced this in commit dc031ca862 on Apr 16, 2023
UdjinM6 referenced this in commit a4e5458daa on Apr 16, 2023
DrahtBot added the label Needs rebase on Apr 21, 2023
Sjors
commented at 6:10 PM on April 28, 2023:
member
Building c86da22175e5bbe40e03624689bd3d4cc1508ba1 fails when configured without a wallet, but with gui.
Intel macOS 13.3.1 with capnp 0.10.3 via homebrew and libmultiprocess 1af83d15239ccfa7e47b8764029320953dd7fdf1.
So it happens without any client connecting, or even listening for one. I sanity checked that master at 6c7ebcc14b7908a67a8f8764b398e76c8fb4fe8b works fine when configured with --enable-multiprocess.
Ran the same commit again with -debug=ipc to get more useful log info. (log)
I also tried disabling the indexes to simplify the setup, but that didn't prevent the issue.
I also tried running with -listen=0 and -connect=0 so that it had no peers, but that made no difference either.
ryanofsky
commented at 10:40 PM on May 11, 2023:
contributor
Thanks @Sjors. I've tried a lot of things and have not been able to reproduce the crash exactly, but the log you posted was helpful, and I could reproduce similar error output by adding an extra call from bitcoin-node to bitcoin-wallet that sleeps in bitcoin-wallet, and then manual killing bitcoin-wallet process. When I do that I see similar kj::Exception errors on the bitcoin-node main thread and bitcoin-node b-capnp-loop thread like in your log. In your log the exact errors are kj/async-io-unix.c++:498: disconnected: ::read(fd, buffer, maxBytes): Connection reset by peer while in my case they are kj::Exception: capnp/rpc.c++:2421: disconnected: Peer disconnected. But maybe this is a macos vs linux platform difference, because otherwise the errors seem identical.
The upshot of this is that I think somehow bitcoin-wallet process is crashing on shutdown and causing the bitcoin-node unexpected network disconnect errors. Could debug this by:
Seeing if ~/.bitcoin/debug.log.wallet has more information
Attaching to bitcoin-wallet process with GDB before it crashes with gdb src/bitcoin-wallet <bitcoin-wallet pid>
Running bitcoin-node under GDB to see what wallet method it is calling when the wallet crashes. From the IPC log it seems to be calling ChainClient.flush, but if you run gdb -ex 'catch throw' -ex run --args src/bitcoin-node -debug=ipc it might be possible to get a more complete stack trace
I should look more into WalletLoaderImpl::flush method to see what that could be doing to cause a crash, too.
knst referenced this in commit f27c4140b0 on May 25, 2023
PastaPastaPasta referenced this in commit fc930dc0f4 on May 31, 2023
bitcoin deleted a comment on Jun 5, 2023
bitcoin deleted a comment on Jun 5, 2023
DrahtBot added the label Needs rebase on Jun 20, 2023
knst referenced this in commit ab426edc77 on Jul 6, 2023
knst referenced this in commit 627dd17132 on Jul 24, 2023
knst referenced this in commit 4c00a33cce on Jul 28, 2023
PastaPastaPasta referenced this in commit 4f00d45e53 on Aug 3, 2023
vijaydasmp referenced this in commit 73500e22ca on Aug 11, 2023
knst referenced this in commit eb9e1a4a49 on Aug 17, 2023
knst referenced this in commit 8d6013ff4b on Aug 17, 2023
This PR adds an --enable-multiprocess configure option which builds new bitcoin-node, bitcoin-wallet, and bitcoin-gui executables 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 Init interface that spawns new wallet and node subprocesses that can be controlled over a socketpair by calling Node, Wallet, and ChainClient methods. When running with split processes, you can see the IPC messages going back and forth in -debug=1 output. Followup PR's #19460 and #19461 add -ipcbind and -ipcconnect options 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).
ryanofsky force-pushed on Sep 20, 2023
ryanofsky
commented at 12:50 AM on September 20, 2023:
contributor
Rebased 1e5d7da989c6a6cab843472675d409bbae34ac3a -> 1ea5d5964bb38f817fb629df0d5f20dfb4d938f1 (pr/ipc.190 -> pr/ipc.191, compare) due to various conflicts
DrahtBot added the label CI failed on Sep 20, 2023
DrahtBot removed the label Needs rebase on Sep 20, 2023
maflcko
commented at 10:55 AM on September 21, 2023:
member
CI looks a bit red
ryanofsky
commented at 11:37 AM on September 21, 2023:
contributor
CI looks a bit red
Thanks. I need to do some more work. There is a problem with the util::Result workaround 79c557f97e096f2a393d35e9eafd2bf13da201be. Also a silent merge conflict with https://github.com/bitcoin-core/gui/pull/738
ryanofsky force-pushed on Sep 26, 2023
ryanofsky force-pushed on Sep 26, 2023
ryanofsky force-pushed on Sep 28, 2023
ryanofsky force-pushed on Sep 28, 2023
ryanofsky force-pushed on Sep 28, 2023
ryanofsky force-pushed on Sep 28, 2023
ryanofsky force-pushed on Sep 29, 2023
ryanofsky force-pushed on Sep 30, 2023
DrahtBot removed the label CI failed on Sep 30, 2023
DrahtBot added the label Needs rebase on Oct 2, 2023
achow101 referenced this in commit fbcf1029a7 on Oct 17, 2023
ryanofsky force-pushed on Oct 20, 2023
ryanofsky
commented at 4:40 PM on October 20, 2023:
contributor
DrahtBot removed the label Needs rebase on Oct 20, 2023
DrahtBot added the label CI failed on Oct 20, 2023
Frank-GER referenced this in commit 285ddfd388 on Oct 21, 2023
ryanofsky marked this as a draft on Oct 24, 2023
ryanofsky force-pushed on Oct 24, 2023
gades referenced this in commit d57ea41ab3 on Nov 10, 2023
fanquake referenced this in commit 29c2c90362 on Nov 13, 2023
DrahtBot added the label Needs rebase on Nov 13, 2023
vijaydasmp referenced this in commit e1339a4f29 on Nov 20, 2023
vijaydasmp referenced this in commit 67941b723f on Nov 20, 2023
vijaydasmp referenced this in commit 91a8ddebb2 on Nov 20, 2023
ryanofsky force-pushed on Nov 21, 2023
ryanofsky
commented at 7:39 PM on November 21, 2023:
contributor
Rebased 18d382f4f8a74612ae0e91efd8a2b83bb665e081 -> 87e3a3b694abc4e0cf8241e31905d3f604c611cf (pr/ipc.201 -> pr/ipc.202, compare) on top of base PR #28921, also fixing conflicts with #28438 and #28503
Rebased 87e3a3b694abc4e0cf8241e31905d3f604c611cf -> 7ba123f1b47a4a790370c9a5975b1e0b4026a241 (pr/ipc.202 -> pr/ipc.203, compare) on top of base PR #28929 to simplify serialization code
DrahtBot removed the label Needs rebase on Nov 21, 2023
DrahtBot removed the label CI failed on Nov 21, 2023
ryanofsky force-pushed on Nov 23, 2023
DrahtBot added the label CI failed on Nov 23, 2023
DrahtBot added the label Needs rebase on Nov 28, 2023
ryanofsky force-pushed on Nov 28, 2023
DrahtBot removed the label Needs rebase on Nov 28, 2023
DrahtBot added the label Needs rebase on Nov 30, 2023
gades referenced this in commit 0dd12b7cc7 on Dec 9, 2023
vijaydasmp referenced this in commit ad023978ce on Dec 11, 2023
PastaPastaPasta referenced this in commit 496a279e16 on Dec 28, 2023
knst referenced this in commit 22e2d966c8 on Dec 28, 2023
PastaPastaPasta referenced this in commit 89940ae821 on Jan 1, 2024
knst referenced this in commit 756551e1c6 on Jan 11, 2024
knst referenced this in commit 327ad380ea on Jan 11, 2024
knst referenced this in commit b3b8097e52 on Jan 11, 2024
ryanofsky force-pushed on Jan 11, 2024
DrahtBot removed the label Needs rebase on Jan 12, 2024
knst referenced this in commit ceae7ad2a6 on Jan 12, 2024
knst referenced this in commit fbf1b56c02 on Jan 13, 2024
knst referenced this in commit 848a58b204 on Jan 15, 2024
knst referenced this in commit 9d43d18df8 on Jan 15, 2024
PastaPastaPasta referenced this in commit 098d0fd430 on Jan 16, 2024
DrahtBot added the label Needs rebase on Jan 23, 2024
achow101 referenced this in commit 2f218c664b on Jan 23, 2024
vijaydasmp referenced this in commit 5430b2b7fe on Feb 3, 2024
vijaydasmp referenced this in commit f54ab8fd4f on Feb 5, 2024
vijaydasmp referenced this in commit 19340bc009 on Feb 23, 2024
willcl-ark
commented at 8:29 PM on May 2, 2024:
member
I've been using this in a side-project and noticed some unintended behaviour, which could be my system; if I make distclean (removing the generated files) and then make -j16 the build fails as it can't find the built artefacts. Running plain make works fine every time, and sometimes a low job number works too.
The Makefile appears (to me) to have the dependencies listed correctly, so I'm not entirely sure what's going wrong, but just thought I'd report here in case anyone else had a similar issue. Also, likely not worth actually fixing now before a move to CMake.
vijaydasmp referenced this in commit f1ff241074 on May 6, 2024
vijaydasmp referenced this in commit f8dd9648f7 on May 6, 2024
vijaydasmp referenced this in commit 01dfa3ac0b on May 6, 2024
vijaydasmp referenced this in commit 51869ea9ba on May 6, 2024
vijaydasmp referenced this in commit cb743ed3b0 on May 7, 2024
vijaydasmp referenced this in commit a37d695dba on May 7, 2024
vijaydasmp referenced this in commit c7ae033432 on May 16, 2024
PastaPastaPasta referenced this in commit bf72bea014 on May 19, 2024
willcl-ark
commented at 10:06 AM on May 24, 2024:
member
I noticed some other unexpected behaviour when interacting with bitcoin-node from an external process which might be of interest to investigate...
I made a (malformed) call to MakeWalletLoader, not including GlobalArgs, which cause the node to shut down whilst it had an open lock on another (default loaded) wallet. This resulted in preventing this wallet from being re-opened again at next startup.
I wonder whether a malformed call to MakeWalletLoader should shut down the node at all, but if it should, then it seems like (perhaps) we should take care to close opened wallets more carefully?
I wonder whether a malformed call to MakeWalletLoader should shut down the node at all,
Thanks for reporting this. I'm confused how it happens because MakeWalletLoader should be called from the node process (via Init::makeWalletLoadermethod) and it should run in the wallet process. So if it crashes, it should kill the wallet process, not the node process. If you can post more details about what you found at https://github.com/chaincodelabs/libmultiprocess/issues/new I can look into it more.
In general, when interface methods are called with invalid parameters they can crash, because most interfaces are just thin wrappers around C++ functions designed for internal use, and many of these rely on asserts. This should not really be an issue for this PR, since this PR is just using an anonymous socketpair and not accepting external connections, but it could be a concern for #19460 which adds code to accept connections on a <datadir>/sockets/node.sock unix socket. It would be better to return errors instead of asserting when invalid parameters are passed, but implementing that will require writing more checks for bad parameters.
but if it should, then it seems like (perhaps) we should take care to close opened wallets more carefully?
By design, if the node process gets disconnected from the wallet process for any reason, all the objects in the wallet process which were owned by the node should be destroyed in mp::Connection destructor.
Wallets should be among the objects destroyed because they are owned by WalletContext, which is owned by WalletLoaderImpl, which is referenced and owned by the node process. It is possible there is a bug here, though so if you want to post an issue to the libmultiprocess github project so I can look into it more, that would be helpful. The wallet process should also completely exit after this happens because the m_num_clients reference count should go to 0.
ryanofsky force-pushed on Jun 11, 2024
DrahtBot removed the label Needs rebase on Jun 11, 2024
ryanofsky force-pushed on Jun 11, 2024
ryanofsky referenced this in commit 8da0524628 on Jun 13, 2024
ryanofsky force-pushed on Jun 13, 2024
ryanofsky
commented at 6:55 PM on June 13, 2024:
contributor
It is possible there is a bug here, though so if you want to post an issue to the libmultiprocess github project so I can look into it more, that would be helpful.
To follow up on this, I actually found two separate bugs which were causing wallet objects not to be destroyed when the node process is killed. Coincidentally a new test was added recently in #26606 which reproduces this problem, because it actually tests killing the node process. So I could fix the bugs by fixing the test. The test led to the to the following CI failure: https://cirrus-ci.com/task/4549686449668096?logs=ci#L2282 where the wallet process hangs after the node process is killed, and there is an error trying to load the wallet afterwards because the wallet directory is still locked.
The hang was caused by two separate deadlocks. One deadlock was happening because the ProxyServerCustom<WalletLoader> class which contains the CScheduler used by the wallet when the wallet in running in multiprocess mode (in single process mode the wallet shares the node's scheduler object) was trying to shut down the scheduler in its own destructor, which is called from the IPC EventLoop thread. This did not work because shutting down the scheduler required waiting for it, which would deadlock if the scheduler thread was calling any IPC method, such as chain().isReadyToBroadcast(), because calling IPC methods requires the EventLoop not to be busy. This is fixed in the latest push 3f12b4362c1b822835889151f6cd54c4829cf3ad -> c023ffefec05b94a00997d27179ea06a18edb7ea (pr/ipc.207 -> pr/ipc.208, compare). I also added some documentation about safer ways to destroy custom state in https://github.com/chaincodelabs/libmultiprocess/commit/537c645652cb98a641bb7e40ff541c89bf4ec471.
The second deadlock happened because wallet uses std::shared_ptr and waits for reference counts to reach 0 on final shutdown, so is sensitive to order objects are destroyed in. In this case, when the node process got killed, the wallet process tried to destroy WalletLoader object before destroying Chain::Notification objects, which deadlocked because WalletLoader destructor would wait for CWallet reference counts to be 0, which would never happen while Chain::Notification references to the wallet still existed. This was fixed in https://github.com/chaincodelabs/libmultiprocess/pull/101 by tweaking libmultiprocess library to destroy objects in LIFO order instead of FIFO order when connections are broken, which is still somewhat fragile, but fixes this problem and should work better in most cases.
DrahtBot removed the label CI failed on Jun 13, 2024
willcl-ark
commented at 1:18 PM on June 14, 2024:
member
Wow, sounds sounds like exactly the bug I hit @ryanofsky, great debugging; sorry I couldn't provide any followup for you sooner.
I'll rebase my ipc + -ipcconnect branch on this one, update my libmultiprocess installation, and see how it goes from there.
Thanks!
DrahtBot added the label Needs rebase on Jun 25, 2024
knst referenced this in commit 7303854d12 on Jul 12, 2024
knst referenced this in commit 4b5b54e290 on Jul 12, 2024
knst referenced this in commit 91d46b9bdf on Jul 12, 2024
knst referenced this in commit ecc2e2cdf8 on Jul 12, 2024
knst referenced this in commit 95fbf7d9c3 on Jul 13, 2024
knst referenced this in commit 84b33517e3 on Jul 13, 2024
knst referenced this in commit 63c5d342ba on Jul 15, 2024
knst referenced this in commit 1c917c76f0 on Jul 15, 2024
knst referenced this in commit c7413d72b1 on Jul 15, 2024
knst referenced this in commit 2f7814acdd on Jul 15, 2024
knst referenced this in commit 6e5c07a4d1 on Jul 16, 2024
knst referenced this in commit 56dd686404 on Jul 16, 2024
knst referenced this in commit fea67f4f66 on Jul 23, 2024
knst referenced this in commit 9002342f94 on Jul 23, 2024
knst referenced this in commit 25e7d5446e on Jul 23, 2024
knst referenced this in commit a08b66693f on Jul 23, 2024
knst referenced this in commit 040c188de2 on Jul 24, 2024
knst referenced this in commit c5b5b927af on Jul 24, 2024
knst referenced this in commit 172d9d0be4 on Jul 24, 2024
ryanofsky referenced this in commit ca2cfedd2b on Jul 24, 2024
ryanofsky referenced this in commit a9e16da55e on Jul 24, 2024
ryanofsky force-pushed on Jul 26, 2024
DrahtBot removed the label Needs rebase on Jul 26, 2024
knst referenced this in commit 3411577473 on Jul 27, 2024
knst referenced this in commit 94d5230b18 on Aug 3, 2024
knst referenced this in commit a7993de34b on Aug 3, 2024
knst referenced this in commit 382047c971 on Aug 7, 2024
knst referenced this in commit aa2d3b8164 on Aug 7, 2024
knst referenced this in commit 5b0b30e7be on Aug 7, 2024
PastaPastaPasta referenced this in commit 9d9d97d4fd on Aug 14, 2024
hebasto added the label Needs CMake port on Aug 16, 2024
knst referenced this in commit 1087849955 on Aug 27, 2024
DrahtBot added the label Needs rebase on Aug 28, 2024
maflcko removed the label Needs CMake port on Aug 29, 2024
vijaydasmp referenced this in commit 71056b45da on Sep 5, 2024
vijaydasmp referenced this in commit d76cd26f15 on Sep 6, 2024
vijaydasmp referenced this in commit 68a18017d0 on Sep 6, 2024
knst referenced this in commit aebd95938b on Sep 6, 2024
vijaydasmp referenced this in commit e94a654385 on Sep 8, 2024
achow101 referenced this in commit df3f63ccfa on Sep 9, 2024
ryanofsky force-pushed on Sep 19, 2024
DrahtBot added the label CI failed on Sep 19, 2024
DrahtBot
commented at 12:12 PM on September 19, 2024:
contributor
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.
</details>
DrahtBot removed the label Needs rebase on Sep 19, 2024
vijaydasmp referenced this in commit d7a20b3ee6 on Sep 25, 2024
DrahtBot added the label Needs rebase on Sep 25, 2024
ryanofsky force-pushed on Sep 26, 2024
DrahtBot removed the label Needs rebase on Sep 26, 2024
DrahtBot added the label Needs rebase on Nov 21, 2024
darosior
commented at 9:08 PM on December 3, 2024:
member
Not sure if it's worth sharing here but hey what's an 1300th comment.
Running bitcoin-node from #29409 with commits 1045047a73a0b3ae15647ab5bf1829d27141b7cd..9ca113f326fc9ba2f661595283f4aef390df984d from this PR cherry-picked on top.
Simply compiled with cmake -B multiprocbuild/ -DWITH_MULTIPROCESS=ON && cmake --build multiprocbuild/ -j20. (Although i did have to apply the following diff to be able to compile on Debian stable.)
<details>
<summary>Expand diff to compile on Debian stable</summary>
diff --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(); }); };
</details>
Just starting bitcoin-node and stopping it with a SIGINT results in the following crash:
ryanofsky
commented at 8:37 PM on December 4, 2024:
contributor
Rebased a295345d3c746d0acdf531391ced5867d6b5e493 -> 5b9a95396595c8a5bdafd221f44c2a69b0327f55 (pr/ipc.211 -> pr/ipc.212, compare) fixing minor conflicts with #24937 and #31335, and silent conflict with #31130
Updated 5b9a95396595c8a5bdafd221f44c2a69b0327f55 -> b5b0664379c1fd848d6652c22181b3055805bb11 (pr/ipc.212 -> pr/ipc.213, compare) to fix silent conflict in feature_config_args test with #31212 and bad change order causing test-each-commit failure
Rebased b5b0664379c1fd848d6652c22181b3055805bb11 -> b57c89793ec03c506a9b6db35dc2b5cbe28c6c8b (pr/ipc.213 -> pr/ipc.214, compare) on top of updated #29409 pr/ipc-chain.12 to fix wallet_backup.py test failure
DrahtBot removed the label Needs rebase on Dec 5, 2024
ryanofsky force-pushed on Dec 5, 2024
ryanofsky force-pushed on Dec 6, 2024
DrahtBot removed the label CI failed on Dec 6, 2024
DrahtBot added the label Needs rebase on Jan 29, 2025
ryanofsky referenced this in commit a92544c43b on Feb 3, 2025
ryanofsky referenced this in commit 056d18134a on Feb 4, 2025
ryanofsky referenced this in commit b6b06bbf5b on Feb 4, 2025
ryanofsky referenced this in commit b75e13e6ec on Feb 5, 2025
ryanofsky referenced this in commit 83e375add8 on Feb 5, 2025
knst referenced this in commit 78bd5b9be2 on Feb 7, 2025
ryanofsky referenced this in commit e93de82e80 on Feb 7, 2025
ryanofsky referenced this in commit 8cde5419f4 on Feb 7, 2025
ryanofsky referenced this in commit e3a79b4d67 on Feb 7, 2025
ryanofsky referenced this in commit aa2745b603 on Feb 7, 2025
ryanofsky referenced this in commit be6ad9f955 on Feb 10, 2025
ryanofsky referenced this in commit f8cbd0d34a on Feb 10, 2025
ryanofsky referenced this in commit 61bf9456f9 on Feb 10, 2025
ryanofsky referenced this in commit 48d01bcca7 on Feb 10, 2025
ryanofsky referenced this in commit 26b9f3dda4 on Feb 10, 2025
ryanofsky referenced this in commit 445106b202 on Feb 10, 2025
ryanofsky referenced this in commit 7d358574fa on Feb 10, 2025
ryanofsky referenced this in commit 97e2b5962f on Feb 10, 2025
ryanofsky referenced this in commit 9b3e050ebc on Feb 10, 2025
knst referenced this in commit 442b2145d9 on Feb 10, 2025
knst referenced this in commit 1a8cf9a58b on Feb 11, 2025
knst referenced this in commit c29f50cabe on Feb 11, 2025
knst referenced this in commit c409339382 on Feb 11, 2025
knst referenced this in commit 2d2da7a7d7 on Feb 11, 2025
ryanofsky referenced this in commit 8eccd5322d on Feb 13, 2025
ryanofsky referenced this in commit 82fc41a73a on Feb 13, 2025
PastaPastaPasta referenced this in commit 029572d4ab on Feb 17, 2025
ryanofsky referenced this in commit f7565fced6 on Feb 24, 2025
ryanofsky referenced this in commit f75b7d871e on Feb 24, 2025
ryanofsky referenced this in commit 674f750158 on Feb 25, 2025
ryanofsky referenced this in commit 7b59a9275f on Feb 25, 2025
ryanofsky referenced this in commit d61d7e8ddf on Feb 25, 2025
ryanofsky referenced this in commit cd89b2d9ee on Feb 25, 2025
ryanofsky referenced this in commit a5df9b2c0b on Mar 11, 2025
ryanofsky referenced this in commit 25c6abe2ea on Mar 11, 2025
ryanofsky referenced this in commit e2c3f0ded0 on Mar 12, 2025
ryanofsky referenced this in commit dbf09be1eb on Mar 12, 2025
ryanofsky referenced this in commit 6ec6f4ba5f on Mar 17, 2025
ryanofsky referenced this in commit b673f5d0a3 on Mar 17, 2025
jimhashhq
commented at 10:58 PM on March 17, 2025:
none
I tried this multiprocess feature out (using clang/Linux) as follows:
Full build multiprocess with gui of bitcoin/master (with CMAKE_PREFIX_PATH correctly pointing to a recent external local build of master on libmultiprocess).
Full build multiprocess with gui of Sjors/02/25/ipc-yea (the multiprocess feature integration branch with subtree).
I can confirm that this latter build was indeed simpler with libmultiprocess as a project subtree.
Interactive regtest ran fine for both, and I did not experience any shutdown issues mentioned much earlier in this discussion either.
Experimenting with both builds, I made the following observations of possible issues:
The planned --ipcconnect is not yet supported by either bitcoin-gui or bitcoin-wallet processes?
Running a bitcoin-gui (w/o --ipcconnect) does not "spawn" a node process? (#19160) was merged long ago, but I cannot get either of the above builds of bitcoin-gui to actually "spawn" a bitcoin-node process?
Running bitcoin-wallet concurrent with either a bitcoin-node or bitcoin-gui fails with error:
SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of Bitcoin Core?
Running bitcoin-gui --ipcbind:unix creates a gui.sock not a node.sock file in <data-dir>/regtest directory.
bitcoin-gui default logfile is debug.log and not debug.log.gui as indicated in comments above.
In general is it true that the plan is to merge the libmultiprocess project as a subtree (#31740 and #31741)
before merging main functionality of multiprocess feature itself (#19460 and #19461)?
I was expecting the opposite order. Maybe rationale is that the latter two need a rebase.
If those get rebased, I would be interested to test them. Thanks so much for any guidance here.
ryanofsky referenced this in commit 2e591a353a on Mar 18, 2025
ryanofsky referenced this in commit 002088280f on Mar 18, 2025
ryanofsky
commented at 3:11 PM on March 18, 2025:
contributor
The planned --ipcconnect is not yet supported by either bitcoin-gui or bitcoin-wallet processes?
It is implemented in #19460 for bitcoin-wallet (where it just connects and doesn't really do anything) and in #19461 for bitcoin-gui where it is more functional and does allow the gui to connect and control the node.
If you use this PR or the PR's building on it (#10102, #19460, #19461) bitcoin-gui will spawn a bitcoin-node process and bitcoin-node will also spawn a bitcoin-wallet process. #19160 added support for spawning processes, but only made use of them in an echoipc test command.
Running bitcoin-wallet concurrent with either a bitcoin-node or bitcoin-gui fails with error:
SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of Bitcoin Core?
It is expected for only one process to be able to open a wallet at time, so this may or may not be a bug depending on what you are trying to do.
Running bitcoin-gui --ipcbind:unix creates a gui.sock not a node.sock file in <data-dir>/regtest directory.
That is unexpected and not something I see running my current branch with:
build/bin/bitcoin-gui -regtest -debug=ipc -printtoconsole -ipcbind=unix
ls -al ~/.bitcoin/regtest/*.sock
I can update this PR though in case maybe this was a transient problem.
bitcoin-gui default logfile is debug.log and not debug.log.gui as indicated in comments above.
That's also not something I'm seeing (ls -al ~/.bitcoin/regtest/*.log*) but I'll update this PR in case that can fix the problem or provide a newer baseline to reproduce it.
In general is it true that the plan is to merge the libmultiprocess project as a subtree (#31740 and #31741) before merging main functionality of multiprocess feature itself (#19460 and #19461)? I was expecting the opposite order. Maybe rationale is that the latter two need a rebase. If those get rebased, I would be interested to test them. Thanks so much for any guidance here.
I'll go ahead and update these. The subtree and multiprocess PRs could be merged in any order since building with a subtree instead of an external library is not even a code change, just a build system change. #31741 is supposed to make it easier for developers to test this functionality so it is easier to review IPC PRs and build more confidence that they work well and are ready to be released.
DrahtBot added the label CI failed on Mar 18, 2025
jimhashhq
commented at 1:19 AM on March 19, 2025:
none
Thank you @ryanofsky, I am in favor of a multiprocess Bitcoin, I find this a well considered change. I am interested to try the updated #19461, thank you, and I do apologize for all of the questions.
From comments, it sounds like 19461 is closest, and provides the most functionality for the feature; would it make any sense to maybe change release order, putting 19461 (and a more complete #19460?) ahead of #31740 and #31741?
In general, RE cmake and external packages, how packages resolve seems not obvious to me anyways, maybe adding a comment to the libmultiprocess project docs somewhere about setting CMAKE_INSTALL_PREFIX (say $HOME/local/libmultiprocess, for example) and a corresponding comment in the bitcoin multiprocess docs about how to set CMAKE_PREFIX_PATH to point below that ($HOME/local/libmultiprocess/lib/cmake) so the bitcoin build correctly finds it might help developers some with existing external build challenges? And/or enhance error messaging (#31709) in top CMakeList.txt to probe if (NOT LibMultiprocess_FOUND) and provide this suggestion.
Should project owners maybe create a MULTIPROCESS label for tracking?
I've been getting design info from multiprocess.md, which might be outdated.
I see no mention of an -ipcfd=<n> (e2c3f0d), does multiprocess.md need to be updated to describe this?
Also, I was expecting "node" to be more of a server in the old fashioned sense, and both "gui" and "wallet" to be more like clients to it, which makes me confused as to why the "node" would spawn a wallet (9648a377)?
ryanofsky
commented at 5:39 PM on March 19, 2025:
contributor
From comments, it sounds like 19461 is closest, and provides the most functionality for the feature; would it make any sense to maybe change release order, putting 19461 (and a more complete #19460?) ahead of #31740 and #31741?
There isn't really a predefined order between #19461 and #31741 and the changes do not overlap so they they could be reviewed, tested, and merged in either order. The merge order will just depend on where people want to spend effort reviewing and testing. I expect #31741 will get merged first just because it has been discussed more recently and will make other PRs easier to test. #31741 also doesn't change code or provide any features, it only affects the build system.
In general, RE cmake and external packages, how packages resolve seems not obvious to me anyways, maybe adding a comment to the libmultiprocess project docs somewhere about setting CMAKE_INSTALL_PREFIX (say $HOME/local/libmultiprocess, for example) and a corresponding comment in the bitcoin multiprocess docs about how to set CMAKE_PREFIX_PATH to point below that ($HOME/local/libmultiprocess/lib/cmake) so the bitcoin build correctly finds it might help developers some with existing external build challenges? And/or enhance error messaging (#31709) in top CMakeList.txt to probe if (NOT LibMultiprocess_FOUND) and provide this suggestion.
It sounds like you have some good ideas here, maybe you would like to open a PR to improve the documentation or current errors from cmake? I would be happy to review and help edit and I think changes like these could be merged pretty easily. It is true that #31741 should also address some of these difficulties by simplifying required build steps, but would be good to improve documentation and error handling either way.
Should project owners maybe create a MULTIPROCESS label for tracking?
I've been using #28722 to track related PRs so that might be helpful. If you have specific ideas about how labels could be useful, they could be added too.
I've been getting design info from multiprocess.md, which might be outdated. I see no mention of an -ipcfd=<n> (e2c3f0d), does multiprocess.md need to be updated to describe this?
That's a good suggestion, and again would welcome a PR, and am happy to help review and provide feedback on a draft. The document is up to date, but it doesn't currently cover details about how processes are spawned and connections are established, focusing more on how IPC calls work after connections are established.
There isn't too much that can be said about the -ipcfd parameter specifically. Before a parent process spawns a child process it just calls socketpair to create a bidirectional socket, and passes one of the two socket file descriptors as an -ipcfd=<fd> argument to the child process so the parent and child process can communicate over the socket. The -ipcfd option isn't described in the command line documentation because it isn't meant to be used externally. Also it will have slightly different behavior on windows where it will contain the path to a named pipe, and the then the child process will create a socket and call WSADuplicateSocket to send one side of the socket to the parent over the named pipe.
Also, I was expecting "node" to be more of a server in the old fashioned sense, and both "gui" and "wallet" to be more like clients to it, which makes me confused as to why the "node" would spawn a wallet (9648a37)?
Yes this is the goal which #19460 and #19461 take steps towards implementing. #19461 achieves that goal for the gui so it can connect to the node and control it, while #19460 starts to achieve that goal for the wallet, although it is more incomplete and just connects to the node without offering any real functionality. The point of the gui spawning a node process and the node spawning a wallet process is mostly just for convenience. In #19461 for example, the gui will skip spawning a node process if a node process is already running that it can connect to.
ryanofsky force-pushed on Mar 19, 2025
ryanofsky
commented at 8:50 PM on March 19, 2025:
contributor
ryanofsky removed the label Needs rebase on Mar 19, 2025
DrahtBot removed the label CI failed on Mar 19, 2025
jimhashhq
commented at 12:27 AM on March 20, 2025:
none
Good news -- with your rebase of #19461, I am now able to try -ipcconnect, thank you.
In this multiprocess taxonomy, for separation-of-concerns and or separation-of-roles support, I was expecting that the default would be for the nodenot to start a wallet at all, instead having the gui start the wallet in addition to the node that it already starts?
I vote somebody create some sort of MULTIPROCESS label for issue tracking if that makes sense.
Describing CMAKE_PREFIX_PATH or enhancing error messaging for the external libmultiprocess package is probably obviated by planned earlier merge into internal subtree.
Thank you for your guidance, I will try to experiment with -ipcconnect & -ipcbind next week and report back.
ryanofsky
commented at 3:22 PM on March 20, 2025:
contributor
In this multiprocess taxonomy, for separation-of-concerns and or separation-of-roles support, I was expecting that the default would be for the nodenot to start a wallet at all, instead having the gui start the wallet in addition to the node that it already starts?
What actually happens in this PR is that the gui starts a node process and the node starts a wallet process. This is done to keep this PR simple and to preserve current UX. We can change the node not to load wallets by default, and change wallets to connect directly through to the instead of going through the node, but some of these these changes would be user-facing, and all of them would be changes to gui, wallet, and node code. This PR is not changing gui, node, and wallet code, just making the existing code run in different processes instead of the same process. #19460 and #19461 start to build new features you might want, and more should be possible as well.
I vote somebody create some sort of MULTIPROCESS label for issue tracking if that makes sense.
Open to the idea. I use #28722 for tracking personally.
Describing CMAKE_PREFIX_PATH or enhancing error messaging for the external libmultiprocess package is probably obviated by planned earlier merge into internal subtree.
Yes these things could still be useful but should be less necessary.
Thank you for your guidance, I will try to experiment with -ipcconnect & -ipcbind next week and report back.
Thanks! it's very useful to have more testing.
ryanofsky force-pushed on Mar 24, 2025
ryanofsky
commented at 9:49 PM on March 24, 2025:
contributor
Rebased 26c2136a2bc7fec9697f61eadf422c439e0735a2 -> c9a49633a027f1c28ebe15c0134f1ce38b712e8a (pr/ipc.216 -> pr/ipc.217, compare) due to silent conflict with #31519
Rebased c9a49633a027f1c28ebe15c0134f1ce38b712e8a -> c1e070d76166f1230758bdcf7a4463008eea46d9 (pr/ipc.217 -> pr/ipc.218, compare) due to various conflicts
<!-- begin push-219 -->
Rebased c1e070d76166f1230758bdcf7a4463008eea46d9 -> 7634a40a8fb0ecd55af802b29955a22c2f87f372 (pr/ipc.218 -> pr/ipc.219, compare)<!-- end -->
<!-- begin push-220 -->
Updated 7634a40a8fb0ecd55af802b29955a22c2f87f372 -> faac28b45aa852eebc0e6b316ccab3f5e4babbe8 (pr/ipc.219 -> pr/ipc.220, compare)<!-- end -->
Rebased 502cd8d3d0abbab54b745968165d105bd0a4cef4 -> 55ce4bae1881669c7108f2c54666d9d3347475fc (pr/ipc.221 -> pr/ipc.222, compare)<!-- end --> due to conflict with #33229
Rebased d902de13f038748192064bb8879b3d02dddaefbc -> 46beb574cc3a41849f253f88f9b8ef63cb080b29 (pr/ipc.224 -> pr/ipc.225, compare)<!-- end --> due to conflict with #33003
<!-- begin push-226 -->
Rebased 46beb574cc3a41849f253f88f9b8ef63cb080b29 -> 3638b89090f539c360c5d4d358c4ed17db59c25e (pr/ipc.225 -> pr/ipc.226, compare)<!-- end -->
<!-- begin push-227 -->
Rebased 3638b89090f539c360c5d4d358c4ed17db59c25e -> 5010acc3d6e42cce5da8fad2b6c42eb31441c387 (pr/ipc.226 -> pr/ipc.227, compare)<!-- end -->
<!-- begin push-228 -->
Rebased 5010acc3d6e42cce5da8fad2b6c42eb31441c387 -> 2503f57e93c32158f837e1c9f0dc9f1061b84346 (pr/ipc.227 -> pr/ipc.228, compare)<!-- end --> on top of #29409 pr/ipc-chain.24
<!-- begin push-229 -->
Rebased 2503f57e93c32158f837e1c9f0dc9f1061b84346 -> cba1e840335580fd7dcdd337e3fb2a00ec3f3c8d (pr/ipc.228 -> pr/ipc.229, compare)<!-- end --> on top of #29409 pr/ipc-chain.28
<!-- begin push-230 -->
Rebased cba1e840335580fd7dcdd337e3fb2a00ec3f3c8d -> d8fdc46a9f92f962d4f7dd72d2434e19efd57ca6 (pr/ipc.229 -> pr/ipc.230, compare)<!-- end --> on top of #29409 pr/ipc-chain.30
Try to run the tests locally, according to the documentation. However, a CI failure may still
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.
</details>
knst referenced this in commit 40f088814f on Nov 21, 2025
knst referenced this in commit e94f69d75a on Dec 3, 2025
knst referenced this in commit 0b699aa659 on Dec 3, 2025
DrahtBot added the label Needs rebase on Dec 4, 2025
knst referenced this in commit b6c29a76d4 on Dec 4, 2025
knst referenced this in commit e48c12e2ac on Dec 4, 2025
knst referenced this in commit 4889b96a1e on Dec 5, 2025
knst referenced this in commit 19aed94085 on Dec 5, 2025
ryanofsky force-pushed on Dec 12, 2025
DrahtBot removed the label Needs rebase on Dec 12, 2025
ryanofsky force-pushed on Dec 16, 2025
ryanofsky force-pushed on Jan 7, 2026
DrahtBot removed the label CI failed on Jan 7, 2026
DrahtBot added the label Needs rebase on Feb 4, 2026
ryanofsky force-pushed on Mar 2, 2026
DrahtBot removed the label Needs rebase on Mar 2, 2026
DrahtBot added the label CI failed on Mar 2, 2026
DrahtBot added the label Needs rebase on Mar 3, 2026
luke-jr referenced this in commit 365dfb2c95 on Mar 24, 2026
luke-jr referenced this in commit a9f11b76fb on Mar 24, 2026
Squashed 'src/ipc/libmultiprocess/' changes from 70f632bda8f..6d796edac65
6d796edac65 tidy fix: modernize-use-nullptr
c206f8328c7 mpgen: support primitive std::optional struct fields
46df71115ed mpgen refactor: add AccessorType function
a4b85dabd84 mpgen refactor: Move field handling code to FieldList class
git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: 6d796edac652292a020bd976cffaa866c9d140f4
c4c677ce91
Merge commit 'c4c677ce917b418249e915b9f67663e75a5a6847' into pr/ipc-chain5467330635
Add capnp serialization code for bitcoin types
- Add capnp ToBlob, ToArray, Wrap, Serialize, and Unserialize helper functions
- Add support for std::chrono::seconds capnp serialization
- Add support for util::Result capnp serialization
2312d50a72
Add capnp wrapper for Handler interface5b6d662d48
Add capnp wrapper for Chain interface1c9dcf2f39
multiprocess: Expose Chain interface
Expose Chain interface to external processes spawning or connecting to
bitcoin-node.
6833d5f2b4
Merge branch 'pr/ipc-chain' into pr/ipcfb04f4f44d
test: Increase feature_block.py and feature_taproot.py timeouts
Needed because BlockConnected notifications are a lot slower with the wallet
running in separate process.
88d808a21c
test: Fix multiprocess test for unclean shutdown on killf1e44dd622
util: Add util::Result workaround to be compatible with libmultiprocess
Make default constructor more generic so it doesn't only work with void types.
db547dd306
multiprocess: Add capnp serialization code for bitcoin typesd31f8b83ff
interfaces, refactor: Change WalletLoader::restoreWallet parameter order
Libmultiprocess requires output parameters to be ordered after input parameter,
so move warnings parameter last. Problematic order was introduced in
4ec2d18a0734f44c0a74f05b59ad1269d323dfdb from
https://github.com/bitcoin-core/gui/pull/877
a56861a481
multiprocess: Add capnp wrapper for Wallet interface1a512710ca
multiprocess: Add capnp wrapper for Node interface269afc10c0
multiprocess: Make bitcoin-gui spawn a bitcoin-node process
Spawn node subprocess instead of running node code internally
6155c8218e
multiprocess: Make bitcoin-node spawn a bitcoin-wallet process
Spawn wallet subprocess instead of running wallet code internally
783b01f5da
multiprocess: Add debug.log .wallet/.gui suffixes
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.
f15fe6936c
doc: Multiprocess misc doc and comment updatesbcef963a0c
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2026-05-02 12:15 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me