Toggle chain from menu (using Settings)

Sjors commented at 7:34 pm on August 31, 2021: member

Implements #78

It’s currently not easy for GUI users to switch to a test chain like signet. E.g on macOS this requires using the command line, since Spotlight doesn’t let you add an argument like -signet.

This PR makes it easier by adding a menu option to restart with a different chain. ~~It does this by adding a .chain_gui file. Unfortunately we can’t use settings.json, because this file is network specific. This file is ignored by bitcoind.~~ It does this by adding chain to the mainnet QSettings.

Sjors requested review from ryanofsky on Aug 31, 2021

ryanofsky commented at 8:00 pm on August 31, 2021: contributor

I think you could just use QSettings for this, the same way the GUI datadir preference and all other other GUI-only settings are stored in QSettings. Even https://github.com/bitcoin/bitcoin/pull/15936 which consolidates bitcoin-qt and bitcoind settings in settings.json still stores GUI-only settings in QSettings.

ryanofsky commented at 8:22 pm on August 31, 2021: contributor

I guess thinking more generally, for example if we did want to add a changenetwork RPC that would save a different network preference and restart, I’d probably do that by adding support for reading the -chain setting from the top level settings.json file, and then having bitcoind and bitcoin-qt both use this value at startup to choose the network and locate network-specific data and network-specific settings file from there. This would maybe be a little more complicated to implement than the QSettings or .chain_gui approaches, but probably not too bad.

I guess my preference would be to take this general approach and store the network preference in the chain field of the the top level settings.json. Or if that is too much work to implement, store it in QSettings. Or if that is not worth the effort either, keep this new .chain_gui file.

No matter which location is chosen, it could in theory be migrated somewhere else later, but migrating would be at least a little messy and could create edge cases when downgrading, so it is good to put a little effort into choosing the best location now.

Sjors commented at 9:37 am on September 1, 2021: member

If we’re keeping some QSettings around anyway, I might as well use that. With bitcoind it’s trivial to just specify the network as a parameter.

Sjors force-pushed on Sep 1, 2021

Sjors renamed this:
~~Toggle chain from menu (adds .chain_gui)~~
Toggle chain from menu (using Settings)
on Sep 1, 2021

Sjors marked this as ready for review on Sep 1, 2021

Sjors commented at 11:06 am on September 1, 2021: member

I switched to using QSettings. Similar to settings.json these are also network dependent. The GUI does this by changing its name during the bootstrap process, right after deciding which chain its on (step 7).

This PR uses the same trick in reverse: it changes the application name back to mainnet right before updating the chain setting, and then quits.

Sjors cross-referenced this on Sep 1, 2021 from issue Switch mainnet / testnet / signet / regtest from GUI by Sjors

shaavan commented at 2:52 pm on September 1, 2021: contributor

Concept ACK

This PR adds the functionality to select a network chain directly from the GUI, making it easier to toggle between the chains. This is achieved by adding a context menu option to set the -chain suboption to the selected chain type and closing the GUI. So that when next time GUI will start, the value of -chain suboption will by default be set to the last selected network by the user. This value, as I observed, can always be overridden while opening the bitcoin GUI through the command line.

Tested the PR on Ubuntu 20.04. The PR works as described by OP. I am adding the screenshot of the added context menu options.

Screenshot from 2021-09-01 19-42-53

This PR also has a byproduct benefit that is not discussed till now. For a person like me who mainly works on a signet network, I do not have to explicitly create an alias or select the signet chain each time I want to open GUI. So that’s great!

I have a few suggestions I want to put forward that might further improve the PR:

Before line 459:

0QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Confirm chain"),

Add translator’s comment for text in the StandardButton.

Add mnemonic shortcuts for the added context menu options.
It would be great if the GUI could automatically start after toggling the chain instead of simply closing the app.

Sjors commented at 4:14 pm on September 1, 2021: member

@ShaMan239 thanks for the review! I added a translator hint and keyboard shortcuts (I can’t test those on macOS, new screenshot is welcome). Unfortunately afaik Bitcoin Core can’t restart itself. This is a problem with some settings changes too.

Sjors force-pushed on Sep 1, 2021

shaavan commented at 6:00 pm on September 1, 2021: contributor

The Translator’s comments look good. Regarding the mnemonics shortcuts, there are some collisions between them.

Screenshot from 2021-09-01 23-11-52

Load PSBT from &clipboard <-> Switch &chain

~~&Backup wallet <-> &Bitcoin~~

Unfortunately, afaik Bitcoin Core can’t restart itself.

Well, that’s a bummer. I was just wondering if this feature could be added and would that even be worth adding.

Sjors commented at 9:19 am on September 2, 2021: member

Maybe I misunderstand how these shortcuts work… Wouldn’t “c” take you to the chain submenu and then “b” to Bitcoin inside that menu? Or all the shortcuts global?

in src/qt/bitcoingui.cpp:455 in 79af0d657b outdated

450+        const std::vector<std::pair<QString, QString>> chains = {{"main", "&Bitcoin"}, {"test", "&Testnet"}, {"regtest", "&Regtest"}, {"signet", "&Signet"}};
451+        for (auto chain : chains) {
452+            QAction* action = m_chain_selection_menu->addAction(chain.second);
453+
454+            if (gArgs.GetChainName() == chain.first.toStdString()) {
455+              action->setEnabled(false);

promag commented at 9:34 am on September 3, 2021:

nit, fix indentation heer and below.

promag commented at 9:35 am on September 3, 2021:

nit, could do action->setEnabled(gArgs.GetChainName() != chain.first.toStdString()).

in src/qt/bitcoingui.cpp:464 in 79af0d657b outdated

459+              //: Switch to the mainnet, testnet, signet or regtest chain.
460+              QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Switch chain"),
461+                  tr("Client restart required to switch chain.") + "<br><br>" + tr("Client will be shut down. Do you want to proceed?"),
462+                  QMessageBox::Yes | QMessageBox::Cancel, QMessageBox::Cancel);
463+
464+              if(btnRetVal == QMessageBox::Cancel) return;

promag commented at 9:40 am on September 3, 2021:

nit, space after if.

promag commented at 9:43 am on September 3, 2021: contributor

Approach ACK, makes sense to have this separate from other options.

hebasto added the label Feature on Sep 3, 2021

hebasto added the label UX on Sep 3, 2021

promag commented at 10:04 am on September 3, 2021: contributor

Tested ACK 79af0d657bc080bfd78b70a968c50f654c1aa329.

Also added the check on the current chain to better indicate why the current is disabled.

with the following

0+            const bool is_current = gArgs.GetChainName() == chain.first.toStdString();
1             QAction* action = m_chain_selection_menu->addAction(chain.second);
2-
3-            if (gArgs.GetChainName() == chain.first.toStdString()) {
4-              action->setEnabled(false);
5-            }
6+            action->setCheckable(true);
7+            action->setChecked(is_current);
8+            action->setEnabled(!is_current);

promag commented at 11:08 am on September 3, 2021: contributor

Now I can’t pick a different chain from the command line, for instance src/qt/bitcoin-qt -regtest gives Error: Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one..

in src/qt/bitcoingui.cpp:459 in 79af0d657b outdated

454+            if (gArgs.GetChainName() == chain.first.toStdString()) {
455+              action->setEnabled(false);
456+            }
457+
458+            connect(action, &QAction::triggered, [this, chain] {
459+              //: Switch to the mainnet, testnet, signet or regtest chain.

jarolrod commented at 9:26 pm on September 3, 2021:

@hebasto: “Why not add translator comments to all of the added tr() arguments?”

Noting down some guidance for translator comments here and for the future:

Translator comments should attempt to provide a translator with the appropriate context to aid them with translating a certain string.

Proper context is:

Where is this string shown?
- e.g; Shown in the Create Wallet dialog, Shown in file menu options …
What action is associated with this string?
- If the string is associated with a button, what is the outcome of pressing the button

Optional context is:

Explain what the string is meant to convey
- This is an explanatory text shown after X that aims to inform the user that …
Provide alternatives to complicated/technical words
- Some words may not have a direct one-to-one translation in certain languages. It is nice to provide a replacement for such a word. For example, some languages may not have a 1-1 translation for the word subnet, an appropriate replacement for this word can be IP address.

This context makes the job of translators easier and allows for more effective translations.

Sjors force-pushed on Sep 4, 2021

Sjors commented at 1:11 pm on September 4, 2021: member

@promag thanks, addressed comments and added the checkmark. Though on my machine I can’t see the checkmark (QT 6.1.2 via homebrew).

Once you use the GUI to select a chain, there’s no way back :-) Actually you can still use -chain=regtest. The error message is a bit confusing, it’s just that you have to use either -chain= or use one of -regtest, -signet, -testnet, -main

The chain is a special case with so many weird rules, I’d rather not mess with:

https://github.com/bitcoin-core/gui/blob/f4e12fd50c23875f4b5f272c94449eb81de43d5d/src/util/settings.cpp#L129-L143

promag commented at 10:23 am on September 5, 2021: contributor

Code review ACK 246d0658b41b232151567356694c8580f32a800e.

in src/qt/bitcoin.cpp:564 in 246d0658b4 outdated

558@@ -559,6 +559,10 @@ int GuiMain(int argc, char* argv[])
559     // - QSettings() will use the new application name after this, resulting in network-specific settings
560     // - Needs to be done before createOptionsModel
561 
562+    QSettings settings;
563+    const std::string chain = settings.value("chain", "").toString().toStdString();
564+    if (chain != "") gArgs.SoftSetArg("-chain", chain);

hebasto commented at 11:05 am on September 8, 2021:

0    if (!chain.empty()) gArgs.SoftSetArg("-chain", chain);

in src/qt/bitcoingui.cpp:460 in 246d0658b4 outdated

455+            action->setChecked(is_current);
456+            action->setEnabled(!is_current);
457+
458+            connect(action, &QAction::triggered, [this, chain] {
459+                //: Switch to the mainnet, testnet, signet or regtest chain.
460+                QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Switch chain"),

hebasto commented at 11:31 am on September 8, 2021:

Could follow our naming convention with btnRetVal?

in src/qt/bitcoingui.cpp:461 in 246d0658b4 outdated

456+            action->setEnabled(!is_current);
457+
458+            connect(action, &QAction::triggered, [this, chain] {
459+                //: Switch to the mainnet, testnet, signet or regtest chain.
460+                QMessageBox::StandardButton btnRetVal = QMessageBox::question(this, tr("Switch chain"),
461+                    tr("Client restart required to switch chain.") + "<br><br>" + tr("Client will be shut down. Do you want to proceed?"),

hebasto commented at 11:35 am on September 8, 2021:

Translators will much appreciate if both strings belong to the same context:

0                    tr("Client restart required to switch chain.\n\nClient will be shut down. Do you want to proceed?"),

Sjors commented at 10:52 am on September 16, 2021:

Done

hebasto commented at 11:40 am on September 8, 2021: member

Concept ACK.

Still having this error on Linux:

Now I can’t pick a different chain from the command line, for instance src/qt/bitcoin-qt -regtest gives Error: Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one..

UPDATE:

Once you use the GUI to select a chain, there’s no way back :-)

Maybe always add the chain value to settings at the first run the new code, and notify users about new behavior?

END OF UPDATE

It seems possible to move all new code from BitcoinGUI::createActions() to BitcoinGUI::createMenuBar() and use the local variable chain_selection_action instead of the data member m_chain_selection_action.
Why not add translator comments to all of the added tr() arguments?

shaavan commented at 2:41 pm on September 11, 2021: contributor

Changes since my last review:

The checkmark is added along with the options to emphasize which chain is currently selected. I think it makes sense to add a checkmark, so I agree with this change Adding screenshots of the visual change before and after updating PR.

Before Update	After Update

Maybe I misunderstand how these shortcuts work… Wouldn’t “c” take you to the chain submenu and then “b” to Bitcoin inside that menu? Or all the shortcuts global?

I just checked if the shortcuts are global. They are not, and so I have updated my comment accordingly. But there is still the collision of mnemonics between Load PSBT from &clipboard and Switch &chain as they are part of the same menu. So I would recommend updating the mnemonics shortcut to &S

 0diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp
 1index 39a58f4d1..5aedbbf43 100644
 2--- a/src/qt/bitcoingui.cpp
 3+++ b/src/qt/bitcoingui.cpp
 4@@ -441,7 +441,7 @@ void BitcoinGUI::createActions()
 5     }
 6 #endif // ENABLE_WALLET
 7 
 8-    m_chain_selection_action = new QAction(tr("Switch &chain"), this);
 9+    m_chain_selection_action = new QAction(tr("&Switch chain"), this);
10     m_chain_selection_action->setStatusTip(tr("Restart application using a different (test) network"));
11     m_chain_selection_menu = new QMenu(this);

Sjors commented at 10:57 am on September 16, 2021: member

Maybe always add the chain value to settings at the first run the new code, and notify users about new behavior?

Done, and added a release note.

I moved the menu stuff to BitcoinGUI::createMenuBar()

Why not add translator comments to all of the added tr() arguments?

Done (I think).

I switched the shortcut to Switch &chain

Sjors force-pushed on Sep 16, 2021

shaavan approved

shaavan commented at 1:10 pm on September 16, 2021: contributor

tACK bc72b19 Tested on Ubuntu 20.04 (Using Qt version 5.12.8)

Updates since my last review:

Release notes are added to inform users about the change in behavior in the way of using chain variables.
GUI will open on the mainnet when the user specifies no specific chain, either through Command-Line or GUI.
The scope of the chain_selction_menu variable has been changed from global to local. btnRetVal renamed to btn_ret_val according to the naming convention.
The mnemonic shortcut of the switch chain option has been changed from &c to &S, removing shortcut collision.

I tested the updated PR on Ubuntu 20.04. The PR works perfectly. Also, I followed the release notes word by word while testing. They are correct too. The collision between mnemonic shortcuts has been resolved, and the new mnemonic shortcut is working perfectly.

I am adding the screenshot of the latest commit.

Thanks for this amazing work, @Sjors!

Sjors commented at 6:27 pm on September 16, 2021: member

@kallewoof @ajtowns you might enjoy this PR

luke-jr commented at 8:05 pm on September 20, 2021: member

Concept NACK. I think there should be a very firm line between Bitcoin and test networks. Something like this can too easily lead to loss because someone forgot they switched back to mainnet.

kallewoof commented at 10:21 pm on September 20, 2021: member

Slightly agree with @luke-jr but I can see why it would be nice to have. Just not sure how often you switch between networks like that.

Maybe this should be a default-disabled feature that developers can turn on when compiling.

achow101 commented at 10:25 pm on September 20, 2021: member

Concept NACK, agree with @luke-jr

jarolrod commented at 1:28 am on September 21, 2021: member

After further consideration, I agree with @luke-jr. While this is a nice-to-have, without including several new UI/UX changes to prevent a user from footgunning; there’s a possibility that this can do harm.

To be clear, I’m concept ACK on the general Idea, I think there’s room for a feature like this. But, this should be done with careful attention and perhaps safeguarded, as mentioned by @kallewoof, behind an options menu-setting.

Sjors commented at 7:42 am on September 21, 2021: member

Something like this can too easily lead to loss because someone forgot they switched back to mainnet.

How? The addresses are different and incompatible. It seems to me that losing funds would require deception, not just forgetting something.

I’m happy to move this into the options dialog, but I don’t see how that helps in the deception scenario. For that it might be more useful to add a stern warning.

Maybe this should be a default-disabled feature that developers can turn on when compiling.

The purpose of this PR is to make it practical for GUI users to use signet and testnet. Anyone who can self-compile can also just open a terminal and enter /Applications/Bitcoin\ Core.app/Contents/MacOS/Bitcoin-Qt -signet.

Being able to use signet and testnet lets users practice with (multisig) wallets without risking real funds. So you lose some safety in one place and gain it in another.

Sjors commented at 7:44 am on September 21, 2021: member

Also note that in Windows it’s already possible to launch with a different network: #78 (comment)

ryanofsky commented at 7:28 pm on September 30, 2021: contributor

This seems harmless, but why is not possible to have different icons on macos like we can have on windows or linux? Is this an operating system limitation? An installer limitation? Also why is it better to shut down the mainnet instance when you want to start a testnet instance? Would you want to shut down your main wallet while experimenting and trying something in your test wallet?

Mainnet and testnet just being separate apps with separate icons that can be started and stopped independently would seem like the ideal thing. How does Microsoft Office work on macos? Can you open Word and Powerpoint at the same time, or is there a menu where you switch from Word mode to Powerpoint mode?

DrahtBot added the label Needs rebase on Oct 3, 2021

Sjors commented at 8:28 am on October 7, 2021: member

possible to have different icons on macos like we can have on windows or linux?

I briefly looked into this. It wasn’t obvious how to do it with the custom DMG generator script(s) we have. But it might be possible. However, macOS apps are installed through a drag and drop process, so we’d have to show 4 icons during the install phase. That seems potentially confusing. Another option would be to make them completely separate downloads, also confusing, and a bunch of extra guix signing.

We could completely revamp the installer into a wizard-like system that moves the binaries itself, rather than drag and drop. One advantage of that is that we can also put bitcoind and bitcoin-cli and the wallet tool in the right places (which are absent now). But that’s a big change.

Also why is it better to shut down the mainnet instance when you want to start a testnet instance?

This is a side effect of the approach I use, by putting the network in the global settings you can’t easily run multiple instances. Separate icons would solve that, but create new problems as I described above.

Another approach could be to always launch in mainnet, and keep that open, but allow the application to launch another instance of itself with e.g. -testnet. But launching apps from within an app might be a bit scary too.

Sjors force-pushed on Oct 7, 2021

in doc/release-notes.md:136 in c6a73850f7 outdated

130@@ -131,6 +131,10 @@ GUI changes
131 - UTXOs which are locked via the GUI are now stored persistently in the
132   wallet database, so are not lost on node shutdown or crash. (#23065)
133 
134+- Toggle chain from mainnet to testnet, signet or regtest. The GUI now always sets `-chain`,
135+  so any occurrences of `-testnet`, `-signet` or `-regtest` in `bitcoin.conf` should
136+  be replace with `chain=test`, `chain=signet` or `chain=regtest` respectively. (#414)

Sjors commented at 8:35 am on October 7, 2021:

Should I prefix the GitHub issue with bitcoin-core/gui#414?

hebasto commented at 8:49 am on October 7, 2021:

I guess so.

Sjors commented at 8:56 am on October 7, 2021:

Ok, done for for release notes. Not sure how to test the end result.

Sjors force-pushed on Oct 7, 2021

DrahtBot removed the label Needs rebase on Oct 7, 2021

kallewoof commented at 10:29 am on October 7, 2021: member

Another approach could be to always launch in mainnet, and keep that open, but allow the application to launch another instance of itself with e.g. -testnet. But launching apps from within an app might be a bit scary too.

That sounds like it could potentially be a really confusing eventual .5 TB loss of disk space for users who think “regtest” means “regtest” and not “regtest with a slice of main net syncing in the background”..

The “document” approach seems sensible, i.e. where you “open” instances (main/test/sig/reg/custom) from within the application. Could be potentially really useful if you are doing multiple regtests and like doing things in the GUI.

dlarchikov commented at 3:00 pm on January 5, 2022: none

@Sjors thx for PR. Please add support of custom private rpc for regtest.

RandyMcMillan commented at 5:55 am on February 8, 2022: contributor

Receiving this alert on MacOS

Screen Shot 2022-02-08 at 12 44 02 AM

 0Process:               Bitcoin-Qt [46840]
 1Path:                  /Users/USER/*/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt
 2Identifier:            org.bitcoinfoundation.Bitcoin-Qt
 3Version:               22.99.0 (22.99.0)
 4Code Type:             X86-64 (Native)
 5Parent Process:        bash [79038]
 6Responsible:           Terminal [425]
 7User ID:               501
 8
 9Date/Time:             2022-02-08 00:52:00.534 -0500
10OS Version:            Mac OS X 10.15.7 (19H1615)
11Report Version:        12
12Anonymous UUID:        50406######################9E77F6
13
14Sleep/Wake UUID:       E9EA##########################199E
15
16Time Awake Since Boot: 96000 seconds
17Time Since Wake:       95000 seconds
18
19System Integrity Protection: disabled
20
21Crashed Thread:        0  Dispatch queue: com.apple.main-thread
22
23Exception Type:        EXC_CRASH (SIGABRT)
24Exception Codes:       0x0000000000000000, 0x0000000000000000
25Exception Note:        EXC_CORPSE_NOTIFY
26
27Application Specific Information:
28abort() called

Screen Shot 2022-02-08 at 12 55 14 AM

Cirrus: https://cirrus-ci.com/build/6466679780671488

RandyMcMillan commented at 6:00 am on February 8, 2022: contributor

Running Cirrus on PR: Cirrus: https://cirrus-ci.com/build/6466679780671488

Running Cirrus on the rebased branch: Rebase: https://cirrus-ci.com/build/4721486761033728

RandyMcMillan commented at 7:41 am on February 8, 2022: contributor

It seems that Switch chain belongs in the Settings menu: https://github.com/RandyMcMillan/gui/commit/9b1456cf59e4ee41a6a9ab4985b4716f231163fc https://cirrus-ci.com/build/4848117060206592

Screen Shot 2022-02-08 at 2 30 11 AM

RandyMcMillan commented at 7:47 am on February 8, 2022: contributor

On a side note:

It could be argued that “FIle” is incorrect since mostly wallet operations are listed under this menu. The wallet related functions could be grouped together - with the other functions in a sub menu group. File could be changed to Wallet

Screen Shot 2022-02-08 at 2 38 13 AM

Sjors force-pushed on Feb 8, 2022

Sjors commented at 9:53 am on February 8, 2022: member

@RandyMcMillan thanks for testing! That crash is worrying. I’m getting this too now some of the time. Inspired by #336 I changed QApplication::quit(); to Q_EMIT quitRequested();. That seems to fix it.

I also moved the menu to Settings. And rebased. @dlarchikov:

Please add support of custom private rpc for regtest.

I’m not sure what you mean by this.

RandyMcMillan commented at 3:37 pm on February 8, 2022: contributor

I will retest shortly - this sounds like a much friendlier method of shutdown…

RandyMcMillan commented at 4:49 pm on February 8, 2022: contributor

Approach ACK f3ddafa

MacOS (x86_64) is now receiving a gentle shutdown signal - removing the unexpected shutdown alert. This should be allowing the data directory to be in a good state upon restarting.

I will test MacOS ARM64 shortly.

Great Work @Sjors !

Screen Shot 2022-02-08 at 11 37 36 AM (3)

Screen Shot 2022-02-08 at 11 37 40 AM 1(3)

gui: toggle chain via menu

The selected chain is stored in the QSettings for mainnet.

df3ad9a68f

Sjors force-pushed on Feb 8, 2022

Sjors commented at 5:14 pm on February 8, 2022: member

Thanks! I fixed the duplicate “ClientClient” string.

RandyMcMillan cross-referenced this on Feb 8, 2022 from issue Indicate network in the Node window title - similar to the main window by RandyMcMillan

RandyMcMillan commented at 4:39 am on February 9, 2022: contributor

reACK Approach https://github.com/bitcoin-core/gui/commit/df3ad9a68ff84c01f62bfb49766da890f4be2c82

Tested on MacOS Arm64 - shutting down cleanly.

in src/qt/bitcoingui.cpp:478 in df3ad9a68f

473+
474+            connect(action, &QAction::triggered, [this, chain] {
475+                //: Switch to the mainnet, testnet, signet or regtest chain.
476+                QMessageBox::StandardButton btn_ret_val = QMessageBox::question(this, tr("Switch chain"),
477+                    //: Switching between mainnet, testnet, signet or regtest chain requires a restart.
478+                    tr("Client restart required to switch chain.\n\nClient will be shut down. Do you want to proceed?"),

ryanofsky commented at 1:08 am on February 26, 2022:

In commit “gui: toggle chain via menu” (12431e0d95157b4df18912b38735acdc927c9762)

I think it would be good for the dialog to describe what the option actually does. I’m concerned that someone could click this without really understanding it, and then next time they start bitcoin they find their wallets are missing and bitcoin is doing a new sync to a different network.

I think ideally dialog would say something like. “Are you sure you want to switch networks from main to test? Testnet coins are separate from actual bitcoins, and are not supposed to have any value. Switching networks requires restarting Bitcoin Core, and any existing wallets from the current network will be hidden while using the other network. It will be possible to switch back to the current network by using the “Switch chain” menu again and restarting.” It could also be good to change the affirmative button text from “Yes” to something more explicit like “Switch to {network name} network and exit.”

in src/qt/bitcoin.cpp:569 in df3ad9a68f

563@@ -564,6 +564,10 @@ int GuiMain(int argc, char* argv[])
564     // - QSettings() will use the new application name after this, resulting in network-specific settings
565     // - Needs to be done before createOptionsModel
566 
567+    QSettings settings;
568+    std::string chain = settings.value("chain", "main").toString().toStdString();
569+    gArgs.SoftSetArg("-chain", chain);

ryanofsky commented at 1:20 am on February 26, 2022:

In commit “gui: toggle chain via menu” (12431e0d95157b4df18912b38735acdc927c9762)

It might be good to show a message on startup the first time bitcoin is started after switching networks.

The toggle chain menu could set a boolean “switched_chain” QSetting, and this code could check for the “switched_chain” setting, clear it, and show a message like “Bitcoin Core is connecting to new network {network}. Use the “Switch chain” menu to switch networks again.” (It could be a dialog with a “Do not show this message in the future checkbox” if users switch network repeatedly.) But I think this would provide a useful alert and a good second chance to recover if someone clicked through the first dialog not expecting bitcoin to be starting from scratch again.

ryanofsky commented at 7:18 pm on March 10, 2022:

re: #414 (review)

It might be good to show a message on startup the first time bitcoin is started after switching networks. […]

Replying to myself, maybe a better startup message would be: “Bitcoin-Qt configuration has changed and will now connect to the {Testnet} network instead of the {Mainnet} network. {Mainnet} wallets will not be loaded while connected to the {Testnet} network. Also {Testnet} coins are separate from actual bitcoins, and are not supposed to have any value.” Dialog could include a checkbox “Do not show this message again,” and buttons “Continue connecting to {Testnet} network” / “Exit and switch back to {Mainnet network}” for a safe escape.

(And again I really think ideally none of these dialogs would be necessary and new networks would just open in new windows without closing existing ones. But showing clear messages could at least avoid pitfalls if only one window can be open at a time)

in doc/release-notes.md:195 in df3ad9a68f

189@@ -190,6 +190,10 @@ GUI changes
190 - UTXOs which are locked via the GUI are now stored persistently in the
191   wallet database, so are not lost on node shutdown or crash. (#23065)
192 
193+- Toggle chain from mainnet to testnet, signet or regtest. The GUI now always sets `-chain`,
194+  so any occurrences of `-testnet`, `-signet` or `-regtest` in `bitcoin.conf` should
195+  be replace with `chain=test`, `chain=signet` or `chain=regtest` respectively. (bitcoin-core/gui#414)

ryanofsky commented at 1:26 am on February 26, 2022:

In commit “gui: toggle chain via menu” (12431e0d95157b4df18912b38735acdc927c9762)

I think it would be better if these notes were clearer about what behavior is. What happens if you don’t follow the advice? Is this saying if you run use -testnet the parameter will be silently ignored because the GUI overrides it internally?

If so, I think this might rise to the level of a bug because command line arguments by design are supposed to override GUI settings and other configuration sources. If not, and there is an explicit error about conflicting settings, that is probably ok. But I think ideally GUI setting would still be overridden if -chain or -testnet or -signet or -regtest were specified. It seems like it should be trivial to implement that instead of requiring users to read this and change their setups.

in src/qt/bitcoingui.cpp:466 in df3ad9a68f

461+    chain_selection_action->setStatusTip(tr("Restart application using a different (test) network"));
462+    chain_selection_menu = new QMenu(this);
463+
464+    connect(chain_selection_menu, &QMenu::aboutToShow, [this, chain_selection_menu] {
465+        chain_selection_menu->clear();
466+        const std::vector<std::pair<QString, QString>> chains = {{"main", "&Bitcoin"}, {"test", "&Testnet"}, {"regtest", "&Regtest"}, {"signet", "&Signet"}};

ryanofsky commented at 5:01 pm on February 28, 2022:

In commit “gui: toggle chain via menu” (12431e0d95157b4df18912b38735acdc927c9762)

Interesting the text is is “Bitcoin” instead of “Mainnet”. I think that it is good because it implies the other networks are not the Bitcoin network. Another way to do this while being a little more consistent could be to make the choices “Mainnet (Bitcoin)” / “Testnet” / “Regtest” / “Signet”

RandyMcMillan commented at 7:40 pm on March 12, 2022:

I noted this as well - IMO “Bitcoin” is a better way to go - “Mainnet” may actually confuse new users at this point.

ryanofsky approved

ryanofsky commented at 6:00 pm on February 28, 2022: contributor

I have reservations about this feature, and also think the implementation could be improved, but code review ACK 12431e0d95157b4df18912b38735acdc927c9762 since I don’t think there are problems with the code.

Code review comments below suggest ways to improve the implementation, and I also agree with luke-jr (https://github.com/bitcoin-core/gui/pull/414#issuecomment-923255521) and kalle and achow and jarolrod that in general this change could result in confusion and be a net negative. I mostly just don’t think it makes sense from a user perspective that you are forced to shut down your connection and wallets for one network to connect to a different network, instead of just opening different networks in different windows. There’s no problem doing this on Linux or Windows (IIUC from #78 (comment)) because Linux and Windows packages can provide separate icons for launching testnet and signet instances. Ideally the mac package would just provide separate icons as well so we wouldn’t need this less flexible and potentially confusing toggle.

I don’t see a technical reason why providing separate icons shouldn’t be possible on mac. This guide about creating DMGs https://intmaker.com/deploy-to-macos/ has a section “Level 3: additional applications” about adding helper applications alongside the main application. In this case, helper applications could be Bitcoin-Qt (testnet) and Bitcoin-Qt (signet) scripts that call the main Bitcoin-Qt application with appropriate command line arguments. https://superuser.com/a/16777 shows how to write applescripts that invoke applications with command lines and https://apple.stackexchange.com/a/407885 shows how to package scripts as applications with icons.

Another approach would be to replace the “Switch Chain” toggle menu with an “Open Chain” menu, which would just open new windows connecting to other networks, instead of closing down the whole application and restarting. The implementation could still use QSettings to remember what windows are open so relaunching bitcoin will reopen the same windows (Or if every window was manually closed by the user, reopen the last closed window).

DrahtBot added the label Needs rebase on Mar 3, 2022

DrahtBot commented at 8:42 pm on March 3, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

Sjors commented at 6:51 pm on March 10, 2022: member

@ryanofsky thanks for all the comments, I’m a bit behind on things, but will get to them.

ryanofsky approved

ryanofsky commented at 7:31 pm on March 10, 2022: contributor

re: #414 (comment)

@ryanofsky thanks for all the comments, I’m a bit behind on things, but will get to them.

:+1:

Even though I suggested a lot of changes, I think this PR is ok in its current form. The other reviewers who objected to this PR previously should speak up if they still have issues with it, or if they think any changes would make it better. Sjors has already put a good amount of work into this, and some of the changes I suggested would require even more effort. So it’d be good to sync up and make sure effort is not wasted.

RandyMcMillan commented at 7:59 pm on March 12, 2022: contributor

Concept reACK

If a custom signet is detected - It would be great to add a truncated “finger print” of the active signet_challenge - I have experimented with adding a signet challenge to the window title and menu and the string is simply too long - but a “finger print” will work in the window title. Maybe use a finger print with 8 chars?

examples:

BitcoinCore - [signet] [stuvwxyz] or
BitcoinCore - [signet] […stuvwxyz] or
BitcoinCore - [signet] [abcd…wxyz]

Just an idea…but if this was added - option 3 seems best IMO.

luke-jr commented at 0:39 am on March 13, 2022: member

Concept re-NACK.

How about for macOS only, a menu item that simply launches a separate instance on a test network?

stickies-v cross-referenced this on Mar 29, 2022 from issue v23.0 testing by laanwj

hebasto commented at 1:18 pm on July 19, 2022: member

What is the status of this PR?

Sjors commented at 1:31 pm on July 19, 2022: member

@hebasto still on my list to look into some of the other approaches suggested above. But if anyone else wants to do it sooner, don’t hesitate!

How about for macOS only, a menu item that simply launches a separate instance on a test network?

That would be fine by me, not sure if it requires adding dependencies to launch an application.

Sjors cross-referenced this on Oct 20, 2022 from issue Add Signet launch shortcut for Windows by Sjors

hebasto commented at 3:48 pm on October 26, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

But if anyone else wants to do it sooner, don’t hesitate!

Will be labeled “Up for grabs”.

hebasto closed this on Oct 26, 2022

hebasto added the label Up for grabs on Oct 26, 2022

bitcoin-core locked this on Oct 26, 2023

Toggle chain from menu (using Settings) #414