gui icon indicating copy to clipboard operation
gui copied to clipboard

Toggle chain from menu (using Settings)

Open Sjors opened this issue 2 years ago • 35 comments

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.

Schermafbeelding 2021-09-01 om 13 01 37 (it's now under Settings, not File)

Sjors avatar Aug 31 '21 19:08 Sjors

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 avatar Aug 31 '21 20:08 ryanofsky

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.

ryanofsky avatar Aug 31 '21 20:08 ryanofsky

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 avatar Sep 01 '21 09:09 Sjors

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 avatar Sep 01 '21 11:09 Sjors

@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 avatar Sep 01 '21 16:09 Sjors

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?

Sjors avatar Sep 02 '21 09:09 Sjors

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

promag avatar Sep 03 '21 11:09 promag

@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

Sjors avatar Sep 04 '21 13:09 Sjors

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 avatar Sep 16 '21 10:09 Sjors

@kallewoof @ajtowns you might enjoy this PR

Sjors avatar Sep 16 '21 18:09 Sjors

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.

luke-jr avatar Sep 20 '21 20:09 luke-jr

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.

kallewoof avatar Sep 20 '21 22:09 kallewoof

Concept NACK, agree with @luke-jr

achow101 avatar Sep 20 '21 22:09 achow101

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.

jarolrod avatar Sep 21 '21 01:09 jarolrod

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 avatar Sep 21 '21 07:09 Sjors

Also note that in Windows it's already possible to launch with a different network: https://github.com/bitcoin-core/gui/issues/78#issuecomment-682673772

Sjors avatar Sep 21 '21 07:09 Sjors

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?

ryanofsky avatar Sep 30 '21 19:09 ryanofsky

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 avatar Oct 07 '21 08:10 Sjors

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.

kallewoof avatar Oct 07 '21 10:10 kallewoof

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

dlarchikov avatar Jan 05 '22 15:01 dlarchikov

Receiving this alert on MacOS

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

Process:               Bitcoin-Qt [46840]
Path:                  /Users/USER/*/Bitcoin-Qt.app/Contents/MacOS/Bitcoin-Qt
Identifier:            org.bitcoinfoundation.Bitcoin-Qt
Version:               22.99.0 (22.99.0)
Code Type:             X86-64 (Native)
Parent Process:        bash [79038]
Responsible:           Terminal [425]
User ID:               501

Date/Time:             2022-02-08 00:52:00.534 -0500
OS Version:            Mac OS X 10.15.7 (19H1615)
Report Version:        12
Anonymous UUID:        50406######################9E77F6

Sleep/Wake UUID:       E9EA##########################199E

Time Awake Since Boot: 96000 seconds
Time Since Wake:       95000 seconds

System Integrity Protection: disabled

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_CRASH (SIGABRT)
Exception Codes:       0x0000000000000000, 0x0000000000000000
Exception Note:        EXC_CORPSE_NOTIFY

Application Specific Information:
abort() called

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

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

RandyMcMillan avatar Feb 08 '22 05:02 RandyMcMillan

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 avatar Feb 08 '22 06:02 RandyMcMillan

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 avatar Feb 08 '22 07:02 RandyMcMillan

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

RandyMcMillan avatar Feb 08 '22 07:02 RandyMcMillan

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

Sjors avatar Feb 08 '22 09:02 Sjors

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

RandyMcMillan avatar Feb 08 '22 15:02 RandyMcMillan

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)

RandyMcMillan avatar Feb 08 '22 16:02 RandyMcMillan

Thanks! I fixed the duplicate "ClientClient" string.

Sjors avatar Feb 08 '22 17:02 Sjors

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

Tested on MacOS Arm64 - shutting down cleanly.

RandyMcMillan avatar Feb 09 '22 04:02 RandyMcMillan

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

DrahtBot avatar Mar 03 '22 20:03 DrahtBot