RPC access is disabled by default for the GUI.
With the proliferation of third party desktop applications that use the Bitcoin Core RPC (e.g. Specter Desktop, Sparrow and Wasabi), this PR makes them slight easier to configure. It's no longer required to find and edit bitcoin.conf to add server=1 to it.
<img width="447" alt="Schermafbeelding 2021-09-02 om 14 25 58" src="https://user-images.githubusercontent.com/10217/131844201-be3b49a8-ae88-47e6-8992-e95ee6b70f69.png">
Strong Concept ACK! Didn't realize the solution to this was seemingly so simple.
Concept ACK.
Concept ACK
Tested ACK 6afe80253fae12265e3744bc35a796670489ab81 on macOS.
Could have a release note or maybe just tag.
If there is already server=1 present in bitcoin.conf, checkbox will be unchecked by default, I don't think it's the right behaviour.
Concept ACK.
The "Network" tab contains settings that are related to the Bitcoin network. I don't think this is a right place for a private RPC interface setting. Maybe the "Main" tab? Or a new one?
Is the node restart really required? Could we just call StartRPC(); / InterruptRPC(); StopRPC();?
If there is already
server=1present inbitcoin.conf, checkbox will be unchecked by default, I don't think it's the right behaviour.
In such a case you'll see a note: "Options set in this dialog are overridden by the command line or in the configuration file: -server=1".
This behavior is well established and documented in bitcoin-conf.md.
Concept ACK
This PR adds the functionality to set server=1 from GUI, eliminating editing the bitcoin.conf file and setting the server’s ‘value' there. This is done by mapping the state of the newly added enableServer button with the value of the server option.
I have tested this PR on Ubuntu 20.04 (using Qt version 5.12.8). The PR seems to work as expected. But I have observed some behavior that I would like to discuss.
| Selected the Checkbox | Disselected the Checkbox |
|---|---|
 |
 |
I hope my observations would be a help to the OP to improve upon the PR.
<p>
318 | @@ -319,6 +319,16 @@ 319 | </property> 320 | </widget> 321 | </item> 322 | + <item> 323 | + <widget class="QCheckBox" name="enableServer"> 324 | + <property name="toolTip"> 325 | + <string>Accept command line and JSON-RPC commands.</string>
We can add a translator comment by using the extracomment field:
<string extracomment="Tooltip text for Options window setting that enables the RPC server. This allows you to communicate with your node through command line and JSON-RPC commands.">Accept command line and JSON-RPC commands.</string>
Added the translator comment. Also using some of that text for the tooltip itself.
323 | + <widget class="QCheckBox" name="enableServer"> 324 | + <property name="toolTip"> 325 | + <string>Accept command line and JSON-RPC commands.</string> 326 | + </property> 327 | + <property name="text"> 328 | + <string>Enable RPC server</string>
Let's add a mnemonic shortcut as well as a translator comment:
<string extracomment="An Options window setting to enable the RPC server.">Enable &RPC server</string>
Approach ACK
Tested on macOS 12, Ubuntu 20.04, and Windows 10. Here is what I tested:
bitcoin-cli.dmg. Installed the bundled app and confirmed that feature works by connecting to specter wallet.bitcoin-cli.bitcoin-cli..exe file to install Bitcoin Core, connected to specter wallet.Steps forward Here are some actionable steps I believe we should take:
Main tabPrecompiled Binary Users
When considering users who will be grabbing precompiled binaries; there is a subset of users that won't actually have the ability to interact through the bitcoin-cli interface, macOS users who installed bitcoin through the .dmg file we provide, and Windows users through the .exe file. For these user's, and without extra effort on their part, the functionality of these feature is limited.
Because this is a function of what we provide with the .dmg/.exe, I don't see this as a big issue. A user who installed Bitcoin through the .dmg file is probably a user who is not too concerned with interacting with bitcoin through bitcoin-cli.
If such users were in fact in need of this functionality, they can always grab the respective zip or tar.gz files and utilize the bitcoin-cli executable.
This still expands the feature set for both users. I'm not aware of a way to enable -server mode for the .dmg/.exe users previous to this PR. Additionally, for both users, the killer feature is still available, which is the ability of third-party apps to connect to bitcoin core through the RPC interface.
I moved it to the Main tab. @hebasto:
Is the node restart really required? Could we just call
StartRPC();/InterruptRPC(); StopRPC();?
The RPC startup is more convoluted than that it seems, see AppInitServers in init.cpp. Maybe better for a followup to remove the need to restart.
@ShaMan239:
I am being warned to restart the app, to apply changes each time I switch the state of Checkbox. Shouldn’t a correct behavior be warned only when I change the state from the last saved state? Like: being warned when changing state from 1 -> 0. But the warning disappears when I go from 1 -> 0 -> 1.
That would be nice, but it's a problem for all settings. In order to fix that we'd have to track whether any setting changed. Because you might change setting A and B, both of which need a restart, and then restore setting A. Right now we just trigger this warning when one setting changes.
Is there some reason behind having two different sentences implying the same thing instead of displaying the same message in both cases?
No, but it seems unrelated this change. @jarolrod:
When considering users who will be grabbing precompiled binaries; there is a subset of users that won't actually have the ability to interact through the bitcoin-cli interface
Third party applications that use the RPC don't need bitcoin-cli for that.
I'm not aware of a way to enable -server mode for the .dmg/.exe users previous to this PR.
You can, by editing bitcoin.conf, but this PR makes it easier.
Shouldn't the UI element react and be aware of the scenario where the server is already enabled through bitcoin.conf. or do we want to keep this mutually exclusive? As in enabling it through the GUI and enabling it through bitcoin.conf are two separate events. Or enabling it at all, whether through the GUI RPC setting or through bitcoin.conf, is a universal event.
It's surprising behavior for someone to have enabled it through their bitcoin.conf, then their GUI settings implies that server mode is not on because this new RPC setting is not checked.
182 | </layout> 183 | </item> 184 | + <item> 185 | + <widget class="QCheckBox" name="enableServer"> 186 | + <property name="toolTip"> 187 | + <string extracomment="Tooltip text for Options window setting that enables the RPC server.">This allows you or a third party tool to communicate with the node through command line and JSON-RPC commands.</string>
just jotting down a nit, only apply if you get a good reason to retouch.
<string extracomment="Tooltip text for Options window setting that enables the RPC server.">This allows you or a third party tool to communicate with the node through command-line and JSON-RPC commands.</string>
nit: extra space:
<string extracomment="Tooltip text for Options window setting that enables the RPC server.">This allows you or a third party tool to communicate with the node through command line and JSON-RPC commands.</string>
Shouldn't the UI element react and be aware of the scenario where the server is already enabled through bitcoin.conf
It is existing (confusing) behaviour that the GUI settings are overriden by bitcoin.conf / arguments. It's better to discuss that in a separate issue.
It's surprising behavior for someone to have enabled it through their bitcoin.conf, then their GUI settings implies that server mode is not on because this new RPC setting is not checked.
See #416 (comment)
tACK 2c420467a980487752ae2b0c9aa89819b027ab70 Tested on Ubuntu 20.04 (Using Qt version 5.12.8)
Changes since my last review:
horizontalSpacer_Main_Threads, just above the newly added RPC console option, has been changed from 20 to 40. I am assuming this is done to create enough white space between these two objects.Tested on Ubuntu 20.04. The changes work correctly and behaving as expected. Adding screenshots to emphasize the difference between master and PR.
| Master | PR |
|---|---|
 |
 |
That would be nice, but it's a problem for all settings. In order to fix that we'd have to track whether any setting changed. Because you might change setting A and B, both of which need a restart, and then restore setting A. Right now we just trigger this warning when one setting changes.
That makes sense. It will unnecessarily make the code complicated. And the additional benefit doesn’t seem to justify the complication introduced.
In the end, I want to say, thanks for this amazing work, @Sjors!
ACK 2c420467a980487752ae2b0c9aa89819b027ab70, only moved option to main tab since last review.
185 | + <widget class="QCheckBox" name="enableServer"> 186 | + <property name="toolTip"> 187 | + <string extracomment="Tooltip text for Options window setting that enables the RPC server.">This allows you or a third party tool to communicate with the node through command line and JSON-RPC commands.</string> 188 | + </property> 189 | + <property name="text"> 190 | + <string extracomment="An Options window setting to enable the RPC server.">Enable &RPC server</string>
Tested 2c420467a980487752ae2b0c9aa89819b027ab70 on Linux Mint 20.2 (Qt 5.12.8).
The Alt-R shortcut is ambiguous with this PR:
$ git grep -n -e '&R' src/qt/forms/optionsdialog.ui
src/qt/forms/optionsdialog.ui:189: <string extracomment="An Options window setting to enable the RPC server.">Enable &RPC server</string>
src/qt/forms/optionsdialog.ui:944: <string>&Reset Options</string>
To solve it, suggesting the following patch:
--- a/src/qt/forms/optionsdialog.ui
+++ b/src/qt/forms/optionsdialog.ui
@@ -33,7 +33,7 @@
<string>Automatically start %1 after logging in to the system.</string>
</property>
<property name="text">
- <string>&Start %1 on system login</string>
+ <string>Start %1 on system &login</string>
</property>
</widget>
</item>
@@ -186,7 +186,7 @@
<string extracomment="Tooltip text for Options window setting that enables the RPC server.">This allows you or a third party tool to communicate with the node through command line and JSON-RPC commands.</string>
</property>
<property name="text">
- <string extracomment="An Options window setting to enable the RPC server.">Enable &RPC server</string>
+ <string extracomment="An Options window setting to enable the RPC server.">Enable RPC &server</string>
</property>
</widget>
</item>
Is there documentation somewhere that explains how global / local these keyboard shortcuts are? I can't test it on Mac.
Not exactly what you ask for, but https://doc.qt.io/qt-5/qshortcut.html#activatedAmbiguously
I think the scope of shortcuts is limited by a window which keeps the focus, and visible (grand)child widgets within it.
Approach ACK 2c420467a980487752ae2b0c9aa89819b027ab70.
Addressed feedback.
ACK bd5c826a9630f41255497e0c9a0f1872b5ab78d5, tested on Linux Mint 20.2 (Qt 5.12.8):
reACK bd5c826a9630f41255497e0c9a0f1872b5ab78d5
Changes since my last review:
bitcoinAtStartup QCheckbox, has been changed from &Start to &loginenableServer QCheckbox has changed from &RPCto &server
Code review ACK bd5c826a9630f41255497e0c9a0f1872b5ab78d5. Just minor fixes to the .ui form since last review.
182 | </layout> 183 | </item> 184 | + <item> 185 | + <widget class="QCheckBox" name="enableServer"> 186 | + <property name="toolTip"> 187 | + <string extracomment="Tooltip text for Options window setting that enables the RPC server.">This allows you or a third party tool to communicate with the node through command-line and JSON-RPC commands.</string>
Post-merge tested ACK bd5c826a9630f41255497e0c9a0f1872b5ab78d5
A practical feature.
Improved state tracking WRT the "need to restart" messages (or even better, avoiding the need to restart) could be nice follow-ups.