Add RPC setting

Sjors commented at 12:31 pm on September 2, 2021: member

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.

jarolrod added the label Feature on Sep 2, 2021

jarolrod added the label UX on Sep 2, 2021

jarolrod commented at 4:24 pm on September 2, 2021: member

Strong Concept ACK! Didn’t realize the solution to this was seemingly so simple.

hebasto renamed this:
~~gui: add RPC setting~~
Add RPC setting
on Sep 2, 2021

hebasto commented at 7:48 pm on September 2, 2021: member

Concept ACK.

jonatack commented at 8:11 pm on September 2, 2021: contributor

Concept ACK

promag approved

promag commented at 8:57 pm on September 2, 2021: contributor

Tested ACK 6afe80253fae12265e3744bc35a796670489ab81 on macOS.

Could have a release note or maybe just tag.

kristapsk commented at 8:25 am on September 3, 2021: contributor

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.

hebasto commented at 10:10 am on September 3, 2021: member

Concept ACK.

hebasto added this to the milestone 23.0 on Sep 3, 2021

hebasto added the label Needs release notes on Sep 3, 2021

hebasto commented at 10:34 am on September 3, 2021: member

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();?

@kristapsk

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.

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.

shaavan commented at 1:40 pm on September 3, 2021: contributor

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.

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.
When I select the box, the warning message is “Client restart required to activate changes.” When I deselect it, the message is, “This change would require a client restart.” Is there some reason behind having two different sentences implying the same thing instead of displaying the same message in both cases?

Selected the Checkbox	Disselected the Checkbox

The message “This change would require a client restart” disappears after around 9.5 seconds. While the message “Client restart required to activate changes” doesn’t seem to disappear. I think this inconsistency should be looked into.

I hope my observations would be a help to the OP to improve upon the PR.

in src/qt/forms/optionsdialog.ui:325 in 6afe80253f outdated

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>

jarolrod commented at 3:51 pm on September 3, 2021:

We can add a translator comment by using the extracomment field:

0          <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>

Sjors commented at 12:49 pm on September 4, 2021:

Added the translator comment. Also using some of that text for the tooltip itself.

in src/qt/forms/optionsdialog.ui:328 in 6afe80253f outdated

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>

jarolrod commented at 3:56 pm on September 3, 2021:

Let’s add a mnemonic shortcut as well as a translator comment:

0          <string extracomment="An Options window setting to enable the RPC server.">Enable &amp;RPC server</string>

jarolrod commented at 3:59 pm on September 3, 2021:

I agree with @hebasto comment. This is probably a better fit under the main tab as it is not related to the P2P network. As we collect more settings, this could be a good candidate to move into a new “Advanced” or “Developer” tab.

jarolrod commented at 3:59 pm on September 3, 2021: member

Approach ACK

Tested on macOS 12, Ubuntu 20.04, and Windows 10. Here is what I tested:

macOS
- Compiled natively, toggled settings on and off, connected with specter wallet, ran some commands through bitcoin-cli.
- Built and deployed a dmg. Installed the bundled app and confirmed that feature works by connecting to specter wallet.
Ubuntu 20.04
- Compiled natively, toggled settings on and off, ran commands through bitcoin-cli.
Windows 10
- Cross-compiled, toggled settings on and off, connected with specter wallet, ran commands through bitcoin-cli.
- Built and deployed a setup .exe file to install Bitcoin Core, connected to specter wallet.

Steps forward Here are some actionable steps I believe we should take:

Add translator comments to the new strings
Add a mnemonic shortcut for this new setting
Move under the Main tab

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

Sjors force-pushed on Sep 4, 2021

Sjors commented at 12:52 pm on September 4, 2021: member

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.

jarolrod commented at 3:57 am on September 6, 2021: member

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.

in src/qt/forms/optionsdialog.ui:186 in 2c420467a9 outdated

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>

jarolrod commented at 3:59 am on September 6, 2021:

just jotting down a nit, only apply if you get a good reason to retouch.

0          <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>

hebasto commented at 3:15 pm on September 12, 2021:

nit: extra space:

0          <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>

Sjors commented at 11:39 am on September 7, 2021: member

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.

hebasto commented at 4:03 pm on September 9, 2021: member

@jarolrod

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)

shaavan approved

shaavan commented at 1:34 pm on September 11, 2021: contributor

tACK 2c420467a980487752ae2b0c9aa89819b027ab70 Tested on Ubuntu 20.04 (Using Qt version 5.12.8)

Changes since my last review:

The RPC console option has been moved under the main tab.
The height of the object, 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!

promag commented at 9:41 pm on September 11, 2021: contributor

ACK 2c420467a980487752ae2b0c9aa89819b027ab70, only moved option to main tab since last review.

in src/qt/forms/optionsdialog.ui:189 in 2c420467a9 outdated

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 &amp;RPC server</string>

hebasto commented at 3:15 pm on September 12, 2021:

Tested 2c420467a980487752ae2b0c9aa89819b027ab70 on Linux Mint 20.2 (Qt 5.12.8).

The Alt-R shortcut is ambiguous with this PR:

0$ git grep -n -e '&amp;R' src/qt/forms/optionsdialog.ui
1src/qt/forms/optionsdialog.ui:189:          <string extracomment="An Options window setting to enable the RPC server.">Enable &amp;RPC server</string>
2src/qt/forms/optionsdialog.ui:944:          <string>&amp;Reset Options</string>

To solve it, suggesting the following patch:

 0--- a/src/qt/forms/optionsdialog.ui
 1+++ b/src/qt/forms/optionsdialog.ui
 2@@ -33,7 +33,7 @@
 3           <string>Automatically start %1 after logging in to the system.</string>
 4          </property>
 5          <property name="text">
 6-          <string>&amp;Start %1 on system login</string>
 7+          <string>Start %1 on system &amp;login</string>
 8          </property>
 9         </widget>
10        </item>
11@@ -186,7 +186,7 @@
12           <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>
13          </property>
14          <property name="text">
15-          <string extracomment="An Options window setting to enable the RPC server.">Enable &amp;RPC server</string>
16+          <string extracomment="An Options window setting to enable the RPC server.">Enable RPC &amp;server</string>
17          </property>
18         </widget>
19        </item>

Sjors commented at 2:03 pm on September 15, 2021:

Is there documentation somewhere that explains how global / local these keyboard shortcuts are? I can’t test it on Mac.

hebasto commented at 2:11 pm on September 15, 2021:

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.

hebasto commented at 3:18 pm on September 12, 2021: member

Approach ACK 2c420467a980487752ae2b0c9aa89819b027ab70.

gui: add RPC setting bd5c826a96

Sjors force-pushed on Sep 16, 2021

Sjors commented at 9:17 am on September 16, 2021: member

Addressed feedback.

hebasto approved

hebasto commented at 10:32 am on September 16, 2021: member

ACK bd5c826a9630f41255497e0c9a0f1872b5ab78d5, tested on Linux Mint 20.2 (Qt 5.12.8):

Screenshot from 2021-09-16 13-30-29

shaavan approved

shaavan commented at 6:29 pm on September 16, 2021: contributor

reACK bd5c826a9630f41255497e0c9a0f1872b5ab78d5

Changes since my last review:

Some Changes to mnemonic shortcuts to avoid collisions:
- the mnemonic shortcut of text of bitcoinAtStartup QCheckbox, has been changed from &Start to &login
- mnemonic shortcut of the text of added enableServer QCheckbox has changed from &RPCto &server

promag commented at 8:08 am on September 17, 2021: contributor

Code review ACK bd5c826a9630f41255497e0c9a0f1872b5ab78d5. Just minor fixes to the .ui form since last review.

hebasto merged this on Sep 29, 2021

hebasto closed this on Sep 29, 2021

Sjors deleted the branch on Sep 29, 2021

in src/qt/forms/optionsdialog.ui:186 in bd5c826a96

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>

jonatack commented at 10:58 am on September 29, 2021:

@jarolrod see #435 (review)

jonatack commented at 11:16 am on September 29, 2021: contributor

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.

sidhujag referenced this in commit 33a810ef37 on Sep 29, 2021

hebasto referenced this in commit 06782cf8e7 on Nov 21, 2021

sidhujag referenced this in commit e426c46d81 on Nov 22, 2021

sidhujag referenced this in commit d0ab4147e4 on Nov 23, 2021

bitcoin-core locked this on Sep 29, 2022

hebasto removed the label Needs release notes on May 16, 2023

Add RPC setting #416