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 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:757
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:33
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"));
683684 // 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
HashUnlimited referenced this in commit
0950e8ac40
on Mar 2, 2018
ryanofsky force-pushed
on Mar 2, 2018
HashUnlimited 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:115
in
006286e731outdated
104@@ -105,7 +105,7 @@ To build without GUI pass `--without-gui`.
105106 To build with Qt 5 you need the following:
107108- 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:
0./configure --disable-bench --disable-zmq --with-miniupnpc=no --with-incompatible-bdb --with-qrencode
1....
2Options used to compile and link:
3 multiprocess = yes
I do still see a few warnings that appear related to this change:
0interfaces/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]
1 return strprintf("%s-%i/%s-%i", exe_name ? exe_name : "", getpid(), thread_name, int(syscall(SYS_gettid)));
2 ^
3/usr/include/unistd.h:745:6: note: 'syscall' has been explicitly marked deprecated here
4int syscall(int, ...);
5 ^
61 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:
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:
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.
1516-* [`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:
0 CXX interfaces/capnp/libbitcoin_util_a-messages.o
1In file included from ../../src/interfaces/capnp/proxy.h:6:0,
2 from ../../src/interfaces/capnp/messages.h:5,
3 from ./interfaces/capnp/messages.capnp.proxy.h:6,
4 from ../../src/interfaces/capnp/messages-impl.h:9,
5 from ../../src/interfaces/capnp/messages.cpp:1:
6../../src/interfaces/capnp/proxy.h: In instantiation of ‘struct interfaces::capnp::ListOutput<capnp::List<interfaces::capnp::messages::Pair<capnp::Text, capnp::List<capnp::Text>>>>’:
7../../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>&]’ 8../../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>&]’ 9../../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>&]’10../../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]’11../../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]’12../../src/interfaces/capnp/messages.cpp:52:109: required from here
13../../src/interfaces/capnp/proxy.h:322:110: error: ‘B’ is not a classor namespace
14 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
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 values, multiple error and warning messages 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.
Sjors
commented at 10:29 am on September 28, 2018:
member
After upgrading to Mojave I’m having compile issues (as of ededfe8800fd2647851d4d42dd765cfcd8d69cde):
0In file included from interfaces/capnp/proxy-codegen.cpp:4:
1In file included from /usr/local/Cellar/capnp/0.7.0/include/capnp/blob.h:28:
2/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." 3#error "This code requires C++14. Either your compiler does not support it or it is not enabled." 4 ^
5/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." 6#error "Pass -std=c++14 on the compiler command line to enable C++14." 7 ^
8In file included from interfaces/capnp/proxy-codegen.cpp:5:
9In file included from /usr/local/Cellar/capnp/0.7.0/include/capnp/schema-parser.h:30:
10In file included from /usr/local/Cellar/capnp/0.7.0/include/kj/filesystem.h:28:
11/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
12 auto operator()(Params&&... params){13 ^
14/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
15 auto operator()(Params&&... params) const {16 ^
17interfaces/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 0:00 am on November 2, 2018:
none
practicalswift
commented at 3:54 pm on November 2, 2018:
Remove trailing ; :-)
in
src/interfaces/init.h:30
in
ee425e2ddfoutdated
25+ virtual IpcProcess* getProcess() { return nullptr; };
26+ virtual IpcProtocol* getProtocol() { return nullptr; };
27+};
28+
29+//! Return implementation of Init interface.
30+//! @param exe_path should be current executable path (argv[0])
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
in
src/interfaces/capnp/test/foo.cpp:13
in
ee425e2ddfoutdated
9+
10+class FooImpl : public FooInterface
11+{
12+public:
13+ int add(int a, int b) override { return a + b; }
14+ int mapSize(const std::map<std::string, std::string>& map) override { return map.size(); }
practicalswift
commented at 4:07 pm on November 2, 2018:
Not used?
ryanofsky
commented at 7:27 pm on November 12, 2018:
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:6
in
ee425e2ddfoutdated
0@@ -0,0 +1,22 @@
1+#include <interfaces/base.h>
practicalswift
commented at 4:08 pm on November 2, 2018:
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:1859
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:1862
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:396
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
0 CXX wallet/libbitcoin_wallet_tool_a-wallettool.o
1 GEN interfaces/capnp/test/foo.capnp.c++ 2*** Uncaught exception *** 3kj/filesystem.c++:315: failed: expected parts.size() >0; can't use ".." to break out of starting directory 4stack: 7ffbcf3c71a0 7ffbcf3c9829 4098e37ffbcf55904d 7ffbcf559082 7ffbcf53208f 7ffbcf534296 7ffbcf5510a4 7ffbcf558893 7ffbcf558901 7ffbcf558c57 7ffbcf532974 7ffbcf532724 7ffbcf533921 7ffbcf5399fc 7ffbcf542aa5 7ffbcf5436d6 7ffbcf555452 7ffbcf55662a 7ffbcf55679f 7ffbcf55a606 7ffbcf55aa53 4124664127707ffbcf3e4c28 7ffbcf3e5d1a
5make[2]: *** [Makefile:17337: interfaces/capnp/test/foo.capnp.c++] Error 1 6make[2]: *** Waiting for unfinished jobs.... 7Generated test/data/base58_encode_decode.json.h
8Generated test/data/key_io_invalid.json.h
9Generated test/data/blockfilters.json.h
10Generated test/data/key_io_valid.json.h
11In file included from interfaces/capnp/config_bitcoin-node.cpp:1:0:
12./interfaces/capnp/config.h:4:10: fatal error: interfaces/capnp/node.capnp.h: No such file or directory
13#include <interfaces/capnp/node.capnp.h>14^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~15compilation terminated.16make[2]: *** [Makefile:12585: interfaces/capnp/bitcoin_node-config_bitcoin-node.o] Error 117In file included from interfaces/capnp/config_bitcoin-wallet.cpp:1:0:
18./interfaces/capnp/config.h:4:10: fatal error: interfaces/capnp/node.capnp.h: No such file or directory
19#include <interfaces/capnp/node.capnp.h>20^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
err 2
0 CXX interfaces/capnp/proxy_codegen-proxy-codegen.o
1In file included from /nix/store/69fkv7ql25r8j496bbchnsvbp42izv8q-capnproto-0.7.0/include/capnp/blob.h:28:0,
2 from interfaces/capnp/proxy-codegen.cpp:4: 3/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." 4#error "This code requires C++14. Either your compiler does not support it or it is not enabled."
5^~~~~ 6/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." 7#error "Pass -std=c++14 on the compiler command line to enable C++14."
8^~~~~ 9interfaces/capnp/proxy-codegen.cpp:10:10: fatal error: interfaces/capnp/proxy.capnp.h: No such file or directory
10#include<interfaces/capnp/proxy.capnp.h>11^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
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:
0$ ./src/bitcoin-node -testnet
1...
22020-03-05T11:21:53Z [init] Shutdown: In progress...
32020-03-05T11:21:53Z [addcon] addcon thread exit
4terminate called after throwing an instance of 'std::logic_error'
5 what(): clientInvoke call made after disconnect
6Aborted (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
HashUnlimited referenced this in commit
e88645f092
on Apr 17, 2020
HashUnlimited referenced this in commit
9746ac3b44
on Apr 17, 2020
HashUnlimited referenced this in commit
8f3b970b3f
on Apr 17, 2020
HashUnlimited 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:92
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:
0^C
1libc++abi.dylib: terminating with uncaught exception of type interfaces::IpcException: kj::Exception: capnp/rpc.c++:2092: disconnected: Peer disconnected.
2stack: 1084bf30a 106e09f40 106e0c090`
If I stop early it’s also not very happy:
02020-06-02T14:06:07Z Using obfuscation key for /Users/sjors/Library/Application Support/Bitcoin/testnet3/blocks/index: 00000000000000001^C2020-06-02T14:06:10Z Shutdown requested. Exiting.
23Assertion 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
0/home/user/Bitcoin/bitcoin/src/ipc/capnp/node.cpp:112: undefined reference to `mp::ProxyClient<ipc::capnp::messages::Node>::customWalletClient()'1/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':2/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()'3/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
0bitcoin-wallet -noipcconnect -wallet=mywallet listtransactions # List limited transaction info1bitcoin-wallet -ipcconnect -wallet=mywallet listtransactions # List full transaction info2bitcoin-wallet -ipcconnect=auto -wallet=mywallet listtransactions # List available transaction info34bitcoin-wallet -noipcconnect -wallet=mywallet serve # Run limited RPC server5bitcoin-wallet -ipcconnect -wallet=mywallet serve # Run full RPC server6bitcoin-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:44
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.
eae99d6 That seems a different Bitcoin Core PR… however there have been a few VectorReader merges recently.
Thanks, switched to SpanReader which was introduced in #23653.
in
src/ipc/capnp/handler.capnp:5
in
0a2a04655aoutdated
0@@ -0,0 +1,16 @@
1+# Copyright (c) 2021 The Bitcoin Core developers
2+# Distributed under the MIT software license, see the accompanying
3+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+@0xebd8f46e2f369076;
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.
0./interfaces/chain.h:227:18: note: overridden virtual function is here
1 virtual void initWarning(const bilingual_str& message) =0;
2^3In file included from ipc/capnp/node.cpp:9:
4In file included from ./ipc/capnp/node-types.h:11:
5In file included from ./ipc/capnp/wallet.capnp.proxy-types.h:6:
6In file included from ./ipc/capnp/wallet.capnp.proxy.h:7:
7In file included from ./ipc/capnp/wallet.h:9:
8./ipc/capnp/chain.capnp.proxy.h:1414:26: warning: 'initError' overrides a member function but is not marked 'override' [-Wsuggest-override]
9 typename M35::Result initError(M35::Param<0> message);
Also a bunch of these:
0/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]
1 __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:
02022-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")
12022-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:
02022-01-14T14:45:54Z [main] GUI: Qt has caught an exception thrown from an event handler. Throwing
1exceptions from an event handler is not supported in Qt.
2You must not let any exception whatsoever propagate through Qt code.
3If that is not possible, in Qt 5 you must at least reimplement
4QCoreApplication::notify() and catch all exceptions there.
5 62022-01-14T14:45:54Z [main]
7 8************************
9EXCEPTION: N3ipc9ExceptionE
10kj::Exception: capnp/rpc.c++:2413: disconnected: Peer disconnected.
11stack: 10d1734ea 10d184242 10cd33590 10cd33ec0
12bitcoin 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:363
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
uvhw referenced this in commit
47d44ccc3e
on Feb 14, 2022
in
src/net_processing.h:41
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:
contributor
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:
It does still show labels for “sent to”.
I still get a crash when generating a taproot address in the GUI:
02022-09-09T13:12:37Z [main] GUI: AddressTablePriv::updateEntry: Warning: Got CT_NEW, but entry is already in model
1libc++abi: terminating with uncaught exception of type std::out_of_range: map::at: key not found
2libc++abi: terminating with uncaught exception of type ipc::Exception: kj::Exception: capnp/rpc.c++:2092: disconnected: Peer disconnected.
3stack: 104d0b010 10181cab0 10181ced04zsh: 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
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:
0 File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/wallet_backup.py", line 217, in run_test
1 assert_equal(res0_rpc.getbalance(), balance0)
2 File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/util.py", line 56, in assert_equal
3 raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
4AssertionError: 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 0: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?
02024-05-22T13:49:06Z [ipc] {bitcoin-wallet-3127478/bitcoin-wallet-3127478} IPC server post request [#3075](/bitcoin-bitcoin/3075/) {bitcoin-wallet-3127478/bitcoin-wallet-3127902 (from bitcoin-node-3127476/b-scheduler-3127477)} 12024-05-22T13:49:06Z [ipc] {bitcoin-wallet-3127478/bitcoin-wallet-3127478} IPC server send response [#3075](/bitcoin-bitcoin/3075/) ChainNotifications.transactionAddedToMempool$Results () 22024-05-22T13:49:06Z [ipc] {bitcoin-node-3127476/b-scheduler-3127477} IPC client recv ChainNotifications.transactionAddedToMempool$Results ()
32024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server recv request [#201](/bitcoin-bitcoin/201/) Init.construct$Params () 42024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server send response [#201](/bitcoin-bitcoin/201/) Init.construct$Results (threadMap = <external capability>) 52024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server recv request [#202](/bitcoin-bitcoin/202/) Init.makeEcho$Params (context = (thread = <external capability>)) 62024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server post request [#202](/bitcoin-bitcoin/202/) {bitcoin-node-3127476/b-capnp-loop-3210934 (from )} 72024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server send response [#202](/bitcoin-bitcoin/202/) Init.makeEcho$Results (result = <external capability>) 82024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server recv request [#203](/bitcoin-bitcoin/203/) Echo.echo$Params (context = (thread = <external capability>), echo = "Hello, world!") 92024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server post request [#203](/bitcoin-bitcoin/203/) {bitcoin-node-3127476/b-capnp-loop-3210934 (from )}102024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server send response [#203](/bitcoin-bitcoin/203/) Echo.echo$Results (result = "Hello, world!")112024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server recv request [#204](/bitcoin-bitcoin/204/) Init.makeChain$Params (context = (thread = <external capability>))122024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server post request [#204](/bitcoin-bitcoin/204/) {bitcoin-node-3127476/b-capnp-loop-3210934 (from )}132024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server send response [#204](/bitcoin-bitcoin/204/) Init.makeChain$Results (result = <external capability>)142024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server recv request [#205](/bitcoin-bitcoin/205/) Chain.getHeight$Params (context = (thread = <external capability>))152024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server post request [#205](/bitcoin-bitcoin/205/) {bitcoin-node-3127476/b-capnp-loop-3210934 (from )}162024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server send response [#205](/bitcoin-bitcoin/205/) Chain.getHeight$Results (result = 196713, hasResult = true)172024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server recv request [#206](/bitcoin-bitcoin/206/) Chain.getSettingsList$Params (context = (thread = <external capability>))182024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server post request [#206](/bitcoin-bitcoin/206/) {bitcoin-node-3127476/b-capnp-loop-3210934 (from )}192024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server send response [#206](/bitcoin-bitcoin/206/) Chain.getSettingsList$Results (result = [])202024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server recv request [#207](/bitcoin-bitcoin/207/) Init.makeWalletLoader$Params (context = (thread = <external capability>))212024-05-22T13:49:15Z [ipc] {bitcoin-node-3127476/b-capnp-loop-3127479} IPC server post request [#207](/bitcoin-bitcoin/207/) {bitcoin-node-3127476/b-capnp-loop-3210934 (from )}22bitcoin-node: common/args.cpp:728: void ArgsManager::SetConfigFilePath(fs::path): Assertion `!m_config_path' failed.
Note, I haven’t tried to reproduce this, but the above is what I think happened.
ryanofsky
commented at 5:33 pm on June 10, 2024:
contributor
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.
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
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
e7619a84bd
Add capnp wrapper for Handler interface76fd4e56c4
Add capnp wrapper for Chain interfacef6c7786c41
multiprocess: Expose Chain interface
Expose Chain interface to external processes spawning or connecting to
bitcoin-node.
64b833854a
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.)
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
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: 2024-12-24 18:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me