ClientModel and WalletModel were already meant as abstraction layer for accessing the core from the GUI. What is your rationale for adding another layer?

ClientModel and WalletModel were already meant as abstraction layer for accessing the core from the GUI. What is your rationale for adding another layer?

ClientModel and WalletModel might have been intended to be an abstraction layer, but they are not functioning like one. There are libbitcoin functions and global variables accessed all over Qt code right now. With this change, all of these calls (there are around 200 of them) are stripped out of Qt code and moved into a one file: src/ipc/local/interfaces.cpp.

I once did a similar thing,.. but stopped at some point and now I know why.
It's an impressive code change and I kinda like a central point (your interfaces.cpp) where communication between the node, the wallet and the GUI happens.

I also agree with @laanwj that the clientmodel (node) and the walletmodal (wallet) are originally though to be that layer.
Though, there are many violations AFAIK.

What would be the downsides of using the exiting layers (clientmodel / walletmodel) better?

What would be the downsides of using the exiting layers (clientmodel / walletmodel) better?

If you look at the ClientModel class, you can see it is doing a lot more work than the ipc::local::Node class is. Similarly with WalletModel and ipc::local::Wallet. The ipc classes are just simple shims around low-level node and wallet functionality, while Qt objects implement higher level logic specific to our current GUI. I think ClientModel and WalletModel classes are still useful after this change. They will just have 1 job instead of 2. Instead of serving as both abstraction layers and MVC model classes, they will serve only as MVC model classes.

Also, and in more concrete terms, the reason these interfaces live outside the src/qt directory is that with #10102, they need to be accessed not only by bitcoin-qt but also by bitcoind (specifically inside the StartServer function in src/ipc/server.cpp which is called here: ryanofsky@ab0afba#diff-6e30027c2045842fe842430d98d099fb

The general IPC interface makes sense to me. The main problem I see for any type of low latency IPC/RPC is the missing asynchronity.
Take getWalletTxDetails. This IPC call may take 2-3 seconds depending on the communication protocol and database you are using. Ideally the GUI is design to handle it asynchronous (like an RPC call) otherwise this will lead to GUI thread freezes. Not sure if this would be solvable as a generic part in the IPC layer of if the wallet/GUI logic must handle it.

The main problem I see for any type of low latency IPC/RPC is the missing asynchronity.

Not sure if you saw the comments about this in the other pr starting here: #10102 (comment)

These changes are orthogonal to event processing / blocking issues in the UI. If UI blocked before, it will still block after these changes, if UI didn't block before, it won't start blocking now because of these changes. If remote calls are too slow because of socket/serialization overhead, we can process UI events in the background while they are being made. There are many ways to accomplish this, with one possible way described in that comment above. If anything, having calls get funnelled through an IPC framework makes it easier, not harder to add more asynchronicity.

Also would point out that Node and Wallet interfaces in ipc/interfaces.h were mainly designed with goal of changing existing Qt code as little as possible. They aren't in any way set in stone, and I would expect them to evolve over time. Probably some calls will get consolidated, others will get broken up, calls that currently return big chunks of data will be made streaming, etc.

Thinking again and discussing this with @sipa / @laanwj, I think we should use the existing client-/walletmodal as node/wallet abstraction (including a possible IPC abstraction).

What's missing in the first place are better asynchronous messaging between the GUI and the wallet/node.

IMO using a thread with queue for general node/wallet communication (and eventual additional threads for procedures that usually take longer) seems after a low hanging fruit with direct benefits.

Using QT slots/signals for all(most?) communication would be required anyways and would be beneficial even without IPC and therefor should be done first.

What's missing in the first place are better asynchronous messaging between the GUI and the wallet/node.

Again I think this is (and should be) an independent issue, but if you want to flesh out some more concrete suggestions and I would be happy to hear them.

IMO using a thread with queue for general node/wallet communication (and eventual additional threads for procedures that usually take longer) seems after a low hanging fruit with direct benefits.

This is exactly what the change I was suggesting in #10102 (comment) does.

Using QT slots/signals for all(most?) communication would be required anyways and would be beneficial even without IPC and therefor should be done first.

This was my point too. Making the GUI asynchronous would avoid ever hard-freezing the GUI. Modern operating systems assume that an application has crashed if its GUI thread is unresponsive. This is a priority for improving user experience. For example: Currently, if e.g. a transaction is sent while the cs_main lock is held the entire thing hangs for a moment. Ideally it would display a modal dialog with a status, or progress animation instead. There are similar issues at startup.

Sure, this is only partially related to IPC work: When the GUI already would communicate with Qt signals and slots with the core backend (similar to how RPCConsole and RPCThread communicate, for example), it could be mostly oblivious whether this backend exists in-process or communicates over a pipe.

Although it's laudable that you're working on this, it looks to me that what you are doing currently is simply replicating what we do now but replacing direct core calls with IPC calls. The drawback is that it calcifies some things that shouldn't have been designed that way in the first place (e.g. into multiple abstraction layers), making it harder to improve later.

The drawback is that it calcifies some things

Could you be more concrete about this? I don't see how it is true. Direct calls before are still direct calls now. If we want to follow the RPCConsole / RPCExecutor model in other parts of Qt code, I don't see how any of the changes I've made for IPC make this more difficult.

I had a look at discussion in IRC (https://botbot.me/freenode/bitcoin-core-dev/msg/84348426/)

With respect, what I think you guys are missing on the WalletModel/ClientModel topic is that the ipc::local::WalletImpl and ipc::local::NodeImpl classes in ipc/local/interfaces.cpp are only temporarily being created and invoked within the bitcoin-qt process. In the next PR they are created and run in the bitcoind process instead of bitcoin-qt. That's the reason these classes do not reside in the src/qt directory and one reason why they don't really substitute for the WalletModel/ClientModel classes. See my previous comment for details and a code pointer: #10244 (comment).

However, I do see that it is kind of silly to have cases where Qt code calls a WalletModel/ClientModel method that merely forwards to a WalletImpl/NodeImpl method. I can easily clear this up by inlining these WalletModel/ClientModel methods, which would make the classes more lean.

Also, if this PR will be too difficult to review because of its size (https://botbot.me/freenode/bitcoin-core-dev/msg/84348447/), I can easily decompose it into smaller PRs that could be gradually merged. It is already broken up into separate commits, and many of the individual commits could be further broken up (right now they try to group together related changes).

Updated 4678ff4 -> 9960137 (pr/ipc-local.68 -> pr/ipc-local.69) from https://github.com/jnewbery/bitcoin/commits/pr10244_fix_guards_copyright (thanks @jnewbery!). Only change is fixing the bad include guard and adding copyrights.

Congrats. 🎉

This PR broke the gitian build on windows
https://bitcoin.jonasschnelli.ch/builds/559/build_win.log

This PR broke the gitian build on windows

Error is:

qt/guiutil.cpp:235:24: error: ‘Node’ in namespace ‘::’ does not name a type
 bool isDust(interface::Node& node, const QString& address, const CAmount& amount)
                        ^
qt/guiutil.cpp: In function ‘bool GUIUtil::isDust(int&, const QString&, const CAmount&)’:
qt/guiutil.cpp:240:31: error: request for member ‘getDustRelayFee’ in ‘node’, which is of non-class type ‘int’
     return IsDust(txOut, node.getDustRelayFee());

It seems like this is happening because microsoft has some headers that do #define interface struct: https://stackoverflow.com/questions/25234203/what-is-the-interface-keyword-in-msvc

I think I'll open a followup PR to rename interface directory & namespace to interfaces to avoid this problem.