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.
laanwj
commented at 7:41 pm on April 20, 2017:
member
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
commented at 7:47 pm on April 20, 2017:
member
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
commented at 7:48 pm on April 20, 2017:
contributor
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 added the label
GUI
on Apr 20, 2017
ryanofsky
commented at 8:10 pm on April 20, 2017:
member
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.
jonasschnelli
commented at 11:09 am on April 21, 2017:
contributor
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
commented at 11:32 am on April 21, 2017:
member
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
commented at 11:50 am on April 21, 2017:
member
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
commented at 11:51 am on April 21, 2017:
contributor
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
commented at 12:00 pm on April 21, 2017:
member
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
commented at 12:10 pm on April 21, 2017:
member
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
commented at 12:59 pm on April 21, 2017:
member
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
commented at 1:32 pm on April 21, 2017:
member
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 force-pushed
on Apr 27, 2017
ryanofsky force-pushed
on Apr 27, 2017
ryanofsky force-pushed
on Apr 28, 2017
ryanofsky force-pushed
on Apr 28, 2017
ryanofsky force-pushed
on May 9, 2017
ryanofsky force-pushed
on May 17, 2017
ryanofsky force-pushed
on May 23, 2017
ryanofsky force-pushed
on May 24, 2017
ryanofsky force-pushed
on May 24, 2017
ryanofsky force-pushed
on May 24, 2017
ryanofsky force-pushed
on May 25, 2017
ryanofsky force-pushed
on May 25, 2017
ryanofsky force-pushed
on May 31, 2017
ryanofsky force-pushed
on May 31, 2017
ryanofsky force-pushed
on Jun 1, 2017
ryanofsky force-pushed
on Jun 1, 2017
ryanofsky
commented at 7:00 pm on June 1, 2017:
member
@laanwj and @jonasschnelli can you let me know if you still see issues with this approach?
On the Qt model class issue, I pulled out a bunch of model methods that were just wrapping IPC calls so it should be clearer what actual work walletmodel.cpp and clientmodel.cpp are doing. I also added a little blurb to the IPC README describing what the distinction between Qt model classes and the IPC interface classes is supposed to be.
On the asynchronous GUI issue, I created #10504 for more discussion, but think that issue is mostly unrelated to the changes in this PR, except as far as some changes here might potentially make it easier to identify blocking calls and make them asynchronous.
laanwj added this to the "Blockers" column in a project
ryanofsky force-pushed
on Jun 2, 2017
ryanofsky force-pushed
on Jun 5, 2017
ryanofsky force-pushed
on Jun 12, 2017
ryanofsky force-pushed
on Jun 15, 2017
laanwj
commented at 1:38 pm on June 15, 2017:
member
Concept ACK on the approach, moving things to interfaces instead of global calls is usually good as it makes it easier to see what the interface to the core is.
jonasschnelli
commented at 1:58 pm on June 15, 2017:
contributor
Concept ACK.
I think we could try to rebase and get this in once 0.15 has been split off
ryanofsky referenced this in commit
e7280bf8de
on Jun 15, 2017
ryanofsky referenced this in commit
3a43bb9487
on Jun 15, 2017
ryanofsky force-pushed
on Jun 15, 2017
ryanofsky force-pushed
on Jun 15, 2017
ryanofsky force-pushed
on Jun 19, 2017
ryanofsky force-pushed
on Jul 5, 2017
laanwj removed this from the "Blockers" column in a project
ryanofsky referenced this in commit
30d2100875
on Jul 11, 2017
ryanofsky force-pushed
on Jul 11, 2017
ryanofsky referenced this in commit
b937494640
on Jul 18, 2017
ryanofsky force-pushed
on Jul 19, 2017
ryanofsky force-pushed
on Jul 20, 2017
ryanofsky force-pushed
on Aug 14, 2017
ryanofsky referenced this in commit
893b4c4692
on Aug 25, 2017
ryanofsky force-pushed
on Aug 25, 2017
ryanofsky referenced this in commit
1ea3597452
on Aug 25, 2017
ryanofsky force-pushed
on Aug 25, 2017
ryanofsky force-pushed
on Sep 21, 2017
ryanofsky force-pushed
on Sep 21, 2017
ryanofsky force-pushed
on Sep 27, 2017
ryanofsky force-pushed
on Sep 28, 2017
ryanofsky force-pushed
on Sep 28, 2017
ryanofsky force-pushed
on Sep 28, 2017
ryanofsky
commented at 1:14 pm on November 1, 2017:
member
<promag> to change the current wallet from widgets to qtquick would take several PR’s (which would sit there for a long time)
<promag> dunno if the IPC PR from ryanofsky is going forward
<promag> both require lots of changes to the wallet ui code
<promag> that’s why I think adding IPC to glue a new wallet ui is probably easier
<promag> we could have both bitcoin-qt and bitcoin-quick and later drop support for the first
<promag> anyway, moving to qtquick is something IMHO we should plan
I don’t think you need any IPC stuff for this. This PR by itself, which is a refactoring, provides interfaces for GUI code to access node&wallet state while being less tied to their internals. The Node and Wallet interfaces here would probably have to be extended in various ways to do things you’d want in a more modern UI, but they do provide a base of functionality that someone could use to implement one.
MarcoFalke referenced this in commit
4ed818060e
on Nov 15, 2017
ryanofsky force-pushed
on Nov 29, 2017
ryanofsky force-pushed
on Nov 29, 2017
ryanofsky renamed this:
[qt] Add abstraction layer for accessing node and wallet functionality from gui
Refactor: separate gui from wallet and node
on Dec 1, 2017
ryanofsky force-pushed
on Dec 1, 2017
ryanofsky
commented at 10:37 pm on December 1, 2017:
member
Removed in a few places in d5c84fdb9d1b6863018a3206e33bb4d57ce2b3fa, 990f4e4503d5f0eaaa3b724d2d9a2396c57a6ff1, and 2da306b742c4487b1fbcee75ece66f1d14a5dfc0.
in
src/interface/wallet.cpp:421
in
9cdbd195baoutdated
The naming of nodes is a little confusing (would expect it to be a vector of interface::Node instances), especially when elements of it are named node in the loop below. Does node_stats make more sense?
Looks like this is already acquired as-needed in CWallet::SetAddressBook, but maybe this acquisition is intentional.
Good catch, removed locks in setAddressBook and delAddressBook in
7fcc6e43025fb2692acdd722dab2ef9598711377 and a25886384f7c2e3f5b2f86ef28fd86d4ccc7e3d7.
sipa
commented at 5:29 pm on March 9, 2018:
member
After reading the discussion again, Concept ACK.
While WalletModel and ClientModel were designed as an abstraction for the “core”, they’re incomplete in that regard (many direct calls bypass the abstraction), and a bit too high level (having significant logic of their own).
I guess we can see this PR as introducing a perfect abstraction, with no logic of its own.
@ryanofsky What do you think the future responsibilities of WalletModel and ClientModel should be? Should they be turned into logic on the core side (abstracted by the new interfaces), or just inlined into the calls sites inside the GUI?
ryanofsky
commented at 6:20 pm on March 9, 2018:
member
@ryanofsky What do you think the future responsibilities of WalletModel and ClientModel should be? Should they be turned into logic on the core signed, abstracted by the new interfaces, or just inlined into the calls sites inside the GUI?
I think if model code is just calling interface functions it should probably be inlined. But if model code is doing something useful to support the gui, like caching, it should be kept. If there is model code that makes too many assumptions about node/wallet internals, or implements functionality that could be useful for RPCs, I think it should be moved into the core and wrapped by new interfaces.
The returned_target output isn’t used here but is needed a little later from SendCoinsDialog::updateSmartFeeLabel in the “Remove direct bitcoin calls from qt/sendcoinsdialog.cpp” commit.
Previous comment looks a little odd in gmail. Github apparently tries to render :man_facepalming: as ['FACE PALM', 'ZERO WIDTH JOINER', 'MALE SIGN', 'VARIATION SELECTOR-16'] in unicode.
in
src/interface/wallet.h:277
in
9ec1240f5eoutdated
This may not be practical to change for legacy reasons, but I find the naming here confusing; it makes me think bool and not vector. Something like my_txout_addresses (and likewise for two others above) seems clearer.
This may not be practical to change for legacy reasons, but I find the naming here confusing; it makes me think bool and not vector. Something like my_txout_addresses (and likewise for two others above) seems clearer.
I think I disagree. A name like my_txout_addresses to me suggests a list of addresses, not a list of isminetype statuses. I think it should be pretty clear in context (e.g. wtx.txout_is_mine[i] or txout_is_mine.emplace_back) that these are vectors and not singular values, but it’s not a problem to to rename the struct members if there’s a different suggestion. Maybe it could be good to change the suffixes from _is_mine to _isminetype to make it clearer values are enums and not bools.
Oh, you’re totally right; I misunderstood the content of these vectors. Thanks for the clarification.
jamesob
commented at 7:40 pm on March 9, 2018:
member
Concept ACK
This represents a good deal of progress towards decomposing the code into its three major subsystems, regardless of how or if we approach actual process separation. Consolidating and making explicit the interfaces these subsystems expose and depend on will make it easier to reason about the best way to move forward. Evaluating whether to use this or that IPC mechanism or how to approach GUI asynchronicity will be framed nicely by the low-risk, low-overhead boundaries that @ryanofsky has defined in this changeset. On top of that, there are numerous readability improvements, e.g. consolidating lock acquisition, extracting functions, and cleaning up naming. I think this is great work.
I’ll be testing this thoroughly in the next few days.
ryanofsky force-pushed
on Mar 13, 2018
ryanofsky
commented at 6:42 pm on March 13, 2018:
member
@jamesob, thanks for the review, really appreciate you looking at this so thoroughly!
In commit “Remove direct bitcoin calls from qt/bitcoin.cpp”
Nit: this comment disappeared?
BitcoinCore::baseInitialize is replaced by Node::baseInitialize and I don’t think it’s helpful to mention gui threads in comments outside of the gui code, so the comment is different there. I could add a comment at the call site in the gui code, but there’s already a pretty good one there:
Since this is inside a try {} catch it means exceptions can bubble up through the interface. Would that be useful to document in the interface header? Or should that be obvious?
In commit “Remove direct bitcoin calls from qt/bitcoin.cpp”
Since this is inside a try {} catch it means exceptions can bubble up through the interface. Would that be useful to document in the interface header? Or should that be obvious?
I definitely think it’s useful to document when functions may throw specific exceptions (or when they are guaranteed not to throw). I’m not sure how useful it is just to document the fact that functions can throw exceptions. I think this is just a basic assumption in c++ when you haven’t disabled exceptions at compile time. Anyway, I’m happy to add a comment if you have a suggestion for one.
in
src/qt/optionsmodel.cpp:106
in
a98a981367outdated
102@@ -108,27 +103,27 @@ void OptionsModel::Init(bool resetSettings)
103 #ifdef ENABLE_WALLET
104 if (!settings.contains("bSpendZeroConfChange"))
105 settings.setValue("bSpendZeroConfChange", true);
106- if (!gArgs.SoftSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
107+ if (!m_node.softSetBoolArg("-spendzeroconfchange", settings.value("bSpendZeroConfChange").toBool()))
Tested that “forgetting” #ifdef ENABLE_WALLET around a handleLoadWallet call indeed makes the compiler fail, although it may have failed due to something else. The error isn’t very clear:
0Undefined symbols for architecture x86_64:
1 "SplashScreen::ConnectWallet(std::__1::unique_ptr<interface::Wallet, std::__1::default_delete<interface::Wallet> >)", referenced from:
2 std::__1::__function::__func<SplashScreen::subscribeToCoreSignals()::$_0, std::__1::allocator<SplashScreen::subscribeToCoreSignals()::$_0>, void (std::__1::unique_ptr<interface::Wallet, std::__1::default_delete<interface::Wallet> >)>::operator()(std::__1::unique_ptr<interface::Wallet, std::__1::default_delete<interface::Wallet> >&&) in libbitcoinqt.a(libbitcoinqt_a-splashscreen.o)
3ld: symbol(s) not found for architecture x86_64
Also checked that QT still works with --disable-wallet.
In commit “Remove direct bitcoin calls from qt/splashscreen.cpp”
The error isn’t very clear
That error isn’t directly related to this code. If you disabled the same #ifdef on master while still including wallet/wallet.h you would see the error. The error is just saying that the code can’t be linked because it is taking the address of a function (SplashScreen::ConnectWallet) that isn’t defined.
If you want to test the runtime error in this method, you could try disabling the same #ifdef and passing a null function pointer in.
Note that I would like to get rid of the checks for the ENABLE_WALLET macro both in this function and its caller. But the refactoring required to do this would make this PR bigger and require changes to both gui code and wallet code instead of just gui code, so I’ve opted towards a compromise for now that avoids any uses ENABLE_WALLET in the Node interface definition, but does fall back to them in a few places in the implementation.
in
src/interface/node.cpp:184
in
bd0f3916aaoutdated
Did you mean to do this ban stuff in a82693c0bdda423c23aa607d9c696b9c9aeff7a2? Or is it here because banning from the RPC console interacts with the ban table UI?
In commit “Remove direct bitcoin calls from qt/rpcconsole.cpp”
Did you mean to do this ban stuff in a82693c? Or is it here because banning from the RPC console interacts with the ban table UI?
The ban() function is only called from rpcconsole.cpp, not bantablemodel.cpp, and since the changes in this PR are grouped by file, it’s in the rpcconsole.cpp commit 9b73244060ae47d7dddee1c9e63c4d1f704685cd rather than bantablemodel.cpp commit a82693c0bdda423c23aa607d9c696b9c9aeff7a2.
in
src/qt/rpcconsole.cpp:680
in
9b73244060outdated
In commit “Remove most direct bitcoin calls from qt/walletmodel.cpp”
Has anyone tested a rebased #11383 or #12610 with this?
I assume one of those PRs will be merged before this one, but this PR will work fine with either of them. This PR just replaces CWallet references in GUI code with interface::Wallet references, and tweaks calls to wallet methods and access to wallet globals like ::vpwallets to go through the interface. This is pretty straightforward to do regardless of the multiwallet implementation.
in
src/interface/wallet.cpp:124
in
16123b1818outdated
In commit “Remove most direct bitcoin calls from qt/walletmodel.cpp”
Why is _6 dropped here?
The signature of NotifyAddressBookChanged is changed above. Instead of receiving both WalletModel and CWallet pointers, it now only receives a WalletModel pointer, which wraps access to the underlying wallet:
Sjors
commented at 8:53 pm on March 14, 2018:
member
tACK84e80629d to the degree that I understand the code.
This change makes code easier to understand for me. The interfaces could use more documentation, especially the various initialization steps, but that can wait. Thanks for splitting into incremental commits; that did make review less daunting.
Since QT doesn’t have amazing test coverage, I left a bunch of comments on stuff I manually tested. Those are FYI or asking for clarification, unless clearly marked otherwise.
I encountered a situation once where I was unable to change tabs (macOS). I had to switch to master, compile and then switch back to your branch and compile again to make it work again. Could be unrelated, but I’ve never seen that before.
meshcollider
commented at 9:40 pm on March 14, 2018:
contributor
Concept ACK, on my to-do list to review
ryanofsky
commented at 4:19 pm on March 16, 2018:
member
Thank you for the review and testing!
jamesob
commented at 8:23 pm on March 23, 2018:
member
Going to hold off on testing this one until it’s rebased.
ryanofsky force-pushed
on Mar 23, 2018
ryanofsky
commented at 9:48 pm on March 23, 2018:
member
Going to hold off on testing this one until it’s rebased.
Any reason to leave this code block in, now that LOCK has been removed? Also, can you just return m_wallet->changeWalletPassphrase(oldPass, newPass) rather than have in intermediary bool?
In fact, there are many headers that no longer need to be included in lots of the qt files. Perhaps worth adding a commit at the end of this PR to remove them?
There’s too much code here for me to closely review all the changes, but I’ve had a look through all the commits and kicked the tires. We’d preferably get this PR merged early in a release cycle to get plenty of soaking.
This is definitely a big step in the right direction in clearly splitting qt from node code.
ryanofsky force-pushed
on Mar 31, 2018
ryanofsky
commented at 10:51 am on March 31, 2018:
member
ryanofsky referenced this in commit
bb6011ba38
on Apr 3, 2018
ryanofsky
commented at 6:43 pm on April 3, 2018:
member
Rebased e73a9f589f6ddeb0e350d887cd1f98fc55aac632 -> 4678ff464f03c09feb0e237343f2af329293343a (pr/ipc-local.67 -> pr/ipc-local.68) due to conflict with #12846.
ryanofsky
commented at 6:52 pm on April 3, 2018:
member
ryanofsky
commented at 7:26 am on April 7, 2018:
member
This PR broke the gitian build on windows
Error is:
0qt/guiutil.cpp:235:24:error: ‘Node’ in namespace‘::’ does not name a type
1bool isDust(interface::Node& node, const QString& address, const CAmount& amount)
2^3qt/guiutil.cpp: In function ‘bool GUIUtil::isDust(int&, const QString&, const CAmount&)’:4qt/guiutil.cpp:240:31:error: request for member ‘getDustRelayFee’ in ‘node’, which is of non-classtype‘int’5return IsDust(txOut, node.getDustRelayFee());
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
