Decouple WalletModel from RPCExecutor

[furszy]

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.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

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.

Conflicts

No conflicts as of last run.

[furszy]

Concept meh, WalletModel is coupled to RPCConsole, why bother with RPCExecutor?

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.

[DrahtBot]

🚧 At least one of the CI tasks failed. _{Debug: https://github.com/bitcoin-core/gui/runs/31084944010}

Hints

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.

[hebasto]

@furszy 3f34c04628f1cb0f59afb5176dae3a0e8d414d7d "gui: bugfix, decouple WalletModel from RPCExecutor"

Maybe remove "bugfix" from the commit message, as there is no reproducible bug in the master branch?

[furszy]

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.

[pablomartin4btc]

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.

[w0xlt]

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.