A more comprehensive fix for the issue described in #837.
Since the WalletModel class is unavailable when compiling without wallet support
(-DENABLE_WALLET=0), the RPC executor class should not be coupled to it.
This decoupling ensures GUI compatibility with builds that omit wallet support.
This also drops an extra #ifdef ENABLE_WALLET block which is always good.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | pablomartin4btc, BrandonOdiwuor, w0xlt, hebasto |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
WalletModel is coupled to RPCConsole, why bother with RPCExecutor?
Concept meh,
WalletModelis coupled toRPCConsole, why bother withRPCExecutor?
Thats because the GUI view code and the RPC command parsing and executing functions are entangled right now, but this doesn’t need to remain the case. While the view might require access to the WalletModel for retrieving certain information to display (perhaps more so in the future because we don’t do much with it now), the underlying procedures for running the commands don’t depend on it. They only need the wallet name, if it exists, to append the URI to the command and they could be placed on a different file.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin-core/gui/runs/31084944010
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Since the `WalletModel` class is unavailable when compiling
without wallet support `(-DENABLE_WALLET=0)`, the RPC executor
class should not be coupled to it. This decoupling ensures GUI
compatibility with builds that omit wallet support.
168@@ -169,7 +169,7 @@ class PeerIdViewDelegate : public QStyledItemDelegate
169 * @param[out] pstrFilteredOut Command line, filtered to remove any sensitive data
nit: describe wallet_name as a param[in] (wallet_model wasn’t there either)
0 * [@param](/bitcoin-core-gui/contributor/param/)[out] pstrFilteredOut Command line, filtered to remove any sensitive data
1 * [@param](/bitcoin-core-gui/contributor/param/)[in] wallet_mame ...
tACK 002b792b9a85100d89e47706c29cf1fd355d9727
Tested the rpc console with and without -DENABLE_WALLET. I think the refactor makes sense as the rpc just needs the wallet name for the related commands.
Out of the scope of this PR, I noticed that the order of the wallets in the combo-boxes corresponds to the settings.json file and if the user changes the wallet in the main window, when the rpc console is open, the wallet in the console combo doesn’t correspond with the one in the main window, which is ok if you want to compare data but I think when the console opens it should have the same. Also perhaps we should show the last used wallet (in ./config/Bitcoin-Qt-*.conf?) when qt starts (if still in the loaded wallets in settings.json) instead of showing the first of the list.
Out of the scope of this PR, I noticed that the order of the wallets in the combo-boxes corresponds to the settings.json file and if the user changes the wallet in the main window, when the rpc console is open, the wallet in the console combo doesn’t correspond with the one in the main window, which is ok if you want to compare data but I think when the console opens it should have the same. Also perhaps we should show the last used wallet (in ./config/Bitcoin-Qt-*.conf?) when qt starts (if still in the loaded wallets in settings.json) instead of showing the first of the list.
This surely comes from the fact that the RPC console view is created during initialization. The presentation of the dialog in screen is merely a show() call. So there is no “open for the first time” concept right now.
If the goal is still to move to a new GUI, I wouldn’t start changing the views behavior now. The current behavior doesn’t seem to be “that” bad anyway.
If the goal is still to move to a new GUI, I wouldn’t start changing the views behavior now.
Yeah, neither do I, just mentioning it here since I just passed thru while testing.
tACK 002b792b9a85100d89e47706c29cf1fd355d9727
Was able to build successfully on Ubuntu 24.04 with Qt version 5.15.13 with -DENABLE_WALLET=OFF and -BUILD_GUI=ON flags and was able to successfully try a couple of RPCs on the GUI RPC console
Code Review ACK https://github.com/bitcoin-core/gui/pull/841/commits/002b792b9a85100d89e47706c29cf1fd355d9727
Passing the wallet name instead of the WalletModel object makes more sense in this case and solves the described issue.