bitcoin Multiprocess bitcoin

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.

Mar 27 '17 21:03 ryanofsky

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?

Mar 28 '17 07:03 jonasschnelli

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.

Mar 28 '17 09:03 ryanofsky

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.

Mar 28 '17 17:03 gmaxwell

strong concept ACK, but if is feasible, would prefer usage of the existing RPC instead of capn'proto

Mar 28 '17 22:03 dcousens

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.

Mar 29 '17 06:03 laanwj

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.

Apr 07 '17 20:04 ryanofsky

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.

Apr 10 '17 22:04 ryanofsky

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

Apr 14 '17 05:04 ryanofsky

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

Apr 14 '17 08:04 laanwj

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

Apr 17 '17 12:04 sipa

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?

Apr 17 '17 12:04 sipa

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.

Apr 17 '17 14:04 ryanofsky

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.

Apr 17 '17 18:04 sipa

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)

Apr 27 '17 18:04 ryanofsky

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

Aug 21 '18 19:08 ryanofsky

Compile errors are gone now.

The multiprocess=yes/no line in ./configure only showed up for me after running ./autogen.sh again. This was without using the --enable-multiprocess argument.

The documentation suggests multiprocess isn't on by default, but it is:

./configure --disable-bench --disable-zmq --with-miniupnpc=no --with-incompatible-bdb --with-qrencode
....
Options used to compile and link:
  multiprocess  = yes

I do still see a few warnings that appear related to this change:

interfaces/capnp/util.cpp:15:90: warning: 'syscall' is deprecated: first deprecated in macOS 10.12 - syscall(2) is unsupported; please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost(). [-Wdeprecated-declarations]
    return strprintf("%s-%i/%s-%i", exe_name ? exe_name : "", getpid(), thread_name, int(syscall(SYS_gettid)));
                                                                                         ^
/usr/include/unistd.h:745:6: note: 'syscall' has been explicitly marked deprecated here
int      syscall(int, ...);
         ^
1 warning generated.

Should the test suite use these new binaries? In order to prevent the functional suite from taking forever to run, perhaps half of the default test suite nodes could use bitcoind and the other half bitcoin-node?

The bitcoin-gui executable is ignoring testnet=1 in bitcoin.conf. bitcoin-node does honor it. When launched with -testnet it correctly sees global options in bitcoin.conf., but it missed or incorrectly parses options for [test]. E.g. if I set dbcache=1000 under [test] the GUI shows this:

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.

Aug 26 '18 10:08 Sjors

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

Aug 27 '18 14:08 ryanofsky

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.

Aug 28 '18 11:08 Sjors

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.

Sep 05 '18 14:09 ryanofsky

createwallet "test" froze the app for me

This is fixed in pr/ipc.56

Sep 06 '18 18:09 ryanofsky

help gcc 7.3.0:

  CXX      interfaces/capnp/libbitcoin_util_a-messages.o
In file included from ../../src/interfaces/capnp/proxy.h:6:0,
                 from ../../src/interfaces/capnp/messages.h:5,
                 from ./interfaces/capnp/messages.capnp.proxy.h:6,
                 from ../../src/interfaces/capnp/messages-impl.h:9,
                 from ../../src/interfaces/capnp/messages.cpp:1:
../../src/interfaces/capnp/proxy.h: In instantiation of ‘struct interfaces::capnp::ListOutput<capnp::List<interfaces::capnp::messages::Pair<capnp::Text, capnp::List<capnp::Text> > > >’:
../../src/interfaces/capnp/proxy-impl.h:945:13:   required from ‘void interfaces::capnp::BuildField(interfaces::capnp::TypeList<std::map<K, V, std::less<_Key>, std::allocator<std::pair<const _Key, _Tp> > > >, interfaces::capnp::Priority<1>, interfaces::capnp::InvokeContext&, Value&&, Output&&) [with KeyLocalType = std::__cxx11::basic_string<char>; ValueLocalType = std::vector<std::__cxx11::basic_string<char> >; Value = const std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >&; Output = interfaces::capnp::StructField<interfaces::capnp::Accessor<interfaces::capnp::FieldOverrideArgs, 19>, interfaces::capnp::messages::GlobalArgs::Builder>&]’
../../src/interfaces/capnp/proxy-impl.h:1060:15:   required from ‘void interfaces::capnp::BuildField(interfaces::capnp::TypeList<const LocalType>, interfaces::capnp::Priority<0>, interfaces::capnp::InvokeContext&, Value&&, Output&&) [with LocalType = std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >; Value = const std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >&; Output = interfaces::capnp::StructField<interfaces::capnp::Accessor<interfaces::capnp::FieldOverrideArgs, 19>, interfaces::capnp::messages::GlobalArgs::Builder>&]’
../../src/interfaces/capnp/proxy-impl.h:1071:15:   required from ‘void interfaces::capnp::BuildField(interfaces::capnp::TypeList<LocalType&>, interfaces::capnp::Priority<0>, interfaces::capnp::InvokeContext&, Value&&, Output&&, void*) [with LocalType = const std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >; Value = const std::map<std::__cxx11::basic_string<char>, std::vector<std::__cxx11::basic_string<char> > >&; Output = interfaces::capnp::StructField<interfaces::capnp::Accessor<interfaces::capnp::FieldOverrideArgs, 19>, interfaces::capnp::messages::GlobalArgs::Builder>&]’
../../src/interfaces/capnp/proxy-impl.h:1098:15:   required from ‘void interfaces::capnp::BuildOne(interfaces::capnp::TypeList<ValueLocalType>, interfaces::capnp::InvokeContext&, Value&&, Output&&, typename std::enable_if<(index < interfaces::capnp::ProxyType<LocalType>::fields)>::type*) [with long unsigned int index = 0; LocalType = interfaces::capnp::GlobalArgs; Value = const interfaces::capnp::GlobalArgs&; Output = interfaces::capnp::messages::GlobalArgs::Builder&; typename std::enable_if<(index < interfaces::capnp::ProxyType<LocalType>::fields)>::type = void]’
../../src/interfaces/capnp/proxy-impl.h:1119:16:   required from ‘void interfaces::capnp::BuildField(interfaces::capnp::TypeList<LocalType>, interfaces::capnp::Priority<1>, interfaces::capnp::InvokeContext&, Value&&, Output&&, typename interfaces::capnp::ProxyType<LocalType>::Struct*) [with LocalType = interfaces::capnp::GlobalArgs; Value = const interfaces::capnp::GlobalArgs&; Output = interfaces::capnp::ValueField<interfaces::capnp::messages::GlobalArgs::Builder>; typename interfaces::capnp::ProxyType<LocalType>::Struct = interfaces::capnp::messages::GlobalArgs]’
../../src/interfaces/capnp/messages.cpp:52:109:   required from here
../../src/interfaces/capnp/proxy.h:322:110: error: ‘B’ is not a class or namespace
     template<typename B = Builder, typename Arg> auto set(Arg&& arg) const -> AUTO_RETURN(this->m_builder.B::set(m_index, std::forward<Arg>(arg)))

Sep 21 '18 07:09 fingera

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

Sep 26 '18 20:09 ryanofsky

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:

#30200 (Introduce Mining interface by Sjors)
#29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
#29523 (Wallet: Add max_tx_weight to transaction funding options (take 2) by ismaelsadeeq)
#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.

Sep 27 '18 21:09 DrahtBot

After upgrading to Mojave I'm having compile issues (as of ededfe8800fd2647851d4d42dd765cfcd8d69cde):

In file included from interfaces/capnp/proxy-codegen.cpp:4:
In file included from /usr/local/Cellar/capnp/0.7.0/include/capnp/blob.h:28:
/usr/local/Cellar/capnp/0.7.0/include/kj/common.h:36:4: error: "This code requires C++14. Either your compiler does not support it or it is not enabled."
  #error "This code requires C++14. Either your compiler does not support it or it is not enabled."
   ^
/usr/local/Cellar/capnp/0.7.0/include/kj/common.h:39:6: error: "Pass -std=c++14 on the compiler command line to enable C++14."
    #error "Pass -std=c++14 on the compiler command line to enable C++14."
     ^
In file included from interfaces/capnp/proxy-codegen.cpp:5:
In file included from /usr/local/Cellar/capnp/0.7.0/include/capnp/schema-parser.h:30:
In file included from /usr/local/Cellar/capnp/0.7.0/include/kj/filesystem.h:28:
/usr/local/Cellar/capnp/0.7.0/include/kj/function.h:264:3: error: 'auto' return without trailing return type; deduced return types are a C++14 extension
  auto operator()(Params&&... params) {
  ^
/usr/local/Cellar/capnp/0.7.0/include/kj/function.h:268:3: error: 'auto' return without trailing return type; deduced return types are a C++14 extension
  auto operator()(Params&&... params) const {
  ^
interfaces/capnp/proxy-codegen.cpp:10:10: fatal error: 'interfaces/capnp/proxy.capnp.h' file not found

Sep 28 '18 10:09 Sjors

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 ?

Oct 09 '18 07:10 Sjors

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.

Oct 09 '18 07:10 ryanofsky

re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-428086112

Don't forget to update .gitignore

Updated now.

re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-428086112, https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-416028413

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: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-428086112

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: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-428086112 shutdown/hang issues. I'm planning to look into these next.

Nov 01 '18 21:11 ryanofsky

Pull request

Nov 02 '18 00:11 ismail120572

codegen seems broken for me: (gcc 7.4.0). I get random errors of these kind:

err1

  CXX      wallet/libbitcoin_wallet_tool_a-wallettool.o
  GEN      interfaces/capnp/test/foo.capnp.c++
*** Uncaught exception ***
kj/filesystem.c++:315: failed: expected parts.size() > 0; can't use ".." to break out of starting directory
stack: 7ffbcf3c71a0 7ffbcf3c9829 4098e3 7ffbcf55904d 7ffbcf559082 7ffbcf53208f 7ffbcf534296 7ffbcf5510a4 7ffbcf558893 7ffbcf558901 7ffbcf558c57 7ffbcf532974 7ffbcf532724 7ffbcf533921 7ffbcf5399fc 7ffbcf542aa5 7ffbcf5436d6 7ffbcf555452 7ffbcf55662a 7ffbcf55679f 7ffbcf55a606 7ffbcf55aa53 412466 412770 7ffbcf3e4c28 7ffbcf3e5d1a
make[2]: *** [Makefile:17337: interfaces/capnp/test/foo.capnp.c++] Error 1
make[2]: *** Waiting for unfinished jobs....
Generated test/data/base58_encode_decode.json.h
Generated test/data/key_io_invalid.json.h
Generated test/data/blockfilters.json.h
Generated test/data/key_io_valid.json.h
In file included from interfaces/capnp/config_bitcoin-node.cpp:1:0:
./interfaces/capnp/config.h:4:10: fatal error: interfaces/capnp/node.capnp.h: No such file or directory
 #include <interfaces/capnp/node.capnp.h>
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
make[2]: *** [Makefile:12585: interfaces/capnp/bitcoin_node-config_bitcoin-node.o] Error 1
In file included from interfaces/capnp/config_bitcoin-wallet.cpp:1:0:
./interfaces/capnp/config.h:4:10: fatal error: interfaces/capnp/node.capnp.h: No such file or directory
 #include <interfaces/capnp/node.capnp.h>
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

err 2

  CXX      interfaces/capnp/proxy_codegen-proxy-codegen.o
In file included from /nix/store/69fkv7ql25r8j496bbchnsvbp42izv8q-capnproto-0.7.0/include/capnp/blob.h:28:0,
                 from interfaces/capnp/proxy-codegen.cpp:4:
/nix/store/69fkv7ql25r8j496bbchnsvbp42izv8q-capnproto-0.7.0/include/kj/common.h:36:4: error: #error "This code requires C++14. Either your compiler does not support it or it is not enabled."
   #error "This code requires C++14. Either your compiler does not support it or it is not enabled."
    ^~~~~
/nix/store/69fkv7ql25r8j496bbchnsvbp42izv8q-capnproto-0.7.0/include/kj/common.h:39:6: error: #error "Pass -std=c++14 on the compiler command line to enable C++14."
     #error "Pass -std=c++14 on the compiler command line to enable C++14."
      ^~~~~
interfaces/capnp/proxy-codegen.cpp:10:10: fatal error: interfaces/capnp/proxy.capnp.h: No such file or directory
 #include <interfaces/capnp/proxy.capnp.h>
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

May 26 '19 01:05 jb55

re: https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-495959606

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

May 27 '19 07:05 ryanofsky

bitcoin bitcoin copied to clipboard

Multiprocess bitcoin

Code Coverage

Reviews

Conflicts

bitcoin
bitcoin copied to clipboard