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

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #692 (Debug Console implementation of generate method by hernanmarino)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

[promag]

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

[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 3f34c04 "gui: bugfix, decouple WalletModel from RPCExecutor"

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

[pablomartin4btc]

tACK 002b792

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.

[pablomartin4btc]

nit: describe wallet_name as a param[in] (wallet_model wasn't there either)

Suggested change

	* @param[out] pstrFilteredOut Command line, filtered to remove any sensitive data
	* @param[out] pstrFilteredOut Command line, filtered to remove any sensitive data
	* @param[in] wallet_mame ...

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

[jarolrod]

strong concept ack

[BrandonOdiwuor]

tACK 002b792

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