Multiprocess bitcoin #10102

pull ryanofsky wants to merge 15 commits into bitcoin:master from ryanofsky:pr/ipc changing 69 files +2348 −87
  1. ryanofsky commented at 9:48 pm on March 27, 2017: contributor

    This is a draft PR because it is based on #29409. The non-base commits are:

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

    This PR is part of the process separation project.

  2. 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?
  3. jonasschnelli added the label GUI on Mar 28, 2017
  4. jonasschnelli added the label RPC/REST/ZMQ on Mar 28, 2017
  5. 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.

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

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

  9. in src/qt/walletmodel.cpp:757 in bf5f8ed6f0 outdated 
    708+    return FIXME_IMPLEMENT_IPC_VALUE(nTxConfirmTarget);
709 }
710 
711 bool WalletModel::getDefaultWalletRbf() const
712 {
713-    return fWalletRbf;
    laanwj commented at 8:29 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.
    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.

    laanwj commented at 10:58 am on March 29, 2017:

    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.

  10. in src/ipc/messages.capnp:33 in bf5f8ed6f0 outdated 
     8+    handleInitMessage @1 (callback: InitMessageCallback) -> (handler :Handler);
 9+    wallet @2 () -> (wallet :Wallet);
10+}
11+
12+interface Wallet {
13+    getBalance @0 () -> (value :Int64);
    laanwj commented at 8:31 am on March 29, 2017:
    Pretty nice. So the IPC endpoint exposes multiple “objects”. It can also expose multiple instances of one class? (e.g. for multiwallet?).
    ryanofsky commented at 9:59 am on March 29, 2017:

    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.

    laanwj commented at 11:00 am on March 29, 2017:
    Cool. I like this auto-generated IPC stuff. Saves writing a lot of boilerplate.
  11. in src/ipc/client.cpp:479 in bf5f8ed6f0 outdated 
    161+        for (int fd = 3; fd < maxFd; ++fd) {
162+            if (fd != fds[0]) {
163+                close(fd);
164+            }
165+        }
166+        if (execlp(BITCOIN_DAEMON_NAME, BITCOIN_DAEMON_NAME, "-ipcfd", std::to_string(fds[0]).c_str(), nullptr) != 0) {
    laanwj commented at 8:37 am on March 29, 2017:
    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?
    ryanofsky commented at 9:35 am on March 29, 2017:

    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.

     0+    //! Set command line arguments.
 1+    void parseParameters(int argc, const char* const argv[]) const;
 2+
 3     //! Get help message string.
 4     std::string helpMessage(HelpMessageMode mode) const;
 5 
 6+    //! Start node.
 7+    bool appInit() const;
 8+
 9+    //! Stop node.
10+    void appShutdown() const;
11+
12+    //! Return whether shutdown was requested.
13+    bool shutdownRequested() const;

    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.

    laanwj commented at 4:19 am on March 31, 2017:
    Right, ideally bitcoin-qt would need only a very small subset of the bitcoind code.
  12. in src/qt/bitcoin.cpp:307 in bf5f8ed6f0 outdated 
    302@@ -298,9 +303,9 @@ void BitcoinCore::shutdown()
303     try
304     {
305         qDebug() << __func__ << ": Running Shutdown in thread";
306-        Interrupt(threadGroup);
307+        FIXME_IMPLEMENT_IPC(Interrupt(threadGroup));
308         threadGroup.join_all();
    laanwj commented at 8:52 am on March 29, 2017:
    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.

  13. in src/qt/bitcoin.cpp:685 in bf5f8ed6f0 outdated 
    681@@ -676,7 +682,7 @@ int main(int argc, char *argv[])
682     app.createOptionsModel(IsArgSet("-resetguisettings"));
683 
684     // Subscribe to global signals from core
685-    uiInterface.InitMessage.connect(InitMessage);
686+    FIXME_IMPLEMENT_IPC_VALUE(uiInterface).InitMessage.connect(InitMessage);
    laanwj commented at 8:56 am on March 29, 2017:
    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.

    laanwj commented at 11:04 am on March 29, 2017:
    So the server then effectively calls an object on the client when the notification happens? No polling/waiting involved? That’s great.
    ryanofsky commented at 1:06 pm on March 29, 2017:

    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.

    laanwj commented at 4:23 am on March 31, 2017:

    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.

    ryanofsky commented at 11:34 pm on April 6, 2017:

    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?

    It should be possible to integrate the qt and capnp event loops (http://stackoverflow.com/questions/33131595/what-is-a-good-way-to-integrate-3rd-party-asynchronous-apis-with-capn-proto-rpc), but for now StartClient is just spawning an event loop thread, since this seems like the path of least resistance.

  14. in src/qt/walletmodel.cpp:692 in bf5f8ed6f0 outdated 
    690 
691 bool WalletModel::abandonTransaction(uint256 hash) const
692 {
693-    LOCK2(cs_main, wallet->cs_wallet);
694-    return wallet->AbandonTransaction(hash);
695+    LOCK2(FIXME_IMPLEMENT_IPC_VALUE(cs_main), wallet->cs_wallet);
    laanwj commented at 11:12 am on March 29, 2017:
    This locking (either of cs_main or wallet->cs_wallet) makes no sense when a remote core is used.
    ryanofsky commented at 1:09 pm on March 29, 2017:

    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.

  15. in src/ipc/server.cpp:10 in bf5f8ed6f0 outdated 
    84+
85+} // namespace
86+
87+bool StartServer(int argc, char* argv[], int& exitStatus)
88+{
89+    if (argc != 3 || strcmp(argv[1], "-ipcfd") != 0) {
    laanwj commented at 1:08 pm on March 29, 2017:

    FYI this is quite similar to how fd handles are passed in for the P2P and RPC port in my cloudabi port: https://github.com/laanwj/bitcoin/blob/2017_03_cabi_fs/src/httpserver.cpp#L357 https://github.com/laanwj/bitcoin/blob/2017_03_cabi_fs/src/init.cpp#L1356

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

    ryanofsky commented at 1:13 pm on March 29, 2017:

    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.

  16. ryanofsky force-pushed on Apr 6, 2017
  17. 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.

  18. ryanofsky force-pushed on Apr 7, 2017
  19. 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.

  20. ryanofsky force-pushed on Apr 10, 2017
  21. ryanofsky force-pushed on Apr 10, 2017
  22. ryanofsky commented at 10:25 pm on April 10, 2017: contributor

    Updated 5e28c2fcc2757479d29ca83cd3256584ab908e48 -> dda375662d060ce42b5113247301e0289584e14d (pr/ipc.3 -> pr/ipc.4)

    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.

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

    However, this doesn’t have to be the case. The place where IPC calls currently block waiting for responses is the return promise.get_future().get(); line in ipc::util::Call::send method here: https://github.com/ryanofsky/bitcoin/blob/pr/ipc.4/src/ipc/util.h#L166

    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.

  24. 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).
  25. 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.
  26. 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?

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

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

  29. ryanofsky referenced this in commit 7ba55396a0 on Apr 20, 2017
  30. ryanofsky force-pushed on Apr 20, 2017
  31. ryanofsky force-pushed on Apr 20, 2017
  32. ryanofsky referenced this in commit fb463d1717 on Apr 20, 2017
  33. ryanofsky force-pushed on Apr 27, 2017
  34. 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)
  35. ryanofsky force-pushed on Apr 27, 2017
  36. practicalswift referenced this in commit baf971cc8c on Apr 27, 2017
  37. ryanofsky referenced this in commit c59ffa1968 on Apr 28, 2017
  38. ryanofsky force-pushed on Apr 28, 2017
  39. ryanofsky force-pushed on Apr 28, 2017
  40. ryanofsky referenced this in commit 7e9ee9193f on May 4, 2017
  41. ryanofsky referenced this in commit 6135fc0948 on May 5, 2017
  42. ryanofsky referenced this in commit 333426d65d on May 9, 2017
  43. ryanofsky force-pushed on May 9, 2017
  44. ryanofsky referenced this in commit d944bd7a27 on May 17, 2017
  45. ryanofsky force-pushed on May 17, 2017
  46. ryanofsky force-pushed on May 23, 2017
  47. ryanofsky force-pushed on May 24, 2017
  48. ryanofsky force-pushed on May 25, 2017
  49. 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
  50. ryanofsky force-pushed on Jun 2, 2017
  51. ryanofsky force-pushed on Jun 2, 2017
  52. luke-jr referenced this in commit 651d189f51 on Jun 15, 2017
  53. ryanofsky force-pushed on Jun 19, 2017
  54. ryanofsky force-pushed on Aug 25, 2017
  55. ryanofsky force-pushed on Aug 25, 2017
  56. ryanofsky force-pushed on Sep 21, 2017
  57. ryanofsky force-pushed on Feb 14, 2018
  58. ryanofsky renamed this:
    bitcoin-qt: spawn bitcoind and communicate over pipe (Experimental, WIP, Depends on #10244)
    [experimental] Multiprocess bitcoin
    on Feb 14, 2018
  59. ryanofsky force-pushed on Feb 14, 2018
  60. ryanofsky force-pushed on Feb 14, 2018
  61. ryanofsky force-pushed on Feb 23, 2018
  62. ryanofsky force-pushed on Feb 26, 2018
  63. HashUnlimited referenced this in commit 0950e8ac40 on Mar 2, 2018
  64. ryanofsky force-pushed on Mar 2, 2018
  65. HashUnlimited referenced this in commit 7f09072241 on Mar 3, 2018
  66. ryanofsky force-pushed on Mar 7, 2018
  67. ryanofsky force-pushed on Mar 7, 2018
  68. ryanofsky force-pushed on Mar 12, 2018
  69. ryanofsky force-pushed on Mar 14, 2018
  70. ryanofsky force-pushed on Mar 23, 2018
  71. laanwj referenced this in commit 5f0c6a7b0e on Apr 5, 2018
  72. ryanofsky force-pushed on Apr 6, 2018
  73. ryanofsky force-pushed on Apr 7, 2018
  74. ryanofsky force-pushed on Apr 9, 2018
  75. ryanofsky force-pushed on Apr 10, 2018
  76. ryanofsky force-pushed on Apr 10, 2018
  77. ryanofsky force-pushed on Apr 11, 2018
  78. ryanofsky force-pushed on Apr 17, 2018
  79. ryanofsky force-pushed on Apr 17, 2018
  80. maflcko added the label Needs rebase on Jun 6, 2018
  81. ryanofsky force-pushed on Jul 18, 2018
  82. DrahtBot removed the label Needs rebase on Jul 18, 2018
  83. DrahtBot added the label Needs rebase on Jul 20, 2018
  84. ryanofsky force-pushed on Aug 3, 2018
  85. DrahtBot removed the label Needs rebase on Aug 3, 2018
  86. DrahtBot added the label Needs rebase on Aug 6, 2018
  87. ryanofsky force-pushed on Aug 7, 2018
  88. ryanofsky force-pushed on Aug 7, 2018
  89. DrahtBot removed the label Needs rebase on Aug 7, 2018
  90. ryanofsky force-pushed on Aug 8, 2018
  91. DrahtBot added the label Needs rebase on Aug 9, 2018
  92. ryanofsky force-pushed on Aug 9, 2018
  93. DrahtBot removed the label Needs rebase on Aug 9, 2018
  94. in doc/build-unix.md:115 in 006286e731 outdated 
    104@@ -105,7 +105,7 @@ To build without GUI pass `--without-gui`.
105 
106 To build with Qt 5 you need the following:
107 
108-    sudo apt-get install libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools libprotobuf-dev protobuf-compiler
109+    sudo apt-get install libqt5gui5 libqt5core5a libqt5dbus5 qttools5-dev qttools5-dev-tools libprotobuf-dev protobuf-compiler libcapnp-dev capnproto
    Sjors commented at 12:05 pm on August 11, 2018:
    For MacOS it’s brew install capnp
  95. 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.

    I got a bunch of warnings and eventually an error on macOS 10.13.6 with QT 5.11.1: https://gist.github.com/Sjors/c66198496a79fc5fc8bc280f510cf082

    Should capnp be added to depends (in a future PR) as well?

  96. DrahtBot added the label Needs rebase on Aug 13, 2018
  97. ryanofsky force-pushed on Aug 14, 2018
  98. DrahtBot removed the label Needs rebase on Aug 14, 2018
  99. ryanofsky force-pushed on Aug 21, 2018
  100. 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

  101. ryanofsky force-pushed on Aug 22, 2018
  102. DrahtBot added the label Needs rebase on Aug 23, 2018
  103. ryanofsky force-pushed on Aug 23, 2018
  104. DrahtBot removed the label Needs rebase on Aug 23, 2018
  105. DrahtBot added the label Needs rebase on Aug 25, 2018
  106. 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.

  107. 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.log debug.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.

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

  109. ryanofsky force-pushed on Aug 29, 2018
  110. DrahtBot removed the label Needs rebase on Aug 29, 2018
  111. DrahtBot added the label Needs rebase on Aug 30, 2018
  112. ryanofsky force-pushed on Aug 30, 2018
  113. DrahtBot removed the label Needs rebase on Aug 30, 2018
  114. DrahtBot added the label Needs rebase on Aug 31, 2018
  115. ryanofsky force-pushed on Aug 31, 2018
  116. DrahtBot removed the label Needs rebase on Aug 31, 2018
  117. in src/interfaces/README.md:17 in 2e8bee7b47 outdated 
    14 * [`Handler`](handler.h) — returned by `handleEvent` methods on interfaces above and used to manage lifetimes of event handlers.
15 
16-* [`Init`](init.h) — used by multiprocess code to access interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).
17+* [`Init`](init.h) — used by multiprocess code to access other interfaces above on startup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).
18+
19+* [`Base`](base.h) — base interface class used by multiprocess code for bookeeping and cleanup. Added in [#10102](https://github.com/bitcoin/bitcoin/pull/10102).
    practicalswift commented at 6:27 pm on September 2, 2018:
    Typo found by codespell: bookeeping
    ryanofsky commented at 2:58 pm on September 4, 2018:
    Fixed
  118. in src/interfaces/capnp/init.cpp:44 in 2e8bee7b47 outdated 
    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
  119. in src/interfaces/capnp/init.cpp:46 in 2e8bee7b47 outdated 
    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
  120. in src/interfaces/capnp/util.h:71 in 2e8bee7b47 outdated 
    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
  121. in src/interfaces/capnp/FIXME:6 in 2e8bee7b47 outdated 
    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
  122. in src/interfaces/capnp/proxy-impl.h:779 in 2e8bee7b47 outdated 
    774+    typename ProxyType<LocalType>::Struct* enable = nullptr)
775+{
776+    ReadOne<0>(param, invoke_context, input, value);
777+}
778+
779+// ReadField calling Emplace when ReadFieldNew is avaiable.
    practicalswift commented at 6:02 pm on September 3, 2018:
    Typo found by codespell: avaiable
  123. in src/interfaces/capnp/init.cpp:71 in 2e8bee7b47 outdated 
    68+        init_promise.set_value(connection->m_rpc_client.bootstrap(ServerVatId().vat_id).castAs<messages::Init>());
69+        loop.loop();
70+    });
71+    auto&& client = init_promise.get_future().get();
72+    auto proxy = MakeUnique<ProxyClient<messages::Init>>(kj::mv(client), *loop_ptr);
73+    proxy->addCloseHook(MakeUnique<ShutdownLoop>(*loop_ptr));
    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.

  124. ryanofsky force-pushed on Sep 4, 2018
  125. 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.

  126. ryanofsky force-pushed on Sep 4, 2018
  127. ryanofsky force-pushed on Sep 5, 2018
  128. 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.

    Updated 8d3df50476fa2f96cf2910238e5a2b3f8380b6e1 -> 5ce9a948c8ec47762d0a23d1cf7f2ce57386052b (pr/ipc.54 -> pr/ipc.55) to add missing SetupServerArgs call.

  129. in src/wallet/wallet.cpp:1634 in 5ce9a948c8 outdated 
    1644  */
1645-CBlockIndex* CWallet::ScanForWalletTransactions(CBlockIndex* pindexStart, CBlockIndex* pindexStop, const WalletRescanReserver &reserver, bool fUpdate)
1646+uint256 CWallet::ScanForWalletTransactions(const uint256& start_block, const uint256& stop_block, const WalletRescanReserver &reserver, bool fUpdate)
1647 {
1648     int64_t nNow = GetTime();
1649     const CChainParams& chainParams = Params();
    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.
  130. in src/rpc/rawtransaction.cpp:801 in 5ce9a948c8 outdated 
    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.

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

  132. DrahtBot added the label Needs rebase on Sep 7, 2018
  133. ryanofsky force-pushed on Sep 14, 2018
  134. DrahtBot removed the label Needs rebase on Sep 14, 2018
  135. DrahtBot added the label Needs rebase on Sep 15, 2018
  136. 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 class or 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)))
  137. ryanofsky force-pushed on Sep 26, 2018
  138. 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

  139. DrahtBot removed the label Needs rebase on Sep 26, 2018
  140. DrahtBot added the label Needs rebase on Sep 26, 2018
  141. ryanofsky force-pushed on Sep 27, 2018
  142. DrahtBot removed the label Needs rebase on Sep 27, 2018
  143. 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.

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept NACK electorr
    Concept ACK dcousens, laanwj, jgarzik, practicalswift, hebasto, promag, jamesob
    Stale ACK Sjors

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #31335 (macOS: swap docs & CI from pkg-config to pkgconf by fanquake)
    • #31250 (wallet: Disable creating and loading legacy wallets by achow101)
    • #30080 (wallet: add coin selection parameter add_excess_to_recipient_position for changeless txs with excess that would be added to fees by remyers)
    • #29409 (multiprocess: Add capnp wrapper for Chain interface by ryanofsky)
    • #27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
    • #26022 (Add util::ResultPtr class by ryanofsky)
    • #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.

  144. 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
  145. DrahtBot added the label Needs rebase on Oct 8, 2018
  146. 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 ?

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

  148. ryanofsky force-pushed on Nov 1, 2018
  149. ryanofsky commented at 9:14 pm on November 1, 2018: contributor

    re: #10102 (comment)

    Don’t forget to update .gitignore

    Updated now.

    re: #10102 (comment), #10102 (comment)

    I do still get the syscall deprecated in macOS 10.12 warning

    Should be fixed now. Worked around using pthread_threadid_np, according to http://elliotth.blogspot.com/2012/04/gettid-on-mac-os.html and https://github.com/google/glog/issues/182

    re: #10102 (comment)

    Some of the tests also throw a compiler warning.

    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.

  150. DrahtBot removed the label Needs rebase on Nov 1, 2018
  151. ismail120572 commented at 0:00 am on November 2, 2018: none
    Pull request
  152. in src/interfaces/init.h:25 in ee425e2ddf outdated 
    20+class Init : public Base
21+{
22+public:
23+    virtual std::unique_ptr<Node> makeNode() = 0;
24+    virtual std::unique_ptr<ChainClient> makeWalletClient(Chain& chain, std::vector<std::string> wallet_filenames) = 0;
25+    virtual IpcProcess* getProcess() { return nullptr; };
    practicalswift commented at 3:54 pm on November 2, 2018:
    Remove trailing ; :-)
  153. in src/interfaces/init.h:26 in ee425e2ddf outdated 
    21+{
22+public:
23+    virtual std::unique_ptr<Node> makeNode() = 0;
24+    virtual std::unique_ptr<ChainClient> makeWalletClient(Chain& chain, std::vector<std::string> wallet_filenames) = 0;
25+    virtual IpcProcess* getProcess() { return nullptr; };
26+    virtual IpcProtocol* getProtocol() { return nullptr; };
    practicalswift commented at 3:54 pm on November 2, 2018:
    Remove trailing ; :-)
  154. in src/interfaces/init.h:30 in ee425e2ddf outdated 
    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? :-)
  155. in src/wallet/wallet.cpp:2635 in ee425e2ddf outdated 
    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?
  156. in src/interfaces/node.cpp:55 in ee425e2ddf outdated 
    50@@ -50,6 +51,8 @@ namespace {
51 
52 class NodeImpl : public Node
53 {
54+public:
55+    NodeImpl(Init& init) : m_init(init) { m_interfaces.chain = MakeChain(); }
    practicalswift commented at 3:58 pm on November 2, 2018:
    Should be explicit? :-)
  157. in src/interfaces/base.h:28 in ee425e2ddf outdated 
    23+//! Close hook that encapsulate and deletes a moveable object.
24+template <typename T>
25+class Deleter : public CloseHook
26+{
27+public:
28+    Deleter(T&& value) : m_value(std::move(value)) {}
    practicalswift commented at 4:00 pm on November 2, 2018:

    Make explicit?

    Applies throughout this PR :-)

  158. in src/interfaces/capnp/proxy-codegen.cpp:422 in ee425e2ddf outdated 
    412+                       << client_args.str() << ")";
413+                client << ";\n";
414+                def << "ProxyClient<" << message_namespace << "::" << node_name << ">::M" << method.getOrdinal()
415+                    << "::Result ProxyClient<" << message_namespace << "::" << node_name << ">::" << method_name << "("
416+                    << client_args.str() << ") {\n";
417+                if (has_result) {
    practicalswift commented at 4:04 pm on November 2, 2018:
    Always true?
    ryanofsky commented at 7:28 pm on November 12, 2018:

    re: #10102 (review)

    Always true?

    has_result is false for any method that doesn’t have an output named result, which is true for any method that returns void.

  159. in src/interfaces/capnp/proxy-codegen.cpp:422 in ee425e2ddf outdated 
    417+                if (has_result) {
418+                    def << "    typename M" << method.getOrdinal() << "::Result result;\n";
419+                }
420+                def << "    clientInvoke(*this, &" << message_namespace << "::" << node_name
421+                    << "::Client::" << method_name << "Request" << client_invoke.str() << ");\n";
422+                if (has_result) def << "    return result;\n";
    practicalswift commented at 4:04 pm on November 2, 2018:
    Always true?
    ryanofsky commented at 7:28 pm on November 12, 2018:

    re: #10102 (review)

    Always true?

    No, see previous comment.

  160. in src/interfaces/capnp/proxy-impl.h:1373 in ee425e2ddf outdated 
    1368+{
1369+    return {values...};
1370+}
1371+
1372+//! Safely convert char pointer to kj pointer.
1373+static inline kj::byte* ByteCast(char* c) { return reinterpret_cast<kj::byte*>(c); }
    practicalswift commented at 4:05 pm on November 2, 2018:
    ByteCast not used?
  161. in src/interfaces/capnp/proxy-impl.h:1374 in ee425e2ddf outdated 
    1369+    return {values...};
1370+}
1371+
1372+//! Safely convert char pointer to kj pointer.
1373+static inline kj::byte* ByteCast(char* c) { return reinterpret_cast<kj::byte*>(c); }
1374+static inline const kj::byte* ByteCast(const char* c) { return reinterpret_cast<const kj::byte*>(c); }
    practicalswift commented at 4:05 pm on November 2, 2018:
    Same here.
  162. in src/interfaces/node.cpp:65 in ee425e2ddf outdated 
    61@@ -59,6 +62,10 @@ class NodeImpl : public Node
62     bool softSetBoolArg(const std::string& arg, bool value) override { return gArgs.SoftSetBoolArg(arg, value); }
63     void selectParams(const std::string& network) override { SelectParams(network); }
64     std::string getNetwork() override { return Params().NetworkIDString(); }
65+    std::string getArg(const std::string& arg, const std::string& default_value) override
    practicalswift commented at 4:06 pm on November 2, 2018:
    Not used?
  163. in src/interfaces/capnp/proxy.cpp:292 in ee425e2ddf outdated 
    79+    }
80+}
81+
82+ProxyServer<ThreadMap>::ProxyServer(EventLoop& loop) : m_loop(loop) {}
83+
84+kj::Promise<void> ProxyServer<ThreadMap>::makeThread(MakeThreadContext context)
    practicalswift commented at 4:07 pm on November 2, 2018:
    Not used?
    ryanofsky commented at 7:27 pm on November 12, 2018:

    re: #10102 (review)

    Not used?

    No, it’s called by generated code.

  164. in src/interfaces/capnp/test/foo.cpp:13 in ee425e2ddf outdated 
     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:

    re: #10102 (review)

    Not used?

    No, it’s called by generated code.

  165. in src/interfaces/capnp/proxy.cpp:44 in ee425e2ddf outdated 
     9+namespace interfaces {
10+namespace capnp {
11+
12+thread_local ThreadContext g_thread_context;
13+
14+void LoggingErrorHandler::taskFailed(kj::Exception&& exception)
    practicalswift commented at 4:07 pm on November 2, 2018:
    Not used?
    ryanofsky commented at 7:27 pm on November 12, 2018:

    re: #10102 (review)

    Not used?

    No, it’s a overridden method called through the base class by the kj library.

  166. in src/init.h:55 in ee425e2ddf outdated 
    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 =
  167. in src/interfaces/base.cpp:6 in ee425e2ddf outdated 
    0@@ -0,0 +1,22 @@
1+#include <interfaces/base.h>
    practicalswift commented at 4:08 pm on November 2, 2018:
    Missing copyright header
  168. in src/interfaces/base.h:5 in ee425e2ddf outdated 
    0@@ -0,0 +1,50 @@
1+#ifndef BITCOIN_INTERFACES_BASE_H
    practicalswift commented at 4:08 pm on November 2, 2018:
    Missing copyright header.
  169. in src/interfaces/capnp/ipc.cpp:5 in ee425e2ddf outdated 
    0@@ -0,0 +1,154 @@
1+#include <interfaces/capnp/ipc.h>
    practicalswift commented at 4:08 pm on November 2, 2018:
    Missing copyright header.
  170. in src/interfaces/capnp/ipc.cpp:59 in ee425e2ddf outdated 
    67+
68+class IpcProtocolImpl : public IpcProtocol
69+{
70+public:
71+    IpcProtocolImpl(const char* exe_name, Init& init) : m_exe_name(exe_name), m_init(init) {}
72+    ~IpcProtocolImpl() noexcept(true){};
    practicalswift commented at 4:09 pm on November 2, 2018:
    Missing whitespace before {.
  171. in src/interfaces/capnp/proxy-impl.h:252 in ee425e2ddf outdated 
    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
  172. in src/wallet/wallet.cpp:1859 in ee425e2ddf outdated 
    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?
  173. in src/wallet/wallet.cpp:1862 in ee425e2ddf outdated 
    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?
  174. DrahtBot added the label Needs rebase on Nov 5, 2018
  175. ryanofsky force-pushed on Nov 12, 2018
  176. DrahtBot removed the label Needs rebase on Nov 12, 2018
  177. DrahtBot added the label Needs rebase on Nov 13, 2018
  178. ryanofsky force-pushed on Nov 26, 2018
  179. DrahtBot removed the label Needs rebase on Nov 26, 2018
  180. DrahtBot added the label Needs rebase on Nov 30, 2018
  181. ryanofsky force-pushed on Dec 6, 2018
  182. ryanofsky force-pushed on Jan 23, 2019
  183. DrahtBot removed the label Needs rebase on Jan 23, 2019
  184. in src/interfaces/capnp/proxy-impl.h:396 in c5bc654324 outdated 
    382+    int req = server_context.req;
383+    auto invoke = MakeAsyncCallable(kj::mvCapture(future.fulfiller,
384+        kj::mvCapture(server_context.call_context,
385+            [&server, req, fn, args...](typename ServerContext::CallContext call_context,
386+                kj::Own<kj::PromiseFulfiller<typename ServerContext::CallContext>> fulfiller) {
387+                const auto& params = call_context.getParams();
    practicalswift commented at 9:02 am on January 24, 2019:
    Shadows variable params in outer scope?
  185. in src/interfaces/capnp/proxy-impl.h:397 in c5bc654324 outdated 
    383+    auto invoke = MakeAsyncCallable(kj::mvCapture(future.fulfiller,
384+        kj::mvCapture(server_context.call_context,
385+            [&server, req, fn, args...](typename ServerContext::CallContext call_context,
386+                kj::Own<kj::PromiseFulfiller<typename ServerContext::CallContext>> fulfiller) {
387+                const auto& params = call_context.getParams();
388+                Context::Reader context_arg = Accessor::get(params);
    practicalswift commented at 9:03 am on January 24, 2019:
    Same here?
  186. in src/interfaces/capnp/util.h:297 in c5bc654324 outdated 
    292+
293+//! Analog to std::lock_guard
294+template <typename Mutex>
295+struct UnlockGuard
296+{
297+    UnlockGuard(Mutex& mutex) : m_mutex(mutex) { m_mutex.unlock(); }
    practicalswift commented at 9:04 am on January 24, 2019:
    Should be explicit? Applies to all single parameter constructors in this PR :-)
  187. in src/interfaces/capnp/proxy-impl.h:1388 in c5bc654324 outdated 
    1400+    template <typename Arg1, typename Arg2, typename ParamList, typename NextFn, typename... NextFnArgs>
1401+    void handleChain(Arg1&& arg1, Arg2&& arg2, ParamList, NextFn&& next_fn, NextFnArgs&&... next_fn_args)
1402+    {
1403+        using S = Split<N, ParamList>;
1404+        handleChain(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2), typename S::First());
1405+        next_fn.handleChain(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2), typename S::Second(),
    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?
  188. meshcollider referenced this in commit 72ca72e637 on Jan 30, 2019
  189. DrahtBot added the label Needs rebase on Jan 30, 2019
  190. ryanofsky force-pushed on Feb 13, 2019
  191. DrahtBot removed the label Needs rebase on Feb 13, 2019
  192. DrahtBot added the label Needs rebase on Feb 15, 2019
  193. maflcko referenced this in commit 45f434f44d on Mar 4, 2019
  194. ryanofsky force-pushed on Mar 4, 2019
  195. DrahtBot removed the label Needs rebase on Mar 4, 2019
  196. ryanofsky force-pushed on Mar 5, 2019
  197. DrahtBot added the label Needs rebase on Mar 6, 2019
  198. meshcollider referenced this in commit 2607d960a0 on Mar 21, 2019
  199. ryanofsky force-pushed on Mar 21, 2019
  200. DrahtBot removed the label Needs rebase on Mar 21, 2019
  201. DrahtBot added the label Needs rebase on Mar 27, 2019
  202. ryanofsky force-pushed on May 24, 2019
  203. DrahtBot removed the label Needs rebase on May 24, 2019
  204. ryanofsky force-pushed on May 24, 2019
  205. ryanofsky force-pushed on May 24, 2019
  206. 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 4098e3 7ffbcf55904d 7ffbcf559082 7ffbcf53208f 7ffbcf534296 7ffbcf5510a4 7ffbcf558893 7ffbcf558901 7ffbcf558c57 7ffbcf532974 7ffbcf532724 7ffbcf533921 7ffbcf5399fc 7ffbcf542aa5 7ffbcf5436d6 7ffbcf555452 7ffbcf55662a 7ffbcf55679f 7ffbcf55a606 7ffbcf55aa53 412466 412770 7ffbcf3e4c28 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 1
17In 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          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  207. ryanofsky commented at 7:41 am on May 27, 2019: contributor
    re: #10102 (comment) @jb55 this looks like it’s caused by changes between capnproto 0.7.0 and 0.6.1: https://github.com/capnproto/capnproto/issues/772. I don’t think it should be too be hard to add compatibility with 0.7.0, but I haven’t tried it yet myself.
  208. 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)
  209. ryanofsky force-pushed on Jun 1, 2019
  210. ryanofsky force-pushed on Jun 1, 2019
  211. ryanofsky force-pushed on Jun 1, 2019
  212. ryanofsky force-pushed on Jun 1, 2019
  213. DrahtBot added the label Needs rebase on Jun 6, 2019
  214. ryanofsky force-pushed on Jun 8, 2019
  215. DrahtBot removed the label Needs rebase on Jun 8, 2019
  216. DrahtBot added the label Needs rebase on Jun 18, 2019
  217. ryanofsky force-pushed on Jun 26, 2019
  218. ryanofsky force-pushed on Jun 26, 2019
  219. DrahtBot removed the label Needs rebase on Jun 26, 2019
  220. DrahtBot added the label Needs rebase on Jun 27, 2019
  221. ryanofsky force-pushed on Jun 27, 2019
  222. DrahtBot removed the label Needs rebase on Jun 27, 2019
  223. DrahtBot added the label Needs rebase on Jun 29, 2019
  224. ryanofsky referenced this in commit 25140b7dcc on Jul 10, 2019
  225. ryanofsky force-pushed on Jul 10, 2019
  226. DrahtBot removed the label Needs rebase on Jul 10, 2019
  227. ryanofsky referenced this in commit 8fd0743716 on Jul 12, 2019
  228. ryanofsky force-pushed on Jul 12, 2019
  229. ryanofsky referenced this in commit 724d2f7fe3 on Jul 15, 2019
  230. ryanofsky referenced this in commit 45785e7f19 on Jul 15, 2019
  231. ryanofsky referenced this in commit 5233494f3f on Jul 15, 2019
  232. ryanofsky force-pushed on Jul 15, 2019
  233. ryanofsky force-pushed on Jul 17, 2019
  234. DrahtBot added the label Needs rebase on Jul 24, 2019
  235. ryanofsky referenced this in commit c685fa9bed on Aug 6, 2019
  236. ryanofsky referenced this in commit 8125688f6c on Aug 6, 2019
  237. ryanofsky referenced this in commit f324c66615 on Aug 6, 2019
  238. ryanofsky referenced this in commit dee0711538 on Aug 6, 2019
  239. ryanofsky referenced this in commit 1938bdebc6 on Aug 6, 2019
  240. ryanofsky force-pushed on Aug 6, 2019
  241. ryanofsky force-pushed on Aug 6, 2019
  242. ryanofsky referenced this in commit ffa6c808f8 on Aug 6, 2019
  243. ryanofsky force-pushed on Aug 6, 2019
  244. DrahtBot removed the label Needs rebase on Aug 6, 2019
  245. DrahtBot added the label Needs rebase on Aug 12, 2019
  246. ryanofsky force-pushed on Aug 20, 2019
  247. DrahtBot removed the label Needs rebase on Aug 20, 2019
  248. ryanofsky referenced this in commit 6208f3e7bd on Aug 20, 2019
  249. DrahtBot added the label Needs rebase on Aug 23, 2019
  250. ryanofsky referenced this in commit 7fbe1cac83 on Aug 28, 2019
  251. ryanofsky force-pushed on Aug 28, 2019
  252. DrahtBot removed the label Needs rebase on Aug 28, 2019
  253. DrahtBot added the label Needs rebase on Aug 29, 2019
  254. ryanofsky referenced this in commit fdc72aed96 on Aug 29, 2019
  255. ryanofsky force-pushed on Aug 29, 2019
  256. DrahtBot removed the label Needs rebase on Aug 29, 2019
  257. ryanofsky referenced this in commit 94730885da on Aug 30, 2019
  258. ryanofsky force-pushed on Aug 30, 2019
  259. ryanofsky force-pushed on Aug 30, 2019
  260. ryanofsky force-pushed on Aug 30, 2019
  261. ryanofsky force-pushed on Aug 30, 2019
  262. DrahtBot added the label Needs rebase on Sep 7, 2019
  263. ryanofsky referenced this in commit 124bbcabe3 on Sep 9, 2019
  264. ryanofsky referenced this in commit 7225ebebdf on Sep 10, 2019
  265. ryanofsky referenced this in commit a3dc53304a on Sep 10, 2019
  266. ryanofsky referenced this in commit 7b65a5356e on Sep 18, 2019
  267. ryanofsky referenced this in commit 858a3f90fc on Sep 18, 2019
  268. ryanofsky force-pushed on Sep 18, 2019
  269. DrahtBot removed the label Needs rebase on Sep 18, 2019
  270. ryanofsky force-pushed on Sep 19, 2019
  271. DrahtBot added the label Needs rebase on Oct 2, 2019
  272. laanwj referenced this in commit 471e5f8829 on Oct 30, 2019
  273. ryanofsky referenced this in commit 616658f1cf on Jan 8, 2020
  274. ryanofsky force-pushed on Jan 8, 2020
  275. ryanofsky force-pushed on Jan 9, 2020
  276. ryanofsky force-pushed on Jan 9, 2020
  277. DrahtBot removed the label Needs rebase on Jan 9, 2020
  278. ryanofsky referenced this in commit 2b3d6bf0e4 on Jan 10, 2020
  279. ryanofsky force-pushed on Jan 10, 2020
  280. jgarzik commented at 9:30 pm on January 10, 2020: contributor
    concept ACK
  281. 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? :)

  282. DrahtBot added the label Needs rebase on Jan 13, 2020
  283. ryanofsky referenced this in commit 26cde384af on Jan 16, 2020
  284. ryanofsky referenced this in commit 17bd1544b5 on Jan 24, 2020
  285. ryanofsky referenced this in commit 30a4088a9a on Jan 24, 2020
  286. ryanofsky force-pushed on Jan 24, 2020
  287. DrahtBot removed the label Needs rebase on Jan 24, 2020
  288. DrahtBot added the label Needs rebase on Jan 29, 2020
  289. ryanofsky referenced this in commit 689424a679 on Feb 10, 2020
  290. ryanofsky referenced this in commit e94785a54f on Feb 25, 2020
  291. ryanofsky referenced this in commit a56950c54a on Feb 25, 2020
  292. ryanofsky force-pushed on Feb 25, 2020
  293. ryanofsky referenced this in commit 360fd9be3c on Feb 25, 2020
  294. DrahtBot removed the label Needs rebase on Feb 25, 2020
  295. ryanofsky referenced this in commit 96cb597325 on Feb 26, 2020
  296. 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.

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

  298. DrahtBot added the label Needs rebase on Mar 5, 2020
  299. hebasto commented at 9:35 am on March 5, 2020: member
    Concept ACK.
  300. 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.

  301. ryanofsky referenced this in commit 6aad158ce1 on Mar 5, 2020
  302. ryanofsky force-pushed on Mar 5, 2020
  303. ryanofsky commented at 5:33 pm on March 5, 2020: contributor

    re: #10102 (comment)

    Testing on Linux Mint 19.3:

    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

  304. ryanofsky referenced this in commit 5fff177756 on Mar 5, 2020
  305. ryanofsky referenced this in commit 9621f4469f on Mar 5, 2020
  306. ryanofsky referenced this in commit 23b79a3555 on Mar 6, 2020
  307. ryanofsky referenced this in commit 78f28a2655 on Mar 6, 2020
  308. ryanofsky referenced this in commit 1c53c9ec9f on Mar 6, 2020
  309. ryanofsky referenced this in commit 0c1e0c04c4 on Mar 6, 2020
  310. ryanofsky referenced this in commit 1390103563 on Mar 6, 2020
  311. ryanofsky referenced this in commit 1242ac8e31 on Mar 6, 2020
  312. ryanofsky referenced this in commit 1e822cd493 on Mar 6, 2020
  313. ryanofsky referenced this in commit 072df2ec6f on Mar 6, 2020
  314. ryanofsky referenced this in commit ddea706fa7 on Mar 6, 2020
  315. ryanofsky referenced this in commit 67e7fe7641 on Mar 6, 2020
  316. ryanofsky referenced this in commit f2809b240e on Mar 6, 2020
  317. ryanofsky referenced this in commit 51c9b1472f on Mar 6, 2020
  318. ryanofsky referenced this in commit a16ea53d67 on Mar 6, 2020
  319. ryanofsky referenced this in commit 5212bd926b on Mar 6, 2020
  320. ryanofsky referenced this in commit c7b9338fd3 on Mar 6, 2020
  321. ryanofsky referenced this in commit 46fa4ba52c on Mar 6, 2020
  322. ryanofsky force-pushed on Mar 6, 2020
  323. ryanofsky referenced this in commit 5df203c45f on Mar 6, 2020
  324. ryanofsky referenced this in commit 720c59a564 on Mar 6, 2020
  325. ryanofsky referenced this in commit 34d68548fa on Mar 6, 2020
  326. ryanofsky referenced this in commit 4231e81ce6 on Mar 6, 2020
  327. ryanofsky referenced this in commit 5c3803bd88 on Mar 6, 2020
  328. ryanofsky referenced this in commit 3335df38bf on Mar 6, 2020
  329. DrahtBot removed the label Needs rebase on Mar 7, 2020
  330. ryanofsky referenced this in commit a9ff82406f on Mar 9, 2020
  331. ryanofsky referenced this in commit 39d05cb2e7 on Mar 9, 2020
  332. DrahtBot added the label Needs rebase on Mar 9, 2020
  333. ryanofsky referenced this in commit 623b19deb3 on Mar 9, 2020
  334. ryanofsky referenced this in commit 5d5899d07b on Mar 9, 2020
  335. ryanofsky referenced this in commit 64764c2f5b on Mar 9, 2020
  336. ryanofsky referenced this in commit 782c7ef202 on Mar 9, 2020
  337. ryanofsky referenced this in commit 0a183e3f02 on Mar 9, 2020
  338. ryanofsky referenced this in commit 07912bfbfb on Mar 9, 2020
  339. ryanofsky referenced this in commit 519afbee0f on Mar 9, 2020
  340. ryanofsky referenced this in commit 6a9e96fa5b on Mar 9, 2020
  341. ryanofsky referenced this in commit d12892ad69 on Mar 9, 2020
  342. ryanofsky referenced this in commit 493421a3d1 on Mar 9, 2020
  343. ryanofsky referenced this in commit b812395298 on Mar 9, 2020
  344. ryanofsky referenced this in commit a752430301 on Mar 9, 2020
  345. ryanofsky referenced this in commit 4437ae0549 on Mar 9, 2020
  346. ryanofsky referenced this in commit e565a694c7 on Mar 9, 2020
  347. ryanofsky referenced this in commit b4bc14ce50 on Mar 20, 2020
  348. ryanofsky referenced this in commit b80e677e35 on Mar 20, 2020
  349. ryanofsky referenced this in commit 5389f3fa8e on Mar 20, 2020
  350. ryanofsky referenced this in commit 69e3ebd8d4 on Mar 20, 2020
  351. ryanofsky referenced this in commit 77e4b06572 on Mar 20, 2020
  352. ryanofsky referenced this in commit 6ceb21909c on Mar 20, 2020
  353. ryanofsky referenced this in commit 96dfe5ced6 on Mar 20, 2020
  354. ryanofsky referenced this in commit 1dca9dc4c7 on Mar 20, 2020
  355. ryanofsky referenced this in commit e43c923314 on Mar 20, 2020
  356. ryanofsky referenced this in commit 005b0716a0 on Mar 20, 2020
  357. ryanofsky referenced this in commit 0957095609 on Mar 20, 2020
  358. ryanofsky referenced this in commit d9c8350933 on Mar 20, 2020
  359. ryanofsky referenced this in commit b3f5eda1a2 on Mar 20, 2020
  360. ryanofsky referenced this in commit 1fa2e9ab89 on Mar 20, 2020
  361. ryanofsky referenced this in commit 6f3e1bdb97 on Mar 20, 2020
  362. ryanofsky referenced this in commit 92a9448b66 on Mar 20, 2020
  363. ryanofsky force-pushed on Mar 20, 2020
  364. DrahtBot removed the label Needs rebase on Mar 20, 2020
  365. maflcko referenced this in commit ac579ada7e on Mar 23, 2020
  366. ryanofsky referenced this in commit cdd066d66b on Mar 25, 2020
  367. ryanofsky force-pushed on Mar 25, 2020
  368. ryanofsky referenced this in commit 671d33e0ed on Mar 25, 2020
  369. ryanofsky referenced this in commit 671749e327 on Mar 25, 2020
  370. ryanofsky force-pushed on Mar 25, 2020
  371. sidhujag referenced this in commit d630e78524 on Mar 28, 2020
  372. DrahtBot added the label Needs rebase on Mar 31, 2020
  373. ryanofsky referenced this in commit e6e44eedd5 on Apr 6, 2020
  374. ryanofsky referenced this in commit fedadaf084 on Apr 6, 2020
  375. maflcko referenced this in commit 1b30761360 on Apr 10, 2020
  376. maflcko referenced this in commit 99d6a5be8b on Apr 10, 2020
  377. ryanofsky referenced this in commit 3f50cdc914 on Apr 10, 2020
  378. ryanofsky referenced this in commit d04533c28e on Apr 10, 2020
  379. ryanofsky force-pushed on Apr 10, 2020
  380. ryanofsky force-pushed on Apr 10, 2020
  381. DrahtBot removed the label Needs rebase on Apr 10, 2020
  382. maflcko referenced this in commit 3eb8b1c392 on Apr 10, 2020
  383. ryanofsky referenced this in commit 9fe489b76f on Apr 10, 2020
  384. ryanofsky referenced this in commit 8928ebd6eb on Apr 10, 2020
  385. DrahtBot added the label Needs rebase on Apr 11, 2020
  386. sidhujag referenced this in commit 292df1690f on Apr 13, 2020
  387. sidhujag referenced this in commit cb75c3b555 on Apr 13, 2020
  388. sidhujag referenced this in commit c486c7458a on Apr 13, 2020
  389. ryanofsky referenced this in commit f3ecfb553f on Apr 15, 2020
  390. ryanofsky referenced this in commit 26c1dc9c2d on Apr 15, 2020
  391. ryanofsky referenced this in commit fa4c9ff83c on Apr 15, 2020
  392. ryanofsky force-pushed on Apr 15, 2020
  393. ryanofsky referenced this in commit ae38954238 on Apr 15, 2020
  394. ryanofsky referenced this in commit 165ec1f83d on Apr 15, 2020
  395. DrahtBot removed the label Needs rebase on Apr 15, 2020
  396. ryanofsky referenced this in commit bc5f6da84e on Apr 16, 2020
  397. HashUnlimited referenced this in commit e88645f092 on Apr 17, 2020
  398. HashUnlimited referenced this in commit 9746ac3b44 on Apr 17, 2020
  399. HashUnlimited referenced this in commit 8f3b970b3f on Apr 17, 2020
  400. HashUnlimited referenced this in commit ee710bddc0 on Apr 17, 2020
  401. ryanofsky referenced this in commit 4f359eb256 on Apr 20, 2020
  402. ryanofsky referenced this in commit cf333aba15 on Apr 25, 2020
  403. DrahtBot added the label Needs rebase on May 1, 2020
  404. ryanofsky referenced this in commit bf0a510981 on May 1, 2020
  405. ryanofsky referenced this in commit ae9ee3c8fc on May 1, 2020
  406. ryanofsky force-pushed on May 1, 2020
  407. ryanofsky referenced this in commit 803fb13624 on May 1, 2020
  408. ryanofsky referenced this in commit a9014cf50a on May 1, 2020
  409. DrahtBot removed the label Needs rebase on May 1, 2020
  410. 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.

  411. 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.
  412. ryanofsky referenced this in commit 1c51df5bab on May 6, 2020
  413. DrahtBot added the label Needs rebase on May 7, 2020
  414. ryanofsky referenced this in commit 0775109f55 on May 7, 2020
  415. ryanofsky referenced this in commit 0ffa2052cf on May 7, 2020
  416. ryanofsky referenced this in commit 09939485d3 on May 8, 2020
  417. ryanofsky referenced this in commit 6d911a40cf on May 8, 2020
  418. ryanofsky referenced this in commit 1bc9859d6a on May 13, 2020
  419. ryanofsky referenced this in commit 72c2eff332 on May 13, 2020
  420. ryanofsky referenced this in commit 5d1377b52b on May 13, 2020
  421. jonasschnelli referenced this in commit a587f85853 on May 20, 2020
  422. sidhujag referenced this in commit 381c70f27b on May 20, 2020
  423. fanquake referenced this in commit 97b21b302a on May 21, 2020
  424. sidhujag referenced this in commit 6a4320e7a9 on May 21, 2020
  425. ryanofsky force-pushed on Jun 1, 2020
  426. ryanofsky referenced this in commit 294105ebb1 on Jun 1, 2020
  427. DrahtBot removed the label Needs rebase on Jun 1, 2020
  428. in test/functional/feature_block.py:92 in b737e5397a outdated 
    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
    maflcko commented at 1:36 pm on June 2, 2020:

    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.

    ryanofsky commented at 7:28 pm on June 2, 2020:

    in commit b737e53

    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.

    Sjors commented at 7:43 am on December 9, 2021:
    In that case it may be good to add comments that the long timeout is due to IPC.
    ryanofsky commented at 7:07 pm on December 17, 2021:

    re: #10102 (review)

    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.

  429. Sjors commented at 2:16 pm on June 2, 2020: member

    The test suite passes, but I get quite a few warnings when building on macOS 10.15.4 : https://gist.github.com/Sjors/b7b2dc026a39741b019f15e98751cf57#file-make-1-log

    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: 0000000000000000
1^C2020-06-02T14:06:10Z Shutdown requested. Exiting.
2
3Assertion failed: (nThreadsServicingQueue == 0), function ~CScheduler, file scheduler.cpp, line 18

    bitcoin-node doesn’t have the same config file issue, but it hits similar asserts when existing with ctrl+c. I added an example log: https://gist.github.com/Sjors/b7b2dc026a39741b019f15e98751cf57#file-bitcoin-node-log

  430. hebasto commented at 2:33 pm on June 2, 2020: member
    @Sjors see also #10102 (comment), #10102 (comment)
  431. ryanofsky commented at 3:08 pm on June 2, 2020: contributor

    Thanks, I’ll look into the settings bug, and adding a sigint handler since multiple people reported this

    Rebased fd810d2ae759091dadf39d1d6af3bf267e66bbfd -> 5f8b1d3bca82490872fddcb6ff6e1b6ecf4a66e5 (pr/ipc.111 -> pr/ipc.112, compare) on top of #19160 pr/ipc-echo.3 Rebased 5f8b1d3bca82490872fddcb6ff6e1b6ecf4a66e5 -> 37061a71acdd55d95359d898a3f5e8ff00cd7019 (pr/ipc.112 -> pr/ipc.113, compare) on top of #19160 pr/ipc-echo.4 Rebased 37061a71acdd55d95359d898a3f5e8ff00cd7019 -> 7b8d8908582a8a968d9f601745283d7c7bb2eaf4 (pr/ipc.113 -> pr/ipc.114, compare) due to silent merge conflicts with #17938 (destination hash types) and #18027 (fillPSBT), on top of #19160 pr/ipc-echo.5 Updated 7b8d8908582a8a968d9f601745283d7c7bb2eaf4 -> 93bc1db49596bb69e9e53630af53cc1c94a20f25 (pr/ipc.114 -> pr/ipc.115, compare) to fix travis include errors Rebased 93bc1db49596bb69e9e53630af53cc1c94a20f25 -> 7f6afc5d43bffe729443b605328ec5054bb2c806 (pr/ipc.115 -> pr/ipc.116, compare) on top of #19160 pr/ipc-echo.6 due to silent conflict with #19219 Rebased 7f6afc5d43bffe729443b605328ec5054bb2c806 -> 081cf6c40b51a2f456703620b87c27d303dcdf67 (pr/ipc.116 -> pr/ipc.117, compare) due to conflicts with #19098 Updated 081cf6c40b51a2f456703620b87c27d303dcdf67 -> 3076c8c41a627adb2bc7fc6722c50fcaf34ccfdd (pr/ipc.117 -> pr/ipc.118, compare) to fix wallet_dump assert_debug_log test error https://travis-ci.org/github/bitcoin/bitcoin/jobs/707823975 Rebased 3076c8c41a627adb2bc7fc6722c50fcaf34ccfdd -> 3da29016799095b10c7fa5aa0368cdf0cb784846 (pr/ipc.118 -> pr/ipc.119, compare) on top of #19160 pr/ipc-echo.7 Rebased 3da29016799095b10c7fa5aa0368cdf0cb784846 -> 56d577bc1d79cfb3e0a78f49f42abcce0727ea16 (pr/ipc.119 -> pr/ipc.120, compare) due to conflicts with #18571 and #15935 Rebased 56d577bc1d79cfb3e0a78f49f42abcce0727ea16 -> 060a1b55396f4ab142781a39e3f98baaa31ddedd (pr/ipc.120 -> pr/ipc.121, compare) due to conflict with #19326 Rebased 060a1b55396f4ab142781a39e3f98baaa31ddedd -> 49c22b589a2c2d63acd0434c6424880e134ec085 (pr/ipc.121 -> pr/ipc.122, compare) due to conflict with #19098 Updated 49c22b589a2c2d63acd0434c6424880e134ec085 -> 4e3dae7a584650485b1c328577be5b58c99258c4 (pr/ipc.122 -> pr/ipc.123, compare) to fix test errors https://travis-ci.org/github/bitcoin/bitcoin/builds/717076622 https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/34606581 https://cirrus-ci.com/task/4522962420236288 https://cirrus-ci.com/task/5648862327078912 Rebased 4e3dae7a584650485b1c328577be5b58c99258c4 -> 21529efc39ae143a3f4b2a1dd4f697c638fc88a5 (pr/ipc.123 -> pr/ipc.124, compare) due to conflicts with #19011 and #15937 on top of #19160 pr/ipc-echo.11 Rebased 21529efc39ae143a3f4b2a1dd4f697c638fc88a5 -> fa6f80a699aa2f9f001090cd60bf0a02d545f8cb (pr/ipc.124 -> pr/ipc.125, compare) on top of #19160 pr/ipc-echo.7 with fixes for travis errors https://travis-ci.org/github/bitcoin/bitcoin/jobs/721173746#L3338

  432. ryanofsky referenced this in commit d6d835680d on Jun 3, 2020
  433. ryanofsky referenced this in commit 4277501031 on Jun 3, 2020
  434. ryanofsky referenced this in commit 9737cc9b50 on Jun 3, 2020
  435. ryanofsky referenced this in commit 9df8470cd1 on Jun 3, 2020
  436. ryanofsky force-pushed on Jun 4, 2020
  437. DrahtBot added the label Needs rebase on Jun 9, 2020
  438. ryanofsky referenced this in commit 253fe161bb on Jun 11, 2020
  439. ryanofsky referenced this in commit 643d602b3f on Jun 11, 2020
  440. ryanofsky force-pushed on Jun 11, 2020
  441. DrahtBot removed the label Needs rebase on Jun 11, 2020
  442. ryanofsky referenced this in commit 556564f853 on Jun 11, 2020
  443. maflcko referenced this in commit c2bcb99c1d on Jun 18, 2020
  444. ryanofsky referenced this in commit 16a7aa17a4 on Jun 29, 2020
  445. ryanofsky referenced this in commit 0c9dea569a on Jul 1, 2020
  446. ryanofsky referenced this in commit 87bc7618cf on Jul 1, 2020
  447. sidhujag referenced this in commit e593102316 on Jul 7, 2020
  448. ryanofsky force-pushed on Jul 7, 2020
  449. ryanofsky referenced this in commit a1647c3679 on Jul 7, 2020
  450. ryanofsky referenced this in commit 187b769fdd on Jul 7, 2020
  451. ryanofsky referenced this in commit 29e8689442 on Jul 7, 2020
  452. ryanofsky referenced this in commit 6f4acdc687 on Jul 10, 2020
  453. ryanofsky referenced this in commit 7928b7eb72 on Jul 10, 2020
  454. ryanofsky referenced this in commit 91ac0388e8 on Jul 10, 2020
  455. ryanofsky force-pushed on Jul 10, 2020
  456. ryanofsky force-pushed on Jul 11, 2020
  457. DrahtBot added the label Needs rebase on Jul 11, 2020
  458. ryanofsky force-pushed on Jul 13, 2020
  459. DrahtBot removed the label Needs rebase on Jul 14, 2020
  460. ryanofsky force-pushed on Jul 14, 2020
  461. ryanofsky referenced this in commit 6a48a575d1 on Jul 14, 2020
  462. ryanofsky referenced this in commit 45fee5b4e6 on Jul 14, 2020
  463. DrahtBot added the label Needs rebase on Jul 14, 2020
  464. ryanofsky referenced this in commit d616ed5086 on Jul 14, 2020
  465. ryanofsky force-pushed on Jul 14, 2020
  466. ryanofsky referenced this in commit 47a89fbe38 on Jul 14, 2020
  467. ryanofsky referenced this in commit f3ace3d21f on Jul 14, 2020
  468. DrahtBot removed the label Needs rebase on Jul 14, 2020
  469. ryanofsky referenced this in commit 5adf00eb1e on Jul 16, 2020
  470. ryanofsky referenced this in commit 551c50873d on Jul 17, 2020
  471. ryanofsky referenced this in commit 3e54d36b17 on Jul 21, 2020
  472. DrahtBot added the label Needs rebase on Jul 30, 2020
  473. ryanofsky force-pushed on Jul 30, 2020
  474. DrahtBot removed the label Needs rebase on Jul 30, 2020
  475. DrahtBot added the label Needs rebase on Aug 3, 2020
  476. ryanofsky referenced this in commit 1ac160cdfe on Aug 3, 2020
  477. ryanofsky referenced this in commit 09a244fc43 on Aug 3, 2020
  478. ryanofsky force-pushed on Aug 4, 2020
  479. DrahtBot removed the label Needs rebase on Aug 4, 2020
  480. DrahtBot added the label Needs rebase on Aug 7, 2020
  481. ryanofsky referenced this in commit 7595c3b33c on Aug 7, 2020
  482. ryanofsky referenced this in commit 134153fc41 on Aug 7, 2020
  483. ryanofsky referenced this in commit fd96326d31 on Aug 7, 2020
  484. ryanofsky referenced this in commit 53fae31e49 on Aug 7, 2020
  485. ryanofsky referenced this in commit b24ca99478 on Aug 10, 2020
  486. ryanofsky referenced this in commit 207c94f884 on Aug 10, 2020
  487. ryanofsky referenced this in commit 1525c389cc on Aug 11, 2020
  488. ryanofsky force-pushed on Aug 11, 2020
  489. DrahtBot removed the label Needs rebase on Aug 11, 2020
  490. ryanofsky force-pushed on Aug 12, 2020
  491. DrahtBot added the label Needs rebase on Aug 14, 2020
  492. ryanofsky referenced this in commit 234ed648d3 on Aug 17, 2020
  493. ryanofsky referenced this in commit 427c8da711 on Aug 17, 2020
  494. ryanofsky referenced this in commit d4bc442797 on Aug 17, 2020
  495. ryanofsky referenced this in commit 58080afc73 on Aug 17, 2020
  496. ryanofsky referenced this in commit 3eb10c37b6 on Aug 17, 2020
  497. ryanofsky referenced this in commit c0bea1c615 on Aug 24, 2020
  498. ryanofsky referenced this in commit 907f95b07d on Aug 24, 2020
  499. ryanofsky force-pushed on Aug 25, 2020
  500. DrahtBot removed the label Needs rebase on Aug 26, 2020
  501. DrahtBot added the label Needs rebase on Aug 26, 2020
  502. ryanofsky referenced this in commit a3cda11b8c on Aug 26, 2020
  503. ryanofsky referenced this in commit e133631625 on Aug 26, 2020
  504. sidhujag referenced this in commit 797191d45b on Aug 26, 2020
  505. bitcoin deleted a comment on Aug 26, 2020
  506. ryanofsky referenced this in commit f73918eb2d on Aug 26, 2020
  507. ryanofsky referenced this in commit 066117fe3a on Aug 26, 2020
  508. ryanofsky force-pushed on Aug 27, 2020
  509. DrahtBot removed the label Needs rebase on Aug 27, 2020
  510. DrahtBot added the label Needs rebase on Aug 31, 2020
  511. ryanofsky referenced this in commit cb33a694b3 on Sep 1, 2020
  512. ryanofsky referenced this in commit 37881160ed on Sep 1, 2020
  513. ryanofsky force-pushed on Sep 13, 2020
  514. jgarzik commented at 8:29 pm on September 13, 2020: contributor

    Thanks for continuing to work on this. Cloning and building to test now.

    Multi-process isolation boundaries have been needed for… 10 years or so.

  515. ryanofsky force-pushed on Sep 13, 2020
  516. ryanofsky commented at 8:43 pm on September 13, 2020: contributor

    Thanks for continuing to work on this. Cloning and building to test now.

    Thanks! Some instructions are at https://github.com/ryanofsky/bitcoin/blob/pr/ipc/doc/multiprocess.md#installation in case that’s helpful.

    Rebased fa6f80a699aa2f9f001090cd60bf0a02d545f8cb -> 9bb2b0fc428877b5af7e472ca492b09ecdb72394 (pr/ipc.125 -> pr/ipc.126, compare) on top of #19160 pr/ipc-echo.13 due to conflict with #19099 Updated 9bb2b0fc428877b5af7e472ca492b09ecdb72394 -> e9523ebd6e312422c2a891d3004678ba4459d87f (pr/ipc.126 -> pr/ipc.127, compare) with libmultiprocess extends support https://github.com/chaincodelabs/libmultiprocess/pull/38 Updated e9523ebd6e312422c2a891d3004678ba4459d87f -> dd71ab2683c5b69dbacf03f7144cb87e016442f2 (pr/ipc.127 -> pr/ipc.128, compare) to fix travis errors from missing include https://travis-ci.org/github/bitcoin/bitcoin/jobs/726837187 https://travis-ci.org/github/bitcoin/bitcoin/jobs/726837186 https://travis-ci.org/github/bitcoin/bitcoin/jobs/726837185 Updated dd71ab2683c5b69dbacf03f7144cb87e016442f2 -> 3724d0a48c2a82b015738a71cc3580bb48ffdd84 (pr/ipc.128 -> pr/ipc.129, compare) to fix travis missing make dependency error https://travis-ci.org/github/bitcoin/bitcoin/jobs/726899109 Rebased 3724d0a48c2a82b015738a71cc3580bb48ffdd84 -> 8605bae0a5cd4aa1a66a6b3020b23de077cf9121 (pr/ipc.129 -> pr/ipc.130, compare) due to silent conflicts with #15454 and #19572 on top of #19160 pr/ipc-echo.14 Rebased 8605bae0a5cd4aa1a66a6b3020b23de077cf9121 -> 5bd02e8581c4d9fecbaed6f93b4f95920c5cb024 (pr/ipc.130 -> pr/ipc.131, compare) due to conflict with #19725 on top of #19160 pr/ipc-echo.14 Rebased 5bd02e8581c4d9fecbaed6f93b4f95920c5cb024 -> d779c4af435b6d06f29afd07772eb348a0ed94f9 (pr/ipc.131 -> pr/ipc.132, compare) on top of #19160 pr/ipc-echo.16 for cmake depends fix Updated d779c4af435b6d06f29afd07772eb348a0ed94f9 -> 8f155d7afc41760a36972ae353a76d876cf4445f (pr/ipc.132 -> pr/ipc.133, compare) to fix c++11/c++17 constexpr linkage error https://travis-ci.org/github/bitcoin/bitcoin/jobs/731908474#L3044

  517. DrahtBot removed the label Needs rebase on Sep 13, 2020
  518. ryanofsky force-pushed on Sep 14, 2020
  519. ryanofsky force-pushed on Sep 14, 2020
  520. DrahtBot added the label Needs rebase on Sep 18, 2020
  521. ryanofsky referenced this in commit c832c85561 on Sep 24, 2020
  522. ryanofsky referenced this in commit d563dfa46a on Sep 24, 2020
  523. ryanofsky referenced this in commit 4e7906269c on Sep 25, 2020
  524. ryanofsky referenced this in commit 97e683d1a1 on Sep 25, 2020
  525. ryanofsky force-pushed on Sep 26, 2020
  526. DrahtBot removed the label Needs rebase on Sep 26, 2020
  527. DrahtBot added the label Needs rebase on Sep 26, 2020
  528. ryanofsky force-pushed on Sep 28, 2020
  529. DrahtBot removed the label Needs rebase on Sep 28, 2020
  530. ryanofsky referenced this in commit c915cea0db on Oct 1, 2020
  531. ryanofsky referenced this in commit a8fbbd7364 on Oct 1, 2020
  532. ryanofsky force-pushed on Oct 1, 2020
  533. ryanofsky force-pushed on Oct 2, 2020
  534. ryanofsky referenced this in commit 2f521b2c24 on Oct 2, 2020
  535. ryanofsky referenced this in commit bf525a8d60 on Oct 21, 2020
  536. 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.

  537. DrahtBot added the label Needs rebase on Oct 27, 2020
  538. janus referenced this in commit b5e8dd35ed on Nov 5, 2020
  539. janus referenced this in commit ee612c6fce on Nov 15, 2020
  540. ryanofsky referenced this in commit f663f7031c on Nov 25, 2020
  541. ryanofsky referenced this in commit 7723ab68ca on Nov 25, 2020
  542. ryanofsky referenced this in commit af792718de on Nov 25, 2020
  543. ryanofsky force-pushed on Nov 25, 2020
  544. DrahtBot removed the label Needs rebase on Nov 25, 2020
  545. DrahtBot added the label Needs rebase on Dec 2, 2020
  546. janus referenced this in commit ed6e35e3be on Dec 7, 2020
  547. ryanofsky referenced this in commit d0abe4e56d on Dec 8, 2020
  548. ryanofsky referenced this in commit 3fbbb9a640 on Dec 8, 2020
  549. ryanofsky referenced this in commit 1d1688f600 on Dec 8, 2020
  550. ryanofsky referenced this in commit 40fba17b1a on Dec 8, 2020
  551. maflcko referenced this in commit e98d1d6740 on Dec 8, 2020
  552. ryanofsky referenced this in commit 5a695fd248 on Dec 8, 2020
  553. ryanofsky referenced this in commit cc392c014d on Dec 8, 2020
  554. ryanofsky referenced this in commit c58ddd466c on Dec 11, 2020
  555. ryanofsky referenced this in commit fe184ae2df on Dec 11, 2020
  556. ryanofsky force-pushed on Dec 11, 2020
  557. DrahtBot removed the label Needs rebase on Dec 11, 2020
  558. ryanofsky force-pushed on Dec 11, 2020
  559. ryanofsky force-pushed on Dec 11, 2020
  560. ryanofsky commented at 6:19 pm on December 11, 2020: contributor
    Rebased 8f155d7afc41760a36972ae353a76d876cf4445f -> e3b965e13216acc111bf0241fb14e55c63612a10 (pr/ipc.133 -> pr/ipc.134, compare) on top of #19160 pr/ipc-echo.19 Rebased e3b965e13216acc111bf0241fb14e55c63612a10 -> 62f68cb14e6c8702d14bb34680a6abddf9342b5e (pr/ipc.134 -> pr/ipc.135, compare) on top of #19160 pr/ipc-echo.22 with FoundBlock fixes after #19425 and silent conflict fix after #18766 Updated 62f68cb14e6c8702d14bb34680a6abddf9342b5e -> b5d2ae65b2d870a02e157846236160f35ad6608c (pr/ipc.135 -> pr/ipc.136, compare) to fix unique_ptr forward include errors https://cirrus-ci.com/task/5058230931947520 https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/36782415 Updated b5d2ae65b2d870a02e157846236160f35ad6608c -> a4aaf6993b63b545a341094201011f663ed92fbc (pr/ipc.136 -> pr/ipc.137, compare) with build tweaks to fix appveyor object name error https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/36786087 and cirrus missing make header error https://cirrus-ci.com/task/5234663356628992?command=ci#L2602 Rebased a4aaf6993b63b545a341094201011f663ed92fbc -> 1e3bfe692e978966ad1607b7f3d3679508d39644 (pr/ipc.137 -> pr/ipc.138, compare) on top of #19160 pr/ipc-echo.23 including https://github.com/chaincodelabs/libmultiprocess/pull/41 to fix build errors caused by #20588 Rebased 1e3bfe692e978966ad1607b7f3d3679508d39644 -> 011a8cd6f84c4163c6550dfa3fad696b4458a9c7 (pr/ipc.138 -> pr/ipc.139, compare) on top of #19160 pr/ipc-echo.24 due to (mostly silent) conflicts with #18077, #19829, #20671, #20736, #20755, #20786 Rebased 011a8cd6f84c4163c6550dfa3fad696b4458a9c7 -> 03501ab4b74e45a68ef9fa6a8d51ef99695f7588 (pr/ipc.139 -> pr/ipc.140, compare) on top of #21035 pr/argmap.1 to fix CI failure https://cirrus-ci.com/task/5469433013469184?command=ci#L2275 Rebased 03501ab4b74e45a68ef9fa6a8d51ef99695f7588 -> 1f789912f3dae5aecdce157d0eefdb22c2b5a61c (pr/ipc.140 -> pr/ipc.141, compare) due to silent conflict with #20464 (https://cirrus-ci.com/task/6558387276087296) Rebased 1f789912f3dae5aecdce157d0eefdb22c2b5a61c -> ce6e283ed2fc91994dd23b930feb2a768b0062ad (pr/ipc.141 -> pr/ipc.142, compare) due to silent conflict with #21051 Rebased ce6e283ed2fc91994dd23b930feb2a768b0062ad -> 585585c28f69cc232c2f9e6d1ae96c8d3a11eb5a (pr/ipc.142 -> pr/ipc.143, compare) on top of #19060 pr/ipc-echo.26 due to silent conflict with #20721
  561. Fabcien referenced this in commit c2a9184653 on Dec 14, 2020
  562. DrahtBot added the label Needs rebase on Dec 16, 2020
  563. ryanofsky referenced this in commit a51b9f463d on Dec 18, 2020
  564. ryanofsky referenced this in commit fe0a68197a on Dec 18, 2020
  565. ryanofsky force-pushed on Dec 18, 2020
  566. DrahtBot removed the label Needs rebase on Dec 18, 2020
  567. DrahtBot added the label Needs rebase on Jan 7, 2021
  568. Fabcien referenced this in commit 1d7377e74d on Jan 11, 2021
  569. ryanofsky referenced this in commit 35d2091134 on Jan 28, 2021
  570. ryanofsky referenced this in commit 94b8f19f98 on Jan 28, 2021
  571. ryanofsky force-pushed on Jan 29, 2021
  572. DrahtBot removed the label Needs rebase on Jan 29, 2021
  573. ryanofsky referenced this in commit 8a6e870cbb on Jan 29, 2021
  574. ryanofsky force-pushed on Jan 30, 2021
  575. ryanofsky force-pushed on Feb 1, 2021
  576. ryanofsky force-pushed on Feb 3, 2021
  577. ryanofsky force-pushed on Feb 20, 2021
  578. DrahtBot added the label Needs rebase on Mar 2, 2021
  579. 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.
  580. ryanofsky force-pushed on Mar 4, 2021
  581. ryanofsky commented at 2:39 pm on March 4, 2021: contributor

    re: #10102 (comment)

    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

  582. ryanofsky referenced this in commit f228b9bac4 on Mar 4, 2021
  583. DrahtBot removed the label Needs rebase on Mar 4, 2021
  584. DrahtBot added the label Needs rebase on Mar 8, 2021
  585. ryanofsky referenced this in commit 9048c58e10 on Mar 9, 2021
  586. ryanofsky force-pushed on Mar 9, 2021
  587. DrahtBot removed the label Needs rebase on Mar 9, 2021
  588. DrahtBot added the label Needs rebase on Mar 11, 2021
  589. in src/init/bitcoin-gui.cpp:47 in 26bc68baf4 outdated 
    42+} // namespace init
43+
44+namespace interfaces {
45+std::unique_ptr<Init> MakeGuiInit(int argc, char* argv[])
46+{
47+    return MakeUnique<init::BitcoinGuiInit>(argc > 0 ? argv[0] : "");
    fanquake commented at 0:28 am on March 12, 2021:
    Please use std::make_unique in new code.
  590. maflcko referenced this in commit 1e57d14d96 on Mar 15, 2021
  591. ryanofsky force-pushed on Mar 23, 2021
  592. DrahtBot removed the label Needs rebase on Mar 23, 2021
  593. ariard commented at 2:40 pm on March 26, 2021: contributor

    I’ve been following the instructions in doc/multiprocess.md, especially the ones in ##Installation

    0cd <BITCOIN_SOURCE_DIRECTORY>
1make -C depends NO_QT=1 MULTIPROCESS=1
2./configure --prefix=$PWD/depends/x86_64-pc-linux-gnu
3make
4src/bitcoin-node -regtest -printtoconsole -debug=ipc
5BITCOIND=bitcoin-node test/functional/test_runner.py

    But I’m hitting linker failures likewise :

    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 ?

    EDIT: I’m building well #19160 branch

  594. ryanofsky commented at 6:01 pm on March 26, 2021: contributor

    re: #10102 (comment)

    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.

  595. ryanofsky referenced this in commit ddcfd078b6 on Mar 27, 2021
  596. backpacker69 referenced this in commit b666892912 on Mar 28, 2021
  597. backpacker69 referenced this in commit 415759cd01 on Mar 28, 2021
  598. backpacker69 referenced this in commit 4e20eb5e2e on Mar 28, 2021
  599. backpacker69 referenced this in commit 03726cf79a on Mar 28, 2021
  600. achow101 commented at 7:45 pm on March 29, 2021: member

    Tried to run bitcoin-gui and hit an assert:

    bitcoin-gui: logging.cpp:274: void BCLog::Logger::LogPrintStr(const string&, const string&, const string&, int): Assertion `m_fileout != nullptr' failed.

    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.

  601. 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 info
1bitcoin-wallet -ipcconnect      -wallet=mywallet listtransactions  # List full transaction info
2bitcoin-wallet -ipcconnect=auto -wallet=mywallet listtransactions  # List available transaction info
3
4bitcoin-wallet -noipcconnect    -wallet=mywallet serve # Run limited RPC server
5bitcoin-wallet -ipcconnect      -wallet=mywallet serve # Run full RPC server
6bitcoin-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).

  602. DrahtBot added the label Needs rebase on Mar 30, 2021
  603. ryanofsky force-pushed on Apr 8, 2021
  604. ryanofsky commented at 2:47 am on April 8, 2021: contributor

    re: #10102 (comment)

    Tried to run bitcoin-gui and hit an assert:

    0bitcoin-gui: logging.cpp:274: void BCLog::Logger::LogPrintStr(const string&, const string&, const string&, int): Assertion `m_fileout != nullptr' failed.

    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.

    Rebased 667affafa4b1a086746de9ef17c459e40380829b -> e6e53b5955a184a672d8c74ac25f787f2ae46482 (pr/ipc.146 -> pr/ipc.147, compare) fixing gui assert failure reported by achow in #10102 (comment) and silent merge conflict after #21366 Rebased e6e53b5955a184a672d8c74ac25f787f2ae46482 -> 6e7a814da00c9f8ecaf7c57921515e61e0a037b6 (pr/ipc.147 -> pr/ipc.148, compare) due to silent conflict with #21574 and to fix fuzz timeout https://cirrus-ci.com/task/6058699123851264 #21639

  605. DrahtBot removed the label Needs rebase on Apr 8, 2021
  606. ryanofsky force-pushed on Apr 9, 2021
  607. maflcko referenced this in commit 66fd3b28e8 on Apr 23, 2021
  608. laanwj referenced this in commit ac219dcbcc on Apr 27, 2021
  609. DrahtBot added the label Needs rebase on Apr 27, 2021
  610. random-zebra referenced this in commit 4a470eda19 on Apr 28, 2021
  611. random-zebra referenced this in commit 66f12d2558 on Apr 29, 2021
  612. ryanofsky referenced this in commit 90369e5271 on Apr 30, 2021
  613. ryanofsky force-pushed on Apr 30, 2021
  614. 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

  615. ryanofsky referenced this in commit a5d176c023 on Apr 30, 2021
  616. DrahtBot removed the label Needs rebase on Apr 30, 2021
  617. random-zebra referenced this in commit e2825e0b15 on May 8, 2021
  618. random-zebra referenced this in commit df2c9ab116 on May 17, 2021
  619. ryanofsky referenced this in commit 3287cff0b9 on May 19, 2021
  620. ryanofsky referenced this in commit 26f46feffa on Jun 7, 2021
  621. ryanofsky referenced this in commit fe45c37989 on Jun 7, 2021
  622. ryanofsky referenced this in commit d7bafb5065 on Jun 7, 2021
  623. DrahtBot added the label Needs rebase on Jun 9, 2021
  624. ryanofsky referenced this in commit 93cc53a2b2 on Jun 10, 2021
  625. maflcko referenced this in commit f66eceaecf on Jun 11, 2021
  626. fanquake referenced this in commit 9795e8ec8c on Jun 12, 2021
  627. sidhujag referenced this in commit 5b721bb042 on Jun 13, 2021
  628. ryanofsky force-pushed on Jun 15, 2021
  629. ryanofsky referenced this in commit 75ad998da9 on Jun 15, 2021
  630. DrahtBot removed the label Needs rebase on Jun 15, 2021
  631. ryanofsky force-pushed on Jun 16, 2021
  632. ryanofsky force-pushed on Jun 16, 2021
  633. DrahtBot added the label Needs rebase on Jun 17, 2021
  634. ryanofsky force-pushed on Jun 17, 2021
  635. DrahtBot removed the label Needs rebase on Jun 17, 2021
  636. DrahtBot added the label Needs rebase on Jun 23, 2021
  637. ryanofsky force-pushed on Jun 23, 2021
  638. DrahtBot removed the label Needs rebase on Jun 23, 2021
  639. DrahtBot added the label Needs rebase on Jun 24, 2021
  640. ryanofsky force-pushed on Jun 24, 2021
  641. DrahtBot removed the label Needs rebase on Jun 24, 2021
  642. DrahtBot added the label Needs rebase on Jun 28, 2021
  643. ryanofsky force-pushed on Jul 1, 2021
  644. DrahtBot removed the label Needs rebase on Jul 1, 2021
  645. ryanofsky referenced this in commit 2516d397d5 on Jul 8, 2021
  646. ryanofsky force-pushed on Jul 9, 2021
  647. ryanofsky commented at 5:17 pm on July 9, 2021: contributor
    Rebased 1326c4a9a443e4169822eab569be68e53c58de36 -> 3f2ca2a484a7f040c41d2e29e8bb9e615050694e (pr/ipc.156 -> pr/ipc.157, compare) on top of #22218 pr/ipc-ctx.2 with comment improvements that were discussed in #22218#pullrequestreview-702107209 and https://github.com/bitcoin/bitcoin/commit/2516d397d54b021e37d6e7628655fe7c54d2fb06#commitcomment-53226160 Rebased 3f2ca2a484a7f040c41d2e29e8bb9e615050694e -> bc918ffaaaf108a9bf49325cef7773ba3797feb4 (pr/ipc.157 -> pr/ipc.158, compare) due to merge conflicts with #20354 #22387 and #21528 and silent conflict with #22570
  648. in src/init/bitcoin-node.cpp:44 in 7df963de80 outdated 
    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] {
    ryanofsky commented at 3:49 pm on July 12, 2021:

    In commit “Make bitcoin-gui spawn a bitcoin-node process” (2516d397d54b021e37d6e7628655fe7c54d2fb06):

    re: https://github.com/bitcoin/bitcoin/commit/2516d397d54b021e37d6e7628655fe7c54d2fb06#r53368428

    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

  649. DrahtBot added the label Needs rebase on Jul 14, 2021
  650. maflcko referenced this in commit ba15ab4990 on Jul 22, 2021
  651. hebasto referenced this in commit 439e58c4d8 on Aug 12, 2021
  652. stratospher referenced this in commit 34e8efb5bb on Aug 14, 2021
  653. fanquake referenced this in commit 62cb4009c2 on Aug 18, 2021
  654. ryanofsky force-pushed on Aug 18, 2021
  655. DrahtBot removed the label Needs rebase on Aug 18, 2021
  656. in src/netaddress.h:552 in aa05f698fe outdated 
    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());
    maflcko commented at 5:56 am on August 18, 2021:
    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?
    ryanofsky commented at 4:06 pm on August 31, 2021:

    re: #10102 (review)

    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

  657. sidhujag referenced this in commit cfb4bf97a5 on Aug 20, 2021
  658. meshcollider referenced this in commit e9d6eb1b80 on Aug 22, 2021
  659. ryanofsky referenced this in commit b3f46e9058 on Aug 31, 2021
  660. ryanofsky force-pushed on Aug 31, 2021
  661. 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
    • fixing silent conflict with #22782

    Rebased d7c617e3580195398bbc34f1efcde548a834427e -> a56f8ebeb93e6016b2719644a1212c5a34a44396 (pr/ipc.159 -> pr/ipc.160, compare) after #22219 and #22848 merges

  662. DrahtBot added the label Needs rebase on Sep 1, 2021
  663. ryanofsky referenced this in commit c1e7dc4b89 on Sep 3, 2021
  664. ryanofsky referenced this in commit 19b23cf076 on Sep 3, 2021
  665. ryanofsky referenced this in commit 6919c823cb on Sep 3, 2021
  666. maflcko referenced this in commit 2c6707be8b on Sep 6, 2021
  667. arnabsen1729 referenced this in commit 7e22c06419 on Sep 6, 2021
  668. fanquake referenced this in commit 528e08119f on Sep 16, 2021
  669. ryanofsky force-pushed on Sep 17, 2021
  670. DrahtBot removed the label Needs rebase on Sep 17, 2021
  671. DrahtBot added the label Needs rebase on Sep 24, 2021
  672. ryanofsky force-pushed on Oct 6, 2021
  673. ryanofsky commented at 3:40 am on October 6, 2021: contributor
    Rebased a56f8ebeb93e6016b2719644a1212c5a34a44396 -> 884d3e1dfa0d46472cd26093142331e51fb9f2ae (pr/ipc.160 -> pr/ipc.161, compare) due to conflicts with #12677, #23065, #22818, #23060, #17211, and #22951
  674. DrahtBot removed the label Needs rebase on Oct 6, 2021
  675. DrahtBot added the label Needs rebase on Oct 15, 2021
  676. PastaPastaPasta referenced this in commit 1ada50fee0 on Oct 23, 2021
  677. PastaPastaPasta referenced this in commit 3e8670d81a on Oct 24, 2021
  678. PastaPastaPasta referenced this in commit df0373cd76 on Oct 25, 2021
  679. PastaPastaPasta referenced this in commit 1834aa9a9a on Oct 25, 2021
  680. ryanofsky force-pushed on Oct 25, 2021
  681. 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
  682. DrahtBot removed the label Needs rebase on Oct 25, 2021
  683. maflcko referenced this in commit e77d9679fd on Oct 26, 2021
  684. DrahtBot added the label Needs rebase on Oct 26, 2021
  685. janus referenced this in commit f21dd521fa on Oct 29, 2021
  686. ryanofsky force-pushed on Oct 29, 2021
  687. DrahtBot removed the label Needs rebase on Oct 29, 2021
  688. ryanofsky renamed this:
    [experimental] Multiprocess bitcoin
    Multiprocess bitcoin
    on Nov 1, 2021
  689. PastaPastaPasta referenced this in commit 75fda781df on Nov 1, 2021
  690. PastaPastaPasta referenced this in commit 051bdef7f5 on Nov 1, 2021
  691. PastaPastaPasta referenced this in commit 48826b429d on Nov 3, 2021
  692. ryanofsky force-pushed on Nov 9, 2021
  693. janus referenced this in commit c16023118c on Nov 11, 2021
  694. DrahtBot added the label Needs rebase on Nov 15, 2021
  695. ryanofsky force-pushed on Nov 17, 2021
  696. DrahtBot removed the label Needs rebase on Nov 17, 2021
  697. pravblockc referenced this in commit a7a32011d3 on Nov 18, 2021
  698. DrahtBot added the label Needs rebase on Nov 22, 2021
  699. ryanofsky force-pushed on Nov 29, 2021
  700. ryanofsky force-pushed on Nov 29, 2021
  701. DrahtBot removed the label Needs rebase on Nov 29, 2021
  702. DrahtBot added the label Needs rebase on Nov 30, 2021
  703. in src/ipc/capnp/common-types.h:158 in eae99d609c outdated 
    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.
    Sjors commented at 8:06 am on December 9, 2021:
    eae99d609c7a08fde35e723967d078904ad1cd05 That seems a different Bitcoin Core PR… however there have been a few VectorReader merges recently.
    ryanofsky commented at 6:49 pm on December 17, 2021:

    re: #10102 (review)

    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.

  704. in src/ipc/capnp/handler.capnp:5 in 0a2a04655a outdated 
    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;
    Sjors commented at 8:12 am on December 9, 2021:
    0a2a04655abf572005b7fefb474f47dce0480d32 : what is this?
    ryanofsky commented at 6:49 pm on December 17, 2021:

    re: #10102 (review)

    0a2a046 : what is this?

    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.

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

    When building on macOS, using capnp 0.9.1 via homebrew and https://github.com/chaincodelabs/libmultiprocess/commit/34ce921d2daecb0b20772363621554a2b763b1db, I get flooded with warnings like this:

    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:

     02021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server recv request  [#2955](/bitcoin-bitcoin/2955/) Node.shutdownRequested$Params (context = (thread = <external capability>, callbackThread = <external capability>))
 12021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server post request  [#2955](/bitcoin-bitcoin/2955/) {bitcoin-node-2082/0x7000083d3000 (from bitcoin-gui-2081/0x111e41600)}
 22021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server send response [#2955](/bitcoin-bitcoin/2955/) Node.shutdownRequested$Results (result = false)
 32021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server recv request  [#2956](/bitcoin-bitcoin/2956/) Node.getMempoolSize$Params (context = (thread = <external capability>, callbackThread = <external capability>))
 42021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server post request  [#2956](/bitcoin-bitcoin/2956/) {bitcoin-node-2082/0x7000093b0000 (from bitcoin-gui-2081/b-qt-clientmodl-0x700009e4a000)}
 52021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server send response [#2956](/bitcoin-bitcoin/2956/) Node.getMempoolSize$Results (result = 0)
 62021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server recv request  [#2957](/bitcoin-bitcoin/2957/) Node.getMempoolDynamicUsage$Params (context = (thread = <external capability>, callbackThread = <external capability>))
 72021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server post request  [#2957](/bitcoin-bitcoin/2957/) {bitcoin-node-2082/0x7000093b0000 (from bitcoin-gui-2081/b-qt-clientmodl-0x700009e4a000)}
 82021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server send response [#2957](/bitcoin-bitcoin/2957/) Node.getMempoolDynamicUsage$Results (result = 0)
 92021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server recv request  [#2958](/bitcoin-bitcoin/2958/) Node.getTotalBytesRecv$Params (context = (thread = <external capability>, callbackThread = <external capability>))
102021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server post request  [#2958](/bitcoin-bitcoin/2958/) {bitcoin-node-2082/0x7000093b0000 (from bitcoin-gui-2081/b-qt-clientmodl-0x700009e4a000)}
112021-12-09T08:36:56Z [] {bitcoin-node-2082/0x10d3e0600} IPC server send response [#2958](/bitcoin-bitcoin/2958/) Node.getTotalBytesRecv$Results (result = 60191)

    Though using the log files directly might help; e47c133dfc4e1b8ecec908bcb1bb535f12659205 is very handy.

  706. PhotoshiNakamoto referenced this in commit 3085f314c0 on Dec 11, 2021
  707. PhotoshiNakamoto referenced this in commit bb233e8ba6 on Dec 11, 2021
  708. PhotoshiNakamoto referenced this in commit 89effc4ae5 on Dec 11, 2021
  709. ryanofsky force-pushed on Dec 17, 2021
  710. ryanofsky commented at 9:29 pm on December 17, 2021: contributor

    Rebased eef402d2913fdb9f13af73f9fbc789611cf60360 -> 8a18a12a23277c64e2af05867607ed4471b1a692 (pr/ipc.167 -> pr/ipc.168, compare) due to conflicts with #19499, #23758, #23721, and #23289

    re: #10102#pullrequestreview-827293918

    Thanks for the review!

    When building on macOS, using capnp 0.9.1 via homebrew and chaincodelabs/libmultiprocess@34ce921, I get flooded with warnings like this:

    Thanks, filed https://github.com/chaincodelabs/libmultiprocess/issues/61 to track & fix

    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.

  711. DrahtBot removed the label Needs rebase on Dec 17, 2021
  712. ryanofsky force-pushed on Dec 22, 2021
  713. ryanofsky commented at 6:24 pm on December 22, 2021: contributor
    Updated 8a18a12a23277c64e2af05867607ed4471b1a692 -> c9d036fbbbcaa8ed2f152e96199a1a8c4e9af61a (pr/ipc.168 -> pr/ipc.169, compare) to address Sjors review comments fixing compile warnings, resolving common-types.h todos, and cleaning comon-types.h up in general Rebased c9d036fbbbcaa8ed2f152e96199a1a8c4e9af61a -> 5b01b969d983a18c1c9b355561881e5cd2cceba9 (pr/ipc.169 -> pr/ipc.170, compare) due to conflicts with #23842 and bitcoin-core/gui#459 Rebased 5b01b969d983a18c1c9b355561881e5cd2cceba9 -> 8ae8971c4add0e7d576da1ec42e78677e1a4bc29 (pr/ipc.170 -> pr/ipc.171, compare) updating libmultiprocess to fix CI failure https://github.com/bitcoin/bitcoin/pull/10102/checks?check_run_id=4671391974 https://github.com/chaincodelabs/libmultiprocess/pull/65 and fixing python conflict with #23737 Rebased 8ae8971c4add0e7d576da1ec42e78677e1a4bc29 -> d1b4509566cbd04e22eb4532dbb02cfa2c7431b2 (pr/ipc.171 -> pr/ipc.172, compare) due to conflicts with #23828 and #23737 which caused feature_init failures https://cirrus-ci.com/task/6140731099185152?logs=ci#L3687 https://cirrus-ci.com/task/6476143623667712?logs=ci#L4499 https://cirrus-ci.com/task/4626904987729920?logs=ci#L3459 https://cirrus-ci.com/task/4801360519495680?logs=ci#L2542 https://cirrus-ci.com/task/4852867377922048?logs=ci#L7675 https://cirrus-ci.com/task/5210661306236928?logs=ci#L5807 Rebased d1b4509566cbd04e22eb4532dbb02cfa2c7431b2 -> 69fd898bb1b598293ac54e9125809ee667b82c83 (pr/ipc.172 -> pr/ipc.173, compare) due to conflicts with #23497, #24039, and #24026
  714. DrahtBot added the label Needs rebase on Dec 23, 2021
  715. ryanofsky force-pushed on Dec 31, 2021
  716. DrahtBot removed the label Needs rebase on Dec 31, 2021
  717. DrahtBot added the label Needs rebase on Jan 3, 2022
  718. ryanofsky force-pushed on Jan 3, 2022
  719. DrahtBot removed the label Needs rebase on Jan 3, 2022
  720. Fabcien referenced this in commit 64ea6e739c on Jan 5, 2022
  721. DrahtBot added the label Needs rebase on Jan 6, 2022
  722. ryanofsky force-pushed on Jan 7, 2022
  723. DrahtBot removed the label Needs rebase on Jan 7, 2022
  724. DrahtBot added the label Needs rebase on Jan 11, 2022
  725. ryanofsky force-pushed on Jan 13, 2022
  726. DrahtBot removed the label Needs rebase on Jan 13, 2022
  727. Sjors commented at 2:51 pm on January 14, 2022: member

    tACK 69fd898bb1b598293ac54e9125809ee667b82c83 modulo crash :-(

    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:

     02022-01-14T14:49:48Z [] {bitcoin-node-75387/0x10aba1600} IPC server recv request  [#202](/bitcoin-bitcoin/202/) Chain.destroy$Params (context = (thread = <external capability>, callbackThread = <external capability>))
 12022-01-14T14:49:48Z [] {bitcoin-node-75387/0x10aba1600} IPC server post request  [#202](/bitcoin-bitcoin/202/) {bitcoin-node-75387/0x70000b480000 (from bitcoin-gui-75385/b-qt-init-0x7000103c3000)}
 22022-01-14T14:49:48Z [] {bitcoin-node-75387/0x10aba1600} IPC server send response [#202](/bitcoin-bitcoin/202/) Chain.destroy$Results ()
 32022-01-14T14:49:48Z [] {bitcoin-wallet-75389/0x70000744f000 (from bitcoin-node-75387/0x70000b480000 (from bitcoin-gui-75385/b-qt-init-0x7000103c3000))} IPC client recv Chain.destroy$Results ()
 42022-01-14T14:49:48Z [] {bitcoin-node-75387/0x10aba1600} IPC server destroyN2mp11ProxyServerIN3ipc5capnp8messages5ChainEEE
 52022-01-14T14:49:48Z [] {bitcoin-wallet-75389/0x11c07a600} IPC server send response [#19](/bitcoin-bitcoin/19/) ChainClient.destroy$Results ()
 62022-01-14T14:49:48Z [] {bitcoin-node-75387/0x70000b480000 (from bitcoin-gui-75385/b-qt-init-0x7000103c3000)} IPC client recv ChainClient.destroy$Results ()
 72022-01-14T14:49:48Z [shutoff] {bitcoin-node-75387/0x70000b480000 (from bitcoin-gui-75385/b-qt-init-0x7000103c3000)} IPC client destroy N2mp11ProxyClientIN3ipc5capnp8messages4InitEEE
 82022-01-14T14:49:48Z [] {bitcoin-wallet-75389/0x11c07a600} IPC server destroyN2mp11ProxyServerIN3ipc5capnp8messages12WalletLoaderEEE
 92022-01-14T14:49:48Z [] {bitcoin-wallet-75389/0x11c07a600} IPC server: socket disconnected.
102022-01-14T14:49:48Z [] {bitcoin-wallet-75389/0x11c07a600} IPC server destroyN2mp11ProxyServerIN3ipc5capnp8messages4InitEEE
112022-01-14T14:49:48Z [] {bitcoin-wallet-75389/0x11c07a600} EventLoop::loop done, cancelling event listeners.
122022-01-14T14:49:48Z [] {bitcoin-wallet-75389/0x11c07a600} EventLoop::loop bye.
132022-01-14T14:49:48Z [shutoff] Process bitcoin-wallet pid 75389 exited with status 0
142022-01-14T14:49:48Z [shutoff] Shutdown: done
152022-01-14T14:49:48Z [] {bitcoin-node-75387/0x10aba1600} IPC server send response [#116](/bitcoin-bitcoin/116/) Node.appShutdown$Results ()
162022-01-14T14:49:48Z [] {bitcoin-node-75387/0x10aba1600} IPC server recv request  [#203](/bitcoin-bitcoin/203/) Node.appShutdown$Params (context = (thread = <external capability>, callbackThread = <external capability>))
172022-01-14T14:49:48Z [] {bitcoin-node-75387/0x10aba1600} IPC server post request  [#203](/bitcoin-bitcoin/203/) {bitcoin-node-75387/0x70000b480000 (from bitcoin-gui-75385/b-qt-init-0x7000103c3000)}
182022-01-14T14:49:48Z [shutoff] Shutdown: In progress...
19Assertion failed: ("node.args" && check), function operator(), file init.cpp, line 203.
202022-01-14T14:49:48Z [qt-init] 
21
22************************
23EXCEPTION: N3ipc9ExceptionE       
24kj::Exception: capnp/rpc.c++:2413: disconnected: Peer disconnected.
25stack: 1057544ea 105765242 105221a30 105222360       
26bitcoin in Runaway exception
  728. in src/ipc/capnp/common-types.h:363 in cdd239245c outdated 
    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(
    Sjors commented at 2:53 pm on January 14, 2022:
    cdd239245c30e167a853a75ba37ace42ad5376a3 nit: it’s a bit odd to introduce this here, since this commit doesn’t use it.
    ryanofsky commented at 8:27 pm on February 4, 2022:

    re: #10102 (review)

    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.

  729. DrahtBot added the label Needs rebase on Jan 31, 2022
  730. uvhw referenced this in commit 47d44ccc3e on Feb 14, 2022
  731. in src/net_processing.h:41 in a5116a57a9 outdated 
    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).
    ariard commented at 6:00 pm on April 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.
    ryanofsky commented at 3:54 pm on September 12, 2022:

    re: #10102 (review)

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

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

  733. Fabcien referenced this in commit 5e2fdfcbd4 on May 5, 2022
  734. ryanofsky referenced this in commit a1d72012c7 on Aug 24, 2022
  735. ryanofsky force-pushed on Aug 24, 2022
  736. DrahtBot removed the label Needs rebase on Aug 24, 2022
  737. DrahtBot added the label Needs rebase on Aug 30, 2022
  738. ryanofsky referenced this in commit 6c6e0ee75a on Sep 8, 2022
  739. ryanofsky force-pushed on Sep 8, 2022
  740. ryanofsky referenced this in commit 8da740308f on Sep 8, 2022
  741. ryanofsky referenced this in commit 9b587201f1 on Sep 8, 2022
  742. ryanofsky force-pushed on Sep 8, 2022
  743. DrahtBot removed the label Needs rebase on Sep 8, 2022
  744. 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 10181ced0
4zsh: segmentation fault  src/bitcoin-gui

    When shutting down the GUI very quickly after starting, it shuts down cleanly now.

  745. ryanofsky referenced this in commit 1f0b8eb161 on Sep 12, 2022
  746. ryanofsky force-pushed on Sep 12, 2022
  747. ryanofsky commented at 3:51 pm on September 12, 2022: contributor

    re: #10102 (comment)

    🙌 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

  748. Sjors commented at 7:04 am on September 13, 2022: member
    @ryanofsky linter complains: Unrecognized address type. Serialization not implemented for
  749. ryanofsky referenced this in commit 150f2fa5da on Sep 13, 2022
  750. ryanofsky force-pushed on Sep 13, 2022
  751. ryanofsky commented at 7:21 pm on September 13, 2022: contributor

    @ryanofsky linter complains: Unrecognized address type. Serialization not implemented for

    Thanks, fixed now.

    Updated d0bea139b7a4a158eea6fec62285646a826a62b9 -> d6277e015c2cca0f6e890989024b7edef52da1cb (pr/ipc.177 -> pr/ipc.178, compare) fixing Unrecognized address type strprintf

  752. 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!)
  753. 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
  754. 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.

  755. aureleoules commented at 8:30 am on September 15, 2022: contributor
    @JeremyRubin if that helps, I wrote a script to expand PR comments: https://gist.github.com/aureleoules/6bafcf3e255bfd528ef4e3d034f87dbc. Takes a bit of time to run on this PR though.
  756. ryanofsky referenced this in commit a9587178bc on Sep 20, 2022
  757. ryanofsky force-pushed on Sep 20, 2022
  758. ryanofsky referenced this in commit e690496ea7 on Sep 20, 2022
  759. ryanofsky force-pushed on Sep 20, 2022
  760. 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
  761. ryanofsky referenced this in commit 17ad236d67 on Sep 26, 2022
  762. ryanofsky force-pushed on Sep 26, 2022
  763. ryanofsky referenced this in commit 40a92f0fa0 on Sep 26, 2022
  764. 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)
  765. ryanofsky referenced this in commit 4d84f9ba64 on Sep 26, 2022
  766. ryanofsky force-pushed on Sep 26, 2022
  767. 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.
  768. jamesob commented at 11:32 am on October 25, 2022: contributor
    Concept ACK, will review this week.
  769. DrahtBot added the label Needs rebase on Dec 6, 2022
  770. ryanofsky referenced this in commit dfb7b92f02 on Jan 20, 2023
  771. ryanofsky force-pushed on Jan 20, 2023
  772. DrahtBot removed the label Needs rebase on Jan 20, 2023
  773. 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)

    https://cirrus-ci.com/task/4576796809625600

  774. ryanofsky force-pushed on Feb 6, 2023
  775. ryanofsky force-pushed on Feb 10, 2023
  776. ryanofsky commented at 5:37 pm on February 13, 2023: contributor

    re: #10102 (comment)

    Not sure if related or not:

    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

  777. DrahtBot added the label Needs rebase on Feb 28, 2023
  778. ryanofsky force-pushed on Feb 28, 2023
  779. DrahtBot removed the label Needs rebase on Feb 28, 2023
  780. DrahtBot added the label Needs rebase on Mar 11, 2023
  781. ryanofsky force-pushed on Mar 16, 2023
  782. DrahtBot removed the label Needs rebase on Mar 16, 2023
  783. DrahtBot added the label Needs rebase on Apr 11, 2023
  784. ryanofsky force-pushed on Apr 13, 2023
  785. DrahtBot removed the label Needs rebase on Apr 13, 2023
  786. PastaPastaPasta referenced this in commit a094d50f33 on Apr 15, 2023
  787. PastaPastaPasta referenced this in commit 00f58c0a2e on Apr 15, 2023
  788. PastaPastaPasta referenced this in commit f5557bed67 on Apr 16, 2023
  789. PastaPastaPasta referenced this in commit dc031ca862 on Apr 16, 2023
  790. UdjinM6 referenced this in commit a4e5458daa on Apr 16, 2023
  791. DrahtBot added the label Needs rebase on Apr 21, 2023
  792. 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.

    0./configure --enable-suppress-external-warnings  --enable-c++20  --with-gui --enable-multiprocess --disable-bench --disable-tests --disable-external-signer --disable-fuzz-binary --disable-wallet
1...
2make
3...
4  CXXLD    bitcoin-gui
5Undefined symbols for architecture x86_64:
6  "wallet::CCoinControl::CCoinControl()", referenced from:
7      void mp::PassField<mp::Accessor<mp::wallet_fields::CoinControl, 17>, wallet::CCoinControl const&, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Wallet>, capnp::CallContext<ipc::capnp::messages::Wallet::CreateTransactionParams, ipc::capnp::messages::Wallet::CreateTransactionResults>>, mp::ServerField<1, mp::Accessor<mp::wallet_fields::Sign, 1>, mp::ServerField<1, mp::Accessor<mp::wallet_fields::ChangePos, 3>, mp::ServerField<1, mp::Accessor<mp::wallet_fields::Fee, 2>, mp::ServerRet<mp::Accessor<mp::wallet_fields::Result, 18>, mp::ServerCall>>>> const&, mp::TypeList<bool, int&, long long&>, std::__1::vector<wallet::CRecipient, std::__1::allocator<wallet::CRecipient>> const&>(mp::Priority<0>, mp::TypeList<wallet::CCoinControl const&>, mp::ServerInvokeContext<mp::ProxyServer<ipc::capnp::messages::Wallet>, capnp::CallContext<ipc::capnp::messages::Wallet::CreateTransactionParams, ipc::capnp::messages::Wallet::CreateTransactionResults>>&, mp::ServerField<1, mp::Accessor<mp::wallet_fields::Sign, 1>, mp::ServerField<1, mp::Accessor<mp::wallet_fields::ChangePos, 3>, mp::ServerField<1, mp::Accessor<mp::wallet_fields::Fee, 2>, mp::ServerRet<mp::Accessor<mp::wallet_fields::Result, 18>, mp::ServerCall>>>> const&, mp::TypeList<bool, int&, long long&>&&, std::__1::vector<wallet::CRecipient, std::__1::allocator<wallet::CRecipient>> const&) in libbitcoin_ipc.a(libbitcoin_ipc_a-wallet.capnp.proxy-server.o)
8...

    With the wallet enabled it builds fine (and connecting my GUI over SSH to a daemon with the followup PR still works great).

  793. ryanofsky force-pushed on May 2, 2023
  794. DrahtBot removed the label Needs rebase on May 2, 2023
  795. DrahtBot added the label CI failed on May 2, 2023
  796. DrahtBot removed the label CI failed on May 2, 2023
  797. ryanofsky force-pushed on May 4, 2023
  798. Sjors commented at 10:51 am on May 5, 2023: member

    I was able to reproduce the crash I found here using only this PR.

    Details: running Ubuntu 23.04, libmultiprocess at fc28a48f01af9730be3b49585e718e11c5eea0c5 and capnproto 0.10.4 compiled from source (because of https://github.com/chaincodelabs/libmultiprocess/issues/68#issuecomment-1527856219).

     0./configure --enable-werror --enable-suppress-external-warnings --without-gui --enable-multiprocess
 1…
 2src/bitcoin-node
 3…
 4
 52023-05-05T10:45:36Z New outbound peer connected: version: 70016, blocks=788360, peer=1 (outbound-full-relay)
 6^C2023-05-05T10:45:41Z [rpc] Interrupting HTTP RPC server
 72023-05-05T10:45:41Z [rpc] Interrupting RPC
 82023-05-05T10:45:41Z tor: Thread interrupt
 92023-05-05T10:45:41Z dnsseed thread exit
102023-05-05T10:45:41Z addcon thread exit
112023-05-05T10:45:41Z torcontrol thread exit
122023-05-05T10:45:41Z Shutdown: In progress...
132023-05-05T10:45:41Z [rpc] Stopping HTTP RPC server
142023-05-05T10:45:41Z [rpc] Stopping RPC
152023-05-05T10:45:41Z [rpc] RPC stopped.
16terminate called after throwing an instance of 'ipc::Exception'
17  what():  kj::Exception: kj/async-io-unix.c++:498: disconnected: ::read(fd, buffer, maxBytes): Connection reset by peer
18stack: 7f689c7ae644 7f689c7bdeab 7f689c74d1b5 7f689c7a4db0 7f689c91e720 7f689c917c20 7f689c91bdf0 7f689c97fb30 7f689c97a220 7f689c96fcc0 7f689c96b800 7f689c95b5d0 7f689c95c084 7f689c958610 55678261ed80 556782623610
19Aborted (core dumped)

    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.

    Running bitcoind does not have this issue.

  799. Sjors commented at 1:44 pm on May 8, 2023: member

    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.

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

  801. knst referenced this in commit f27c4140b0 on May 25, 2023
  802. PastaPastaPasta referenced this in commit fc930dc0f4 on May 31, 2023
  803. bitcoin deleted a comment on Jun 5, 2023
  804. bitcoin deleted a comment on Jun 5, 2023
  805. DrahtBot added the label Needs rebase on Jun 20, 2023
  806. knst referenced this in commit ab426edc77 on Jul 6, 2023
  807. knst referenced this in commit 627dd17132 on Jul 24, 2023
  808. knst referenced this in commit 4c00a33cce on Jul 28, 2023
  809. PastaPastaPasta referenced this in commit 4f00d45e53 on Aug 3, 2023
  810. vijaydasmp referenced this in commit 73500e22ca on Aug 11, 2023
  811. knst referenced this in commit eb9e1a4a49 on Aug 17, 2023
  812. knst referenced this in commit 8d6013ff4b on Aug 17, 2023
  813. atangangabbieian4 commented at 5:32 am on September 19, 2023: none

    This PR is part of the process separation project.

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

  814. ryanofsky force-pushed on Sep 20, 2023
  815. 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
  816. DrahtBot added the label CI failed on Sep 20, 2023
  817. DrahtBot removed the label Needs rebase on Sep 20, 2023
  818. maflcko commented at 10:55 am on September 21, 2023: member
    CI looks a bit red
  819. 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

  820. ryanofsky force-pushed on Sep 26, 2023
  821. ryanofsky force-pushed on Sep 26, 2023
  822. ryanofsky force-pushed on Sep 28, 2023
  823. ryanofsky force-pushed on Sep 28, 2023
  824. ryanofsky force-pushed on Sep 28, 2023
  825. ryanofsky force-pushed on Sep 28, 2023
  826. ryanofsky force-pushed on Sep 29, 2023
  827. ryanofsky force-pushed on Sep 30, 2023
  828. DrahtBot removed the label CI failed on Sep 30, 2023
  829. DrahtBot added the label Needs rebase on Oct 2, 2023
  830. achow101 referenced this in commit fbcf1029a7 on Oct 17, 2023
  831. ryanofsky force-pushed on Oct 20, 2023
  832. ryanofsky commented at 4:40 pm on October 20, 2023: contributor
    Rebased d898e962d7f7a3a084158530ae0ec135212cd872 -> f4f3d7b029cdb9d9d6b8ed2e5cf92313f76a3c69 (pr/ipc.199 -> pr/ipc.200, compare) due to conflicts with #27596, #28508, and #28556
  833. DrahtBot removed the label Needs rebase on Oct 20, 2023
  834. DrahtBot added the label CI failed on Oct 20, 2023
  835. Frank-GER referenced this in commit 285ddfd388 on Oct 21, 2023
  836. ryanofsky marked this as a draft on Oct 24, 2023
  837. ryanofsky force-pushed on Oct 24, 2023
  838. gades referenced this in commit d57ea41ab3 on Nov 10, 2023
  839. fanquake referenced this in commit 29c2c90362 on Nov 13, 2023
  840. DrahtBot added the label Needs rebase on Nov 13, 2023
  841. vijaydasmp referenced this in commit e1339a4f29 on Nov 20, 2023
  842. vijaydasmp referenced this in commit 67941b723f on Nov 20, 2023
  843. vijaydasmp referenced this in commit 91a8ddebb2 on Nov 20, 2023
  844. ryanofsky force-pushed on Nov 21, 2023
  845. 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
  846. DrahtBot removed the label Needs rebase on Nov 21, 2023
  847. DrahtBot removed the label CI failed on Nov 21, 2023
  848. ryanofsky force-pushed on Nov 23, 2023
  849. DrahtBot added the label CI failed on Nov 23, 2023
  850. DrahtBot added the label Needs rebase on Nov 28, 2023
  851. ryanofsky force-pushed on Nov 28, 2023
  852. DrahtBot removed the label Needs rebase on Nov 28, 2023
  853. DrahtBot added the label Needs rebase on Nov 30, 2023
  854. gades referenced this in commit 0dd12b7cc7 on Dec 9, 2023
  855. vijaydasmp referenced this in commit ad023978ce on Dec 11, 2023
  856. PastaPastaPasta referenced this in commit 496a279e16 on Dec 28, 2023
  857. knst referenced this in commit 22e2d966c8 on Dec 28, 2023
  858. PastaPastaPasta referenced this in commit 89940ae821 on Jan 1, 2024
  859. knst referenced this in commit 756551e1c6 on Jan 11, 2024
  860. knst referenced this in commit 327ad380ea on Jan 11, 2024
  861. knst referenced this in commit b3b8097e52 on Jan 11, 2024
  862. ryanofsky force-pushed on Jan 11, 2024
  863. DrahtBot removed the label Needs rebase on Jan 12, 2024
  864. knst referenced this in commit ceae7ad2a6 on Jan 12, 2024
  865. knst referenced this in commit fbf1b56c02 on Jan 13, 2024
  866. knst referenced this in commit 848a58b204 on Jan 15, 2024
  867. knst referenced this in commit 9d43d18df8 on Jan 15, 2024
  868. PastaPastaPasta referenced this in commit 098d0fd430 on Jan 16, 2024
  869. DrahtBot added the label Needs rebase on Jan 23, 2024
  870. achow101 referenced this in commit 2f218c664b on Jan 23, 2024
  871. vijaydasmp referenced this in commit 5430b2b7fe on Feb 3, 2024
  872. vijaydasmp referenced this in commit f54ab8fd4f on Feb 5, 2024
  873. vijaydasmp referenced this in commit 19340bc009 on Feb 23, 2024
  874. 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.

  875. vijaydasmp referenced this in commit f1ff241074 on May 6, 2024
  876. vijaydasmp referenced this in commit f8dd9648f7 on May 6, 2024
  877. vijaydasmp referenced this in commit 01dfa3ac0b on May 6, 2024
  878. vijaydasmp referenced this in commit 51869ea9ba on May 6, 2024
  879. vijaydasmp referenced this in commit cb743ed3b0 on May 7, 2024
  880. vijaydasmp referenced this in commit a37d695dba on May 7, 2024
  881. vijaydasmp referenced this in commit c7ae033432 on May 16, 2024
  882. PastaPastaPasta referenced this in commit bf72bea014 on May 19, 2024
  883. 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.

  884. ryanofsky commented at 5:33 pm on June 10, 2024: contributor

    re: #10102 (comment)

    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.

  885. ryanofsky force-pushed on Jun 11, 2024
  886. DrahtBot removed the label Needs rebase on Jun 11, 2024
  887. ryanofsky force-pushed on Jun 11, 2024
  888. ryanofsky referenced this in commit 8da0524628 on Jun 13, 2024
  889. ryanofsky force-pushed on Jun 13, 2024
  890. ryanofsky commented at 6:55 pm on June 13, 2024: contributor

    re: #10102 (comment)

    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.

  891. DrahtBot removed the label CI failed on Jun 13, 2024
  892. 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!

  893. DrahtBot added the label Needs rebase on Jun 25, 2024
  894. knst referenced this in commit 7303854d12 on Jul 12, 2024
  895. knst referenced this in commit 4b5b54e290 on Jul 12, 2024
  896. knst referenced this in commit 91d46b9bdf on Jul 12, 2024
  897. knst referenced this in commit ecc2e2cdf8 on Jul 12, 2024
  898. knst referenced this in commit 95fbf7d9c3 on Jul 13, 2024
  899. knst referenced this in commit 84b33517e3 on Jul 13, 2024
  900. knst referenced this in commit 63c5d342ba on Jul 15, 2024
  901. knst referenced this in commit 1c917c76f0 on Jul 15, 2024
  902. knst referenced this in commit c7413d72b1 on Jul 15, 2024
  903. knst referenced this in commit 2f7814acdd on Jul 15, 2024
  904. knst referenced this in commit 6e5c07a4d1 on Jul 16, 2024
  905. knst referenced this in commit 56dd686404 on Jul 16, 2024
  906. knst referenced this in commit fea67f4f66 on Jul 23, 2024
  907. knst referenced this in commit 9002342f94 on Jul 23, 2024
  908. knst referenced this in commit 25e7d5446e on Jul 23, 2024
  909. knst referenced this in commit a08b66693f on Jul 23, 2024
  910. knst referenced this in commit 040c188de2 on Jul 24, 2024
  911. knst referenced this in commit c5b5b927af on Jul 24, 2024
  912. knst referenced this in commit 172d9d0be4 on Jul 24, 2024
  913. ryanofsky referenced this in commit ca2cfedd2b on Jul 24, 2024
  914. ryanofsky referenced this in commit a9e16da55e on Jul 24, 2024
  915. ryanofsky force-pushed on Jul 26, 2024
  916. DrahtBot removed the label Needs rebase on Jul 26, 2024
  917. knst referenced this in commit 3411577473 on Jul 27, 2024
  918. knst referenced this in commit 94d5230b18 on Aug 3, 2024
  919. knst referenced this in commit a7993de34b on Aug 3, 2024
  920. knst referenced this in commit 382047c971 on Aug 7, 2024
  921. knst referenced this in commit aa2d3b8164 on Aug 7, 2024
  922. knst referenced this in commit 5b0b30e7be on Aug 7, 2024
  923. PastaPastaPasta referenced this in commit 9d9d97d4fd on Aug 14, 2024
  924. hebasto added the label Needs CMake port on Aug 16, 2024
  925. knst referenced this in commit 1087849955 on Aug 27, 2024
  926. DrahtBot added the label Needs rebase on Aug 28, 2024
  927. maflcko removed the label Needs CMake port on Aug 29, 2024
  928. vijaydasmp referenced this in commit 71056b45da on Sep 5, 2024
  929. vijaydasmp referenced this in commit d76cd26f15 on Sep 6, 2024
  930. vijaydasmp referenced this in commit 68a18017d0 on Sep 6, 2024
  931. knst referenced this in commit aebd95938b on Sep 6, 2024
  932. vijaydasmp referenced this in commit e94a654385 on Sep 8, 2024
  933. achow101 referenced this in commit df3f63ccfa on Sep 9, 2024
  934. ryanofsky force-pushed on Sep 19, 2024
  935. DrahtBot added the label CI failed on Sep 19, 2024
  936. DrahtBot commented at 12:12 pm on September 19, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30370431135

    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.

  937. DrahtBot removed the label Needs rebase on Sep 19, 2024
  938. vijaydasmp referenced this in commit d7a20b3ee6 on Sep 25, 2024
  939. DrahtBot added the label Needs rebase on Sep 25, 2024
  940. 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
  941. Add capnp wrapper for Handler interface 76fd4e56c4
  942. Add capnp wrapper for Chain interface 968f14fc7a
  943. Merge remote-tracking branch 'origin/pull/29409/head' 77d96a081c
  944. test: Increase feature_block.py and feature_taproot.py timeouts 
    Needed because BlockConnected notifications are a lot slower with the wallet
running in separate process.
    1045047a73
  945. test: Fix multiprocess test for unclean shutdown on kill 878af18fee
  946. util: Add util::Result workaround to be compatible with libmultiprocess 
    Make default constructor more generic so it doesn't only work with void types.
    3b6e94e078
  947. multiprocess: Add capnp serialization code for bitcoin types 05c74a2d4c
  948. multiprocess: Add capnp wrapper for Wallet interface e23ed53cf9
  949. multiprocess: Add capnp wrapper for Node interface c7c0c37f60
  950. multiprocess: Make bitcoin-gui spawn a bitcoin-node process 
    Spawn node subprocess instead of running node code internally
    fd311f2af1
  951. multiprocess: Make bitcoin-node spawn a bitcoin-wallet process 
    Spawn wallet subprocess instead of running wallet code internally
    9ca113f326
  952. multiprocess: Add debug.log .wallet/.gui suffixes 
    Add .wallet/.gui suffixes to log files created by bitcoin-gui and
bitcoin-wallet processes so they don't clash with bitcoin-node log file.
    c78ea785a9
  953. doc: Multiprocess misc doc and comment updates 4c4273615a
  954. combine_logs: Handle multiprocess wallet log files a295345d3c
  955. ryanofsky force-pushed on Sep 26, 2024
  956. DrahtBot removed the label Needs rebase on Sep 26, 2024
  957. DrahtBot added the label Needs rebase on Nov 21, 2024
  958. DrahtBot commented at 2:08 pm on November 21, 2024: contributor

    🐙 This pull request conflicts with the target branch and needs rebase.


ryanofsky jonasschnelli gmaxwell dcousens laanwj sipa Sjors practicalswift leishman fingera DrahtBot ismail120572 jb55 jgarzik hebasto JeremyRubin maflcko promag electorr fanquake ariard achow101 jonatack aureleoules jamesob atangangabbieian4 willcl-ark

Labels
GUI RPC/REST/ZMQ Needs rebase CI failed

github-metadata-mirror

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-11-23 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me