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

Refactor: separate gui from wallet and node #10244

Merged
merged 21 commits into from Apr 5, 2018

Conversation

Projects
None yet
@ryanofsky
Copy link
Contributor

commented Apr 20, 2017

This is a refactoring PR that does not change behavior in any way. This change:

  1. Creates abstract Node and Wallet interfaces in src/interface/
  2. Updates Qt code to call the new interfaces. This largely consists of diffs of the form:
-    InitLogging();
-    InitParameterInteraction();
+    node.initLogging();
+    node.initParameterInteraction();

This change allows followup PR #10102 (makes bitcoin-qt control bitcoind over an IPC socket) to work without any significant updates to Qt code. Additionally:

  • It provides a single place to describe the interface between GUI and daemon code.
  • It can make better GUI testing possible, because Node and Wallet objects have virtual methods that can be overloaded for mocking.
  • It can be used to help make the GUI more responsive (see #10504)

Other notes:

  • I used python scripts hide-globals.py and replace-syms.py to identify all the places where Qt code was accessing libbitcoin global variables and calling functions accessing those global variables.
  • These changes were originally part of #10102. Thanks to @JeremyRubin for the suggestion of splitting them out.

Commits:

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2017

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.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Apr 20, 2017

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?

@jonasschnelli jonasschnelli added the GUI label Apr 20, 2017

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2017

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

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-local branch from 769bf2f to df4684c Apr 20, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2017

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2017

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.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2017

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.

@laanwj

This comment has been minimized.

Copy link
Member

commented Apr 21, 2017

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2017

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.

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 21, 2017

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

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-local branch 4 times, most recently from 3f77678 to 4c91131 Apr 27, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-local branch from 4c91131 to 6a69ec8 May 9, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-local branch 2 times, most recently from ca542df to 78be4d1 May 17, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-local branch 5 times, most recently from f31ecfc to 89cad8e May 24, 2017

@ryanofsky ryanofsky force-pushed the ryanofsky:pr/ipc-local branch from 89cad8e to 324dabb May 31, 2017

laanwj added a commit that referenced this pull request Apr 5, 2018

Merge #10244: Refactor: separate gui from wallet and node
9960137 Add developer notes about blocking GUI code (Russell Yanofsky)
9a61eed Use WalletBalances struct in Qt (Russell Yanofsky)
56f33ca Remove direct bitcoin calls from qt/sendcoinsdialog.cpp (Russell Yanofsky)
e872c93 Remove direct bitcoin access from qt/guiutil.cpp (Russell Yanofsky)
5884558 Remove direct bitcoin calls from qt transaction table files (Russell Yanofsky)
3cab2ce Remove direct bitcoin calls from qt/paymentserver.cpp (Russell Yanofsky)
3ec2ebc Remove direct bitcoin calls from qt/addresstablemodel.cpp (Russell Yanofsky)
827de03 Remove direct bitcoin calls from qt/coincontroldialog.cpp (Russell Yanofsky)
a0704a8 Remove most direct bitcoin calls from qt/walletmodel.cpp (Russell Yanofsky)
90d4640 Remove direct bitcoin calls from qt/optionsdialog.cpp (Russell Yanofsky)
582daf6 Remove direct bitcoin calls from qt/rpcconsole.cpp (Russell Yanofsky)
3034a46 Remove direct bitcoin calls from qt/bantablemodel.cpp (Russell Yanofsky)
e0b66a3 Remove direct bitcoin calls from qt/peertablemodel.cpp (Russell Yanofsky)
d7c2c95 Remove direct bitcoin calls from qt/intro.cpp (Russell Yanofsky)
fe6f27e Remove direct bitcoin calls from qt/clientmodel.cpp (Russell Yanofsky)
5fba3af Remove direct bitcoin calls from qt/splashscreen.cpp (Russell Yanofsky)
c2f672f Remove direct bitcoin calls from qt/utilitydialog.cpp (Russell Yanofsky)
3d619e9 Remove direct bitcoin calls from qt/bitcoingui.cpp (Russell Yanofsky)
c0f2756 Remove direct bitcoin calls from qt/optionsmodel.cpp (Russell Yanofsky)
71e0d90 Remove direct bitcoin calls from qt/bitcoin.cpp (Russell Yanofsky)
ea73b84 Add src/interface/README.md (Russell Yanofsky)

Pull request description:

  This is a refactoring PR that does not change behavior in any way. This change:

  1. Creates abstract [`Node`](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-local/src/interface/node.h) and [`Wallet`](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-local/src/interface/wallet.h) interfaces in [`src/interface/`](https://github.com/ryanofsky/bitcoin/tree/pr/ipc-local/src/interface)
  1. Updates Qt code to call the new interfaces. This largely consists of diffs of the form:

  ```diff
  -    InitLogging();
  -    InitParameterInteraction();
  +    node.initLogging();
  +    node.initParameterInteraction();
  ```

  This change allows followup PR #10102 (makes `bitcoin-qt` control `bitcoind` over an IPC socket) to work without any significant updates to Qt code. Additionally:

  * It provides a single place to describe the interface between GUI and daemon code.
  * It can make better GUI testing possible, because Node and Wallet objects have virtual methods that can be overloaded for mocking.
  * It can be used to help make the GUI more responsive (see #10504)

  Other notes:

  * I used python scripts [hide-globals.py](https://github.com/ryanofsky/home/blob/master/src/2017/hide-globals/hide-globals.py) and [replace-syms.py](https://github.com/ryanofsky/home/blob/master/src/2017/hide-globals/replace-syms.py) to identify all the places where Qt code was accessing libbitcoin global variables and calling functions accessing those global variables.
  * These changes were originally part of #10102. Thanks to @JeremyRubin for the suggestion of splitting them out.

  Commits:

  - [`ea73b84d2d` Add src/interface/README.md](ea73b84)
  - [`71e0d90876` Remove direct bitcoin calls from qt/bitcoin.cpp](71e0d90)
  - [`c0f2756be5` Remove direct bitcoin calls from qt/optionsmodel.cpp](c0f2756)
  - [`3d619e9d36` Remove direct bitcoin calls from qt/bitcoingui.cpp](3d619e9)
  - [`c2f672fb19` Remove direct bitcoin calls from qt/utilitydialog.cpp](c2f672f)
  - [`5fba3af21e` Remove direct bitcoin calls from qt/splashscreen.cpp](5fba3af)
  - [`fe6f27e6ea` Remove direct bitcoin calls from qt/clientmodel.cpp](fe6f27e)
  - [`d7c2c95948` Remove direct bitcoin calls from qt/intro.cpp](d7c2c95)
  - [`e0b66a3b7c` Remove direct bitcoin calls from qt/peertablemodel.cpp](e0b66a3)
  - [`3034a462a5` Remove direct bitcoin calls from qt/bantablemodel.cpp](3034a46)
  - [`582daf6d22` Remove direct bitcoin calls from qt/rpcconsole.cpp](582daf6)
  - [`90d4640b7e` Remove direct bitcoin calls from qt/optionsdialog.cpp](90d4640)
  - [`a0704a8996` Remove most direct bitcoin calls from qt/walletmodel.cpp](a0704a8)
  - [`827de038ab` Remove direct bitcoin calls from qt/coincontroldialog.cpp](827de03)
  - [`3ec2ebcd9b` Remove direct bitcoin calls from qt/addresstablemodel.cpp](3ec2ebc)
  - [`3cab2ce5f9` Remove direct bitcoin calls from qt/paymentserver.cpp](3cab2ce)
  - [`58845587e1` Remove direct bitcoin calls from qt transaction table files](5884558)
  - [`e872c93ee8` Remove direct bitcoin access from qt/guiutil.cpp](e872c93)
  - [`56f33ca349` Remove direct bitcoin calls from qt/sendcoinsdialog.cpp](56f33ca)
  - [`9a61eed1fc` Use WalletBalances struct in Qt](9a61eed)
  - [`9960137697` Add developer notes about blocking GUI code](9960137)

Tree-SHA512: 7b9eff2f37d4ea21972d7cc6a3dbe144248595d6c330524396d867f3cd2841d666cdc040fd3605af559dab51b075812402f61d628d16cf13719335c1d8bf8ed3
@tmornini

This comment has been minimized.

Copy link

commented Apr 6, 2018

Congrats. 🎉

@ken2812221

This comment has been minimized.

Copy link
Member

commented Apr 7, 2018

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

@ryanofsky

This comment has been minimized.

Copy link
Contributor Author

commented Apr 7, 2018

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.

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 7, 2018

Avoid `interface` keyword to fix windows gitian build
Rename `interface` to `interfaces`

Build failure reported by Chun Kuan Lee <ken2812221@gmail.com>
bitcoin#10244 (comment)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 7, 2018

scripted-diff: Avoid `interface` keyword to fix windows gitian build
Rename `interface` to `interfaces`

Build failure reported by Chun Kuan Lee <ken2812221@gmail.com>
bitcoin#10244 (comment)

-BEGIN VERIFY SCRIPT-

git mv src/interface src/interfaces
ren() { git grep -l "$1" | xargs sed -i "s,$1,$2,g"; }
ren interface/            interfaces/
ren interface::           interfaces::
ren BITCOIN_INTERFACE_    BITCOIN_INTERFACES_
ren "namespace interface" "namespace interfaces"

-END VERIFY SCRIPT-

MarcoFalke added a commit that referenced this pull request Apr 7, 2018

Merge #12906: Avoid `interface` keyword to fix windows gitian build
17780d6 scripted-diff: Avoid `interface` keyword to fix windows gitian build (Russell Yanofsky)

Pull request description:

  Rename `interface` to `interfaces`

  Build failure reported by ken2812221 in #10244 (comment)

Tree-SHA512: e02c97c728540f344202c13b036f9f63af23bd25e25ed7a5cfe9e2c2f201a12ff232cc94a93fbe37ef6fb6bf9e036fe62210ba798ecd30de191d09338754a8d0

stamhe added a commit to stamhe/bitcoin that referenced this pull request Jun 27, 2018

Use WalletBalances struct in Qt
Suggested by John Newbery <john@johnnewbery.com>
bitcoin#10244 (comment)

stamhe added a commit to stamhe/bitcoin that referenced this pull request Jun 27, 2018

scripted-diff: Avoid `interface` keyword to fix windows gitian build
Rename `interface` to `interfaces`

Build failure reported by Chun Kuan Lee <ken2812221@gmail.com>
bitcoin#10244 (comment)

-BEGIN VERIFY SCRIPT-

git mv src/interface src/interfaces
ren() { git grep -l "$1" | xargs sed -i "s,$1,$2,g"; }
ren interface/            interfaces/
ren interface::           interfaces::
ren BITCOIN_INTERFACE_    BITCOIN_INTERFACES_
ren "namespace interface" "namespace interfaces"

-END VERIFY SCRIPT-

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 3, 2018

Use WalletBalances struct in Qt
Suggested by John Newbery <john@johnnewbery.com>
bitcoin#10244 (comment)

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 15, 2018

scripted-diff: Avoid `interface` keyword to fix windows gitian build
Rename `interface` to `interfaces`

Build failure reported by Chun Kuan Lee <ken2812221@gmail.com>
bitcoin#10244 (comment)

-BEGIN VERIFY SCRIPT-

git mv src/interface src/interfaces
ren() { git grep -l "$1" | xargs sed -i "s,$1,$2,g"; }
ren interface/            interfaces/
ren interface::           interfaces::
ren BITCOIN_INTERFACE_    BITCOIN_INTERFACES_
ren "namespace interface" "namespace interfaces"

-END VERIFY SCRIPT-

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

jasonbcox added a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 3, 2019

Use WalletBalances struct in Qt
Summary:
Suggested by John Newbery <john@johnnewbery.com>
bitcoin/bitcoin#10244 (comment)

Partial backport of Core PR 10244
bitcoin/bitcoin@9a61eed
Progress towards T417
Depends on D2889

Test Plan:
make check
ninja check
bitcoin-qt # sanity checks

Reviewers: deadalnix, Fabien, #bitcoin_abc

Reviewed By: Fabien, #bitcoin_abc

Differential Revision: https://reviews.bitcoinabc.org/D2899
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.