Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[experimental] Multiprocess bitcoin #10102

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@ryanofsky
Copy link
Contributor

ryanofsky commented Mar 27, 2017

Current status: This PR is a work in progress. Multiprocess code is functional but needs to be cleaned up and tested more. Windows support needs to be added, since the current implementation only works on linux and mac.


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/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. A followup PR will 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/interfaces/capnp/ directory, and not in any public headers or other parts of bitcoin code.


Slides from a presentation describing this change are available on google drive. Demo code used in the presentation was from an older version this PR (tag ipc.21, commits).

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 28, 2017

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?
@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Mar 28, 2017

Would the performance be impractical if we would try to use the existing RPC API?

Reason this is currently using capnp is not performance but convenience. Capnp provides a high level API that supports bidirectional, synchronous, and asynchronous calls out of the box and allows me to easily explore implementation choices in bitcoin-qt without having to worry about low level protocol details, write a lot of parameter packing/unpacking boilerplate, and implement things like long polling.

Capnp could definitely be replaced by JSON-RPC, though, and I've gone out of my way to support this by not calling capnp functions or using capnp types or headers anywhere except the ipc/server.cpp and ipc/client.cpp files. No code outside of these two files has to change in order to move to a different protocol.

Could the capnp approach (or lets say IPC approach) be designed as a (or the) new API ("JSON RPC v2" and replacement for ZMQ)?

It could, but I'm going out of my way right now specifically NOT to add yet another bitcoind public API that could add to the JSON-RPC/REST/ZMQ/-blocknotify/-walletnotify confusion. The IPC here doesn't happen over a TCP port or even a unix socket path but over an anonymous socketpair using an inherited file descriptor. (I haven't done a windows implementation yet but similar things are possible there).

I'm trying to make the change completely internal for now and transparent to users. Bitcoin-qt should still be invoked the same way and behave the same way as before, starting its own node and wallet. It just will happen to do this internally now by forking a bitcoind executable rather than calling in-process functions.

This change will not add any new command line or GUI options allowing bitcoin-qt to connect to bitcoinds other than the one it spawns internally. Adding these features and supporting new public APIs might be things we want to do in the future, but they would involve downsides and complications that I'm trying to avoid here.

Does capnp provide a basic form of authentication? Would that even be required?

It's not required here because this change doesn't expose any new socket or endpoint, but it could be supported. Capnp's security model is based on capabilities, so to add authentication, you would just define a factory function that takes credentials as parameters and returns a reference to an object exposing the appropriate functionality.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Mar 28, 2017

I'm really uncomfortable with using capn proto, but fine enough for some example testing stuff!

I'm a fan of this general approach (ignoring the use of capn proto) and I think we should have done something like it a long time ago.

@dcousens

This comment has been minimized.

Copy link
Contributor

dcousens commented Mar 28, 2017

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 29, 2017

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.

}

bool WalletModel::getDefaultWalletRbf() const
{
return fWalletRbf;

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 29, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

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.

}

interface Wallet {
getBalance @0 () -> (value :Int64);

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

Pretty nice. So the IPC endpoint exposes multiple "objects". It can also expose multiple instances of one class? (e.g. for multiwallet?).

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 29, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

Cool. I like this auto-generated IPC stuff. Saves writing a lot of boilerplate.

close(fd);
}
}
if (execlp(BITCOIN_DAEMON_NAME, BITCOIN_DAEMON_NAME, "-ipcfd", std::to_string(fds[0]).c_str(), nullptr) != 0) {

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

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?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 29, 2017

Author Contributor

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.

+    //! Set command line arguments.
+    void parseParameters(int argc, const char* const argv[]) const;
+
     //! Get help message string.
     std::string helpMessage(HelpMessageMode mode) const;
 
+    //! Start node.
+    bool appInit() const;
+
+    //! Stop node.
+    void appShutdown() const;
+
+    //! Return whether shutdown was requested.
+    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.

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 31, 2017

Member

Right, ideally bitcoin-qt would need only a very small subset of the bitcoind code.

@@ -298,9 +303,9 @@ void BitcoinCore::shutdown()
try
{
qDebug() << __func__ << ": Running Shutdown in thread";
Interrupt(threadGroup);
FIXME_IMPLEMENT_IPC(Interrupt(threadGroup));
threadGroup.join_all();

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 29, 2017

Author Contributor

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.

@@ -676,7 +682,7 @@ int main(int argc, char *argv[])
app.createOptionsModel(IsArgSet("-resetguisettings"));

// Subscribe to global signals from core
uiInterface.InitMessage.connect(InitMessage);
FIXME_IMPLEMENT_IPC_VALUE(uiInterface).InitMessage.connect(InitMessage);

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

How would you handle uiInterface in this model, e.g. signals from the server to the client?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 29, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

So the server then effectively calls an object on the client when the notification happens? No polling/waiting involved? That's great.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 29, 2017

Author Contributor

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.

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 31, 2017

Member

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.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 6, 2017

Author Contributor

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.

return false;
return true;
}

bool WalletModel::abandonTransaction(uint256 hash) const
{
LOCK2(cs_main, wallet->cs_wallet);
return wallet->AbandonTransaction(hash);
LOCK2(FIXME_IMPLEMENT_IPC_VALUE(cs_main), wallet->cs_wallet);

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

This locking (either of cs_main or wallet->cs_wallet) makes no sense when a remote core is used.

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 29, 2017

Author Contributor

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.


bool StartServer(int argc, char* argv[], int& exitStatus)
{
if (argc != 3 || strcmp(argv[1], "-ipcfd") != 0) {

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 29, 2017

Member

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 29, 2017

Author Contributor

It's a bit of a shame that argument parsing doesn't work here yet so this needs to use manual parsing using C functions :/

I think I could change this to at least use GetArg like you are doing in your cloudabi code.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc branch from bf5f8ed to 0ca73bc Apr 6, 2017

@ryanofsky
Copy link
Contributor Author

ryanofsky left a comment

Updated and rebased bf5f8ed -> 0ca73bc (pr/ipc.0 -> pr/ipc.1)

There's a lot of new changes here. More functions and callbacks have been wrapped, and there's now support for asynchronous calls that don't block event threads in the client and server.

At this point it would be very helpful to have code review for the main commit (0ca73bc "Add barebones IPC framework to bitcoin-qt and bitcoind"), because all the real infrastructure is now in place, and the main thing left to do is wrap up more functions and callbacks in IPC calls so the GUI can be functional.

@@ -676,7 +682,7 @@ int main(int argc, char *argv[])
app.createOptionsModel(IsArgSet("-resetguisettings"));

// Subscribe to global signals from core
uiInterface.InitMessage.connect(InitMessage);
FIXME_IMPLEMENT_IPC_VALUE(uiInterface).InitMessage.connect(InitMessage);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 6, 2017

Author Contributor

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.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc branch from 0ca73bc to 1ea8987 Apr 7, 2017

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Apr 7, 2017

Updated and rebased 0ca73bc -> 5e28c2f (pr/ipc.1 -> pr/ipc.3) to avoid a conflict. Main addition is an expanded src/ipc/README.md file.

Again it would be very helpful to have some code review for the main commit (5e28c2f "Add barebones IPC framework to bitcoin-qt and bitcoind"). Giving feedback on the README file would be an easy place to start.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc branch 2 times, most recently from 5e28c2f to dda3756 Apr 10, 2017

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Apr 10, 2017

Updated 5e28c2f -> dda3756 (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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Apr 14, 2017

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 14, 2017

@ryanofsky Yes, integrating the IPC event loop and Qt event loop would help responsiveness.
Though I remember there were some issues in some cases with recursively calling into the Qt event loop (e.g. things need to be reentrant, deleteLater stuff runs earlier than expected, to keep in mind).

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Apr 17, 2017

@ryanofsky I'm not familiar with Qt or capnproto, but I don't understand what the move to a different process has to do with making things less blocking. Any changes in architecture that would result in less blocks should equally be possible within the same process.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Apr 17, 2017

This change will not add any new command line or GUI options allowing bitcoin-qt to connect to bitcoinds other than the one it spawns internally. Adding these features and supporting new public APIs might be things we want to do in the future, but they would involve downsides and complications that I'm trying to avoid here.

I don't understand the goal here. On itself, there seems little benefit in separating the GUI and the rest into separate processes if those two processes still depend on each other (this is different from separating the wallet from the node, for example, as there as security considerations there... but for that use case the easiest approach seems to just have a lightweight mode and running two instances).

I think it would be awesome if bitcoin-qt could be started and stopped independently to control a bitcoind process in the background, but if that's not the intent, what is the purpose?

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

ryanofsky commented Apr 17, 2017

Any changes in architecture that would result in less blocks should equally be possible within the same process.

Let's say there are 50 places where bitcoin-qt calls a libbitcoin function. That means there are 50 places to update if you want bitcoin-qt handle to events while the function calls are executing. WIth the IPC framework, there is only one place you have to update instead of 50 places (if you want to do this).

On itself, there seems little benefit in separating the GUI and the rest into separate processes if those two processes still depend on each other.

Ok, so you think the benefits are small, and I think they are more significant.

I think it would be awesome if bitcoin-qt could be started and stopped independently to control a bitcoind process in the background,

This is trivial once bitcoin-qt is controlling bitcoind across a socket. I'm just implementing the socket part first, without introducing new UI features for now.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Apr 17, 2017

I think it would be awesome if bitcoin-qt could be started and stopped independently to control a bitcoind process in the background,

This is trivial once bitcoin-qt is controlling bitcoind across a socket. I'm just implementing the socket part first, without introducing new UI features for now.

Ok, that's what I was missing. It wasn't clear to me that this was a just first step towards a more useful separation.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 20, 2017

[qt] Don't call method on null WalletModel object
This doesn't crash currently because the method doesn't access any object
members, but this behavior is fragile and incompatible with bitcoin#10102.

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc branch from dda3756 to ab0afba Apr 20, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc branch from f2233bc to da8719d Dec 6, 2018

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc branch from da8719d to c5bc654 Jan 23, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jan 23, 2019

kj::mvCapture(server_context.call_context,
[&server, req, fn, args...](typename ServerContext::CallContext call_context,
kj::Own<kj::PromiseFulfiller<typename ServerContext::CallContext>> fulfiller) {
const auto& params = call_context.getParams();

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 25, 2019

Member

Shadows variable params in outer scope?

[&server, req, fn, args...](typename ServerContext::CallContext call_context,
kj::Own<kj::PromiseFulfiller<typename ServerContext::CallContext>> fulfiller) {
const auto& params = call_context.getParams();
Context::Reader context_arg = Accessor::get(params);

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 25, 2019

Member

Same here?

template <typename Mutex>
struct UnlockGuard
{
UnlockGuard(Mutex& mutex) : m_mutex(mutex) { m_mutex.unlock(); }

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 25, 2019

Member

Should be explicit? Applies to all single parameter constructors in this PR :-)

{
using S = Split<N, ParamList>;
handleChain(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2), typename S::First());
next_fn.handleChain(std::forward<Arg1>(arg1), std::forward<Arg2>(arg2), typename S::Second(),

This comment has been minimized.

Copy link
@practicalswift

practicalswift Jan 25, 2019

Member

Are arg1 and arg2 guaranteed to be valid here? Could have been moved from on the call on the line before?

meshcollider added a commit that referenced this pull request Jan 30, 2019

Merge #14711: Remove uses of chainActive and mapBlockIndex in wallet …
…code

44de156 Remove remaining chainActive references from CWallet (Russell Yanofsky)
db21f02 Convert CWallet::ScanForWalletTransactions and SyncTransaction to the new Chain apis (Russell Yanofsky)
2ffb079 Add findFork and findBlock to the Chain interface (Russell Yanofsky)
d93c4c1 Add time methods to the Chain interface (Russell Yanofsky)
700c42b Add height, depth, and hash methods to the Chain interface (Russell Yanofsky)

Pull request description:

  This change removes uses of `chainActive` and `mapBlockIndex` globals in wallet code. It is a refactoring change which does not affect external behavior.

  This is the next step in the larger #10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in #10102.

Tree-SHA512: 4dcec8a31c458f54e2ea6ecf01e430469b0994c5b41a21a2d150efa67cd209f4c93ae210a101e064b3a87c52c6edfc70b070e979992be0e3a00fd425de6230a8

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc branch from c5bc654 to cc23f74 Feb 13, 2019

@DrahtBot DrahtBot added Needs rebase and removed Needs rebase labels Feb 13, 2019

MarcoFalke added a commit that referenced this pull request Mar 4, 2019

Merge #15288: Remove wallet -> node global function calls
f7efd87 Change brace formatting (Russell Yanofsky)
a1df1b4 Remove use of IsInitialBlockDownload in wallet code (Russell Yanofsky)
1106a6f Remove use of uiInterface.LoadWallet in wallet code (Russell Yanofsky)
318f41f circular-dependencies: Avoid treating some .h/.cpp files as a unit (Russell Yanofsky)
d02b34c Remove use of AcceptToMemoryPool in wallet code (Russell Yanofsky)
e2c8ba9 Remove uses of InitMessage/Warning/Error in wallet code (Russell Yanofsky)
c5e59a9 Remove uses of GetAdjustedTime in wallet code (Russell Yanofsky)
6d6bcc7 Remove use of g_connman / PushInventory in wallet code (Russell Yanofsky)
00dfb2a Remove uses of g_connman in wallet code (Russell Yanofsky)
cc3836e Remove uses of fPruneMode in wallet code (Russell Yanofsky)
cc02c79 Remove uses of fee globals in wallet code (Russell Yanofsky)
1fb0a4a Remove use of CalculateMemPoolAncestors in wallet code (Russell Yanofsky)
cd32160 Remove use of GetTransactionAncestry in wallet code (Russell Yanofsky)
291276f Remove use of GetCountWithDescendants in wallet code (Russell Yanofsky)
bdc6628 Remove use of IsRBFOptIn in wallet code (Russell Yanofsky)
80f52a2 Remove uses of CheckFinalTx in wallet code (Russell Yanofsky)

Pull request description:

  This change removes wallet calls to node functions that access global chain and mempool state.

  This is the next step in the larger #10973 refactoring change, which removes all other accesses to node global variables from wallet code. Doing this is useful to provide a better defined interface between the wallet and node, and necessary to allow wallet and node code to run in separate processes in #10102.

Tree-SHA512: 40dbaf1f59fb22b32e70b054b30ba5638d638aa3240fa30e0f721d53c721cd6138a7ab4d423a24d7d2fda0b956e68d44c733abc2c9259c3d6c9fd6d4be89aa23

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc branch from cc23f74 to c5dadcc Mar 4, 2019

@DrahtBot DrahtBot removed the Needs rebase label Mar 4, 2019

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc branch from c5dadcc to 191e240 Mar 5, 2019

meshcollider added a commit that referenced this pull request Mar 21, 2019

Merge #10973: Refactor: separate wallet from node
d358466 Remove remaining wallet accesses to node globals (Russell Yanofsky)
b1b2b23 Remove use of CCoinsViewMemPool::GetCoin in wallet code (Russell Yanofsky)
4e4d9e9 Remove use of CRPCTable::appendCommand in wallet code (Russell Yanofsky)
91868e6 Remove use CValidationInterface in wallet code (Russell Yanofsky)

Pull request description:

  This PR is the last in a chain of PRs (#14437, #14711, and #15288) that make the wallet code access node state through an abstract [`Chain`](https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep/src/interfaces/chain.h) class in [`src/interfaces/`](https://github.com/ryanofsky/bitcoin/tree/pr/wipc-sep/src/interfaces) instead of using global variables like `cs_main`, `chainActive`, and `g_connman`. After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the `hide-globals` script in #10244).

  This PR and the previous PRs have been refactoring changes that do not affect behavior. Previous PRs have consisted of lots of mechanical changes like:

  ```diff
  -    wtx.nTimeReceived = GetAdjustedTime();
  +    wtx.nTimeReceived = m_chain->getAdjustedTime();
  ```

  This PR is smaller, but less mechanical. It replaces last few bits of wallet code that access node state directly (through `CValidationInterface`, `CRPCTable`, and `CCoinsViewMemPool` interfaces) with code that uses the `Chain` interface.

  These changes allow followup PR #10102 (multiprocess gui & wallet PR) to work without any significant updates to wallet code. Additionally they:

  * Provide a single place to describe the interface between wallet and node code.
  * Can make better wallet testing possible, because the `Chain` object consists of virtual methods that can be overloaded for mocking. (This could be used to test edge cases in the rescan code, for example).

Tree-SHA512: e6291d8a3c50bdff18a9c8ad11e729beb30b5b7040d7aaf31ba678800b4a97b2dd2be76340b1e5c01fe2827d67d37ed1bb4c8380cf8ed653aadfea003e9b22e7

ryanofsky added some commits Mar 27, 2017

Multiprocess bitcoin
This adds an --enable-multiprocess configure option which will build new
bitcoin-node, bitcoin-wallet, and bitcoin-gui executables with node, wallet,
and gui functionality isolated into different processes. See
doc/multiprocess.md for details.

Work in progress, many improvements and cleanups are needed (see FIXMEs)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.