Add RPC setting #416

Sjors · 2021-09-02T12:31:53Z

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 · 2021-09-02T16:24:12Z

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

hebasto · 2021-09-02T19:48:18Z

Concept ACK.

jonatack · 2021-09-02T20:11:41Z

Concept ACK

promag

Tested ACK 6afe802 on macOS.

Could have a release note or maybe just tag.

kristapsk · 2021-09-03T08:25:39Z

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 · 2021-09-03T10:10:02Z

Concept ACK.

hebasto · 2021-09-03T10:34:02Z

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

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.

jarolrod

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.

jarolrod · 2021-09-03T15:51:29Z

src/qt/forms/optionsdialog.ui

+       <item>
+        <widget class="QCheckBox" name="enableServer">
+         <property name="toolTip">
+          <string>Accept command line and JSON-RPC commands.</string>


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

Suggested change

<string>Accept command line and JSON-RPC commands.</string>

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

src/qt/forms/optionsdialog.ui

Sjors · 2021-09-04T12:52:55Z

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 · 2021-09-06T03:57:38Z

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.

src/qt/forms/optionsdialog.ui

Sjors · 2021-09-07T11:39:40Z

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 · 2021-09-09T16:03:44Z

@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

tACK 2c42046
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

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

hebasto

Approach ACK 2c42046.

hebasto · 2021-09-12T15:15:44Z

src/qt/forms/optionsdialog.ui

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


Tested 2c42046 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.

src/qt/forms/optionsdialog.ui

Sjors · 2021-09-16T09:17:44Z

Addressed feedback.

hebasto

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

shaavan

reACK bd5c826

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

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

jonatack

Post-merge tested ACK bd5c826

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.

jonatack · 2021-09-29T10:58:00Z

src/qt/forms/optionsdialog.ui

+       <item>
+        <widget class="QCheckBox" name="enableServer">
+         <property name="toolTip">
+          <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 see #435 (comment)

bd5c826 gui: add RPC setting (Sjors Provoost) Pull request description: 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"> ACKs for top commit: hebasto: ACK bd5c826, tested on Linux Mint 20.2 (Qt 5.12.8): shaavan: reACK bd5c826 promag: Code review ACK bd5c826. Just minor fixes to the .ui form since last review. Tree-SHA512: ab377e2358826096b499013bc3a864b7b63dff9859e96041e93ff0897d2319a35e8b3adcfb8df5f83274466c83d040d4ea18c546699421425c835e6f42562ae0

25a5814 GUI/Options: Restore "S" accelerator for "Start on system login" option (Luke Dashjr) Pull request description: #416 changed the option assigned to accelerator key "S", but there's no rationale given. Best to leave it alone, and give the new option a new accelerator key. Since "R" is already taken for Reset, this shifts the new RPC server option to use "P" instead ACKs for top commit: jarolrod: ACK 25a5814 hebasto: ACK 25a5814, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: 2212aa32572cbcbcce78304ffcb1dcfc65f9d7e2ffd6c6a4b65e4a3ca2a8a7cc7505c28314ad46e0bc13b4e3bb3fc61e7e196356d26354f3689fad71fb688b27

…tem login" option 25a5814 GUI/Options: Restore "S" accelerator for "Start on system login" option (Luke Dashjr) Pull request description: bitcoin-core/gui#416 changed the option assigned to accelerator key "S", but there's no rationale given. Best to leave it alone, and give the new option a new accelerator key. Since "R" is already taken for Reset, this shifts the new RPC server option to use "P" instead ACKs for top commit: jarolrod: ACK 25a5814 hebasto: ACK 25a5814, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: 2212aa32572cbcbcce78304ffcb1dcfc65f9d7e2ffd6c6a4b65e4a3ca2a8a7cc7505c28314ad46e0bc13b4e3bb3fc61e7e196356d26354f3689fad71fb688b27

jarolrod added Feature UX All about "how to get things done" labels Sep 2, 2021

hebasto changed the title ~~gui: add RPC setting~~ Add RPC setting Sep 2, 2021

promag approved these changes Sep 2, 2021

View reviewed changes

hebasto added this to the 23.0 milestone Sep 3, 2021

hebasto added the Needs release notes label Sep 3, 2021

shaavan reviewed Sep 3, 2021

View reviewed changes

jarolrod reviewed Sep 3, 2021

View reviewed changes

Sjors force-pushed the 2021/09/rpc_setting branch from 6afe802 to 2c42046 Compare September 4, 2021 12:52

jarolrod reviewed Sep 6, 2021

View reviewed changes

src/qt/forms/optionsdialog.ui Outdated Show resolved Hide resolved

shaavan approved these changes Sep 11, 2021

View reviewed changes

promag reviewed Sep 11, 2021

View reviewed changes

hebasto reviewed Sep 12, 2021

View reviewed changes

gui: add RPC setting

bd5c826

Sjors force-pushed the 2021/09/rpc_setting branch from 2c42046 to bd5c826 Compare September 16, 2021 09:17

hebasto approved these changes Sep 16, 2021

View reviewed changes

shaavan approved these changes Sep 16, 2021

View reviewed changes

promag reviewed Sep 17, 2021

View reviewed changes

hebasto merged commit ad47fb8 into bitcoin-core:master Sep 29, 2021

Sjors deleted the 2021/09/rpc_setting branch September 29, 2021 09:37

jonatack reviewed Sep 29, 2021

View reviewed changes

ryanofsky mentioned this pull request Sep 30, 2021

interfaces: Expose settings.json methods to GUI bitcoin/bitcoin#15936

Merged

luke-jr mentioned this pull request Oct 7, 2021

Restore "S" accelerator for "Start on system login" option #449

Merged

bitcoin-core locked as resolved and limited conversation to collaborators Sep 29, 2022

hebasto removed the Needs release notes label May 16, 2023

	<string>Accept command line and JSON-RPC commands.</string>
	<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>

Add RPC setting #416

Add RPC setting #416

Uh oh!

Conversation

Sjors commented Sep 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jarolrod commented Sep 2, 2021

Uh oh!

hebasto commented Sep 2, 2021

Uh oh!

jonatack commented Sep 2, 2021

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

kristapsk commented Sep 3, 2021

Uh oh!

hebasto commented Sep 3, 2021

Uh oh!

hebasto commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

jarolrod left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jarolrod Sep 3, 2021

Choose a reason for hiding this comment

Uh oh!

Sjors Sep 4, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Sep 4, 2021

Uh oh!

jarolrod commented Sep 6, 2021

Uh oh!

Uh oh!

Sjors commented Sep 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Sep 9, 2021

Uh oh!

shaavan left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Sep 12, 2021

Choose a reason for hiding this comment

Uh oh!

Sjors Sep 15, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto Sep 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Sep 16, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

shaavan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Sjors commented Sep 2, 2021 •

edited

Loading

hebasto commented Sep 3, 2021 •

edited

Loading

jarolrod left a comment •

edited

Loading

Sjors commented Sep 7, 2021 •

edited

Loading

hebasto Sep 15, 2021 •

edited

Loading

shaavan left a comment •

edited

Loading